From de3c5aa56207252f10f73a18350db6137a358472 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Mon, 24 Aug 2026 08:42:31 -0400 Subject: [PATCH] Stop re-creating a controlling-PC link that was already there Reported: moving machine 3016 on the map and saving answered "this relationship already exists" and refused the save. The map had nothing to do with it. Every save of the machine form re-POSTed the controlling-PC link, and the server was right to refuse it - the row was already active, and assetrelationships is unique on (source, target, type). Any edit to a machine that has a controlling PC hit this; a position change is just the one somebody happened to be making. The form asked the wrong question. It used "do I know the existing link's id" as a stand-in for "is there an existing link", and read that id from rel.assetrelationshipid - a key the API has never emitted. to_dict serialises column names and the column is relationshipid, so the read was undefined on every load while controllingPcId was populated from the same relationship. The two answers disagreed, and the disagreement was invisible: undefined is not an error, it is just a falsy id. That single conflation caused both failures. Saving anything re-created the link, and CLEARING the controlling PC did nothing at all, because "no selection and no known id" reads as "nothing to do" - so removing a machine's controlling PC silently kept it. Only the first of those was reported, and the second is the one somebody would have trusted. Fixed by making the LOADED PC the authority on whether a link exists, with the relationship id used only to delete a row. If that id ever goes missing again the worst case is a link that cannot be removed, never a duplicate create. The decision moved to controllingPc.js as a pure function so it could be pinned by tests, following labelPages.js. Unchanged is now explicitly no-op rather than delete-then-create: re-linking a machine to the same PC opens a window where it has no controlling PC at all, and resolve_asset_position walks that edge, so a failure between the two calls would drop the machine's inherited position along with its link. The specs also cover a select handing back '42' against an API that gave 42, which would make an untouched link look like a swap and reach the same duplicate POST by another route. Not the printer assignment, which was the first guess: PrinterAssignmentPicker makes no relationship calls, and its save is wrapped in a catch that only logs, so it cannot surface an error from this form at all. --- CHANGELOG.md | 10 +++ .../machines/frontend/views/MachineForm.vue | 60 +++++++++----- .../machines/frontend/views/controllingPc.js | 68 ++++++++++++++++ .../frontend/views/controllingPc.spec.js | 81 +++++++++++++++++++ 4 files changed, 200 insertions(+), 19 deletions(-) create mode 100644 plugins/machines/frontend/views/controllingPc.js create mode 100644 plugins/machines/frontend/views/controllingPc.spec.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a39dbe87..bbccfd1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,16 @@ ADR-007 and ADR-002. the wrong code. The page index now travels with the page. - The back label's dotted preview outline outspecified the print reset and drew a dashed rule down the edge of every second card. +- **Saving a machine that has a controlling PC answered "this relationship + already exists" and refused the save.** Reported against a map position + change, but the map was incidental: the form re-POSTed the controlling-PC + link on every save, and the row was already there. It used "do I know the + existing link's id" as a stand-in for "is there an existing link", and read + that id from a key the API does not emit, so the two disagreed on any machine + that had a PC. The same conflation meant CLEARING a machine's controlling PC + silently did nothing, which was the more dangerous half and had not been + reported. The loaded PC is now what decides, and re-selecting the same PC is + a no-op rather than a delete-and-recreate. ### Changed diff --git a/plugins/machines/frontend/views/MachineForm.vue b/plugins/machines/frontend/views/MachineForm.vue index efe0846c..cf8c28f2 100644 --- a/plugins/machines/frontend/views/MachineForm.vue +++ b/plugins/machines/frontend/views/MachineForm.vue @@ -379,6 +379,7 @@ import PrinterAssignmentPicker from '@/components/PrinterAssignmentPicker.vue' import { currentTheme } from '@/stores/theme' import { useIdentifierFlags } from '@/composables/identifierSettings' import { apiError } from '@/utils/apiError' +import { planControllingPcChange } from './controllingPc' const { isEnabled } = useIdentifierFlags() @@ -440,6 +441,9 @@ const relationshipTypes = ref([]) const controllingPcId = ref(null) const relationshipTypeId = ref(null) const existingRelationshipId = ref(null) +// Which PC the link named when the form loaded, so an unchanged one can be left +// alone. Editing a machine is mostly editing something else about it. +const loadedControllingPcId = ref(null) const currentMachine = ref(null) // Filter models by selected machine vendor @@ -575,7 +579,16 @@ onMounted(async () => { if (rel.sourceasset?.assettypename === 'computer' && (rel.relationshiptypename || '').toLowerCase() === 'controls') { controllingPcId.value = rel.sourceasset.assetid relationshipTypeId.value = rel.relationshiptypeid - existingRelationshipId.value = rel.assetrelationshipid + // relationshipid. `assetrelationshipid` is not a key the API has + // ever emitted - to_dict serialises the column names and the + // column is relationshipid - so this read undefined every time, + // and saveRelationship then took the "there is nothing to + // replace" path on a machine that had a controlling PC. Saving + // ANY change, a map position included, re-POSTed the link that + // was already there and the server correctly answered 409 "this + // relationship already exists". + existingRelationshipId.value = rel.relationshipid + loadedControllingPcId.value = rel.sourceasset.assetid break } } @@ -693,31 +706,40 @@ async function saveMachine() { } async function saveRelationship(assetId) { - // If no PC selected and no existing relationship, nothing to do - if (!controllingPcId.value && !existingRelationshipId.value) { + // The decision lives in controllingPc.js, where it is covered by tests. + // See that file for why "is there a link" and "do I know its id" are two + // different questions and what happened when they were treated as one. + const plan = planControllingPcChange({ + selected: controllingPcId.value, + loaded: loadedControllingPcId.value, + existingRelationshipId: existingRelationshipId.value, + }) + + if (plan.action === 'none') { return } - // If clearing the relationship - if (!controllingPcId.value && existingRelationshipId.value) { - await assetsApi.deleteRelationship(existingRelationshipId.value) + if (plan.deleteId !== null) { + await assetsApi.deleteRelationship(plan.deleteId) + existingRelationshipId.value = null + loadedControllingPcId.value = null + } + + if (plan.action === 'unlink') { return } - // If PC is selected - if (controllingPcId.value) { - // If there's an existing relationship, delete it first - if (existingRelationshipId.value) { - await assetsApi.deleteRelationship(existingRelationshipId.value) - } + // PC controls Machine, so the PC is the source and the machine the target. + const created = await assetsApi.createRelationship({ + sourceassetid: controllingPcId.value, + targetassetid: assetId, + relationshiptypeid: relationshipTypeId.value + }) - // Create new relationship (PC controls Machine, so PC is source, Machine is target) - await assetsApi.createRelationship({ - sourceassetid: controllingPcId.value, - targetassetid: assetId, - relationshiptypeid: relationshipTypeId.value - }) - } + // Keep the form honest about what is now stored, so a second save without a + // reload does not repeat the create it just did. + existingRelationshipId.value = created?.data?.data?.relationshipid ?? null + loadedControllingPcId.value = controllingPcId.value } diff --git a/plugins/machines/frontend/views/controllingPc.js b/plugins/machines/frontend/views/controllingPc.js new file mode 100644 index 00000000..1352a4e8 --- /dev/null +++ b/plugins/machines/frontend/views/controllingPc.js @@ -0,0 +1,68 @@ +// What saving the machine form should do about its controlling-PC link. +// +// Pulled out of MachineForm.vue because the component decided this inline and +// got it wrong in a way nothing could catch: it treated "I know the id of the +// existing link" as "there IS an existing link". The id was read from +// `rel.assetrelationshipid`, a key the API has never emitted - the column is +// `relationshipid` and to_dict serialises column names - so it was undefined on +// every load, and the two questions gave opposite answers on a machine that +// plainly had a controlling PC. +// +// Both failures followed from that single conflation: +// - Saving ANY change, a map position included, re-POSTed the link that was +// already there. The server answered 409 "this relationship already +// exists", correctly, and the form surfaced it as a failed save. +// - Clearing the controlling PC did nothing at all, because "no selection and +// no known id" reads as "nothing to do". +// +// So THE LOADED PC IS THE AUTHORITY on whether a link exists, and the +// relationship id is only ever used to delete a row. If the id goes missing +// again the worst case is a link that cannot be removed, never a duplicate +// create. + +/** + * Decide what to do with the controlling-PC link on save. + * + * @param {object} state + * @param {number|string|null} state.selected - PC id chosen in the form now. + * @param {number|string|null} state.loaded - PC id the link named on load. + * @param {number|null} state.existingRelationshipId - row id to delete, if known. + * @returns {{action: 'none'|'unlink'|'link'|'relink', deleteId: number|null}} + */ +export function planControllingPcChange({ selected, loaded, existingRelationshipId } = {}) { + const now = normalize(selected) + const before = normalize(loaded) + const deleteId = existingRelationshipId ?? null + + // Unchanged is not a change. Somebody moving a marker on the map is not + // asking to be re-linked to the same PC, and delete-then-create on an + // untouched link opens a window where the machine has no controlling PC - + // which resolve_asset_position walks across, so a failure between the two + // calls would lose the machine's inherited position along with the link. + if (now === before) { + return { action: 'none', deleteId: null } + } + + if (now === null) { + // Cleared. Nothing to delete means nothing to do, rather than an error: + // the link is already absent, which is the state being asked for. + return deleteId === null + ? { action: 'none', deleteId: null } + : { action: 'unlink', deleteId } + } + + if (before === null) { + return { action: 'link', deleteId: null } + } + + // Swapped to a different PC. The old row goes first: 'controls' is singular + // per source, not per target, so leaving it would leave the machine with two. + return { action: 'relink', deleteId } +} + +// Ids arrive as numbers from the API and strings from a hands back '42' while the relationship carried 42. Comparing + // those raw makes an untouched link look like a swap, which is the same + // duplicate POST by another route. + expect(planControllingPcChange({ + selected: '42', loaded: 42, existingRelationshipId: 7, + })).toEqual({ action: 'none', deleteId: null }) + }) + + it('treats an empty selection as cleared, not as a PC named ""', () => { + expect(planControllingPcChange({ + selected: '', loaded: 42, existingRelationshipId: 7, + })).toEqual({ action: 'unlink', deleteId: 7 }) + }) + + it('does nothing when there was no PC and none was chosen', () => { + expect(planControllingPcChange({ + selected: null, loaded: null, existingRelationshipId: null, + })).toEqual({ action: 'none', deleteId: null }) + }) + + it('does not attempt a delete it cannot perform', () => { + // Cleared, but the row id was never learned. Asking the API to delete + // `undefined` produced a request against /relationships/undefined. + expect(planControllingPcChange({ + selected: null, loaded: 42, existingRelationshipId: undefined, + })).toEqual({ action: 'none', deleteId: null }) + }) +})