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'