fix(soft-delete): name the recovery location in the archive confirmation - #43401
fix(soft-delete): name the recovery location in the archive confirmation#43401mikebridge wants to merge 2 commits into
Conversation
dosubot
Bot
added
change:frontend
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43401 +/- ##
=======================================
Coverage 66.81% 66.81%
=======================================
Files 2876 2876
Lines 164454 164466 +12
Branches 37960 37961 +1
=======================================
+ Hits 109873 109885 +12
Misses 52388 52388
Partials 2193 2193
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:
|
The archive confirmation told users their chart, dashboard, or dataset moves to Recently Archived without saying where that is. The design of record (SC-117152, Figma 2209-2689) includes the location; the app omitted it. The body now reads '...moved to Recently Archived in the Settings menu' across all four variants (singular/plural, with and without a retention window). Archive terminology is confirmed as the copy of record -- the design node's 'Delete / Recently Deleted' wording is the stale side and is being updated separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1a7fa36 to
b82bc13
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| 'These %(type)s will be moved to Recently Archived in the Settings menu. You can recover them there within %(days)s days.', | ||
| { type: typeLabel, days }, |
There was a problem hiding this comment.
Suggestion: The retention setting accepts a valid one-day window, but this message always uses the plural unit “days”, producing “within 1 days.” Use singular/plural-aware translation variants so the confirmation remains grammatically correct for a one-day retention period. [logic error]
Severity Level: Minor 🧹
- ⚠️ Archive confirmations display incorrect singular grammar.
- ⚠️ Affects one-day chart, dashboard, and dataset retention windows.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/utils/softDeleteCopy.ts
**Line:** 69:70
**Comment:**
*Logic Error: The retention setting accepts a valid one-day window, but this message always uses the plural unit “days”, producing “within 1 days.” Use singular/plural-aware translation variants so the confirmation remains grammatically correct for a one-day retention period.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 1dbf497: the timed variants now pluralize on the day count with tn() (each case remains one complete translation unit for translators), messages.pot carries the matching msgid/msgid_plural pairs (validated with msgfmt --check-format), and the copy tests pin the one-day singular for both the single and bulk variants.
Comment by Claude (AI) on behalf of @mikebridge.
|
The flagged issue is correct. The current implementation uses a hardcoded "days" string, which results in grammatically incorrect output when the retention period is one day. To resolve this, you should use the Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/src/utils/softDeleteCopy.ts |
Code Review Agent Run #656bd9Actionable 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
The existing one-day pluralization thread looks valid: SOFT_DELETE_RETENTION_DAYS accepts 1, while both timed confirmation variants currently produce “within 1 days.” Could we switch those variants to tn() and add a days = 1 expectation before merging?
I found no additional substantive issues at b82bc13; CI is green.
…rmation SOFT_DELETE_RETENTION_DAYS accepts 1, and both timed confirmation variants hardcoded "days", producing "within 1 days". The timed variants now pluralize on the day count with tn(), each case still a single complete translation unit; messages.pot carries the matching msgid/msgid_plural pairs (msgfmt --check-format clean). Adds the one-day singular expectation to the copy tests. Addresses the review thread on apache#43401. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@richardfogaca Done in 1dbf497 — both timed variants switched to Comment by Claude (AI) on behalf of @mikebridge. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #f4220cActionable 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 |
2 participants
SUMMARY
With soft delete enabled, the archive confirmation told users their chart, dashboard, or dataset "will be moved to Recently Archived" without saying where that is. QA (SC-117152, TC-099/TC-101) compared the modal against the design of record and found the recovery-location hint missing — the one genuine usability gap among the diffs: a user reading only the modal had no way to know recovery lives under Settings → Manage → Recently Archived.
The confirmation body now names the location in all four variants (singular/plural, with and without a configured retention window):
Each variant remains a single, complete translation unit; only the noun and day count are interpolated. The four corresponding
messages.potmsgids are updated in step.Terminology decision (recorded on the ticket): "Archive / Recently Archived" is confirmed as the copy of record — it was chosen deliberately during the soft-delete rollout to keep recoverable deletion distinct from the "Delete permanently" purge action offered on the recovery page. The design node's "Delete / Recently Deleted" wording is the stale side and is being updated separately by design; that work does not block this change.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Copy-only change; QA's before screenshots are attached to [SC-117152]. Before/after body text:
TESTING INSTRUCTIONS
SOFT_DELETE.npm run test -- src/utils/softDeleteCopy.test.ts— 9 tests, expectations updated to the new copy.ADDITIONAL INFORMATION
SOFT_DELETE(copy renders only with the flag on; flag-off copy unchanged)This PR was developed with AI assistance (Claude Code); a human (@mikebridge) reviews before undrafting.
🤖 Generated with Claude Code