Conversion Deletions Involving Suffix Units #1470
Conversation
…onversion involving a suffix unit is deleted.
… a conversion involving a suffix unit.
|
Hello @huss - I pushed some recent changes I made to try and fix how conversions involving suffix units are handled. The code I pushed adds a call to removeAdditionalConversionsAndUnits() in the deletion route of units.js, reflecting the behavior seen in the edit route to keep things consistent. In addition, when deleting a conversion involving a suffix unit, conversions.js calls removeAdditionalConversionsAndUnits() to remove the associated units and conversions automatically. The EditConversionModalComponent.tsx file also now warns admin when deleting a conversion involving a suffix unit and lists the units and/or conversions that would be affected. I'm not sure if these changes exhibit the desired behavior exactly, so let me know if you'd prefer a different approach, or if there is anything I may have misinterpreted. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@esummerh Thanks for working on this somewhat vague issue. Overall, I like how it is done. Now that it is concrete, I have been thinking about the details more. Given this, here are some thoughts. Know that I welcome your ideas on this.
I like the unit delete is now similar to unit edit. I realized that neither warns the admin of the consequences. Furthermore, you cannot delete a suffix unit if there are conversions associated with it. Thus, I'm thinking that the normal way an admin gets rid of a suffix unit and related conversions is to delete the suffix unit on the edit units page, they are warned of the conversions and units that will be removed and then the admin decides if they want to proceed.
I also noticed that when you delete a conversion, it does not seem to remove all the conversions and units in the warning message. I have some unusual suffix items in my system due to testing so I'm not sure how general the situation is. However, given the idea above, maybe OED should not delete lots of items involved with suffix units when a suffix conversion is deleted. One option is not to allow deletion of suffix conversions where the source is a suffix unit. These are the ones that involve automatically generated OED units (unless someone adds one on their own which is strange). If the admin tries then they would be told to delete the related suffix unit which will now warn them and delete all the conversions. Option two is to delete that one conversion and leave the others. I'm uncertain if that would ever be used, if OED would automatically recreate anyway and if it would all be safe. If there is time, someone could test this to see if it makes sense. For now, OED could do option one where a lot of the code to warn above would be removed from conversions and put into units. The issue of not seeing all the items removed will now be part of units to see if it exists and what needs to be fixed.
I do like the logging you did on the server. I just had a few thoughts on those that are in comments.
There is another issue on the table. Deleting conversions can orphan units. In general, OED tries to catch these and someone is currently working to check the hard cases (see issue #905). I think the removal of suffix units will ultimately have to work with this as they could cause an issue. It is in unit edit but involves conversion removal. I think this is outside this PR but I'm noting it.
I'm sorry this is changing while you are working on it. Know that your good work has led to better understanding the issue. I'm happy to discuss more and help as you would like.
| "conversion.delete.meter.related": "The following meters use meter unit\u{26A1}", | ||
| "conversion.delete.meter.reduce.graphable": "Deleting this meter conversion will reduce the number of graphable units for meters using meter unit\u{26A1}", | ||
| "conversion.delete.restricted": "This conversion cannot be deleted until the significant consequences of doing this are addressed.\u{26A1}", | ||
| "conversion.delete.suffix.conversions.to.delete": "The following units will also be deleted\u{26A1}", |
There was a problem hiding this comment.
Does the text have units/conversions switched for the two fr keys?
| const dest = await Unit.getById(req.body.destinationId, conn); | ||
| // Check if the source or the destination is a suffix unit | ||
| if (source.typeOfUnit === 'suffix') { | ||
| log.info('Suffix unit is used in conversion deletion as source.'); |
There was a problem hiding this comment.
I think both these messages would be clearer in the logs if the actual conversion information was given. For example, stating the source and destination of the conversion, maybe with the the arrow that is common in other displays of a conversion.
| try { | ||
| const unit = await Unit.getById(req.body.id, conn); | ||
| if (unit.typeOfUnit === 'suffix') { | ||
| log.info('Deleting a suffix unit. Now deleting associated units and conversions.'); |
There was a problem hiding this comment.
Same idea so put in the name of the unit that is being deleted.
| const unit = await Unit.getById(req.body.id, conn); | ||
| if (unit.typeOfUnit === 'suffix') { | ||
| log.info('Deleting a suffix unit. Now deleting associated units and conversions.'); | ||
| await removeAdditionalConversionsAndUnits(unit, conn); |
There was a problem hiding this comment.
I cannot easily put a comment in removeAdditionalConversionsAndUnit so I'm putting the comment here. I think it would be a good idea to log each items removed in that function. Thus, the ones in the route are in overview log msg and the ones in the function log as each is removed.
|
@esummerh Another developer is working to do other aspects of deleting conversions so I was wondering the status of addressing the comments. If you cannot do them then someone else can likely finish this up if that is desired. |
@esummerh Just wanted to touch base again. If OED does not hear from you by 7/9 then someone else may finish this work. Please let me know if you have any questions/thoughts. |
2 participants
Description
This PR improves how OED handles the deletion of conversions involving suffix units. When a conversion involving a suffix unit is deleted, any autogenerated units and conversions associated with it are removed using removeAdditionalConverionsAndUnits(). The EditConversionModalComponent.tsx file was also updated to warn users of the units and conversions that would be affected by deleting a conversion involving a suffix unit.
Fixes #1448
Type of change
Checklist
Limitations
I'm not sure if this behavior aligns completely with OED's desires in this regard, so any update suggestions are welcome.