Skip to content

Fix unit type with a suffix on create/edit of a unit - #1727

Closed
huss wants to merge 19 commits into
OpenEnergyDashboard:developmentfrom
huss:fixSuffixUI
Closed

huss wants to merge 19 commits into
OpenEnergyDashboard:developmentfrom
huss:fixSuffixUI

Conversation

@huss

@huss huss commented Sep 30, 2026

Copy link
Copy Markdown
Member

Description

Summary

The desired behavior is:

When a site/admin adds a suffix unit, it will be a unit of of type unit with the desired suffix. When OED adds units based on a unit added by the site/admin, it will be of type suffix with a blank suffix.

It seems issue #1019 was wrong by restricting units to have a type of suffix if they have a suffix. This should be allowed and required.

Actions taken

Changes:

  • The code from PR Fixes to suffix type when creating or editing units. #1040 that implemented issue Units of type suffix should have a suffix #1019 needed to be revised so sites/admins inputted units can have a suffix and type unit. It is unclear it should ever be of type meter so that will not be allowed.
  • It is an open question if one should be allowed to make it of type suffix. If it is allowed then it would also need to be displayable and that is currently blocked by the UI. It seems a site really should use the standard mechanism to create the unit with type unit and a suffix and have OED create the actual suffix unit during its processing. Given all of this, OED will not allow this until someone comes up with a good usage since it would probably be a mistake in usage. If it is ever done then it needs to have warnings on the UI pages.

Overall this means the unit UI has been modified to enforce that the type is unit and not suffix when there is a suffix for a unit. This addresses both items above.

Information found

The above information/actions are based on information found and analysis of that information. What follows is that information in greater detail to preserve the record and to explain what is above.

