Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Message timer, autoDismiss, not affecting the duration of message display time #878

Closed
EricE147 opened this issue Mar 29, 2023 · 4 comments
Labels
p-low-priority t-bug The issue is related to an aspect of the software which is incorrect
Milestone

Comments

@EricE147
Copy link
Contributor

Describe the bug

Found a bug with showSuccessNotification and showErrorNotification where the autoDismiss parameter seems to not be considered. It didn't matter if we set autoDismiss to 2 seconds or 2000 seconds the browser loads the next page within a few seconds not allowing the success and error notification to be read or copied. There may be a fixed autoDismiss value deeper into the call stack.

To Reproduce

Steps to reproduce the behavior:

  1. While running the website locally, sign in using the test login information.
  2. Click on the conversions tab and attempt to edit, create, or delete a conversion,
  3. In the top right corner a success message should be displayed briefly as the page reloads.
  4. Regardless of how long autoDismiss is set in Notifications.ts or as a parameter for showSuccessNotification and showErrorNotification the message will only display for a short time.

Expected behavior

The success and error notifications should display for the allotted time before loading the next page.

Screenshots

image

Machine info (please complete the following information)

  • OS: Windows
  • Browser & Version: Chrome Version 111.0.5563.111
  • OED Version: v0.8.0

Additional context

Related to issue #869 and found when doing Pull Request #877.

@huss huss added t-bug The issue is related to an aspect of the software which is incorrect p-medium-priority labels Mar 31, 2023
@huss
Copy link
Member

huss commented Mar 31, 2023

Thank you for raising this issue. I believe it began because of my suggestion to a developer to reload OED to reset the state after certain changes as an admin. This is being done via window.location.reload which causes the following:

  1. All the code in the web browser bundle is reloaded from scratch.
  2. The logo and other misc. actions occur.
  3. The Redux state is reset to the initialization state.
  4. Something else I'm not thinking of right now.

Of these actions, only the third one of resetting the Redux state is actually needed in these cases. The basic issue is that certain changes by admins can cause the state to be invalidated. For example, in the case of conversions that was the immediate issue here, changing a conversion can mean the Cik values are recalculated which can change the unit index. Thus, the values in the Redux state are no longer in sync with what the sever will return. Other changes can make a selected meter or group become ungraphable, e.g., the unit of a meter is changed so it is no longer compatible with other meters/groups that are selected. Given the complexity, the decision was made to wipe the entire state. This is also acceptable because these types of changes do not happen very often and only to an admin and not a general user. In some of the cases, OED limits when the reload happens to cases where it is needed by the change. For example, changing the note on an item is safe so no reload happens. The first item, reloading the web browser code, is believed to be the cause of the message being lost since that executing code is wiped out.

The following is a list of where this is known to be an issue:

  • src/client/app/actions/admin.ts in updateCikAndDBViewsIfNeeded.
  • The upcoming group admin page also does this. The code is not yet deployed to OED.
  • Meters would also do it after it is properly set up.
  • There may be more places.

Doing a reload was easy but does too much. I think we should look into resetting the Redux state. We could try to figure out which state needs resetting but, at least as a first fix, we can reset (almost) all of it. Then we can see if it fixes this issue and consider limiting the state reset. We may find that some state reset does not lead to all pages being redrawn by React. These would need to be fixed up if this is the case. I don't think we have ever tested this as it only happened when the code was reloaded. However, I'm hopeful that it will (mostly) work given the pages properly redraw on many state changes.

These are my thoughts and those of others are welcome/encouraged.

@huss
Copy link
Member

huss commented Jan 5, 2024

Issue #901 may fix this issue since the reloads may all go away.

@huss
Copy link
Member

huss commented Mar 24, 2024

After recent updates, only src/client/app/components/csv/ReadingsCSVUploadComponent.tsx still has this issue with a reload. It has a TODO to remove when React hooks are done (issue #887).

@huss huss added this to the 1.x milestone Mar 24, 2024
@huss
Copy link
Member

huss commented May 31, 2024

Only the CSV page does a reload and it is currently being converted to the Redux Toolkit. Once done, all messages will stay up as expected. Thus, I am closing this issue.

@huss huss closed this as completed May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-low-priority t-bug The issue is related to an aspect of the software which is incorrect
Projects
None yet
Development

No branches or pull requests

2 participants