Tables tab: delete a Table from the row, published ones included - #2583
Merged
Merged
Conversation
Delete joins the table action service as its third action (Data maintainer or above) and the row's ⋯ menu as its last entry. A draft is confirmed plainly; one published Table by typing its name, a batch with a published Table or more than ten by typing the count, all enforced by the server inside execute's transaction, against the Tables as they are then. A mismatch is the dialog's own 400, no toast. The dialog states what deleting breaks: the Datasets left (other people's by owner), the Review state, an active embargo, and that knowledge-graph links stop resolving. Order of work: the Django rows go in execute's transaction, now durable for delete so it cannot be nested; the OEDB tables are dropped after it has committed, one at a time (Table.delete_record / drop_oedb_table). A failed drop is logged at WARNING with drop=failed and comes back as a lasting warning toast naming the Table, never as a success. Log fields: datasets=<names left>|- published=yes|no drop=ok|failed. Ceiling: 50 Tables per request, enforced in execute and stated in the dialog. Production runs Timeout 300 / socket-timeout=300 (read on the host 2026-10-02); locally a delete costs 17-38 ms per Table up to 1M rows and at worst 1.13 s (10M rows), so 50 at 1.2 s is 60 s, a safety factor of 5. benchmarks/tables_tab/delete_cost.py takes the measurement. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
#2563 (PR #2582) landed first and touched the same seams. Resolved by keeping both sides: - table_actions.py: develop's Dataset actions plus delete. Delete's ceiling also blocks Preflight.confirmable, the property #2563 added for the confirm button, so the dialog reads one condition. - table_action_dialog.html: delete joins #2563's elif chains (title, "Will be …", the body include, "Nothing to …", the confirm button, red for delete only). - cells/menu.html: Delete stays last, after the Dataset entries. - views.py: "confirm" joins TableActionView.PARAMS. - test_table_actions: only Manage access is still absent from the menu. - changelog: both entries, #2563's first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2562 (slice 10 of spec #2551).
What it does
Delete joins the table action service from #2561 as its third action, and the row's ⋯ menu as its last entry (red, behind a divider). It reuses the same path as publish and unpublish:
ROLE_GATES[DELETE]atDELETE_PERM). Below that, the entry stays in the menu, disabled, with the reason.execute's transaction, against the Tables as they are at that moment. If a draft was published after the dialog opened, the request now asks for the name. A mismatch is the dialog's own input: 400, the error beside the field, no toast.Order of work across the two databases
Table.deleteis split intodelete_record()(Django rows and cascades) anddrop_oedb_table().delete()still calls both, so the API is unchanged.executeremoves the Django rows inside its transaction, which holds the row lock. For delete that transaction isdurable=True, so it cannot run nested inside another transaction. When the block is left, the rows are committed, and only then are the OEDB tables dropped, one Table at a time.A failed drop:
drop=failed;Outcome.drop_failed;tables-changedwithwarning: true.tables_tab.jsshows that message as an assertive toast that stays until dismissed (dash-toast--warning) and names the Table, with its technical name. It never shows as a success.Log line per Table:
table_action … action=delete … datasets=<names left>|- published=yes|no drop=ok|failed.The delete ceiling: 50
Production timeout. Read on the host today (2026-10-02 18:07 UTC, read-only):
Timeout 300(httpd.conf:123);socket-timeout=300on bothWSGIDaemonProcessgroups;request-timeout.This is unchanged since WF-21's capture on 2026-09-21.
Per-Table cost. Measured locally with the new
benchmarks/tables_tab/delete_cost.py(Postgres 14,shared_buffers128 MB, three rounds). Each run deletes a batch of real OEDB-backed Tables throughexecute:Safety factor 5. At a worst case of 1.2 s per Table, 50 Tables take 60 s, which is one fifth of the 300 s timeout. The margin is meant to cover production's OEDB sitting on another host and its larger buffer pool. A typical batch of 50 takes 1–2 s.
Where it is enforced.
executerefuses more than 50 names before it reads anything (400,errors.table). The dialog states the limit ("Delete takes at most 50 tables at a time; you selected 412") and offers no confirm button. The reasoning sits next toCEILINGSin the code.Preflight.ceilingis now set, so #2564 adds its own ceilings to the same dict.Points for you to rule on
DROP TABLEwaits for any session that holds a lock on that table, with nolock_timeout. Theapi_tableDELETE has the same behaviour today. I left the shareddrop_if_existspath alone. If you want a guard, the place is aSET LOCAL lock_timeoutin the drop, and a timed-out drop would then report asdrop=failed.PeerReview.tableis a name, not a foreign key, so it is not cascaded. The dialog says so ("The review stays on record without its table") rather than hiding it. This is unchanged behaviour. A new Table that reuses the name would inherit those reviews.Tests
login/tests/test_table_delete.py: 22 tests through HTTP. They cover the role gate, consequences, typed name and typed count, a draft published since the dialog opened, refused-whole, the ceiling, and that rows are gone from both databases. One test checks that the drop runs after the rows are gone and outside the service's transaction. Another checks a failed drop: rows gone, data still in the OEDB, the warning naming the Table, and the WARNING log line. Plus the log fields and the re-fetch.tables-changed).login323 green, vitest 161 green. The full suite ran 1,224 tests, green apart from the knownLightImportTest, which fails only under the isolated-settings trick and passes alone.Browser check (headless Chrome, throwaway DB, 1440 px):
No migration and no deploy step. This lifts the release gate for #2553 once it merges.
🤖 Generated with Claude Code