OED icon indicating copy to clipboard operation
OED copied to clipboard

protect admin when deleting a conversion

Open huss opened this issue 2 years ago • 3 comments

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 avatar May 20 '23 22:05 huss

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.

huss avatar Oct 25 '23 00:10 huss

I will be working on this.

danielshid avatar Oct 22 '24 21:10 danielshid

For the record, this is the ideas of doing all the checks. It may need some modification and more details but should generally be okay. The parts of issues with meter units/possible graphic units in the first subsection are covered by PR #1400.

Possible warnings or issues if deleting a conversion

Several cases seem to need running the graph algorithm to determine the impact. So far that is limited to the server, partly to avoid loading the software on the client. OED could reconsider this but this write up tries to avoid that.

Impact on meter units and possible graphing units

  • If source is meter unit (cannot be bidirectional).
    • This case was more complex than described here. The code in PR #1400 has the correct code with comments on what it is doing.
    • If only conversion, i.e., the source meter unit is only source of a conversion, then
      • warn that will orphan this meter unit.
      • If used by a meter (the source unit is a meter unit) then inform admin and cancel delete.
    • If not only conversion then warn admin for each meter that this will reduce the graphable units for a meter with the source meter unit.
  • If source is suffix unit (I think they can only be a source but should check)
    • Issue #1448 discusses issues with suffix units and esp. deleting them. That needs to be addressed before this can move forward. Note there is a TODO in the code and commented out code that does what is stated below.
    • Check if no other conversion is present with this suffix unit (as source). It is unclear if there ever would be a second conversion but the suffix unit can be a destination of a non-bidirectional conversion, probably from another meter. If so, warn admin that will disable use of this suffix unit.
  • If the conversion involves two units of type unit that is not bidirectioaln (this is unusual) and there is no other conversion that has the destination in a conversion (either as the destination if not bidirectional or in a bidirectional conversion), then inform admin that will orphan the destination unit.
  • If the conversion involves two units of type unit then you may reduce the allowed graphic units for a meter. It seems you must run the graph algorithm to figure this out. For the example in the resource generalization document, if you delete the conversion between M3 of gas and MJ then Electric utility meters cannot graph in M3 of gas and Natural Gas meters cannot graph in MJ, kWh or BTU. None of these will orphan a meter but does reduce the options. Given admins can do the same thing when creating units/conversions, I think it may be best to enhance the warning on delete so if this is the case they are told it may have consequences and if they proceed they should check the result of doing this.

default graphic units

  • If the default graphic unit of a meter is the common case of the one directly linked to the meter unit, i.e., kWh for an Electric utility meter, then it would be stopped above. This is because the meter unit was the source of the conversion and the delete was cancelled.
  • If the default graphic unit is from a chained conversion then it has the same issue of two units above. This seems to require running the graphic algorithm. Clearly OED should warn the admin but that was done above. The message should also include about default graphic units. However, in this case, OED can, after the delete is complete and cik is updated in Redux state, check if any default graphic unit for a meter or group is no longer valid. The admin can be informed for each impacted group or meter. The admin then has two choices:
    1. Undo the conversion delete so it is put back.
    2. Leave the conversion delete but know that all these meters/groups will have their default graphic unit set to no unit.

Groups

This case seems ugly given groups are recursive. It was hard to figure out if changing members caused issues and this involved compatible units. Without running the graph algorithm, it would mean running the tests after the change is made. These would involve seeing if any group member is not longer valid. If this is the case then the admin is informed and the conversion deletion is undone.

All the messages will be listed. If any cancel the delete then it is not performed. If not, the admin is asked if they want to continue. The post delete checks are done after this.

huss avatar Dec 05 '24 22:12 huss