From 45acaf0b8fd05be6d9763a6108c672fe8e0d5503 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Thu, 3 Sep 2026 14:42:24 -0400 Subject: [PATCH] Resolve an asset type by name, and send a map position with its level Wave 5, the asset forms. Both defects are the same mistake in different clothes: a form deciding something it should have been told. Four forms carried a hardcoded asset-type id - const COMPUTER_ASSETTYPEID = 2, and the same for machines, printers and network devices - to tell CustomFieldsInputs which type's fields to render. Those ids come from seed order. On the instance they were written against, computer was 2; on a freshly seeded database computer is 1 and 2 is network_device. So the PC form asked for another type's custom fields, rendered them, and dropped everything typed into them behind a success toast, because the values were keyed to a type the record does not have. Verified in a browser against a correctly built dist: the form now requests assettypeid=1 on an instance where computer is 1. The name is the stable thing - assettype is a fixed vocabulary and the API already publishes the mapping - so composables/assetTypeIds.js resolves it once per page load. A failed load leaves it undefined, which renders no custom-field panel: a missing panel is honest, a guessed id is what this replaces. Three of the five forms that emit a map position sent mapx and mapy without levelid. ADR-017 is explicit that a position without its level cannot be drawn on the right floor plan, so a picked pin saved a null level and rendered nowhere - while the form displayed "Position: x, y" back, because it kept the value it had never sent. The ADR-017 gate should have caught that and did not: it globbed '*.py' only, so it policed what the server emits and never what the browser sends. It now covers .vue and .js too, and accepts both quoting styles. Verified by planting: drop levelid from PCForm again and the gate names that exact line. Five composable tests, including a fixture with a DIFFERENT seed order - the case the constants got wrong. Still open from this area, and deliberately not in this commit: the PC form writes an INHERITED location back as the record's own. Fixing it needs the API to distinguish an owned locationid from a derived one, which is a contract change rather than a form fix. Scope: product --- frontend/src/composables/assetTypeIds.js | 56 +++++++++++ frontend/src/composables/assetTypeIds.spec.js | 94 +++++++++++++++++++ plugins/computers/frontend/views/PCForm.vue | 18 +++- .../machines/frontend/views/MachineForm.vue | 17 +++- .../frontend/views/NetworkDeviceForm.vue | 9 +- .../printers/frontend/views/PrinterForm.vue | 17 +++- scripts/check-naming-and-style.sh | 10 +- 7 files changed, 208 insertions(+), 13 deletions(-) create mode 100644 frontend/src/composables/assetTypeIds.js create mode 100644 frontend/src/composables/assetTypeIds.spec.js diff --git a/frontend/src/composables/assetTypeIds.js b/frontend/src/composables/assetTypeIds.js new file mode 100644 index 00000000..eb013aef --- /dev/null +++ b/frontend/src/composables/assetTypeIds.js @@ -0,0 +1,56 @@ +// Asset-type ids resolved by NAME, once per page load. +// +// Four asset forms carried a hardcoded id - `const COMPUTER_ASSETTYPEID = 2`, +// and the same for machines, printers and network devices - to tell +// CustomFieldsInputs which type's fields to render. The ids come from seed +// order, not from anything stable: on the instance those constants were written +// against, computer was 2; on a freshly seeded one it is 1 and 2 is +// network_device. So the PC form asked for another type's custom fields, +// rendered them, and dropped whatever the operator typed - with a success +// toast, because the values were keyed to a type the record does not have. +// +// A name is the stable thing. `assettype` is a fixed vocabulary (ADR-001) and +// the API already publishes the mapping; nothing else should hardcode a number. +import { reactive } from 'vue' +import { assetsApi } from '../api' + +const state = reactive({ + byname: {}, + loaded: false +}) + +let inflight = null + +export async function loadAssetTypeIds() { + if (state.loaded) return state + if (inflight) return inflight + inflight = assetsApi.types.list() + .then((response) => { + const rows = response?.data?.data || response?.data || [] + const byname = {} + rows.forEach((row) => { + if (row && row.assettype) byname[row.assettype] = row.assettypeid + }) + state.byname = byname + state.loaded = true + return state + }) + .catch(() => { + // Leave it unloaded rather than caching a failure: a form that could not + // resolve its type renders no custom fields, which is honest. Guessing an + // id is what caused the defect this replaces. + state.loaded = false + return state + }) + .finally(() => { inflight = null }) + return inflight +} + +// Undefined until loaded, and undefined for a name the server does not know. +// CustomFieldsInputs treats an undefined assettypeid as "nothing to render", +// which is the safe reading: better a missing panel than the wrong type's. +export function assetTypeId(name) { + return state.byname[name] +} + +export { state } diff --git a/frontend/src/composables/assetTypeIds.spec.js b/frontend/src/composables/assetTypeIds.spec.js new file mode 100644 index 00000000..995d01cd --- /dev/null +++ b/frontend/src/composables/assetTypeIds.spec.js @@ -0,0 +1,94 @@ +// Asset-type ids must come from the server by name. +// +// Four forms hardcoded the id. Those constants were written against one +// instance's seed order and were wrong on another: PCForm said 2, and on a +// freshly seeded database 2 is network_device. The form asked +// CustomFieldsInputs for that type's fields, rendered them, and dropped what +// the operator typed - with a success toast, because the values were keyed to +// a type the record does not have. +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('../api', () => ({ + assetsApi: { + types: { + list: vi.fn() + } + } +})) + +import { assetsApi } from '../api' +import { loadAssetTypeIds, assetTypeId, state } from './assetTypeIds' + +function reset() { + state.byname = {} + state.loaded = false + assetsApi.types.list.mockReset() +} + +describe('assetTypeIds', () => { + beforeEach(reset) + + it('resolves an id by name from what the server actually has', async () => { + assetsApi.types.list.mockResolvedValue({ + data: { data: [ + { assettypeid: 1, assettype: 'computer' }, + { assettypeid: 2, assettype: 'network_device' }, + { assettypeid: 4, assettype: 'machine' } + ] } + }) + + await loadAssetTypeIds() + + expect(assetTypeId('computer')).toBe(1) + expect(assetTypeId('network_device')).toBe(2) + expect(assetTypeId('machine')).toBe(4) + }) + + it('survives a differently seeded site, which is the whole point', async () => { + // The same names, different ids. A hardcoded 2 would be network_device + // here and computer in the other fixture. + assetsApi.types.list.mockResolvedValue({ + data: { data: [ + { assettypeid: 2, assettype: 'computer' }, + { assettypeid: 3, assettype: 'network_device' } + ] } + }) + + await loadAssetTypeIds() + + expect(assetTypeId('computer')).toBe(2) + expect(assetTypeId('network_device')).toBe(3) + }) + + it('fetches once even when several forms ask at the same time', async () => { + assetsApi.types.list.mockResolvedValue({ + data: { data: [{ assettypeid: 1, assettype: 'computer' }] } + }) + + await Promise.all([loadAssetTypeIds(), loadAssetTypeIds(), loadAssetTypeIds()]) + await loadAssetTypeIds() + + expect(assetsApi.types.list).toHaveBeenCalledTimes(1) + }) + + it('returns undefined rather than a guess when the load fails', async () => { + assetsApi.types.list.mockRejectedValue(new Error('network down')) + + await loadAssetTypeIds() + + // Undefined means CustomFieldsInputs renders nothing. A missing panel is + // honest; the wrong type's panel is what this replaces. + expect(assetTypeId('computer')).toBeUndefined() + expect(state.loaded).toBe(false) + }) + + it('returns undefined for a type the server does not know', async () => { + assetsApi.types.list.mockResolvedValue({ + data: { data: [{ assettypeid: 1, assettype: 'computer' }] } + }) + + await loadAssetTypeIds() + + expect(assetTypeId('forklift')).toBeUndefined() + }) +}) diff --git a/plugins/computers/frontend/views/PCForm.vue b/plugins/computers/frontend/views/PCForm.vue index 6924908f..13b64e20 100644 --- a/plugins/computers/frontend/views/PCForm.vue +++ b/plugins/computers/frontend/views/PCForm.vue @@ -373,6 +373,7 @@ import CustomFieldsInputs from '@/components/CustomFieldsInputs.vue' import PrinterAssignmentPicker from '@/components/PrinterAssignmentPicker.vue' import { currentTheme } from '@/stores/theme' import { useIdentifierFlags } from '@/composables/identifierSettings' +import { loadAssetTypeIds, assetTypeId } from '@/composables/assetTypeIds' import { isPluginEnabled, loadEnabledPlugins } from '@/composables/enabledPlugins' import { useToast } from '@/composables/toast' import { apiError } from '@/utils/apiError' @@ -390,9 +391,11 @@ const isEdit = computed(() => !!route.params.id) // falls back to the PCs list. const returnTo = computed(() => (route.query.from === 'fleet' ? '/endpoints' : '/pcs')) -// Seeded asset-type id for computers (see /api/assets/types). Custom-field -// values are keyed by the underlying asset id, captured on load / create. -const COMPUTER_ASSETTYPEID = 2 +// Resolved by NAME at load, never hardcoded: the id comes from seed +// order, so the constant that used to live here pointed at a different +// asset type on a differently-seeded site - the form then rendered THAT +// type's custom fields and dropped whatever was typed into them. +const COMPUTER_ASSETTYPEID = computed(() => assetTypeId('computer')) const customFieldsRef = ref(null) const printerPickerRef = ref(null) const currentAssetId = ref(null) @@ -519,6 +522,7 @@ const filteredVendors = computed(() => { }) onMounted(async () => { + await loadAssetTypeIds() try { // Load reference data // perpage 100 so dropdowns aren't truncated to the default 20-row page @@ -649,7 +653,13 @@ async function savePC() { notes: form.value.notes || null, ipaddress: form.value.ipaddress || null, mapx: form.value.mapx, - mapy: form.value.mapy + mapy: form.value.mapy, + // ADR-017: a position without its level cannot be drawn on the right + // floor plan. The picker sets all three and this payload sent two, so a + // picked pin saved a levelid of null and then rendered nowhere - the + // form even showed "Position: x, y" back, because it kept the value it + // had never sent. + levelid: form.value.levelid } // only set display name when an alias is given, so we don't clobber it if (form.value.alias) { diff --git a/plugins/machines/frontend/views/MachineForm.vue b/plugins/machines/frontend/views/MachineForm.vue index 4f159a5c..6350c449 100644 --- a/plugins/machines/frontend/views/MachineForm.vue +++ b/plugins/machines/frontend/views/MachineForm.vue @@ -416,6 +416,7 @@ import CustomFieldsInputs from '@/components/CustomFieldsInputs.vue' import PrinterAssignmentPicker from '@/components/PrinterAssignmentPicker.vue' import { currentTheme } from '@/stores/theme' import { useIdentifierFlags } from '@/composables/identifierSettings' +import { loadAssetTypeIds, assetTypeId } from '@/composables/assetTypeIds' import { apiError } from '@/utils/apiError' import { planControllingPcChange } from './controllingPc' @@ -426,8 +427,11 @@ const router = useRouter() const isEdit = computed(() => !!route.params.id) -// Seeded asset-type id for machines (see /api/assets/types). -const MACHINE_ASSETTYPEID = 1 +// Resolved by NAME at load, never hardcoded: the id comes from seed +// order, so the constant that used to live here pointed at a different +// asset type on a differently-seeded site - the form then rendered THAT +// type's custom fields and dropped whatever was typed into them. +const MACHINE_ASSETTYPEID = computed(() => assetTypeId('machine')) const customFieldsRef = ref(null) const printerPickerRef = ref(null) const currentAssetId = ref(null) @@ -571,6 +575,7 @@ watch(() => form.value.controllervendorid, (newVal, oldVal) => { }) onMounted(async () => { + await loadAssetTypeIds() try { // Load reference data in parallel const [typesRes, statusRes, vendorRes, locRes, allModels, buRes, pcsRes, relTypesRes] = await Promise.all([ @@ -763,7 +768,13 @@ async function saveMachine() { maintenanceintervaldays: form.value.maintenanceintervaldays || null, notes: form.value.notes || null, mapx: form.value.mapx, - mapy: form.value.mapy + mapy: form.value.mapy, + // ADR-017: a position without its level cannot be drawn on the right + // floor plan. The picker sets all three and this payload sent two, so a + // picked pin saved a levelid of null and then rendered nowhere - the + // form even showed "Position: x, y" back, because it kept the value it + // had never sent. + levelid: form.value.levelid } let savedMachine diff --git a/plugins/network/frontend/views/NetworkDeviceForm.vue b/plugins/network/frontend/views/NetworkDeviceForm.vue index ff6c6ef1..e3b6cf4d 100644 --- a/plugins/network/frontend/views/NetworkDeviceForm.vue +++ b/plugins/network/frontend/views/NetworkDeviceForm.vue @@ -334,6 +334,7 @@ import { loadMapConfig, levelOptions, levelName, state as mapConfig } import Modal from '@/components/Modal.vue' import { currentTheme } from '@/stores/theme' import { useIdentifierFlags } from '@/composables/identifierSettings' +import { loadAssetTypeIds, assetTypeId } from '@/composables/assetTypeIds' import { apiError } from '@/utils/apiError' const { isEnabled } = useIdentifierFlags() @@ -350,8 +351,11 @@ const router = useRouter() const deviceId = route.params.id const isEdit = computed(() => !!deviceId) -// Seeded asset-type id for network devices (see /api/assets/types). -const NETWORK_ASSETTYPEID = 3 +// Resolved by NAME at load, never hardcoded: the id comes from seed +// order, so the constant that used to live here pointed at a different +// asset type on a differently-seeded site - the form then rendered THAT +// type's custom fields and dropped whatever was typed into them. +const NETWORK_ASSETTYPEID = computed(() => assetTypeId('network_device')) const customFieldsRef = ref(null) const currentAssetId = ref(null) @@ -424,6 +428,7 @@ const saving = ref(false) const error = ref('') onMounted(async () => { + await loadAssetTypeIds() await Promise.all([ loadDeviceTypes(), loadVendors(), diff --git a/plugins/printers/frontend/views/PrinterForm.vue b/plugins/printers/frontend/views/PrinterForm.vue index 9a92f7d8..b5a2932d 100644 --- a/plugins/printers/frontend/views/PrinterForm.vue +++ b/plugins/printers/frontend/views/PrinterForm.vue @@ -336,6 +336,7 @@ import Modal from '@/components/Modal.vue' import CustomFieldsInputs from '@/components/CustomFieldsInputs.vue' import { currentTheme } from '@/stores/theme' import { useIdentifierFlags } from '@/composables/identifierSettings' +import { loadAssetTypeIds, assetTypeId } from '@/composables/assetTypeIds' import { apiError } from '@/utils/apiError' import { getPrinterHostnameTemplate } from '@/utils/siteSettings' @@ -346,8 +347,11 @@ const router = useRouter() const isEdit = computed(() => !!route.params.id) -// Seeded asset-type id for printers (see /api/assets/types). -const PRINTER_ASSETTYPEID = 4 +// Resolved by NAME at load, never hardcoded: the id comes from seed +// order, so the constant that used to live here pointed at a different +// asset type on a differently-seeded site - the form then rendered THAT +// type's custom fields and dropped whatever was typed into them. +const PRINTER_ASSETTYPEID = computed(() => assetTypeId('printer')) const customFieldsRef = ref(null) const currentAssetId = ref(null) const manualHostname = ref(false) @@ -542,6 +546,7 @@ function onWindowsNameInput() { } onMounted(async () => { + await loadAssetTypeIds() try { // Load reference data (models paged in full via listAll, see api/index.js) // perpage 100 so dropdowns aren't truncated to the default 20-row page @@ -686,7 +691,13 @@ async function savePrinter() { isduplex: !!form.value.isduplex, isnetwork: !!form.value.isnetwork, mapx: form.value.mapx, - mapy: form.value.mapy + mapy: form.value.mapy, + // ADR-017: a position without its level cannot be drawn on the right + // floor plan. The picker sets all three and this payload sent two, so a + // picked pin saved a levelid of null and then rendered nowhere - the + // form even showed "Position: x, y" back, because it kept the value it + // had never sent. + levelid: form.value.levelid } // only set the display name when an alias is given, so we don't clobber it if (form.value.alias) { diff --git a/scripts/check-naming-and-style.sh b/scripts/check-naming-and-style.sh index b251c412..d6743f82 100755 --- a/scripts/check-naming-and-style.sh +++ b/scripts/check-naming-and-style.sh @@ -191,11 +191,19 @@ while IFS= read -r hit; do [ -z "$hit" ] && continue file=${hit%%:*} line=${hit#*:}; line=${line%%:*} - if ! sed -n "${line},$((line + 8))p" "$file" | grep -q "'levelid'"; then + # Python writes 'levelid', Vue writes levelid: - accept either, so the + # check covers the payloads a browser sends as well as the ones the server + # emits. It was Python-only, which is how three asset forms shipped a + # position with no level. + if ! sed -n "${line},$((line + 8))p" "$file" | grep -qE "'levelid'|levelid:"; then MISSING_LEVEL="$MISSING_LEVEL$file:$line"$'\n' fi done </dev/null | grep -v '/tests\?/' || true) +$(grep -rn "mapx: " --include='*.vue' --include='*.js' \ + frontend/src plugins/*/frontend 2>/dev/null \ + | grep -v '.plugins-staged' | grep -v '\.spec\.js' \ + | grep -vE "mapx: (null|0)," || true) EOF if [ -n "$MISSING_LEVEL" ]; then echo "FAIL: these emit 'mapx' with no 'levelid' beside it - a position with no"