Design doc

  • The OED design doc on resource generalization has:
    • "suffix. Are units created by OED as part of analyzing suffix units (such as CO2)" for one of the three types of OED units.
    • The examples with kg CO2 has it with a suffix of CO2 and type of unit. The table of final units shows the suffix type for the added units but there is no suffix ('').
    • It states: "Note a type_of_unit of suffix is different than a unit with a suffix string.". The OED creation of the suffix unit based on the site creating a unit with a suffix has the pseudocode new Unit(... Unit.type.suffix, "", showing the type of suffix with a blank suffix.
    • It defines: "Only units in the units table that are of type unit or suffix (so not meter) can be a graphic unit." This clearly indicates that any unit of type suffix is graphable. OED fixes this up by making the input unit with a suffix be undisplayable after it adds the new suffix units. Thus, the new ones are graphable but not the original unit with a suffix is not.
    • It warns: "An admin should never have a unit that has a suffix but cannot get to another unit as it will be hidden without another unit created." The revised UI automatically sets a site/admin created unit with a suffix to displayable none. This is also done during processing of that unit so it is somewhat duplicated but a good safety check in case a unit is added in another way. Thus, this is no longer an issue for normal usage.

Code

  • The standard OED test data is created in src/server/data/automatedTestingData.js and set in specialUnitsGeneral where kg CO₂ has typeOfUnit: Unit.unitType.UNIT, suffix: 'CO₂'. Thus, the unit type is unit but there is a suffix. This is consistent with the design document.
  • PR Fixes to suffix type when creating or editing units. #1040 (based on issue Units of type suffix should have a suffix #1019) from 11/7/23 changed the input of units so if there is a suffix (!= '') then the type of unit must be suffix. If not, it automatically changes on save so this is the case. This is done in src/client/app/components/unit/CreateUnitModalComponent.tsx in handleSaveChanges() where is has typeOfUnit: (state.typeOfUnit != UnitType.suffix && state.suffix != '') ? UnitType.suffix : state.typeOfUnit. This means you cannot enter a unit that mirrors the OED test data and is inconsistent with the design document since it should be of type unit not suffix.
  • PR Implementing cik visual graph #1382 from 11/21/24 added visualization of units that included suffix capabilities (the original version was more limited from PR Add VisualUnitsComponent #1328). In src/client/app/components/visual-unit/CreateVisualUnitComponent.tsx in a useEffect it has d.suffix && d.typeOfUnit === 'unit' ? colorSchema('suffix.input') : colorSchema(d.typeOfUnit)) so it colors a unit as suffix input if it has a suffix and it is of type unit. This is consistent with the test data and design document.
    • The test data shows pink (suffix input) because it matches the first part of the conditional given it is of type unit and has a suffix.
    • Any UI added ones are orange (suffix analyzed) because it matches the second part of the conditional because it is of type suffix and it has a suffix. In this case the type of unit is suffix and that is orange in the visualization. The visualization does match the design document but the UI entered unit does not and confuses the code into thinking it was an analyzed unit because it if of type suffix.
  • This work was done about 3/18/22 (other changes for the non-suffix conditions came later). When src/server/services/graph/redoCik.js runs it calls src/server/services/graph/handleSuffixUnits.js which gets the units to process via Unit.getSuffix(conn) that calls src/server/sql/unit/get_suffix.sql which returns units which match SELECT * FROM units WHERE suffix != '' so it is any unit type but must have a suffix. It then skips considering new OED created units (the special suffix processing) using if (destinationUnit.typeOfUnit === Unit.unitType.SUFFIX || destinationUnit.displayable === Unit.displayableType.NONE) {. If this input unit with a suffix was previously processed then the OED added units are of type suffix. These will be found during the graph path checking but will be skipped by the first part of this if statement. The second part makes sure that if it isn't visible then it isn't done (I think but not careful about that). Note any data currently input via the UI for create/edit of units still passes the check since it has a suffix and it does not matter if it is of type suffix for the unit. It will be processed and OED will find a linked unit to create new suffix units from.

Type of change

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section. This checkbox is required.
  • By submitting this PR I agree to the OED AI policy specified below and acknowledge that I followed its terms in this submission. This checkbox is required.

Limitations

This PR is built on top of PR #1693 since it was found during review of that code. There are many files listed as changed that are from that PR. This PR will need to wait until that is merged and then only 3 files should have modest changes.

AI Disclosure

List all AI tools used including version(s). You can submit without the versions but OED may request further information before accepting the PR.

None used.

If used, describe how each AI tool was used including giving the prompts used. You can submit without the prompts but OED may request further information before accepting the PR.

NA

List of GitHub IDs for anyone working on or contributing to this PR that you cannot certify has followed this policy. Any conttributors not listed are being certified by you to have complied with the terms of the OED AI policy shown below.

None

OED AI policy

Given the expanding usage of AI and review of submissions to OED, OED is developing an AI policy. OED is working on a more general solution but for now OED has this AI policy and each submission is required to:

  • Disclose of all AI tools used including version.
  • Disclose how each AI tool was used including giving the prompts used.
  • By checking the AI policy checkbox above, you certify that you have the copyright to the submitted work. Given recent court cases, this will require that less than 50% of the code and documentation came from AI. Less is even better. Every AI system you use must be checked that they provide you with copyright to this work that is compatible with the OED project MPL license.
  • By checking the AI policy checkbox above, you certify that you have personally reviewed all code and properly tested it. Thus, you are taking personal responsibility for the quality of the submission by guaranteeing you carefully reviewed every aspect in detail.
    If more than one person is involved in a request, then each person on the team needs to certify and provide the information above. If you don't list anyone else in the section above on unknown compliance with this policy then you must have personal knowledge that the other team members provided you with the necessary information and you are faithfully providing or indicate the answers only apply for yourself or a given list of authors (by GitHub ID).

Other items may be added in the future. It is also open to change given input, new information or changes in the AI sphere and you can make comments/suggestions to the OED project.

BunnyBea83 and others added 19 commits August 4, 2026 14:38
- 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
- Added conversion.delete.suffix.conversions.to.delete prompt for conversion deletion 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
- 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.
If there is a suffix the unit type should be unit and not suffix.
@huss

huss commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

The PR is being closed since it is replaced by PR #1729. That PR covers more items and is no longer tied to another PR. The changes in the PR were updated in that PR but carried over.

@huss huss closed this Oct 7, 2026
@huss
huss deleted the fixSuffixUI branch October 7, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

2 participants