Skip to content

Conversion Deletions Involving Suffix Units #1470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
esummerh wants to merge 4 commits into
base: development
Choose a base branch
from
Open

Conversion Deletions Involving Suffix Units #1470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ae0d360
Added suffix unit deletion to unit deletion route to keep consistent …
esummerh Apr 14, 2025
ee25eb4
Added deletion logic to delete related conversions and units when a c…
esummerh Apr 14, 2025
db0d3ea
Added warning message to user interface when the user tries to delete…
esummerh Apr 14, 2025
f4adba2
Added warning messages and translations.
esummerh Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/client/app/components/conversion/EditConversionModalComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,29 @@ export default function EditConversionModalComponent(props: EditConversionModalC
// TODO: Check after deleting the conversion to see if a change happens.
// Notify the admin of any consequences caused by deleting the conversion.
}
} else if (source.typeOfUnit === UnitType.suffix || dest.typeOfUnit === UnitType.suffix) {
const unit = source.typeOfUnit === UnitType.suffix ? source : dest;
// Get the units that use this suffix
const affectedUnits = Object.values(unitDataById).filter(u => u.suffix === unit.identifier);
// Get the conversions that use these units
const affectedConversions = conversionDetails.filter(c =>
affectedUnits.some(u => u.id === c.sourceId || u.id === c.destinationId)
);
// Send user a warning before deletion
if (affectedUnits.length > 0) {
msg += `${translate('conversion.delete.suffix.units.to.delete')}:\n`;
affectedUnits.forEach(u => {
msg += `- ${u.name} (${u.identifier})\n`;
});
}
if (affectedConversions.length > 0) {
msg += `${translate('conversion.delete.suffix.conversions.to.delete')}:\n`;
affectedConversions.forEach(c => {
const s = unitDataById[c.sourceId]?.identifier || c.sourceId;
const d = unitDataById[c.destinationId]?.identifier || c.destinationId;
msg += `- ${s} -> ${d}\n`;
});
}
}

if (msg === '') {
Expand Down
6 changes: 6 additions & 0 deletions src/client/app/translations/data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,9 @@ const LocaleTranslationData = {
"conversion.delete.meter.related": "The following meters use meter unit",
"conversion.delete.meter.reduce.graphable": "Deleting this meter conversion will reduce the number of graphable units for meters using meter unit",
"conversion.delete.restricted": "This conversion cannot be deleted until the significant consequences of doing this are addressed.",
"conversion.delete.suffix.conversions.to.delete": "The following conversions will also be deleted",
"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.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:",
Expand Down Expand Up @@ -644,7 +646,9 @@ const LocaleTranslationData = {
"conversion.delete.meter.related": "The following meters use meter unit\u{26A1}",
"conversion.delete.meter.reduce.graphable": "Deleting this meter conversion will reduce the number of graphable units for meters using meter unit\u{26A1}",
"conversion.delete.restricted": "This conversion cannot be deleted until the significant consequences of doing this are addressed.\u{26A1}",
"conversion.delete.suffix.conversions.to.delete": "The following units will also be deleted\u{26A1}",
Copy link
Member

@huss huss Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the text have units/conversions switched for the two fr keys?

"conversion.delete.suffix.disable": "Deleting this suffix conversion will disable use of suffix\u{26A1}",
"conversion.delete.suffix.units.to.delete": "The following conversions 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}",
Expand Down Expand Up @@ -1211,7 +1215,9 @@ const LocaleTranslationData = {
"conversion.delete.meter.related": "The following meters use meter unit\u{26A1}",
"conversion.delete.meter.reduce.graphable": "Deleting this meter conversion will reduce the number of graphable units for meters using meter unit\u{26A1}",
"conversion.delete.restricted": "This conversion cannot be deleted until the significant consequences of doing this are addressed.\u{26A1}",
"conversion.delete.suffix.conversions.to.delete": "The following 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.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",
Expand Down
14 changes: 14 additions & 0 deletions src/server/routes/conversions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const { getConnection } = require('../db');
const Conversion = require('../models/Conversion');
const { success, failure } = require('./response');
const validate = require('jsonschema').validate;
const Unit = require('../models/Unit');
const { removeAdditionalConversionsAndUnits } = require('../services/graph/handleSuffixUnits');

const router = express.Router();

Expand Down Expand Up @@ -175,6 +177,18 @@ router.post('/delete', async (req, res) => {
} else {
const conn = getConnection();
try {
// Get the source and destination units for the conversion
const source = await Unit.getById(req.body.sourceId, conn);
const dest = await Unit.getById(req.body.destinationId, conn);
// Check if the source or the destination is a suffix unit
if (source.typeOfUnit === 'suffix') {
log.info('Suffix unit is used in conversion deletion as source.');
Copy link
Member

@huss huss Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both these messages would be clearer in the logs if the actual conversion information was given. For example, stating the source and destination of the conversion, maybe with the the arrow that is common in other displays of a conversion.

await removeAdditionalConversionsAndUnits(source, conn);
}
if (dest.typeOfUnit === 'suffix') {
log.info('Suffix unit is used in conversion deletion as destination.');
await removeAdditionalConversionsAndUnits(dest, conn);
}
// Don't worry about checking if the conversion 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 Conversion.delete(req.body.sourceId, req.body.destinationId, conn);
Expand Down
5 changes: 5 additions & 0 deletions src/server/routes/units.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,11 @@ router.post('/delete', async (req, res) => {
} else {
const conn = getConnection();
try {
const unit = await Unit.getById(req.body.id, conn);
if (unit.typeOfUnit === 'suffix') {
log.info('Deleting a suffix unit. Now deleting associated units and conversions.');
Copy link
Member

@huss huss Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea so put in the name of the unit that is being deleted.

await removeAdditionalConversionsAndUnits(unit, conn);
Copy link
Member

@huss huss Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot easily put a comment in removeAdditionalConversionsAndUnit so I'm putting the comment here. I think it would be a good idea to log each items removed in that function. Thus, the ones in the route are in overview log msg and the ones in the function log as each is removed.

}
// 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(req.body.id, conn);
Expand Down
Loading