From 377cc692645b2994308790b0b5ec58fb68d842c5 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Thu, 3 Sep 2026 09:09:55 -0400 Subject: [PATCH] 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 --- .../src/views/settings/ImportSiteAssets.vue | 43 ++++- .../versions/7d38_widen_importbatch_status.py | 51 ++++++ scripts/site_imports/approval.py | 47 +++++- scripts/site_imports/journal.py | 28 +++- scripts/site_imports/winc/dry_run.py | 4 +- scripts/site_imports/winc/writer.py | 54 ++++++- shopdb/api/__init__.py | 1 + shopdb/core/api/settings.py | 6 +- shopdb/core/models/__init__.py | 3 +- shopdb/core/models/importbatch.py | 10 +- shopdb/core/services/wincimport.py | 31 +++- tests/test_winc_import_routes.py | 151 ++++++++++++++++++ 12 files changed, 408 insertions(+), 21 deletions(-) create mode 100644 migrations/versions/7d38_widen_importbatch_status.py diff --git a/frontend/src/views/settings/ImportSiteAssets.vue b/frontend/src/views/settings/ImportSiteAssets.vue index db0dff50..279d1e43 100644 --- a/frontend/src/views/settings/ImportSiteAssets.vue +++ b/frontend/src/views/settings/ImportSiteAssets.vue @@ -269,7 +269,7 @@ below if needed. @@ -314,6 +314,29 @@ + +
+

Rows this import cannot create (blocking)

+

+ 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. +

+ +
+

Unresolved values (blocking)

@@ -354,15 +377,27 @@ {{ b.createdat }} {{ b.createdby || '-' }} {{ b.rowcount }} - {{ b.status }} + {{ b.status }} + + - some rows could not be put back + + + + undone diff --git a/migrations/versions/7d38_widen_importbatch_status.py b/migrations/versions/7d38_widen_importbatch_status.py new file mode 100644 index 00000000..2a0891f2 --- /dev/null +++ b/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) diff --git a/scripts/site_imports/approval.py b/scripts/site_imports/approval.py index 5892f867..aada8b80 100644 --- a/scripts/site_imports/approval.py +++ b/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) diff --git a/scripts/site_imports/journal.py b/scripts/site_imports/journal.py index 651af9f4..45db52fe 100644 --- a/scripts/site_imports/journal.py +++ b/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] diff --git a/scripts/site_imports/winc/dry_run.py b/scripts/site_imports/winc/dry_run.py index fc14a1b3..c1231e59 100644 --- a/scripts/site_imports/winc/dry_run.py +++ b/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 diff --git a/scripts/site_imports/winc/writer.py b/scripts/site_imports/winc/writer.py index ce18d3b9..ac0f7f76 100644 --- a/scripts/site_imports/winc/writer.py +++ b/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' % ( diff --git a/shopdb/api/__init__.py b/shopdb/api/__init__.py index db33a9d6..7ef75df5 100644 --- a/shopdb/api/__init__.py +++ b/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, diff --git a/shopdb/core/api/settings.py b/shopdb/core/api/settings.py index 55090226..c314688a 100644 --- a/shopdb/core/api/settings.py +++ b/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: diff --git a/shopdb/core/models/__init__.py b/shopdb/core/models/__init__.py index 35eabeb7..19ba8dc5 100644 --- a/shopdb/core/models/__init__.py +++ b/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', diff --git a/shopdb/core/models/importbatch.py b/shopdb/core/models/importbatch.py index 7eb06da4..1042c3af 100644 --- a/shopdb/core/models/importbatch.py +++ b/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( diff --git a/shopdb/core/services/wincimport.py b/shopdb/core/services/wincimport.py index 52c09c76..1911f371 100644 --- a/shopdb/core/services/wincimport.py +++ b/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 { diff --git a/tests/test_winc_import_routes.py b/tests/test_winc_import_routes.py index 6c71a259..9281703f 100644 --- a/tests/test_winc_import_routes.py +++ b/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'