377cc69264
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
52 lines
1.7 KiB
Python
52 lines
1.7 KiB
Python
"""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)
|