Skip to content

fix(ui): say why a blocked delete failed instead of printing the error repr - #16566

Merged
blakeaowens merged 2 commits into
devfrom
fix/classic-delete-warning-message
Oct 27, 2026
Merged

blakeaowens merged 2 commits into
devfrom
fix/classic-delete-warning-message

Conversation

@blakeaowens

@blakeaowens blakeaowens commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Description

When a delete is blocked by records that still refer to the row (a RestrictedError), three UI delete views put str() of the error in their warning: deleting a user, an environment, and a product platform, lifecycle or origin. str() of that exception is a Python tuple repr naming every referencing row, so a user referenced by many findings got a very long banner listing all of them:

User cannot be deleted: ("Cannot delete some instances of model 'Dojo_User' because they are referenced through restricted foreign keys: ...", {<Finding: ...>, <Finding: ...>, ...})

The warning now gives the reason as the API's answer to a blocked delete does: how many records still refer to the row and which kinds they are.

User cannot be deleted: 3 other records still refer to it. Records that refer to it: finding, product.

The API's sentence moves out of the exception handler into dojo.utils.restricted_delete_message, unchanged, and dojo.utils.restricted_delete_reason gives the views the reason alone (their warning already says what cannot be deleted); both share the count and the kinds. One record reads in the singular, and an error that carries no records reads other records still refer to it. The environment and product attribute warnings were f-strings and are now marked for translation; the user warning keeps its existing message id.

Test results

New unittests/test_classic_delete_warning_message.py: a blocked user, environment and product platform delete each shows one warning with the count and kinds, naming no row; one record reads in the singular, and no records still reads. The three view tests failed before the change with the repr. unittests.test_classic_delete_warning_message, test_apiv2_restricted_delete_message, test_product_attributes, test_async_delete, test_apiv2_user, test_apiv2_user_delete_authz, test_apiv2_delete_preview, test_apiv2_export, test_finding_helper, test_sample_data, api_v3.test_apiv3_users, authorization.test_authorization and test_rest_framework: 1114 tests, all pass. After the wording change, the delete-message, product attribute and v2 user suites (31 tests) pass again. ruff check passes on the changed files.

Documentation

None needed: no page documents these warnings.

Checklist

  • Make sure to rebase your PR against the very latest dev.
  • Submit all PRs, features and bug fixes alike, against the dev branch.
  • Give a meaningful name to your PR, as it may end up being used in the release notes.
  • Your code is Ruff compliant (see ruff.toml).
  • Your code is python 3.13 compliant.
  • Add applicable tests to the unit tests.

🤖 Generated with Claude Code

…r repr

The user, environment and product attribute delete views put str() of a
RestrictedError in their warning, a tuple repr naming every referencing row.
They now use the same sentence the API answers a blocked delete with: how
many records still refer to the row and which kinds they are. The sentence
moves to dojo.utils.restricted_delete_message so both share it, and the
environment and product attribute warnings are marked for translation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@blakeaowens

Copy link
Copy Markdown
Contributor Author

Adversarial review: full tier, Opus

