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

Five findings from the review of the WINC import, plus the rendering each fix
needed to be usable.

An undo that hit a conflict wrote status 'undone' anyway, and undo_batch returns
immediately for an undone batch. So the one entry that needed a retry was the
only one that could never get it: the human resolves the conflict, clicks Undo,
and gets a no-op. A conflicted undo is now 'partially-undone', undoneat is
stamped only when the batch is genuinely finished, and the batches table offers
"Retry undo" for that status. The column was String(16) and the new value is
exactly 16 characters, so migration 7d38 widens it to 32 rather than rely on an
exact fit.

_UNDO_REFERENCES listed only assets, so a business unit this batch created was
hard-deleted even when something else still pointed at it. Two of the three
missing referrers are real FKs (dashboarddefaults.businessunitid,
businessunits.parentid): MySQL raises and the whole undo rolls back, so the
batch cannot be undone at all. The third, notifications.businessunitid, has no
FK, so the delete succeeded and left a notification pointing at an id that no
longer exists - silently, on both engines. All four are listed and the guard is
tested through the route.

The reference guard also queried tables that need not exist. A lean site drops
the tables of plugins it does not install (ADR-014), and notifications is
plugin-owned, so on such a site the query raised 1146, rolled the undo back and
left the batch permanently un-undoable. Absent tables are skipped; a table that
does not exist cannot hold a reference.

_PACKET_GROUPS reports on location, but apply_plan only ever creates business
units - it sets locationid only when the location already resolved. A plan
needing a new location asked the admin to acknowledge it, got the tick, wrote
nothing, and reported nothing. build_packet now takes the groups the writer can
actually create; anything else needed is 'uncreatable' and blocks at plan time,
where the admin can act on it, rather than dying at commit on a message about
acknowledging something the screen had no control for. The writer refuses the
same case independently. Both the console packet and the import page name the
blocking groups: setting blocking without printing the reason would leave the
banner pointing at nothing.

The web undo route caught ValueError only, so refusing a batch belonging to
another importer surfaced as a 500. That refusal is new too: the journal is a
general primitive and undo_batch does not filter on kind, so any other
importer's batches were reversible through this endpoint, carrying WINC's
references map - which lists none of that importer's tables, making every
reference check pass and every delete unconditional.

Nine tests. The two guards were verified by planting violations: dropping
dashboarddefaults from the references map, and passing creatable=None, each fail
the test written for them.

Scope: product

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