ge-aerospace/shopdb-flask
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Make a partly-undone import retryable, and refuse a plan it cannot deliver

Browse Source
This commit is contained in:
Cameron Proudlock
2026-09-03 09:10:13 -04:00
parent 8ad6511f14
commit cf95daf3bb
20 changed files with 588 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
frontend/src/assets/style.css
View File

@@ -1323,13 +1323,33 @@ td.actions .btn + .btn {
gap: 0.75rem;
margin-top: 0.4rem;
}
/* A branding preview has to show a logo whose colour we do not control. The
shipped GE mark is fill="white" because it lives on the dark sidebar, so on a
plain white tile it rendered white-on-white and read as a failed load. A
checker ground makes any logo visible - light, dark or transparent - and is
the usual convention for previewing artwork with alpha. */
.map-thumb {
height: 40px;
min-width: 40px;
border: 1px solid var(--border);
border-radius: 4px;
background: #fff;
/* MID-TONE, not white and not near-white. The tile has to show artwork whose
colour we do not control: the shipped GE mark is fill="white" for the dark
sidebar, the monogram is fill="black". A light ground hides the first, a
dark ground hides the second, so the checker sits either side of mid grey
and both read against it. */
background-color: #9aa1ad;
background-image:
linear-gradient(45deg, #6f7783 25%, transparent 25%, transparent 75%, #6f7783 75%),
linear-gradient(45deg, #6f7783 25%, transparent 25%, transparent 75%, #6f7783 75%);
background-size: 12px 12px;
background-position: 0 0, 6px 6px;
padding: 2px;
}
.map-thumb-dark {
background-color: #222;
background-image: none;
}
.map-thumb-dark { background: #222; }
.settings-success {
margin-top: 1rem;
padding: 0.75rem;

8
frontend/src/views/settings/BrandingSettings.vue
View File

@@ -24,7 +24,7 @@
>
<div class="map-upload-row">
<input type="file" :accept="logo.accept" @change="uploadLogo(logo.kind, logo.key, $event)" :disabled="brandingUploading" />
<img v-if="settings[logo.key]" :src="settings[logo.key]" class="map-thumb" :alt="logo.label" />
<img v-if="settings[logo.key]" :src="withBase(settings[logo.key])" class="map-thumb" :alt="logo.label" />
</div>
<small class="input-hint">{{ logo.hint }}</small>
</label>
@@ -132,6 +132,12 @@
<script setup>
import { onMounted } from 'vue'
import { useSystemSettings, brandingLogos } from '../../composables/systemSettings'
// The stored value is root-relative ('/ge-aerospace-logo.svg'). This app is
// mounted under a base path at every real deployment (/shopdb, /ops), so a raw
// src resolves against the SERVER root and 404s - which is what "error loading
// image" was. siteSettings.js already wraps every logo this way for the rest of
// the app; this preview was the one place that did not.
import { withBase } from '@/utils/basePath'
const {
settings, saving, brandingUploading, error, success,

22
frontend/src/views/settings/FloorMapSettings.vue
View File

@@ -26,6 +26,14 @@
<button class="btn btn-small" @click="startAddLevel(building)">Add level</button>
</div>
<!-- Scroll rather than squeeze: nine columns of inputs do not fit a
narrow window, and without a wrapper the table ran off the right of
the page with the cells crushed out of shape.
NOT the global .table-container - that also sets white-space: nowrap,
and the sizing hint below is a full sentence, so nowrap made the
table WIDER instead of letting the paragraph wrap. Scroll here, wrap
inside the cells. -->
<div class="level-table-scroll">
<table class="data-table">
<thead>
<tr>
@@ -46,7 +54,7 @@
<td>
<input
v-model="level.levelname"
class="form-control"
class="form-control level-name-input"
@blur="renameLevel(level)"
:disabled="saving"
/>
@@ -123,6 +131,7 @@
</tr>
</tbody>
</table>
</div>
</div>
<!-- Add building -->
@@ -437,6 +446,17 @@ async function remove(level) {
}
.mono { font-family: monospace; }
/* Wide enough that every column keeps a usable width; past this the wrapper
scrolls instead of the inputs collapsing to a few pixels. */
.level-table-scroll { overflow-x: auto; }
.data-table { min-width: 58rem; }
/* The sizing note is prose and must wrap; it is the reason the shared
table-container wrapper is wrong for this table. */
.data-table .input-hint { white-space: normal; display: block; max-width: 22rem; }
.data-table td, .data-table th { vertical-align: top; }
/* A bare form-control in a table cell collapses to whatever width is left over,
which put "Ground floor" in a box one character wide. */
.level-name-input { width: 11rem; }
.order-input { width: 4.5rem; }
.size-input { width: 6rem; }
.size-fields { display: flex; align-items: center; gap: 0.35rem; }

43
frontend/src/views/settings/ImportSiteAssets.vue
View File

@@ -269,7 +269,7 @@
below if needed.
</template>
<template v-else-if="wincReport.blocking">
{{ wincReport.rowcount }} row(s) in scope, but unresolved values
{{ wincReport.rowcount }} row(s) in scope, but the problems below
block the import - nothing was written. Resolve them and preview
again.
</template>
@@ -314,6 +314,29 @@
</label>
</div>
<!-- Blocking and NOT acknowledgeable, unlike needscreate above.
The packet blocks on these, so the page must name them or the
banner points at nothing the admin can see. -->
<div
v-if="wincReport.uncreatable && Object.keys(wincReport.uncreatable).length"
class="problems"
>
<h4>Rows this import cannot create (blocking)</h4>
<p class="muted">
The export needs these, but this import only ever creates business
units. Create them in Settings first, or alias the export values
to rows that already exist, then preview again.
</p>
<ul>
<li v-for="(items, group) in wincReport.uncreatable" :key="group">
<strong>{{ group }}:</strong>
<span v-for="(count, name) in items" :key="name">
"{{ name }}" ({{ count }}){{ ' ' }}
</span>
</li>
</ul>
</div>
<div v-if="wincReport.unresolved && Object.keys(wincReport.unresolved).length" class="problems">
<h4>Unresolved values (blocking)</h4>
<p class="muted">
@@ -354,15 +377,27 @@
<td>{{ b.createdat }}</td>
<td>{{ b.createdby || '-' }}</td>
<td>{{ b.rowcount }}</td>
<td>{{ b.status }}</td>
<td>
{{ b.status }}
<small v-if="b.status === 'partially-undone'" class="muted">
- some rows could not be put back
</small>
</td>
<td>
<!-- 'partially-undone' is retryable and MUST offer the
button: that status exists because an undo stopped at a
conflict, and retrying once the conflict is resolved is
the whole point. Testing only for 'applied' left the
backend willing and the button hidden. -->
<button
v-if="b.status === 'applied'"
v-if="b.status === 'applied' || b.status === 'partially-undone'"
class="btn btn-secondary btn-sm"
:disabled="wincUndoingId === b.batchid"
@click="undoWincBatch(b.batchid)"
>
{{ wincUndoingId === b.batchid ? 'Undoing...' : 'Undo' }}
{{ wincUndoingId === b.batchid
? 'Undoing...'
: (b.status === 'partially-undone' ? 'Retry undo' : 'Undo') }}
</button>
<span v-else class="muted">undone</span>
</td>

51
migrations/versions/7d38_widen_importbatch_status.py Normal file
View File

@@ -0,0 +1,51 @@
"""Give importbatches.status room for a value longer than 'undone'.
7d37 sized the column String(16), which fitted the two statuses it shipped with.
'partially-undone' - the status an undo writes when some entries reverted and
others hit conflicts - is exactly 16 characters. It fits, with nothing to spare:
the next status anyone adds, or a rename of this one, silently truncates on
MySQL in non-strict mode and errors in strict.
Widened to 32 rather than left at the exact fit, because a column that happens
to be wide enough is not the same as a column that is wide enough.
Revision ID: 7d38_widen_importbatch_status
Revises: 7d37_import_batches_and_journal
"""
from alembic import op
import sqlalchemy as sa
revision = '7d38_widen_importbatch_status'
down_revision = '7d37_import_batches_and_journal'
branch_labels = None
depends_on = None
def _has_table(bind, name):
return sa.inspect(bind).has_table(name)
def upgrade():
bind = op.get_bind()
if not _has_table(bind, 'importbatches'):
return
# SQLite cannot ALTER a column type; it also does not enforce VARCHAR
# length, so there is nothing to widen there.
if bind.dialect.name == 'sqlite':
return
op.alter_column('importbatches', 'status',
existing_type=sa.String(length=16),
type_=sa.String(length=32),
existing_nullable=False)
def downgrade():
bind = op.get_bind()
if not _has_table(bind, 'importbatches') or bind.dialect.name == 'sqlite':
return
op.alter_column('importbatches', 'status',
existing_type=sa.String(length=32),
type_=sa.String(length=16),
existing_nullable=False)

4
plugins/computers/frontend/views/ActionPicker.vue
View File

@@ -63,7 +63,7 @@
:placeholder="createkind === 'service' ? 'e.g. Spooler' : 'e.g. agent.exe'"
v-model="createname"
@keydown.enter.prevent="submitCreate"
@keydown.esc.prevent="cancelCreate"
@keydown.esc.prevent.stop="cancelCreate"
/>
<template v-if="createkind === 'process'">
<label class="action-create-label">
@@ -75,7 +75,7 @@
placeholder="e.g. C:\Program Files (x86)\Microsoft\Edge\Application\msedge.exe"
v-model="createlaunchpath"
@keydown.enter.prevent="submitCreate"
@keydown.esc.prevent="cancelCreate"
@keydown.esc.prevent.stop="cancelCreate"
/>
</template>
<div class="action-create-actions">

24
scripts/check-naming-and-style.sh
View File

@@ -35,12 +35,18 @@ EXCLUDES=(
--exclude-dir=.git
--exclude-dir=versions
--exclude-dir=staticdocs
# Local, gitignored scratch notes (see .gitignore devnotes/). Not product
# source, never committed, and free to hold pasted logs full of em-dashes
# and arrows - grep does not consult .gitignore, so exclude it explicitly.
--exclude-dir=devnotes
)
# Root-level gitignored scratch: review writeups and chat handoffs, free to hold
# pasted logs full of em-dashes and arrows. grep does not consult .gitignore, so
# it has to be dropped here.
#
# A POST-FILTER, not --exclude-dir=devnotes. That flag matches a directory of
# that NAME at any depth, so a force-added plugins/<plugin>/devnotes/ would be
# invisible to every check in this file, ADR-015 included - a hole in the gate
# that nothing would report. This drops only the path we mean.
drop_scratch() { grep -v '^\./devnotes/' || true; }
INCLUDES_CODE=(
--include='*.py'
--include='*.vue'
@@ -64,7 +70,7 @@ INCLUDES_ALL=(
# and did, including in files this repo's own convention forbids them in.
# Markdown, JSON and YAML are now covered too.
echo "==> Checking for non-ASCII characters..."
NON_ASCII=$(grep -rPn '[^\x00-\x7F]' "${EXCLUDES[@]}" "${INCLUDES_ALL[@]}" . 2>/dev/null || true)
NON_ASCII=$(grep -rPn '[^\x00-\x7F]' "${EXCLUDES[@]}" "${INCLUDES_ALL[@]}" . 2>/dev/null | drop_scratch || true)
if [ -n "$NON_ASCII" ]; then
echo "FAIL: non-ASCII characters found (em-dashes, smart quotes, arrows, emojis):"
echo "$NON_ASCII"
@@ -89,7 +95,7 @@ for word in cfg ctx mgr req res; do
done
echo "==> Checking for snake_case DB tablenames..."
SNAKE_TABLES=$(grep -rPn "__tablename__\s*=\s*['\"][^'\"]*_" "${EXCLUDES[@]}" --include='*.py' . 2>/dev/null || true)
SNAKE_TABLES=$(grep -rPn "__tablename__\s*=\s*['\"][^'\"]*_" "${EXCLUDES[@]}" --include='*.py' . 2>/dev/null | drop_scratch || true)
if [ -n "$SNAKE_TABLES" ]; then
echo "FAIL: snake_case __tablename__ found (must be lowercase concatenated):"
echo "$SNAKE_TABLES"
@@ -98,7 +104,7 @@ if [ -n "$SNAKE_TABLES" ]; then
fi
echo "==> Checking for snake_case DB column attrs..."
SNAKE_COLS=$(grep -rPn "^\s+[a-z]+_[a-z_]+\s*=\s*db\.Column" "${EXCLUDES[@]}" --include='*.py' . 2>/dev/null || true)
SNAKE_COLS=$(grep -rPn "^\s+[a-z]+_[a-z_]+\s*=\s*db\.Column" "${EXCLUDES[@]}" --include='*.py' . 2>/dev/null | drop_scratch || true)
if [ -n "$SNAKE_COLS" ]; then
echo "FAIL: snake_case db.Column attribute found (must match column name, no underscores):"
echo "$SNAKE_COLS"
@@ -107,7 +113,7 @@ if [ -n "$SNAKE_COLS" ]; then
fi
echo "==> Checking for snake_case ForeignKey targets..."
SNAKE_FK=$(grep -rPn "ForeignKey\(['\"][^'\"]*_[^'\"]*['\"]" "${EXCLUDES[@]}" --include='*.py' . 2>/dev/null || true)
SNAKE_FK=$(grep -rPn "ForeignKey\(['\"][^'\"]*_[^'\"]*['\"]" "${EXCLUDES[@]}" --include='*.py' . 2>/dev/null | drop_scratch || true)
if [ -n "$SNAKE_FK" ]; then
echo "FAIL: snake_case ForeignKey target found:"
echo "$SNAKE_FK"
@@ -116,7 +122,7 @@ if [ -n "$SNAKE_FK" ]; then
fi
echo "==> Checking for snake_case API params in frontend (DB-mirrored fields)..."
SNAKE_FE=$(grep -rPn "params\.(machine_id|location_id|vendor_id|type_id|business_unit_id|model_id|status_id|operating_system_id|asset_id|user_id|is_active|is_shopfloor)" "${EXCLUDES[@]}" --include='*.vue' --include='*.js' --include='*.ts' . 2>/dev/null || true)
SNAKE_FE=$(grep -rPn "params\.(machine_id|location_id|vendor_id|type_id|business_unit_id|model_id|status_id|operating_system_id|asset_id|user_id|is_active|is_shopfloor)" "${EXCLUDES[@]}" --include='*.vue' --include='*.js' --include='*.ts' . 2>/dev/null | drop_scratch || true)
if [ -n "$SNAKE_FE" ]; then
echo "FAIL: snake_case API params in frontend (must match DB column names without underscores):"
echo "$SNAKE_FE"

47
scripts/site_imports/approval.py
View File

@@ -57,7 +57,7 @@ def plan_hash(plan):
return hasher.hexdigest()
def build_packet(kind, plan, counts, groups):
def build_packet(kind, plan, counts, groups, creatable=None):
"""Build an approval packet from a plan.
kind a short label identifying the importer, stored on the packet.
@@ -69,6 +69,12 @@ def build_packet(kind, plan, counts, groups):
column is what a human would act on - the raw export value for
UNRESOLVED (so they know what to alias) and the canonical name for
NEEDS_CREATE (so they know what would be created).
creatable
the group names the caller's writer can actually create, or None
for "no opinion". A needs-create group outside this set is reported
under `uncreatable` and makes the packet blocking, because a plan
that promises rows nothing will create is as unrunnable as one with
unresolved values.
needscreate is keyed on the canonical name column when present; callers pass
the *_name column for that via the fourth tuple element when it differs from
@@ -86,6 +92,26 @@ def build_packet(kind, plan, counts, groups):
if needs_hits:
needscreate[group_name] = needs_hits
# A needs-create group the writer cannot actually create BLOCKS the plan,
# the same as an unresolved value. It is the same failure either way: the
# import cannot do what the preview says it would.
#
# It has to be decided HERE rather than at write time. The writer's own
# refusal sits behind the acknowledgement check, and the UI only ever
# acknowledges the groups it renders a control for - so a plan needing an
# uncreatable group reached Commit looking clean, and died on a message
# about acknowledging something the screen had no checkbox for. Blocking at
# plan time puts it in the preview, where the admin can act on it.
#
# creatable=None means "no opinion" - the CLI callers that do not pass it
# keep the old behaviour.
uncreatable = OrderedDict()
if creatable is not None:
allowed = set(creatable)
for group_name, hits in needscreate.items():
if group_name not in allowed:
uncreatable[group_name] = hits
return OrderedDict([
('kind', kind),
('batchid', str(uuid.uuid4())),
@@ -93,8 +119,9 @@ def build_packet(kind, plan, counts, groups):
('rowcount', len(plan)),
('counts', OrderedDict(counts)),
('needscreate', needscreate),
('uncreatable', uncreatable),
('unresolved', unresolved),
('blocking', bool(unresolved)),
('blocking', bool(unresolved) or bool(uncreatable)),
])
@@ -123,6 +150,20 @@ def format_packet(packet):
lines.append(' + %-28s (%d rows)' % (name, count))
lines.append(' total new %s rows: %d' % (group, total))
# An uncreatable group blocks, so it has to be ON SCREEN. Setting
# blocking without printing the reason left "BLOCKED (resolve the values
# above)" pointing at nothing.
uncreatable = packet.get('uncreatable') or {}
if uncreatable:
for group, items in uncreatable.items():
lines.append('')
lines.append(' BLOCKING: this import cannot create %s rows, but '
'%d are needed - create them first, or alias the '
'export values to rows that exist:'
% (group, len(items)))
for name, count in items.items():
lines.append(' x %-28s (%d rows)' % (name, count))
unresolved = packet.get('unresolved') or {}
if unresolved:
for group, items in unresolved.items():
@@ -134,7 +175,7 @@ def format_packet(packet):
lines.append('')
lines.append(' status : %s' % (
'BLOCKED (resolve the values above)' if packet.get('blocking')
'BLOCKED (resolve the problems above)' if packet.get('blocking')
else 'READY for acknowledgment'))
return '\n'.join(lines)

28
scripts/site_imports/journal.py
View File

@@ -34,6 +34,8 @@ from datetime import datetime, timezone
BATCH_APPLIED = 'applied'
BATCH_UNDONE = 'undone'
# See shopdb/core/models/importbatch.py for why this exists.
BATCH_PARTIAL = 'partially-undone'
OP_UPDATE = 'update'
OP_INSERT = 'insert'
@@ -150,6 +152,9 @@ def undo_batch(connection, batchid, references=None):
{'batchid': batchid}).fetchone()
if batch is None:
raise ValueError('unknown batchid: %s' % batchid)
# Only a FULLY undone batch is finished. A partially-undone one still has
# entries with isreverted = 0, and the whole point of retrying is to pick
# them up once whatever conflicted has been resolved.
if batch[0] == BATCH_UNDONE:
return UndoResult(batchid, 0, [])
@@ -184,10 +189,20 @@ def undo_batch(connection, batchid, references=None):
else:
conflicts.append(conflict)
# Record what actually happened, not what was attempted. Writing 'undone'
# unconditionally is what made a conflicted entry unrevertable: this call
# is the only way back, and the early return above refuses a second attempt
# on an undone batch.
#
# undoneat is stamped only when the batch is genuinely finished; on a
# partial it stays null, so "when was this reversed" keeps its meaning.
status = BATCH_PARTIAL if conflicts else BATCH_UNDONE
connection.execute(text(
'UPDATE importbatches SET status = :status, undoneat = :undoneat '
'WHERE batchid = :batchid'),
{'status': BATCH_UNDONE, 'undoneat': _utcnow(), 'batchid': batchid})
{'status': status,
'undoneat': None if conflicts else _utcnow(),
'batchid': batchid})
return UndoResult(batchid, reverted, conflicts)
@@ -218,10 +233,19 @@ def _revert_update(connection, table, targetid, column, oldvalue, newvalue):
def _revert_insert(connection, batchid, table, targetid, references):
"""Hard-delete a row this batch created, only if nothing outside the batch
still references it. Returns a Conflict when it is not safe, else None."""
from sqlalchemy import text
from sqlalchemy import inspect, text
pk = _primary_key(connection, table)
# A reference table may not exist here. Some referrers are plugin-owned -
# notifications, for one - and a lean site drops the tables of plugins it
# does not install (ADR-014). Querying an absent table raises 1146 on MySQL,
# which rolls the whole undo back and leaves the batch permanently
# un-undoable on that site. Skip what is not there; a table that does not
# exist cannot hold a reference.
present = set(inspect(connection).get_table_names())
for reftable, refcolumn in (references or {}).get(table, []):
if reftable not in present:
continue
referencing = connection.execute(text(
'SELECT COUNT(*) FROM %s WHERE %s = :targetid'
% (reftable, refcolumn)), {'targetid': targetid}).fetchone()[0]

4
scripts/site_imports/winc/dry_run.py
View File

@@ -56,6 +56,7 @@ from dotenv import load_dotenv
load_dotenv(Path(__file__).resolve().parents[3] / '.env')
from scripts.site_imports.approval import build_packet, format_packet
from scripts.site_imports.winc import writer
from scripts.site_imports.suggest import suggest
from scripts.site_imports.resolver import (
BLANK,
@@ -375,7 +376,8 @@ def main(argv=None):
'winc-machine-adoption', plan, summary,
[('businessunit', 'businessunit_status', 'raw_eq1',
'businessunit_name'),
('location', 'location_status', 'raw_location', 'location_name')])
('location', 'location_status', 'raw_location', 'location_name')],
creatable=writer._CREATABLE_GROUPS)
print('\n' + format_packet(packet))
return 0

54
scripts/site_imports/winc/writer.py
View File

@@ -51,17 +51,40 @@ _PACKET_GROUPS = [
('location', 'location_status', 'raw_location', 'location_name'),
]
# Groups apply_plan can actually create. _PACKET_GROUPS is what the plan
# REPORTS on; this is what the writer can act on, and the two are not the same.
_CREATABLE_GROUPS = ('businessunit',)
# References into a created row's id, so undo can tell whether an inserted
# business unit is still pointed at by something outside this batch before it
# hard-deletes it. Shared with the undo CLI and the web undo path.
_UNDO_REFERENCES = {
'businessunits': [('assets', 'businessunitid')],
# EVERY referrer, not just the obvious one. Undo hard-deletes a business
# unit this batch created, and it may only do that when nothing outside the
# batch still points at it. assets was the only entry, so a BU referenced
# from anywhere else was deleted anyway:
#
# - dashboarddefaults.businessunitid and businessunits.parentid are real
# FKs, so MySQL raises and the whole undo rolls back - consistent, but
# the batch then cannot be undone at all;
# - notifications.businessunitid has NO FK, so the delete succeeded and
# left the notification pointing at an id that no longer exists.
# Silently, on both engines.
#
# The guard itself is a SELECT COUNT(*), not FK enforcement, so it runs the
# same on SQLite - a wrong-but-real column name here would silently match
# nothing and delete the row anyway, which is why it is tested rather than
# trusted. What SQLite cannot show is the CONSEQUENCE of getting it wrong:
# MySQL raises on the FK cases, SQLite lets the dangling row through.
'businessunits': [
('assets', 'businessunitid'),
('dashboarddefaults', 'businessunitid'),
('businessunits', 'parentid'),
('notifications', 'businessunitid'),
],
}
# References into a created row's id, so undo can tell whether an inserted
# business unit is still pointed at by something outside this batch before it
# hard-deletes it.
# Max width of assets.name; the export description can exceed it (a few rows
# carry a whole safety notice in the field), so a set name is clamped rather
# than letting the whole transaction fail on one over-long row. The pre-image is
@@ -148,7 +171,8 @@ def apply_plan(connection, plan, approved_packet, acknowledge=None,
# (2)-(3) rebuild the packet and prove nothing drifted since approval.
current_packet = build_packet('winc-machine-adoption', plan,
approved_packet.get('counts', {}),
_PACKET_GROUPS)
_PACKET_GROUPS,
creatable=_CREATABLE_GROUPS)
if not packet_matches_plan(approved_packet, plan):
raise WriterError(
'plan has changed since approval (hash mismatch) - re-run the '
@@ -167,6 +191,24 @@ def apply_plan(connection, plan, approved_packet, acknowledge=None,
'needs-create not acknowledged for: %s - confirm these will be '
'created before importing' % ', '.join(sorted(missing_ack)))
# (5b) and the writer must actually be able to create what it just made the
# admin agree to. _PACKET_GROUPS lists location, so a plan needing a new
# location asked for acknowledgement - and then step 6a creates business
# units only, and 6b sets locationid only when the location already
# RESOLVED. The admin was told "this would create 3 locations", ticked it,
# and nothing was created, nothing was set, and nothing was reported.
#
# Refusing is the honest answer: better a plan that will not run than one
# that quietly does less than it said. Creating locations is a real feature
# and can be added here; until then this cannot pretend.
uncreatable = (set(current_packet.get('needscreate', {}))
- set(_CREATABLE_GROUPS))
if uncreatable:
raise WriterError(
'this import cannot create: %s - the rows must exist before it '
'runs, or be aliased to rows that do. Nothing was written.'
% ', '.join(sorted(uncreatable)))
batchid = approved_packet['batchid']
journal.open_batch(connection, approved_packet, createdby=createdby)
sequence = 1
@@ -244,7 +286,7 @@ def main(argv=None):
_app, connection = build_app_connection()
plan, summary, _lookups = analyze(connection, args.export)
packet = build_packet('winc-machine-adoption', plan, summary,
_PACKET_GROUPS)
_PACKET_GROUPS, creatable=_CREATABLE_GROUPS)
if not args.commit:
print('DRY: rebuilt plan of %d rows, hash %s' % (

1
shopdb/api/__init__.py
View File

@@ -150,6 +150,7 @@ from shopdb.core.models import (
ImportBatch,
ImportJournalEntry,
BATCH_APPLIED,
BATCH_PARTIAL,
BATCH_UNDONE,
OP_UPDATE,
OP_INSERT,

16
shopdb/config.py
View File

@@ -134,6 +134,22 @@ class Config:
DEFAULT_PAGE_SIZE = 20
MAX_PAGE_SIZE = 100
# Hard ceiling on a request body, enforced by Werkzeug before a view runs.
#
# Upload routes cap their own payloads, but every one of those checks runs
# AFTER the body has arrived: the first touch of request.form or
# request.files makes Werkzeug parse the whole multipart and spool the file
# part to a temp file, so a route can reject the content and still have paid
# for the transfer and the disk. Three separate reviews found that shape in
# three different routes, which makes it a property of the app rather than
# of any handler.
#
# 128 MB clears the largest thing anyone legitimately sends - the agent
# binary upload caps itself at 64 MB - with room for the multipart envelope.
# Over this, Werkzeug answers 413 without reading the body.
MAX_CONTENT_LENGTH = int(
os.environ.get('MAX_CONTENT_LENGTH', 128 * 1024 * 1024))
class DevelopmentConfig(Config):
"""Development configuration."""

6
shopdb/core/api/settings.py
View File

@@ -701,6 +701,7 @@ def winc_import_batches():
def winc_import_undo():
"""Reverse an applied WINC import batch by id. Conservative and reversible."""
from shopdb.core.services.wincimport import undo as winc_undo
from shopdb.core.services.wincimport import WincImportError
payload = request.get_json(silent=True) or {}
batchid = (payload.get('batchid') or request.form.get('batchid') or '').strip()
@@ -714,7 +715,10 @@ def winc_import_undo():
changes={'batchid': batchid, 'reverted': result['reverted'],
'conflicts': len(result['conflicts'])})
db.session.commit()
except ValueError as exc:
except (ValueError, WincImportError) as exc:
# WincImportError too: refusing a batch of another importer's kind is a
# bad request, not a server fault. Preview and commit already catch it;
# this route did not, so the refusal surfaced as a 500.
db.session.rollback()
return error_response(ErrorCodes.VALIDATION_ERROR, str(exc))
except Exception:

3
shopdb/core/models/__init__.py
View File

@@ -24,7 +24,7 @@ from .auditlog import AuditLog
from .customfield import CustomField, CustomFieldValue
from .apitoken import ApiToken
from .importbatch import (ImportBatch, ImportJournalEntry, BATCH_APPLIED,
BATCH_UNDONE, OP_UPDATE, OP_INSERT)
BATCH_PARTIAL, BATCH_UNDONE, OP_UPDATE, OP_INSERT)
__all__ = [
# Base
@@ -88,6 +88,7 @@ __all__ = [
'ImportBatch',
'ImportJournalEntry',
'BATCH_APPLIED',
'BATCH_PARTIAL',
'BATCH_UNDONE',
'OP_UPDATE',
'OP_INSERT',

29
shopdb/core/models/base.py
View File

@@ -29,10 +29,37 @@ class BaseModel(db.Model):
)
isactive = db.Column(db.Boolean, default=True, nullable=False)
# Columns the default to_dict NEVER emits, whatever model carries them.
#
# The default serialiser walks __table__.columns and publishes every one, so
# a model that does not override it publishes whatever it has - including
# columns added to it later, by someone who was not thinking about this
# method. User is the live example: it does not override to_dict, so
# User.to_dict() returns passwordhash. Nothing calls it in a response path
# today, which is luck rather than design.
#
# A blanket allowlist would be the stronger rule, but it would also silently
# empty the output of all 34 models that rely on this default. This is the
# part that can be fixed without breaking anything: name the columns that
# must never be published, and let a model add its own.
#
# A model with something sensitive should still override to_dict and say
# exactly what it emits. This is a floor, not permission to skip that.
HIDDEN_COLUMNS = frozenset({
'passwordhash',
'tokenhash',
'apikey',
'clientsecret',
'collectorkey',
})
def to_dict(self):
"""Convert model to dictionary."""
"""Convert model to dictionary, minus anything secret-bearing."""
hidden = self.HIDDEN_COLUMNS
result = {}
for c in self.__table__.columns:
if c.name in hidden:
continue
value = getattr(self, c.name)
if isinstance(value, datetime):
value = value.isoformat() + 'Z'

10
shopdb/core/models/importbatch.py
View File

@@ -26,6 +26,12 @@ from shopdb.extensions import db
# Batch lifecycle.
BATCH_APPLIED = 'applied'
BATCH_UNDONE = 'undone'
# An undo that reverted some entries and hit conflicts on the rest. It is a
# distinct state because 'undone' is a door that locks: undo_batch returns
# immediately for an undone batch, so marking a partly-reverted one 'undone'
# made the leftovers permanently unrevertable - the human resolves the conflict,
# clicks undo again, and gets a no-op.
BATCH_PARTIAL = 'partially-undone'
# Journal operation kinds.
OP_UPDATE = 'update'
@@ -43,7 +49,9 @@ class ImportBatch(db.Model):
rowcount = db.Column(db.Integer, nullable=False, default=0)
createdat = db.Column(db.DateTime, nullable=False)
createdby = db.Column(db.String(100), nullable=True)
status = db.Column(db.String(16), nullable=False, default=BATCH_APPLIED)
# 32, not 16: BATCH_PARTIAL is exactly 16 characters, so the original
# width fitted it with nothing to spare. See migration 7d38.
status = db.Column(db.String(32), nullable=False, default=BATCH_APPLIED)
undoneat = db.Column(db.DateTime, nullable=True)
entries = db.relationship(

31
shopdb/core/services/wincimport.py
View File

@@ -12,6 +12,8 @@ units, and records every change to the reversible import journal. That contract
lives here and in the writer, not in the generic canonical-CSV importer.
"""
import sqlalchemy as sa
from shopdb.extensions import db
from scripts.site_imports import journal
from scripts.site_imports.approval import build_packet
@@ -38,7 +40,8 @@ def _plan_and_packet(export_text):
plan, summary, _lookups = analyze_rows(_connection(), rows)
packet = build_packet('winc-machine-adoption', plan, summary,
winc_writer._PACKET_GROUPS)
winc_writer._PACKET_GROUPS,
creatable=winc_writer._CREATABLE_GROUPS)
return plan, summary, packet
@@ -53,6 +56,11 @@ def _report(summary, packet):
for group, items in packet.get('needscreate', {}).items()},
'unresolved': {group: dict(items)
for group, items in packet.get('unresolved', {}).items()},
# Carried separately from needscreate: both are "rows that do not
# exist", but only this one cannot be acknowledged away, and the page
# has to say WHY it is blocked rather than only that it is.
'uncreatable': {group: dict(items)
for group, items in packet.get('uncreatable', {}).items()},
'blocking': packet.get('blocking', False),
}
@@ -111,8 +119,27 @@ def commit(export_text, acknowledge=None, createdby=None, planhash=None):
return report
IMPORT_KIND = 'winc-machine-adoption'
def undo(batchid):
"""Reverse an applied batch. Caller owns the transaction. Returns a dict."""
"""Reverse an applied batch. Caller owns the transaction. Returns a dict.
Refuses a batch of another kind. The journal is a general primitive and
undo_batch does not filter on kind, so without this check the first other
importer's batches would be reversible through THIS endpoint - carrying
WINC's references map, which lists none of that importer's tables, so the
reference guard would pass on everything and the deletes would be
unconditional.
"""
kind = _connection().execute(
sa.text('SELECT kind FROM importbatches WHERE batchid = :batchid'),
{'batchid': batchid}).scalar()
if kind is not None and kind != IMPORT_KIND:
raise WincImportError(
'batch %s was created by a different importer (%s) and cannot be '
'reversed here' % (batchid, kind))
result = journal.undo_batch(_connection(), batchid,
references=winc_writer._UNDO_REFERENCES)
return {

69
tests/test_core/test_to_dict_floor.py Normal file
View File

@@ -0,0 +1,69 @@
"""BaseModel.to_dict never publishes a secret-bearing column.
The default serialiser walks __table__.columns and emits every one, so a model
that does not override it publishes whatever it happens to carry - including
columns added later by someone not thinking about this method. User is the live
example: it has no override, so User.to_dict() returned its bcrypt hash. Nothing
called it in a response path, which was luck.
A blanket allowlist is the stronger rule and would empty the output of every
model relying on the default, so this pins the floor instead: named columns are
never emitted, and a model may extend the set.
"""
import pytest
from shopdb.core.models import User
from shopdb.core.models.base import BaseModel
def test_the_default_serialiser_hides_a_password_hash(app):
with app.app_context():
user = User(username='floor', email='floor@example.invalid',
passwordhash='$2b$12$notarealhashbutlookslikeone')
data = user.to_dict()
assert 'passwordhash' not in data
# Still a useful dict - this is a filter, not a mute button.
assert data['username'] == 'floor'
assert data['email'] == 'floor@example.invalid'
def test_every_hidden_name_is_actually_dropped(app):
"""Each entry earns its place: drop one and the column comes back."""
with app.app_context():
user = User(username='floor2', email='f2@example.invalid',
passwordhash='x')
assert 'passwordhash' in {c.name for c in User.__table__.columns}
assert 'passwordhash' in BaseModel.HIDDEN_COLUMNS
assert 'passwordhash' not in user.to_dict()
def test_a_model_can_extend_the_hidden_set(app):
"""A model with its own secret adds to the floor rather than replacing
to_dict wholesale."""
with app.app_context():
class Extended(User):
__abstract__ = True
HIDDEN_COLUMNS = BaseModel.HIDDEN_COLUMNS | {'email'}
assert 'passwordhash' in Extended.HIDDEN_COLUMNS
assert 'email' in Extended.HIDDEN_COLUMNS
@pytest.mark.parametrize('name', sorted(BaseModel.HIDDEN_COLUMNS))
def test_no_model_publishes_a_hidden_column_through_the_default(name, app):
"""Sweep every mapped model: if it carries a hidden column AND relies on the
default to_dict, the column must not come out."""
with app.app_context():
from shopdb.extensions import db
for mapper in db.Model.registry.mappers:
model = mapper.class_
table = getattr(model, '__table__', None)
if table is None or name not in table.columns:
continue
if 'to_dict' in vars(model):
continue # overrides the default; its own business
assert name in getattr(model, 'HIDDEN_COLUMNS', frozenset()), (
'%s carries %s and uses the default to_dict, so it would be '
'published' % (model.__name__, name))

151
tests/test_winc_import_routes.py
View File

@@ -8,6 +8,7 @@ in-memory schema: seed a machine asset, upload an export that matches it, and
assert what preview reports, what commit writes, and that undo puts it back.
"""
import io
from datetime import datetime
import pytest
@@ -268,3 +269,153 @@ def test_a_preview_of_one_file_does_not_approve_another(client, db,
'planhash': approved})
assert response.status_code == 400
assert Asset.query.filter_by(name='Something Else Entirely').first() is None
# --- an undo that hit a conflict must stay retryable -------------------------
#
# undo_batch used to write 'undone' whatever happened, and its early return
# refuses a second undo on an undone batch - so the one entry that needed a
# retry was the only one that could never get it.
def test_a_conflicted_undo_is_marked_partial_and_can_be_retried(
client, db, auth_headers, app):
from shopdb.core.models import Asset, ImportBatch, BATCH_PARTIAL, BATCH_UNDONE
assetid, _ = _seed_machine(db)
committed = _commit(client, auth_headers, extra={'acknowledge': 'businessunit'})
assert committed.status_code == 200
batchid = committed.get_json()['data']['batchid']
# A human edits a cell the import wrote. Undo must refuse to clobber it.
with app.app_context():
asset = db.session.get(Asset, assetid)
asset.name = 'Edited By A Human'
db.session.commit()
first = client.post(UNDO, json={'batchid': batchid}, headers=auth_headers)
assert first.status_code == 200
body = first.get_json()['data']
assert body['conflicts'], 'expected the human edit to conflict'
with app.app_context():
assert db.session.get(ImportBatch, batchid).status == BATCH_PARTIAL
# The human resolves it. The retry must actually do something - this is
# what the old 'undone' status made impossible.
with app.app_context():
db.session.get(Asset, assetid).name = 'Renamed Machine'
db.session.commit()
second = client.post(UNDO, json={'batchid': batchid}, headers=auth_headers)
assert second.status_code == 200
assert second.get_json()['data']['reverted'] >= 1, 'retry did nothing'
with app.app_context():
assert db.session.get(ImportBatch, batchid).status == BATCH_UNDONE
def test_a_clean_undo_is_still_marked_undone(client, db, auth_headers, app):
from shopdb.core.models import ImportBatch, BATCH_UNDONE
_seed_machine(db)
committed = _commit(client, auth_headers, extra={'acknowledge': 'businessunit'})
batchid = committed.get_json()['data']['batchid']
response = client.post(UNDO, json={'batchid': batchid}, headers=auth_headers)
assert response.status_code == 200
assert not response.get_json()['data']['conflicts']
with app.app_context():
batch = db.session.get(ImportBatch, batchid)
assert batch.status == BATCH_UNDONE
assert batch.undoneat is not None
# --- the guards themselves, not just the paths through them ------------------
def test_undo_refuses_a_business_unit_something_else_points_at(
client, db, auth_headers, app):
"""_UNDO_REFERENCES named only assets, so a BU referenced from anywhere
else was hard-deleted anyway. The guard is a SELECT COUNT(*), so SQLite
exercises it exactly as MySQL does."""
from shopdb.core.models import BusinessUnit, DashboardDefault
_seed_machine(db)
committed = _commit(client, auth_headers, extra={'acknowledge': 'businessunit'})
assert committed.status_code == 200
batchid = committed.get_json()['data']['batchid']
with app.app_context():
created = BusinessUnit.query.filter_by(businessunit='Large Parts').first()
assert created is not None, 'the import should have created the BU'
db.session.add(DashboardDefault(businessunitid=created.businessunitid))
db.session.commit()
buid = created.businessunitid
response = client.post(UNDO, json={'batchid': batchid}, headers=auth_headers)
assert response.status_code == 200
reasons = [c['reason'] for c in response.get_json()['data']['conflicts']]
assert any('dashboarddefaults' in r for r in reasons), reasons
with app.app_context():
assert db.session.get(BusinessUnit, buid) is not None, \
'the BU was deleted despite still being referenced'
def test_undo_refuses_a_batch_from_another_importer(client, db, auth_headers, app):
from shopdb.core.models import ImportBatch, BATCH_APPLIED
with app.app_context():
db.session.add(ImportBatch(batchid='not-ours', kind='some-other-import',
planhash='0' * 64, rowcount=0,
createdat=datetime.utcnow(),
status=BATCH_APPLIED))
db.session.commit()
response = client.post(UNDO, json={'batchid': 'not-ours'}, headers=auth_headers)
assert response.status_code == 400, response.get_json()
assert 'different importer' in response.get_json()['data']['error']['message']
# --- a group the writer cannot create must block, visibly ---------------------
#
# _PACKET_GROUPS reports on location, but apply_plan only ever creates business
# units: it sets locationid only for a location that already RESOLVED. So a plan
# needing a new location used to preview clean, get acknowledged, commit, and
# silently do none of it.
# 'WM-T6/U6' is an alias override onto the canonical location 'T6'. Seeding the
# machine without a T6 location is what makes the location NEEDS_CREATE rather
# than UNRESOLVED - an unaliased bay is merely unresolved, which already blocked.
NEEDS_LOCATION_CSV = (
'assetnum,description,location,eq1\r\n'
'W0083A,Renamed Machine,WM-T6/U6,lg parts\r\n'
)
def test_preview_blocks_and_names_a_group_it_cannot_create(client, db,
auth_headers):
_seed_machine(db)
response = _upload(client, auth_headers, PREVIEW,
csv_text=NEEDS_LOCATION_CSV)
assert response.status_code == 200, response.get_json()
report = response.get_json()['data']
assert report['needscreate'].get('location') == {'T6': 1}, report
assert report['blocking'] is True, report
# Named, not merely blocked: the page renders this list, and without it the
# banner says "blocked" with nothing on screen to act on.
assert report['uncreatable'].get('location') == {'T6': 1}, report
def test_commit_refuses_a_group_it_cannot_create(client, db, auth_headers):
from shopdb.core.models.asset import Asset
assetid, _ = _seed_machine(db)
response = _commit(client, auth_headers, csv_text=NEEDS_LOCATION_CSV,
extra={'acknowledge': 'businessunit,location'})
assert response.status_code == 400, response.get_json()
with client.application.app_context():
assert db.session.get(Asset, assetid).name == 'Old Name', \
'nothing may be written'