From 94fd70391a8d4523aa0bf9c010db6ef8d0b55c75 Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Fri, 21 Aug 2026 15:11:04 -0400 Subject: [PATCH 01/13] Secure send: the pages a person actually touches --- plugins/tools/frontend/api.js | 23 +++ plugins/tools/frontend/routes.js | 22 ++ plugins/tools/frontend/securesend.js | 136 +++++++++++++ plugins/tools/frontend/tools.js | 10 + .../tools/frontend/views/SecureSendCreate.vue | 190 ++++++++++++++++++ .../tools/frontend/views/SecureSendRead.vue | 154 ++++++++++++++ 6 files changed, 535 insertions(+) create mode 100644 plugins/tools/frontend/api.js create mode 100644 plugins/tools/frontend/securesend.js create mode 100644 plugins/tools/frontend/views/SecureSendCreate.vue create mode 100644 plugins/tools/frontend/views/SecureSendRead.vue diff --git a/plugins/tools/frontend/api.js b/plugins/tools/frontend/api.js new file mode 100644 index 00000000..5d5ed13c --- /dev/null +++ b/plugins/tools/frontend/api.js @@ -0,0 +1,23 @@ +// Tech Tools API client. +// +// Only secure send needs one - every other tool runs entirely in the browser +// and talks to nothing. +import api from '@/api' + +export const toolsApi = { + createSecret(payload) { + return api.post('/tools/secrets', payload) + }, + + // Is the link still live? Spends nothing, so it is safe for the read page to + // ask before anyone clicks. + peekSecret(token) { + return api.get(`/tools/secrets/${token}`) + }, + + // POST, not GET, and that is deliberate: chat and mail clients prefetch GET + // urls to build previews, and a preview must not consume a one-view secret. + redeemSecret(token) { + return api.post(`/tools/secrets/${token}`) + }, +} diff --git a/plugins/tools/frontend/routes.js b/plugins/tools/frontend/routes.js index e17ac47e..3af4a1c0 100644 --- a/plugins/tools/frontend/routes.js +++ b/plugins/tools/frontend/routes.js @@ -23,4 +23,26 @@ export default [ component: () => import('./views/CodeGenerator.vue'), meta: { requiresAuth: true, plugin: 'tools' } }, + { + path: 'tools/secure-send', + name: 'tools-secure-send', + component: () => import('./views/SecureSendCreate.vue'), + // Creating a secret needs a login AND tools.secrets. The route guard covers + // the login; the server enforces the permission, and the page surfaces its + // refusal rather than pretending the form worked. + meta: { requiresAuth: true, plugin: 'tools' } + }, +] + +// PUBLIC and OUTSIDE AppLayout. The recipient of a secret usually has no account +// here, so this page carries no navigation and asks for no login - the +// unguessable token in the path, and the key in the fragment, are what let them +// read it. Short path because the whole link gets pasted into a chat message. +export const toplevel = [ + { + path: '/s/:token', + name: 'secure-send-read', + component: () => import('./views/SecureSendRead.vue'), + meta: { plugin: 'tools' } + }, ] diff --git a/plugins/tools/frontend/securesend.js b/plugins/tools/frontend/securesend.js new file mode 100644 index 00000000..e2ced454 --- /dev/null +++ b/plugins/tools/frontend/securesend.js @@ -0,0 +1,136 @@ +// Client-side crypto for secure send. +// +// THE KEY NEVER LEAVES THIS FILE'S PROCESS. It is generated here, put in the +// URL fragment, and read back out of the fragment on the other side. Browsers +// do not transmit fragments, so it reaches the recipient's browser and nowhere +// else - not the server, not its logs, not a proxy in between. That is what +// makes the stored ciphertext useless to anyone who reads the database. +// +// AES-GCM because it authenticates as well as encrypts: a tampered ciphertext +// fails to decrypt rather than producing plausible rubbish. +// +// Web Crypto only, no dependency. It is available on every browser the fleet +// runs and requires a secure context, which is the right constraint anyway - +// the page and its scripts still cross the network even though the key does not. + +const ALGORITHM = 'AES-GCM' +const KEY_BITS = 256 +const IV_BYTES = 12 + +function toBase64(bytes) { + let binary = '' + for (const byte of new Uint8Array(bytes)) binary += String.fromCharCode(byte) + return btoa(binary) +} + +function fromBase64(text) { + const binary = atob(text) + const bytes = new Uint8Array(binary.length) + for (let i = 0; i < binary.length; i += 1) bytes[i] = binary.charCodeAt(i) + return bytes +} + +// base64url for the fragment: '+' and '/' are legal in a fragment but survive +// copy-paste and link rewriting far less reliably than the url-safe alphabet. +function toBase64Url(bytes) { + return toBase64(bytes).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '') +} + +function fromBase64Url(text) { + const padded = text.replace(/-/g, '+').replace(/_/g, '/') + return fromBase64(padded + '='.repeat((4 - (padded.length % 4)) % 4)) +} + +export function cryptoAvailable() { + return typeof crypto !== 'undefined' && !!crypto.subtle +} + +/** Encrypt text. Returns the payload for the server AND the key for the link. */ +export async function sealSecret(plaintext) { + const key = await crypto.subtle.generateKey( + { name: ALGORITHM, length: KEY_BITS }, true, ['encrypt', 'decrypt']) + const iv = crypto.getRandomValues(new Uint8Array(IV_BYTES)) + const ciphertext = await crypto.subtle.encrypt( + { name: ALGORITHM, iv }, key, new TextEncoder().encode(plaintext)) + const raw = await crypto.subtle.exportKey('raw', key) + return { + ciphertext: toBase64(ciphertext), + iv: toBase64(iv), + // Goes in the fragment. Never sent anywhere. + fragmentKey: toBase64Url(raw), + } +} + +/** Decrypt what the server handed back, using the key from the fragment. */ +export async function openSecret({ ciphertext, iv, fragmentKey }) { + const key = await crypto.subtle.importKey( + 'raw', fromBase64Url(fragmentKey), + { name: ALGORITHM, length: KEY_BITS }, false, ['decrypt']) + const plaintext = await crypto.subtle.decrypt( + { name: ALGORITHM, iv: fromBase64(iv) }, key, fromBase64(ciphertext)) + return new TextDecoder().decode(plaintext) +} + +// -- Keeping this from becoming a document transfer --------------------------- +// +// The server can only bound SIZE, because it cannot read what it holds. These +// run here, where the plaintext is still visible, and are advisory: they guide +// somebody who is about to do the wrong thing without realising. A determined +// person can POST directly, which is why the size limit on the server is the +// control that actually enforces. + +// Matches MAX_CIPHERTEXT server-side, minus room for base64 expansion and the +// GCM tag, so the browser refuses before a round trip rather than after one. +export const MAX_PLAINTEXT = 2800 + +const MAX_LINES = 20 + +// A pasted file, recognised by how it starts. Cheap, high signal. +const FILE_SIGNATURES = [ + ['%PDF', 'a PDF'], + ['PK\x03\x04', 'a zip or Office file'], + ['{\\rtf', 'an RTF document'], + ['\x89PNG', 'a PNG image'], + // JPEG magic bytes as escapes rather than literals: the naming gate bans + // non-ASCII source, and these are bytes rather than text anyway. + ['\xff\xd8\xff', 'a JPEG image'], + ['data:', 'a data URI'], +] + +/** Why this text should not be sent, or null when it is fine. */ +export function refuseReason(text) { + const value = text || '' + if (!value.trim()) return 'Nothing to send yet.' + + if (value.length > MAX_PLAINTEXT) { + return `That is ${value.length} characters. This is for passwords and short ` + + `text, not documents - the limit is ${MAX_PLAINTEXT}.` + } + + for (const [signature, description] of FILE_SIGNATURES) { + if (value.startsWith(signature)) { + return `That looks like ${description} pasted as text. Secure send is for ` + + 'credentials, not file transfer.' + } + } + + const lines = value.split('\n').length + if (lines > MAX_LINES) { + return `That is ${lines} lines. A password is one - this looks like a ` + + 'document, and secure send is not a file transfer.' + } + + return null +} + +/** + * Reject a paste that carries FILES rather than text. + * + * Removes the easiest wrong path - dragging a PDF in, or Ctrl+V on a copied + * file - without anyone having to read a rule about it. + */ +export function pasteCarriesFiles(event) { + const items = event?.clipboardData?.items + if (!items) return false + return Array.from(items).some(item => item.kind === 'file') +} diff --git a/plugins/tools/frontend/tools.js b/plugins/tools/frontend/tools.js index aa93420b..95127ee0 100644 --- a/plugins/tools/frontend/tools.js +++ b/plugins/tools/frontend/tools.js @@ -24,11 +24,21 @@ export const TOOLS = [ standalone: true, keywords: ['qr', 'barcode', 'code128', 'label', 'zebra', 'sticker', 'csv', 'print'], }, + { + id: 'secure-send', + name: 'Secure Send', + description: 'Hand somebody a password by a link that works once and then destroys it.', + category: 'security', + route: '/tools/secure-send', + keywords: ['password', 'secret', 'credential', 'share', 'link', 'expire', + 'one time', 'burn', 'pusher'], + }, ] export const CATEGORY_LABELS = { labels: 'Labels & Printing', convert: 'Conversion', + security: 'Secrets & Access', network: 'Network', } diff --git a/plugins/tools/frontend/views/SecureSendCreate.vue b/plugins/tools/frontend/views/SecureSendCreate.vue new file mode 100644 index 00000000..dd308d72 --- /dev/null +++ b/plugins/tools/frontend/views/SecureSendCreate.vue @@ -0,0 +1,190 @@ + + + + + diff --git a/plugins/tools/frontend/views/SecureSendRead.vue b/plugins/tools/frontend/views/SecureSendRead.vue new file mode 100644 index 00000000..e562f3c0 --- /dev/null +++ b/plugins/tools/frontend/views/SecureSendRead.vue @@ -0,0 +1,154 @@ + + + + + From 627804535e7be41f95a9932da2b645c59e69905e Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Fri, 21 Aug 2026 15:19:43 -0400 Subject: [PATCH 02/13] Secret links carry the site URL and the mount path --- .../tools/frontend/views/SecureSendCreate.vue | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/plugins/tools/frontend/views/SecureSendCreate.vue b/plugins/tools/frontend/views/SecureSendCreate.vue index dd308d72..5c2621a8 100644 --- a/plugins/tools/frontend/views/SecureSendCreate.vue +++ b/plugins/tools/frontend/views/SecureSendCreate.vue @@ -91,6 +91,8 @@ + + diff --git a/plugins/tools/plugin.py b/plugins/tools/plugin.py index 6f369519..b86a564b 100644 --- a/plugins/tools/plugin.py +++ b/plugins/tools/plugin.py @@ -92,6 +92,28 @@ class ToolsPlugin(BasePlugin): }, ] + def get_settings_cards(self) -> List[dict]: + """A page for the one switch this plugin owns (ADR-010). + + get_settings_defaults SEEDS the row; this is what gives somebody a place + to change it. Declaring only the first left the setting in the database + with no way to reach it, which is indistinguishable from the feature not + existing. + """ + return [ + { + 'group': 'Tools', + 'to': '/settings/secure-send', + # From the settings iconMap; an unknown key falls back to a + # puzzle piece, which reads as "nobody chose this". + 'icon': 'wrench', + 'title': 'Secure Send', + 'description': 'Whether people without an account can create ' + 'one-view password links', + 'position': 60, + }, + ] + def get_navigation_items(self) -> List[dict]: return [ { From ce3a0f43380539d19b320da7de0d9ae105c8626f Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Mon, 24 Aug 2026 08:55:45 -0400 Subject: [PATCH 08/13] Secure Send documented, and the machine form stops re-creating a controlling-PC link that already exists --- CHANGELOG.md | 42 ++++++++++ .../machines/frontend/views/MachineForm.vue | 60 +++++++++----- .../machines/frontend/views/controllingPc.js | 68 ++++++++++++++++ .../frontend/views/controllingPc.spec.js | 81 +++++++++++++++++++ 4 files changed, 232 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 4f8ca72f..e51b1789 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,30 @@ ADR-007 and ADR-002. ### Added +- **Secure Send: hand somebody a password without leaving it in a chat + window.** A link in Tech Tools that carries one credential, dies when it is + opened, and expires on its own if nobody opens it. The secret is encrypted in + the sender's browser and the key rides in the URL fragment, which browsers + never transmit, so the server stores ciphertext it cannot read and a + compromised database yields nothing. Nobody can recover a lost secret either, + including an administrator, which is the same property seen from the other + side. Redeeming is a POST because chat and mail clients prefetch links to + build previews, and a one-view secret consumed by a preview window is a secret + the recipient never sees; a GET reports availability only. Expiry defaults to + 24 hours (maximum a week) and views to one, whichever comes first, and a spent + row is deleted. Creating needs a login and `tools.secrets`; reading needs + neither, because the recipient usually has no account and the 128-bit token is + the authorisation. The 4096-character ciphertext limit is deliberately the + whole server-side enforcement: a zero-knowledge server cannot tell a password + from a document, so size is the only control it has, and raising it turns this + into the file transfer it was scoped not to be. Documented in + `docs/SECURE-SEND.md`. +- **A site can let people without an account create a Secure Send link.** + New `securesend_allow_guests` setting (Settings > Tools > Secure Send), off by + default because a guest secret has no creator and the audit trail can then say + when and from where but never who. Guest creation is rate limited per address + and the setting fails closed if it cannot be read, so an error can never be + what opens an unauthenticated write endpoint. - **The code generator can print a back side.** A badge card is two-sided and the generator only ever emitted one page per card. The Back side section turns on a second page per card carrying either the same code again, a fixed @@ -81,9 +105,27 @@ 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 +- **Tech Tools no longer asks people to sign in for tools that need no + account.** `/tools`, `/tools/codes` and `/tools/secure-send` were all behind + `requiresAuth`. The label generator runs entirely in the browser - no API, no + data, nothing to protect - so gating it only ever cost a guest the use of it, + and gating the Secure Send page contradicted the guest setting that lets + somebody with no account create a link. The server still decides who may + create a secret: making a page reachable is not the same as allowing the + action, and the two were conflated. - **Settings on the left, labels on the right.** The controls were a narrow column with the preview trailing underneath, so the labels being adjusted were off the bottom of the screen. The preview is now a column of its own that 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 }) + }) +}) From f109dd134946139da0d0b2bc841fd67506ca6321 Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Mon, 24 Aug 2026 09:28:52 -0400 Subject: [PATCH 09/13] Plugin baselines skip tables that already exist --- CHANGELOG.md | 14 +++ .../versions/0001_backups_baseline.py | 21 +++-- .../versions/0001_geenforce_baseline.py | 71 ++++++++------- .../versions/0002_manifest_blobs.py | 7 +- .../versions/0001_measuringtools_baseline.py | 13 ++- .../versions/0001_printedparts_baseline.py | 23 +++-- .../versions/0002_printeditemfiles.py | 11 ++- .../versions/0002_printersupplyalerts.py | 11 ++- .../versions/0005_observedqueues.py | 15 ++-- .../versions/0001_securesend_baseline.py | 19 ++-- shopdb/plugins/alembic_template.py | 68 ++++++++++++++- tests/test_plugin_migrations.py | 86 +++++++++++++++++++ 12 files changed, 285 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e51b1789..c3c714d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,20 @@ 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. +- **`flask plugin upgrade-all` failed a plugin whose tables already existed** + ("table securesendsecrets already exists" on the tools plugin). Every + post-cutover baseline emitted a plain `op.create_table`, which is correct only + while a table's existence and its plugin's Alembic stamp cannot disagree. + They can: anything that builds tables from the models stamps nothing, and a + run that creates a table then fails before its stamp commits leaves the same + split, where re-running is the obvious response. Creates and indexes across + all nine table-creating plugin migrations now skip what is already there. + Indexes are matched on their COLUMNS rather than their name, because + `db.create_all()` names them by SQLAlchemy's convention and a migration by + this repo's, so a name-keyed check would silently add a duplicate index + instead of skipping. Note the remaining limit: this makes a baseline + idempotent, not a whole chain. A longer chain can still meet an `ALTER` whose + column is already present, which no guard on `create_table` addresses. - **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 diff --git a/plugins/backups/migrations/versions/0001_backups_baseline.py b/plugins/backups/migrations/versions/0001_backups_baseline.py index 885a3ca9..5af4fb47 100644 --- a/plugins/backups/migrations/versions/0001_backups_baseline.py +++ b/plugins/backups/migrations/versions/0001_backups_baseline.py @@ -12,6 +12,11 @@ time (NoReferencedTableError). Same shape autogenerate produces. Tables inherit the connection's default charset, matching how the core chain creates its own. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -23,7 +28,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'backuprevisions', sa.Column('backuprevisionid', sa.Integer(), nullable=False), sa.Column('assetid', sa.Integer(), nullable=False), @@ -42,15 +47,15 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('backuprevisionid'), ) - op.create_index('ixbackuprevisionsassetid', 'backuprevisions', ['assetid']) - op.create_index('ixbackuprevisionsbackupkind', 'backuprevisions', - ['backupkind']) - op.create_index('ixbackuprevisionscontenthash', 'backuprevisions', - ['contenthash']) + create_index_if_missing('ixbackuprevisionsassetid', 'backuprevisions', ['assetid']) + create_index_if_missing('ixbackuprevisionsbackupkind', 'backuprevisions', + ['backupkind']) + create_index_if_missing('ixbackuprevisionscontenthash', 'backuprevisions', + ['contenthash']) # The dedup read path is "latest revision for this asset+kind", so the # composite index is the one that actually gets used on every collector post. - op.create_index('ixbackuprevisionsassetkind', 'backuprevisions', - ['assetid', 'backupkind']) + create_index_if_missing('ixbackuprevisionsassetkind', 'backuprevisions', + ['assetid', 'backupkind']) def downgrade(): diff --git a/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py b/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py index 2c9831f5..e232aacb 100644 --- a/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py +++ b/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py @@ -8,6 +8,11 @@ tables are created parent-first. computertypeid / measuringtooltypeid / publishedby are SOFT references (plain columns, no FK) to optional plugins/core. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -19,7 +24,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'manifestscopes', sa.Column('scopeid', sa.Integer(), nullable=False), sa.Column('scopename', sa.String(length=64), nullable=False), @@ -37,7 +42,7 @@ def upgrade(): sa.PrimaryKeyConstraint('scopeid'), sa.UniqueConstraint('scopename', 'phase', name='uq_scope_name_phase'), ) - op.create_table( + create_table_if_missing( 'manifestentries', sa.Column('entryid', sa.Integer(), nullable=False), sa.Column('scopeid', sa.Integer(), nullable=False), @@ -80,10 +85,10 @@ def upgrade(): sa.PrimaryKeyConstraint('entryid'), sa.UniqueConstraint('scopeid', 'name', name='uq_entry_scope_name'), ) - op.create_index('idx_entry_scope', 'manifestentries', ['scopeid']) - op.create_index('idx_entry_scope_order', 'manifestentries', - ['scopeid', 'sortorder']) - op.create_table( + create_index_if_missing('idx_entry_scope', 'manifestentries', ['scopeid']) + create_index_if_missing('idx_entry_scope_order', 'manifestentries', + ['scopeid', 'sortorder']) + create_table_if_missing( 'manifestentrypctypes', sa.Column('entrypctypeid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -93,8 +98,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entrypctypeid'), ) - op.create_index('idx_pctype_entry', 'manifestentrypctypes', ['entryid']) - op.create_table( + create_index_if_missing('idx_pctype_entry', 'manifestentrypctypes', ['entryid']) + create_table_if_missing( 'manifestentryhostnames', sa.Column('entryhostnameid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -104,8 +109,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entryhostnameid'), ) - op.create_index('idx_hostname_entry', 'manifestentryhostnames', ['entryid']) - op.create_table( + create_index_if_missing('idx_hostname_entry', 'manifestentryhostnames', ['entryid']) + create_table_if_missing( 'manifestentrymachinenumbers', sa.Column('entrymachinenumberid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -115,9 +120,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entrymachinenumberid'), ) - op.create_index('idx_machinenumber_entry', 'manifestentrymachinenumbers', - ['entryid']) - op.create_table( + create_index_if_missing('idx_machinenumber_entry', 'manifestentrymachinenumbers', + ['entryid']) + create_table_if_missing( 'manifestinusechecks', sa.Column('inusecheckid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -127,7 +132,7 @@ def upgrade(): sa.PrimaryKeyConstraint('inusecheckid'), sa.UniqueConstraint('entryid'), ) - op.create_table( + create_table_if_missing( 'manifestinusecheckprocesses', sa.Column('inusecheckprocessid', sa.Integer(), nullable=False), sa.Column('inusecheckid', sa.Integer(), nullable=False), @@ -140,9 +145,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('inusecheckprocessid'), ) - op.create_index('idx_process_check', 'manifestinusecheckprocesses', - ['inusecheckid']) - op.create_table( + create_index_if_missing('idx_process_check', 'manifestinusecheckprocesses', + ['inusecheckid']) + create_table_if_missing( 'manifestpublishedversions', sa.Column('publishedversionid', sa.Integer(), nullable=False), sa.Column('scopeid', sa.Integer(), nullable=False), @@ -158,9 +163,9 @@ def upgrade(): sa.UniqueConstraint('scopeid', 'versionnumber', name='uq_published_scope_version'), ) - op.create_index('idx_published_scope', 'manifestpublishedversions', - ['scopeid']) - op.create_table( + create_index_if_missing('idx_published_scope', 'manifestpublishedversions', + ['scopeid']) + create_table_if_missing( 'manifestpayloads', sa.Column('payloadid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -173,8 +178,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('payloadid'), ) - op.create_index('idx_payload_entry', 'manifestpayloads', ['entryid']) - op.create_table( + create_index_if_missing('idx_payload_entry', 'manifestpayloads', ['entryid']) + create_table_if_missing( 'manifestenforcementreports', sa.Column('reportid', sa.Integer(), nullable=False), sa.Column('hostname', sa.String(length=100), nullable=False), @@ -192,13 +197,13 @@ def upgrade(): sa.Column('iscurrent', sa.Boolean(), nullable=False), sa.PrimaryKeyConstraint('reportid'), ) - op.create_index('idx_report_host', 'manifestenforcementreports', - ['hostname']) - op.create_index('idx_report_current', 'manifestenforcementreports', - ['iscurrent']) - op.create_index('idx_report_host_scope', 'manifestenforcementreports', - ['hostname', 'scopename', 'phase']) - op.create_table( + create_index_if_missing('idx_report_host', 'manifestenforcementreports', + ['hostname']) + create_index_if_missing('idx_report_current', 'manifestenforcementreports', + ['iscurrent']) + create_index_if_missing('idx_report_host_scope', 'manifestenforcementreports', + ['hostname', 'scopename', 'phase']) + create_table_if_missing( 'manifestenforcementresults', sa.Column('resultid', sa.Integer(), nullable=False), sa.Column('reportid', sa.Integer(), nullable=False), @@ -212,9 +217,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('resultid'), ) - op.create_index('idx_result_report', 'manifestenforcementresults', - ['reportid']) - op.create_table( + create_index_if_missing('idx_result_report', 'manifestenforcementresults', + ['reportid']) + create_table_if_missing( 'pctypealiases', sa.Column('aliasid', sa.Integer(), nullable=False), sa.Column('aliasgroup', sa.Integer(), nullable=False), @@ -223,7 +228,7 @@ def upgrade(): sa.UniqueConstraint('aliasgroup', 'aliasname', name='uq_alias_group_name'), ) - op.create_index('idx_alias_group', 'pctypealiases', ['aliasgroup']) + create_index_if_missing('idx_alias_group', 'pctypealiases', ['aliasgroup']) def downgrade(): diff --git a/plugins/geenforce/migrations/versions/0002_manifest_blobs.py b/plugins/geenforce/migrations/versions/0002_manifest_blobs.py index d196a2ce..31081846 100644 --- a/plugins/geenforce/migrations/versions/0002_manifest_blobs.py +++ b/plugins/geenforce/migrations/versions/0002_manifest_blobs.py @@ -10,6 +10,11 @@ Create Date: 2026-07-21 """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -24,7 +29,7 @@ def upgrade(): insp = sa.inspect(bind) if 'manifestblobs' in insp.get_table_names(): return - op.create_table( + create_table_if_missing( 'manifestblobs', sa.Column('sha256', sa.String(length=64), nullable=False), sa.Column('filename', sa.String(length=255), nullable=False), diff --git a/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py b/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py index 8b102464..15722d64 100644 --- a/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py +++ b/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py @@ -17,6 +17,11 @@ connection's default charset, so on a utf8mb4 database they are utf8mb4, matchin how the core chain creates its tables. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -28,7 +33,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'measuringtooltypes', sa.Column('measuringtooltypeid', sa.Integer(), nullable=False), sa.Column('name', sa.String(length=100), nullable=False), @@ -40,7 +45,7 @@ def upgrade(): sa.PrimaryKeyConstraint('measuringtooltypeid'), sa.UniqueConstraint('name'), ) - op.create_table( + create_table_if_missing( 'measuringtools', sa.Column('measuringtoolid', sa.Integer(), nullable=False), sa.Column('assetid', sa.Integer(), nullable=False), @@ -59,8 +64,8 @@ def upgrade(): sa.PrimaryKeyConstraint('measuringtoolid'), sa.UniqueConstraint('assetid'), ) - op.create_index('idx_measuringtool_type', 'measuringtools', - ['measuringtooltypeid']) + create_index_if_missing('idx_measuringtool_type', 'measuringtools', + ['measuringtooltypeid']) def downgrade(): diff --git a/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py b/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py index f0bb3484..3d16b508 100644 --- a/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py +++ b/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py @@ -11,6 +11,11 @@ ops are written out explicitly anyway to match the measuringtools exemplar and keep the baseline reviewable. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -22,7 +27,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'printeditems', sa.Column('printeditemid', sa.Integer(), nullable=False), sa.Column('itemcode', sa.String(length=20), nullable=True), @@ -39,9 +44,9 @@ def upgrade(): sa.PrimaryKeyConstraint('printeditemid'), sa.UniqueConstraint('itemcode'), ) - op.create_index('ix_printeditems_itemcode', 'printeditems', ['itemcode']) + create_index_if_missing('ix_printeditems_itemcode', 'printeditems', ['itemcode']) - op.create_table( + create_table_if_missing( 'printeditemtransactions', sa.Column('transactionid', sa.Integer(), nullable=False), sa.Column('printeditemid', sa.Integer(), nullable=False), @@ -58,12 +63,12 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('transactionid'), ) - op.create_index('ix_printeditemtransactions_printeditemid', - 'printeditemtransactions', ['printeditemid']) - op.create_index('ix_printeditemtransactions_employeesso', - 'printeditemtransactions', ['employeesso']) - op.create_index('ix_printeditemtransactions_transactiondate', - 'printeditemtransactions', ['transactiondate']) + create_index_if_missing('ix_printeditemtransactions_printeditemid', + 'printeditemtransactions', ['printeditemid']) + create_index_if_missing('ix_printeditemtransactions_employeesso', + 'printeditemtransactions', ['employeesso']) + create_index_if_missing('ix_printeditemtransactions_transactiondate', + 'printeditemtransactions', ['transactiondate']) def downgrade(): diff --git a/plugins/printedparts/migrations/versions/0002_printeditemfiles.py b/plugins/printedparts/migrations/versions/0002_printeditemfiles.py index d0b2c9a9..0cfd8553 100644 --- a/plugins/printedparts/migrations/versions/0002_printeditemfiles.py +++ b/plugins/printedparts/migrations/versions/0002_printeditemfiles.py @@ -5,6 +5,11 @@ the ADR-008 payoff: the plugin evolves its own schema without touching the core chain. Applied by `flask plugin upgrade-all`. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -16,7 +21,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'printeditemfiles', sa.Column('fileid', sa.Integer(), nullable=False), sa.Column('printeditemid', sa.Integer(), nullable=False), @@ -35,8 +40,8 @@ def upgrade(): sa.PrimaryKeyConstraint('fileid'), sa.UniqueConstraint('storedfilename'), ) - op.create_index('ix_printeditemfiles_printeditemid', - 'printeditemfiles', ['printeditemid']) + create_index_if_missing('ix_printeditemfiles_printeditemid', + 'printeditemfiles', ['printeditemid']) def downgrade(): diff --git a/plugins/printers/migrations/versions/0002_printersupplyalerts.py b/plugins/printers/migrations/versions/0002_printersupplyalerts.py index e563e2ca..fe909c18 100644 --- a/plugins/printers/migrations/versions/0002_printersupplyalerts.py +++ b/plugins/printers/migrations/versions/0002_printersupplyalerts.py @@ -4,6 +4,11 @@ Stores the last tier alerted per (printer, supply) so the poller fires once on a downward crossing and re-arms after a refill. Idempotent create. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -19,7 +24,7 @@ def upgrade(): inspector = sa.inspect(bind) if 'printersupplyalerts' in inspector.get_table_names(): return - op.create_table( + create_table_if_missing( 'printersupplyalerts', sa.Column('printersupplyalertid', sa.Integer(), primary_key=True), sa.Column('printerid', sa.Integer(), nullable=False), @@ -35,8 +40,8 @@ def upgrade(): sa.UniqueConstraint('printerid', 'supplykey', name='uq_printersupplyalert_printer_key'), ) - op.create_index('ix_printersupplyalerts_printerid', - 'printersupplyalerts', ['printerid']) + create_index_if_missing('ix_printersupplyalerts_printerid', + 'printersupplyalerts', ['printerid']) def downgrade(): diff --git a/plugins/printers/migrations/versions/0005_observedqueues.py b/plugins/printers/migrations/versions/0005_observedqueues.py index 8eeea6cd..0a92c9a9 100644 --- a/plugins/printers/migrations/versions/0005_observedqueues.py +++ b/plugins/printers/migrations/versions/0005_observedqueues.py @@ -21,6 +21,11 @@ Revision ID: printers0005observedqueues Revises: printers0004drivervendor """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -36,7 +41,7 @@ def upgrade(): inspector = sa.inspect(bind) if 'printerobservedqueues' in inspector.get_table_names(): return - op.create_table( + create_table_if_missing( 'printerobservedqueues', sa.Column('printerobservedqueueid', sa.Integer(), nullable=False), # Nullable and no index of its own beyond the explicit one below: an @@ -67,10 +72,10 @@ def upgrade(): ) # Port address is the primary match key from an observed queue back to a # printer asset, so every comparison read hits it. - op.create_index('idx_printerobservedqueues_portaddress', - 'printerobservedqueues', ['portaddress']) - op.create_index('idx_printerobservedqueues_assetid', - 'printerobservedqueues', ['assetid']) + create_index_if_missing('idx_printerobservedqueues_portaddress', + 'printerobservedqueues', ['portaddress']) + create_index_if_missing('idx_printerobservedqueues_assetid', + 'printerobservedqueues', ['assetid']) def downgrade(): diff --git a/plugins/tools/migrations/versions/0001_securesend_baseline.py b/plugins/tools/migrations/versions/0001_securesend_baseline.py index ebdc7dae..2ead58df 100644 --- a/plugins/tools/migrations/versions/0001_securesend_baseline.py +++ b/plugins/tools/migrations/versions/0001_securesend_baseline.py @@ -12,6 +12,11 @@ plugin's tables, so the foreign key to the core users table cannot resolve at CreateTable-compile time. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -23,7 +28,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'securesendsecrets', sa.Column('secretid', sa.Integer(), nullable=False), # 128 bits of randomness as hex. With no login on the read side this is @@ -47,13 +52,13 @@ def upgrade(): ) # Unique, because the token is looked up on every read and two rows sharing # one would make a link ambiguous. - op.create_index('ixsecuresendsecretstoken', 'securesendsecrets', - ['token'], unique=True) + create_index_if_missing('ixsecuresendsecretstoken', 'securesendsecrets', + ['token'], unique=True) # Expiry sweeps scan on this. - op.create_index('ixsecuresendsecretsexpiresat', 'securesendsecrets', - ['expiresat']) - op.create_index('ixsecuresendsecretscreatedby', 'securesendsecrets', - ['createdbyuserid']) + create_index_if_missing('ixsecuresendsecretsexpiresat', 'securesendsecrets', + ['expiresat']) + create_index_if_missing('ixsecuresendsecretscreatedby', 'securesendsecrets', + ['createdbyuserid']) def downgrade(): diff --git a/shopdb/plugins/alembic_template.py b/shopdb/plugins/alembic_template.py index fa91fa24..70e272fe 100644 --- a/shopdb/plugins/alembic_template.py +++ b/shopdb/plugins/alembic_template.py @@ -104,7 +104,12 @@ def create_plugin_tables(plugin_name: str): this is safe on an existing database that has the table from the pre-cutover core baseline as well as on a fresh install (ADR-014 Phase 2). - Called from each plugin's 0001 baseline.py upgrade(). + NOT actually called from anywhere: every post-cutover baseline emits + explicit Alembic ops instead, because this helper's per-plugin MetaData + cannot resolve a foreign key to a CORE table at CreateTable-compile time. + Kept because it is still the reference for what idempotent means here, and + because create_table_if_missing below is the same rule in a form the + baselines can use. See that function for what went wrong without it. """ from alembic import op from sqlalchemy import inspect @@ -120,6 +125,67 @@ def create_plugin_tables(plugin_name: str): op.execute(str(CreateTable(table).compile(dialect=bind.dialect))) +def create_table_if_missing(table_name: str, *columns, **kwargs) -> bool: + """op.create_table, skipped when the table is already there. + + WHY EVERY POST-CUTOVER BASELINE NEEDS THIS. A baseline is the sole creator + of its plugin's tables, so it was written as a plain op.create_table - which + is correct only if the table's existence and the plugin's Alembic stamp can + never disagree. They can, and routinely do: + + * `flask db-utils create-all` runs db.create_all(), which builds EVERY + table registered on the metadata - plugin tables included - and stamps + NOTHING. `flask plugin upgrade-all` then runs the baseline against + tables that already exist. + * A run that creates the table and then fails before its stamp commits + leaves the same split, and re-running is the obvious thing to try. + + Reported as `flask plugin upgrade-all` failing the tools plugin with + "table securesendsecrets already exists". Nothing caught it because the + migration test drops the post-cutover tables db.create_all() made before + running upgrade-all, which arranges the one state where an unguarded create + works and never the state a real database is in. + + Returns True when it created the table, so a caller can skip the indexes + that belong to it. + """ + from alembic import op + from sqlalchemy import inspect + + if inspect(op.get_bind()).has_table(table_name): + logger.info('%s already exists, skipping create', table_name) + return False + op.create_table(table_name, *columns, **kwargs) + return True + + +def create_index_if_missing(index_name: str, table_name: str, columns, **kwargs): + """op.create_index, skipped when an equivalent index already exists. + + Matched on the COLUMN LIST rather than the name, because the two creators + do not agree on names. db.create_all() takes them from SQLAlchemy's default + convention (`ix_securesendsecrets_token`) while a baseline spells them the + way this repo's naming rules require (`ixsecuresendsecretstoken`). Matching + on the name would call the index missing and add a second one over the same + column - MySQL permits that, with only a warning, so the duplicate would + accumulate silently on every affected site. + """ + from alembic import op + from sqlalchemy import inspect + + wanted = [str(column) for column in columns] + inspector = inspect(op.get_bind()) + if not inspector.has_table(table_name): + op.create_index(index_name, table_name, columns, **kwargs) + return + for existing in inspector.get_indexes(table_name): + if list(existing.get('column_names') or []) == wanted: + logger.info('index over %s(%s) already exists as %s, skipping', + table_name, ', '.join(wanted), existing.get('name')) + return + op.create_index(index_name, table_name, columns, **kwargs) + + def drop_plugin_tables(plugin_name: str): """Mirror of create_plugin_tables for downgrade(). Drops in reverse FK order.""" diff --git a/tests/test_plugin_migrations.py b/tests/test_plugin_migrations.py index 54654416..e4ef3976 100644 --- a/tests/test_plugin_migrations.py +++ b/tests/test_plugin_migrations.py @@ -276,3 +276,89 @@ def test_upgrade_all_on_fresh_db_is_clean_and_idempotent(tmp_path, monkeypatch): (plugin_manager._app, plugin_manager._db, plugin_manager.registry, plugin_manager.loader, plugin_manager.migration_manager, plugin_manager._registered_prefixes) = saved + + + +# Plugins whose whole chain is a single baseline. Their tables existing already +# is the ONLY divergence possible between schema and stamp, so upgrade-all must +# handle it end to end. A longer chain can also meet an ALTER whose column is +# already present, which no amount of guarding a create_table fixes - see the +# test below for why that is a different problem. +SINGLE_BASELINE_PLUGINS = tuple( + plugin for plugin in POST_CUTOVER_PLUGINS + if len(list((PLUGINS_DIR / plugin / 'migrations' / 'versions').glob('0*.py'))) == 1 +) + + +@pytest.mark.parametrize('plugin', SINGLE_BASELINE_PLUGINS) +def test_baseline_skips_tables_that_already_exist(plugin, tmp_path, monkeypatch): + """A baseline must stamp, not fail, when its table is already there. + + Reported: `flask plugin upgrade-all` failed the tools plugin with "table + securesendsecrets already exists". Every post-cutover baseline was a plain + op.create_table, correct only while a table's existence and its plugin's + Alembic stamp can never disagree. They can - anything that builds tables + from the models (db.create_all) stamps nothing, and a run that creates a + table then fails before its stamp commits leaves the same split. + + The other upgrade-all test DROPS these tables before running, so it only + ever covered the state where they are absent. This is the other state. + """ + from sqlalchemy import inspect, text + from shopdb.config import TestingConfig + from shopdb import create_app + from shopdb.extensions import db + from shopdb.plugins import plugin_manager + + monkeypatch.setattr(TestingConfig, 'SQLALCHEMY_DATABASE_URI', + f'sqlite:///{tmp_path / "preexisting.db"}') + + saved = (plugin_manager._app, plugin_manager._db, plugin_manager.registry, + plugin_manager.loader, plugin_manager.migration_manager, + plugin_manager._registered_prefixes) + try: + app = create_app('testing') + with app.app_context(): + db.create_all() + + # Leave THIS plugin's tables in place - that is the whole point - + # and drop the other post-cutover ones so their own (longer) chains + # run the way the sibling test already covers. + insp0 = inspect(db.engine) + for other in POST_CUTOVER_PLUGINS: + if other == plugin: + continue + for tablename in PLUGIN_TABLE_OWNERS[other]: + if insp0.has_table(tablename): + db.session.execute(text(f'DROP TABLE {tablename}')) + db.session.commit() + + for tablename in PLUGIN_TABLE_OWNERS[plugin]: + assert inspect(db.engine).has_table(tablename), ( + f'{tablename} should have been built by create_all; ' + f'this test is meaningless without it') + + first = app.extensions['plugin_manager'].upgrade_all_plugins() + assert first[plugin] == 'ok', first[plugin] + + second = app.extensions['plugin_manager'].upgrade_all_plugins() + assert second[plugin] == 'ok', second[plugin] + + insp = inspect(db.engine) + row = db.session.execute( + text(f'SELECT version_num FROM alembic_version_{plugin}') + ).fetchone() + assert row and row[0] == EXPECTED_HEAD_REVISION[plugin] + + # No second index over the same columns: create_all names indexes by + # SQLAlchemy's convention and the baseline by this repo's, so a + # name-keyed guard would have added a duplicate rather than skipped. + for tablename in PLUGIN_TABLE_OWNERS[plugin]: + seen = [tuple(ix.get('column_names') or ()) + for ix in insp.get_indexes(tablename)] + assert len(seen) == len(set(seen)), ( + f'{tablename} has two indexes over the same columns: {seen}') + finally: + (plugin_manager._app, plugin_manager._db, plugin_manager.registry, + plugin_manager.loader, plugin_manager.migration_manager, + plugin_manager._registered_prefixes) = saved From 0398c55fb5eb9ddec48cefdb6829c5fb2f8652b4 Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Mon, 24 Aug 2026 11:52:48 -0400 Subject: [PATCH 10/13] Warranty search finds a machine number; the collector can no longer set a printer assignment --- CHANGELOG.md | 21 ++ plugins/computers/plugin.py | 199 ++++-------------- .../frontend/views/WarrantiesList.vue | 18 +- .../warranty/frontend/views/warrantySearch.js | 44 ++++ .../frontend/views/warrantySearch.spec.js | 79 +++++++ tests/test_core/test_collector_printers.py | 174 ++++++--------- 6 files changed, 263 insertions(+), 272 deletions(-) create mode 100644 plugins/warranty/frontend/views/warrantySearch.js create mode 100644 plugins/warranty/frontend/views/warrantySearch.spec.js diff --git a/CHANGELOG.md b/CHANGELOG.md index c3c714d0..2d0b1566 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,13 @@ 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. +- **The warranty list could not be searched by machine number.** The number was + in the payload and rendered in the table as a chip, but the search box looked + only at vendor, service level, service tag and asset number. That made the + identifier the feature was built around - a shopfloor PC is warranted as a PC + but found by the machine it drives - the one thing the box could not find. + Machine number and machine name are now searched too, and the placeholder + names them. - **`flask plugin upgrade-all` failed a plugin whose tables already existed** ("table securesendsecrets already exists" on the tools plugin). Every post-cutover baseline emitted a plain `op.create_table`, which is correct only @@ -132,6 +139,20 @@ ADR-007 and ADR-002. ### Changed +- **The computers collector no longer accepts `defaultprinter` or `printers`, + and can no longer write a printer assignment.** Those keys used to become + `defaultprinter`/`connectedto` relationship rows straight from a report, which + contradicts the rule the whole printer feature rests on: a collector report is + OBSERVED state and never ASSIGNED state, or a bay that installed the wrong + printer makes itself right simply by reporting it. It was not a dormant code + path - the collector schema advertised both keys and the example client in + `docs/COLLECTOR-INTEGRATION.md` set `defaultprinter`, while another page of + the same document promised nothing on that path writes an assignment row. + **A client still sending them keeps working**: the keys are ignored, not + rejected, and the response carries a warning naming them and pointing at + `POST /api/collector/printers` for reporting queues and + `seed-from-observed` for adopting them. Silently dropping them would have left + an integrator believing assignments were still being written. - **Tech Tools no longer asks people to sign in for tools that need no account.** `/tools`, `/tools/codes` and `/tools/secure-send` were all behind `requiresAuth`. The label generator runs entirely in the browser - no API, no diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index 96f1e6b6..01bea5cd 100644 --- a/plugins/computers/plugin.py +++ b/plugins/computers/plugin.py @@ -16,16 +16,14 @@ from .api import computers_bp logger = logging.getLogger(__name__) -# Marker stamped on PC->printer links this collector creates. The stale-link -# archive only touches rows carrying this label, so manually-created printer -# relationships are never removed by a collector push. Stored in the -# assetrelationships.label column (no origin column exists; see BUILD notes). -PRINTER_LINK_ORIGIN = 'collector:printers' - # Marker stamped on PC->measuringtool "controls" links this collector creates -# for metrology PCs (CMM, Keyence, Genspect, wax-and-trace). Same stale-link -# discipline as PRINTER_LINK_ORIGIN: only rows carrying this label are archived -# by a collector push, so hand-made tool links survive. +# for metrology PCs (CMM, Keyence, Genspect, wax-and-trace). Only rows carrying +# this label are archived by a collector push, so hand-made tool links survive. +# Stored in assetrelationships.label (no origin column exists; see BUILD notes). +# +# There is deliberately no printer equivalent any more. PRINTER_LINK_ORIGIN went +# with _sync_printer_links: printer ASSIGNMENT is never collected, so there are +# no collector-owned printer rows left to label or archive. MEASURINGTOOL_LINK_ORIGIN = 'collector:measuringtool' # Marker stamped on the PC->machine "controls" link built from the reported @@ -139,26 +137,10 @@ class ComputersPlugin(BasePlugin): 'type': 'array', 'items': {'name': 'string', 'version': 'string'}, }, - # Printer identifiers reported by the GE-Enforce side, which - # runs Get-CimInstance Win32_Printer and marks the default with - # the Default flag. An identifier is the printer's windows name, - # share name, hostname, or port/IP; the collector resolves it - # flexibly to a printer asset. Both optional. Presence of either - # key drives the PC->printer relationship sync (and stale-link - # archive); absence leaves existing printer links untouched. - 'defaultprinter': { - 'type': 'string', - 'description': ('Default printer identifier (Win32_Printer ' - 'with Default=true): windows name, share ' - 'name, hostname, or port IP.'), - }, - 'printers': { - 'type': 'array', - 'items': {'type': 'string'}, - 'description': ('All installed network printer identifiers ' - '(Win32_Printer): windows name / share / ' - 'hostname / IP. Unresolved -> warning.'), - }, + # RETIRED: 'defaultprinter' and 'printers' used to live here and + # wrote PC->printer assignment rows from a report. That is the + # one thing this path must never do - see _RETIRED_PRINTER_KEYS. + # A client still sending them is warned, not rejected. 'accessprotocols': { 'type': 'array', 'items': {'type': 'string'}, @@ -423,8 +405,9 @@ class ComputersPlugin(BasePlugin): else: machinelinks = [] - # Printer relationship sync (only when the payload carried printer data). - printerlinks = self._sync_printer_links(comp.asset, payload, warnings) + # Printer assignment is NOT collected. See _warn_retired_printer_keys. + self._warn_retired_printer_keys(payload, warnings) + printerlinks = [] # Measuring-tool sync: metrology PCs (CMM/Keyence/Genspect/wax-trace) # get an attached MeasuringTool asset auto-created and linked. @@ -518,134 +501,38 @@ class ComputersPlugin(BasePlugin): AccessProtocol.protocolid.in_(wanted)).all()] if wanted else [] return sorted(names) - def _sync_printer_links(self, pcasset, payload, warnings): - """Idempotently sync PC->printer relationships from collector printer data. + def _warn_retired_printer_keys(self, payload, warnings): + """Refuse to turn a report into an assignment, out loud. - Default printer -> 'defaultprinter' (directional). Other reported - printers -> 'connectedto' (symmetric). Resolves each identifier to a - printer asset by windows name / share / hostname / asset number / name - or a communications IP. Unresolved identifiers add a warning and never - fail the push. + RETIRED 2026-08-24. This used to be _sync_printer_links, which wrote + 'defaultprinter' and 'connectedto' rows straight from a collector + payload. That contradicted the rule the whole printer feature rests on: + ASSIGNED and OBSERVED never mix. A bay that installed the wrong printer + would have made itself right simply by reporting it, and enforcement + would stop meaning anything. - Stale-link archive: on each push, collector-tagged links (label == - PRINTER_LINK_ORIGIN) whose (target, type) pair is not in the reported - desired set are set inactive. Only tagged rows are touched, so manual - links survive. Runs only when the payload carried a printer key - ('defaultprinter' or 'printers'); a PC that reports without printer data - keeps its existing links. Returns the desired-link list (created + kept). + Worse than dormant, it was ADVERTISED: the collector schema listed both + keys and COLLECTOR-INTEGRATION.md's example client set defaultprinter, + while the same document promised on another page that nothing on this + path writes an assignment row. The document contradicted itself and the + code sided with the wrong half. + + Observed queues have their own path (POST /api/collector/printers) and + become assignments only when a person asks, through + seed-from-observed after reviewing the comparison. + + WARNS RATHER THAN REJECTS. Unknown keys are not validated here, so a + client written against the old docs keeps pushing successfully; only its + printer keys stop doing anything. Silently ignoring them would leave an + integrator believing assignments were being written. """ - from shopdb.api import ( - AssetRelationship, RelationshipType, Asset, Communication) - - has_default = 'defaultprinter' in payload - has_list = 'printers' in payload - if not has_default and not has_list: - return [] - - try: - from plugins.printers.models import Printer - except ImportError: - warnings.append('printers plugin unavailable; printer links skipped') - return [] - - dp_type = RelationshipType.query.filter_by( - relationshiptype='defaultprinter').first() - ct_type = RelationshipType.query.filter_by( - relationshiptype='connectedto').first() - if not dp_type or not ct_type: - warnings.append('printer relationship types missing; ' - 'run flask seed reference-data') - return [] - - def resolve(identifier): - # first match wins: printer text identity, then a printer IP. - ident = (identifier or '').strip() - if not ident: - return None - printer = ( - Printer.query.join(Asset, Asset.assetid == Printer.assetid) - .filter(Asset.isactive == True) - .filter(db.or_( - Printer.windowsname.ilike(ident), - Printer.hostname.ilike(ident), - Printer.sharename.ilike(ident), - Asset.assetnumber.ilike(ident), - Asset.name.ilike(ident), - )).first()) - if printer: - return printer.asset - comm = ( - db.session.query(Communication) - .join(Printer, Printer.assetid == Communication.assetid) - .filter(Communication.ipaddress == ident) - .first()) - if comm: - return db.session.get(Asset, comm.assetid) - return None - - pcid = pcasset.assetid - desired = set() # (targetassetid, relationshiptypeid) to keep - printerlinks = [] - - default_id = None - default_ident = (payload.get('defaultprinter') or '').strip() - if default_ident: - target = resolve(default_ident) - if target: - default_id = target.assetid - desired.add((default_id, dp_type.relationshiptypeid)) - self._sync_one(pcid, default_id, dp_type) - printerlinks.append({'assetid': default_id, - 'relationshiptype': 'defaultprinter'}) - else: - warnings.append(f'unresolved default printer: {default_ident}') - - for ident in payload.get('printers') or []: - target = resolve(ident) - if not target: - warnings.append(f'unresolved printer: {ident}') - continue - if target.assetid == default_id: - continue # already the default link - desired.add((target.assetid, ct_type.relationshiptypeid)) - self._sync_one(pcid, target.assetid, ct_type) - printerlinks.append({'assetid': target.assetid, - 'relationshiptype': 'connectedto'}) - - # Archive collector-tagged links no longer reported (manual links, with - # a NULL/other label, are never matched here). - collector_rows = AssetRelationship.query.filter( - AssetRelationship.sourceassetid == pcid, - AssetRelationship.relationshiptypeid.in_( - [dp_type.relationshiptypeid, ct_type.relationshiptypeid]), - AssetRelationship.isactive == True, - AssetRelationship.label == PRINTER_LINK_ORIGIN, - ).all() - for rel in collector_rows: - if (rel.targetassetid, rel.relationshiptypeid) not in desired: - rel.isactive = False - - return printerlinks - - def _sync_one(self, pcid, printerid, reltype): - """Reactivate or create one collector PC->printer link (idempotent).""" - from shopdb.api import AssetRelationship - existing = AssetRelationship.query.filter_by( - sourceassetid=pcid, targetassetid=printerid, - relationshiptypeid=reltype.relationshiptypeid).first() - if existing: - # do not re-stamp label: a pre-existing manual row stays manual. - if not existing.isactive: - existing.isactive = True - return existing - rel = AssetRelationship( - sourceassetid=pcid, targetassetid=printerid, - relationshiptypeid=reltype.relationshiptypeid, - label=PRINTER_LINK_ORIGIN) - db.session.add(rel) - return rel - - # -- measuring-tool sync ----------------------------------------------- + sent = [key for key in ('defaultprinter', 'printers') if key in payload] + if sent: + warnings.append( + '%s ignored: a collector report never sets printer assignment. ' + 'Report queues to POST /api/collector/printers, then adopt them ' + 'with POST /api/printers/assignments/seed-from-observed.' + % ' and '.join(sent)) def _sync_machine_link(self, comp, machinenumber, warnings): """Link a PC to the machine it drives, from the reported machine number. diff --git a/plugins/warranty/frontend/views/WarrantiesList.vue b/plugins/warranty/frontend/views/WarrantiesList.vue index e208cc04..1913dd92 100644 --- a/plugins/warranty/frontend/views/WarrantiesList.vue +++ b/plugins/warranty/frontend/views/WarrantiesList.vue @@ -24,7 +24,7 @@ - + {{ filteredItems.length }} of {{ items.length }} @@ -186,6 +186,7 @@ import api, { warrantyApi, assetsApi, vendorsApi } from '@/api' import { useToast } from '@/composables/toast' import { apiError } from '@/utils/apiError' import MachineMapChip from '../components/MachineMapChip.vue' +import { matchesWarrantySearch } from './warrantySearch' const toast = useToast() const route = useRoute() @@ -197,16 +198,11 @@ const search = ref('') const page = ref(1) const perPage = 25 -// Client-side search across vendor / service level / tag / linked asset numbers. -const filteredItems = computed(() => { - const term = search.value.trim().toLowerCase() - if (!term) return items.value - return items.value.filter(w => - (w.vendor || '').toLowerCase().includes(term) || - (w.servicelevel || '').toLowerCase().includes(term) || - (w.servicetag || '').toLowerCase().includes(term) || - (w.assets || []).some(a => (a.assetnumber || '').toLowerCase().includes(term))) -}) +// Client-side search. The field list lives in warrantySearch.js, where it is +// covered by tests - the machine number was rendered in this table for a long +// time before anything let you search for it. +const filteredItems = computed(() => + items.value.filter(w => matchesWarrantySearch(w, search.value))) const totalPages = computed(() => Math.max(1, Math.ceil(filteredItems.value.length / perPage))) const paginatedItems = computed(() => filteredItems.value.slice((page.value - 1) * perPage, page.value * perPage)) diff --git a/plugins/warranty/frontend/views/warrantySearch.js b/plugins/warranty/frontend/views/warrantySearch.js new file mode 100644 index 00000000..8579e299 --- /dev/null +++ b/plugins/warranty/frontend/views/warrantySearch.js @@ -0,0 +1,44 @@ +// What the warranty list's search box matches against. +// +// Pulled out of WarrantiesList.vue so the field list is somewhere a test can +// see it. It went wrong the way a field list usually does: silently, by a +// column being added to the table and not to the filter. +// +// THE MACHINE NUMBER IS THE POINT. _related_machine in the warranty API exists +// precisely because "a shopfloor PC is bought, warranted and replaced as a PC, +// but it is FOUND by the machine it drives - nobody walks the floor looking for +// an asset number". The payload carries it, the table renders it in a chip, and +// the filter did not look at it - so the one identifier the feature was built +// around was the one thing the search box could not find. + +/** + * Does this warranty match the search term? + * + * @param {object} warranty - a row from GET /api/warranties + * @param {string} term - the raw search box contents + * @returns {boolean} true when the term is empty or any searched field contains it + */ +export function matchesWarrantySearch(warranty, term) { + const needle = (term || '').trim().toLowerCase() + if (!needle) return true + if (!warranty) return false + + const has = (value) => String(value ?? '').toLowerCase().includes(needle) + + if (has(warranty.vendor) || has(warranty.servicelevel) || has(warranty.servicetag)) { + return true + } + + return (warranty.assets || []).some((asset) => { + if (has(asset?.assetnumber)) return true + // The related machine is optional: a warranty on something that drives no + // machine has none, and a machine that was never linked leaves it null. + const machine = asset?.machine + return !!machine && (has(machine.machinenumber) || has(machine.name)) + }) +} + +/** The fields above, in the order the placeholder should name them. */ +export const WARRANTY_SEARCH_FIELDS = [ + 'vendor', 'service level', 'tag', 'asset number', 'machine', +] diff --git a/plugins/warranty/frontend/views/warrantySearch.spec.js b/plugins/warranty/frontend/views/warrantySearch.spec.js new file mode 100644 index 00000000..fc2e17cd --- /dev/null +++ b/plugins/warranty/frontend/views/warrantySearch.spec.js @@ -0,0 +1,79 @@ +import { describe, it, expect } from 'vitest' + +import { matchesWarrantySearch } from './warrantySearch' + +// Reported: the warranty page could not be searched by machine number. +// +// The number was in the payload (_related_machine puts it there) and rendered +// in the table as a chip, but the filter looked only at vendor, service level, +// service tag and asset number. So the identifier the feature was built around +// - a PC is warranted as a PC but FOUND by the machine it drives - was the one +// thing the box could not find. + +function warranty(overrides = {}) { + return { + vendor: 'Dell', + servicelevel: 'ProSupport', + servicetag: 'ABC1234', + assets: [{ + assetnumber: 'PC-00912', + name: 'Bay 3105 workstation', + machine: { machinenumber: '3016', name: 'Makino a61nx' }, + }], + ...overrides, + } +} + +describe('matchesWarrantySearch', () => { + it('matches on the machine number', () => { + // The reported case. + expect(matchesWarrantySearch(warranty(), '3016')).toBe(true) + }) + + it('matches on the machine name', () => { + expect(matchesWarrantySearch(warranty(), 'makino')).toBe(true) + }) + + it('still matches the fields it always did', () => { + expect(matchesWarrantySearch(warranty(), 'dell')).toBe(true) + expect(matchesWarrantySearch(warranty(), 'prosupport')).toBe(true) + expect(matchesWarrantySearch(warranty(), 'abc1234')).toBe(true) + expect(matchesWarrantySearch(warranty(), 'pc-00912')).toBe(true) + }) + + it('is case insensitive and ignores surrounding space', () => { + expect(matchesWarrantySearch(warranty(), ' MAKINO ')).toBe(true) + }) + + it('does not match an unrelated term', () => { + expect(matchesWarrantySearch(warranty(), '9999')).toBe(false) + }) + + it('keeps every row when the term is empty', () => { + expect(matchesWarrantySearch(warranty(), '')).toBe(true) + expect(matchesWarrantySearch(warranty(), ' ')).toBe(true) + expect(matchesWarrantySearch(warranty(), undefined)).toBe(true) + }) + + it('handles a warranty whose asset drives no machine', () => { + // machine is null whenever the covered asset has no controls link - a + // printer, or a PC nobody linked yet. It must not throw. + const row = warranty({ assets: [{ assetnumber: 'PR-42', machine: null }] }) + expect(matchesWarrantySearch(row, 'PR-42')).toBe(true) + expect(matchesWarrantySearch(row, '3016')).toBe(false) + }) + + it('handles a warranty with no linked assets at all', () => { + const row = warranty({ assets: [] }) + expect(matchesWarrantySearch(row, 'dell')).toBe(true) + expect(matchesWarrantySearch(row, '3016')).toBe(false) + }) + + it('tolerates missing fields rather than matching everything', () => { + // String(undefined) is 'undefined', which would make a search for 'und' + // match every row with an empty field. + const row = { vendor: null, servicelevel: undefined, assets: null } + expect(matchesWarrantySearch(row, 'und')).toBe(false) + expect(matchesWarrantySearch(row, 'null')).toBe(false) + }) +}) diff --git a/tests/test_core/test_collector_printers.py b/tests/test_core/test_collector_printers.py index ac48bd93..576b039a 100644 --- a/tests/test_core/test_collector_printers.py +++ b/tests/test_core/test_collector_printers.py @@ -1,11 +1,19 @@ -"""Tests for the computers collector PC->printer relationship sync. +"""The computers collector must NEVER write a printer assignment. -A collector payload carrying defaultprinter/printers creates AssetRelationship -rows linking the PC asset to the resolved printer assets ('defaultprinter' for -the default, 'connectedto' for the rest). Stale collector links archive on -re-report; manual links survive; unresolved identifiers warn without failing. +This file used to pin the opposite: `_sync_printer_links` turned a payload's +`defaultprinter`/`printers` into AssetRelationship rows. That contradicted the +rule the printer feature rests on - ASSIGNED and OBSERVED never mix - because a +bay that installed the wrong printer would have made itself right just by +reporting it. + +It was not dormant. The collector schema advertised both keys and +COLLECTOR-INTEGRATION.md's example client set `defaultprinter`, while another +page of the same document promised nothing on this path writes an assignment +row. Retired 2026-08-24; these tests are the guard that it stays retired. + +Observed queues have their own path (POST /api/collector/printers) and become +assignments only when a person asks, via seed-from-observed. """ - import pytest from shopdb.core.models import ( @@ -81,119 +89,75 @@ def _active_links(db, pcid): sourceassetid=pcid, isactive=True).all() -def test_creates_default_and_connected_links(client, db, collector_key, scene): - """Default -> defaultprinter, others -> connectedto, resolved by three id forms.""" - resp = _post(client, { - 'hostname': 'WJPC-P1', - 'defaultprinter': 'HP-Materials', # windowsname - 'printers': ['prn-front.wjs.local', '10.1.2.3'], # hostname, IP - }) - assert resp.status_code == 200, resp.get_json() - data = resp.get_json()['data'] - assert data['printerlinkcount'] == 3 - assert not data['warnings'] - - pc = _pc_asset(db) - links = {(r.targetassetid, r.relationshiptype.relationshiptype) - for r in _active_links(db, pc.assetid)} - assert (scene['a'].assetid, 'defaultprinter') in links - assert (scene['b'].assetid, 'connectedto') in links - assert (scene['c'].assetid, 'connectedto') in links - - -def test_unresolved_identifier_warns_not_fails(client, db, collector_key, scene): - """An unknown printer identifier is a warning, the rest still link.""" - resp = _post(client, { - 'hostname': 'WJPC-P2', +def test_printer_keys_create_no_relationships(client, db, collector_key, scene): + """The reported case, inverted: sending them must change nothing.""" + response = _post(client, { + 'hostname': 'BAY-3105-PC', 'defaultprinter': 'HP-Materials', - 'printers': ['does-not-exist'], + 'printers': ['HP-Materials', 'prn-front.wjs.local', '10.1.2.3'], }) - assert resp.status_code == 200 - data = resp.get_json()['data'] - assert any('unresolved printer: does-not-exist' in w - for w in data['warnings']) - pc = _pc_asset(db) - assert data['printerlinkcount'] == 1 - assert len(_active_links(db, pc.assetid)) == 1 + assert response.status_code in (200, 201), response.get_json() + + pcid = _pc_asset(db).assetid + assert _active_links(db, pcid) == [], ( + 'a collector report wrote printer relationships') -def test_idempotent_no_duplicates(client, db, collector_key, scene): - """Re-report the same set: no new rows, still one link per printer.""" - payload = {'hostname': 'WJPC-P3', 'defaultprinter': 'HP-Materials', - 'printers': ['Zebra-Ship']} - assert _post(client, payload).status_code == 200 - assert _post(client, payload).status_code == 200 +def test_the_push_still_succeeds_and_says_why(client, db, collector_key, scene): + """Warn, do not reject. - pc = _pc_asset(db) - all_rows = AssetRelationship.query.filter_by(sourceassetid=pc.assetid).all() - assert len(all_rows) == 2 # one default + one connected, no dupes - assert all(r.isactive for r in all_rows) + Unknown keys are not validated, so a client written against the old docs + keeps pushing successfully - only its printer keys stop doing anything. + Silently dropping them would leave an integrator believing assignments were + still being written, which is worse than the original bug. + """ + response = _post(client, { + 'hostname': 'BAY-3105-PC', + 'defaultprinter': 'HP-Materials', + }) + assert response.status_code in (200, 201) + warnings = ' '.join(response.get_json()['data']['warnings']).lower() + assert 'defaultprinter' in warnings + assert 'seed-from-observed' in warnings, ( + 'the warning should say where assignment actually comes from') -def test_dropped_printer_is_archived(client, db, collector_key, scene): - """A printer that stops being reported loses its collector link.""" - _post(client, {'hostname': 'WJPC-P4', 'defaultprinter': 'HP-Materials', - 'printers': ['Zebra-Ship']}) - pc = _pc_asset(db) - assert len(_active_links(db, pc.assetid)) == 2 - - # Re-report without Zebra-Ship. - _post(client, {'hostname': 'WJPC-P4', 'defaultprinter': 'HP-Materials', - 'printers': []}) - active = _active_links(db, pc.assetid) - assert len(active) == 1 - assert active[0].targetassetid == scene['a'].assetid - # the dropped row still exists but is inactive - zebra = AssetRelationship.query.filter_by( - sourceassetid=pc.assetid, targetassetid=scene['c'].assetid).first() - assert zebra is not None and zebra.isactive is False +def test_a_payload_without_printer_keys_warns_about_nothing( + client, db, collector_key, scene): + """The normal case must stay quiet - this is every push in the fleet.""" + response = _post(client, {'hostname': 'BAY-3105-PC'}) + assert response.status_code in (200, 201) + warnings = ' '.join(response.get_json()['data']['warnings']).lower() + assert 'printer' not in warnings -def test_default_change_archives_old_default(client, db, collector_key, scene): - """Changing the default printer archives the previous defaultprinter link.""" - _post(client, {'hostname': 'WJPC-P5', 'defaultprinter': 'HP-Materials'}) - pc = _pc_asset(db) - _post(client, {'hostname': 'WJPC-P5', 'defaultprinter': 'prn-front.wjs.local'}) - - active = _active_links(db, pc.assetid) - assert len(active) == 1 - assert active[0].targetassetid == scene['b'].assetid - assert active[0].relationshiptype.relationshiptype == 'defaultprinter' - - -def test_manual_link_survives_collector_sync(client, db, collector_key, scene): - """A manually-created link (no collector label) is never archived.""" - # PC created first via a bare report. - _post(client, {'hostname': 'WJPC-P6', 'defaultprinter': 'HP-Materials', - 'printers': ['Zebra-Ship']}) - pc = _pc_asset(db) - - # Manual connectedto link PC -> printer B (label stays NULL). +def test_a_manual_assignment_survives_a_report_that_names_printers( + client, db, collector_key, scene): + """The row a person made is the one that matters, and it stays untouched.""" + _post(client, {'hostname': 'BAY-3105-PC'}) + pcasset = _pc_asset(db) manual = AssetRelationship( - sourceassetid=pc.assetid, targetassetid=scene['b'].assetid, - relationshiptypeid=scene['ct_type'].relationshiptypeid, notes='by hand') + sourceassetid=pcasset.assetid, targetassetid=scene['a'].assetid, + relationshiptypeid=scene['dp_type'].relationshiptypeid) db.session.add(manual) db.session.commit() - manual_id = manual.relationshipid - # Re-report dropping Zebra-Ship and never mentioning printer B. - _post(client, {'hostname': 'WJPC-P6', 'defaultprinter': 'HP-Materials', - 'printers': []}) + _post(client, { + 'hostname': 'BAY-3105-PC', + 'defaultprinter': 'prn-front.wjs.local', # a DIFFERENT printer + 'printers': ['prn-front.wjs.local'], + }) - surviving = db.session.get(AssetRelationship, manual_id) - assert surviving.isactive is True # manual link untouched - # Zebra (collector) archived. - zebra = AssetRelationship.query.filter_by( - sourceassetid=pc.assetid, targetassetid=scene['c'].assetid).first() - assert zebra.isactive is False + links = _active_links(db, pcasset.assetid) + assert len(links) == 1 + assert links[0].targetassetid == scene['a'].assetid, ( + 'the report retargeted a human-made default printer') -def test_no_printer_keys_leaves_links_untouched(client, db, collector_key, scene): - """A report with no printer keys does not archive existing links.""" - _post(client, {'hostname': 'WJPC-P7', 'defaultprinter': 'HP-Materials'}) - pc = _pc_asset(db) - assert len(_active_links(db, pc.assetid)) == 1 +def test_the_retired_keys_are_gone_from_the_collector_schema(app): + """The schema advertised them, which is how integrators found them.""" + from plugins.computers.plugin import ComputersPlugin - # A plain update (no defaultprinter, no printers key) must not archive. - _post(client, {'hostname': 'WJPC-P7', 'currentuser': 'alice'}) - assert len(_active_links(db, pc.assetid)) == 1 + fields = ComputersPlugin().get_collector_schema()['fields'] + assert 'defaultprinter' not in fields + assert 'printers' not in fields From ead9fd6b5641a535d1731a72092430d845ad271f Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Mon, 24 Aug 2026 14:06:14 -0400 Subject: [PATCH 11/13] Search results open the record they found; plugin scaffolder emits a prunable frontend --- CHANGELOG.md | 21 ++ shopdb/core/api/search.py | 9 +- shopdb/plugins/scaffolder.py | 52 +-- .../plugins/templates/frontend/routes.js.tmpl | 32 +- .../templates/frontend/views/Detail.vue.tmpl | 6 +- .../templates/frontend/views/Form.vue.tmpl | 4 +- .../templates/frontend/views/List.vue.tmpl | 6 +- tests/test_core/test_search_coverage.py | 67 ++++ tests/test_plugin_scaffold_frontend.py | 318 +++++++++--------- 9 files changed, 312 insertions(+), 203 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d0b1566..2e0437ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,27 @@ ADR-007 and ADR-002. ### Fixed +- **A search result opened the list it sits in, not the record it found.** A + NOTICE linked to `/notifications`, so clicking the row search had already + identified sent the reader to the list page to find it again by hand. The id + was in the payload the whole time and only the URL ignored it. The subnet + searcher had the same defect: an IP search resolves which subnet an address + falls inside, then linked to the `/network` hub rather than that subnet. + Both now link to the record. Every other result type already carried its id. + +- **`flask plugin new` scaffolded a frontend that no lean build could prune.** + It wrote views to `frontend/src/views//` and a route file to + `frontend/src/router/routes/.js`, while all 16 bundled plugins keep + their frontend in `plugins//frontend/`, staged by + `stage-frontend.mjs`. Nothing looked broken, which is why it lasted: the + router still globs `./routes/*.js` during the Phase 4 dual-location + transition, so the pages loaded. What a new plugin author silently lost was + ADR-013 lean builds, since pruning works off the staged tree, plus stranded + views on deleting the plugin. The templates for the right layout already + existed; only the output paths were wrong. Their core imports moved to the + `@` alias, which resolves the same before and after staging, where the + previous `../../api` was correct only in the old location. + - **A zero bar height printed a barcode 176 wide and 0 high.** Nothing rejected it: the value went straight into a CSS length, `height: 0in` is perfectly valid, and an image at `height: 100%` of a zero-height box is zero pixels diff --git a/shopdb/core/api/search.py b/shopdb/core/api/search.py index a308bc5b..d023304c 100644 --- a/shopdb/core/api/search.py +++ b/shopdb/core/api/search.py @@ -658,7 +658,10 @@ def _search_subnets(query): 'id': subnet.subnetid, 'title': f'{subnet.name} ({subnet.cidr})', 'subtitle': subnet.description or subnet.subnettype, - 'url': f'/network', + # the matched subnet, not the hub: searching an IP + # already identified WHICH subnet, so landing on the + # hub makes the reader find it again by hand + 'url': f'/networks/{subnet.subnetid}', 'relevance': 70 }) except ValueError: @@ -779,7 +782,9 @@ def _search_notifications(query, search_term): 'id': notif.notificationid, 'title': notif.title, 'subtitle': notif.notificationtype.typename if notif.notificationtype else None, - 'url': f'/notifications', + # the notification itself, not the list it sits in: the + # `notification-detail` route renders one by id + 'url': f'/notifications/{notif.notificationid}', 'relevance': min(int(base_relevance), 100), 'ticketnumber': notif.ticketnumber, 'iscurrent': notif.is_current diff --git a/shopdb/plugins/scaffolder.py b/shopdb/plugins/scaffolder.py index ea6eebe7..acb70434 100644 --- a/shopdb/plugins/scaffolder.py +++ b/shopdb/plugins/scaffolder.py @@ -81,18 +81,28 @@ def _scaffold_frontend( name: str, substitutions: dict, plugin_target: Path, - frontend_dir: Path, template_root: Path, overwrite: bool, ) -> None: """Render the frontend starting points for a scaffolded plugin. - Writes the paste-in api-client snippet into the plugin directory, then the - Vue views and the router route file into the real frontend tree. The - snippet is emitted regardless of whether the frontend tree exists, because - it is a plugin-directory artifact useful even for external-repo plugins. - Views and the route file are skipped when frontend_dir is missing, which is - the normal case for a plugin developed in its own repository. + Everything lands INSIDE the plugin, under `plugins//frontend/`: + `routes.js` plus `views/{List,Detail,Form}.vue`, and the paste-in + api-client snippet at the plugin root. + + The plugin owns its frontend because that is what makes it prunable. + scripts/stage-frontend.mjs copies each plugin's `frontend/` into + `frontend/src/.plugins-staged//` and aggregates the route modules + into `routes.gen.js`; a plugin left out of SITE_PLUGINS is simply not + copied, so its frontend leaves the build with it (ADR-013). A route file + written into the CORE tree instead is still picked up - the router globs + `./routes/*.js` during the Phase 4 dual-location transition - but it is + invisible to that pruning, and deleting the plugin strands its views. + + This is also why nothing here depends on the core frontend tree existing. + A plugin developed in its own repo gets exactly the same layout, and its + frontend starts working the moment the plugin directory is dropped into + `plugins/`. Ordering matters: every view is written before the route file. A route file that lazy-imports a view that is not on disk crashes the Vite dev server, @@ -104,7 +114,7 @@ def _scaffold_frontend( plugin_name = substitutions['Name'] - # snippet lands in the plugin dir; author pastes it into api/index.js + # snippet lands at the plugin root; author pastes it into api/index.js snippet_template = fe_templates / 'frontend-api-snippet.js.tmpl' if snippet_template.exists(): _render_template( @@ -114,11 +124,8 @@ def _scaffold_frontend( overwrite, ) - # views and route need the real frontend tree; external repos skip these - if not frontend_dir.exists(): - return - - views_dir = frontend_dir / 'views' / name + frontend_dir = plugin_target / 'frontend' + views_dir = frontend_dir / 'views' # views first: route file lazy-imports them, missing views 500 vite view_templates = { @@ -141,7 +148,7 @@ def _scaffold_frontend( if route_template.exists(): _render_template( route_template, - frontend_dir / 'router' / 'routes' / f'{name}.js', + frontend_dir / 'routes.js', substitutions, overwrite, ) @@ -154,7 +161,6 @@ def scaffold_plugin( template_root: Optional[Path] = None, overwrite: bool = False, frontend: bool = True, - frontend_dir: Optional[Path] = None, ) -> Path: """Generate a new plugin from templates. @@ -166,11 +172,10 @@ def scaffold_plugin( overwrite: If True, overwrite an existing plugin directory and any existing generated frontend files frontend: If True, also render the Vue frontend starting points (list, - detail, form views, a route file, and a paste-in api-client snippet) - frontend_dir: Frontend src directory to render views/routes into - (default: /../frontend/src). Views and the route file - are skipped when this directory does not exist, which is the normal - case for a plugin developed in its own repository. + detail, form views, a route file, and a paste-in api-client + snippet) into `plugins//frontend/`. There is no separate + output directory: the plugin owns its frontend so that + stage-frontend.mjs can stage it and a lean build can prune it. Returns: Path to the generated plugin directory. @@ -205,7 +210,9 @@ def scaffold_plugin( for template_path in template_root.rglob('*.tmpl'): rel = template_path.relative_to(template_root) - # frontend templates render into the frontend tree, not the plugin dir + # frontend templates are handled by _scaffold_frontend: they are + # renamed on the way out (List.vue.tmpl -> List.vue) and are + # skipped entirely when frontend=False if rel.parts and rel.parts[0] == 'frontend': continue @@ -222,13 +229,10 @@ def scaffold_plugin( out_path.write_text(rendered) if frontend: - if frontend_dir is None: - frontend_dir = plugins_dir.parent / 'frontend' / 'src' _scaffold_frontend( name=name, substitutions=substitutions, plugin_target=target, - frontend_dir=Path(frontend_dir), template_root=template_root, overwrite=overwrite, ) diff --git a/shopdb/plugins/templates/frontend/routes.js.tmpl b/shopdb/plugins/templates/frontend/routes.js.tmpl index 758b6f70..1616ee33 100644 --- a/shopdb/plugins/templates/frontend/routes.js.tmpl +++ b/shopdb/plugins/templates/frontend/routes.js.tmpl @@ -1,35 +1,49 @@ /** * ${Name} plugin routes. * - * Auto-discovered by the router via import.meta.glob, so no registration - * edit is needed. Every route carries meta.plugin '${name}' so the ADR-009 - * guard redirects to the dashboard when the ${name} backend plugin is - * disabled. Form routes add requiresAuth so anonymous users cannot reach - * create or edit. + * This file lives in the plugin, not in the core frontend tree. + * scripts/stage-frontend.mjs copies plugins/${name}/frontend/ into + * frontend/src/.plugins-staged/${name}/ and aggregates this default export + * into routes.gen.js, which the router spreads in - so no registration edit + * is needed, and pruning the plugin for a lean build removes its frontend + * with it (ADR-013). + * + * Every route carries meta.plugin '${name}' so the ADR-009 guard redirects to + * the dashboard when the ${name} backend plugin is disabled. Form routes add + * requiresAuth so anonymous users cannot reach create or edit. + * + * View imports are relative (./views/...) because they travel with this file. + * Anything from the CORE tree uses the @ alias (@/api, @/components/...), + * which resolves the same before and after staging. */ export default [ { path: '${name}', name: '${name}', - component: () => import('../../views/${name}/${Name}List.vue'), + component: () => import('./views/${Name}List.vue'), meta: { plugin: '${name}' } }, { path: '${name}/new', name: '${name}-new', - component: () => import('../../views/${name}/${Name}Form.vue'), + component: () => import('./views/${Name}Form.vue'), meta: { requiresAuth: true, plugin: '${name}' } }, { path: '${name}/:id', name: '${name}-detail', - component: () => import('../../views/${name}/${Name}Detail.vue'), + component: () => import('./views/${Name}Detail.vue'), meta: { plugin: '${name}' } }, { path: '${name}/:id/edit', name: '${name}-edit', - component: () => import('../../views/${name}/${Name}Form.vue'), + component: () => import('./views/${Name}Form.vue'), meta: { requiresAuth: true, plugin: '${name}' } } ] + +// Full-screen routes that render OUTSIDE AppLayout (kiosk, print pages). +// Declared even when empty so routes.gen.js can read `.toplevel` off the +// module without Rollup warning about a missing export. +export const toplevel = [] diff --git a/shopdb/plugins/templates/frontend/views/Detail.vue.tmpl b/shopdb/plugins/templates/frontend/views/Detail.vue.tmpl index 9777a799..b1dc5ec1 100644 --- a/shopdb/plugins/templates/frontend/views/Detail.vue.tmpl +++ b/shopdb/plugins/templates/frontend/views/Detail.vue.tmpl @@ -78,12 +78,12 @@