Story #2438: Account Deletion - #2537
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a V3 account-deletion flow with confirmation, grace-period scheduling, cancellation, notification emails, site-wide status UI, and comprehensive user-data scrubbing while preserving legacy behavior outside V3. ChangesAccount deletion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant ProfilePage
participant DeleteUserView
participant ScheduledEmailTask
participant EmailTemplates
User->>ProfilePage: Confirm account deletion
ProfilePage->>DeleteUserView: POST confirmation
DeleteUserView->>ScheduledEmailTask: Enqueue scheduled-deletion email
ScheduledEmailTask->>EmailTemplates: Render text and HTML content
ScheduledEmailTask-->>User: Send cancellation instructions
DeleteUserView-->>ProfilePage: Redirect to scheduled-deletion state
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
users/management/commands/send_test_emails.py (1)
248-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider importing
POSTORIUS_URLto avoid duplication.This hardcodes the Postorius URL, duplicating the
POSTORIUS_URLconstant defined inusers/tasks.py. Consider importing it to keep the test command automatically in sync with the real task.♻️ Proposed refactor
First, add
from users.tasks import POSTORIUS_URLto the imports at the top of the file, then apply:- "postorius_url": "https://lists.boost.org/mailman3/lists/", + "postorius_url": POSTORIUS_URL,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@users/management/commands/send_test_emails.py` at line 248, Replace the hardcoded "postorius_url" value in the test email command with the existing POSTORIUS_URL constant by importing it from users.tasks, keeping the command synchronized with the task’s configured URL.users/views.py (1)
795-805: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelegate URL resolution to
get_success_url.For consistency with Django's class-based view lifecycle and how you cleanly implemented
CancelDeletionView, consider overridingget_success_urlto return the V3 URL, allowingsuper().form_valid(form)to naturally handle the redirect.♻️ Proposed refactor
+ def get_success_url(self): + if flag_is_active(self.request, "v3"): + return _v3_profile_edit_url() + return super().get_success_url() + def form_valid(self, form): user = self.get_object() user.delete_permanently_at = timezone.now() + datetime.timedelta( days=settings.ACCOUNT_DELETION_GRACE_PERIOD_DAYS ) user.save() if flag_is_active(self.request, "v3"): tasks.send_account_deletion_scheduled_email.delay( email=user.email, first_name=user.first_name, grace_days=settings.ACCOUNT_DELETION_GRACE_PERIOD_DAYS, login_url=self.request.build_absolute_uri(reverse("account_login")), scheme=self.request.scheme, host=self.request.get_host(), ) - return HttpResponseRedirect(_v3_profile_edit_url()) return super().form_valid(form)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@users/views.py` around lines 795 - 805, Move the V3 redirect selection from the inline return in the form-valid flow into an override of get_success_url, returning _v3_profile_edit_url() when flag_is_active(self.request, "v3") and otherwise delegating to the parent implementation. Keep the deletion email scheduling in form_valid, then call super().form_valid(form) for the standard redirect lifecycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@templates/v3/users/_delete_account_modal.html`:
- Around line 35-74: Update the Alpine validation in the form using confirmText
so comparison trims surrounding whitespace, matching Django’s
CharField(strip=True) behavior. Add submit handling to prevent the form’s
default submission when the trimmed confirmation phrase is invalid, while
allowing valid confirmations to submit normally.
---
Nitpick comments:
In `@users/management/commands/send_test_emails.py`:
- Line 248: Replace the hardcoded "postorius_url" value in the test email
command with the existing POSTORIUS_URL constant by importing it from
users.tasks, keeping the command synchronized with the task’s configured URL.
In `@users/views.py`:
- Around line 795-805: Move the V3 redirect selection from the inline return in
the form-valid flow into an override of get_success_url, returning
_v3_profile_edit_url() when flag_is_active(self.request, "v3") and otherwise
delegating to the parent implementation. Keep the deletion email scheduling in
form_valid, then call super().form_valid(form) for the standard redirect
lifecycle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f61579b0-8bf1-46f7-911f-cb944465ec6e
📒 Files selected for processing (15)
static/css/v3/account-deletion-banner.cssstatic/css/v3/components.cssstatic/css/v3/delete-account-modal.csstemplates/base.htmltemplates/emails/account_deletion_scheduled.htmltemplates/emails/account_deletion_scheduled.txttemplates/emails/account_deletion_scheduled_subject.txttemplates/v3/includes/_account_deletion_banner.htmltemplates/v3/user_profile_edit.htmltemplates/v3/users/_delete_account_modal.htmlusers/management/commands/send_test_emails.pyusers/models.pyusers/tasks.pyusers/tests/test_delete_account.pyusers/views.py
d0bff96 to
0c0e230
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
users/views.py (1)
795-810: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep an existing deletion schedule immutable.
A second valid POST overwrites
delete_permanently_atand queues another email, allowing the 10-day deadline to be extended indefinitely. For V3, atomically schedule only when the field is null; otherwise redirect without changing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@users/views.py` around lines 795 - 810, Update form_valid to preserve an existing delete_permanently_at value: for V3, atomically set the deletion deadline only when the field is null, and queue send_account_deletion_scheduled_email only when that update succeeds. If a schedule already exists, redirect without modifying the deadline or sending another email.
🧹 Nitpick comments (1)
static/css/v3/delete-account-modal.css (1)
158-161: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve the form in the accessibility tree.
display: contentscan cause browsers to omit<form>semantics from accessibility trees. This form is the cancellation action rendered bytemplates/v3/user_profile_edit.html:160-210; use a normal flex item such asdisplay: flex, then verify the supported browser/screen-reader matrix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/css/v3/delete-account-modal.css` around lines 158 - 161, Update the .user-profile__delete-inline-form rule to use a normal flex layout item, such as display: flex, instead of display: contents, preserving the form’s semantics in the accessibility tree while retaining the existing cancellation-action styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@users/views.py`:
- Around line 153-154: Update the comment describing the delete-account card
flow to remove the obsolete “delete-now” control, leaving only the cancellation
behavior available after scheduling.
- Around line 800-810: Update the v3 account-deletion flow around user.save()
and send_account_deletion_scheduled_email.delay() to use a durable transactional
outbox or equivalent retryable post-commit publisher. Persist the deletion
schedule and email event atomically, publish only after commit, and ensure
broker failures are retried without leaving the account scheduled without its
confirmation email; preserve the existing redirect behavior.
---
Outside diff comments:
In `@users/views.py`:
- Around line 795-810: Update form_valid to preserve an existing
delete_permanently_at value: for V3, atomically set the deletion deadline only
when the field is null, and queue send_account_deletion_scheduled_email only
when that update succeeds. If a schedule already exists, redirect without
modifying the deadline or sending another email.
---
Nitpick comments:
In `@static/css/v3/delete-account-modal.css`:
- Around line 158-161: Update the .user-profile__delete-inline-form rule to use
a normal flex layout item, such as display: flex, instead of display: contents,
preserving the form’s semantics in the accessibility tree while retaining the
existing cancellation-action styling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cff9f727-04f9-41bc-82c3-027656704883
📒 Files selected for processing (15)
static/css/v3/account-deletion-banner.cssstatic/css/v3/components.cssstatic/css/v3/delete-account-modal.csstemplates/base.htmltemplates/emails/account_deletion_scheduled.htmltemplates/emails/account_deletion_scheduled.txttemplates/emails/account_deletion_scheduled_subject.txttemplates/v3/includes/_account_deletion_banner.htmltemplates/v3/user_profile_edit.htmltemplates/v3/users/_delete_account_modal.htmlusers/management/commands/send_test_emails.pyusers/models.pyusers/tasks.pyusers/tests/test_delete_account.pyusers/views.py
🚧 Files skipped from review as they are similar to previous changes (10)
- static/css/v3/components.css
- static/css/v3/account-deletion-banner.css
- templates/emails/account_deletion_scheduled.txt
- templates/v3/user_profile_edit.html
- templates/emails/account_deletion_scheduled_subject.txt
- templates/base.html
- users/management/commands/send_test_emails.py
- users/tasks.py
- users/models.py
- users/tests/test_delete_account.py
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Hi @herzog0 ! The core flow of deletion and canceling the deletion works great!
I've left a couple of suggestions and a question below related to legacy behaviour. Asides from those, might you be able to update the UI to more closely match the Figma design as well (e.g. with font sizes, text colors, and the Delete button should have a solid red background, etc) 🙏
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
users/models.py (1)
454-465: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDelete the derived HQ ImageKit cache before anonymization succeeds.
hq_image_renderis cached separately fromhq_image, butdelete_account()only clearsimage_thumbnailand deletes the source file after commit. ImageKit cache files remain on storage/cache even after the source file is deleted, so also deletehq_image_renderforextended_scrub=True. Since cleanup happens inon_commit, handle failures so a storage/cache delete error does not leave anonymized PII committed, and do not let it block later callbacks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@users/models.py` around lines 454 - 465, The account deletion flow around delete_account and the extended_scrub image cleanup must also remove the derived hq_image_render cache. Schedule that cache deletion in the existing transaction.on_commit cleanup, handle deletion failures so they prevent anonymized PII from being committed without blocking subsequent callbacks, and preserve the existing deferred source-file cleanup behavior.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@users/models.py`:
- Line 417: Update delete_account and the scheduled-deletion flow to persist and
reuse the user’s V3 extended-scrub choice. Ensure the grace-period task invokes
delete_account with extended_scrub=True for users requiring V3 scrubbing, while
preserving the existing behavior for other users.
In `@users/tasks.py`:
- Around line 84-89: Update the scheduling flow to persist whether each deletion
request requires the V3/extended scrub, then have the task query that marker and
pass the corresponding extended_scrub value to User.delete_account() instead of
always defaulting to False. Preserve legacy behavior for records without the
marker while ensuring V3-scheduled accounts receive the extended scrub.
---
Outside diff comments:
In `@users/models.py`:
- Around line 454-465: The account deletion flow around delete_account and the
extended_scrub image cleanup must also remove the derived hq_image_render cache.
Schedule that cache deletion in the existing transaction.on_commit cleanup,
handle deletion failures so they prevent anonymized PII from being committed
without blocking subsequent callbacks, and preserve the existing deferred
source-file cleanup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dddff95-6c7e-47bb-9850-ff1829d3f07b
📒 Files selected for processing (10)
static/css/v3/user-profile-page.csstemplates/emails/account_deletion_scheduled.htmltemplates/emails/account_deletion_scheduled.txttemplates/emails/base_email.htmltemplates/v3/user_profile_edit.htmltemplates/v3/users/_delete_account_modal.htmlusers/models.pyusers/tasks.pyusers/tests/test_delete_account.pyusers/views.py
💤 Files with no reviewable changes (2)
- templates/emails/account_deletion_scheduled.txt
- templates/v3/users/_delete_account_modal.html
🚧 Files skipped from review as they are similar to previous changes (5)
- static/css/v3/user-profile-page.css
- templates/emails/account_deletion_scheduled.html
- templates/v3/user_profile_edit.html
- users/views.py
- users/tests/test_delete_account.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/css/v3/delete-account-modal.css`:
- Around line 155-164: Update the destructive submit selectors for
.delete-account-modal .btn-error so the solid red fill and reversed text apply
only when the button is not disabled, while preserving the existing shared
disabled outlined styling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17026d2f-f841-4a34-8521-c4036d0ebcbb
📒 Files selected for processing (3)
static/css/v3/delete-account-modal.cssstatic/css/v3/semantics.csstemplates/v3/users/_delete_account_modal.html
💤 Files with no reviewable changes (1)
- templates/v3/users/_delete_account_modal.html
Totally missed those, thanks Julia. Addressed!
javiercoronadonarvaez
left a comment
javiercoronadonarvaez
left a comment
There was a problem hiding this comment.
LGTM! Sorry for the delay. I think Julia addressed everything worth noting.
Has one conflict, but other than that, it's ready to go.
…ML unsubscribe, top banner, accurate copy
… link to Postorius
…eed dividers, disabled submit until confirmed
chore: drop comments from delete-account modal styles
cdfaa70 to
bc371d9
Compare
3 participants
Issue: #2438
Summary & Context
Reworks the account-deletion flow for the V3 design system. Deletion is now driven by a confirmation modal (matching Figma) that schedules the account for anonymization after a grace period, with a site-wide banner, an in-modal confirmation error, and a confirmation email. Legacy (non-V3) behaviour is left byte-identical - every change is gated behind the
v3Waffle flag.Changes
_delete_account_modal.html+delete-account-modal.css) built to the Figma spec: white rounded inner card on a light-red canvas, full-bleed section dividers, fixed section spacing, and a destructive submit button that stays disabled (via Alpine) until the exact phrasedelete my accountis typed. No-JS fallback: the button stays enabled and the server validates.User.delete_account()preserves the row so authored content stays attributed to an anonymized user); extended the scrub to cover profile links, GitHub username, avatar images, badges, and login-method flags.UserMailingListSubscriptionrows are deleted to remove stored-email PII, but the Mailman/Postorius API is deliberately not called - list membership is left for the user to manage in Postorius (linked from the modal)._account_deletion_banner.html+account-deletion-banner.css) that pins to the top above the navbar and pushes content down, with a "Cancel deletion" action (no "Delete now").emails/account_deletion_scheduled.*) via a Celery task, explaining what happens, how to cancel, and linking to Postorius; registered insend_test_emailsfor previews.?delete_error=1).test_delete_account.pycovering PII scrub, linked-record removal, no-Mailman-API deletion, idempotency, V3 schedule/cancel redirects, the scheduling email, inline error rendering, and that legacy behaviour is unchanged.Please list any potential risks or areas that need extra attention during review/testing
User.delete_account()anonymizes rather than deletes the row - confirm authored content (news, library authorship) stays attributed to the anonymized user and no PII leaks when the Wagtail integration is in place.v3flag; with the flag off, the flow must match current production (legacy banner, legacy delete pages, success/error message banners) exactly..delay()).Screenshots
Peer-review testing steps
v3Waffle flag./users/me/?edit=trueand open the "Delete account" modal. Confirm the layout matches Figma (white card on light-red, full-bleed dividers, small bullets) in light and dark mode.delete my accountexactly (wrong case / trailing space keep it disabled).v3flag off and confirm the legacy flow (legacy red banner with "Delete Now", legacy delete pages, and the success/error message banners) is unchanged.Self-review Checklist
Frontend
Summary by CodeRabbit
New Features
Bug Fixes
Tests