From fe71d764400d15560c27d8ce3991b06967896c81 Mon Sep 17 00:00:00 2001 From: jh-RLI Date: Sat, 3 Oct 2026 02:10:29 +0200 Subject: [PATCH 1/2] Tables tab: bulk delete and bulk dataset add/remove (#2565) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bulk bar gains "Add to dataset…", "Remove from dataset…" and "Delete…" (red, last), all through the table action service and the one action dialog #2564 built. Bulk delete lists every Table in a scrollable, focusable list with the published ones marked, and counts what deleting breaks instead of repeating it per Table: how many are published, reviewed or in review, under an active embargo, which Datasets lose how many of them (other people's by owner), and the knowledge-graph links that stop resolving. _delete_consequences now returns the per-Dataset counts and the review counts, in three queries instead of five. The typed count and the failed drops named in a lasting warning were already the service's; they are now tested through the bulk preflight and the joined `tables` field. Bulk Dataset actions: which Tables are already in the chosen Dataset (add) or not in it (remove) depends on the choice, so choosing re-asks the bulk preflight with the whole selection (`selection`) and swaps only the preview, the footer and a status line, never the select. A closed select fires `change` on each arrow key, so the request waits until the choice has rested (delay 400 ms) and a newer one replaces an older one; checked in headless Chrome: focus stays on the select, three quick arrow presses send one request. A confirmation sent before the re-check came back is refused whole and the dialog re-run, as before. Ceilings: adding to and removing from a Dataset take at most 2,500 Tables, measured with the new benchmarks/tables_tab/dataset_cost.py (worst 6.8 ms per Table at 500 KB of metadata, about 17 s for 2,500 against the host's 300 s); every measured account (2,068 Tables at most) fits one request. The ceiling message names actions in words ("Adding to a dataset takes at most ..."). Co-Authored-By: Claude Opus 5.5 (1M context) --- api/services/table_actions.py | 73 ++- benchmarks/tables_tab/dataset_cost.py | 171 +++++++ .../login/partials/table_action_dataset.html | 73 ++- .../partials/table_action_dataset_select.html | 23 + .../login/partials/table_action_delete.html | 78 ++- .../login/partials/table_action_dialog.html | 252 ++++----- login/templates/login/user_tables.html | 6 +- login/tests/test_table_delete.py | 4 +- .../tests/test_tables_bulk_delete_datasets.py | 483 ++++++++++++++++++ login/views.py | 29 +- modelview/tests/html.py | 48 ++ versions/changelogs/current.md | 12 + 12 files changed, 1055 insertions(+), 197 deletions(-) create mode 100644 benchmarks/tables_tab/dataset_cost.py create mode 100644 login/templates/login/partials/table_action_dataset_select.html create mode 100644 login/tests/test_tables_bulk_delete_datasets.py diff --git a/api/services/table_actions.py b/api/services/table_actions.py index a39b98fca..3045a6b85 100644 --- a/api/services/table_actions.py +++ b/api/services/table_actions.py @@ -108,8 +108,7 @@ class RoleGate: DELETE: RoleGate(DELETE_PERM, "Only Data maintainers and Table admins can delete"), } -# The most Tables one request may name, per action; an action not listed has -# no ceiling yet (the Dataset actions get theirs in #2565). The dashboard is +# The most Tables one request may name, per action. The dashboard is # synchronous by design (no task queue), so a request has to finish inside # the host's timeout; mass work stays possible through the API, one call per # Table. Over the ceiling the preflight reads nothing and nothing can be @@ -144,9 +143,25 @@ class RoleGate: # a safety factor of 5 against the 300 s, which covers production's OEDB # sitting on another host and its larger buffer pool. Typical: 1-2 s. Not # covered: a drop waiting for a lock another session holds on that Table. +# +# Adding to and removing from a Dataset: 2,500 each. The same host limit +# (300 s). Measured locally with ``benchmarks/tables_tab/dataset_cost.py`` +# (Postgres 14, batches of 100, 400 and 1,000, three rounds), per Table, +# against 6 / 60 / 500 KB of metadata: adding 2.5-3.0 / 2.7-3.3 / 5.0-6.8 +# ms, removing 0.8-1.2 / 1.1-1.4 / 3.3-3.9 ms; the preflight with a Dataset +# chosen is 0.1-0.3 / 0.4-0.7 / 3.0-3.5 ms of that. Adding is the dearer: it +# writes the membership and seeds the Table's Topics into the Dataset. +# Neither saves the Table, so the metadata costs only its decoding in the +# preflight. At the worst 6.8 ms, 2,500 Tables take about 17 s: a safety +# factor of about 17 against the 300 s, more than publish's, because this +# ceiling is set by what a curator needs rather than by time: "select all" +# on the largest account (2,068 Tables), then "Add to dataset", is one +# request. CEILINGS = { PUBLISH: 1000, UNPUBLISH: 1000, + DATASET_ADD: 2500, + DATASET_REMOVE: 2500, DELETE: 50, } @@ -224,6 +239,10 @@ class Preflight: consequences: dict = field(default_factory=dict) subject: str = "" confirmation: str = "" + # every name sent, once each, in the order sent: what a bulk dialog + # re-checks when the user chooses a Dataset, so the left-out groups stay + # complete although the form posts only the eligible names + requested: list = field(default_factory=list) # The Dataset actions only: the user's own Datasets the action could # change for these Tables, and the one it is about (None until chosen). datasets: list = field(default_factory=list) @@ -321,9 +340,20 @@ def _gate_reason(table) -> str: return "Fails the Publish gate: " + ", ".join(failed) +# What an action is called at the start of a sentence, as in the ceiling's +# "Delete takes at most 50 tables at a time". +ACTION_NAMES = { + PUBLISH: "Publish", + UNPUBLISH: "Unpublish", + DELETE: "Delete", + DATASET_ADD: "Adding to a dataset", + DATASET_REMOVE: "Removing from a dataset", +} + + def _ceiling_message(action, ceiling, total) -> str: return ( - f"{action.capitalize()} takes at most {ceiling:,} tables at a time; " + f"{ACTION_NAMES[action]} takes at most {ceiling:,} tables at a time; " f"you selected {total:,}." ) @@ -380,17 +410,25 @@ def _others_datasets(user, tables) -> list: def _delete_consequences(user, tables) -> dict: """What deleting ``tables`` breaks, for the dialog: the Datasets they leave (the user's own by name, other people's by owner and name, since - their membership goes silently), which are published, their Review - state, an active embargo, and whether knowledge-graph links may point at - them. Five queries whatever the number of Tables.""" + their membership goes silently), each with how many of ``tables`` leave + it, which are published, their Review state, an active embargo, and + whether knowledge-graph links may point at them. A batch is shown + counted rather than listed per Table, so the review states are counted + here too. Three queries whatever the number of Tables.""" names = [table.name for table in tables] published = [table for table in tables if table.is_publish] datasets = ( - Dataset.objects.filter(tables__in=tables) - .filter(pk__in=visible_datasets(user).values("pk")) - .order_by("name") - .distinct() + Dataset.objects.filter(pk__in=visible_datasets(user).values("pk")) + .annotate(leaving=Count("tables", filter=Q(tables__in=tables))) + .filter(leaving__gt=0) + .values_list("creator_id", "creator__name", "name", "leaving") ) + own, others = [], [] + for creator, owner, name, leaving in datasets: + if creator == user.pk: + own.append((name, leaving)) + else: + others.append((owner or "Unknown owner", name, leaving)) reviews = {} for name, finished in PeerReview.objects.filter(table__in=names).values_list( "table", "is_finished" @@ -402,16 +440,17 @@ def _delete_consequences(user, tables) -> dict: .values_list("table__name", "date_ended") ) by_name = {table.name: table for table in tables} + finished = sum(1 for state in reviews.values() if state) return { "published": published, - "own_datasets": list( - datasets.filter(creator=user).values_list("name", flat=True) - ), - "others_datasets": _others_datasets(user, tables), + "own_datasets": sorted(own), + "others_datasets": sorted(others), "reviewed": [ - (by_name[name], "Reviewed" if finished else "In review") - for name, finished in sorted(reviews.items()) + (by_name[name], "Reviewed" if state else "In review") + for name, state in sorted(reviews.items()) ], + "finished_reviews": finished, + "open_reviews": len(reviews) - finished, "embargoed": [ (by_name[name], until) for name, until in sorted(embargoes.items()) ], @@ -502,6 +541,7 @@ def preflight(user, action, names, params=None) -> Preflight: left_out=[], ceiling=ceiling, subject=f"{len(names)} tables", + requested=names, ) found = {table.name: table for table in Table.objects.filter(name__in=names)} levels = table_levels(user, found.values()) @@ -559,6 +599,7 @@ def preflight(user, action, names, params=None) -> Preflight: confirmation=_confirmation(action, eligible), datasets=datasets, dataset=dataset, + requested=names, ) diff --git a/benchmarks/tables_tab/dataset_cost.py b/benchmarks/tables_tab/dataset_cost.py new file mode 100644 index 000000000..1bd6fa626 --- /dev/null +++ b/benchmarks/tables_tab/dataset_cost.py @@ -0,0 +1,171 @@ +"""What adding a Table to a Dataset and removing it cost, to set their ceilings (#2565). + + # the default: batches of 100 and 400 Tables, 6 KB and 60 KB of metadata + python -m benchmarks.tables_tab.dataset_cost + + python -m benchmarks.tables_tab.dataset_cost --tables 1000 --metadata-kb 500 + +Spec #2551 owes these numbers: the dashboard adds a selection to one of the +user's own Datasets, or removes it, in one request, with no task queue, so +the most Tables one request may name (``CEILINGS["dataset_add"]`` and +``CEILINGS["dataset_remove"]`` in ``api/services/table_actions.py``) has to +finish inside the host's request timeout, with a stated safety factor. + +Like ``publish_cost.py`` this never touches production or a developer +database: it asks Django's test runner for a throwaway database. Dataset +membership lives in Django only, so no OEDB table is created. + +For each metadata size and batch size it creates that many Tables shaped +like real ones (the metadata, a Data editor grant, two Topics each, which +adding seeds into the Dataset), half of them drafts (the curation rule reads +the grant for those) and half published, and one Dataset of the user's own, +then times what a bulk add and a bulk remove do, through the service the +dashboard calls: + +- ``check``: the preflight the dialog shows once a Dataset is chosen + (``table_actions.preflight`` with ``dataset``), which also runs the + curation rule and the membership split; +- ``add``: ``table_actions.execute`` adding every Table; +- ``remove``: ``table_actions.execute`` removing them again. + +Each is reported per Table: the whole call divided by the Tables in it. +""" + +from __future__ import annotations + +import argparse +import csv +import logging +import time +import uuid +from datetime import datetime, timezone +from pathlib import Path + +from benchmarks.tables_tab.publish_cost import metadata +from benchmarks.tables_tab.run import bootstrap + +DEFAULT_RESULTS = Path("benchmarks/results/tables_tab_dataset.csv") + + +def parse_args(argv=None): + p = argparse.ArgumentParser( + prog="python -m benchmarks.tables_tab.dataset_cost", + description="Measure what adding a Table to a Dataset and removing it cost.", + ) + p.add_argument("--tables", default="100,400") + p.add_argument("--metadata-kb", default="6,60") + p.add_argument("--results", type=Path, default=DEFAULT_RESULTS) + p.add_argument("--no-results", action="store_true") + return p.parse_args(argv) + + +def main(argv=None) -> int: + args = parse_args(argv) + runner, old_config = bootstrap() + # one line per Table is the service's record, not this measurement's + logging.getLogger("oeplatform.table_actions").setLevel(logging.WARNING) + + from django.db import connection + + from api.services import table_actions + from dataedit.models import Dataset, Table, Topic + from login.models import WRITE_PERM, UserPermission, myuser + + owner = myuser.objects.create( + name=f"bench_dataset_{uuid.uuid4().hex[:6]}", + email=f"bench_{uuid.uuid4().hex[:6]}@example.org", + did_agree=True, + is_mail_verified=True, + ) + topics = [ + Topic.objects.get_or_create(name=name)[0] + for name in ("bench_topic_a", "bench_topic_b") + ] + for action in table_actions.DATASET_ACTIONS: + table_actions.CEILINGS[action] = None + stamp = datetime.now(timezone.utc).isoformat(timespec="seconds") + results = [] + + def per_table(fn, n): + connection.close() # a fresh connection, like a request's + start = time.perf_counter() + fn() + return round((time.perf_counter() - start) * 1000 / n, 2) + + try: + for kb in [int(k) for k in args.metadata_kb.split(",")]: + document = metadata(kb) + for n in [int(t) for t in args.tables.split(",")]: + names = [f"bench_ds_{uuid.uuid4().hex[:10]}" for _ in range(n)] + tables = Table.objects.bulk_create( + Table(name=name, oemetadata=document, is_publish=i % 2 == 0) + for i, name in enumerate(names) + ) + UserPermission.objects.bulk_create( + UserPermission(holder=owner, table=table, level=WRITE_PERM) + for table in tables + ) + for topic in topics: + topic.tables.add(*tables) + dataset = Dataset.objects.create( + name=f"bench_ds_{uuid.uuid4().hex[:8]}", + metadata={"title": "Bench"}, + creator=owner, + ) + params = {"dataset": dataset.name} + problem = table_actions.preflight(owner, "dataset_add", names, params) + assert len(problem.eligible) == n, problem.left_out + + check = per_table( + lambda: table_actions.preflight( + owner, "dataset_add", names, params + ), + n, + ) + add = per_table( + lambda: table_actions.execute( + owner, "dataset_add", names, params, via="benchmark" + ), + n, + ) + assert dataset.tables.count() == n + remove = per_table( + lambda: table_actions.execute( + owner, "dataset_remove", names, params, via="benchmark" + ), + n, + ) + assert dataset.tables.count() == 0 + row = { + "run_utc": stamp, + "metadata_kb": kb, + "tables": n, + "check_per_table_ms": check, + "add_per_table_ms": add, + "remove_per_table_ms": remove, + } + results.append(row) + print( + "{metadata_kb:>4} KB x {tables:>4}: per Table check " + "{check_per_table_ms} ms, add {add_per_table_ms} ms, " + "remove {remove_per_table_ms} ms".format(**row) + ) + dataset.delete() + Table.objects.filter(name__in=names).delete() + finally: + runner.teardown_databases(old_config) + + if results and not args.no_results: + args.results.parent.mkdir(parents=True, exist_ok=True) + exists = args.results.exists() + with args.results.open("a", newline="", encoding="utf-8") as fh: + writer = csv.DictWriter(fh, fieldnames=list(results[0])) + if not exists: + writer.writeheader() + writer.writerows(results) + print(f"\nappended {len(results)} rows to {args.results}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/login/templates/login/partials/table_action_dataset.html b/login/templates/login/partials/table_action_dataset.html index 543394a65..f9c5ac99e 100644 --- a/login/templates/login/partials/table_action_dataset.html +++ b/login/templates/login/partials/table_action_dataset.html @@ -2,11 +2,23 @@ SPDX-FileCopyrightText: 2026 Jonas Huber © Reiner Lemoine Institut SPDX-License-Identifier: AGPL-3.0-or-later -The Dataset branch of the action dialog (table_action_dialog.html): which of -the user's own Datasets to add the eligible Tables to, or remove them from. -``check.datasets`` holds only Datasets the action can change, so a row's -choice is never wrong; the only one is chosen already. Without any, the -dialog says why and offers no confirmation. +The Dataset choice of the action dialog (table_action_dialog.html), above +its preview: which of the user's own Datasets to add the eligible Tables to, +or remove them from. ``check.datasets`` holds only Datasets the action can +change, so a row's choice is never wrong; the only one is chosen already. +Without any, the dialog says why and offers no confirmation. + +In a batch, which Tables are already in the chosen Dataset (add) or not in +it (remove) depends on the choice, so choosing asks the bulk preflight +again (#table-action-chooser, which the select's ``change`` bubbles up to), +with the whole selection (``selection``), and swaps only the preview and the +footer, plus the status line below the select: never the select itself, +which keeps focus. ``change`` fires on every arrow key in a closed select, +so the request waits until the choice has rested (``delay``) and a newer one +replaces an older one in flight (``hx-sync``). A row's choice needs no +re-check: its Datasets are those the action can change for that one Table. Confirm is disabled +while a re-check is on its way (the form's ``hx-disabled-elt``); one sent +before it would be refused whole by the server and the dialog re-run. {% endcomment %} {% if check.datasets %}

@@ -28,24 +40,39 @@ The table itself stays as it is. {% endif %}

-
- - - {% if errors.dataset %}
{{ errors.dataset }}
{% endif %} -
+ {% if check.total > 1 %} + {% comment %}A change of the select bubbles up to here.{% endcomment %} +
+ + {% include "login/partials/table_action_dataset_select.html" %} +

+ {% if check.dataset %} + {{ check.eligible|length }} of {{ check.total }} tables will be + {% if check.action == "dataset_add" %} + added to + {% else %} + removed from + {% endif %} + “{{ check.dataset.metadata.title|default:check.dataset.name }}”. + {% endif %} +

+
+ {% else %} +
+ {% include "login/partials/table_action_dataset_select.html" %} +
+ {% endif %} {% elif check.action == "dataset_add" %}

None of your datasets is left to add diff --git a/login/templates/login/partials/table_action_dataset_select.html b/login/templates/login/partials/table_action_dataset_select.html new file mode 100644 index 000000000..37ea6a34e --- /dev/null +++ b/login/templates/login/partials/table_action_dataset_select.html @@ -0,0 +1,23 @@ +{% comment %} +SPDX-FileCopyrightText: 2026 Jonas Huber © Reiner Lemoine Institut +SPDX-License-Identifier: AGPL-3.0-or-later + +The Dataset select of the action dialog's Dataset choice +(table_action_dataset.html), by title; the only choice is chosen already. +{% endcomment %} + + +{% if errors.dataset %}

{{ errors.dataset }}
{% endif %} diff --git a/login/templates/login/partials/table_action_delete.html b/login/templates/login/partials/table_action_delete.html index 42ed3cf23..f2524c006 100644 --- a/login/templates/login/partials/table_action_delete.html +++ b/login/templates/login/partials/table_action_delete.html @@ -7,33 +7,40 @@ (the Table's name for one published Table, the number of Tables for a batch holding a published Table or more than ten). A mismatch comes back as ``errors.confirm`` beside the field; the server is what enforces it. + +One Table is described by itself. A batch is counted: the dialog's list +above names every Table and marks the published ones, so this block says how +many are published, reviewed, in review or under embargo, and which Datasets +lose how many of them (other people's by owner, since their membership goes +silently). {% endcomment %} -

- This deletes the {{ check.total|pluralize:"table,tables" }} and {{ check.total|pluralize:"its,their" }} data for - good. It cannot be undone. -

-{% with c=check.consequences %} +{% with c=check.consequences n=check.eligible|length %} +

+ {% if n == 1 %} + This deletes the table and its data for good. + {% else %} + This deletes {{ n }} tables and their data for good. + {% endif %} + It cannot be undone. +

{% if c.published or c.own_datasets or c.others_datasets or c.reviewed or c.embargoed %}

What this breaks:

    {% if c.published %}
  • - {% if check.total == 1 %} + {% if n == 1 %} It is published: anyone may have found, cited or downloaded it. {% else %} - {{ c.published|length }} of them {{ c.published|length|pluralize:"is,are" }} published: - {% for table in c.published %} - {{ table.human_readable_name|default:table.name }}{% if not forloop.last %},{% endif %} - {% endfor %} + {{ c.published|length }} of them {{ c.published|length|pluralize:"is,are" }} published: anyone may have found, cited or downloaded {{ c.published|length|pluralize:"it,them" }}. {% endif %}
    • {% endif %} {% if c.own_datasets %}
  • Removed from your {{ c.own_datasets|length|pluralize:"dataset,datasets" }}: - {% for name in c.own_datasets %} - {{ name }}{% if not forloop.last %},{% endif %} + {% for name, leaving in c.own_datasets %} + {{ name }}{% if n > 1 %} ({{ leaving }} {{ leaving|pluralize:"table,tables" }}){% endif %}{% if not forloop.last %},{% endif %} {% endfor %}
    • {% endif %} @@ -41,29 +48,44 @@
  • Removed from {{ c.others_datasets|length|pluralize:"another person's dataset,other people's datasets" }}, without telling {{ c.others_datasets|length|pluralize:"its owner,their owners" }}:
      - {% for owner, name in c.others_datasets %} + {% for owner, name, leaving in c.others_datasets %}
    • - {{ name }} ({{ owner }}) + {{ name }} ({{ owner }}){% if n > 1 %}: {{ leaving }} {{ leaving|pluralize:"table,tables" }}{% endif %}
      • {% endfor %}
    • {% endif %} - {% for table, state in c.reviewed %} -
  • - {% if check.total > 1 %}{{ table.human_readable_name|default:table.name }}:{% endif %} - {{ state }}. The review stays on record without its table. -
    • - {% endfor %} - {% for table, until in c.embargoed %} -
  • - {% if check.total > 1 %}{{ table.human_readable_name|default:table.name }}:{% endif %} - Under embargo until {{ until|date:"j M Y" }}. -
    • - {% endfor %} + {% if n == 1 %} + {% for table, state in c.reviewed %} +
  • {{ state }}. The review stays on record without its table.
    • + {% endfor %} + {% for table, until in c.embargoed %} +
  • Under embargo until {{ until|date:"j M Y" }}.
    • + {% endfor %} + {% else %} + {% if c.reviewed %} +
  • + {% if c.finished_reviews %} + {{ c.finished_reviews }} reviewed{% if c.open_reviews %},{% endif %} + {% endif %} + {% if c.open_reviews %}{{ c.open_reviews }} in review{% endif %}. + The reviews stay on record without their tables. +
    • + {% endif %} + {% if c.embargoed %} +
  • + {{ c.embargoed|length }} {{ c.embargoed|length|pluralize:"is,are" }} under an embargo that has not ended yet. +
    • + {% endif %} + {% endif %} {% if c.knowledge_graph %}
  • - Links to {{ c.published|length|pluralize:"it,them" }} from the knowledge graph (scenario bundles) stop resolving. + {% if n == 1 %} + Links to it from the knowledge graph (scenario bundles) stop resolving. + {% else %} + Links to the {{ c.published|length }} published {{ c.published|length|pluralize:"table,tables" }} from the knowledge graph (scenario bundles) stop resolving. + {% endif %}
    • {% endif %}
@@ -73,7 +95,7 @@ {% if check.confirmation %}