OED icon indicating copy to clipboard operation
OED copied to clipboard

enhance suffix unit conversions

Open huss opened this issue 9 months ago • 10 comments

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

There appears to be multiple issues/considerations conversions that involve suffix units:

  • A suffix unit can be in a bidirectional conversion. OED is curious if there is any use case where this makes sense. If not, it should not be allowed.
  • When you delete a conversion involving a suffix unit it works but all the autocreated units and conversions stay. The admin can delete the conversions one-by-one and then do the same with all the units. This seems unnecessarily complex and OED should probably do this automatically. OED should warn the admin of the units/conversions that will be eliminated and prompt for approval.

There may be other issues/items to consider. A careful analysis of what would be needed and how to do it needs to be done and implemented. @huss is willing to work with a developer to work this out.

Describe the solution you'd like

It should work as described above.

Describe alternatives you've considered

The manual method described does work and admins can avoid bidirectional.

Additional context

None

huss avatar Mar 19 '25 19:03 huss

Hi @huss - I've submitted a few PRs for smaller UI-related issues and found this task interesting because it involves a bit more design and analysis. I'd like to give this issue a try if it is still open to contributors.

esummerh avatar Apr 09 '25 17:04 esummerh

@esummerh You are welcome to work on this issue. As you noted, it is less settled. Know that I'm available to discuss via message or a video call it that would help at any point.

huss avatar Apr 09 '25 17:04 huss

@huss - Great! Thanks!

esummerh avatar Apr 09 '25 18:04 esummerh

Hi @huss - I wanted to start working on this issue and I have a few questions. For the bidirectional conversion portion, does OED allow suffix units to act as both a source and a destination in conversions, or should it be restricted to only one role (i.e. source)? Could you also elaborate a bit on why bidirectional conversions involving suffix units could be problematic for OED? I just want to make sure I understand the design intent regarding this and any edge cases before I implement anything.

esummerh avatar Apr 10 '25 18:04 esummerh

@esummerh

For the bidirectional conversion portion, does OED allow suffix units to act as both a source and a destination in conversions, or should it be restricted to only one role (i.e. source)?

I don't know of any use for bidirectional. Since no one has brought one up in this issue I think OED should be restricted to one direction. Note it is okay for the suffix unit to be either the source or the destination.

Could you also elaborate a bit on why bidirectional conversions involving suffix units could be problematic for OED?

Suffix units are special as are meter units. In the case of meter units it must be one direction with the meter as the source. The reason is you cannot create more of what the meter reads. For example, a meter that reads in kWh cannot gain electrical usage because someone tries to convert from money to that meter (why OED does not allow). For a suffix, the suffix unit is never directly used but is a pass through unit to other units that OED creates. So, some units go into the suffix unit to then continue to the created units but not the other way around. That is why they are one directional. If you want some more details you can visit the suffix help info on the OED documentation. OED also has a unit/conversion visualization for the site on an admin page for visualizing them that you can see with the developer test data.

I just want to make sure I understand the design intent regarding this and any edge cases before I implement anything.

No problem and it is good to ask. Understanding the big picture usually leads to the code doing what is desired rather than what was stated (and can be wrong). Thanks for the questions and let me know if anything is not clear or you have more questions.

huss avatar Apr 10 '25 23:04 huss

Hi @huss - Thanks so much for the response. It definitely cleared things up. By the way, would you prefer that I submit one PR for both of the issues described, or one for each?

esummerh avatar Apr 11 '25 03:04 esummerh

By the way, would you prefer that I submit one PR for both of the issues described, or one for each?

@esummerh Either is fine. If you are going to do both and it is easy to do a single PR then that is fine.

huss avatar Apr 11 '25 13:04 huss

Hi @huss - I just submitted a PR that partly addresses this issue by implementing the bidirectional suffix conversion behavior described above. For the conversion deletion piece, are there any files other than EditConversionModalComponent.tsx that I should look at and/or write code for?

esummerh avatar Apr 11 '25 18:04 esummerh

@esummerh I had seen the problem happen but had not looked at the code until your comment. I have noticed a few things:

  • src/server/routes/units.js: edit does removeAdditionalConversionsAndUnits but delete does not. I'm not certain but this seems inconsistent.
  • The newer checks on unit deletion don't allow for removing a suffix unit that is linked to an OED created unit as part of the suffix process. This forces one to delete each conversion to delete the unit.

I think it should be thought through if one can delete a suffix unit where the admin is told which units would be orphaned/removed as part of the process to confirm the action. This would (I think) allow for automatic removal. I'm open to thoughts on other ways to do this as I have not thought about it too much. There is still the case of removing the single conversion between the suffix unit and the OED generated one that will orphan the generated one. When I tried to do this it allowed the delete but the conversion does not go away. This seems unfortunate that it does not warn or give an error message for this. It seems you have to start with the conversion linking the suffix unit to the main other unit (not one with an OED generated unit) and then delete the ones with the OED generated unit.

So, this does not seem trivial. If you want to chat to figure out what to do then I'm happy to do that.

huss avatar Apr 11 '25 19:04 huss

Thanks @huss - Your response was very helpful. I'll try a few things out and see what I can come up with.

esummerh avatar Apr 12 '25 03:04 esummerh