-
Notifications
You must be signed in to change notification settings - Fork 456
Issue869 - Implemented Failure/Success Notifications to Edit/Create pages #1566
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
base: development
Are you sure you want to change the base?
Conversation
…exclude maps and admin), added failure/success notifications to the delete feature on edit pages, slightly modified data.ts to include more messages that were needed for some notifications, added TODO DEBUG used to test failure
|
Below are the results of the failure/success notification in case if it helps to compare with for initial testing. CreateUnitModalComponent.tsx EditUnitModalComponent.tsx PreferencesComponent.tsx NOTE: Left this one out, because I was unable to figure out how to test the failure notifications. CreateMeterModalComponent.tsx EditMeterModalComponent.tsx CreateUserModalComponent.tsx NOTE: The failure message from the Promise (the forced failure notification) is generic, but in actual use cases, the real error message should still appear. However, I am unable to figure out how to test the failure naturally as using the Promise was my work around. EditUserModalComponent.tsx ReadingsCSVUploadComponent.tsx Failure Message: NOTE: Tested Failure by using the provided sampleReadings.csv. MetersCSVUploadComponent.tsx Failure Message: NOTE: Tested Failure by using threeYearA.csv CreateGroupModalComponent.tsx NOTE: Most likely will have to change what values get displayed on the notification. The only required. EditGroupModalComponent.tsx CreateConversionModalComponent.tsx EditConversionModalComponent.tsx NOTE: I have not checked whether or not the notifications are applied elsewhere such as deleting in the edit pages. DELETE MESSAGES EditConversionModalComponent.tsx EditGroupModalComponent.tsx NOTE: data.ts does not have group.delete.success or group.delete.failure EditUserModalComponent.tsx EditMeterModalComponent.tsx NOTE: No delete capabilities currently in EditMeterModalComponent.tsx. EditUnitModalComponent.tsx |
|
While working on this PR I became confused by the file src/client/app/redux/actions/conversions.ts. After some checking, it appears it is not used at all. This is also consistent with the fact that webpack does not rebuild the client when it is changed. I tried deleting it and everything still worked fine. I believe src/client/app/redux/api/conversionsApi.ts is the newer way with the RTK that has now completely replaced that file. @aduques if you agree then it can be removed as part of this PR even though it is only indirectly related. |
huss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to thank @aduques for another contribution to OED. I'm sorry it took a little while to look this over. Overall, it seems good. You noted it is a WIP so you can, if you prefer, convert this PR to a draft until you think it is more complete. I made some comments for you to consider. Please let me know if anything is not clear or I can help in any way.
| "group.create.nounit": "The default graphic unit was changed to no unit from ", | ||
| "group.delete.group": "Delete Group", | ||
| "group.delete.issue": "is contained in the following groups and cannot be deleted", | ||
| "group.delete.success": "Successfully deleted group.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The translation key needs to be in all three languages and it is only in en here. Please copy both of the new keys into the other languages. You don't need to do the translation and can put \u{26A1} at the end of the value/translation string to indicate it needs translation.
| }) | ||
| .catch(err => { | ||
| showErrorNotification( | ||
| translate('group.failed.to.create.group') + '"' + err.data + '"'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message seems to be about groups and not a failed adding of a conversion.
| .then(() => { | ||
| showSuccessNotification( | ||
| translate('conversion.successfully.create.conversion') + | ||
| ' (sourceID: ' + conversionState.sourceId + ', destinationID: ' + conversionState.destinationId + ')' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a general comment. OED should not show ids on messages. I think I understand that you wanted to help a developer with debugging but the real usage is for an admin at an OED site. The id is something internal to OED that an admin does not need and should not see. If a developer needs this information then they should use a debugger and/or temporary console statements. If more information is to be added then one could put in the name of the source/destination in a similar fashion to what you did in other places.
| // Omit the source options , do not need to send in request so remove here. | ||
| // If source is a meter, make bidirectional false | ||
| // If source or destination is a suffix unit, make bidirectional false | ||
| addConversionMutation({...omit(conversionState, 'sourceOptions'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot add a comment above so am doing it here. handleWarningConfirm also does an add of the conversion when a warning is done first. It needs to deal with the error message.
The older code did the error checks and success/failure popups in the actions file but that is now gone with the RTK. I don't know if the new API files could easily be changed to do this so it is centralized so any conversion add (and for other ones too) would always show the message no matter where it is called. I think this is outside this PR but something OED could look into sometime in the future.
| Button, Col, Container, FormFeedback, FormGroup, Input, Label, Modal, | ||
| ModalBody, ModalFooter, ModalHeader, Row | ||
| } from 'reactstrap'; | ||
| // TODO DEBUG: added in to test the showErrorNotification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear on this comment on this being for debug as the include(s) that existed before your changes.
| .unwrap() | ||
| .then(() => { | ||
| showSuccessNotification( | ||
| translate('group.successfully.edited.group') + ' "' + submitState.name + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see create group for comments.
| showSuccessNotification( | ||
| translate('meter.successfully.create.meter') + '"' + submitState.name + | ||
| '" (identifier: ' + submitState.identifier + ', type: ' + submitState.meterType + ')' | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot put a comment below so it is here. I wonder if the error msg should also include the meter name (maybe other values?) to be sure it is identified in case the err does not have it.
| showSuccessNotification( | ||
| translate('meter.successfully.edited.meter') + '"' + submitState.name + | ||
| '" (identifier: ' + submitState.identifier + ', type: ' + submitState.meterType + ')' | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see create meter for comments.
| showErrorNotification(translate('unit.failed.to.create.unit')); | ||
| .catch(err => { | ||
| showErrorNotification( | ||
| translate('unit.failed.to.create.unit') + '"' + err.data + '"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I marked them all but see comment on more info on error and please make them all consistent to whatever is done.
| handleClose(); | ||
| }} | ||
| onCancel={() => setShowUnsavedWarning(false)} | ||
| // TODO DEBUG: Disabled is always set to false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine. I noticed you can limit to only changing one place for testing if you modify the setCanSave to be true above.
Description
These changes address Issue #869. These changes aim to reimplement the missing success and failure notifications across all of the available Edit/Create pages (excluding Maps and Admin Settings). The additions of these notifications do not intend to change the functionality of the edit/create pages, but instead help future developers understand if a request is successful or not. The messages on each of the notifications come from the predetermined messages listed on data.ts.
In this pull request:
Motivation: After working on Issue #1191 and Issue #1517, I have noticed that there was inconsistency with the failure/success notifications across the edit/create pages. Some pages had it implemented with vague messages, while some did not have the notifications implemented at all. After discussing with the project maintainer, it was brought to my attention that there was an existing issue that had overlapping problems. As someone who is familiar with the edit/create pages, I wanted to take the opportunity to work on this issue.
Partly Addresses #869
Type of change
Checklist
Limitations
As discussed in the comments of the Issue page, I was not sure whether or not the implemented notifications have the desired information. In addition, no progress was done on the PreferencesComponent.tsx (Admin Settings page) at the time of initially creating the pull request. I plan on addressing it after getting feedback on the other parts of the implementation.
To reiterate, this pull requests is intended to show the current progress and to improve once feedback is given.
Edit: In addition, I was not able to figure out how to test the failure notifications for the delete features on the edit pages.