Repository navigation
Improve suffix unit conversion deletion with cascade delete and UI warnings - #1693
BunnyBea83 wants to merge 22 commits into
Conversation
- Add checkUnitDependencies service for validation - Fix async race condition in removeAdditionalConversionsAndUnits - Add bidirectional conversion support and depth limiting - Enhance simulation to account for suffix unit cascades
- Add checkUnitDependencies service for validation - Fix async race condition in removeAdditionalConversionsAndUnits - Add bidirectional conversion support and depth limiting - Enhance simulation to account for suffix unit cascades
- Add getConversionsByUnitId method - Add deleteConversionAndRelatedSuffixes with dependency checks
- Add dependency checks before suffix unit cleanup - Handle bidirectional conversions safely - Add row-level locking for concurrent operations - Improve error messages with meter/group details
- Show affected meters/groups in deletion warnings - Improve suffix unit warning formatting - Add translation keys for dependency warnings
- Removed getConversionsByUnitId method as it is no longer used - Removed deleteConversionAndRelatedSuffixes method as it is no longer used
- Refactored hide suffix functions to delete suffix functions - Suffix checks now account for suffix inputs (type of unit = unit, suffix = <contains a string>) when checking for dependencies.
- Added deleteUnitSafely method - Method checks for dependencies of the unit, clears unit connections to meters, groups, and conversions, then deletes the unit.
- Deletes suffix units instead of hiding them (setting displayable to None) - Calls deleteUnitSafely to check for unit dependacies, deleting the unit, and cleaning cik
- isSuffixRelated function determines if a unit is a suffix unit based on unit type (Suffix Analyzed) or if the suffix field is filled in (Suffix Input) Refactor: - Altered suffix detection cases to call isSuffixRelated. Reduces code redundancy and accounts for Suffix inputs. - Added code commentry - Modified suffixTypeUnitsToDelete to a simpler display
- Modified relatedConversions Fix cascade deletion walking backward into parent suffix units removeAdditionalConversionsAndUnits previously matched any conversion touching a suffix unit as either source or destination, regardless of direction. This caused cleanup started from a child unit (e.g. deleting "kg of X" -> "gallon") to walk backward through the parent-to-child conversion that created it, incorrectly cascading the delete up to the parent unit itself. Now only conversions where the unit is the source are followed, except for bidirectional conversions, where both directions are still valid since they genuinely work both ways. - deleteUnitSafely now occurs after removeAdditionalConversionsAndUnits
Unit tests for suffix unit deletion tests for: - Deletion of OED created units and their conversions - Clearing a dependent meter's unit ID to allow the auto-created unit to be deleted - Clearing converstions that reference OED created units - Clears a group's default graphic unit reference instead of blocking deletion of the unit it points to - Recursively cleaning up nested suffix chains - Prevention of deleting parent unit when deleting conversion from child unit - Deletion of bidirectional conversions relating to suffix units - Clearing cik rows before unit deletion - Prevention of deleting regular units when deleting a suffix related conversion
|
@BunnyBea83 Thank you for this PR. It will take me about a week to get to review this. The description seems to be fine from my quick review. |
There was a problem hiding this comment.
@BunnyBea83 First, I want to apologize for how long this review took (see Discord msg on developer channel on OED server). I want to thank you for this contribution to OED. It is a larger effort that did many aspects (changes, tests, etc.) and esp. nice given this is your first contribution to OED.
I have tried to make a good first pass on this PR. Overall, it works well and the code is fine. I've made a number of comments for you to consider. Some are to get your thoughts and we can discuss those before changes are made, as appropriate. Given this and the scope of the PR, I am probably going to need to do another pass on aspects of the work and will delay extensive testing until it is closer to finalized (I'm assuming changes will be needed). This is normal on a PR of this size. Please let me know if anything is not clear or you have thoughts/ideas.
Everything below here is consider resolved either by being done, another comment or planned actions outside this PR.*
Below are some comments that seemed best to place here:
-
The description says it Partly Addresses issue 1448. Are there aspects that you feel are not completed in that issue if this PR is merged?
-
I tried to find the old git commit record for the previous PRs but did not readily find it. Am I missing it? If not, I'm not sure you may have caused this. If it is gone then maybe the description should list the previous developers in an appropriate way so it is easy for others to see they worked on this previously. I can do this if you prefer and agree.
-
I had an unusual suffix unit in my setup so I tried to delete it. The visualization shows:
When I went to the conversion and tried to delete it it failed (note lack of failure reason):
and during the process it also a gave warning about what it was going to do:
The terminal where OED was started had these messages:
web-1 | [INFO@2026-09-08T17:12:23.859+00:00] Suffix-related unit 43 is used in conversion deletion as source. Cleaning up related conversions and units.
web-1 |
web-1 | [INFO@2026-09-08T17:12:23.864+00:00] Suffix-related unit 44 is used in conversion deletion as destination. Cleaning up related conversions and units.
web-1 |
web-1 | [ERROR@2026-09-08T17:12:23.864+00:00] Error while deleting conversion and updating meters/groups: QueryResultError {
web-1 | code: queryResultErrorCode.noData
web-1 | message: "No data returned from the query."
web-1 | received: 0
web-1 | query: "SELECT * FROM units WHERE id = 44;"
web-1 | }
web-1 | Stacktrace:
web-1 | QueryResultError: No data returned from the query.
web-1 | at Query.<anonymous> (/usr/src/app/node_modules/pg-promise/lib/query.js:217:41)
web-1 | at Query.handleReadyForQuery (/usr/src/app/node_modules/pg-promise/node_modules/pg/lib/query.js:137:14)
web-1 | at Client._handleReadyForQuery (/usr/src/app/node_modules/pg-promise/node_modules/pg/lib/client.js:312:19)
web-1 | at Connection.emit (node:events:514:28)
web-1 | at /usr/src/app/node_modules/pg-promise/node_modules/pg/lib/connection.js:117:12
web-1 | at Parser.parse (/usr/src/app/node_modules/pg-protocol/dist/parser.js:36:17)
web-1 | at Socket.<anonymous> (/usr/src/app/node_modules/pg-protocol/dist/index.js:11:42)
web-1 | at Socket.emit (node:events:514:28)
web-1 | at addChunk (node:internal/streams/readable:324:12)
web-1 | at readableAddChunk (node:internal/streams/readable:297:9)
The web console had an uncaught exception:
I have not tried to trace this down. When I deleted the other suffix conversion (to 100 w bulb) it worked fine and cleaned up the other one too.
- Small point but could space be added before the final delete statement at the end of the conversion delete modal so it is separated. I might also make it bold to stand out more.
- If I try to delete the standard OED test conversion of kg CO2 -> kg it has in the warning (meter one is similar):
The admin is not used to seeing the connection to kg and Metric ton as these are artifacts of OED's processing of the suffix unit. This is seen in the unit visualization on the lower analyzed graphic. I don't know if it is easy to remove these or somehow label them/improve the message so it is clear for an admin. I'm happy to discuss possibilities.
- The delete seemed to work fine when done in this case. I'm noting that the link to kg CO2 for the meters stays and in the analyzed unit graphic it is shown as orphaned (as it is). This is a suffix unit and I welcome thoughts on if/how there might be another/better way to deal with it. At a minimum, the help system should note this if it stays.
- Also relating to deleting the standard OED test conversion of kg CO2 -> kg, It has:
Affected meters:
"Electric Utility kWh in MTon CO₂"
A lot of other meters are listed below that have affected so I think it may be confusing to the admin. Is it possible to clarify how they differ in the text?
|
@huss added 3 Oct 26: See later comment on the resolution of all of these.*
Issue 1448 feels completed in terms of successfully removing chains of auto-created units. However, I listed the PR as partially addressing the issue because it currently cannot delete multi-hop paths, as described in the “Limitation” section of the PR. This limitation may be addressed as I make the changes requested during this PR review.
I was unable to successfully merge the previous PR due to the merge conflicts it was presenting. Because of those conflicts, I manually incorporated the code from that PR into my branch and used the same commit messages that were present in the original PR. I would appreciate any advice on the most appropriate way to credit the contributors for their previous work. The following commits contain the features from hmhngx's PR that I manually implemented from PR #1561: Hmhngx mentioned that their work was built off of PR #1470, so I referenced both PRs in this PR to keep the overlapping work documented in one place and to credit the contributors who previously worked on this issue. The remaining commits contain my own implementations and changes.
This failure occurs in lines 243 to 252 of I will address this by handling this edge case so that a unit that has already been deleted during the source cleanup is not processed again during the destination cleanup.
I'm not sure these can be removed outright because they're reachable through the conversion chain and used for real unit-compatibility calculations. However, I agree that the messaging wasn't clear enough as to why they're relevant to an admin who didn't set the connection themselves. I've now added labeling instead where OED created units are now tagged in the affected meters/groups lists. This way the admin can tell which units they set up directly vs internal artifacts. I also restructured the surrounding lists so meters/groups sharing the same lost units are grouped together more clearly. Each group now gets its own heading with a bulleted list of lost units underneath, and groupings are separated by a light divider. I recognize that the style of formatting is slightly different compared to other UI notifications. Let me know if this addresses your concern or if you have something more specific in mind.
Thanks for flagging this. Based on the structure of the code the real underlying conversion One direction I'm considering is blocking the deletion of a derived suffix conversion directly, and instead redirecting the admin to the action that resolves the issue. Either deleting the real underlying conversion, or clearing the unit's suffix field via
Good question. Yes, it's intentional for safety. That said, I think the delete functionality can use a split solution as opposed to a blanket solution since not everything carries the same risk of stale:
A more thorough approach, worth considering separately, would be to re-run the simulation when the admin confirms the deletion and compare the results against what was originally shown. If they differ, the updated results could be displayed to the admin and require them to reconfirm before proceeding. This would close the staleness gap more completely than either trusting the original simulation or silently re-verifying during deletion. However, I think this would be a larger change than what is currently included in the PR, so I wanted to raise it as a possible future approach rather than implement it without first confirming that it is desired. You raised related versions of the same question on the |
- Corrected spelling errors - Removed unused functions - Added translations for French and Spanish - Added `utils` conversion arrow for UI display - Added Bold casing to UI display - Removed `.trim()` in suffix checks - Removed single letter const identifiers - Fixed formatting issues - Replaces rew SQL calls with SQL files and associated imports. - Added suffixUnitCheck.js to `utils` for consistent function calls.
- Added UI notification if a unit may be orphaned due to conversion deletion - Simulation now detects for orphaned units. - Implemented orphan unit check post actual deletion, and the admin is notified of said units. - Replaced teargeted `cik`row deletion with `redoCik` after a completed delete transaction.Cik is rebuilt once per request rather than per unit to avoid redundant recalculation on deeper suffix chains. - Reformatted references to former `hide` functionality into current `delete` functionality. - Conversion deletion logic now throws an error as opposed to logging an error. - Resolved formatting issues.
- Refactored unit tests to call from `util/insertData` and simplified function calls - Added clarifying notes to code functionality - Implemented CSS features for UI display during conversion deletion - Conversion deletion UI now created blocked segments for groups/meters that will lose the same units.
| const gps = new Point(90, 45); | ||
| const { removeAdditionalConversionsAndUnits } = require('../../services/graph/handleSuffixUnits'); | ||
|
|
||
| mocha.describe('Suffix unit cascade deletion', () => { |
There was a problem hiding this comment.
Thank you for letting me know about the utility functions. I rewrote the tests to use insertUnits,insertConversions, and insertGroups instead of making the objects directly. Since the insertMeters appeared to require a file or data array, I kept the meter test on a direct constructor as my tests only needed a meter with a given unitID. insertStandardUnits didn't appear to have typeOfUnit === SUFFIX or non-empty suffix feilds so I kept my parent/child implementation and condensed their duplicated calls into one helper function.
huss
moved this to Waiting for comments to be addressed & submission updated
in OED PR status
|
Everything below here is consider resolved by being done, another comment or planned actions outside this PR.*
I think what you did was good. I appreciate your careful enumeration of what you did. I suggest the following modification to the description text: to be precise where you can change the wording if you think that would be better: This PR builds off of PR #1561 by @hmhngx by incorporating parts of their work into the first five commits in this PR and PR #1470 that was used as the basis for PR #1561. |
|
Everything below here is consider resolved either by being done, another comment or planned actions outside this PR.*
Thanks for carefully thinking about this. Here is my reflection on what you said:
There might be other cases/considerations that others see. I'm open to other ideas and modification of what I am thinking. I think either option is okay. If the (hopefully) modest fix is not done then it could become a future issue and include the full fix as an option/information so this is not lost. |
|
From @huss on 3 Oct 26: All items in this comment are considered resolved.
As you described, the code I implemented handles the case silently.
I've also implemented this option. Both
I agree that this case would need an implementation of the Given that the recently pushed code covers a few of the fail cases you mentioned, I don't think additional implementation is needed here, and I can document the deferrance of |
This comment was marked as duplicate.
This comment was marked as duplicate.
This is now considered resolved for this PR. The undone issues on chains will be future work and a new issue if desired (unsure if want to). The issue with bidrectional was supposed to be solved but my open PR will be augmented to address the issue I found.
The new PR description gives credit so this is now resolved.
Done.
Done
I think admins should be told not to do this unless they mean it. I think the message makes it clear that it was not removed. I consider this resolved for this PR.
This is complicated and not likely to be an issue so I consider it resolved for this PR (and maybe forever). |
|
This is to note that the only items left before the recent review is this comment. All others are either resolved or have a bold comment indicating that the comment is resolved. I left the comment chain so it is available for the future. |
There was a problem hiding this comment.
@BunnyBea83 Thank you for the considerable updates to address my previous comments. I'm sorry this took a little while to review but I was trying to be systematic and not put aside anything I wanted to consider. Overall, your changes work well and I appreciate you going beyond the basic comments to make further changes. I have made some comments to consider but I think they are becoming more straightforward. I have resolved previous comments and only items with new comments are still under consideration. FYI, I feel I will need to do one complete pass of all changes when this is close to ready. This is because it is so large and comprehensive so I will want to do another high-level evaluation to be sure nothing slipped in.
These are comments that were more appropriate here rather than in files:
TheResolved.npm run checkshows these issues:
/usr/src/app/src/client/app/components/conversion/EditConversionModalComponent.tsx
267:80 error Missing semicolon semi
364:47 error Unexpected parentheses around single function argument arrow-parens
399:18 error Strings must use singlequote quotes
414:41 error 'i' is defined but never used @typescript-eslint/no-unused-vars
441:18 error Strings must use singlequote quotes
456:41 error 'i' is defined but never used @typescript-eslint/no-unused-vars
✖ 6 problems (6 errors, 0 warnings)
I did the following:Later comments address this.- Create meter unit aum
- Create unit of type unit aus with auss for suffix. Doing this requires the patch from the PR I recently opened and linked to this PR. It may work the same way if it is of type suffix but I did not try that.
- Create unit of type unit aux & auz
- Create conversion aux <-> auz
- Create conversion aus -> aux
- Create conversion from aum -> aus
- Delete aum -> aus. It works but it incorrectly states it will remove units aux of auss & auz of auss and conversions aus -> aux of auss & auz -> aux of auss. They are not removed. Maybe they should be left so message is wrong or do you think otherwise?
| orphaned: boolean; | ||
| } | ||
|
|
||
| export interface SimulateDeletePotentiallyOrphanedUnit { |
There was a problem hiding this comment.
I don't see this used outside of this file and removing the export does not seem to cause any issues for me. Is it needed?
There was a problem hiding this comment.
The export isn't needed so I removed the export. The two existing SimulateDeleteAffected__ interfaces are also only used within this file. However, I've left them as is since they predate this PR. I can unexport them if you'd like.
There was a problem hiding this comment.
If you are willing to remove those too that would be nice. Thanks for catching them.
| const gps = new Point(90, 45); | ||
| const { removeAdditionalConversionsAndUnits } = require('../../services/graph/handleSuffixUnits'); | ||
|
|
||
| mocha.describe('Suffix unit cascade deletion', () => { |
There was a problem hiding this comment.
This is okay but it would not, I think, be hard to use those functions. The insertMeters has if (meterData.file) { to load from a file so not having a proper value (I think missing is okay?) skips doing that. It then goes to a else that uses meterData.data. If it was changes to an else if that checked if meterData.data as the file check does then it would skip it if there is not data. I think that might do what is desired without impacting other uses. What do you think? If it seems okay then you can either do it or an issue can be opened up to make the change and modify your new tests as needed.
- Resolved npm run check errors - Removed export from SimulateDeletePotentiallyOrphanedUnit - Corrected spelling errors and formatted descriptions
- Altered SQL files to include the name of the model they are referencing - Modified the model files calling the SQL functions to include the changed names. - Modified `findOrphanedUnits` to reference a specified limit
- Implemented a deleteAll feature for Cik - redoCik is called in conversions.js and units.js - Modified CheckUnitDependencies to call the deleteAll on Cik as opposed to deleteByUnitId - Removed deleteByUnitId SQL call
While testing, the delete confirmation listed an already orphaned unit as a unit that would be orphaned due to the deletion. `potentiallyOrphanedUnits` has now been restricted to units that lose their last conversion in the delete.
The units are removed and then recreated. Deleting aum → aus calls the cleanup for aus, since it has a suffix, and then redoCik runs handleSuffixUnits, which regenerates aux of auss and auz of auss because aus still has those paths. In a test of this conversion delete, I saw the client call refresh right after delete, which is consistent with that rebuild. So the end state looks right, but the warning is misleading and any admin edits to those derived units would be lost. I agree they should be left alone when deleting an incoming conversion. Would you like me to skip the cleanup and the warning in that case, or should it be handled in a follow-up? |
Thanks for clearly explaining what was happening. Assuming the change does not impact other cases then I think it should be skipped. I'm assuming doing this is not hard. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@BunnyBea83 Thank you again for the quick update that addressed my comments along with the comprehensive evaluation. Overall, most items are now resolved. I made a few comments to clarify what I prefer or for minor items. I wanted to get this back quickly so I have not done my final sweep - it would be final anyway if all changes were basically done so waiting would be easier. Please let me know if anything is not clear or you have any thoughts.
| orphaned: boolean; | ||
| } | ||
|
|
||
| export interface SimulateDeletePotentiallyOrphanedUnit { |
There was a problem hiding this comment.
If you are willing to remove those too that would be nice. Thanks for catching them.
| * Finds units that may have been orphaned (still displayable but with no | ||
| * remaining conversions). Used as a post-delete sanity check, not a guarantee. | ||
| * @param {*} conn The connection to use. | ||
| * @param {number} [limit=100] The maximum number of units to return. |
There was a problem hiding this comment.
From what I see, the JSDoc displayed (at least in VSC) does not show the limit. I think in other places OED puts the default value in the description text and that might be a benefit here. It isn't a big deal as the function signature shows the default.
| const gps = new Point(90, 45); | ||
| const { removeAdditionalConversionsAndUnits } = require('../../services/graph/handleSuffixUnits'); | ||
|
|
||
| mocha.describe('Suffix unit cascade deletion', () => { |
There was a problem hiding this comment.
Sounds good. Do you want to open (you certainly did more investigation of it than I did) the issue or should I?
| // Full Cik recalculation after all cascading deletes complete, to | ||
| // correctly handle any unit pairs whose path routed through a deleted | ||
| // unit, not just the units/conversions directly touched above. | ||
| await redoCik(conn); |
There was a problem hiding this comment.
I noticed this since you have changed this units route in this file. Thus, if you are willing and see it the same way, it would be nice to fix up now.
I was also thinking that using the refresh is the way to go but was not sure how you might feel about doing that. Since you raised it, I think it is a good way to go. The refresh api has these invalidations: ['ConversionDetails', 'Cik', 'Readings', 'Units']. Given this, I didn't think those need to be invalidated here (I did not see the other routes using this refresh doing it). If this is right then you would only need to invalidate the meter and group data.
github-project-automation
Bot
moved this from Ready for review
to Waiting
in OED PR status
This branch has not been deployed
2 participants
Description
This PR solves OED's opporations for deleting conversions involving suffix units. Upon deletion of a suffix unit related conversion, OED created units and related conversions will be cascade deleted. UI warnings are inplace to notify the user of units and conversions that will be deleted, and groups/meters that will be affected by the deletion. This PR builds off of PR #1561 by @hmhngx by incorporating parts of their work into the first five commits in this PR and PR #1470 that was used as the basis for PR #1561.
Key improvements:
Partly Addresses #1448
Type of change
Checklist
Limitations
Multi-hop-generated suffix units are not yet covered by this cascade delete since it likely needs a
design decision before implementation.
This PR resolves cascade deletion for the direct/single-hop case (deleting a
conversion from a suffix root, or from an auto-created child, correctly
cleans up what it should). It does not yet handle units auto-created via
multi-hop paths (e.g. suffix unit → new auto-created unit→ further conversion). Since OED always attaches such units
directly to the root rather than the intermediate unit, deleting the
intermediate conversion currently leaves these orphaned. Possible implementations could include either a re-walk of valid paths after delete, or tracking explicit provenance on auto-created units, both of
which are a larger design change than this PR's scope. Flagging for
follow-up discussion with maintainers.
Before Deletion of multi-hop path 'metric Ton of TestUnit -> gallon'
After Deletion of multi-hop path
Deletion of TestUnit -> kg removes all OED created units
Deferred: comprehensive stale-simulation protection.
The current delete flow re-verifies the cascade logic server-side, but doesn't detect the case where a new connection is added to a unit after the simulation runs and before the admin confirms the delete. That connection would be silently lost without warning. A fuller fix would re-run the simulation at confirm-time, compare it against what was originally shown, and prompt the admin again if they differ. Flagged as low-priority/unlikely per discussion with @huss, but noted here so it isn't lost.