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

protect admin when deleting a conversion #905

Open
huss opened this issue May 20, 2023 · 1 comment
Open

protect admin when deleting a conversion #905

huss opened this issue May 20, 2023 · 1 comment
Labels
assigned the issue is currently being worked on by a developer p-high-priority This issue should be the focus of design and developement effort. t-enhancement This issues tracks a potential improvement to the software
Milestone

Comments

@huss
Copy link
Member

huss commented May 20, 2023

Is your feature request related to a problem? Please describe.

Admins can delete a conversion. This can cause undesirable effects including:

  • If this is the only conversion for a meter using that unit then you can no longer graph it.
  • You can orphan a unit if this is the only conversion to it.
  • This can change the compatible units. This might cause a default graphic unit for a meter to become invalid. For a group, it could make the default graphic unit or the members invalid.

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

@huss huss added t-enhancement This issues tracks a potential improvement to the software p-high-priority This issue should be the focus of design and developement effort. labels May 20, 2023
@huss huss added this to the 1.1 release milestone May 20, 2023
@huss huss added the assigned the issue is currently being worked on by a developer label Oct 24, 2023
@huss
Copy link
Member Author

huss commented Oct 25, 2023

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:

  1. 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.
  2. 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).
  3. 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".
  4. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned the issue is currently being worked on by a developer p-high-priority This issue should be the focus of design and developement effort. t-enhancement This issues tracks a potential improvement to the software
Projects
None yet
Development

No branches or pull requests

1 participant