huss
commented
1 year ago After talking with @mrathgeber and thinking about this, I have decided that deleting a conversion involving any unit that is compatible with any used meter unit is problematic. This same type of issue (but more complex here) came up about deleting a unit (see issue #1020) and it was decided you cannot delete a unit that is used in places. The same ideas will be used here.
The basic outline of a solution is:
- Use Redux meter state to find all the meter ids that are used. Technically if two meters have the same meter unit then you only need to check one of them. However, this should be fast without this, easier and will give the admin better warning messages.
- Use unitsCompatibleWithMeters() in src/client/app/utils/determineCompatibleUnits.ts where supply the meter ids just found to the function. In principle you could do them all at once but to make the messages for the admin better the meter ids are done one at a time (each argument is a set of size 1).
- See if the returned set of compatible units contains the source unit id for the conversion that is being deleted. If so, it may be an issue to delete the conversion so it will not be allowed. Add this meter to the message. The start of the message can be something along the lines of "Deleting this conversion may cause the compatible units to change on the following meter(s) so deleting it is not allowed".
- Once all meters have been processed, pop up the message if any meter is an issue. See validateGroupPostAddChild() in src/client/app/components/groups/EditGroupModalComponent.tsx for how it looks and works there as what is desired. In this case the delete is not executed. Otherwise the delete happens.
A few notes:
- Checking all meters means that any groups containing this meter are indirectly protected. However, the admin will not know about these groups in the message. That could be added if desired.
- It is possible that the delete would be okay because the source unit was also included due to a different conversion. The proposed solution is conservative to avoid any possible issue.
- You only need to check the source of the conversion since the destination must also be compatible since the conversion would be in the analyzed graph.
- Doing this correctly to only stop bad situations and help fix up other ones requires non-trivial logic. See what groups need to do in that case for some idea of the minimum involved. Also, since removing one conversion changes the graph used to analyze chained conversions, it would need to be redone. Currently that is only done on the server so another complication.
- Here is an example of how it is difficult to know what is going on: meter unit A -> unit B unit B <-> unit C Just checking meter units would miss the impact of deleting the second conversion involving B & C. Since unit C may have conversions to other units it is tricky to know the overall impact of the delete.
This isn't trivial and I may have missed things. Thoughts and other ideas are welcome.
danielshid
Is your feature request related to a problem? Please describe.
Admins can delete a conversion. This can cause undesirable effects including:
There may be other cases but hopefully this covers them.
Describe the solution you'd like
Checks need to be added to warning the admin. As was done for groups, the default graphic unit is set to no unit if needed but not allowed if there is no remaining default graphic unit. If members of a group become invalid then the change is not allowed (as is done in group edit page).
Describe alternatives you've considered
The admin is on their own and that is not the philosophy of OED.
Additional context
None