Tier: full (backend change to Classic UI delete views plus a refactor of the API exception handler's 409 body). Confirmed against the diff: 6 files, no migration, no new endpoint, no permission change.

CI read (at 7970a9e): 28 pass, 19 pending (Rest Framework Unit Tests, User Interface Tests, k8s, performance), none red. ruff-linting passed.

Regression test: verified. With the three view files taken back to origin/dev and everything else at HEAD, all three tests in unittests/test_classic_delete_warning_message.py fail with the tuple repr in the banner ('secret' unexpectedly found in 'User cannot be deleted: ("Cannot delete some instances of model ...", {<Finding: secret finding one>, ...})'). At HEAD they pass, together with the 5 tests in test_apiv2_restricted_delete_message.py (8/8, run against Postgres 16 in a throwaway container from an exported copy of the tree).

What I drove: the three Classic delete views through the Django test client, against real RESTRICT foreign keys instead of a mocked delete(), so the error is the one Django's collector actually raises.

Break attempts

  1. Real restricted rows, user. A user who reports 3 findings (one of which also has them as mitigated_by) and leads an engagement. Banner: User cannot be deleted: It cannot be deleted while 4 other records still refer to it. Records that refer to it: engagement, finding. One warning, 302 back to the list, user still in the DB. The finding that references the user twice is counted once (the collector's set dedupes instances), which is the right count. Pass.
  2. Real restricted rows, environment and platform. An environment used by a test, a platform used by a product. Environment cannot be deleted: ... 1 other record ... Records that refer to it: test. and Platform cannot be deleted: ... Records that refer to it: product. One warning each, rows kept. Pass. The mocked shape in the new tests (a set of model instances as restricted_objects) matches what the real collector produced here, so the tests are not mocking a shape the code never sees.
  3. No row leaks. No title, <Finding ...> repr or id in any banner, real or mocked. Pass.
  4. Another language. Under translation.override("de") the helper returns English: the new sentences and the two new prefixes have no entries in dojo/locale/*/django.po. Before this PR the banner body was an English repr anyway, so nothing regresses, and the catalogs in this repo are evidently regenerated in bulk rather than per PR (the API sentences from the earlier PR are not in them either). Not a finding.
  5. API 409 body unchanged. The handler now delegates to restricted_delete_message; the existing test_apiv2_restricted_delete_message.py passes unchanged against the moved helper, and no other caller references the removed ngettext import. Pass.
  6. Reading the copy in place (finding R1). See below.

Findings

  • R1 (nit, this-pr) dojo/user/ui/views.py:506, dojo/development_environment/ui/views.py:86, dojo/product_attributes/ui/views.py:127: the prefix and the reused API sentence say the same thing twice, with a capital after the colon: "User cannot be deleted: It cannot be deleted while 4 other records still refer to it." The API sentence stands alone in a 409 body, but behind a "X cannot be deleted:" prefix it reads as a stutter. Suggest giving the helper a reason-only form for the banners (for example "4 other records still refer to it (engagement, finding).") while the API keeps its full sentence, or dropping the prefix in the three views and leading with the label in the sentence. Reproduces only on this PR (the base banner is the repr).

Nothing else found. Not findings: the %(error)s vs %(reason)s placeholder names (the user msgid was kept on purpose so its translations survive), and the untranslated cfg["label"] / breadcrumb f-strings in product_attributes/ui/views.py (pre-existing, outside the changed lines, and the same pattern as the existing "%(label)s added successfully.").

Not checked

  • No screenshots: this is a Classic UI flash message and the change is fully visible in the strings above; images stay out of this repo's PR threads.
  • The pending Rest Framework and User Interface suites; no Selenium test asserts on these banners (grep over tests/), so they should not move.
  • Production-scale counts: the collector already materializes every restricting row before the view sees the error, so the helper adds one len() and a set of verbose names; no new query.

Recommendation

Request changes, for R1 only. The fix is correct, the regression test fails without it, and the real-row paths behave. Once R1 is resolved (fixed or answered invalid: <reason>), this is ready.

"User cannot be deleted: It cannot be deleted while ..." said the same thing
twice. The Classic UI warnings now use restricted_delete_reason, which counts
the referencing records and names their kinds without restating that the row
cannot be deleted; the API keeps its full sentence.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@blakeaowens

Copy link
Copy Markdown
Contributor Author

Review resolution

  • R1 fixed in 3f685cb: the Classic UI warnings use the new restricted_delete_reason, so they read User cannot be deleted: 4 other records still refer to it. Records that refer to it: engagement, finding. without restating that the row cannot be deleted; the API keeps its full sentence. The singular and no-records forms have tests too.

@blakeaowens
blakeaowens added this pull request to the merge queue Oct 27, 2026
Merged via the queue into dev with commit 8b575ec Oct 27, 2026
49 checks passed
@blakeaowens
blakeaowens deleted the fix/classic-delete-warning-message branch October 27, 2026 07:36
@defectdojo-release-tracker defectdojo-release-tracker Bot modified the milestones: 3.5.0, 3.4.302 Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants