From d879267d55bcb80d590d2e491e1b05f1e5dd446d Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 21 Jul 2026 16:01:33 -0700 Subject: [PATCH 01/21] feat: add suffix unit dependency checking and fix async cleanup - 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 --- .../services/graph/checkUnitDependencies.js | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 src/server/services/graph/checkUnitDependencies.js diff --git a/src/server/services/graph/checkUnitDependencies.js b/src/server/services/graph/checkUnitDependencies.js new file mode 100644 index 0000000000..b9ac4a8cd7 --- /dev/null +++ b/src/server/services/graph/checkUnitDependencies.js @@ -0,0 +1,90 @@ +/* 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'); + +/** + * 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 with hasDependencies boolean and details + */ +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) + conn.any(` + SELECT id, name, unit_id, default_graphic_unit + FROM meters + WHERE unit_id = $1 OR default_graphic_unit = $1 + `, [unitId]), + + // Check if any groups use this unit as default_graphic_unit + conn.any(` + SELECT id, name, default_graphic_unit + FROM groups + WHERE default_graphic_unit = $1 + `, [unitId]), + + // Check if unit is used in any conversions (as source or destination) + conn.any(` + SELECT source_id, destination_id, bidirectional + FROM conversions + WHERE source_id = $1 OR destination_id = $1 + `, [unitId]) + ]); + + return { + hasDependencies: meters.length > 0 || groups.length > 0 || conversions.length > 0, + meters, + groups, + conversions + }; +} + +/** + * Checks if a suffix-type unit (created by OED) can be safely hidden. + * These units should only be hidden if they're not used by meters/groups. + * Conversions are expected and will be cleaned up separately. + * @param {number} unitId The suffix unit ID to check + * @param {*} conn Database connection (can be a transaction) + * @returns {Promise} True if safe to hide + */ +async function canSafelyHideSuffixUnit(unitId, conn) { + const deps = await checkUnitDependencies(unitId, conn); + // Suffix units created by OED can be hidden if no meters/groups depend on them + // (conversions are expected and will be cleaned up) + return deps.meters.length === 0 && deps.groups.length === 0; +} + +/** + * Gets detailed dependency information for UI warnings. + * @param {number} unitId The unit ID to check + * @param {*} conn Database connection + * @returns {Promise} Detailed dependency info + */ +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(m => ({ id: m.id, name: m.name })), + groups: deps.groups.map(g => ({ id: g.id, name: g.name })), + conversions: deps.conversions.map(c => ({ + sourceId: c.source_id, + destinationId: c.destination_id, + bidirectional: c.bidirectional + })) + }; +} + +module.exports = { + checkUnitDependencies, + canSafelyHideSuffixUnit, + getUnitDependencyDetails +}; \ No newline at end of file From b3b1b34b76da071e7c9a6857f6e1f6e0ff842a2e Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 21 Jul 2026 16:08:55 -0700 Subject: [PATCH 02/21] feat: add suffix unit dependency checking and fix async cleanup - 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 --- src/server/services/conversionSimulation.js | 92 +++++++++++++++++-- .../services/graph/handleSuffixUnits.js | 85 ++++++++++++++--- 2 files changed, 155 insertions(+), 22 deletions(-) diff --git a/src/server/services/conversionSimulation.js b/src/server/services/conversionSimulation.js index e5901c6e71..23b8bf1a4b 100644 --- a/src/server/services/conversionSimulation.js +++ b/src/server/services/conversionSimulation.js @@ -10,6 +10,54 @@ const Unit = require('../models/Unit'); const { createConversionGraph, createConversionGraphFromArray } = require('./graph/createConversionGraph'); const { intersectSets, compatibleUnitsForMeter } = require('../util/compatibleUnits'); +/** + * 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 unitsToHide = []; + + // Find all conversions involving this suffix unit + const relatedConversions = allConversions.filter((conversion) => + conversion.sourceId === suffixUnit.id || + conversion.destinationId === 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 (!unitsToHide.includes(otherUnitId)) { + unitsToHide.push(otherUnitId); + } + } + } + + return { conversionsToRemove, unitsToHide }; +} async function simulateDeleteConversion({ sourceId, destinationId }, conn) { // 1. Load all data @@ -20,20 +68,46 @@ 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 = [ + { sourceId, destinationId } // The conversion being deleted + ]; + + if (sourceUnit && sourceUnit.typeOfUnit === 'suffix') { + const cleanup = simulateSuffixUnitCleanup(sourceUnit, allConversions, allUnits); + conversionsToRemove.push(...cleanup.conversionsToRemove); + } + + if (destUnit && destUnit.typeOfUnit === 'suffix') { + const cleanup = simulateSuffixUnitCleanup(destUnit, allConversions, allUnits); + conversionsToRemove.push(...cleanup.conversionsToRemove); + } + + // 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 => { + return !uniqueConversionsToRemove.some(toRemove => + c.sourceId === toRemove.sourceId && c.destinationId === toRemove.destinationId + ); + }); - // 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 +115,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 +136,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]; diff --git a/src/server/services/graph/handleSuffixUnits.js b/src/server/services/graph/handleSuffixUnits.js index df7b1ed477..581502cb25 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 { canSafelyHideSuffixUnit } = require('./checkUnitDependencies'); /** * Adds the new unit and conversions to the database and the conversion graph. @@ -160,22 +162,79 @@ async function handleSuffixUnits(graph, conn) { * 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. * @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. +const MAX_SUFFIX_CLEANUP_DEPTH = 10; + +async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) { + 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}`); + } + // 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.destinationId === suffixUnit.id || + (conversion.bidirectional && (conversion.sourceId === suffixUnit.id || conversion.destinationId === suffixUnit.id)) + ); + + // Process all related conversions in parallel with proper async handling + await Promise.all(relatedConversions.map(async (conversion) => { + try { + // 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 otherUnit = await Unit.getById(otherUnitId, conn); + + // Validate that the other unit exists + if (!otherUnit) { + log.warn(`Unit ${otherUnitId} not found when cleaning up suffix unit ${suffixUnit.id}. Conversion ${conversion.sourceId}->${conversion.destinationId} may be orphaned.`); + return; + } + + // The units that OED adds are suffix units (typeOfUnit === SUFFIX) + if (otherUnit.typeOfUnit === Unit.unitType.SUFFIX) { + // Check if unit can be safely hidden (no meter/group dependencies) + const canHide = await canSafelyHideSuffixUnit(otherUnitId, conn); + + // Always delete the conversion, but only hide unit if safe 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) { + // Check if reverse conversion exists (some systems store bidirectional as two entries) + const reverseConversion = await Conversion.getBySourceDestination( + conversion.destinationId, + conversion.sourceId, + conn + ); + if (reverseConversion) { + await Conversion.delete(conversion.destinationId, conversion.sourceId, conn); + } + } + + if (canHide) { + // Hide the destination unit (the other unit in the conversion) + otherUnit.displayable = Unit.displayableType.NONE; + await otherUnit.update(conn); + + // Recursively clean up this unit's related conversions/units + // This handles nested suffix chains (A -> B -> C) + await removeAdditionalConversionsAndUnits(otherUnit, conn, depth + 1); + } else { + log.warn(`Cannot hide suffix unit ${otherUnitId} - has dependencies (meters/groups). Conversion ${conversion.sourceId}->${conversion.destinationId} deleted but unit remains visible.`); + } + } + } catch (err) { + log.error(`Error processing conversion ${conversion.sourceId}->${conversion.destinationId} during suffix unit cleanup: ${err}`, err); + // Continue processing other conversions even if one fails } - }); + })); + + // Restore the suffix unit's displayable status suffixUnit.displayable = Unit.displayableType.ALL; await suffixUnit.update(conn); } From 4cb3205990ce62268c8fdb53695c895ec0f5f1d7 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 21 Jul 2026 16:09:48 -0700 Subject: [PATCH 03/21] feat: add DB layer methods for cascading suffix deletions - Add getConversionsByUnitId method - Add deleteConversionAndRelatedSuffixes with dependency checks --- src/server/models/Conversion.js | 77 +++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/src/server/models/Conversion.js b/src/server/models/Conversion.js index 37ce9c0a20..c292df220c 100644 --- a/src/server/models/Conversion.js +++ b/src/server/models/Conversion.js @@ -96,6 +96,83 @@ class Conversion { destination: destination }); } + + /** + * Gets all conversions involving a specific unit (as source or destination). + * @param {number} unitId The unit ID to search for + * @param {*} conn The connection to use + * @returns {Promise.>} Array of conversions involving the unit + */ + static async getConversionsByUnitId(unitId, conn) { + const rows = await conn.any(` + SELECT * FROM conversions + WHERE source_id = $1 OR destination_id = $1 + `, [unitId]); + return rows.map(Conversion.mapRow); + } + + /** + * Deletes a conversion and all related suffix units/conversions in a transaction. + * Checks for dependencies before deletion and throws error if unit is used in meters/groups. + * @param {number} sourceId The source unit ID + * @param {number} destinationId The destination unit ID + * @param {*} conn The connection to use (should be a transaction) + * @returns {Promise.} Object with deletedUnits and deletedConversions arrays + * @throws {Error} If unit has dependencies (meters/groups) + */ + static async deleteConversionAndRelatedSuffixes(sourceId, destinationId, conn) { + const Unit = require('./Unit'); + const { checkUnitDependencies } = require('../services/graph/checkUnitDependencies'); + const { removeAdditionalConversionsAndUnits } = require('../services/graph/handleSuffixUnits'); + + const deletedUnits = []; + const deletedConversions = []; + + // Get source and destination units + const source = await Unit.getById(sourceId, conn); + const dest = await Unit.getById(destinationId, conn); + + if (!source || !dest) { + throw new Error('Source or destination unit not found'); + } + + // Check dependencies for suffix units + if (source.typeOfUnit === Unit.unitType.SUFFIX) { + const deps = await checkUnitDependencies(sourceId, conn); + 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(', '); + throw new Error(`Cannot delete: source unit "${source.name}" is used in ${deps.meters.length} meter(s): ${meterNames} and ${deps.groups.length} group(s): ${groupNames}`); + } + await removeAdditionalConversionsAndUnits(source, conn); + } + + if (dest.typeOfUnit === Unit.unitType.SUFFIX) { + const deps = await checkUnitDependencies(destinationId, conn); + 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(', '); + throw new Error(`Cannot delete: destination unit "${dest.name}" is used in ${deps.meters.length} meter(s): ${meterNames} and ${deps.groups.length} group(s): ${groupNames}`); + } + await removeAdditionalConversionsAndUnits(dest, conn); + } + + // Delete the conversion + await Conversion.delete(sourceId, destinationId, conn); + deletedConversions.push({ sourceId, destinationId }); + + // Handle bidirectional conversion + const conversion = await Conversion.getBySourceDestination(sourceId, destinationId, conn); + if (conversion && conversion.bidirectional) { + const reverseConversion = await Conversion.getBySourceDestination(destinationId, sourceId, conn); + if (reverseConversion) { + await Conversion.delete(destinationId, sourceId, conn); + deletedConversions.push({ sourceId: destinationId, destinationId: sourceId }); + } + } + + return { deletedUnits, deletedConversions }; + } } module.exports = Conversion; From ff4f60e40404dfc0ca8e78124b9d6cf14128b34f Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 21 Jul 2026 16:10:21 -0700 Subject: [PATCH 04/21] feat: enhance conversion/unit deletion with dependency validation - 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 --- src/server/routes/conversions.js | 116 ++++++++++++++++++++++++++++++- src/server/routes/units.js | 86 +++++++++++++++++++++-- 2 files changed, 195 insertions(+), 7 deletions(-) diff --git a/src/server/routes/conversions.js b/src/server/routes/conversions.js index dd719c6052..3736fd62b7 100644 --- a/src/server/routes/conversions.js +++ b/src/server/routes/conversions.js @@ -6,11 +6,14 @@ 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 { checkUnitDependencies, getUnitDependencyDetails } = require('../services/graph/checkUnitDependencies'); const { adminAuthMiddleware, optionalAuthMiddleware } = require('./authenticator'); const { STRING_GENERAL_MAX_LENGTH } = require('../util/validationConstants'); @@ -180,7 +183,81 @@ 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 + // This provides better error messages before attempting deletion + const suffixUnitsToCheck = []; + if (source.typeOfUnit === 'suffix') { + suffixUnitsToCheck.push({ unit: source, role: 'source' }); + } + if (dest.typeOfUnit === 'suffix') { + 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 => { + // Lock the units to prevent concurrent modifications + await t.one('SELECT * FROM units WHERE id = $1 FOR UPDATE', [sourceId]); + await t.one('SELECT * FROM units WHERE id = $1 FOR UPDATE', [destinationId]); + + // Check if the source or the destination is a suffix unit and clean up related conversions/units + if (source.typeOfUnit === 'suffix') { + log.info(`Suffix unit ${sourceId} is used in conversion deletion as source. Cleaning up related conversions and units.`); + // Reload source unit within transaction to ensure consistency + const sourceInTx = await Unit.getById(sourceId, t); + await removeAdditionalConversionsAndUnits(sourceInTx, t); + } + if (dest.typeOfUnit === 'suffix') { + log.info(`Suffix unit ${destinationId} is used in conversion deletion as destination. Cleaning up related conversions and units.`); + // Reload dest unit within transaction to ensure consistency + const destInTx = await Unit.getById(destinationId, t); + await removeAdditionalConversionsAndUnits(destInTx, t); + } + + // 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 for (const meterId of meterIds) { await t.none(`UPDATE meters SET default_graphic_unit = NULL WHERE id = ${meterId}`); @@ -192,10 +269,45 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re // Delete conversion await Conversion.delete(sourceId, destinationId, t); }); + + // Verify no orphaned suffix units after cleanup (performance: only check if suffix units were involved) + if (source.typeOfUnit === 'suffix' || dest.typeOfUnit === 'suffix') { + try { + // Use efficient query with LIMIT to avoid scanning large tables unnecessarily + // Only check recently affected units to improve performance on large databases + const orphanedUnits = await conn.any(` + SELECT u.id, u.name + FROM units u + WHERE u.type_of_unit = 'suffix'::unit_type + AND u.displayable != 'none'::displayable_type + AND (u.id = $1 OR u.id = $2 OR u.id IN ( + SELECT DISTINCT CASE + WHEN c.source_id IN ($1, $2) THEN c.destination_id + WHEN c.destination_id IN ($1, $2) THEN c.source_id + END + FROM conversions c + WHERE (c.source_id IN ($1, $2) OR c.destination_id IN ($1, $2)) + )) + AND NOT EXISTS ( + SELECT 1 FROM conversions c + WHERE (c.source_id = u.id OR c.destination_id = u.id) + ) + LIMIT 100 + `, [sourceId, destinationId]); + 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 check, log but don't fail + log.warn(`Error checking for orphaned units: ${err}`); + } + } 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..4b0cf82687 100644 --- a/src/server/routes/units.js +++ b/src/server/routes/units.js @@ -7,6 +7,7 @@ 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'); @@ -219,20 +220,95 @@ 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 { checkUnitDependencies } = require('../services/graph/checkUnitDependencies'); + 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 t.one('SELECT * FROM units WHERE id = $1 FOR UPDATE', [unitId]); + + // 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.trim() !== '') { + 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); + }); 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; From e41845c34d605c13e14b57e94104c30d8268ecde Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 21 Jul 2026 16:11:09 -0700 Subject: [PATCH 05/21] feat: enhance UI warnings for suffix unit deletions - Show affected meters/groups in deletion warnings - Improve suffix unit warning formatting - Add translation keys for dependency warnings --- .../EditConversionModalComponent.tsx | 105 +++++++++++++++++- src/client/app/translations/data.ts | 4 + 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index f677e204f5..b4986dd4a2 100644 --- a/src/client/app/components/conversion/EditConversionModalComponent.tsx +++ b/src/client/app/components/conversion/EditConversionModalComponent.tsx @@ -128,6 +128,7 @@ 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]; @@ -223,9 +224,110 @@ export default function EditConversionModalComponent(props: EditConversionModalC ); } + } else if (source.typeOfUnit === UnitType.suffix || dest.typeOfUnit === UnitType.suffix) { + const suffixUnit = source.typeOfUnit === UnitType.suffix ? source : dest; + // Find all conversions involving this suffix unit (as source or destination) + const suffixUnitConversions = conversionDetails.filter(c => + c.sourceId === suffixUnit.id || c.destinationId === suffixUnit.id || + (c.bidirectional && (c.sourceId === suffixUnit.id || c.destinationId === suffixUnit.id)) + ); + + // Find suffix-type units that would be hidden (OED-created suffix units) + const suffixTypeUnitsToHide = suffixUnitConversions + .map(c => { + const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; + return unitDataById[otherId]; + }) + .filter(u => u && u.typeOfUnit === UnitType.suffix); + + // Get unique conversions that would be deleted + const conversionsToDelete = suffixUnitConversions.filter(c => { + const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; + const otherUnit = unitDataById[otherId]; + return otherUnit && otherUnit.typeOfUnit === UnitType.suffix; + }); + + // Check for meters/groups using affected suffix units + const affectedSuffixUnitIds = new Set(suffixTypeUnitsToHide.map(u => u.id)); + affectedSuffixUnitIds.add(suffixUnit.id); // Also check the main suffix unit + + // Check meters using these units (as unitId or defaultGraphicUnit) + const affectedMetersList = Object.values(meterDataById).filter(meter => + affectedSuffixUnitIds.has(meter.unitId) || + (meter.defaultGraphicUnit !== null && affectedSuffixUnitIds.has(meter.defaultGraphicUnit)) + ); + + // Check groups using these units (as defaultGraphicUnit) + const affectedGroupsList = Object.values(groupDataById).filter(group => + group.defaultGraphicUnit !== null && affectedSuffixUnitIds.has(group.defaultGraphicUnit) + ); + + // Display dependency warnings 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 warnings using React elements for consistency + if (suffixTypeUnitsToHide.length > 0) { + msgElements.push( +
+ {translate('conversion.delete.suffix.units.to.delete')}: +
    + {suffixTypeUnitsToHide.map(u => ( +
  • "{u.name}" ({u.identifier})
    • + ))} +
+
+ ); + } + + if (conversionsToDelete.length > 0) { + msgElements.push( +
+ {translate('conversion.delete.suffix.conversions.to.delete')}: +
    + {conversionsToDelete.map((c, idx) => { + const s = unitDataById[c.sourceId]?.name || unitDataById[c.sourceId]?.identifier || c.sourceId; + const d = unitDataById[c.destinationId]?.name || unitDataById[c.destinationId]?.identifier || c.destinationId; + const bidirectional = c.bidirectional ? ' ↔ ' : ' → '; + return ( +
  • + "{s}"{bidirectional}"{d}" +
    • + ); + })} +
+
+ ); + } } - // Only run simulation if the previous orphan check passed and it's unit-to-unit + // Run simulation if the previous orphan check passed and it's not cancelled + // Now supports suffix units since simulation has been enhanced if (source.typeOfUnit !== UnitType.suffix && dest.typeOfUnit === UnitType.unit && !cancel) { try { const result = await triggerSimulate({ @@ -373,6 +475,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC setDeleteConfirmationMessage(msgElements); handleDeleteConfirmationModalOpen(); } + }; /* Confirm Delete Modal */ // Separate from state comment to keep everything related to the warning confirmation modal together diff --git a/src/client/app/translations/data.ts b/src/client/app/translations/data.ts index bf9886cd54..6320e7f85e 100644 --- a/src/client/app/translations/data.ts +++ b/src/client/app/translations/data.ts @@ -92,6 +92,10 @@ const LocaleTranslationData = { "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.disable": "Deleting this suffix conversion will disable use of suffix", + "conversion.delete.suffix.units.to.delete": "The following units will also be deleted", + "conversion.delete.suffix.dependencies.warning": "Warning: Some meters or groups depend on units that will be affected", + "conversion.delete.suffix.meters.affected": "Affected meters", + "conversion.delete.suffix.groups.affected": "Affected groups", "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:", From 041ca551bafd3b2fca458b186349875bc599c638 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Mon, 27 Jul 2026 11:24:27 -0700 Subject: [PATCH 06/21] Revert: Omit DB layer methods for cascading suffix deletions. - Removed getConversionsByUnitId method as it is no longer used - Removed deleteConversionAndRelatedSuffixes method as it is no longer used --- src/server/models/Conversion.js | 76 --------------------------------- 1 file changed, 76 deletions(-) diff --git a/src/server/models/Conversion.js b/src/server/models/Conversion.js index c292df220c..ce783913e9 100644 --- a/src/server/models/Conversion.js +++ b/src/server/models/Conversion.js @@ -97,82 +97,6 @@ class Conversion { }); } - /** - * Gets all conversions involving a specific unit (as source or destination). - * @param {number} unitId The unit ID to search for - * @param {*} conn The connection to use - * @returns {Promise.>} Array of conversions involving the unit - */ - static async getConversionsByUnitId(unitId, conn) { - const rows = await conn.any(` - SELECT * FROM conversions - WHERE source_id = $1 OR destination_id = $1 - `, [unitId]); - return rows.map(Conversion.mapRow); - } - - /** - * Deletes a conversion and all related suffix units/conversions in a transaction. - * Checks for dependencies before deletion and throws error if unit is used in meters/groups. - * @param {number} sourceId The source unit ID - * @param {number} destinationId The destination unit ID - * @param {*} conn The connection to use (should be a transaction) - * @returns {Promise.} Object with deletedUnits and deletedConversions arrays - * @throws {Error} If unit has dependencies (meters/groups) - */ - static async deleteConversionAndRelatedSuffixes(sourceId, destinationId, conn) { - const Unit = require('./Unit'); - const { checkUnitDependencies } = require('../services/graph/checkUnitDependencies'); - const { removeAdditionalConversionsAndUnits } = require('../services/graph/handleSuffixUnits'); - - const deletedUnits = []; - const deletedConversions = []; - - // Get source and destination units - const source = await Unit.getById(sourceId, conn); - const dest = await Unit.getById(destinationId, conn); - - if (!source || !dest) { - throw new Error('Source or destination unit not found'); - } - - // Check dependencies for suffix units - if (source.typeOfUnit === Unit.unitType.SUFFIX) { - const deps = await checkUnitDependencies(sourceId, conn); - 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(', '); - throw new Error(`Cannot delete: source unit "${source.name}" is used in ${deps.meters.length} meter(s): ${meterNames} and ${deps.groups.length} group(s): ${groupNames}`); - } - await removeAdditionalConversionsAndUnits(source, conn); - } - - if (dest.typeOfUnit === Unit.unitType.SUFFIX) { - const deps = await checkUnitDependencies(destinationId, conn); - 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(', '); - throw new Error(`Cannot delete: destination unit "${dest.name}" is used in ${deps.meters.length} meter(s): ${meterNames} and ${deps.groups.length} group(s): ${groupNames}`); - } - await removeAdditionalConversionsAndUnits(dest, conn); - } - - // Delete the conversion - await Conversion.delete(sourceId, destinationId, conn); - deletedConversions.push({ sourceId, destinationId }); - - // Handle bidirectional conversion - const conversion = await Conversion.getBySourceDestination(sourceId, destinationId, conn); - if (conversion && conversion.bidirectional) { - const reverseConversion = await Conversion.getBySourceDestination(destinationId, sourceId, conn); - if (reverseConversion) { - await Conversion.delete(destinationId, sourceId, conn); - deletedConversions.push({ sourceId: destinationId, destinationId: sourceId }); - } - } - - return { deletedUnits, deletedConversions }; - } } module.exports = Conversion; From 26a0c8bc11f6f08f0e141d956a07be1427fe979d Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Mon, 27 Jul 2026 11:31:51 -0700 Subject: [PATCH 07/21] Refactor: Robust Suffix Unit detection. - Refactored hide suffix functions to delete suffix functions - Suffix checks now account for suffix inputs (type of unit = unit, suffix = ) when checking for dependencies. --- .../EditConversionModalComponent.tsx | 16 +++++++++------ src/server/routes/conversions.js | 20 +++++++++++-------- src/server/services/conversionSimulation.js | 8 +++++--- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index b4986dd4a2..472a685bb2 100644 --- a/src/client/app/components/conversion/EditConversionModalComponent.tsx +++ b/src/client/app/components/conversion/EditConversionModalComponent.tsx @@ -224,7 +224,11 @@ export default function EditConversionModalComponent(props: EditConversionModalC ); } - } else if (source.typeOfUnit === UnitType.suffix || dest.typeOfUnit === UnitType.suffix) { + // Suffix unit to Suffix Unit deletion + } else if ( + source.typeOfUnit === UnitType.suffix || (source.suffix && source.suffix.trim() !== '') || + dest.typeOfUnit === UnitType.suffix || (dest.suffix && dest.suffix.trim() !== '') + ) { const suffixUnit = source.typeOfUnit === UnitType.suffix ? source : dest; // Find all conversions involving this suffix unit (as source or destination) const suffixUnitConversions = conversionDetails.filter(c => @@ -232,8 +236,8 @@ export default function EditConversionModalComponent(props: EditConversionModalC (c.bidirectional && (c.sourceId === suffixUnit.id || c.destinationId === suffixUnit.id)) ); - // Find suffix-type units that would be hidden (OED-created suffix units) - const suffixTypeUnitsToHide = suffixUnitConversions + // Find OED-created suffix-type units that would be cascade deleted + const suffixTypeUnitsToDelete = suffixUnitConversions .map(c => { const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; return unitDataById[otherId]; @@ -248,7 +252,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC }); // Check for meters/groups using affected suffix units - const affectedSuffixUnitIds = new Set(suffixTypeUnitsToHide.map(u => u.id)); + const affectedSuffixUnitIds = new Set(suffixTypeUnitsToDelete.map(u => u.id)); affectedSuffixUnitIds.add(suffixUnit.id); // Also check the main suffix unit // Check meters using these units (as unitId or defaultGraphicUnit) @@ -292,12 +296,12 @@ export default function EditConversionModalComponent(props: EditConversionModalC } // Display warnings using React elements for consistency - if (suffixTypeUnitsToHide.length > 0) { + if (suffixTypeUnitsToDelete.length > 0) { msgElements.push(
{translate('conversion.delete.suffix.units.to.delete')}:
    - {suffixTypeUnitsToHide.map(u => ( + {suffixTypeUnitsToDelete.map(u => (
  • "{u.name}" ({u.identifier})
    • ))}
diff --git a/src/server/routes/conversions.js b/src/server/routes/conversions.js index 3736fd62b7..cb9ee97316 100644 --- a/src/server/routes/conversions.js +++ b/src/server/routes/conversions.js @@ -193,12 +193,15 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re } // 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 isSuffixRelated = (unit) => unit.typeOfUnit === 'suffix' || (unit.suffix && unit.suffix.trim() !== ''); + const suffixUnitsToCheck = []; - if (source.typeOfUnit === 'suffix') { + if (isSuffixRelated(source)) { suffixUnitsToCheck.push({ unit: source, role: 'source' }); } - if (dest.typeOfUnit === 'suffix') { + if (isSuffixRelated(dest)) { suffixUnitsToCheck.push({ unit: dest, role: 'destination' }); } @@ -234,15 +237,16 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re await t.one('SELECT * FROM units WHERE id = $1 FOR UPDATE', [destinationId]); // Check if the source or the destination is a suffix unit and clean up related conversions/units - if (source.typeOfUnit === 'suffix') { - log.info(`Suffix unit ${sourceId} is used in conversion deletion as source. Cleaning up related conversions and units.`); - // Reload source unit within transaction to ensure consistency + // Accounts for Suffix Inputs where unit = unit & and Suffix contains a string + const isSuffixRelated = (unit) => unit.typeOfUnit === 'suffix' || (unit.suffix && unit.suffix.trim() !== ''); + + 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 (dest.typeOfUnit === 'suffix') { - log.info(`Suffix unit ${destinationId} is used in conversion deletion as destination. Cleaning up related conversions and units.`); - // Reload dest unit within transaction to ensure consistency + if (isSuffixRelated(dest)) { + 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); } diff --git a/src/server/services/conversionSimulation.js b/src/server/services/conversionSimulation.js index 23b8bf1a4b..2b1d2b9e7d 100644 --- a/src/server/services/conversionSimulation.js +++ b/src/server/services/conversionSimulation.js @@ -77,12 +77,14 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { { sourceId, destinationId } // The conversion being deleted ]; - if (sourceUnit && sourceUnit.typeOfUnit === 'suffix') { + const isSuffixRelated = (unit) => unit && (unit.typeOfUnit === 'suffix' || (unit.suffix && unit.suffix.trim() !== '')); + + if (isSuffixRelated(sourceUnit)) { const cleanup = simulateSuffixUnitCleanup(sourceUnit, allConversions, allUnits); conversionsToRemove.push(...cleanup.conversionsToRemove); } - - if (destUnit && destUnit.typeOfUnit === 'suffix') { + + if (isSuffixRelated(destUnit)) { const cleanup = simulateSuffixUnitCleanup(destUnit, allConversions, allUnits); conversionsToRemove.push(...cleanup.conversionsToRemove); } From 0887fe1f8c34ec09c81de1700db330176b1a4320 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Mon, 27 Jul 2026 11:42:48 -0700 Subject: [PATCH 08/21] Feat: Add safe unit deletion - Added deleteUnitSafely method - Method checks for dependencies of the unit, clears unit connections to meters, groups, and conversions, then deletes the unit. --- .../services/graph/checkUnitDependencies.js | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/src/server/services/graph/checkUnitDependencies.js b/src/server/services/graph/checkUnitDependencies.js index b9ac4a8cd7..c3bc9fd157 100644 --- a/src/server/services/graph/checkUnitDependencies.js +++ b/src/server/services/graph/checkUnitDependencies.js @@ -5,6 +5,8 @@ const Meter = require('../../models/Meter'); const Group = require('../../models/Group'); const Conversion = require('../../models/Conversion'); +const Unit = require('../../models/Unit'); +const { log } = require('../../log'); /** * Checks if a unit has dependencies that prevent safe deletion. @@ -83,8 +85,48 @@ async function getUnitDependencyDetails(unitId, conn) { }; } +/** + * 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. Also clears any stale cik rows + * @param {number} unitId The unit to delete. + * @param {*} conn The connection to use (should be a transaction). + */ +async function deleteUnitSafely(unitId, 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 conn.none('UPDATE meters SET unit_id = NULL WHERE id = $1', [meter.id]); + } + if (meter.default_graphic_unit === unitId) { + await conn.none('UPDATE meters SET default_graphic_unit = NULL WHERE id = $1', [meter.id]); + } + } + + // Clear any group using this as its default graphic unit. + for (const group of deps.groups) { + await conn.none('UPDATE groups SET default_graphic_unit = NULL WHERE id = $1', [group.id]); + } + + // Delete any other conversions still referencing this unit. + for (const conv of deps.conversions) { + log.info(`Deleting conversion ${conv.source_id}->${conv.destination_id} to allow deletion of unit ${unitId}.`); + await conn.none('DELETE FROM conversions WHERE source_id = $1 AND destination_id = $2', [conv.source_id, conv.destination_id]); + } + + await conn.none('DELETE FROM cik WHERE source_id = $1 OR destination_id = $1', [unitId]); + await Unit.delete(unitId, conn); +} + module.exports = { checkUnitDependencies, canSafelyHideSuffixUnit, - getUnitDependencyDetails + getUnitDependencyDetails, + deleteUnitSafely }; \ No newline at end of file From 5e4e5414f2f63dac75c98572e76c4a3678a30f70 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Mon, 27 Jul 2026 11:55:22 -0700 Subject: [PATCH 09/21] Refactor: Suffix unit deletion instead of hiding - Deletes suffix units instead of hiding them (setting displayable to None) - Calls deleteUnitSafely to check for unit dependacies, deleting the unit, and cleaning cik --- .../services/graph/handleSuffixUnits.js | 38 ++++++++----------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/src/server/services/graph/handleSuffixUnits.js b/src/server/services/graph/handleSuffixUnits.js index 581502cb25..8036b714ea 100644 --- a/src/server/services/graph/handleSuffixUnits.js +++ b/src/server/services/graph/handleSuffixUnits.js @@ -8,6 +8,7 @@ const Conversion = require('../../models/Conversion'); const { getAllPaths } = require('./createConversionGraph'); const { log } = require('../../log'); const { canSafelyHideSuffixUnit } = require('./checkUnitDependencies'); +const { deleteUnitSafely } = require('./checkUnitDependencies'); /** * Adds the new unit and conversions to the database and the conversion graph. @@ -159,8 +160,8 @@ 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 (can be a transaction). * @param {number} depth Current recursion depth to prevent infinite loops (default: 0). @@ -197,36 +198,27 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) // The units that OED adds are suffix units (typeOfUnit === SUFFIX) if (otherUnit.typeOfUnit === Unit.unitType.SUFFIX) { - // Check if unit can be safely hidden (no meter/group dependencies) - const canHide = await canSafelyHideSuffixUnit(otherUnitId, conn); - - // Always delete the conversion, but only hide unit if safe - await Conversion.delete(conversion.sourceId, conversion.destinationId, conn); - + // Always delete the conversion + await Conversion.delete(conversion.sourceId, conversion.destinationId, conn); + // If bidirectional, also delete the reverse conversion if it exists separately if (conversion.bidirectional) { - // Check if reverse conversion exists (some systems store bidirectional as two entries) const reverseConversion = await Conversion.getBySourceDestination( - conversion.destinationId, - conversion.sourceId, + conversion.destinationId, + conversion.sourceId, conn ); if (reverseConversion) { await Conversion.delete(conversion.destinationId, conversion.sourceId, conn); } } - - if (canHide) { - // Hide the destination unit (the other unit in the conversion) - otherUnit.displayable = Unit.displayableType.NONE; - await otherUnit.update(conn); - - // Recursively clean up this unit's related conversions/units - // This handles nested suffix chains (A -> B -> C) - await removeAdditionalConversionsAndUnits(otherUnit, conn, depth + 1); - } else { - log.warn(`Cannot hide suffix unit ${otherUnitId} - has dependencies (meters/groups). Conversion ${conversion.sourceId}->${conversion.destinationId} deleted but unit remains visible.`); - } + + // Delete the auto-created unit (dependency checks + cik cleanup handled inside) + await deleteUnitSafely(otherUnitId, conn); + + // Recursively clean up this unit's related conversions/units + // This handles nested suffix chains (A -> B -> C) + await removeAdditionalConversionsAndUnits(otherUnit, conn, depth + 1); } } catch (err) { log.error(`Error processing conversion ${conversion.sourceId}->${conversion.destinationId} during suffix unit cleanup: ${err}`, err); From d0df4e36bc1649d2c1b57784ef839786914c2795 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 4 Aug 2026 14:08:24 -0700 Subject: [PATCH 10/21] Feat: - 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 --- .../EditConversionModalComponent.tsx | 54 +++++++++++++------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index 472a685bb2..2173c30e8e 100644 --- a/src/client/app/components/conversion/EditConversionModalComponent.tsx +++ b/src/client/app/components/conversion/EditConversionModalComponent.tsx @@ -95,11 +95,18 @@ 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 => + !!unit && ( + unit.typeOfUnit === UnitType.suffix || !!unit.suffix?.trim() + ); + // 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); }; /** @@ -206,16 +213,20 @@ 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(
@@ -226,14 +237,12 @@ export default function EditConversionModalComponent(props: EditConversionModalC } // Suffix unit to Suffix Unit deletion } else if ( - source.typeOfUnit === UnitType.suffix || (source.suffix && source.suffix.trim() !== '') || - dest.typeOfUnit === UnitType.suffix || (dest.suffix && dest.suffix.trim() !== '') - ) { - const suffixUnit = source.typeOfUnit === UnitType.suffix ? source : dest; + 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.destinationId === suffixUnit.id || - (c.bidirectional && (c.sourceId === suffixUnit.id || c.destinationId === suffixUnit.id)) + c.sourceId === suffixUnit.id || + (c.bidirectional && c.destinationId === suffixUnit.id) ); // Find OED-created suffix-type units that would be cascade deleted @@ -242,13 +251,15 @@ export default function EditConversionModalComponent(props: EditConversionModalC const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; return unitDataById[otherId]; }) - .filter(u => u && u.typeOfUnit === UnitType.suffix); + .filter(isSuffixRelated); + + // Get unique conversions that would be deleted const conversionsToDelete = suffixUnitConversions.filter(c => { const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; const otherUnit = unitDataById[otherId]; - return otherUnit && otherUnit.typeOfUnit === UnitType.suffix; + return isSuffixRelated(otherUnit); }); // Check for meters/groups using affected suffix units @@ -266,7 +277,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC group.defaultGraphicUnit !== null && affectedSuffixUnitIds.has(group.defaultGraphicUnit) ); - // Display dependency warnings if any + // Display dependency warnings, affected meters and groups if any if (affectedMetersList.length > 0 || affectedGroupsList.length > 0) { msgElements.push(
@@ -295,20 +306,21 @@ export default function EditConversionModalComponent(props: EditConversionModalC ); } - // Display warnings using React elements for consistency + // Display units that will be cascade deleted if (suffixTypeUnitsToDelete.length > 0) { msgElements.push(
{translate('conversion.delete.suffix.units.to.delete')}:
    {suffixTypeUnitsToDelete.map(u => ( -
  • "{u.name}" ({u.identifier})
    • +
  • "{u.name}"
    • ))}
); } + // List the conversions that will be cascade deleted if (conversionsToDelete.length > 0) { msgElements.push(
@@ -332,7 +344,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC // Run simulation if the previous orphan check passed and it's not cancelled // Now supports suffix units since simulation has been enhanced - if (source.typeOfUnit !== UnitType.suffix && dest.typeOfUnit === UnitType.unit && !cancel) { + if (isSuffixRelated(source) && dest.typeOfUnit === UnitType.unit && !cancel) { try { const result = await triggerSimulate({ sourceId: state.sourceId, @@ -355,6 +367,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC cancel = true; } else { // Group meters by lostUnits + // Display all meters that are loosing the same set of units const meterLossMap = new Map(); result.affectedMeters.forEach(meter => { const key = JSON.stringify([...meter.lostUnits].sort()); @@ -383,6 +396,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC }); // Group non-orphaned groups by lostUnits + // Display all groups loosing the same set of units const groupLossMap = new Map(); const nonOrphanedGroups = result.affectedGroups?.filter(group => !group.orphaned); nonOrphanedGroups.forEach(group => { @@ -448,10 +462,12 @@ export default function EditConversionModalComponent(props: EditConversionModalC ); } }); + // Save twhich 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')} @@ -460,7 +476,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC cancel = true; } } - + // Route to appropriate modal based on any above set cancle condition if (cancel) { msgElements.push(
@@ -471,6 +487,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC setDeleteConfirmationMessage(msgElements); handleCancelModalOpen(); } else { + // Confirm deletion if no blcoking issues msgElements.push(
{translate('conversion.delete.conversion')} [{props.conversionIdentifier}] ? @@ -506,7 +523,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 cleat the references as part of the same delete const payload = { sourceId: state.sourceId, destinationId: state.destinationId, @@ -514,6 +532,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC groupIds: groupsWithLostDefault }; deleteConversion(payload) + // Send delete request .unwrap() .then(() => { // Show source/destination identifiers (not numeric IDs) @@ -523,6 +542,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 + '"' + From 307731453e338b63eca750eeb32ebf9fbe62e340 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 4 Aug 2026 14:09:52 -0700 Subject: [PATCH 11/21] Feat: - Added conversion.delete.suffix.conversions.to.delete prompt for conversion deletion display. --- src/client/app/translations/data.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/client/app/translations/data.ts b/src/client/app/translations/data.ts index 6320e7f85e..ca58299710 100644 --- a/src/client/app/translations/data.ts +++ b/src/client/app/translations/data.ts @@ -92,6 +92,7 @@ const LocaleTranslationData = { "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.disable": "Deleting this suffix conversion will disable use of suffix", + "conversion.delete.suffix.conversions.to.delete": "The following conversions will also be deleted", "conversion.delete.suffix.units.to.delete": "The following units will also be deleted", "conversion.delete.suffix.dependencies.warning": "Warning: Some meters or groups depend on units that will be affected", "conversion.delete.suffix.meters.affected": "Affected meters", @@ -704,6 +705,7 @@ 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.conversions.to.delete": "The following conversions will also be deleted.\u{26A1}", "conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix\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}", @@ -1313,6 +1315,7 @@ 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.conversions.to.delete": "The following conversions will also be deleted\u{26A1}", "conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix\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}", From b1ff92fbe5b9d85b1b1191e3c887b1b8c082b938 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 4 Aug 2026 14:21:05 -0700 Subject: [PATCH 12/21] Refactor: - 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 --- src/server/services/graph/handleSuffixUnits.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/server/services/graph/handleSuffixUnits.js b/src/server/services/graph/handleSuffixUnits.js index 8036b714ea..878e72789d 100644 --- a/src/server/services/graph/handleSuffixUnits.js +++ b/src/server/services/graph/handleSuffixUnits.js @@ -175,11 +175,10 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) } // 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.destinationId === suffixUnit.id || - (conversion.bidirectional && (conversion.sourceId === suffixUnit.id || conversion.destinationId === suffixUnit.id)) - ); + const relatedConversions = allConversions.filter((conversion) => + conversion.sourceId === suffixUnit.id || + (conversion.bidirectional && conversion.destinationId === suffixUnit.id) +); // Process all related conversions in parallel with proper async handling await Promise.all(relatedConversions.map(async (conversion) => { @@ -213,12 +212,13 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) } } - // Delete the auto-created unit (dependency checks + cik cleanup handled inside) - await deleteUnitSafely(otherUnitId, 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 + cik cleanup handled inside) + await deleteUnitSafely(otherUnitId, conn); + } } catch (err) { log.error(`Error processing conversion ${conversion.sourceId}->${conversion.destinationId} during suffix unit cleanup: ${err}`, err); From 2355470d26f800fc44d89cd70fb6a069dd152030 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Tue, 4 Aug 2026 14:30:58 -0700 Subject: [PATCH 13/21] Feat: 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 --- src/server/test/db/suffixUnitDeletionTests.js | 264 ++++++++++++++++++ 1 file changed, 264 insertions(+) create mode 100644 src/server/test/db/suffixUnitDeletionTests.js diff --git a/src/server/test/db/suffixUnitDeletionTests.js b/src/server/test/db/suffixUnitDeletionTests.js new file mode 100644 index 0000000000..10115e2fe9 --- /dev/null +++ b/src/server/test/db/suffixUnitDeletionTests.js @@ -0,0 +1,264 @@ +/* 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'); + +mocha.describe('Suffix unit cascade deletion', () => { + + mocha.it('deletes the auto-created suffix unit and its conversion', async () => { + const conn = testDB.getConnection(); + + const parent = new Unit(undefined, 'Parent', 'Parent', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'test-suffix', Unit.displayableType.ALL, true, ''); + await parent.insert(conn); + + const child = new Unit(undefined, 'Child of test-suffix', 'Child', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await child.insert(conn); + + const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); + await conversion.insert(conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); + expect(childRow).to.equal(null); + + 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 = new Unit(undefined, 'Parent2', 'Parent2', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'test-suffix-2', Unit.displayableType.ALL, true, ''); + await parent.insert(conn); + + const child = new Unit(undefined, 'Child of test-suffix-2', 'Child2', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await child.insert(conn); + + const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); + await conversion.insert(conn); + + 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); + + const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); + expect(childRow).to.equal(null); + + 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 = new Unit(undefined, 'Parent3', 'Parent3', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'test-suffix-3', Unit.displayableType.ALL, true, ''); + await parent.insert(conn); + + const child = new Unit(undefined, 'Child of test-suffix-3', 'Child3', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await child.insert(conn); + + const otherUnit = new Unit(undefined, 'Unrelated Unit', 'Unrelated', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.UNIT, '', Unit.displayableType.ALL, true, ''); + await otherUnit.insert(conn); + + const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); + await conversion.insert(conn); + + // A second, unrelated conversion also pointing at the same child unit + const secondConversion = new Conversion(otherUnit.id, child.id, false, 2, 0, 'second conversion'); + await secondConversion.insert(conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); + expect(childRow).to.equal(null); + + const firstConversionAfter = await Conversion.getBySourceDestination(parent.id, child.id, conn); + expect(firstConversionAfter).to.equal(null); + + // Check that second conversion was also deleted + 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 = new Unit(undefined, 'Parent4', 'Parent4', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'test-suffix-4', Unit.displayableType.ALL, true, ''); + await parent.insert(conn); + + const child = new Unit(undefined, 'Child of test-suffix-4', 'Child4', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await child.insert(conn); + + const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); + await conversion.insert(conn); + + const group = new Group(undefined, 'Test Group', true, gps, 'notes', 33.5, child.id); + await group.insert(conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); + expect(childRow).to.equal(null); + + // unitID defaultGraphicUnit returns -99 when registered as NULL + // Expect the unitID to be -99 instead of null for meters and groups + 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(); + + const unitA = new Unit(undefined, 'A', 'A', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'chain-suffix', Unit.displayableType.ALL, true, ''); + await unitA.insert(conn); + + const unitB = new Unit(undefined, 'B (created by OED)', 'B', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await unitB.insert(conn); + + const unitC = new Unit(undefined, 'C (created by OED)', 'C', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await unitC.insert(conn); + + const convAB = new Conversion(unitA.id, unitB.id, false, 1, 0, 'A to B'); + await convAB.insert(conn); + const convBC = new Conversion(unitB.id, unitC.id, false, 1, 0, 'B to C'); + await convBC.insert(conn); + + await removeAdditionalConversionsAndUnits(unitA, conn); + + const bRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [unitB.id]); + const cRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [unitC.id]); + expect(bRow).to.equal(null); + expect(cRow).to.equal(null); + }); + + mocha.it('does not delete the parent when cleaning up from a child unit', async () => { + const conn = testDB.getConnection(); + + const parent = new Unit(undefined, 'Parent5', 'Parent5', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'test-suffix-5', Unit.displayableType.ALL, true, ''); + await parent.insert(conn); + + const child = new Unit(undefined, 'Child of test-suffix-5', 'Child5', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await child.insert(conn); + + const parentToChild = new Conversion(parent.id, child.id, false, 1, 0, 'parent to child'); + await parentToChild.insert(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 parentRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [parent.id]); + expect(parentRow).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(); + + const parent = new Unit(undefined, 'Parent6', 'Parent6', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'test-suffix-6', Unit.displayableType.ALL, true, ''); + await parent.insert(conn); + + const child = new Unit(undefined, 'Child of test-suffix-6', 'Child6', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await child.insert(conn); + + const forward = new Conversion(parent.id, child.id, true, 1, 0, 'forward'); + await forward.insert(conn); + const reverse = new Conversion(child.id, parent.id, true, 1, 0, 'reverse'); + await reverse.insert(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 = new Unit(undefined, 'Parent7', 'Parent7', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'test-suffix-7', Unit.displayableType.ALL, true, ''); + await parent.insert(conn); + + const child = new Unit(undefined, 'Child of test-suffix-7', 'Child7', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); + await child.insert(conn); + + const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); + await conversion.insert(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); + + const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); + expect(childRow).to.equal(null); + }); + + mocha.it('does not touch a regular (non-suffix) unit in the same conversion set', async () => { + const conn = testDB.getConnection(); + + const parent = new Unit(undefined, 'Parent8', 'Parent8', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.SUFFIX, 'test-suffix-8', Unit.displayableType.ALL, true, ''); + await parent.insert(conn); + + const regularUnit = new Unit(undefined, 'Regular Unit', 'Regular', Unit.unitRepresentType.QUANTITY, + 1000, Unit.unitType.UNIT, '', Unit.displayableType.ALL, true, ''); + await regularUnit.insert(conn); + + const conversion = new Conversion(parent.id, regularUnit.id, false, 1, 0, 'to regular unit'); + await conversion.insert(conn); + + await removeAdditionalConversionsAndUnits(parent, conn); + + // The regular unit and its conversion should be untouched -- this + // function only cleans up OED-created (typeOfUnit === SUFFIX) units. + const regularRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [regularUnit.id]); + expect(regularRow).to.not.equal(null); + + const conversionAfter = await Conversion.getBySourceDestination(parent.id, regularUnit.id, conn); + expect(conversionAfter).to.not.equal(null); + }); + +}); + From aade23e90c9fd7aa7330bb87cba36a4f0808f11a Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Wed, 5 Aug 2026 13:40:56 -0700 Subject: [PATCH 14/21] Refactor: Linted Code --- .../conversion/EditConversionModalComponent.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index 2173c30e8e..db928b750e 100644 --- a/src/client/app/components/conversion/EditConversionModalComponent.tsx +++ b/src/client/app/components/conversion/EditConversionModalComponent.tsx @@ -97,7 +97,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC // 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 => + const isSuffixRelated = (unit? : UnitData): boolean => !!unit && ( unit.typeOfUnit === UnitType.suffix || !!unit.suffix?.trim() ); @@ -135,7 +135,7 @@ 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]; @@ -253,8 +253,6 @@ export default function EditConversionModalComponent(props: EditConversionModalC }) .filter(isSuffixRelated); - - // Get unique conversions that would be deleted const conversionsToDelete = suffixUnitConversions.filter(c => { const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; @@ -496,7 +494,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC setDeleteConfirmationMessage(msgElements); handleDeleteConfirmationModalOpen(); } - + }; /* Confirm Delete Modal */ // Separate from state comment to keep everything related to the warning confirmation modal together From 8f8ea5cf87cf03249ab04e64fb99f8a167158836 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Thu, 17 Sep 2026 11:30:59 -0700 Subject: [PATCH 15/21] Refactor: PR cleanup and SQL implementation. - 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. --- .../EditConversionModalComponent.tsx | 90 ++++++++++--------- src/client/app/translations/data.ts | 20 +++-- src/server/models/Conversion.js | 12 ++- src/server/models/Group.js | 19 ++++ src/server/models/Meter.js | 28 ++++++ src/server/models/Unit.js | 36 ++++++++ src/server/routes/conversions.js | 44 ++++----- src/server/routes/units.js | 4 +- src/server/services/conversionSimulation.js | 13 +-- .../services/graph/checkUnitDependencies.js | 74 +++++++-------- .../services/graph/handleSuffixUnits.js | 1 - src/server/sql/cik/delete_by_unit_id.sql | 5 ++ .../get_id_and_direction_by_unit_id.sql | 7 ++ .../sql/group/clear_default_graphic_unit.sql | 5 ++ .../get_group_by_default_graphic_unit.sql | 7 ++ .../sql/meter/clear_default_graphic_unit.sql | 5 ++ src/server/sql/meter/clear_unit_id.sql | 5 ++ .../get_meter_by_unit_or_default_graphic.sql | 7 ++ .../sql/unit/find_orphaned_suffix_units.sql | 21 +++++ src/server/sql/unit/lock_by_id.sql | 5 ++ src/server/util/suffixUnitCheck.js | 17 ++++ 21 files changed, 304 insertions(+), 121 deletions(-) create mode 100644 src/server/sql/cik/delete_by_unit_id.sql create mode 100644 src/server/sql/conversion/get_id_and_direction_by_unit_id.sql create mode 100644 src/server/sql/group/clear_default_graphic_unit.sql create mode 100644 src/server/sql/group/get_group_by_default_graphic_unit.sql create mode 100644 src/server/sql/meter/clear_default_graphic_unit.sql create mode 100644 src/server/sql/meter/clear_unit_id.sql create mode 100644 src/server/sql/meter/get_meter_by_unit_or_default_graphic.sql create mode 100644 src/server/sql/unit/find_orphaned_suffix_units.sql create mode 100644 src/server/sql/unit/lock_by_id.sql create mode 100644 src/server/util/suffixUnitCheck.js diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index db928b750e..6df9cb6779 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; @@ -97,10 +97,11 @@ export default function EditConversionModalComponent(props: EditConversionModalC // 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 => - !!unit && ( - unit.typeOfUnit === UnitType.suffix || !!unit.suffix?.trim() + 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 = () => { @@ -236,8 +237,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC ); } // Suffix unit to Suffix Unit deletion - } else if ( - isSuffixRelated(source) || isSuffixRelated(dest)) { + } 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 => @@ -245,34 +245,35 @@ export default function EditConversionModalComponent(props: EditConversionModalC (c.bidirectional && c.destinationId === suffixUnit.id) ); - // Find OED-created suffix-type units that would be cascade deleted - const suffixTypeUnitsToDelete = suffixUnitConversions - .map(c => { - const otherId = c.sourceId === suffixUnit.id ? c.destinationId : c.sourceId; - return unitDataById[otherId]; - }) - .filter(isSuffixRelated); - - // Get unique conversions that would be deleted + // 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; - const otherUnit = unitDataById[otherId]; - return isSuffixRelated(otherUnit); + 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]; + }); + // Check for meters/groups using affected suffix units const affectedSuffixUnitIds = new Set(suffixTypeUnitsToDelete.map(u => u.id)); - affectedSuffixUnitIds.add(suffixUnit.id); // Also check the main suffix unit + // 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) || - (meter.defaultGraphicUnit !== null && affectedSuffixUnitIds.has(meter.defaultGraphicUnit)) + affectedSuffixUnitIds.has(meter.defaultGraphicUnit) ); // Check groups using these units (as defaultGraphicUnit) const affectedGroupsList = Object.values(groupDataById).filter(group => - group.defaultGraphicUnit !== null && affectedSuffixUnitIds.has(group.defaultGraphicUnit) + affectedSuffixUnitIds.has(group.defaultGraphicUnit) ); // Display dependency warnings, affected meters and groups if any @@ -281,7 +282,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC
{translate('conversion.delete.suffix.dependencies.warning')} {affectedMetersList.length > 0 && ( -
+
{translate('conversion.delete.suffix.meters.affected')}:
    {affectedMetersList.map(m => ( @@ -291,7 +292,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC
)} {affectedGroupsList.length > 0 && ( -
+
{translate('conversion.delete.suffix.groups.affected')}:
    {affectedGroupsList.map(g => ( @@ -324,13 +325,13 @@ export default function EditConversionModalComponent(props: EditConversionModalC
    {translate('conversion.delete.suffix.conversions.to.delete')}:
      - {conversionsToDelete.map((c, idx) => { - const s = unitDataById[c.sourceId]?.name || unitDataById[c.sourceId]?.identifier || c.sourceId; - const d = unitDataById[c.destinationId]?.name || unitDataById[c.destinationId]?.identifier || c.destinationId; - const bidirectional = c.bidirectional ? ' ↔ ' : ' → '; + {conversionsToDelete.map((conversion, idx) => { + const sourceName = unitDataById[conversion.sourceId].name; + const destinationName = unitDataById[conversion.destinationId].name; + const arrow = conversionArrow(conversion.bidirectional); return ( -
    • - "{s}"{bidirectional}"{d}" +
    • + "{sourceName}"{arrow}"{destinationName}"
      • ); })} @@ -365,7 +366,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC cancel = true; } else { // Group meters by lostUnits - // Display all meters that are loosing the same set of units + // 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()); @@ -394,7 +395,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC }); // Group non-orphaned groups by lostUnits - // Display all groups loosing the same set of units + // Display all groups losing the same set of units const groupLossMap = new Map(); const nonOrphanedGroups = result.affectedGroups?.filter(group => !group.orphaned); nonOrphanedGroups.forEach(group => { @@ -460,7 +461,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC ); } }); - // Save twhich meters.groups need their default unit cleared + // Save which meters.groups need their default unit cleared setMetersWithLostDefault(metersLostDefault); setGroupsWithLostDefault(groupsLostDefault); } @@ -474,22 +475,27 @@ export default function EditConversionModalComponent(props: EditConversionModalC cancel = true; } } - // Route to appropriate modal based on any above set cancle condition + // Process the appropriate modal based on the cancel conditions set above if (cancel) { - msgElements.push( -
      -
      - {translate('conversion.delete.restricted')} -
      - ); + msgElements.push( +
      +
      + + {translate('conversion.delete.restricted')} + +
      + ); setDeleteConfirmationMessage(msgElements); handleCancelModalOpen(); } else { - // Confirm deletion if no blcoking issues + // Confirm deletion if no blocking issues msgElements.push(
      - {translate('conversion.delete.conversion')} [{props.conversionIdentifier}] ? -
      +
      + + {translate('conversion.delete.conversion')} [{props.conversionIdentifier}] ? + +
    ); setDeleteConfirmationMessage(msgElements); handleDeleteConfirmationModalOpen(); @@ -522,7 +528,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC // 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 cleat the references as part of the same delete + // Backend will clear the references as part of the same delete const payload = { sourceId: state.sourceId, destinationId: state.destinationId, diff --git a/src/client/app/translations/data.ts b/src/client/app/translations/data.ts index d7f68a0f6c..8ff3e82ca0 100644 --- a/src/client/app/translations/data.ts +++ b/src/client/app/translations/data.ts @@ -98,12 +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.disable": "Deleting this suffix conversion will disable use of suffix", "conversion.delete.suffix.conversions.to.delete": "The following conversions will also be deleted", - "conversion.delete.suffix.units.to.delete": "The following units will also be deleted", "conversion.delete.suffix.dependencies.warning": "Warning: Some meters or groups depend on units that will be affected", - "conversion.delete.suffix.meters.affected": "Affected meters", + "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:", @@ -722,7 +722,11 @@ const LocaleTranslationData = { "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.conversions.to.delete": "The following conversions will also be deleted.\u{26A1}", - "conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix\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}", @@ -1340,8 +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.conversions.to.delete": "The following conversions will also be deleted\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/server/models/Conversion.js b/src/server/models/Conversion.js index ce783913e9..14799a44a0 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 getConversionsByUnitID(unitId, conn) { + return await conn.any(sqlFile('conversion/get_id_and_direction_by_unit_id.sql'), { unitId }); + } + /** * Inserts a new conversion to the database. * @param {*} conn The connection to use. @@ -96,7 +107,6 @@ class Conversion { destination: destination }); } - } module.exports = Conversion; diff --git a/src/server/models/Group.js b/src/server/models/Group.js index ce90087c74..1fe27cc621 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_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..1db208d238 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_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_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..4b674e9b53 100644 --- a/src/server/models/Unit.js +++ b/src/server/models/Unit.js @@ -186,6 +186,42 @@ 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_by_id.sql'), { id: id }); + return Unit.mapRow(row); + } + + /** + * Finds suffix-type units that may have been orphaned (left visible with + * no remaining conversions) after cleaning up around the given source/destination + * units. Used as a post-delete sanity check, not a guarantee. + * @param {number} sourceId The conversion's source unit id. + * @param {number} destinationId The conversion's destination unit id. + * @param {*} conn The connection to use. + * @returns {Promise.>} + */ + static async findOrphanedSuffixUnits(sourceId, destinationId, conn) { + return await conn.any(sqlFile('unit/find_orphaned_suffix_units.sql'), { sourceId, destinationId }); + } + // 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 cb9ee97316..774260b5fd 100644 --- a/src/server/routes/conversions.js +++ b/src/server/routes/conversions.js @@ -13,9 +13,13 @@ const { HTTP_CODES } = require('../util/httpCodes'); const validate = require('jsonschema').validate; const { simulateDeleteConversion } = require('../services/conversionSimulation'); -const { checkUnitDependencies, getUnitDependencyDetails } = require('../services/graph/checkUnitDependencies'); +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(); @@ -195,8 +199,6 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re // 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 isSuffixRelated = (unit) => unit.typeOfUnit === 'suffix' || (unit.suffix && unit.suffix.trim() !== ''); - const suffixUnitsToCheck = []; if (isSuffixRelated(source)) { suffixUnitsToCheck.push({ unit: source, role: 'source' }); @@ -262,42 +264,28 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re await Conversion.delete(destinationId, sourceId, t); } } - // Update meters if any + // 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 no orphaned suffix units after cleanup (performance: only check if suffix units were involved) - if (source.typeOfUnit === 'suffix' || dest.typeOfUnit === 'suffix') { + // 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 = []; + // Track if failures occured during orphan check + let orphanedCheckFailed = false; + 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 - const orphanedUnits = await conn.any(` - SELECT u.id, u.name - FROM units u - WHERE u.type_of_unit = 'suffix'::unit_type - AND u.displayable != 'none'::displayable_type - AND (u.id = $1 OR u.id = $2 OR u.id IN ( - SELECT DISTINCT CASE - WHEN c.source_id IN ($1, $2) THEN c.destination_id - WHEN c.destination_id IN ($1, $2) THEN c.source_id - END - FROM conversions c - WHERE (c.source_id IN ($1, $2) OR c.destination_id IN ($1, $2)) - )) - AND NOT EXISTS ( - SELECT 1 FROM conversions c - WHERE (c.source_id = u.id OR c.destination_id = u.id) - ) - LIMIT 100 - `, [sourceId, destinationId]); + orphanedUnits = await Unit.findOrphanedSuffixUnits(sourceId, destinationId, 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 diff --git a/src/server/routes/units.js b/src/server/routes/units.js index 4b0cf82687..bb9f518297 100644 --- a/src/server/routes/units.js +++ b/src/server/routes/units.js @@ -257,7 +257,7 @@ router.post('/delete', adminAuthMiddleware('delete units'), async (req, res) => // Perform all operations in a single transaction for atomicity await conn.tx(async t => { // Lock the unit to prevent concurrent modifications - await t.one('SELECT * FROM units WHERE id = $1 FOR UPDATE', [unitId]); + await Unit.lockById(unitId, t); // Reload unit within transaction to ensure consistency const unitInTx = await Unit.getById(unitId, t); @@ -270,7 +270,7 @@ router.post('/delete', adminAuthMiddleware('delete units'), async (req, res) => // Handle units with suffix string (not suffix-type) // These units may have conversions involving suffix units that need cleanup - if (unitInTx.suffix && unitInTx.suffix.trim() !== '') { + if (unitInTx.suffix && unitInTx.suffix !== '') { log.info(`Unit ${unitId} has suffix "${unitInTx.suffix}". Checking for suffix-involved conversions.`); // Get all conversions involving this unit diff --git a/src/server/services/conversionSimulation.js b/src/server/services/conversionSimulation.js index 2b1d2b9e7d..0c6a21da50 100644 --- a/src/server/services/conversionSimulation.js +++ b/src/server/services/conversionSimulation.js @@ -9,6 +9,8 @@ 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'); +const { getOtherConnections } = require('./graph/checkUnitDependencies'); /** * Simulates what conversions and units would be removed when deleting a conversion involving suffix units. @@ -74,16 +76,17 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { // 3. Simulate suffix unit cleanup if applicable let conversionsToRemove = [ - { sourceId, destinationId } // The conversion being deleted + // The conversion being deleted + { sourceId, destinationId } ]; - - const isSuffixRelated = (unit) => unit && (unit.typeOfUnit === 'suffix' || (unit.suffix && unit.suffix.trim() !== '')); - + // 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); } - + // 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); diff --git a/src/server/services/graph/checkUnitDependencies.js b/src/server/services/graph/checkUnitDependencies.js index c3bc9fd157..4fdcaf655f 100644 --- a/src/server/services/graph/checkUnitDependencies.js +++ b/src/server/services/graph/checkUnitDependencies.js @@ -7,35 +7,26 @@ 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 with hasDependencies boolean and details + * @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) - conn.any(` - SELECT id, name, unit_id, default_graphic_unit - FROM meters - WHERE unit_id = $1 OR default_graphic_unit = $1 - `, [unitId]), - + Meter.getByUnitOrDefaultGraphic(unitId, conn), // Check if any groups use this unit as default_graphic_unit - conn.any(` - SELECT id, name, default_graphic_unit - FROM groups - WHERE default_graphic_unit = $1 - `, [unitId]), - + Group.getByDefaultGraphicUnit(unitId, conn), // Check if unit is used in any conversions (as source or destination) - conn.any(` - SELECT source_id, destination_id, bidirectional - FROM conversions - WHERE source_id = $1 OR destination_id = $1 - `, [unitId]) + Conversion.getConversionsByUnitID(unitId,conn) ]); return { @@ -47,25 +38,36 @@ async function checkUnitDependencies(unitId, conn) { } /** - * Checks if a suffix-type unit (created by OED) can be safely hidden. - * These units should only be hidden if they're not used by meters/groups. - * Conversions are expected and will be cleaned up separately. - * @param {number} unitId The suffix unit ID to check - * @param {*} conn Database connection (can be a transaction) - * @returns {Promise} True if safe to hide + * Checks if a unit has any other conversions beyond the one specified. + * Used to detect whether deleting a conversion would sever connections + * that an admin may not expect. Like orphaning a unit via cascade deletion. + * @param {number} unitId The unit to check. + * @param {number} excludeSourceId The source id of the conversion to exclude from the check. + * @param {number} excludeDestinationId The destination id of the conversion to exclude from the check. + * @param {*} conn The connection to use. + * @returns {Promise.} The other conversions, if any. */ -async function canSafelyHideSuffixUnit(unitId, conn) { - const deps = await checkUnitDependencies(unitId, conn); - // Suffix units created by OED can be hidden if no meters/groups depend on them - // (conversions are expected and will be cleaned up) - return deps.meters.length === 0 && deps.groups.length === 0; +async function getOtherConnections(unitId, excludeSourceId, excludeDestinationId, conn) { + const allConversions = await Conversion.getConversionsByUnitID(unitId, conn); + // Exclude converstions that contain the source or destination Id provided from results + return allConversions.filter( conv => + !((conv.sourceId === excludeSourceId && conv.destinationId === excludeDestinationId) || + (conv.sourceId === excludeDestinationId && conv.destinationId === excludeSourceId)) + ); } /** * Gets detailed dependency information for UI warnings. * @param {number} unitId The unit ID to check * @param {*} conn Database connection - * @returns {Promise} Detailed dependency info + * @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); @@ -75,12 +77,12 @@ async function getUnitDependencyDetails(unitId, conn) { meterCount: deps.meters.length, groupCount: deps.groups.length, conversionCount: deps.conversions.length, - meters: deps.meters.map(m => ({ id: m.id, name: m.name })), - groups: deps.groups.map(g => ({ id: g.id, name: g.name })), - conversions: deps.conversions.map(c => ({ - sourceId: c.source_id, - destinationId: c.destination_id, - bidirectional: c.bidirectional + 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 })) }; } diff --git a/src/server/services/graph/handleSuffixUnits.js b/src/server/services/graph/handleSuffixUnits.js index 878e72789d..f3b30d7237 100644 --- a/src/server/services/graph/handleSuffixUnits.js +++ b/src/server/services/graph/handleSuffixUnits.js @@ -7,7 +7,6 @@ const { pathConversion } = require('./pathConversion'); const Conversion = require('../../models/Conversion'); const { getAllPaths } = require('./createConversionGraph'); const { log } = require('../../log'); -const { canSafelyHideSuffixUnit } = require('./checkUnitDependencies'); const { deleteUnitSafely } = require('./checkUnitDependencies'); /** diff --git a/src/server/sql/cik/delete_by_unit_id.sql b/src/server/sql/cik/delete_by_unit_id.sql new file mode 100644 index 0000000000..c256c0da20 --- /dev/null +++ b/src/server/sql/cik/delete_by_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/. */ + +DELETE FROM cik WHERE source_id = ${unitId} OR destination_id = ${unitId}; \ No newline at end of file diff --git a/src/server/sql/conversion/get_id_and_direction_by_unit_id.sql b/src/server/sql/conversion/get_id_and_direction_by_unit_id.sql new file mode 100644 index 0000000000..7c766d623c --- /dev/null +++ b/src/server/sql/conversion/get_id_and_direction_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_default_graphic_unit.sql b/src/server/sql/group/clear_default_graphic_unit.sql new file mode 100644 index 0000000000..5302cb12d3 --- /dev/null +++ b/src/server/sql/group/clear_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_default_graphic_unit.sql b/src/server/sql/meter/clear_default_graphic_unit.sql new file mode 100644 index 0000000000..cc11f89a97 --- /dev/null +++ b/src/server/sql/meter/clear_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_unit_id.sql b/src/server/sql/meter/clear_unit_id.sql new file mode 100644 index 0000000000..99ffbde2c8 --- /dev/null +++ b/src/server/sql/meter/clear_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_suffix_units.sql b/src/server/sql/unit/find_orphaned_suffix_units.sql new file mode 100644 index 0000000000..832293ac21 --- /dev/null +++ b/src/server/sql/unit/find_orphaned_suffix_units.sql @@ -0,0 +1,21 @@ +/* This Source Code Form is subject to the terms of the Mozilla Punblic + * 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.type_of_unit = 'suffix'::unit_type +AND unit.displayable != 'none'::displayable_type +AND (unit.id = ${sourceId} OR unit.id = ${destinationId} OR unit.id IN ( + SELECT DISTINCT CASE + WHEN conv.source_id IN (${sourceId}, ${destinationId}) THEN conv.destination_id + WHEN conv.destination_id IN (${sourceId}, ${destinationId}) THEN conv.source_id + END + FROM conversions conv + WHERE (conv.source_id IN (${sourceId}, ${destinationId}) OR conv.destination_id IN (${sourceId}, ${destinationId})) +)) +AND NOT EXISTS ( + SELECT 1 FROM conversions conv + WHERE (conv.source_id = unit.id OR conv.destination_id = unit.id) +) +LIMIT 100; \ No newline at end of file diff --git a/src/server/sql/unit/lock_by_id.sql b/src/server/sql/unit/lock_by_id.sql new file mode 100644 index 0000000000..3ae4081e9d --- /dev/null +++ b/src/server/sql/unit/lock_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/util/suffixUnitCheck.js b/src/server/util/suffixUnitCheck.js new file mode 100644 index 0000000000..6e0e41d648 --- /dev/null +++ b/src/server/util/suffixUnitCheck.js @@ -0,0 +1,17 @@ +/* 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 From 1c095a439d3dc58df39d2558df0b74893e3f9678 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Fri, 18 Sep 2026 17:29:23 -0700 Subject: [PATCH 16/21] Refactor: redoCik and orphan unit detection - 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. --- .../EditConversionModalComponent.tsx | 14 ++ src/client/app/types/redux/conversions.ts | 6 + src/server/models/Cik.js | 13 ++ src/server/models/Unit.js | 4 +- src/server/routes/conversions.js | 25 ++-- src/server/routes/units.js | 8 +- src/server/services/conversionSimulation.js | 38 ++++-- .../services/graph/checkUnitDependencies.js | 39 ++---- .../services/graph/handleSuffixUnits.js | 127 +++++++++++------- .../sql/unit/find_orphaned_suffix_units.sql | 21 --- src/server/sql/unit/find_orphaned_units.sql | 12 ++ src/server/util/suffixUnitCheck.js | 4 +- 12 files changed, 195 insertions(+), 116 deletions(-) delete mode 100644 src/server/sql/unit/find_orphaned_suffix_units.sql create mode 100644 src/server/sql/unit/find_orphaned_units.sql diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index 6df9cb6779..af99ee104e 100644 --- a/src/client/app/components/conversion/EditConversionModalComponent.tsx +++ b/src/client/app/components/conversion/EditConversionModalComponent.tsx @@ -350,6 +350,20 @@ export default function EditConversionModalComponent(props: EditConversionModalC destinationId: state.destinationId }).unwrap(); + // Orphaned Suffix unit check + if (result.potentiallyOrphanedUnits?.length > 0) { + msgElements.push( +
    + {translate('conversion.delete.unit.orphan')}: +
      + {result.potentiallyOrphanedUnits.map((u) => ( +
    • "{u.name}"
      • + ))} +
    +
    + ); + } + // Orphaned groups check const orphanedGroups = result.affectedGroups?.filter(group => group.orphaned); if (orphanedGroups.length > 0) { diff --git a/src/client/app/types/redux/conversions.ts b/src/client/app/types/redux/conversions.ts index b4a2619abc..879f7ff8f8 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; +} + +export 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..a3ab97ed73 100644 --- a/src/server/models/Cik.js +++ b/src/server/models/Cik.js @@ -52,6 +52,19 @@ class Cik { return rows.map(Cik.mapRow); } + /** + * Deletes cik rows directly referencing the given unit. This is a + * stopgap to satisfy the foreign key on cik before deleting a unit's + * row. It does not catch rows for unrelated unit pairs whose path + * happened to route through this unit. redoCik should be called + * afterward for a fully correct recalculation. + * @param {number} unitId The unit's id. + * @param {*} conn The connection to use. + */ + static async deleteByUnitId(unitId, conn) { + await conn.none(sqlFile('cik/delete_by_unit_id.sql'), { unitId }); + } + /** * 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/Unit.js b/src/server/models/Unit.js index 4b674e9b53..e65899384a 100644 --- a/src/server/models/Unit.js +++ b/src/server/models/Unit.js @@ -218,8 +218,8 @@ class Unit { * @param {*} conn The connection to use. * @returns {Promise.>} */ - static async findOrphanedSuffixUnits(sourceId, destinationId, conn) { - return await conn.any(sqlFile('unit/find_orphaned_suffix_units.sql'), { sourceId, destinationId }); + static async findOrphanedUnits(conn) { + return await conn.any(sqlFile('unit/find_orphaned_units.sql')); } // TODO: Returns a special value if it doesn't exist diff --git a/src/server/routes/conversions.js b/src/server/routes/conversions.js index 774260b5fd..3e66a3000f 100644 --- a/src/server/routes/conversions.js +++ b/src/server/routes/conversions.js @@ -235,22 +235,25 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re // Perform all operations in a single transaction for atomicity await conn.tx(async t => { // Lock the units to prevent concurrent modifications - await t.one('SELECT * FROM units WHERE id = $1 FOR UPDATE', [sourceId]); - await t.one('SELECT * FROM units WHERE id = $1 FOR UPDATE', [destinationId]); + 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 - const isSuffixRelated = (unit) => unit.typeOfUnit === 'suffix' || (unit.suffix && unit.suffix.trim() !== ''); - 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)) { - 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); + 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 @@ -279,21 +282,19 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re // 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 = []; - // Track if failures occured during orphan check - let orphanedCheckFailed = false; 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.findOrphanedSuffixUnits(sourceId, destinationId, conn); + 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 check, log but don't fail - log.warn(`Error checking for orphaned units: ${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}`); } } success(res, 'Successfully deleted conversion and updated meters/groups'); diff --git a/src/server/routes/units.js b/src/server/routes/units.js index bb9f518297..c0bfc83ae9 100644 --- a/src/server/routes/units.js +++ b/src/server/routes/units.js @@ -14,6 +14,8 @@ 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 { @@ -231,7 +233,6 @@ router.post('/delete', adminAuthMiddleware('delete units'), async (req, res) => return; } // Check for dependencies before deletion - const { checkUnitDependencies } = require('../services/graph/checkUnitDependencies'); const deps = await checkUnitDependencies(unitId, conn); // If unit has meter/group dependencies, provide detailed error @@ -303,6 +304,11 @@ router.post('/delete', adminAuthMiddleware('delete units'), async (req, res) => // 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}`; diff --git a/src/server/services/conversionSimulation.js b/src/server/services/conversionSimulation.js index 0c6a21da50..ef9bc3a6d4 100644 --- a/src/server/services/conversionSimulation.js +++ b/src/server/services/conversionSimulation.js @@ -10,7 +10,6 @@ const Unit = require('../models/Unit'); const { createConversionGraph, createConversionGraphFromArray } = require('./graph/createConversionGraph'); const { intersectSets, compatibleUnitsForMeter } = require('../util/compatibleUnits'); const { isSuffixRelated } = require('../util/suffixUnitCheck'); -const { getOtherConnections } = require('./graph/checkUnitDependencies'); /** * Simulates what conversions and units would be removed when deleting a conversion involving suffix units. @@ -22,12 +21,11 @@ const { getOtherConnections } = require('./graph/checkUnitDependencies'); */ function simulateSuffixUnitCleanup(suffixUnit, allConversions, allUnits) { const conversionsToRemove = []; - const unitsToHide = []; + const unitsToDelete = []; // Find all conversions involving this suffix unit const relatedConversions = allConversions.filter((conversion) => - conversion.sourceId === suffixUnit.id || - conversion.destinationId === suffixUnit.id || + conversion.sourceId === suffixUnit.id || (conversion.bidirectional && (conversion.sourceId === suffixUnit.id || conversion.destinationId === suffixUnit.id)) ); @@ -52,13 +50,13 @@ function simulateSuffixUnitCleanup(suffixUnit, allConversions, allUnits) { }); } - if (!unitsToHide.includes(otherUnitId)) { - unitsToHide.push(otherUnitId); + if (!unitsToDelete.includes(otherUnitId)) { + unitsToDelete.push(otherUnitId); } } } - return { conversionsToRemove, unitsToHide }; + return { conversionsToRemove, unitsToDelete}; } async function simulateDeleteConversion({ sourceId, destinationId }, conn) { @@ -79,17 +77,22 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { // 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) @@ -99,11 +102,28 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { // 4. Remove all affected conversions from the simulation const newConversions = allConversions.filter(c => { - return !uniqueConversionsToRemove.some(toRemove => + // 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 visable, and have zero conversions post delete. + const potentiallyOrphanedUnits = allUnits.filter(u => + u.displayable !== Unit.displayableType.NONE && + !allUnitsToDelete.includes(u.id) && + !newConversions.some(c => c.sourceId === u.id || c.destinationId === u.id) + ).map(u => ({ id: u.id, name: u.name })); + + // 5. Build simulated graph and Cik array const simulatedGraph = createConversionGraphFromArray(allUnits, newConversions); const simulatedCik = await createCikArray(simulatedGraph, conn); @@ -163,7 +183,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 index 4fdcaf655f..62daecc2b2 100644 --- a/src/server/services/graph/checkUnitDependencies.js +++ b/src/server/services/graph/checkUnitDependencies.js @@ -37,25 +37,6 @@ async function checkUnitDependencies(unitId, conn) { }; } -/** - * Checks if a unit has any other conversions beyond the one specified. - * Used to detect whether deleting a conversion would sever connections - * that an admin may not expect. Like orphaning a unit via cascade deletion. - * @param {number} unitId The unit to check. - * @param {number} excludeSourceId The source id of the conversion to exclude from the check. - * @param {number} excludeDestinationId The destination id of the conversion to exclude from the check. - * @param {*} conn The connection to use. - * @returns {Promise.} The other conversions, if any. - */ -async function getOtherConnections(unitId, excludeSourceId, excludeDestinationId, conn) { - const allConversions = await Conversion.getConversionsByUnitID(unitId, conn); - // Exclude converstions that contain the source or destination Id provided from results - return allConversions.filter( conv => - !((conv.sourceId === excludeSourceId && conv.destinationId === excludeDestinationId) || - (conv.sourceId === excludeDestinationId && conv.destinationId === excludeSourceId)) - ); -} - /** * Gets detailed dependency information for UI warnings. * @param {number} unitId The unit ID to check @@ -104,31 +85,39 @@ async function deleteUnitSafely(unitId, conn) { 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 conn.none('UPDATE meters SET unit_id = NULL WHERE id = $1', [meter.id]); + await Meter.clearUnitId(meter.id, conn); } if (meter.default_graphic_unit === unitId) { - await conn.none('UPDATE meters SET default_graphic_unit = NULL WHERE id = $1', [meter.id]); + await Meter.clearDefaultGraphicUnit(meter.id, conn); } } // Clear any group using this as its default graphic unit. for (const group of deps.groups) { - await conn.none('UPDATE groups SET default_graphic_unit = NULL WHERE id = $1', [group.id]); + 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 conn.none('DELETE FROM conversions WHERE source_id = $1 AND destination_id = $2', [conv.source_id, conv.destination_id]); + await Conversion.delete(conv.source_id, conv.destination_id, conn); } - await conn.none('DELETE FROM cik WHERE source_id = $1 OR destination_id = $1', [unitId]); + await Cik.deleteByUnitId(unitId, conn); await Unit.delete(unitId, conn); } module.exports = { checkUnitDependencies, - canSafelyHideSuffixUnit, 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 f3b30d7237..a6bc4ab9ed 100644 --- a/src/server/services/graph/handleSuffixUnits.js +++ b/src/server/services/graph/handleSuffixUnits.js @@ -165,70 +165,107 @@ async function handleSuffixUnits(graph, conn) { * @param {*} conn The connection to use (can be a transaction). * @param {number} depth Current recursion depth to prevent infinite loops (default: 0). */ -const MAX_SUFFIX_CLEANUP_DEPTH = 10; 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}`); } - // 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) -); - - // Process all related conversions in parallel with proper async handling - await Promise.all(relatedConversions.map(async (conversion) => { - try { + // Track units being deleted to notify admin of the internal change + + 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 otherUnit = await Unit.getById(otherUnitId, conn); - - // Validate that the other unit exists - if (!otherUnit) { - log.warn(`Unit ${otherUnitId} not found when cleaning up suffix unit ${suffixUnit.id}. Conversion ${conversion.sourceId}->${conversion.destinationId} may be orphaned.`); - return; - } - - // The units that OED adds are suffix units (typeOfUnit === SUFFIX) - if (otherUnit.typeOfUnit === Unit.unitType.SUFFIX) { - // Always delete the conversion - await Conversion.delete(conversion.sourceId, conversion.destinationId, 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); - } - } + const exists = await Unit.exists(otherUnitId, conn); + const otherUnit = exists ? await Unit.getById(otherUnitId, conn) : null; + return { conversion, otherUnitId, otherUnit }; + })); - // Recursively clean up this unit's related conversions/units - // This handles nested suffix chains (A -> B -> C) - await removeAdditionalConversionsAndUnits(otherUnit, conn, depth + 1); + // 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; + } - // Delete the auto-created unit (dependency checks + cik cleanup handled inside) - await deleteUnitSafely(otherUnitId, conn); + // 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.getConversionsByUnitID(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); + + // 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 + cik cleanup handled inside) + await deleteUnitSafely(otherUnitId, conn); + } catch (err) { log.error(`Error processing conversion ${conversion.sourceId}->${conversion.destinationId} during suffix unit cleanup: ${err}`, err); - // Continue processing other conversions even if one fails + throw err; } - })); - + } + // Restore the suffix unit's displayable status suffixUnit.displayable = Unit.displayableType.ALL; await suffixUnit.update(conn); -} + } module.exports = { handleSuffixUnits, diff --git a/src/server/sql/unit/find_orphaned_suffix_units.sql b/src/server/sql/unit/find_orphaned_suffix_units.sql deleted file mode 100644 index 832293ac21..0000000000 --- a/src/server/sql/unit/find_orphaned_suffix_units.sql +++ /dev/null @@ -1,21 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Punblic - * 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.type_of_unit = 'suffix'::unit_type -AND unit.displayable != 'none'::displayable_type -AND (unit.id = ${sourceId} OR unit.id = ${destinationId} OR unit.id IN ( - SELECT DISTINCT CASE - WHEN conv.source_id IN (${sourceId}, ${destinationId}) THEN conv.destination_id - WHEN conv.destination_id IN (${sourceId}, ${destinationId}) THEN conv.source_id - END - FROM conversions conv - WHERE (conv.source_id IN (${sourceId}, ${destinationId}) OR conv.destination_id IN (${sourceId}, ${destinationId})) -)) -AND NOT EXISTS ( - SELECT 1 FROM conversions conv - WHERE (conv.source_id = unit.id OR conv.destination_id = unit.id) -) -LIMIT 100; \ 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..e26fc04bbe --- /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 Punblic + * 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 100; \ No newline at end of file diff --git a/src/server/util/suffixUnitCheck.js b/src/server/util/suffixUnitCheck.js index 6e0e41d648..4f72e895f8 100644 --- a/src/server/util/suffixUnitCheck.js +++ b/src/server/util/suffixUnitCheck.js @@ -14,4 +14,6 @@ function isSuffixRelated(unit) { return !!unit && (unit.typeOfUnit === 'suffix' || unit.suffix !== ''); } -module.exports = { isSuffixRelated }; \ No newline at end of file +module.exports = { + isSuffixRelated + }; \ No newline at end of file From 2bcf3546283e72a8a3a38ba6a50dd6a8bdef25db Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Sun, 20 Sep 2026 13:45:25 -0700 Subject: [PATCH 17/21] Refactor: UI display and Unit Tests. - 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. --- .../EditConversionModalComponent.tsx | 125 +++++--- src/client/app/styles/modal.css | 16 + .../services/graph/checkUnitDependencies.js | 15 +- .../services/graph/handleSuffixUnits.js | 25 +- src/server/test/db/suffixUnitDeletionTests.js | 286 +++++++++--------- 5 files changed, 261 insertions(+), 206 deletions(-) diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index af99ee104e..c5365607b3 100644 --- a/src/client/app/components/conversion/EditConversionModalComponent.tsx +++ b/src/client/app/components/conversion/EditConversionModalComponent.tsx @@ -97,7 +97,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC // 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 => { + const isSuffixRelated = (unit?: UnitData): boolean => { return !!unit && ( unit.typeOfUnit === UnitType.suffix || unit.suffix !== '' ); @@ -107,7 +107,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC const isSuffixUsed = () => { const source = unitDataById[state.sourceId]; const dest = unitDataById[state.destinationId]; - return isSuffixRelated(source) || isSuffixRelated(dest); + return isSuffixRelated(source) || isSuffixRelated(dest); }; /** @@ -143,6 +143,8 @@ export default function EditConversionModalComponent(props: EditConversionModalC 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) { @@ -232,7 +234,9 @@ export default function EditConversionModalComponent(props: EditConversionModalC msgElements.push(
    {translate('conversion.delete.unit.orphan')} - "{dest.name}". +
      +
    • "{dest.name}"
      • +
    ); } @@ -259,11 +263,12 @@ export default function EditConversionModalComponent(props: EditConversionModalC 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(suffixTypeUnitsToDelete.map(u => u.id)); + const affectedSuffixUnitIds = new Set(suffixTypeUnitsToDeleteIds); // Also check the main suffix unit - affectedSuffixUnitIds.add(suffixUnit.id); + affectedSuffixUnitIds.add(suffixUnit.id); // Check meters using these units (as unitId or defaultGraphicUnit) const affectedMetersList = Object.values(meterDataById).filter(meter => @@ -357,7 +362,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC {translate('conversion.delete.unit.orphan')}:
      {result.potentiallyOrphanedUnits.map((u) => ( -
    • "{u.name}"
      • +
    • "{u.name}"
      • ))}
    @@ -389,24 +394,36 @@ 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, i: 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 @@ -419,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, i: number) => ( +
    • + "{unitDataById[id]?.name || id}" + {suffixTypeUnitsToDeleteIds.has(id) ? ' (created by OED)' : ''} +
      • + ))} +
    +
    + ); + })}
    ); - }); + } // After you get result from simulateDeleteConversion check default graphic units const metersLostDefault: number[] = []; @@ -491,25 +520,25 @@ export default function EditConversionModalComponent(props: EditConversionModalC } // Process the appropriate modal based on the cancel conditions set above if (cancel) { - msgElements.push( -
    -
    - - {translate('conversion.delete.restricted')} - -
    - ); + msgElements.push( +
    +
    + + {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(); 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/server/services/graph/checkUnitDependencies.js b/src/server/services/graph/checkUnitDependencies.js index 62daecc2b2..b7208f9137 100644 --- a/src/server/services/graph/checkUnitDependencies.js +++ b/src/server/services/graph/checkUnitDependencies.js @@ -98,15 +98,12 @@ async function deleteUnitSafely(unitId, 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. - */ + // 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); diff --git a/src/server/services/graph/handleSuffixUnits.js b/src/server/services/graph/handleSuffixUnits.js index a6bc4ab9ed..7ab72fe013 100644 --- a/src/server/services/graph/handleSuffixUnits.js +++ b/src/server/services/graph/handleSuffixUnits.js @@ -143,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. @@ -191,7 +194,7 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) 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; + const otherUnit = exists ? await Unit.getById(otherUnitId, conn) : null; return { conversion, otherUnitId, otherUnit }; })); @@ -212,7 +215,7 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) // 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; @@ -220,20 +223,20 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) // The units that OED adds are suffix units (typeOfUnit === SUFFIX) try { - const {conversion, otherUnitId, otherUnit } = target; + 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.getConversionsByUnitID(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)) - ); + (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); @@ -255,7 +258,7 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) // Delete the auto-created unit (dependency checks + cik cleanup handled inside) await deleteUnitSafely(otherUnitId, conn); - + } catch (err) { log.error(`Error processing conversion ${conversion.sourceId}->${conversion.destinationId} during suffix unit cleanup: ${err}`, err); throw err; @@ -263,9 +266,13 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) } // Restore the suffix unit's displayable status + // Note: This funtion 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 + // dericed units from it during the next graph rebuild. + // See note there for the full mechanism. suffixUnit.displayable = Unit.displayableType.ALL; await suffixUnit.update(conn); - } +} module.exports = { handleSuffixUnits, diff --git a/src/server/test/db/suffixUnitDeletionTests.js b/src/server/test/db/suffixUnitDeletionTests.js index 10115e2fe9..af349a90d9 100644 --- a/src/server/test/db/suffixUnitDeletionTests.js +++ b/src/server/test/db/suffixUnitDeletionTests.js @@ -10,27 +10,63 @@ 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.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: 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 = new Unit(undefined, 'Parent', 'Parent', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'test-suffix', Unit.displayableType.ALL, true, ''); - await parent.insert(conn); - - const child = new Unit(undefined, 'Child of test-suffix', 'Child', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await child.insert(conn); - - const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); - await conversion.insert(conn); + const { parent, child } = await setupParentChild(conn); await removeAdditionalConversionsAndUnits(parent, conn); - const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); - expect(childRow).to.equal(null); + await expectUnitDeleted(child.id, conn); const conversionAfter = await Conversion.getBySourceDestination(parent.id, child.id, conn); expect(conversionAfter).to.equal(null); @@ -38,18 +74,10 @@ mocha.describe('Suffix unit cascade deletion', () => { mocha.it('clears a meter\'s unit_id instead of blocking deletion', async () => { const conn = testDB.getConnection(); + const { parent, child } = await setupParentChild(conn); - const parent = new Unit(undefined, 'Parent2', 'Parent2', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'test-suffix-2', Unit.displayableType.ALL, true, ''); - await parent.insert(conn); - - const child = new Unit(undefined, 'Child of test-suffix-2', 'Child2', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await child.insert(conn); - - const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); - await conversion.insert(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, @@ -58,8 +86,7 @@ mocha.describe('Suffix unit cascade deletion', () => { await removeAdditionalConversionsAndUnits(parent, conn); - const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); - expect(childRow).to.equal(null); + await expectUnitDeleted(child.id, conn); const meterAfter = await Meter.getByID(meter.id, conn); // unitID defaultGraphicUnit returns -99 when registered as NULL @@ -69,63 +96,44 @@ mocha.describe('Suffix unit cascade deletion', () => { mocha.it('cleans up other conversions referencing the auto-created unit', async () => { const conn = testDB.getConnection(); - - const parent = new Unit(undefined, 'Parent3', 'Parent3', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'test-suffix-3', Unit.displayableType.ALL, true, ''); - await parent.insert(conn); - - const child = new Unit(undefined, 'Child of test-suffix-3', 'Child3', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await child.insert(conn); - - const otherUnit = new Unit(undefined, 'Unrelated Unit', 'Unrelated', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.UNIT, '', Unit.displayableType.ALL, true, ''); - await otherUnit.insert(conn); - - const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); - await conversion.insert(conn); - - // A second, unrelated conversion also pointing at the same child unit - const secondConversion = new Conversion(otherUnit.id, child.id, false, 2, 0, 'second conversion'); - await secondConversion.insert(conn); + 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); - const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); - expect(childRow).to.equal(null); + await expectUnitDeleted(child.id, conn); const firstConversionAfter = await Conversion.getBySourceDestination(parent.id, child.id, conn); expect(firstConversionAfter).to.equal(null); - - // Check that second conversion was also deleted + 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); - const parent = new Unit(undefined, 'Parent4', 'Parent4', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'test-suffix-4', Unit.displayableType.ALL, true, ''); - await parent.insert(conn); - - const child = new Unit(undefined, 'Child of test-suffix-4', 'Child4', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await child.insert(conn); - - const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); - await conversion.insert(conn); - - const group = new Group(undefined, 'Test Group', true, gps, 'notes', 33.5, child.id); - await group.insert(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); - const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); - expect(childRow).to.equal(null); + await expectUnitDeleted(child.id, conn); - // unitID defaultGraphicUnit returns -99 when registered as NULL - // Expect the unitID to be -99 instead of null for meters and groups const groupAfter = await Group.getByID(group.id, conn); expect(groupAfter.defaultGraphicUnit).to.equal(-99); }); @@ -133,51 +141,48 @@ mocha.describe('Suffix unit cascade deletion', () => { mocha.it('recursively cleans up nested suffix chains (A -> B -> C)', async () => { const conn = testDB.getConnection(); - const unitA = new Unit(undefined, 'A', 'A', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'chain-suffix', Unit.displayableType.ALL, true, ''); - await unitA.insert(conn); - - const unitB = new Unit(undefined, 'B (created by OED)', 'B', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await unitB.insert(conn); - - const unitC = new Unit(undefined, 'C (created by OED)', 'C', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await unitC.insert(conn); - - const convAB = new Conversion(unitA.id, unitB.id, false, 1, 0, 'A to B'); - await convAB.insert(conn); - const convBC = new Conversion(unitB.id, unitC.id, false, 1, 0, 'B to C'); - await convBC.insert(conn); + 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); - const bRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [unitB.id]); - const cRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [unitC.id]); - expect(bRow).to.equal(null); - expect(cRow).to.equal(null); + 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 = new Unit(undefined, 'Parent5', 'Parent5', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'test-suffix-5', Unit.displayableType.ALL, true, ''); - await parent.insert(conn); - - const child = new Unit(undefined, 'Child of test-suffix-5', 'Child5', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await child.insert(conn); - - const parentToChild = new Conversion(parent.id, child.id, false, 1, 0, 'parent to child'); - await parentToChild.insert(conn); + 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 parentRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [parent.id]); - expect(parentRow).to.not.equal(null); + 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); @@ -186,18 +191,25 @@ mocha.describe('Suffix unit cascade deletion', () => { mocha.it('deletes the reverse conversion when bidirectional', async () => { const conn = testDB.getConnection(); - const parent = new Unit(undefined, 'Parent6', 'Parent6', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'test-suffix-6', Unit.displayableType.ALL, true, ''); - await parent.insert(conn); - - const child = new Unit(undefined, 'Child of test-suffix-6', 'Child6', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await child.insert(conn); - - const forward = new Conversion(parent.id, child.id, true, 1, 0, 'forward'); - await forward.insert(conn); - const reverse = new Conversion(child.id, parent.id, true, 1, 0, 'reverse'); - await reverse.insert(conn); + 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); @@ -209,17 +221,7 @@ mocha.describe('Suffix unit cascade deletion', () => { mocha.it('clears stale cik rows before deleting the unit', async () => { const conn = testDB.getConnection(); - - const parent = new Unit(undefined, 'Parent7', 'Parent7', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'test-suffix-7', Unit.displayableType.ALL, true, ''); - await parent.insert(conn); - - const child = new Unit(undefined, 'Child of test-suffix-7', 'Child7', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, '', Unit.displayableType.ALL, true, 'suffix unit created by OED'); - await child.insert(conn); - - const conversion = new Conversion(parent.id, child.id, false, 1, 0, 'test conversion'); - await conversion.insert(conn); + const { parent, child } = await setupParentChild(conn); // Simulate a stale cik row referencing the child unit, as would exist // before Cik regeneration @@ -231,34 +233,38 @@ mocha.describe('Suffix unit cascade deletion', () => { // Should not throw. await removeAdditionalConversionsAndUnits(parent, conn); - const childRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [child.id]); - expect(childRow).to.equal(null); + 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(); - const parent = new Unit(undefined, 'Parent8', 'Parent8', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.SUFFIX, 'test-suffix-8', Unit.displayableType.ALL, true, ''); - await parent.insert(conn); - - const regularUnit = new Unit(undefined, 'Regular Unit', 'Regular', Unit.unitRepresentType.QUANTITY, - 1000, Unit.unitType.UNIT, '', Unit.displayableType.ALL, true, ''); - await regularUnit.insert(conn); - - const conversion = new Conversion(parent.id, regularUnit.id, false, 1, 0, 'to regular unit'); - await conversion.insert(conn); + 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); - // The regular unit and its conversion should be untouched -- this - // function only cleans up OED-created (typeOfUnit === SUFFIX) units. - const regularRow = await conn.oneOrNone('SELECT * FROM units WHERE id = $1', [regularUnit.id]); - expect(regularRow).to.not.equal(null); + 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 From 67061e98f090f444a6b80b80bd4d824f7f3e3276 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Sun, 4 Oct 2026 19:28:12 -0700 Subject: [PATCH 18/21] Refactor: Naming conventions and formatting - Resolved npm run check errors - Removed export from SimulateDeletePotentiallyOrphanedUnit - Corrected spelling errors and formatted descriptions --- .../conversion/EditConversionModalComponent.tsx | 12 ++++++------ src/client/app/types/redux/conversions.ts | 2 +- src/server/services/conversionSimulation.js | 6 +++--- src/server/services/graph/checkUnitDependencies.js | 14 +++++++------- src/server/services/graph/handleSuffixUnits.js | 12 +++++++----- src/server/test/db/suffixUnitDeletionTests.js | 2 +- 6 files changed, 25 insertions(+), 23 deletions(-) diff --git a/src/client/app/components/conversion/EditConversionModalComponent.tsx b/src/client/app/components/conversion/EditConversionModalComponent.tsx index c5365607b3..09ba70d8b8 100644 --- a/src/client/app/components/conversion/EditConversionModalComponent.tsx +++ b/src/client/app/components/conversion/EditConversionModalComponent.tsx @@ -264,7 +264,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC return unitDataById[otherId]; }); // Populate computed units - suffixTypeUnitsToDeleteIds = new Set(suffixTypeUnitsToDelete.map(u => u.id)) + 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 @@ -361,7 +361,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC
    {translate('conversion.delete.unit.orphan')}:
      - {result.potentiallyOrphanedUnits.map((u) => ( + {result.potentiallyOrphanedUnits.map(u => (
    • "{u.name}"
      • ))}
    @@ -396,7 +396,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC }); if (meterLossMap.size > 0) { msgElements.push( -
    +
    {translate('conversion.delete.meter.affected')}:
    @@ -411,7 +411,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC
    {translate('conversion.delete.lost.units')}:
      - {lostUnits.map((id: number, i: number) => ( + {lostUnits.map((id: number) => (
    • "{unitDataById[id]?.name || id}" {suffixTypeUnitsToDeleteIds.has(id) ? '(created by OED)' : ''} @@ -438,7 +438,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC }); if (groupLossMap.size > 0) { msgElements.push( -
      +
      {translate('conversion.delete.group.affected')}:
      @@ -453,7 +453,7 @@ export default function EditConversionModalComponent(props: EditConversionModalC
      {translate('conversion.delete.lost.units')}:
        - {lostUnits.map((id: number, i: number) => ( + {lostUnits.map((id: number) => (
      • "{unitDataById[id]?.name || id}" {suffixTypeUnitsToDeleteIds.has(id) ? ' (created by OED)' : ''} diff --git a/src/client/app/types/redux/conversions.ts b/src/client/app/types/redux/conversions.ts index 879f7ff8f8..43c15ce495 100644 --- a/src/client/app/types/redux/conversions.ts +++ b/src/client/app/types/redux/conversions.ts @@ -39,7 +39,7 @@ export interface SimulateDeleteAffectedGroup { orphaned: boolean; } -export interface SimulateDeletePotentiallyOrphanedUnit { +interface SimulateDeletePotentiallyOrphanedUnit { id: number; name: string; } \ No newline at end of file diff --git a/src/server/services/conversionSimulation.js b/src/server/services/conversionSimulation.js index ef9bc3a6d4..fa78579bf4 100644 --- a/src/server/services/conversionSimulation.js +++ b/src/server/services/conversionSimulation.js @@ -104,11 +104,11 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { 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 => + 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 + // `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; @@ -116,7 +116,7 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { // 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 visable, and have zero conversions post delete. + // 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) && diff --git a/src/server/services/graph/checkUnitDependencies.js b/src/server/services/graph/checkUnitDependencies.js index b7208f9137..be4be90b2f 100644 --- a/src/server/services/graph/checkUnitDependencies.js +++ b/src/server/services/graph/checkUnitDependencies.js @@ -42,13 +42,13 @@ async function checkUnitDependencies(unitId, conn) { * @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 + * - 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); diff --git a/src/server/services/graph/handleSuffixUnits.js b/src/server/services/graph/handleSuffixUnits.js index 7ab72fe013..8bb594d84d 100644 --- a/src/server/services/graph/handleSuffixUnits.js +++ b/src/server/services/graph/handleSuffixUnits.js @@ -176,8 +176,9 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) 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}`); } - // Track units being deleted to notify admin of the internal change + // 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); @@ -256,19 +257,20 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) // This handles nested suffix chains (A -> B -> C) await removeAdditionalConversionsAndUnits(otherUnit, conn, depth + 1); - // Delete the auto-created unit (dependency checks + cik cleanup handled inside) + // 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 ${conversion.sourceId}->${conversion.destinationId} during suffix unit cleanup: ${err}`, 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 funtion only cleans up derived suffix conversions, never the original conversion the suffix unit was created from + // 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 - // dericed units from it during the next graph rebuild. + // 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/test/db/suffixUnitDeletionTests.js b/src/server/test/db/suffixUnitDeletionTests.js index af349a90d9..29968a3ab8 100644 --- a/src/server/test/db/suffixUnitDeletionTests.js +++ b/src/server/test/db/suffixUnitDeletionTests.js @@ -22,7 +22,7 @@ async function setupParentChild(conn) { await insertUnits([ { name: 'Parent', identifier: 'Parent', unitRepresent: Unit.unitRepresentType.QUANTITY, - secInRate: 1000, typeOfUnit: Unit.unitType.SUFFIX, suffix: 'test-suffix', + secInRate: 1000, typeOfUnit: Unit.unitType.UNIT, suffix: 'test-suffix', displayable: Unit.displayableType.ALL, preferredDisplay: true, note: '' }, { From f25a64eab9f345260df1cd473060749e40800d9e Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Sun, 4 Oct 2026 19:34:07 -0700 Subject: [PATCH 19/21] Refactor: SQL naming conventions - 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 --- src/server/models/Conversion.js | 4 ++-- src/server/models/Group.js | 2 +- src/server/models/Meter.js | 4 ++-- src/server/models/Unit.js | 14 ++++++-------- src/server/services/graph/checkUnitDependencies.js | 2 +- src/server/services/graph/handleSuffixUnits.js | 2 +- ..._unit_id.sql => get_conversions_by_unit_id.sql} | 0 ...it.sql => clear_group_default_graphic_unit.sql} | 0 ...it.sql => clear_meter_default_graphic_unit.sql} | 0 .../{clear_unit_id.sql => clear_meter_unit_id.sql} | 0 src/server/sql/unit/find_orphaned_units.sql | 4 ++-- .../unit/{lock_by_id.sql => lock_unit_by_id.sql} | 0 12 files changed, 15 insertions(+), 17 deletions(-) rename src/server/sql/conversion/{get_id_and_direction_by_unit_id.sql => get_conversions_by_unit_id.sql} (100%) rename src/server/sql/group/{clear_default_graphic_unit.sql => clear_group_default_graphic_unit.sql} (100%) rename src/server/sql/meter/{clear_default_graphic_unit.sql => clear_meter_default_graphic_unit.sql} (100%) rename src/server/sql/meter/{clear_unit_id.sql => clear_meter_unit_id.sql} (100%) rename src/server/sql/unit/{lock_by_id.sql => lock_unit_by_id.sql} (100%) diff --git a/src/server/models/Conversion.js b/src/server/models/Conversion.js index 14799a44a0..5deba61420 100644 --- a/src/server/models/Conversion.js +++ b/src/server/models/Conversion.js @@ -73,8 +73,8 @@ class Conversion { * @param {*} conn The connection to use. * @returns {Promise.} */ - static async getConversionsByUnitID(unitId, conn) { - return await conn.any(sqlFile('conversion/get_id_and_direction_by_unit_id.sql'), { unitId }); + static async getByUnitID(unitId, conn) { + return await conn.any(sqlFile('conversion/get_conversions_by_unit_id.sql'), { unitId }); } /** diff --git a/src/server/models/Group.js b/src/server/models/Group.js index 1fe27cc621..39c87da2e1 100644 --- a/src/server/models/Group.js +++ b/src/server/models/Group.js @@ -259,7 +259,7 @@ class Group { * @param {*} conn The connection to use. */ static async clearDefaultGraphicUnit(id, conn) { - await conn.none(sqlFile('group/clear_default_graphic_unit.sql'), { id: id }); + await conn.none(sqlFile('group/clear_group_default_graphic_unit.sql'), { id: id }); } /** diff --git a/src/server/models/Meter.js b/src/server/models/Meter.js index 1db208d238..293db0582b 100644 --- a/src/server/models/Meter.js +++ b/src/server/models/Meter.js @@ -320,7 +320,7 @@ class Meter { * @param {*} conn The connection to use. */ static async clearDefaultGraphicUnit(id, conn) { - await conn.none(sqlFile('meter/clear_default_graphic_unit.sql'), { id: id }); + await conn.none(sqlFile('meter/clear_meter_default_graphic_unit.sql'), { id: id }); } /** @@ -329,7 +329,7 @@ class Meter { * @param {*} conn The connection to use. */ static async clearUnitId(id, conn) { - await conn.none(sqlFile('meter/clear_unit_id.sql'), { id: id }); + await conn.none(sqlFile('meter/clear_meter_unit_id.sql'), { id: id }); } /** diff --git a/src/server/models/Unit.js b/src/server/models/Unit.js index e65899384a..e51151474b 100644 --- a/src/server/models/Unit.js +++ b/src/server/models/Unit.js @@ -205,21 +205,19 @@ class Unit { * @returns {Promise.} */ static async lockById(id, conn) { - const row = await conn.one(sqlFile('unit/lock_by_id.sql'), { id: id }); + const row = await conn.one(sqlFile('unit/lock_unit_by_id.sql'), { id: id }); return Unit.mapRow(row); } /** - * Finds suffix-type units that may have been orphaned (left visible with - * no remaining conversions) after cleaning up around the given source/destination - * units. Used as a post-delete sanity check, not a guarantee. - * @param {number} sourceId The conversion's source unit id. - * @param {number} destinationId The conversion's destination unit id. + * 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) { - return await conn.any(sqlFile('unit/find_orphaned_units.sql')); + 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 diff --git a/src/server/services/graph/checkUnitDependencies.js b/src/server/services/graph/checkUnitDependencies.js index be4be90b2f..03a82394b2 100644 --- a/src/server/services/graph/checkUnitDependencies.js +++ b/src/server/services/graph/checkUnitDependencies.js @@ -26,7 +26,7 @@ async function checkUnitDependencies(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.getConversionsByUnitID(unitId,conn) + Conversion.getByUnitID(unitId,conn) ]); return { diff --git a/src/server/services/graph/handleSuffixUnits.js b/src/server/services/graph/handleSuffixUnits.js index 8bb594d84d..3c000f0d79 100644 --- a/src/server/services/graph/handleSuffixUnits.js +++ b/src/server/services/graph/handleSuffixUnits.js @@ -228,7 +228,7 @@ async function removeAdditionalConversionsAndUnits(suffixUnit, conn, depth = 0) // Check if otherUnitId has connections besides this one. // Deletion of other connections may lead to orphaning a unit. - const otherUnitConversions = await Conversion.getConversionsByUnitID(otherUnitId, conn); + 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)) diff --git a/src/server/sql/conversion/get_id_and_direction_by_unit_id.sql b/src/server/sql/conversion/get_conversions_by_unit_id.sql similarity index 100% rename from src/server/sql/conversion/get_id_and_direction_by_unit_id.sql rename to src/server/sql/conversion/get_conversions_by_unit_id.sql diff --git a/src/server/sql/group/clear_default_graphic_unit.sql b/src/server/sql/group/clear_group_default_graphic_unit.sql similarity index 100% rename from src/server/sql/group/clear_default_graphic_unit.sql rename to src/server/sql/group/clear_group_default_graphic_unit.sql diff --git a/src/server/sql/meter/clear_default_graphic_unit.sql b/src/server/sql/meter/clear_meter_default_graphic_unit.sql similarity index 100% rename from src/server/sql/meter/clear_default_graphic_unit.sql rename to src/server/sql/meter/clear_meter_default_graphic_unit.sql diff --git a/src/server/sql/meter/clear_unit_id.sql b/src/server/sql/meter/clear_meter_unit_id.sql similarity index 100% rename from src/server/sql/meter/clear_unit_id.sql rename to src/server/sql/meter/clear_meter_unit_id.sql diff --git a/src/server/sql/unit/find_orphaned_units.sql b/src/server/sql/unit/find_orphaned_units.sql index e26fc04bbe..27a5409581 100644 --- a/src/server/sql/unit/find_orphaned_units.sql +++ b/src/server/sql/unit/find_orphaned_units.sql @@ -1,4 +1,4 @@ -/* This Source Code Form is subject to the terms of the Mozilla Punblic +/* 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/. */ @@ -9,4 +9,4 @@ AND NOT EXISTS ( SELECT 1 FROM conversions conv WHERE (conv.source_id = unit.id OR conv.destination_id = unit.id) ) -LIMIT 100; \ No newline at end of file +LIMIT ${limit}; \ No newline at end of file diff --git a/src/server/sql/unit/lock_by_id.sql b/src/server/sql/unit/lock_unit_by_id.sql similarity index 100% rename from src/server/sql/unit/lock_by_id.sql rename to src/server/sql/unit/lock_unit_by_id.sql From 542302d282f5eca4cf00f84307c503a30a5ebec2 Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Sun, 4 Oct 2026 19:38:00 -0700 Subject: [PATCH 20/21] Refactor: redoCik usage - 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 --- src/server/models/Cik.js | 12 ++++-------- src/server/routes/conversions.js | 5 +++++ src/server/services/graph/checkUnitDependencies.js | 9 +++++++-- src/server/sql/cik/delete_by_unit_id.sql | 5 ----- 4 files changed, 16 insertions(+), 15 deletions(-) delete mode 100644 src/server/sql/cik/delete_by_unit_id.sql diff --git a/src/server/models/Cik.js b/src/server/models/Cik.js index a3ab97ed73..8d8efef68c 100644 --- a/src/server/models/Cik.js +++ b/src/server/models/Cik.js @@ -53,16 +53,12 @@ class Cik { } /** - * Deletes cik rows directly referencing the given unit. This is a - * stopgap to satisfy the foreign key on cik before deleting a unit's - * row. It does not catch rows for unrelated unit pairs whose path - * happened to route through this unit. redoCik should be called - * afterward for a fully correct recalculation. - * @param {number} unitId The unit's id. + * 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 deleteByUnitId(unitId, conn) { - await conn.none(sqlFile('cik/delete_by_unit_id.sql'), { unitId }); + static async deleteAll(conn) { + await conn.none(sqlFile('cik/delete_all_conversions.sql')); } /** diff --git a/src/server/routes/conversions.js b/src/server/routes/conversions.js index 3e66a3000f..32fd895864 100644 --- a/src/server/routes/conversions.js +++ b/src/server/routes/conversions.js @@ -297,6 +297,11 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re 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}`, err); diff --git a/src/server/services/graph/checkUnitDependencies.js b/src/server/services/graph/checkUnitDependencies.js index 03a82394b2..bc6a701db8 100644 --- a/src/server/services/graph/checkUnitDependencies.js +++ b/src/server/services/graph/checkUnitDependencies.js @@ -74,11 +74,17 @@ async function getUnitDependencyDetails(unitId, conn) { * 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. Also clears any stale cik rows + * 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. @@ -109,7 +115,6 @@ async function deleteUnitSafely(unitId, conn) { await Conversion.delete(conv.source_id, conv.destination_id, conn); } - await Cik.deleteByUnitId(unitId, conn); await Unit.delete(unitId, conn); } diff --git a/src/server/sql/cik/delete_by_unit_id.sql b/src/server/sql/cik/delete_by_unit_id.sql deleted file mode 100644 index c256c0da20..0000000000 --- a/src/server/sql/cik/delete_by_unit_id.sql +++ /dev/null @@ -1,5 +0,0 @@ -/* 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/. */ - -DELETE FROM cik WHERE source_id = ${unitId} OR destination_id = ${unitId}; \ No newline at end of file From 24c0592b0212bdf99aeac1b6dd1d941d8085056f Mon Sep 17 00:00:00 2001 From: Bea Sauve Date: Sun, 4 Oct 2026 19:57:30 -0700 Subject: [PATCH 21/21] Refactor: Orphan Check 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. --- src/server/services/conversionSimulation.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/server/services/conversionSimulation.js b/src/server/services/conversionSimulation.js index fa78579bf4..10a52e0249 100644 --- a/src/server/services/conversionSimulation.js +++ b/src/server/services/conversionSimulation.js @@ -120,6 +120,8 @@ async function simulateDeleteConversion({ sourceId, destinationId }, conn) { 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 }));