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

frontend/src/assets/style.css

@@ -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;

frontend/src/views/settings/BrandingSettings.vue

@@ -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,

frontend/src/views/settings/FloorMapSettings.vue

@@ -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; }

frontend/src/views/settings/ImportSiteAssets.vue

@@ -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>

migrations/versions/7d38_widen_importbatch_status.py

@@ -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)

plugins/computers/frontend/views/ActionPicker.vue

@@ -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">

scripts/check-naming-and-style.sh

@@ -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"

scripts/site_imports/approval.py

@@ -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)
 

scripts/site_imports/journal.py

@@ -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]

scripts/site_imports/winc/dry_run.py

@@ -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

scripts/site_imports/winc/writer.py

@@ -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' % (

shopdb/api/__init__.py

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

shopdb/config.py

@@ -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."""

shopdb/core/api/settings.py

@@ -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:

shopdb/core/models/__init__.py

@@ -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',

shopdb/core/models/base.py

@@ -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'

shopdb/core/models/importbatch.py

@@ -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(

shopdb/core/services/wincimport.py

@@ -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 {

tests/test_core/test_to_dict_floor.py

@@ -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))

tests/test_winc_import_routes.py

@@ -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'