fix(soft-delete): card-view chart delete shows the archive dialog - #43469
Conversation
With SOFT_DELETE on, the Charts card view's delete confirmation still showed the permanent-delete dialog (destructive styling, type-DELETE friction, hardcoded copy) while the list view showed the archive dialog for the identical, recoverable action. The card's ConfirmStatusChange now mirrors the list view: recoverable under the flag, "Archive [name]?" title, body from the shared archiveConfirmDescription helper, and the kebab entry labeled via deleteActionLabel. Flag-off rendering is unchanged and pinned by a test; no new translation units (all strings already exist for the list view). SC-118046. Blocked deletions keep surfacing after confirm via the shared handler; the list view's pre-flight blocker listing is deliberately not adopted (scope decision recorded in the spec). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
change:frontend
Code Review Agent Run #0d7acfActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43469 +/- ##
===========================================
+ Coverage 57.39% 78.96% +21.56%
===========================================
Files 2878 2878
Lines 165181 165188 +7
Branches 38168 38174 +6
===========================================
+ Hits 94812 130437 +35625
+ Misses 69473 32299 -37174
- Partials 896 2452 +1556
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Review — verified at head f1a23726 (author's real work is abc5ca70; the head commit is a GitHub "Update branch" merge, not hand-written code).
This is a clean, well-scoped fix. I verified every claim in the description against the code at the PR head rather than trusting the summary, and it holds up. Concretely:
The recoverable prop genuinely does the work (not just a title swap)
ConfirmStatusChange forwards recoverable straight to DeleteModal, and DeleteModal (packages/superset-ui-core/src/components/DeleteModal/index.tsx) is the thing that actually behaves:
showConfirmationInput = !recoverable→ thedata-test="delete-modal-input"type-DELETE block is not rendered;disablePrimaryButton = false→ the confirm button is live immediately;primaryButtonName = t('Archive'),primaryButtonStyle = 'primary'(not'danger').
So the modal really "does it on its own" — the card just has to pass the flag. Good separation.
Parity with the list view is faithful
ChartCard's new call mirrors ChartList's single-delete DeleteModal idiom exactly:
- title:
softDelete ? t('Archive %(name)s?', { name: slice_name }) : t('Please confirm')— identical construction topages/ChartList/index.tsx; - body soft-delete branch:
<p>{archiveConfirmDescription(t('chart'))}</p>— the same shared helperChartList'sChartArchiveDescriptionuses; - kebab label:
deleteActionLabel()— same helperChartListuses for its row/bulk labels.
The one intentional divergence is the pre-flight blocking-alerts/reports enumeration (ChartList fetches related objects and lists them inside the dialog; the card does not). See the note below.
No new translation units
archiveConfirmDescription() and deleteActionLabel() live in src/utils/softDeleteCopy.ts and are flag-aware; Archive %(name)s? and the "moved to Recently Archived" strings are already extracted in superset/translations/messages.pot (via ChartList), and messages.pot is not in the diff. No new t() literal is introduced. Claim confirmed.
Flag-off is byte-identical
Read, not trusted: the softDelete ? … : … false-branches reproduce the prior title (Please confirm), prior description (Are you sure you want to delete <b>{slice_name}</b>? — the trailing ? moves to its own line but JSX trims the newline, so the rendered DOM is unchanged), and deleteActionLabel() returns t('Delete') when the flag is off. Rendered output matches master exactly.
Permission gating untouched
canDelete = hasPerm('can_write') and the isUserEditorOrAdmin(...) / allowEdit gate are unchanged; the entire edit lives inside the existing if (canDelete) block and only alters modal props + the label. onConfirm still calls handleChartDelete(...) with the same arguments.
Tests
INSPECTED (frontend node_modules is absent in my environment, so I did not run Jest — I did not fake a run). Reasoning about the two new tests under a mental revert of only ChartCard.tsx:
- Flag-ON test ("shows the archive dialog") is the real coverage: on revert the kebab label reverts to
Delete, sofindByText('Archive')fails — as do theArchive Sample Chart?title, the shared-body prefix, theArchivebutton, and thedelete-modal-inputabsence assertions. It fails on revert → genuinely covers the change. - Flag-OFF test is a regression guard for flag-off behavior; by construction it passes with and without the production edit. It correctly is not evidence the fix works, and it is not vacuous either.
Fixtures line up (mockChart.slice_name = 'Sample Chart' → Archive Sample Chart?; chart-card-menu testid exists on the KebabMenuButton).
One thing worth a follow-up (non-blocking): visual parity without behavioral parity
This is the most interesting consequence of the change and it's worth stating plainly for the record. The card's dialog now looks identical to the list's, but on a blocked deletion it behaves differently: the list enumerates the blocking alerts/reports in the dialog before you confirm (its openChartDeleteModal does a pre-flight lookup feeding ChartArchiveDescription), whereas the card omits that lookup — the blocking reason only surfaces after confirming, via the error toast from handleChartDelete.
I don't consider this a blocker:
- It's a strict improvement over the prior state — before this PR the card dialog was also missing the pre-flight enumeration; the only thing that changes is the (correct) archive framing.
- The blocking reason is still surfaced, just post-confirm.
But it's a real, if subtle, trade: making the two dialogs look the same arguably makes the behavioral divergence harder to notice than when they looked different. I'd suggest a follow-up to give the card the same pre-flight enumeration (or to extract the shared description-plus-lookup into one component both views render), rather than leaving the two dialogs to drift. Flagging, not gating.
Sibling observation (out of scope, informational)
The description says the dashboards card view is unaffected. Worth noting the dashboard single-delete modal — the one both the list row and the card's onDelete open via setDashboardToDelete in pages/DashboardList/index.tsx — is not soft-delete-aware at all: it hardcodes title={t('Please confirm')}, the permanent-delete copy, and no recoverable prop (only the bulk dashboard action was wired for archive). So under SOFT_DELETE, dashboards still show the permanent-delete dialog for single deletes in both views — the same defect this PR fixes for charts. Not this PR's job, but a candidate for the sibling story.
CI at review time (statusCheckRollup, grouped by __typename|status|conclusion): 63 CheckRun SUCCESS + 1 StatusContext SUCCESS, 4 NEUTRAL, 9 SKIPPED, 1 IN_PROGRESS, zero failures.
Bottom line: the fix is correct, faithfully mirrors the list view, leaves flag-off and permission gating untouched, and is covered by a test that fails on revert. From my side this is ready for a committer to approve; the pre-flight-parity item above is a reasonable follow-up rather than a blocker. (Leaving this as a comment — I'm not a committer on this repo.)
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
With
SOFT_DELETEenabled, the Charts card view's delete confirmation still showed the permanent-delete dialog — "Please confirm" / "Are you sure you want to delete [name]?", destructive styling, and the type-DELETE friction — while the list view shows the archive dialog for the identical, recoverable action. The same click archived the chart either way; only the card's dialog misstated what was about to happen.The card's
ConfirmStatusChangenow mirrors the list view's idiom (ChartList): under the flag it passesrecoverable(the underlyingDeleteModalthen drops the type-DELETE input and renders a primary-styled Archive button on its own), titles the dialog "Archive [name]?", sources the body from the sharedarchiveConfirmDescription()helper, and labels the kebab entry viadeleteActionLabel(). With the flag off, rendering is unchanged and pinned by a test.Design notes:
messages.potis untouched.handleChartDelete, permission gating (isUserEditorOrAdmin), and toasts are untouched.This is a pre-existing inconsistency from the soft-delete rollout, not a regression:
ChartCardpredates the flag and was never wired to it. The Dashboards card view is unaffected (it delegates to its list page's flag-aware dialog) and datasets have no card view.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Card view,
SOFT_DELETEenabled, same chart (live dev stack, default 30-day retention window):Before — a permanent-delete dialog for a recoverable action (type-DELETE friction, no recovery information):
After — the archive dialog, identical to the list view's for the same chart (shared copy, primary Archive button, no type-to-confirm):
Flag off: byte-identical to before this change (pinned by test).
TESTING INSTRUCTIONS
5 tests: the 3 pre-existing card tests plus the archive-dialog assertions (title, Archive button, shared body, no type-DELETE input) and a flag-off pinning test.
Manual: enable
SOFT_DELETE, Charts page → card view → kebab (⋮) on any chart → entry reads Archive; clicking it opens the archive dialog matching the list view's. Disable the flag → entry reads Delete and the dialog is the unchanged permanent-delete one.ADDITIONAL INFORMATION
SOFT_DELETE(flag-off behavior unchanged)This PR was developed with AI assistance (Claude Code), including the implementation and tests; a human (@mikebridge) reviews before merge.