Secure Send documented, and the machine form stops re-creating a controlling-PC link that already exists

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
 }
 </script>
 

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 <select>, and '' means
+// "nothing chosen". Comparing those raw is how an unchanged PC looks changed.
+function normalize(value) {
+  if (value === null || value === undefined || value === '') return null
+  return String(value)
+}

plugins/machines/frontend/views/controllingPc.spec.js

@@ -0,0 +1,81 @@
+import { describe, it, expect } from 'vitest'
+
+import { planControllingPcChange } from './controllingPc'
+
+// Reported: moving machine 3016 on the map, then saving, answered "this
+// relationship already exists" and refused the save. The machine had a
+// controlling PC and the form re-created that link on every save, because it
+// asked "do I know the existing link's id" instead of "is there an existing
+// link" - and the id was read from a key the API does not emit, so it was
+// always undefined.
+//
+// The first two specs are that bug. The rest pin the behaviour it was hiding.
+
+describe('planControllingPcChange', () => {
+  it('does nothing when the controlling PC is unchanged', () => {
+    // The reported case: only the map position moved.
+    expect(planControllingPcChange({
+      selected: 42, loaded: 42, existingRelationshipId: 7,
+    })).toEqual({ action: 'none', deleteId: null })
+  })
+
+  it('still does nothing when the existing row id was never learned', () => {
+    // The exact shape of the bug: the link is there, its id is not known. The
+    // answer must be "leave it alone", never "create it again" - a create here
+    // is the 409.
+    expect(planControllingPcChange({
+      selected: 42, loaded: 42, existingRelationshipId: undefined,
+    })).toEqual({ action: 'none', deleteId: null })
+  })
+
+  it('removes the link when the PC is cleared', () => {
+    // This never fired before either: no selection and no known id read as
+    // "nothing to do", so clearing a controlling PC silently kept it.
+    expect(planControllingPcChange({
+      selected: null, loaded: 42, existingRelationshipId: 7,
+    })).toEqual({ action: 'unlink', deleteId: 7 })
+  })
+
+  it('creates a link when there was none', () => {
+    expect(planControllingPcChange({
+      selected: 42, loaded: null, existingRelationshipId: null,
+    })).toEqual({ action: 'link', deleteId: null })
+  })
+
+  it('replaces the old row when the PC is swapped', () => {
+    // controls is singular per SOURCE, not per target, so the old row has to go
+    // or the machine ends up with two controlling PCs.
+    expect(planControllingPcChange({
+      selected: 99, loaded: 42, existingRelationshipId: 7,
+    })).toEqual({ action: 'relink', deleteId: 7 })
+  })
+
+  it('treats a select string and an API number as the same PC', () => {
+    // A <select> 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 })
+  })
+})