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
321 lines
14 KiB
Python
321 lines
14 KiB
Python
"""WINC machine adoption - WRITER (applies the approved plan, reversibly).
|
|
|
|
This is the write half of the WINC import; scripts/site_imports/winc/dry_run.py
|
|
is the read-only half. The writer never invents its own view of the data: it
|
|
rebuilds the exact same plan the dry-run produced, verifies that plan against the
|
|
approval packet the human signed off on, and only then writes - recording every
|
|
change to the shared import journal so the whole batch can be undone.
|
|
|
|
The contract, in order (the one documented in approval.py, made concrete here):
|
|
|
|
1. rebuild the plan from the same export + live DB,
|
|
2. build a packet from it,
|
|
3. compare planhash to the approved packet - abort on mismatch (drift),
|
|
4. refuse if the packet is blocking (any unresolved value),
|
|
5. require explicit acknowledgment for every needs-create group,
|
|
6. write inside ONE transaction, stamping the batch id onto the journal:
|
|
a. create each acknowledged needs-create business unit (journal insert),
|
|
b. update each in-scope asset's name / location / business unit
|
|
(journal update, capturing the pre-image first).
|
|
|
|
Everything commits together or not at all. Ambiguous rows (one workstation key
|
|
matching several machines) are never written - the dry-run flags them and the
|
|
writer skips them, because guessing which machine an export row means is exactly
|
|
the kind of cleverness this import refuses.
|
|
"""
|
|
|
|
import argparse
|
|
import os
|
|
import sys
|
|
|
|
sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.dirname(
|
|
os.path.dirname(os.path.abspath(__file__))))))
|
|
|
|
from dotenv import load_dotenv
|
|
from pathlib import Path
|
|
|
|
load_dotenv(Path(__file__).resolve().parents[3] / '.env')
|
|
|
|
from scripts.site_imports import journal
|
|
from scripts.site_imports.approval import build_packet, packet_matches_plan
|
|
from scripts.site_imports.resolver import NEEDS_CREATE, RESOLVED, norm
|
|
from scripts.site_imports.winc.dry_run import (
|
|
analyze,
|
|
build_app_connection,
|
|
)
|
|
|
|
# The packet groups the WINC dry-run declares, reused verbatim so the writer
|
|
# hashes and reads the plan exactly the way approval did.
|
|
_PACKET_GROUPS = [
|
|
('businessunit', 'businessunit_status', 'raw_eq1', 'businessunit_name'),
|
|
('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 = {
|
|
# 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'),
|
|
],
|
|
}
|
|
|
|
|
|
# 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
|
|
# journalled, so the clamp is reversible like any other change.
|
|
_ASSET_NAME_MAXLEN = 100
|
|
|
|
|
|
class WriterError(Exception):
|
|
"""The plan cannot be written as approved (drift, blocking, or missing
|
|
acknowledgment). Carries a human-readable reason; never a partial write."""
|
|
|
|
|
|
def _load_asset_ids(connection):
|
|
"""assetnumber -> assetid for every asset, so a plan row (which carries the
|
|
matched asset *number*) can be turned into the id the update targets."""
|
|
from sqlalchemy import text
|
|
|
|
out = {}
|
|
for assetid, assetnumber in connection.execute(text(
|
|
'SELECT assetid, assetnumber FROM assets')):
|
|
out[assetnumber] = assetid
|
|
return out
|
|
|
|
|
|
def _create_businessunit(connection, batchid, sequence, name):
|
|
"""Insert one business unit and journal it. Returns (new_id, next_sequence).
|
|
|
|
Only ever called for an acknowledged needs-create name, so the journal's
|
|
insert entry describes strictly a row this batch created."""
|
|
from sqlalchemy import text
|
|
|
|
now = journal._utcnow()
|
|
result = connection.execute(text(
|
|
'INSERT INTO businessunits '
|
|
'(businessunit, createddate, modifieddate, isactive) '
|
|
'VALUES (:name, :now, :now, 1)'),
|
|
{'name': name, 'now': now})
|
|
new_id = result.lastrowid
|
|
journal.record_insert(connection, batchid, sequence, 'businessunits',
|
|
new_id)
|
|
return new_id, sequence + 1
|
|
|
|
|
|
def _update_cell(connection, batchid, sequence, table, targetid, column,
|
|
oldvalue, newvalue):
|
|
"""Apply one cell update and journal its pre/post image. Returns the next
|
|
sequence. Skips (and does not journal) a no-op where the value is unchanged."""
|
|
from sqlalchemy import text
|
|
|
|
if journal._as_text(oldvalue) == journal._as_text(newvalue):
|
|
return sequence
|
|
connection.execute(text(
|
|
'UPDATE %s SET %s = :newvalue WHERE %s = :targetid'
|
|
% (table, column, _asset_pk(table))),
|
|
{'newvalue': newvalue, 'targetid': targetid})
|
|
journal.record_update(connection, batchid, sequence, table, targetid,
|
|
column, oldvalue, newvalue)
|
|
return sequence + 1
|
|
|
|
|
|
def _asset_pk(table):
|
|
# The writer only updates the assets table; keep the mapping explicit rather
|
|
# than reflecting, so an unexpected table name fails loudly.
|
|
return {'assets': 'assetid'}[table]
|
|
|
|
|
|
def apply_plan(connection, plan, approved_packet, acknowledge=None,
|
|
createdby=None):
|
|
"""Apply an approved WINC plan, reversibly. Returns the batch id.
|
|
|
|
connection a live SQLAlchemy connection in an open transaction.
|
|
plan the per-row plan from dry_run.analyze (rebuilt fresh).
|
|
approved_packet the packet a human signed off on.
|
|
acknowledge set of needs-create group names the human confirmed
|
|
(e.g. {'businessunit'}); required for every group the
|
|
packet lists under needscreate.
|
|
createdby who acknowledged, stamped on the batch.
|
|
|
|
Raises WriterError before touching anything if the plan drifted, is
|
|
blocking, or a needs-create group was not acknowledged.
|
|
"""
|
|
acknowledge = set(acknowledge or ())
|
|
|
|
# (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,
|
|
creatable=_CREATABLE_GROUPS)
|
|
if not packet_matches_plan(approved_packet, plan):
|
|
raise WriterError(
|
|
'plan has changed since approval (hash mismatch) - re-run the '
|
|
'dry-run and re-approve before importing')
|
|
|
|
# (4) never write over an unresolved value.
|
|
if current_packet.get('blocking'):
|
|
raise WriterError(
|
|
'plan is blocking: unresolved values remain - add the missing '
|
|
'aliases and re-run the dry-run')
|
|
|
|
# (5) every needs-create group must be explicitly acknowledged.
|
|
missing_ack = set(current_packet.get('needscreate', {})) - acknowledge
|
|
if missing_ack:
|
|
raise WriterError(
|
|
'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
|
|
|
|
# (6a) create acknowledged needs-create business units once each, first, so
|
|
# the asset updates below can point at them.
|
|
created_bu = {}
|
|
for row in plan:
|
|
if row['businessunit_status'] != NEEDS_CREATE:
|
|
continue
|
|
name = row['businessunit_name']
|
|
key = norm(name)
|
|
if key not in created_bu:
|
|
created_bu[key], sequence = _create_businessunit(
|
|
connection, batchid, sequence, name)
|
|
|
|
# (6b) update each in-scope, unambiguous asset.
|
|
asset_ids = _load_asset_ids(connection)
|
|
from sqlalchemy import text
|
|
for row in plan:
|
|
if row['ambiguous']:
|
|
continue
|
|
assetnumber = row['matched_asset']
|
|
assetid = asset_ids.get(assetnumber)
|
|
if assetid is None:
|
|
continue
|
|
|
|
current = connection.execute(text(
|
|
'SELECT name, locationid, businessunitid FROM assets '
|
|
'WHERE assetid = :assetid'), {'assetid': assetid}).fetchone()
|
|
cur_name, cur_location, cur_bu = current
|
|
|
|
if row['name']:
|
|
new_name = row['name'][:_ASSET_NAME_MAXLEN]
|
|
sequence = _update_cell(connection, batchid, sequence, 'assets',
|
|
assetid, 'name', cur_name, new_name)
|
|
|
|
if row['location_status'] == RESOLVED and row['locationid'] != '':
|
|
sequence = _update_cell(connection, batchid, sequence, 'assets',
|
|
assetid, 'locationid', cur_location,
|
|
row['locationid'])
|
|
|
|
new_bu = None
|
|
if row['businessunit_status'] == RESOLVED and row['businessunitid'] != '':
|
|
new_bu = row['businessunitid']
|
|
elif row['businessunit_status'] == NEEDS_CREATE:
|
|
new_bu = created_bu[norm(row['businessunit_name'])]
|
|
if new_bu is not None:
|
|
sequence = _update_cell(connection, batchid, sequence, 'assets',
|
|
assetid, 'businessunitid', cur_bu, new_bu)
|
|
|
|
return batchid
|
|
|
|
|
|
def main(argv=None):
|
|
parser = argparse.ArgumentParser(
|
|
description='WINC machine adoption writer. Applies an approved plan '
|
|
'reversibly. Requires --commit to write.')
|
|
parser.add_argument('--export', required=True,
|
|
help='Path to the ASSET export CSV.')
|
|
parser.add_argument('--acknowledge', action='append', default=[],
|
|
metavar='GROUP',
|
|
help='Acknowledge a needs-create group (repeatable), '
|
|
'e.g. --acknowledge businessunit.')
|
|
parser.add_argument('--createdby', default=None,
|
|
help='Who is approving this import (stamped on batch).')
|
|
parser.add_argument('--commit', action='store_true',
|
|
help='Actually write. Without it, this is a no-op that '
|
|
'reports what would happen.')
|
|
args = parser.parse_args(argv)
|
|
|
|
if not os.path.exists(args.export):
|
|
parser.error('export file not found: %s' % args.export)
|
|
|
|
_app, connection = build_app_connection()
|
|
plan, summary, _lookups = analyze(connection, args.export)
|
|
packet = build_packet('winc-machine-adoption', plan, summary,
|
|
_PACKET_GROUPS, creatable=_CREATABLE_GROUPS)
|
|
|
|
if not args.commit:
|
|
print('DRY: rebuilt plan of %d rows, hash %s' % (
|
|
packet['rowcount'], packet['planhash']))
|
|
print('DRY: pass --commit with --acknowledge for each needs-create '
|
|
'group to write. Needs-create groups: %s'
|
|
% (', '.join(packet.get('needscreate', {})) or 'none'))
|
|
return 0
|
|
|
|
from shopdb.api import db
|
|
try:
|
|
batchid = apply_plan(connection, plan, packet,
|
|
acknowledge=args.acknowledge,
|
|
createdby=args.createdby)
|
|
db.session.commit()
|
|
except WriterError as error:
|
|
db.session.rollback()
|
|
print('REFUSED: %s' % error)
|
|
return 2
|
|
except Exception:
|
|
db.session.rollback()
|
|
raise
|
|
|
|
print('COMMITTED batch %s' % batchid)
|
|
print(' to undo: python -m scripts.site_imports.winc.undo --batch %s'
|
|
% batchid)
|
|
return 0
|
|
|
|
|
|
if __name__ == '__main__':
|
|
sys.exit(main())
|