diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index f677e204f5..09ba70d8b8 100644 --- a/src/client/app/components/conversion/EditConversionModalComponent.tsx +++ b/src/client/app/components/conversion/EditConversionModalComponent.tsx @@ -22,7 +22,7 @@ import ConfirmActionModalComponent from '../ConfirmActionModalComponent'; import { showErrorNotification, showSuccessNotification } from '../../utils/notifications'; import TooltipMarkerComponent from '../TooltipMarkerComponent'; import { SimpleUnsavedWarningComponent } from '../SimpleUnsavedWarningComponent'; - +import { conversionArrow } from '../../utils/conversionArrow'; interface EditConversionModalComponentProps { show: boolean; @@ -95,11 +95,19 @@ export default function EditConversionModalComponent(props: EditConversionModalC return source?.typeOfUnit === UnitType.meter; }; + // Determine whether the selected unit is a Suffix unit + // Including Suffix input with unit type Unit and Suffix is filled in + const isSuffixRelated = (unit?: UnitData): boolean => { + return !!unit && ( + unit.typeOfUnit === UnitType.suffix || unit.suffix !== '' + ); + }; + // Determine whether the selected source or destination is a suffix unit const isSuffixUsed = () => { const source = unitDataById[state.sourceId]; const dest = unitDataById[state.destinationId]; - return source?.typeOfUnit === UnitType.suffix || dest?.typeOfUnit === UnitType.suffix; + return isSuffixRelated(source) || isSuffixRelated(dest); }; /** @@ -128,12 +136,15 @@ export default function EditConversionModalComponent(props: EditConversionModalC } return count; }; + // Performs checks to warn the admin of the impact deleting a conversion will have on meter units and possible graphing units. const checkState = async () => { const source = unitDataById[state.sourceId]; const dest = unitDataById[state.destinationId]; const msgElements: React.ReactNode[] = []; let cancel = false; + // Use to label OED-created units + let suffixTypeUnitsToDeleteIds: Set = new Set(); // Meter source orphan check if (source.typeOfUnit === UnitType.meter) { @@ -205,34 +216,159 @@ export default function EditConversionModalComponent(props: EditConversionModalC } // Unit-to-unit orphan check - if (source.typeOfUnit === UnitType.unit && dest.typeOfUnit === UnitType.unit) { + if (source.typeOfUnit === UnitType.unit && dest.typeOfUnit === UnitType.unit + && !isSuffixRelated(source) && !isSuffixRelated(dest)) { + // Find conversions to destination or source of bidirectional conversion const destConversions = conversionDetails.filter(conversion => (conversion.destinationId === dest.id) || (conversion.bidirectional && conversion.sourceId === dest.id) ); - + // Exclude conversion being deleted + // Find remaining connections to destination unit afterward const remainingDestConversions = destConversions.filter(conversion => !(conversion.sourceId === source.id && conversion.destinationId === dest.id) ); + // Display units that will be orphaned by conversion deletion if (remainingDestConversions.length === 0) { msgElements.push(
{translate('conversion.delete.unit.orphan')} - "{dest.name}". + +
+ ); + } + // Suffix unit to Suffix Unit deletion + } else if (isSuffixRelated(source) || isSuffixRelated(dest)) { + const suffixUnit = isSuffixRelated(source) ? source : dest; + // Find all conversions involving this suffix unit (as source or destination) + const suffixUnitConversions = conversionDetails.filter(c => + c.sourceId === suffixUnit.id || + (c.bidirectional && c.destinationId === suffixUnit.id) + ); + + // Find conversions involving the given suffix unit + // that connect to another suffix-related unit. + const conversionsToDelete = suffixUnitConversions.filter(c => { + const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; + return isSuffixRelated(unitDataById[otherId]); + }); + + // Find the suffix-related units of conversionsToDelete. + // Return the units that may be cascade deleted. + const suffixTypeUnitsToDelete = conversionsToDelete + .map(c => { + const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; + return unitDataById[otherId]; + }); + // Populate computed units + suffixTypeUnitsToDeleteIds = new Set(suffixTypeUnitsToDelete.map(u => u.id)); + // Check for meters/groups using affected suffix units + const affectedSuffixUnitIds = new Set(suffixTypeUnitsToDeleteIds); + // Also check the main suffix unit + affectedSuffixUnitIds.add(suffixUnit.id); + + // Check meters using these units (as unitId or defaultGraphicUnit) + const affectedMetersList = Object.values(meterDataById).filter(meter => + affectedSuffixUnitIds.has(meter.unitId) || + affectedSuffixUnitIds.has(meter.defaultGraphicUnit) + ); + + // Check groups using these units (as defaultGraphicUnit) + const affectedGroupsList = Object.values(groupDataById).filter(group => + affectedSuffixUnitIds.has(group.defaultGraphicUnit) + ); + + // Display dependency warnings, affected meters and groups if any + if (affectedMetersList.length > 0 || affectedGroupsList.length > 0) { + msgElements.push( +
+ {translate('conversion.delete.suffix.dependencies.warning')} + {affectedMetersList.length > 0 && ( +
+ {translate('conversion.delete.suffix.meters.affected')}: +
    + {affectedMetersList.map(m => ( +
  • "{m.name}"
  • + ))} +
+
+ )} + {affectedGroupsList.length > 0 && ( +
+ {translate('conversion.delete.suffix.groups.affected')}: +
    + {affectedGroupsList.map(g => ( +
  • "{g.name}"
  • + ))} +
+
+ )} +
+ ); + } + + // Display units that will be cascade deleted + if (suffixTypeUnitsToDelete.length > 0) { + msgElements.push( +
+ {translate('conversion.delete.suffix.units.to.delete')}: + +
+ ); + } + + // List the conversions that will be cascade deleted + if (conversionsToDelete.length > 0) { + msgElements.push( +
+ {translate('conversion.delete.suffix.conversions.to.delete')}: +
); } } - // Only run simulation if the previous orphan check passed and it's unit-to-unit - if (source.typeOfUnit !== UnitType.suffix && dest.typeOfUnit === UnitType.unit && !cancel) { + // Run simulation if the previous orphan check passed and it's not cancelled + // Now supports suffix units since simulation has been enhanced + if (isSuffixRelated(source) && dest.typeOfUnit === UnitType.unit && !cancel) { try { const result = await triggerSimulate({ sourceId: state.sourceId, destinationId: state.destinationId }).unwrap(); + // Orphaned Suffix unit check + if (result.potentiallyOrphanedUnits?.length > 0) { + msgElements.push( +
+ {translate('conversion.delete.unit.orphan')}: + +
+ ); + } + // Orphaned groups check const orphanedGroups = result.affectedGroups?.filter(group => group.orphaned); if (orphanedGroups.length > 0) { @@ -249,6 +385,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC cancel = true; } else { // Group meters by lostUnits + // Display all meters that are losing the same set of units const meterLossMap = new Map(); result.affectedMeters.forEach(meter => { const key = JSON.stringify([...meter.lostUnits].sort()); @@ -257,26 +394,39 @@ export default function EditConversionModalComponent(props: EditConversionModalC } meterLossMap.get(key)!.push(meter.meterName); }); - meterLossMap.forEach((meterNames, lostUnitsKey) => { - const lostUnits = JSON.parse(lostUnitsKey).map(Number); + if (meterLossMap.size > 0) { msgElements.push( -
- {translate('conversion.delete.meter.affected')}: -
- {meterNames.map(name => ( -
"{name}"
- ))} +
+
+ {translate('conversion.delete.meter.affected')}:
- {translate('conversion.delete.lost.units')}: - {lostUnits.map((id: number, i: number) => ( - "{unitDataById[id]?.name || id}"{i < lostUnits.length - 1 ? ', ' : ''} - ))} -

+ {Array.from(meterLossMap.entries()).map(([lostUnitsKey, meterNames]) => { + const lostUnits = JSON.parse(lostUnitsKey).map(Number); + return ( +
+
+ {meterNames.map(name => ( +
"{name}"
+ ))} +
+ {translate('conversion.delete.lost.units')}: +
    + {lostUnits.map((id: number) => ( +
  • + "{unitDataById[id]?.name || id}" + {suffixTypeUnitsToDeleteIds.has(id) ? '(created by OED)' : ''} +
  • + ))} +
+
+ ); + })}
); - }); + } // Group non-orphaned groups by lostUnits + // Display all groups losing the same set of units const groupLossMap = new Map(); const nonOrphanedGroups = result.affectedGroups?.filter(group => !group.orphaned); nonOrphanedGroups.forEach(group => { @@ -286,24 +436,36 @@ export default function EditConversionModalComponent(props: EditConversionModalC } groupLossMap.get(key)!.push(group.groupName); }); - groupLossMap.forEach((groupNames, lostUnitsKey) => { - const lostUnits = JSON.parse(lostUnitsKey).map(Number); + if (groupLossMap.size > 0) { msgElements.push( -
- {translate('conversion.delete.group.affected')}: -
- {groupNames.map(name => ( -
"{name}"
- ))} +
+
+ {translate('conversion.delete.group.affected')}:
- {translate('conversion.delete.lost.units')}: - {lostUnits.map((id: number, i: number) => ( - "{unitDataById[id]?.name || id}"{i < lostUnits.length - 1 ? ', ' : ''} - ))} -

+ {Array.from(groupLossMap.entries()).map(([lostUnitsKey, groupNames]) => { + const lostUnits = JSON.parse(lostUnitsKey).map(Number); + return ( +
+
+ {groupNames.map(name => ( +
"{name}"
+ ))} +
+ {translate('conversion.delete.lost.units')}: +
    + {lostUnits.map((id: number) => ( +
  • + "{unitDataById[id]?.name || id}" + {suffixTypeUnitsToDeleteIds.has(id) ? ' (created by OED)' : ''} +
  • + ))} +
+
+ ); + })}
); - }); + } // After you get result from simulateDeleteConversion check default graphic units const metersLostDefault: number[] = []; @@ -342,10 +504,12 @@ export default function EditConversionModalComponent(props: EditConversionModalC ); } }); + // Save which meters.groups need their default unit cleared setMetersWithLostDefault(metersLostDefault); setGroupsWithLostDefault(groupsLostDefault); } } catch (e) { + // If the simulation fails block the delete msgElements.push(
{translate('conversion.delete.simulation.error')} @@ -354,25 +518,32 @@ export default function EditConversionModalComponent(props: EditConversionModalC cancel = true; } } - + // Process the appropriate modal based on the cancel conditions set above if (cancel) { msgElements.push(

