Add Group Default Graphic Unit Error Handling for Unit Deletion #1542
Conversation
- Added group dependency checking in error handling - Moved dependency checking to happen before confirmation modal
There was a problem hiding this comment.
Thanks to @hmhngx for another contribution. Overall, it works as written. The linked issue #1513 discussed PR #1505 and its changes. I note the following:
Your PR is over 3 weeks behind development. This means the new code from that PR is not reflected in your code.The look/formatting of the messages in that PR is cleaner and nicer. While units did not use it before, I think it should be used here.The order and logic is different for PR 1505. I like that it has default graphic units later and its ordering. The next bullet on default graphic units should probably not be done if conversions are impacted so only those messages are shown. I think the changes to conversions also limit later messages if earlier ones are present. This is needed since changing conversions is not allowed and impacts default graphic units.It checks for impacted default graphic units, warns the user and then fixes them as needed. In the case of conversions it is done on the server since it involved a graph algorithm. In the case of units, the checks on if it is used in a conversion will stop these types of changes. Thus, the client can check for impacted default graphic units (as done) and then warn the admin as in the updated conversions that the default graphic unit will become no unit. As noted in the updated issue, this should be done for units as with conversions. It does say it could be done later but maybe now would be good (we can discuss). Note it is important that the actual delete and changes for default graphic units happens on the server as a transaction to avoid inconsistencies in the DB.
I think these should be done and we can discuss if you have questions or thoughts.
- Update error message formatting to use React elements matching conversion deletion style - Reorder dependency checks: conversions first (highest priority), then meters/groups - Add logic to limit default graphic unit messages when conversion errors exist - Implement proper modal navigation to return to main units page on error - Add cancel modal for error notifications with proper action handling - Use structured message elements with bold text, lists, and proper formatting - Follow mentor feedback to match PR OpenEnergyDashboard#1505 patterns and logic
- Add English translations for unit deletion warnings - Add French translations with proper translation markers - Add Spanish translations for all supported languages - Include keys for: - unit.delete.meter.default.lost - unit.delete.group.default.lost - unit.default.graphic.unit - unit.delete.restricted - Support comprehensive error messaging in all languages
- Implement database transaction for unit deletion with default graphic unit updates - Update meters: SET default_graphic_unit = NULL WHERE default_graphic_unit = unitId - Update groups: SET default_graphic_unit = NULL WHERE default_graphic_unit = unitId - Delete unit after updating dependent records - Follow same transaction pattern as conversion deletion - Ensure database consistency during unit deletion process - Update success message to reflect meter/group updates
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Now that the code is working and I tried to test it, I'm thinking the thoughts above were not needed and not thought out enough. When I tried to test the catching of losing a default graphic unit it did not show up. I thought about it and I now think that it cannot happen for units. To set the default graphic unit for a meter/group, the unit must be compatible with a compatible meter unit which implies there is a chained conversion from the meter unit to the default graphic unit. At a minimum, this means the default graphic unit is the destination of a conversion (it might also be a source). The first tests check for this and stop the deletion if the unit is either a source or destination. It skips the default graphic unit test in this case so it is never seen. If the admin removes the conversion involving the default graphic unit then the warning for that delete will show that the default graphic unit must be changed to no unit and it will happen if the admin proceeds with the delete. Overall, this means, if I am correct, that the second set of tests for the default graphic unit will never be true so they are not needed. It also means the route can have the setting of the default graphic unit to no unit removed. A comment in the route file notes this too (coming soon). As a side note, conversions differ since the chaining means the conversion removed is not sufficient to know if a default graphic unit is impacted. It requires running a graph path algorithm. With a unit this is not the case. This was done and is complete. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
Thanks to @hmhngx for addressing the previous comments. I'm sorry this took so long while my time was turned elsewhere. I've made one big picture comment on the default graphic unit check that I asked for last time. If you agree, it will change the code needed. Several comments note the impact of this and there are a couple of other comments to consider. Please let me know if anything is not clear or you have thoughts.
- Remove unnecessary default graphic unit checks (caught by conversion checks) - Fix message formatting to match conversion deletion style - Add forceCancel to error modal - Remove unused groupDataByID import
- Remove unit.delete.meter.default.lost - Remove unit.delete.group.default.lost - Remove unit.default.graphic.unit - Keys no longer needed after removing default graphic unit checks
- Remove unnecessary transaction code for default graphic unit updates - Extract req.body.id to const unitId - Simplify to direct unit deletion only
…ponent - Change actionConfirmMessage prop from required to optional - Conditionally render ModalBody only when actionConfirmMessage is provided - Allows modals with forceCancel=true to work without confirmation message when needed
- Simplify message building by pushing directly to msgElements instead of using intermediate arrays - Fix spacing in conversion error messages (add space before unit name in quotes) - Improve meter error message: make it bold and use 'The following meters use' for clarity - Move error header to beginning of message array using splice for better structure - Add actionConfirmMessage to cancel modal to display error messages when deletion is blocked - Remove unnecessary conditional checks and simplify control flow
- Add 'unit.delete.meters.use' key in English, French, and Spanish - Supports clearer error message format: 'The following meters use [unit] as meter unit'
2 participants
Description
This PR implements comprehensive error handling for unit deletion following mentor feedback to match the patterns and logic from PR #1505 (conversion deletion). The implementation provides immediate feedback to users and prevents accidental deletion of units that are still in use.
Fixes #1513
(In general, OED likes to have at least one issue associated with each pull request. Replace [issue] with the OED GitHub issue number. In the preview you will see an issue description if you hover over that number. You can create one yourself before doing this pull request. This is where details are normally given on what is being addressed. Note you should not use the word "Fixes" if it does not completely address the issue since the issue would automatically be closed on merging the pull request. In that case use "Partly Addresses #[issue].)
Type of change
Enhanced Error Handling & User Experience:
Comprehensive Dependency Support:
Internationalization:
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
(Describe any issues that remain or work that should still be done.)