- {translate('conversion.delete.restricted')} + + {translate('conversion.delete.restricted')} +
); setDeleteConfirmationMessage(msgElements); handleCancelModalOpen(); } else { + // Confirm deletion if no blocking issues msgElements.push(
- {translate('conversion.delete.conversion')} [{props.conversionIdentifier}] ? +
+ + {translate('conversion.delete.conversion')} [{props.conversionIdentifier}] ? +
); setDeleteConfirmationMessage(msgElements); handleDeleteConfirmationModalOpen(); } + }; /* Confirm Delete Modal */ // Separate from state comment to keep everything related to the warning confirmation modal together @@ -399,7 +570,8 @@ export default function EditConversionModalComponent(props: EditConversionModalC // Closes the warning modal // Do not call the handler function because we do not want to open the parent modal setShowDeleteConfirmationModal(false); - + // Bundle the conversions, meters, and groups to delete + // Backend will clear the references as part of the same delete const payload = { sourceId: state.sourceId, destinationId: state.destinationId, @@ -407,6 +579,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC groupIds: groupsWithLostDefault }; deleteConversion(payload) + // Send delete request .unwrap() .then(() => { // Show source/destination identifiers (not numeric IDs) @@ -416,6 +589,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC ', ' + translate('conversion.destination') + ' "' + unitDataById[payload.destinationId]?.identifier + '")' ); }).catch(error => { + // Show any backend error messages alongside identifiers showErrorNotification( translate('conversion.delete.failure') + ' (' + translate('conversion.source') + ' "' + unitDataById[payload.sourceId]?.identifier + '"' + diff --git a/src/client/app/styles/modal.css b/src/client/app/styles/modal.css index c8a6682a54..ac8e5e2be4 100644 --- a/src/client/app/styles/modal.css +++ b/src/client/app/styles/modal.css @@ -25,4 +25,20 @@ select option[value="true"] { .bold { font-weight: bold; +} + +.lost-units-section-heading { + border-bottom: 1px solid #ddd; + padding-bottom: 8px; + margin-bottom: 8px; +} + +.lost-units-group { + margin-top: 12px; + padding-bottom: 8px; + border-bottom: 1px solid #ddd; +} + +.lost-units-group:last-child { + border-bottom: none; } \ No newline at end of file diff --git a/src/client/app/translations/data.ts b/src/client/app/translations/data.ts index e5d757726c..8ff3e82ca0 100644 --- a/src/client/app/translations/data.ts +++ b/src/client/app/translations/data.ts @@ -98,7 +98,12 @@ const LocaleTranslationData = { "conversion.delete.group.default.lost": "Group default graphic unit lost", "conversion.delete.restricted": "This conversion cannot be deleted until the significant consequences of doing this are addressed.", "conversion.delete.simulation.error": "There was an error simulating the deletion of this conversion. Please try again later.", + "conversion.delete.suffix.conversions.to.delete": "The following conversions will also be deleted", + "conversion.delete.suffix.dependencies.warning": "Warning: Some meters or groups depend on units that will be affected", "conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix", + "conversion.delete.suffix.groups.affected": "Affected groups", + "conversion.delete.suffix.meters.affected": "Affected meters", + "conversion.delete.suffix.units.to.delete": "The following units will also be deleted", "conversion.delete.unit": "Deleting this conversion between two units of type unit may reduce the possible graphing units for some meter(s) but cannot be known until after the deletion happens.", "conversion.delete.unit.orphan": "Deleting this unit conversion will orphan unit", "conversion.destination": "Destination:", @@ -716,7 +721,12 @@ const LocaleTranslationData = { "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.simulation.error": "There was an error simulating the deletion of this conversion. Please try again later.\u{26A1}", - "conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix\u{26A1}", + "conversion.delete.suffix.conversions.to.delete": "The following conversions will also be deleted.\u{26A1}", + "conversion.delete.suffix.dependencies.warning": "Warning: Some meters or groups depend on units that will be affected.\u{26A1}", + "conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix.\u{26A1}", + "conversion.delete.suffix.groups.affected": "Affected groups\u{26A1}", + "conversion.delete.suffix.meters.affected": "Affected meters\u{26A1}", + "conversion.delete.suffix.units.to.delete": "The following units will also be deleted\u{26A1}", "conversion.delete.unit": "Deleting this conversion between two units of type unit may reduce the possible graphing units for some meter(s) but cannot be known until after the deletion happens.\u{26A1}", "conversion.delete.unit.orphan": "Deleting this unit conversion will orphan unit\u{26A1}", "conversion.destination": "Destination:\u{26A1}", @@ -1334,7 +1344,12 @@ const LocaleTranslationData = { "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.simulation.error": "There was an error simulating the deletion of this conversion. Please try again later.\u{26A1}", - "conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix\u{26A1}", + "conversion.delete.suffix.conversions.to.delete": "The following conversions will also be deleted.\u{26A1}", + "conversion.delete.suffix.dependencies.warning": "Warning: Some meters or groups depend on units that will be affected.\u{26A1}", + "conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix.\u{26A1}", + "conversion.delete.suffix.groups.affected": "Affected groups\u{26A1}", + "conversion.delete.suffix.meters.affected": "Affected meters\u{26A1}", + "conversion.delete.suffix.units.to.delete": "The following units will also be deleted\u{26A1}", "conversion.delete.unit": "Deleting this conversion between two units of type unit may reduce the possible graphing units for some meter(s) but cannot be known until after the deletion happens.\u{26A1}", "conversion.delete.unit.orphan": "Deleting this unit conversion will orphan unit\u{26A1}", "conversion.destination": "Destinación", diff --git a/src/client/app/types/redux/conversions.ts b/src/client/app/types/redux/conversions.ts index b4a2619abc..43c15ce495 100644 --- a/src/client/app/types/redux/conversions.ts +++ b/src/client/app/types/redux/conversions.ts @@ -23,6 +23,7 @@ export interface ConversionsState { export interface SimulateDeleteResult { affectedMeters: SimulateDeleteAffectedMeter[]; affectedGroups: SimulateDeleteAffectedGroup[]; + potentiallyOrphanedUnits: SimulateDeletePotentiallyOrphanedUnit[]; } export interface SimulateDeleteAffectedMeter { @@ -36,4 +37,9 @@ export interface SimulateDeleteAffectedGroup { groupName: string; lostUnits: number[]; orphaned: boolean; +} + +interface SimulateDeletePotentiallyOrphanedUnit { + id: number; + name: string; } \ No newline at end of file diff --git a/src/server/models/Cik.js b/src/server/models/Cik.js index c32eaf033e..8d8efef68c 100644 --- a/src/server/models/Cik.js +++ b/src/server/models/Cik.js @@ -52,6 +52,15 @@ class Cik { return rows.map(Cik.mapRow); } + /** + * Deletes every row in the cik table. Cik is empty until redoCik is run, + * so the caller must call redoCik before the overall process finishes. + * @param {*} conn The connection to use. + */ + static async deleteAll(conn) { + await conn.none(sqlFile('cik/delete_all_conversions.sql')); + } + /** * Inserts each element of the array with an actual conversion into the cik table. * The current values in the table are removed first. diff --git a/src/server/models/Conversion.js b/src/server/models/Conversion.js index 37ce9c0a20..5deba61420 100644 --- a/src/server/models/Conversion.js +++ b/src/server/models/Conversion.js @@ -66,6 +66,17 @@ class Conversion { return row === null ? null : Conversion.mapRow(row); } + /** + * Returns source, destination, and directionality of a conversion + * if the conversion is associated with the given unit + * @param {*} unitId The unit's id. + * @param {*} conn The connection to use. + * @returns {Promise.} + */ + static async getByUnitID(unitId, conn) { + return await conn.any(sqlFile('conversion/get_conversions_by_unit_id.sql'), { unitId }); + } + /** * Inserts a new conversion to the database. * @param {*} conn The connection to use. diff --git a/src/server/models/Group.js b/src/server/models/Group.js index ce90087c74..39c87da2e1 100644 --- a/src/server/models/Group.js +++ b/src/server/models/Group.js @@ -106,6 +106,16 @@ class Group { return Group.mapRow(row); } + /** + * Gets all groups that use the given unit as their default graphic unit. + * @param {number} unitId The unit's id. + * @param {*} conn The connection to use. + * @returns {Promise.} + */ + static async getByDefaultGraphicUnit(unitId, conn) { + return await conn.any(sqlFile('group/get_group_by_default_graphic_unit.sql'), { unitId }); + } + /** * returns a promise to retrieve all groups in the database * @param conn the connection to be used. @@ -243,6 +253,15 @@ class Group { await conn.none(sqlFile('group/update_group.sql'), group); } + /** + * Clears a group's default graphic unit (sets it to NULL). + * @param {number} id The group's id. + * @param {*} conn The connection to use. + */ + static async clearDefaultGraphicUnit(id, conn) { + await conn.none(sqlFile('group/clear_group_default_graphic_unit.sql'), { id: id }); + } + /** * Returns a promise to remove the group with childID from the children of this group. * @param childID The child group to be disowned diff --git a/src/server/models/Meter.js b/src/server/models/Meter.js index 739283e102..293db0582b 100644 --- a/src/server/models/Meter.js +++ b/src/server/models/Meter.js @@ -129,6 +129,16 @@ class Meter { return Meter.mapRow(row); } + /** + * Gets all meters that use the given unit as either their base unit or default graphic unit. + * @param {number} unitId The unit's id. + * @param {*} conn The connection to use. + * @returns {Promise.} + */ + static async getByUnitOrDefaultGraphic(unitId, conn) { + return await conn.any(sqlFile('meter/get_meter_by_unit_or_default_graphic.sql'), { unitId }); + } + /** * Check if a meter with the same name is already in the database. * @param conn the connection to be used. @@ -303,6 +313,24 @@ class Meter { const resp = await conn.one(sqlFile('meter/update_meter.sql'), meter); return resp.reading_frequency; } + + /** + * Clears a meter's default graphic unit (sets it to NULL). + * @param {number} id The meter's id. + * @param {*} conn The connection to use. + */ + static async clearDefaultGraphicUnit(id, conn) { + await conn.none(sqlFile('meter/clear_meter_default_graphic_unit.sql'), { id: id }); + } + + /** + * Clears a meter's base unit (sets unit_id to NULL). + * @param {number} id The meter's id. + * @param {*} conn The connection to use. + */ + static async clearUnitId(id, conn) { + await conn.none(sqlFile('meter/clear_meter_unit_id.sql'), { id: id }); + } /** * Returns a promise to get all of the readings for this meter from the database. diff --git a/src/server/models/Unit.js b/src/server/models/Unit.js index f414716592..e51151474b 100644 --- a/src/server/models/Unit.js +++ b/src/server/models/Unit.js @@ -186,6 +186,40 @@ class Unit { return Unit.mapRow(row); } + /** + * Checks whether a unit with the given id currently exists. + * @param {number} id The unit's id. + * @param {*} conn The connection to use. + * @returns {Promise.} + */ + static async exists(id, conn) { + const row = await conn.oneOrNone(sqlFile('unit/get_by_id.sql'), { id: id }); + return row !== null; + } + + /** + * Locks a unit's row for the duration of the current transaction, preventing + * concurrent modification by other sessions. Must be called within a transaction. + * @param {number} id The unit's id. + * @param {*} conn The connection to use (should be a transaction). + * @returns {Promise.} + */ + static async lockById(id, conn) { + const row = await conn.one(sqlFile('unit/lock_unit_by_id.sql'), { id: id }); + return Unit.mapRow(row); + } + + /** + * 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. + * @returns {Promise.>} + */ + static async findOrphanedUnits(conn, limit = 100) { + return await conn.any(sqlFile('unit/find_orphaned_units.sql'), { limit }); + } + // TODO: Returns a special value if it doesn't exist /** * Returns the associated unit for the given name. diff --git a/src/server/routes/conversions.js b/src/server/routes/conversions.js index dd719c6052..32fd895864 100644 --- a/src/server/routes/conversions.js +++ b/src/server/routes/conversions.js @@ -6,13 +6,20 @@ const express = require('express'); const { log } = require('../log'); const { getConnection } = require('../db'); const Conversion = require('../models/Conversion'); +const Unit = require('../models/Unit'); +const { removeAdditionalConversionsAndUnits } = require('../services/graph/handleSuffixUnits'); const { success, failure } = require('./response'); const { HTTP_CODES } = require('../util/httpCodes'); const validate = require('jsonschema').validate; const { simulateDeleteConversion } = require('../services/conversionSimulation'); +const { getUnitDependencyDetails } = require('../services/graph/checkUnitDependencies'); const { adminAuthMiddleware, optionalAuthMiddleware } = require('./authenticator'); const { STRING_GENERAL_MAX_LENGTH } = require('../util/validationConstants'); +const Meter = require('../models/Meter'); +const Group = require('../models/Group'); +const { redoCik } = require('../services/graph/redoCik'); +const { isSuffixRelated } = require('../util/suffixUnitCheck'); const router = express.Router(); @@ -180,22 +187,125 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re const { sourceId, destinationId, meterIds = [], groupIds = [] } = req.body; const conn = getConnection(); try { + // Get the source and destination units for the conversion (before transaction for validation) + const source = await Unit.getById(sourceId, conn); + const dest = await Unit.getById(destinationId, conn); + + if (!source || !dest) { + failure(res, 404, 'Source or destination unit not found'); + return; + } + + // Check for dependencies on suffix units that will be affected + // Accounts for Suffix Inputs where unit = unit & and Suffix contains a string + // This provides better error messages before attempting deletion + const suffixUnitsToCheck = []; + if (isSuffixRelated(source)) { + suffixUnitsToCheck.push({ unit: source, role: 'source' }); + } + if (isSuffixRelated(dest)) { + suffixUnitsToCheck.push({ unit: dest, role: 'destination' }); + } + + // Check dependencies for each suffix unit that will be cleaned up + for (const { unit, role } of suffixUnitsToCheck) { + const deps = await getUnitDependencyDetails(unit.id, conn); + + // If suffix unit has meter/group dependencies, provide detailed error + if (deps.meterCount > 0 || deps.groupCount > 0) { + const meterNames = deps.meters.map(m => `"${m.name}"`).join(', '); + const groupNames = deps.groups.map(g => `"${g.name}"`).join(', '); + + let errorMsg = `Cannot delete conversion: ${role} unit "${unit.name}" (ID: ${unit.id}) is used by `; + const parts = []; + if (deps.meterCount > 0) { + parts.push(`${deps.meterCount} meter(s): ${meterNames}`); + } + if (deps.groupCount > 0) { + parts.push(`${deps.groupCount} group(s): ${groupNames}`); + } + errorMsg += parts.join(' and '); + + log.warn(`Conversion deletion blocked due to dependencies: ${errorMsg}`); + failure(res, 400, errorMsg); + return; + } + } + + // Perform all operations in a single transaction for atomicity await conn.tx(async t => { - // Update meters if any + // Lock the units to prevent concurrent modifications + await Unit.lockById(sourceId, t); + await Unit.lockById(destinationId, t); + + // Check if the source or the destination is a suffix unit and clean up related conversions/units + // Accounts for Suffix Inputs where unit = unit & and Suffix contains a string + if (isSuffixRelated(source)) { + log.info(`Suffix-related unit ${sourceId} is used in conversion deletion as source. Cleaning up related conversions and units.`); + const sourceInTx = await Unit.getById(sourceId, t); + await removeAdditionalConversionsAndUnits(sourceInTx, t); + } + if (isSuffixRelated(dest)) { + const destStillExists = await Unit.exists(destinationId, t); + if (destStillExists) { + log.info(`Suffix-related unit ${destinationId} is used in conversion deletion as destination. Cleaning up related conversions and units.`); + const destInTx = await Unit.getById(destinationId, t); + await removeAdditionalConversionsAndUnits(destInTx, t); + } else { + log.info(`Suffix-related unit ${destinationId} was already cleaned up as part of source unit ${sourceId}'s cascade.`); + } + } + + // Handle bidirectional conversion deletion safely + // Check if this conversion is bidirectional and if reverse exists + const conversion = await Conversion.getBySourceDestination(sourceId, destinationId, t); + if (conversion && conversion.bidirectional) { + // Check for reverse conversion (some systems store bidirectional as two entries) + const reverseConversion = await Conversion.getBySourceDestination(destinationId, sourceId, t); + if (reverseConversion) { + log.info(`Deleting bidirectional conversion pair: ${sourceId} <-> ${destinationId}`); + await Conversion.delete(destinationId, sourceId, t); + } + } + // Update meters, if any, by setting their default graphic unit to NULL for (const meterId of meterIds) { - await t.none(`UPDATE meters SET default_graphic_unit = NULL WHERE id = ${meterId}`); + await Meter.clearDefaultGraphicUnit(meterId, t); } - // Update groups if any + // Update groups, if any, by setting their default graphic unit to NULL for (const groupId of groupIds) { - await t.none(`UPDATE groups SET default_graphic_unit = NULL WHERE id = ${groupId}`); + await Group.clearDefaultGraphicUnit(groupId, t); } // Delete conversion await Conversion.delete(sourceId, destinationId, t); }); + + // Verify if units were orphaned after suffix cleanup (performance: only check if suffix units were involved) + // Track the units that may be orphaned + let orphanedUnits = []; + if (isSuffixRelated(source) || isSuffixRelated(dest)) { + try { + // Use efficient query with LIMIT to avoid scanning large tables unnecessarily + // Only check recently affected units to improve performance on large databases + orphanedUnits = await Unit.findOrphanedUnits(conn); + if (orphanedUnits.length > 0) { + log.warn(`Found ${orphanedUnits.length} potentially orphaned suffix units after cleanup: ${orphanedUnits.map(u => `${u.id} (${u.name})`).join(', ')}`); + // Log metrics for monitoring + log.info(`Conversion deletion metrics: sourceId=${sourceId}, destId=${destinationId}, orphanedUnits=${orphanedUnits.length}`); + } + } catch (err) { + // Non-critical orphan unit check, log but don't fail + log.warn(`Error checking for orphaned units. Could not successfully verify if units were orphaned by this deletion : ${err}`); + } + } + // 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); + success(res, 'Successfully deleted conversion and updated meters/groups'); } catch (err) { - log.error(`Error while deleting conversion and updating meters/groups: ${err}`); - failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while deleting conversion and updating meters/groups: ${err}`); + log.error(`Error while deleting conversion and updating meters/groups: ${err}`, err); + failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while deleting conversion and updating meters/groups: ${err.message || err}`); } } }); diff --git a/src/server/routes/units.js b/src/server/routes/units.js index 359517bb06..c0bfc83ae9 100644 --- a/src/server/routes/units.js +++ b/src/server/routes/units.js @@ -7,12 +7,15 @@ const { adminAuthMiddleware, optionalAuthMiddleware } = require('./authenticator const { log } = require('../log'); const { getConnection } = require('../db'); const Unit = require('../models/Unit'); +const Conversion = require('../models/Conversion'); const { removeAdditionalConversionsAndUnits } = require('../services/graph/handleSuffixUnits'); const validate = require('jsonschema').validate; const { success, failure } = require('./response'); const { HTTP_CODES } = require('../util/httpCodes'); const { STRING_GENERAL_MAX_LENGTH, STRING_SHORT_MAX_LENGTH } = require('../util/validationConstants'); const router = express.Router(); +const { checkUnitDependencies } = require('../services/graph/checkUnitDependencies'); +const { redoCik } = require('../services/graph/redoCik'); function formatUnitForResponse(unit) { return { @@ -219,20 +222,99 @@ router.post('/delete', adminAuthMiddleware('delete units'), async (req, res) => const errorMsg = `Got request to delete a unit with invalid data, error(s): ${validatorResult.errors}`; log.warn(errorMsg); failure(res, HTTP_CODES.BAD_REQUEST, errorMsg); - } else { + return; + } const conn = getConnection(); const unitId = req.body.id; try { - // Don't worry about checking if the unit already exists - // Just try to delete it to save the extra database call, since the database will return an error anyway if the row does not exist - await Unit.delete(unitId, conn); + const unit = await Unit.getById(req.body.id, conn); + if (!unit) { + failure(res, HTTP_CODES.NOT_FOUND, 'Unit not found'); + return; + } + // Check for dependencies before deletion + const deps = await checkUnitDependencies(unitId, conn); + + // If unit has meter/group dependencies, provide detailed error + if (deps.meters.length > 0 || deps.groups.length > 0) { + const meterNames = deps.meters.map(m => `"${m.name}"`).join(', '); + const groupNames = deps.groups.map(g => `"${g.name}"`).join(', '); + + let errorMsg = `Cannot delete unit "${unit.name}" (ID: ${unitId}): used by `; + const parts = []; + if (deps.meters.length > 0) { + parts.push(`${deps.meters.length} meter(s): ${meterNames}`); + } + if (deps.groups.length > 0) { + parts.push(`${deps.groups.length} group(s): ${groupNames}`); + } + errorMsg += parts.join(' and '); + + log.warn(`Unit deletion blocked due to dependencies: ${errorMsg}`); + failure(res, 400, errorMsg); + return; + } + + // Perform all operations in a single transaction for atomicity + await conn.tx(async t => { + // Lock the unit to prevent concurrent modifications + await Unit.lockById(unitId, t); + + // Reload unit within transaction to ensure consistency + const unitInTx = await Unit.getById(unitId, t); + + // Handle suffix-type units (created by OED) + if (unitInTx.typeOfUnit === 'suffix') { + log.info(`Deleting suffix-type unit ${unitId}. Cleaning up associated units and conversions.`); + await removeAdditionalConversionsAndUnits(unitInTx, t); + } + + // Handle units with suffix string (not suffix-type) + // These units may have conversions involving suffix units that need cleanup + if (unitInTx.suffix && unitInTx.suffix !== '') { + log.info(`Unit ${unitId} has suffix "${unitInTx.suffix}". Checking for suffix-involved conversions.`); + + // Get all conversions involving this unit + const allConversions = await Conversion.getAll(t); + const relatedConversions = allConversions.filter(c => + c.sourceId === unitId || c.destinationId === unitId + ); + + // Delete bidirectional conversions safely + for (const conversion of relatedConversions) { + // Delete the conversion + await Conversion.delete(conversion.sourceId, conversion.destinationId, t); + + // If bidirectional, also delete reverse if it exists separately + if (conversion.bidirectional) { + const reverseConversion = await Conversion.getBySourceDestination( + conversion.destinationId, + conversion.sourceId, + t + ); + if (reverseConversion) { + await Conversion.delete(conversion.destinationId, conversion.sourceId, t); + log.info(`Deleted bidirectional conversion pair: ${conversion.sourceId} <-> ${conversion.destinationId}`); + } + } + } + } + + // Don't worry about checking if the unit already exists + // Just try to delete it to save the extra database call, since the database will return an error anyway if the row does not exist + await Unit.delete(unitId, t); + }); + // 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); + success(res, 'Successfully deleted unit'); } catch (err) { const errorMsg = `Error while deleting unit with error(s): ${err}`; log.error(errorMsg); failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, errorMsg); } - } }); module.exports = router; diff --git a/src/server/services/conversionSimulation.js b/src/server/services/conversionSimulation.js index e5901c6e71..10a52e0249 100644 --- a/src/server/services/conversionSimulation.js +++ b/src/server/services/conversionSimulation.js @@ -9,7 +9,55 @@ const Group = require('../models/Group'); const Unit = require('../models/Unit'); const { createConversionGraph, createConversionGraphFromArray } = require('./graph/createConversionGraph'); const { intersectSets, compatibleUnitsForMeter } = require('../util/compatibleUnits'); +const { isSuffixRelated } = require('../util/suffixUnitCheck'); +/** + * Simulates what conversions and units would be removed when deleting a conversion involving suffix units. + * This mirrors the logic in removeAdditionalConversionsAndUnits but without actually deleting. + * @param {*} suffixUnit The suffix unit to check + * @param {*} allConversions All conversions in the system + * @param {*} allUnits All units in the system + * @returns {Object} Object with arrays of conversionIds and unitIds that would be removed + */ +function simulateSuffixUnitCleanup(suffixUnit, allConversions, allUnits) { + const conversionsToRemove = []; + const unitsToDelete = []; + + // Find all conversions involving this suffix unit + const relatedConversions = allConversions.filter((conversion) => + conversion.sourceId === suffixUnit.id || + (conversion.bidirectional && (conversion.sourceId === suffixUnit.id || conversion.destinationId === suffixUnit.id)) + ); + + // Process each related conversion + for (const conversion of relatedConversions) { + const isSource = conversion.sourceId === suffixUnit.id; + const otherUnitId = isSource ? conversion.destinationId : conversion.sourceId; + const otherUnit = allUnits.find(u => u.id === otherUnitId); + + // If the other unit is also a suffix unit (created by OED), it would be hidden + if (otherUnit && otherUnit.typeOfUnit === Unit.unitType.SUFFIX) { + conversionsToRemove.push({ + sourceId: conversion.sourceId, + destinationId: conversion.destinationId + }); + + // If bidirectional, also mark reverse for removal + if (conversion.bidirectional) { + conversionsToRemove.push({ + sourceId: conversion.destinationId, + destinationId: conversion.sourceId + }); + } + + if (!unitsToDelete.includes(otherUnitId)) { + unitsToDelete.push(otherUnitId); + } + } + } + + return { conversionsToRemove, unitsToDelete}; +} async function simulateDeleteConversion({ sourceId, destinationId }, conn) { // 1. Load all data @@ -20,20 +68,73 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { Group.getAll(conn) ]); - // 2. Remove the conversion to be deleted - const newConversions = allConversions.filter(c => - !(c.sourceId === sourceId && c.destinationId === destinationId) + // 2. Get source and destination units to check for suffix units + const sourceUnit = allUnits.find(u => u.id === sourceId); + const destUnit = allUnits.find(u => u.id === destinationId); + + // 3. Simulate suffix unit cleanup if applicable + let conversionsToRemove = [ + // The conversion being deleted + { sourceId, destinationId } + ]; + // Track units that will be deleted + let allUnitsToDelete = []; + + // Validate if the source unit is a suffix related unit + // Accounts for Suffix Inputs where unit = unit & and Suffix contains a string + if (isSuffixRelated(sourceUnit)) { + const cleanup = simulateSuffixUnitCleanup(sourceUnit, allConversions, allUnits); + conversionsToRemove.push(...cleanup.conversionsToRemove); + allUnitsToDelete.push(...cleanup.unitsToDelete); + } + // Validate if the destination unit is a suffix related unit + // Accounts for Suffix Inputs where unit = unit & and Suffix contains a string + if (isSuffixRelated(destUnit)) { + const cleanup = simulateSuffixUnitCleanup(destUnit, allConversions, allUnits); + conversionsToRemove.push(...cleanup.conversionsToRemove); + allUnitsToDelete.push(...cleanup.unitsToDelete); + } + + // Remove duplicates (in case both source and dest are suffix units and share conversions) + const uniqueConversionsToRemove = Array.from( + new Map(conversionsToRemove.map(c => [`${c.sourceId}-${c.destinationId}`, c])).values() ); + + // 4. Remove all affected conversions from the simulation + const newConversions = allConversions.filter(c => { + // Remove conversions explicitly identified for removal + // (e.g The deleted conversion, it's parent -> link) + const isExplicitlyRemoved = uniqueConversionsToRemove.some(toRemove => + c.sourceId === toRemove.sourceId && c.destinationId === toRemove.destinationId + ); + // Remove conversions touching the units being deleted. + // `uniqueConversionsToRemove` doesn't catch unit conversions + // beyond a units direct parent link. This resolves the missed detection. + const touchesDeletedUnit = allUnitsToDelete.includes(c.sourceId) || allUnitsToDelete.includes(c.destinationId); + return !isExplicitlyRemoved && !touchesDeletedUnit; + }); + + // Check which units would have zero remaining conversions after the simulated delete. + // Mirrors `Unit.findOrphanedUnits` logic but against the simulation. + // Filter units that are created by OED, currently visible, and have zero conversions post delete. + const potentiallyOrphanedUnits = allUnits.filter(u => + u.displayable !== Unit.displayableType.NONE && + !allUnitsToDelete.includes(u.id) && + // Only report units this delete would leave without conversions, not ones that already had none. + allConversions.some(c => c.sourceId === u.id || c.destinationId === u.id) && + !newConversions.some(c => c.sourceId === u.id || c.destinationId === u.id) + ).map(u => ({ id: u.id, name: u.name })); + - // 3. Build simulated graph and Cik array + // 5. Build simulated graph and Cik array const simulatedGraph = createConversionGraphFromArray(allUnits, newConversions); const simulatedCik = await createCikArray(simulatedGraph, conn); - // 4. Get the current Cik array + // 6. Get the current Cik array const currentGraph = await createConversionGraph(conn); const currentCik = await createCikArray(currentGraph, conn); - // 5. Precompute compatible units for each meter (current and simulated) + // 7. Precompute compatible units for each meter (current and simulated) const meterIdToUnitsCurrent = {}; const meterIdToUnitsSim = {}; for (const meter of allMeters) { @@ -41,13 +142,13 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { meterIdToUnitsSim[meter.id] = compatibleUnitsForMeter(meter.unitId, simulatedCik); } - // 6. Batch load all group-to-meter relationships + // 8. Batch load all group-to-meter relationships const groupIdToMeterIds = {}; await Promise.all(allGroups.map(async group => { groupIdToMeterIds[group.id] = await Group.getDeepMetersByGroupID(group.id, conn); })); - // 7. For each meter, compare compatible units before/after + // 9. For each meter, compare compatible units before/after const affectedMeters = []; for (const meter of allMeters) { const before = meterIdToUnitsCurrent[meter.id] || new Set(); @@ -62,7 +163,7 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { } } - // 8. For each group, intersect the sets (using cached meter compatible units) + // 10. For each group, intersect the sets (using cached meter compatible units) const affectedGroups = []; for (const group of allGroups) { const meterIds = groupIdToMeterIds[group.id]; @@ -84,7 +185,7 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { } } - return { affectedMeters, affectedGroups }; + return { affectedMeters, affectedGroups, potentiallyOrphanedUnits }; } module.exports = { simulateDeleteConversion }; diff --git a/src/server/services/graph/checkUnitDependencies.js b/src/server/services/graph/checkUnitDependencies.js new file mode 100644 index 0000000000..bc6a701db8 --- /dev/null +++ b/src/server/services/graph/checkUnitDependencies.js @@ -0,0 +1,125 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +const Meter = require('../../models/Meter'); +const Group = require('../../models/Group'); +const Conversion = require('../../models/Conversion'); +const Unit = require('../../models/Unit'); +const { log } = require('../../log'); +const Cik = require('../../models/Cik'); + +/** + * Checks if a unit has dependencies that prevent safe deletion. + * @param {number} unitId The unit ID to check + * @param {*} conn Database connection (can be a transaction) + * @returns {Promise} Object containing: + * - hasDependencies {boolean} Whether the unit has any dependencies + * - meters {Array} Meters using the unit + * - groups {Array} Groups using the unit + * - conversions {Array} Conversions using the unit + */ +async function checkUnitDependencies(unitId, conn) { + const [meters, groups, conversions] = await Promise.all([ + // Check if any meters use this unit (as unitId or default_graphic_unit) + Meter.getByUnitOrDefaultGraphic(unitId, conn), + // Check if any groups use this unit as default_graphic_unit + Group.getByDefaultGraphicUnit(unitId, conn), + // Check if unit is used in any conversions (as source or destination) + Conversion.getByUnitID(unitId,conn) + ]); + + return { + hasDependencies: meters.length > 0 || groups.length > 0 || conversions.length > 0, + meters, + groups, + conversions + }; +} + +/** + * Gets detailed dependency information for UI warnings. + * @param {number} unitId The unit ID to check + * @param {*} conn Database connection + * @returns {Promise} Object containing: + * - hasDependencies: boolean, + * - meterCount: number, + * - groupCount: number, + * - conversionCount: number, + * - meters: Array, + * - groups: Array, + * - conversions: Array + */ +async function getUnitDependencyDetails(unitId, conn) { + const deps = await checkUnitDependencies(unitId, conn); + + return { + hasDependencies: deps.hasDependencies, + meterCount: deps.meters.length, + groupCount: deps.groups.length, + conversionCount: deps.conversions.length, + meters: deps.meters.map(meter => ({ id: meter.id, name: meter.name })), + groups: deps.groups.map(group => ({ id: group.id, name: group.name })), + conversions: deps.conversions.map(conv => ({ + sourceId: conv.source_id, + destinationId: conv.destination_id, + bidirectional: conv.bidirectional + })) + }; +} + +/** + * Deletes a single unit, clearing all dependencies that would otherwise block it. + * If it's used as a meter's or group's default graphic unit, that reference is + * cleared first (cosmetic, safe to drop). If it's used as a meter's actual base + * unit (unit_id), that reference is also cleared (the meter is left without a + * defined unit) so the deletion can proceed. Any other conversions still + * referencing this unit are deleted as well. Cik is emptied first, so the + * caller must run redoCik before the overall process finishes. + * @param {number} unitId The unit to delete. + * @param {*} conn The connection to use (should be a transaction). + */ +async function deleteUnitSafely(unitId, conn) { + // Empty Cik first so it cannot be left partly correct and the foreign key on + // cik cannot block Unit.delete. The caller must run redoCik before the overall + // process finishes. Later calls find nothing to delete so they are fast. + await Cik.deleteAll(conn); + + const deps = await checkUnitDependencies(unitId, conn); + + // Clear any meter using this as its base unit or default graphic unit. + for (const meter of deps.meters) { + if (meter.unit_id === unitId) { + log.warn(`Clearing base unit for meter "${meter.name}" (ID: ${meter.id}) to allow deletion of unit ${unitId}.`); + await Meter.clearUnitId(meter.id, conn); + } + if (meter.default_graphic_unit === unitId) { + await Meter.clearDefaultGraphicUnit(meter.id, conn); + } + } + + // Clear any group using this as its default graphic unit. + for (const group of deps.groups) { + await Group.clearDefaultGraphicUnit(group.id, conn); + } + + // Delete any other conversions still referencing this unit. + // NOTE: This can orphan a downstream auto-created unit if the other side of + // a deleted conversion is itself a suffix-type unit that depended on this + // link. The orphaning of a unit is no longer silent to the admin/user + // and the delete simulation detects and reports this case to the admin. + // TODO: A full path re-walk (to prevent the orphaning) would be needed to detect + // and clean up that case. + for (const conv of deps.conversions) { + log.info(`Deleting conversion ${conv.source_id}->${conv.destination_id} to allow deletion of unit ${unitId}.`); + await Conversion.delete(conv.source_id, conv.destination_id, conn); + } + + await Unit.delete(unitId, conn); +} + +module.exports = { + checkUnitDependencies, + getUnitDependencyDetails, + deleteUnitSafely +}; \ No newline at end of file diff --git a/src/server/services/graph/handleSuffixUnits.js b/src/server/services/graph/handleSuffixUnits.js index df7b1ed477..3c000f0d79 100644 --- a/src/server/services/graph/handleSuffixUnits.js +++ b/src/server/services/graph/handleSuffixUnits.js @@ -6,6 +6,8 @@ const Unit = require('../../models/Unit'); const { pathConversion } = require('./pathConversion'); const Conversion = require('../../models/Conversion'); const { getAllPaths } = require('./createConversionGraph'); +const { log } = require('../../log'); +const { deleteUnitSafely } = require('./checkUnitDependencies'); /** * Adds the new unit and conversions to the database and the conversion graph. @@ -141,6 +143,9 @@ async function handleSuffixUnits(graph, conn) { // See if this unit already exists. Would if this was done before where this path existed. if (neededSuffixUnit === null) { // If not then add the new unit and conversion. + // Note: This regenerates a derived suffix unit/conversion whenever this function runs and finds one missing for a still valid path + // This includes if an admin has deleted it directly. Deleting a derived converion alone + // does not persis unless the underlying conversion it was generated from is also removed. await addNewUnitAndConversion(sourceId, destinationId, slope, intercept, unitName, unitIdentifier, graph, conn); } else { // If it already exists then check if the unit and conversion are correct. @@ -157,25 +162,116 @@ async function handleSuffixUnits(graph, conn) { /** * OED handles suffix units by adding conversions and units automatically. * When a unit's suffix changes, these additional conversions and units need to be removed. - * Units are complicated to remove so we just set their displayable to NONE. - * Since this function makes changes to conversions and units, Cik must be recalculated after calling this function. + * Each conversion and OED created unit is cleaned up independently. + * Cik must be recalculated after calling this function. * @param {*} suffixUnit Additional conversions/units of this suffixUnit will be removed. - * @param {*} conn The connection to use. + * @param {*} conn The connection to use (can be a transaction). + * @param {number} depth Current recursion depth to prevent infinite loops (default: 0). */ -async function removeAdditionalConversionsAndUnits(suffixUnit, conn) { - // Get all conversions from this suffix unit. - const conversions = (await Conversion.getAll(conn)).filter((conversion) => conversion.sourceId === suffixUnit.id); - conversions.forEach(async (conversion) => { - const destinationUnit = await Unit.getById(conversion.destinationId, conn); - // The units that OED adds are suffix unit. - if (destinationUnit.typeOfUnit === Unit.unitType.SUFFIX) { - // Delete the conversion. + +async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) { + const MAX_SUFFIX_CLEANUP_DEPTH = 10; + + if (depth > MAX_SUFFIX_CLEANUP_DEPTH) { + log.error(`Max depth (${MAX_SUFFIX_CLEANUP_DEPTH}) reached cleaning up suffix unit ${suffixUnit.id}. Possible circular dependency.`); + throw new Error(`Suffix unit cleanup depth limit exceeded for unit ${suffixUnit.id}`); + } + + // Repeat until no related conversions remain. The conversions are re-read on each + // pass because earlier passes delete units and conversions. + while (true) { + // Get all conversions involving this suffix unit (as source, destination, or bidirectional) + const allConversions = await Conversion.getAll(conn); + const relatedConversions = allConversions.filter((conversion) => + conversion.sourceId === suffixUnit.id || + (conversion.bidirectional && conversion.destinationId === suffixUnit.id) + ); + if (relatedConversions.length === 0) { + break; + } + + const resolved = await Promise.all(relatedConversions.map(async (conversion) => { + // Determine which unit is the suffix unit and which is the destination + const isSource = conversion.sourceId === suffixUnit.id; + const otherUnitId = isSource ? conversion.destinationId : conversion.sourceId; + const exists = await Unit.exists(otherUnitId, conn); + const otherUnit = exists ? await Unit.getById(otherUnitId, conn) : null; + return { conversion, otherUnitId, otherUnit }; + })); + + // Handle a conversion pointing at a unit that's already been deleted. + // This occurs when a bidirectional conversion is stored as two rows. + // Both rows point to the same otherUnitId. If an earlier processing pass deleted + // otherUnitId during the same batch, this lookup will correctly register it as + // missing rather than indicating an actual data problem. + const missing = resolved.find(r => !r.otherUnit); + if (missing) { + log.warn(`Unit ${missing.otherUnitId} not found when cleaning up suffix unit ${suffixUnit.id}. Conversion ${missing.conversion.sourceId}->${missing.conversion.destinationId} may be orphaned.`); + await Conversion.delete(missing.conversion.sourceId, missing.conversion.destinationId, conn); + continue; + } + + // Find the first related conversion whose other unit is actually a + // suffix-type unit this function is responsible for cleaning up. + // Conversions to non-suffix units (e.g. the original conversion the + // admin is deleting) are left alone rather than aborting the loop. + const target = resolved.find(r => r.otherUnit.typeOfUnit === Unit.unitType.SUFFIX); + + if (!target) { + // Nothing left to process for this unit. + break; + } + + // The units that OED adds are suffix units (typeOfUnit === SUFFIX) + try { + const { conversion, otherUnitId, otherUnit } = target; + + // Check if otherUnitId has connections besides this one. + // Deletion of other connections may lead to orphaning a unit. + const otherUnitConversions = await Conversion.getByUnitID(otherUnitId, conn); + const hasOtherConnections = otherUnitConversions.some(c => + !((c.source_id === conversion.sourceId && c.destination_id === conversion.destinationId) || + (c.source_id === conversion.destinationId && c.destination_id === conversion.sourceId)) + ); + // Warn the admin of connected units + if (hasOtherConnections) { + log.warn(`Unit ${otherUnitId} has other connections beyond conversion ${conversion.sourceId}->${conversion.destinationId}; deleting it will also remove those.`); + } + + // Always delete the conversion await Conversion.delete(conversion.sourceId, conversion.destinationId, conn); - // Hide the destination unit. - destinationUnit.displayable = Unit.displayableType.NONE; - await destinationUnit.update(conn); + + // If bidirectional, also delete the reverse conversion if it exists separately + if (conversion.bidirectional) { + const reverseConversion = await Conversion.getBySourceDestination( + conversion.destinationId, + conversion.sourceId, + conn + ); + if (reverseConversion) { + await Conversion.delete(conversion.destinationId, conversion.sourceId, conn); + } + } + + // Recursively clean up this unit's related conversions/units + // This handles nested suffix chains (A -> B -> C) + await removeAdditionalConversionsAndUnits(otherUnit, conn, depth + 1); + + // Delete the auto-created unit (dependency checks handled inside, Cik is emptied + // there and must be rebuilt with redoCik by the caller) + await deleteUnitSafely(otherUnitId, conn); + + } catch (err) { + log.error(`Error processing conversion ${target.conversion.sourceId}->${target.conversion.destinationId} during suffix unit cleanup: ${err}`, err); + throw err; } - }); + } + + // Restore the suffix unit's displayable status + // Note: This function only cleans up derived suffix conversions, never the original conversion the suffix unit was created from + // handleSuffixUnits will make the unit visible again and regenerate + // derived units from it during the next graph rebuild. + // See note there for the full mechanism. suffixUnit.displayable = Unit.displayableType.ALL; await suffixUnit.update(conn); } diff --git a/src/server/sql/conversion/get_conversions_by_unit_id.sql b/src/server/sql/conversion/get_conversions_by_unit_id.sql new file mode 100644 index 0000000000..7c766d623c --- /dev/null +++ b/src/server/sql/conversion/get_conversions_by_unit_id.sql @@ -0,0 +1,7 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +SELECT source_id, destination_id, bidirectional +FROM conversions +WHERE source_id = ${unitId} OR destination_id = ${unitId}; \ No newline at end of file diff --git a/src/server/sql/group/clear_group_default_graphic_unit.sql b/src/server/sql/group/clear_group_default_graphic_unit.sql new file mode 100644 index 0000000000..5302cb12d3 --- /dev/null +++ b/src/server/sql/group/clear_group_default_graphic_unit.sql @@ -0,0 +1,5 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +UPDATE groups SET default_graphic_unit = NULL WHERE id = ${id}; \ No newline at end of file diff --git a/src/server/sql/group/get_group_by_default_graphic_unit.sql b/src/server/sql/group/get_group_by_default_graphic_unit.sql new file mode 100644 index 0000000000..9a30ac90b1 --- /dev/null +++ b/src/server/sql/group/get_group_by_default_graphic_unit.sql @@ -0,0 +1,7 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +SELECT id, name, default_graphic_unit +FROM groups +WHERE default_graphic_unit = ${unitId}; \ No newline at end of file diff --git a/src/server/sql/meter/clear_meter_default_graphic_unit.sql b/src/server/sql/meter/clear_meter_default_graphic_unit.sql new file mode 100644 index 0000000000..cc11f89a97 --- /dev/null +++ b/src/server/sql/meter/clear_meter_default_graphic_unit.sql @@ -0,0 +1,5 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +UPDATE meters SET default_graphic_unit = NULL WHERE id = ${id}; \ No newline at end of file diff --git a/src/server/sql/meter/clear_meter_unit_id.sql b/src/server/sql/meter/clear_meter_unit_id.sql new file mode 100644 index 0000000000..99ffbde2c8 --- /dev/null +++ b/src/server/sql/meter/clear_meter_unit_id.sql @@ -0,0 +1,5 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +UPDATE meters SET unit_id = NULL WHERE id = ${id}; \ No newline at end of file diff --git a/src/server/sql/meter/get_meter_by_unit_or_default_graphic.sql b/src/server/sql/meter/get_meter_by_unit_or_default_graphic.sql new file mode 100644 index 0000000000..714a8a9cf2 --- /dev/null +++ b/src/server/sql/meter/get_meter_by_unit_or_default_graphic.sql @@ -0,0 +1,7 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +SELECT id, name, unit_id, default_graphic_unit +FROM meters +WHERE unit_id = ${unitId} OR default_graphic_unit = ${unitId}; \ No newline at end of file diff --git a/src/server/sql/unit/find_orphaned_units.sql b/src/server/sql/unit/find_orphaned_units.sql new file mode 100644 index 0000000000..27a5409581 --- /dev/null +++ b/src/server/sql/unit/find_orphaned_units.sql @@ -0,0 +1,12 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +SELECT unit.id, unit.name +FROM units unit +WHERE unit.displayable != 'none'::displayable_type +AND NOT EXISTS ( + SELECT 1 FROM conversions conv + WHERE (conv.source_id = unit.id OR conv.destination_id = unit.id) +) +LIMIT ${limit}; \ No newline at end of file diff --git a/src/server/sql/unit/lock_unit_by_id.sql b/src/server/sql/unit/lock_unit_by_id.sql new file mode 100644 index 0000000000..3ae4081e9d --- /dev/null +++ b/src/server/sql/unit/lock_unit_by_id.sql @@ -0,0 +1,5 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + + SELECT * FROM units WHERE id = ${id} FOR UPDATE; \ No newline at end of file diff --git a/src/server/test/db/suffixUnitDeletionTests.js b/src/server/test/db/suffixUnitDeletionTests.js new file mode 100644 index 0000000000..29968a3ab8 --- /dev/null +++ b/src/server/test/db/suffixUnitDeletionTests.js @@ -0,0 +1,270 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +const { mocha, expect, testDB } = require('../common'); +const Unit = require('../../models/Unit'); +const Meter = require('../../models/Meter'); +const Group = require('../../models/Group'); +const Point = require('../../models/Point'); +const Conversion = require('../../models/Conversion'); +const gps = new Point(90, 45); +const { removeAdditionalConversionsAndUnits } = require('../../services/graph/handleSuffixUnits'); +const { insertUnits, insertConversions, insertGroups } = require('../../util/insertData'); + +/** + * Sets up a standard suffix unit parent/child pair for tests. Must be + * called inside each test, since the DB resets before every test runs. + * @param {*} conn The connection to use. + * @returns {Promise.<{parent: Unit, child: Unit}>} + */ +async function setupParentChild(conn) { + await insertUnits([ + { + name: 'Parent', identifier: 'Parent', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.UNIT, suffix: 'test-suffix', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: '' + }, + { + name: 'Child', identifier: 'Child', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.SUFFIX, suffix: '', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: 'suffix unit created by OED' + } + ], false, conn); + await insertConversions([ + { sourceName: 'Parent', destinationName: 'Child', bidirectional: false, slope: 1, intercept: 0, note: 'test conversion' } + ], conn); + + return { + parent: await Unit.getByName('Parent', conn), + child: await Unit.getByName('Child', conn) + }; +} + +/** + * Asserts that Unit.getById throws for the given id, i.e. the unit no + * longer exists. Unit.getById uses conn.one internally, so it rejects + * rather than returning null when nothing matches. + * @param {number} id The unit id expected to no longer exist. + * @param {*} conn The connection to use. + */ +async function expectUnitDeleted(id, conn) { + let threw = false; + try { + await Unit.getById(id, conn); + } catch (e) { + threw = true; + } + expect(threw).to.equal(true); +} + +mocha.describe('Suffix unit cascade deletion', () => { + + mocha.it('deletes the auto-created suffix unit and its conversion', async () => { + const conn = testDB.getConnection(); + const { parent, child } = await setupParentChild(conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + await expectUnitDeleted(child.id, conn); + + const conversionAfter = await Conversion.getBySourceDestination(parent.id, child.id, conn); + expect(conversionAfter).to.equal(null); + }); + + mocha.it('clears a meter\'s unit_id instead of blocking deletion', async () => { + const conn = testDB.getConnection(); + const { parent, child } = await setupParentChild(conn); + + // Kept as a direct constructor rather than insertMeters this test only needs + // a meter with a given unitId, not any readings. + const meter = new Meter(undefined, 'Test Meter', null, false, true, Meter.type.OTHER, null, gps, + 'Identified Test Meter', 'notes', 35.0, true, true, '01:01:25', '00:00:00', 5, 1, 1, 'increasing', false, + 1.5, '0001-01-01 23:59:59', '2020-07-02 01:00:10', '2020-03-05 02:12:00', child.id, -99, + Unit.areaUnitType.METERS, undefined); + await meter.insert(conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + await expectUnitDeleted(child.id, conn); + + const meterAfter = await Meter.getByID(meter.id, conn); + // unitID defaultGraphicUnit returns -99 when registered as NULL + // Expect the unitID to be -99 instead of null for meters and groups + expect(meterAfter.unitId).to.equal(-99); + }); + + mocha.it('cleans up other conversions referencing the auto-created unit', async () => { + const conn = testDB.getConnection(); + const { parent, child } = await setupParentChild(conn); + + await insertUnits([ + { + name: 'Unrelated Unit', identifier: 'Unrelated', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.UNIT, suffix: '', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: '' + } + ], false, conn); + await insertConversions([ + { sourceName: 'Unrelated Unit', destinationName: 'Child', bidirectional: false, slope: 2, intercept: 0, note: 'second conversion' } + ], conn); + const otherUnit = await Unit.getByName('Unrelated Unit', conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + await expectUnitDeleted(child.id, conn); + + const firstConversionAfter = await Conversion.getBySourceDestination(parent.id, child.id, conn); + expect(firstConversionAfter).to.equal(null); + + const secondConversionAfter = await Conversion.getBySourceDestination(otherUnit.id, child.id, conn); + expect(secondConversionAfter).to.equal(null); + }); + + mocha.it('clears a group\'s default_graphic_unit instead of blocking deletion', async () => { + const conn = testDB.getConnection(); + const { parent, child } = await setupParentChild(conn); + + await insertGroups([ + { name: 'Test Group', displayable: true, childMeters: [], childGroups: [], defaultGraphicUnit: 'Child' } + ], conn); + const group = await Group.getByName('Test Group', conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + await expectUnitDeleted(child.id, conn); + + const groupAfter = await Group.getByID(group.id, conn); + expect(groupAfter.defaultGraphicUnit).to.equal(-99); + }); + + mocha.it('recursively cleans up nested suffix chains (A -> B -> C)', async () => { + const conn = testDB.getConnection(); + + await insertUnits([ + { + name: 'A', identifier: 'A', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.SUFFIX, suffix: 'chain-suffix', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: '' + }, + { + name: 'B (created by OED)', identifier: 'B', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.SUFFIX, suffix: '', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: 'suffix unit created by OED' + }, + { + name: 'C (created by OED)', identifier: 'C', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.SUFFIX, suffix: '', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: 'suffix unit created by OED' + } + ], false, conn); + await insertConversions([ + { sourceName: 'A', destinationName: 'B (created by OED)', bidirectional: false, slope: 1, intercept: 0, note: 'A to B' }, + { sourceName: 'B (created by OED)', destinationName: 'C (created by OED)', bidirectional: false, slope: 1, intercept: 0, note: 'B to C' } + ], conn); + + const unitA = await Unit.getByName('A', conn); + const unitB = await Unit.getByName('B (created by OED)', conn); + const unitC = await Unit.getByName('C (created by OED)', conn); + + await removeAdditionalConversionsAndUnits(unitA, conn); + + await expectUnitDeleted(unitB.id, conn); + await expectUnitDeleted(unitC.id, conn); + }); + + mocha.it('does not delete the parent when cleaning up from a child unit', async () => { + const conn = testDB.getConnection(); + const { parent, child } = await setupParentChild(conn); + + // Call cleanup starting FROM the child, not the parent -- this is the + // scenario that previously walked backward and deleted the parent. + await removeAdditionalConversionsAndUnits(child, conn); + + const parentAfter = await Unit.getById(parent.id, conn); + expect(parentAfter).to.not.equal(null); + + const conversionAfter = await Conversion.getBySourceDestination(parent.id, child.id, conn); + expect(conversionAfter).to.not.equal(null); + }); + + mocha.it('deletes the reverse conversion when bidirectional', async () => { + const conn = testDB.getConnection(); + + await insertUnits([ + { + name: 'Parent', identifier: 'Parent', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.SUFFIX, suffix: 'test-suffix', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: '' + }, + { + name: 'Child', identifier: 'Child', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.SUFFIX, suffix: '', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: 'suffix unit created by OED' + } + ], false, conn); + await insertConversions([ + { sourceName: 'Parent', destinationName: 'Child', bidirectional: true, slope: 1, intercept: 0, note: 'forward' }, + { sourceName: 'Child', destinationName: 'Parent', bidirectional: true, slope: 1, intercept: 0, note: 'reverse' } + ], conn); + + const parent = await Unit.getByName('Parent', conn); + const child = await Unit.getByName('Child', conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + const forwardAfter = await Conversion.getBySourceDestination(parent.id, child.id, conn); + const reverseAfter = await Conversion.getBySourceDestination(child.id, parent.id, conn); + expect(forwardAfter).to.equal(null); + expect(reverseAfter).to.equal(null); + }); + + mocha.it('clears stale cik rows before deleting the unit', async () => { + const conn = testDB.getConnection(); + const { parent, child } = await setupParentChild(conn); + + // Simulate a stale cik row referencing the child unit, as would exist + // before Cik regeneration + await conn.none( + 'INSERT INTO cik (source_id, destination_id, slope, intercept) VALUES ($1, $2, 1, 0)', + [child.id, parent.id] + ); + + // Should not throw. + await removeAdditionalConversionsAndUnits(parent, conn); + + await expectUnitDeleted(child.id, conn); + }); + + mocha.it('does not touch a regular (non-suffix) unit in the same conversion set', async () => { + const conn = testDB.getConnection(); + + await insertUnits([ + { + name: 'Parent', identifier: 'Parent', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.SUFFIX, suffix: 'test-suffix', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: '' + }, + { + name: 'Regular Unit', identifier: 'Regular', unitRepresent: Unit.unitRepresentType.QUANTITY, + secInRate: 1000, typeOfUnit: Unit.unitType.UNIT, suffix: '', + displayable: Unit.displayableType.ALL, preferredDisplay: true, note: '' + } + ], false, conn); + await insertConversions([ + { sourceName: 'Parent', destinationName: 'Regular Unit', bidirectional: false, slope: 1, intercept: 0, note: 'to regular unit' } + ], conn); + + const parent = await Unit.getByName('Parent', conn); + const regularUnit = await Unit.getByName('Regular Unit', conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + const regularAfter = await Unit.getById(regularUnit.id, conn); + expect(regularAfter).to.not.equal(null); + + const conversionAfter = await Conversion.getBySourceDestination(parent.id, regularUnit.id, conn); + expect(conversionAfter).to.not.equal(null); + }); + +}); \ No newline at end of file diff --git a/src/server/util/suffixUnitCheck.js b/src/server/util/suffixUnitCheck.js new file mode 100644 index 0000000000..4f72e895f8 --- /dev/null +++ b/src/server/util/suffixUnitCheck.js @@ -0,0 +1,19 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +/** + * Determines whether a unit is suffix-related. Meaning either an OED-created + * suffix-type unit (typeOfUnit === 'suffix'), or a "Suffix Input" unit + * with a non-empty suffix string set on it. Matches the same check used + * by the unit create/edit modals (no trimming, plain '' comparison). + * @param {*} unit The unit to check. May be undefined. + * @returns {boolean} + */ +function isSuffixRelated(unit) { + return !!unit && (unit.typeOfUnit === 'suffix' || unit.suffix !== ''); +} + +module.exports = { + isSuffixRelated + }; \ No newline at end of file