Story 2436: Mailing List Subscription Post Auth#2475
Conversation
a2851ad to
a7c3795
Compare
75d904c to
fcfd84e
Compare
a7c3795 to
d4c3e88
Compare
fcfd84e to
6bc71c1
Compare
julioest
left a comment
There was a problem hiding this comment.
Works as expected on my end ✅
d4c3e88 to
6a64a43
Compare
c722aa4 to
bd2f4c3
Compare
99178e5 to
879e91d
Compare
bd2f4c3 to
abb2720
Compare
|
Works great, approving :) Should the modal be disimissable with Esc key? I noticed that pattern in other modals with aria-label="Close", it does not work here so maybe that attribute that dialog.js uses to bind the Esc key is missing? |
879e91d to
2c676fc
Compare
abb2720 to
d62840d
Compare
📝 WalkthroughWalkthroughAdds a post-login mailing list subscription modal. Login now sets a session flag, the homepage consumes it to render the modal, and a new authenticated subscribe endpoint handles submissions. Dialog behavior, template markup, and CSS were updated to support the new modal flow. ChangesPost-auth mailing list modal
Sequence DiagramsequenceDiagram
participant Browser
participant HomepageView
participant PostAuthSubscribeView
participant _subscribe_pending
participant UserMailingListSubscription
Browser->>HomepageView: GET homepage after login
HomepageView->>HomepageView: consume show_ml_post_auth_modal
HomepageView->>HomepageView: save ml_post_auth_seen and build modal context
HomepageView-->>Browser: render page with post-auth modal
Browser->>PostAuthSubscribeView: POST subscription form
PostAuthSubscribeView->>_subscribe_pending: request, user, email, list_ids
_subscribe_pending->>UserMailingListSubscription: upsert PENDING rows
_subscribe_pending-->>PostAuthSubscribeView: succeeded IDs and error state
PostAuthSubscribeView-->>Browser: HTMX no-op or redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)mailing_list/views.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path static/js/dialog.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path 🔧 Stylelint (17.14.0)static/css/v3/dialog.cssError: ENOENT: no such file or directory, open '/.stylelintrc.json' 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 |
a9e7cdf to
c8e16c5
Compare
83e72a6 to
1bb6fe2
Compare
c8e16c5 to
3fb8455
Compare
61a5bfe to
f5b0031
Compare
3fb8455 to
b7f1334
Compare
feat: inject post-auth modal context in HomepageView feat: add PostAuthSubscribeView for post-login modal feat: add post-auth mailing list modal template feat: add post-auth modal CSS feat: wire post-auth modal into v3 homepage fix: correct post-auth modal width, email field, and button padding fix: use $refs for checkbox container to fix unselect all fix: drop only from modal include so CSRF token is available refactor: extract _subscribe_pending helper to remove duplicate sub logic feat: toggle select all / unselect all button based on checked state fix: add missing bottom padding below select/unselect all row fix: title padding fix: correct email field text color and font size to match text field spec fix: revert email field color and font-size to match figma spec fix: colors and radii fix: remove aria-hidden from backdrop links to prevent focus conflict warning fix: rebase fixes
b7f1334 to
3061f4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@mailing_list/views.py`:
- Around line 695-707: The HTMX branch in the subscription flow is returning an
empty response even when `_is_rate_limited(request)` blocks the action or
`_subscribe_pending()` fails, which causes the modal to close on a silent
failure. Update the subscription handler around the `list_ids`,
`_is_rate_limited`, and `_subscribe_pending` logic so it detects these failure
cases, keeps the modal open, and returns an error state/message instead of
`HttpResponse("")` when the subscription did not succeed. Ensure the
`_is_htmx(request)` response path only dismisses the modal after a successful
subscribe.
- Around line 95-117: The rollback in the confirmation-email failure path is
deleting all subscriptions in succeeded, but some of those rows may have already
existed before this request. In the UserMailingListSubscription update_or_create
flow, capture the created flag or primary key for each list_id inside the
transaction, store only rows actually created by this request, and use that
tracked set in the exception handler instead of filtering by user and
list_id__in=succeeded. Keep the fix localized to the subscription creation loop
and the _send_confirmation_email rollback block in mailing_list/views.py.
In `@static/css/v3/mailing-list-card.css`:
- Around line 254-263: The email field’s invalid state is overriding the
keyboard focus cue, so fix the interaction between
`.ml-post-auth-modal__email-field:focus-visible` and
`.ml-post-auth-modal__email-field--error`. Keep a visible focus indicator when
the field is both focused and in error by adding a more specific focused-error
state or adjusting the error styles so they don’t suppress the focus ring.
Update the focus styling in this CSS module to ensure the focus-visible cue
remains clear even when validation errors are present.
In `@templates/v3/includes/_content_modal.html`:
- Line 21: The dialog backdrop anchor is now exposed to assistive tech without
an accessible name, so update the backdrop link in the modal template to either
remain hidden from screen readers or provide an appropriate label. Adjust the
dialog-modal__backdrop element so it has a clear accessible name consistent with
the close control, or mark it non-interactive for assistive technologies if it
is only decorative.
In `@templates/v3/includes/_dialog.html`:
- Line 28: The dialog backdrop anchor is now exposed to assistive tech without
any accessible name, so it should either be hidden from screen readers or given
a label. Update the dialog template around dialog-modal__backdrop to make the
backdrop non-interactive for accessibility, or add an appropriate accessible
name so it no longer appears as an empty link before the labeled close action.
In `@templates/v3/includes/_mailing_list_card.html`:
- Line 102: The modal backdrop link in the mailing list card template is now
exposed to assistive tech without an accessible name, so it should be hidden
from the accessibility tree or otherwise given a proper label. Update the dialog
backdrop anchor in the modal markup to match the accessible behavior used by the
other modal templates, and ensure the visible close control remains the first
labeled focus target in this component.
In `@templates/v3/includes/_mailing_list_post_auth_modal.html`:
- Around line 38-39: The modal hash handling in the mailing list post-auth
template leaves stale history when closing via the backdrop or ESC path. Update
the close behavior for the modal setup in the x-init / dialog-modal backdrop
flow so every dismissal path clears the `#ml-post-auth-modal` hash, not just the
Cancel action. Make the cleanup consistent in the modal’s close link/backdrop
handling so Back does not reopen the modal after it has been dismissed.
🪄 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: 3a177ca3-b364-47e1-8255-eb8ad955e9fa
📒 Files selected for processing (10)
ak/views.pymailing_list/urls.pymailing_list/views.pystatic/css/v3/mailing-list-card.csstemplates/v3/homepage.htmltemplates/v3/includes/_content_modal.htmltemplates/v3/includes/_dialog.htmltemplates/v3/includes/_mailing_list_card.htmltemplates/v3/includes/_mailing_list_post_auth_modal.htmlusers/signals.py
| succeeded = [] | ||
| for lid in list_ids: | ||
| try: | ||
| with transaction.atomic(): | ||
| UserMailingListSubscription.objects.update_or_create( | ||
| user=user, | ||
| list_id=lid, | ||
| defaults={"email": email, "status": SubscriptionStatus.PENDING}, | ||
| ) | ||
| succeeded.append(lid) | ||
| except IntegrityError: | ||
| pass | ||
|
|
||
| if not succeeded: | ||
| return [], None | ||
|
|
||
| try: | ||
| _send_confirmation_email(request, email, user.pk, succeeded) | ||
| except Exception as exc: | ||
| logger.error("Failed to send confirmation email to %s: %s", email, exc) | ||
| UserMailingListSubscription.objects.filter( | ||
| user=user, list_id__in=succeeded | ||
| ).delete() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Roll back only rows created by this request.
The rollback path deletes every UserMailingListSubscription matching user/list_id__in=succeeded, but update_or_create() does not guarantee those rows were created here. A duplicate submit race can hit created=False, and an email failure in this request would then delete the other request’s pending row. Track the created PKs (or the created flag) and only delete those on rollback.
Suggested fix
def _subscribe_pending(
request, user, email: str, list_ids: list[str]
) -> tuple[list[str], str | None]:
@@
- succeeded = []
+ succeeded = []
+ created_pks = []
for lid in list_ids:
try:
with transaction.atomic():
- UserMailingListSubscription.objects.update_or_create(
+ sub, created = UserMailingListSubscription.objects.update_or_create(
user=user,
list_id=lid,
defaults={"email": email, "status": SubscriptionStatus.PENDING},
)
+ if created:
+ created_pks.append(sub.pk)
succeeded.append(lid)
except IntegrityError:
pass
@@
try:
_send_confirmation_email(request, email, user.pk, succeeded)
except Exception as exc:
logger.error("Failed to send confirmation email to %s: %s", email, exc)
- UserMailingListSubscription.objects.filter(
- user=user, list_id__in=succeeded
- ).delete()
+ UserMailingListSubscription.objects.filter(pk__in=created_pks).delete()
return [], "Could not send confirmation email. Please try again."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| succeeded = [] | |
| for lid in list_ids: | |
| try: | |
| with transaction.atomic(): | |
| UserMailingListSubscription.objects.update_or_create( | |
| user=user, | |
| list_id=lid, | |
| defaults={"email": email, "status": SubscriptionStatus.PENDING}, | |
| ) | |
| succeeded.append(lid) | |
| except IntegrityError: | |
| pass | |
| if not succeeded: | |
| return [], None | |
| try: | |
| _send_confirmation_email(request, email, user.pk, succeeded) | |
| except Exception as exc: | |
| logger.error("Failed to send confirmation email to %s: %s", email, exc) | |
| UserMailingListSubscription.objects.filter( | |
| user=user, list_id__in=succeeded | |
| ).delete() | |
| succeeded = [] | |
| created_pks = [] | |
| for lid in list_ids: | |
| try: | |
| with transaction.atomic(): | |
| sub, created = UserMailingListSubscription.objects.update_or_create( | |
| user=user, | |
| list_id=lid, | |
| defaults={"email": email, "status": SubscriptionStatus.PENDING}, | |
| ) | |
| if created: | |
| created_pks.append(sub.pk) | |
| succeeded.append(lid) | |
| except IntegrityError: | |
| pass | |
| if not succeeded: | |
| return [], None | |
| try: | |
| _send_confirmation_email(request, email, user.pk, succeeded) | |
| except Exception as exc: | |
| logger.error("Failed to send confirmation email to %s: %s", email, exc) | |
| UserMailingListSubscription.objects.filter(pk__in=created_pks).delete() | |
| return [], "Could not send confirmation email. Please try again." |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 113-113: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@mailing_list/views.py` around lines 95 - 117, The rollback in the
confirmation-email failure path is deleting all subscriptions in succeeded, but
some of those rows may have already existed before this request. In the
UserMailingListSubscription update_or_create flow, capture the created flag or
primary key for each list_id inside the transaction, store only rows actually
created by this request, and use that tracked set in the exception handler
instead of filtering by user and list_id__in=succeeded. Keep the fix localized
to the subscription creation loop and the _send_confirmation_email rollback
block in mailing_list/views.py.
| if list_ids and not _is_rate_limited(request): | ||
| current = { | ||
| sub.list_id | ||
| for sub in UserMailingListSubscription.objects.filter( | ||
| user=request.user, list_id__in=managed_lists | ||
| ) | ||
| } | ||
| to_subscribe = [lid for lid in list_ids if lid not in current] | ||
|
|
||
| _subscribe_pending(request, request.user, email, to_subscribe) | ||
|
|
||
| if _is_htmx(request): | ||
| return HttpResponse("") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t close the modal on failed subscriptions.
This branch swallows both rate-limit failures and _subscribe_pending() errors, then still returns an empty HTMX response. Since HomepageView.get_v3_context_data() already persists user.data["ml_post_auth_seen"] = True, any transient mail/backend failure becomes a silent one-shot no-op for the user. Keep the modal open and surface an error state before dismissing it.
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 706-706: Lack of sanitization of user data
Context: HttpResponse("")
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(http-response-from-request)
🤖 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 `@mailing_list/views.py` around lines 695 - 707, The HTMX branch in the
subscription flow is returning an empty response even when
`_is_rate_limited(request)` blocks the action or `_subscribe_pending()` fails,
which causes the modal to close on a silent failure. Update the subscription
handler around the `list_ids`, `_is_rate_limited`, and `_subscribe_pending`
logic so it detects these failure cases, keeps the modal open, and returns an
error state/message instead of `HttpResponse("")` when the subscription did not
succeed. Ensure the `_is_htmx(request)` response path only dismisses the modal
after a successful subscribe.
| .ml-post-auth-modal__email-field:focus-visible { | ||
| background-color: var(--color-surface-mid); | ||
| border-color: var(--color-stroke-strong); | ||
| } | ||
|
|
||
| .ml-post-auth-modal__email-field--error { | ||
| background-color: var(--color-surface-error-weak) !important; | ||
| border-color: var(--color-stroke-error) !important; | ||
| color: var(--color-text-error); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Invalid email state hides the keyboard focus indicator.
outline: none removes the native ring, and the --error rule’s !important border/background override the :focus-visible styling. Once validation fails, the focused field no longer has a clear focus cue.
🤖 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/mailing-list-card.css` around lines 254 - 263, The email
field’s invalid state is overriding the keyboard focus cue, so fix the
interaction between `.ml-post-auth-modal__email-field:focus-visible` and
`.ml-post-auth-modal__email-field--error`. Keep a visible focus indicator when
the field is both focused and in error by adding a more specific focused-error
state or adjusting the error styles so they don’t suppress the focus ring.
Update the focus styling in this CSS module to ensure the focus-visible cue
remains clear even when validation errors are present.
| {% endcomment %} | ||
| <div class="dialog-modal content-modal" id="{{ modal_id }}"> | ||
| <a class="dialog-modal__backdrop" href="{{ close_url|default:'#_' }}" aria-hidden="true" tabindex="-1"></a> | ||
| <a class="dialog-modal__backdrop" href="{{ close_url|default:'#_' }}" tabindex="-1"></a> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Backdrop link is now exposed without an accessible name.
Removing aria-hidden makes this anchor visible to assistive tech, but unlike the explicit close control on Line 30 it still has no label. Screen readers can encounter it as an empty link.
🤖 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 `@templates/v3/includes/_content_modal.html` at line 21, The dialog backdrop
anchor is now exposed to assistive tech without an accessible name, so update
the backdrop link in the modal template to either remain hidden from screen
readers or provide an appropriate label. Adjust the dialog-modal__backdrop
element so it has a clear accessible name consistent with the close control, or
mark it non-interactive for assistive technologies if it is only decorative.
|
|
||
| <div class="dialog-modal" id="{{ dialog_id }}"> | ||
| <a class="dialog-modal__backdrop" href="#_" aria-hidden="true" tabindex="-1"></a> | ||
| <a class="dialog-modal__backdrop" href="#_" tabindex="-1"></a> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Backdrop link is now exposed without an accessible name.
After removing aria-hidden, this backdrop anchor is no longer purely decorative for assistive tech, but it still has no label. That leaves screen readers with an empty link ahead of the actual labeled close action on Line 39.
🤖 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 `@templates/v3/includes/_dialog.html` at line 28, The dialog backdrop anchor is
now exposed to assistive tech without any accessible name, so it should either
be hidden from screen readers or given a label. Update the dialog template
around dialog-modal__backdrop to make the backdrop non-interactive for
accessibility, or add an appropriate accessible name so it no longer appears as
an empty link before the labeled close action.
| {% if modal_subscribe_url and mailing_lists %} | ||
| <div class="dialog-modal" id="mailing-list-modal"> | ||
| <a class="dialog-modal__backdrop" href="#_" tabindex="-1" aria-hidden="true"></a> | ||
| <a class="dialog-modal__backdrop" href="#_" tabindex="-1"></a> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Backdrop link is now exposed without an accessible name.
Removing aria-hidden here has the same side effect as the other modal templates: assistive tech can now hit an unlabeled link before reaching the labeled close button on Line 113.
🤖 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 `@templates/v3/includes/_mailing_list_card.html` at line 102, The modal
backdrop link in the mailing list card template is now exposed to assistive tech
without an accessible name, so it should be hidden from the accessibility tree
or otherwise given a proper label. Update the dialog backdrop anchor in the
modal markup to match the accessible behavior used by the other modal templates,
and ensure the visible close control remains the first labeled focus target in
this component.
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 `@templates/v3/includes/_mailing_list_post_auth_modal.html`:
- Line 20: The no-JS fallback for the mailing list post-auth modal is not
keyboard operable because the hidden toggle and label-based close controls
cannot be reliably activated without JavaScript. Update the fallback in the
mailing list modal markup so the close path uses a native keyboard-accessible
control, or make the existing toggle operable without JS; focus on the controls
around ml-post-auth-toggle, the backdrop close affordance, and the visible
Cancel action in the modal template.
🪄 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: 9c6df8e8-4254-4f21-bf6b-7862a72183c9
📒 Files selected for processing (5)
mailing_list/views.pystatic/css/v3/dialog.cssstatic/js/dialog.jstemplates/v3/includes/_mailing_list_card.htmltemplates/v3/includes/_mailing_list_post_auth_modal.html
| JS enhances this with HTMX submit, inline email validation, and ESC/focus trap | ||
| via dialog.js (which recognises the checkbox toggle). | ||
| {% endcomment %} | ||
| <input type="checkbox" id="ml-post-auth-toggle" class="dialog-modal__toggle" checked hidden> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The documented no-JS close path is not keyboard-operable.
The controlling checkbox is hidden, the backdrop close affordance is unfocusable, and the visible “Cancel” control is a <label> rather than a natively activatable button. That means a keyboard user cannot reliably dismiss this auto-opened modal unless dialog.js is running, which breaks the fallback described on Lines 13-18. Please switch the fallback to a natively keyboard-operable close mechanism, or make the toggle itself operable in the no-JS path.
Also applies to: 43-43, 124-127
🤖 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 `@templates/v3/includes/_mailing_list_post_auth_modal.html` at line 20, The
no-JS fallback for the mailing list post-auth modal is not keyboard operable
because the hidden toggle and label-based close controls cannot be reliably
activated without JavaScript. Update the fallback in the mailing list modal
markup so the close path uses a native keyboard-accessible control, or make the
existing toggle operable without JS; focus on the controls around
ml-post-auth-toggle, the backdrop close affordance, and the visible Cancel
action in the modal template.
Issue: #2436
Summary & Context
Adds a post-login mailing list subscription modal that appears automatically on the homepage the first (and only) time a user lands there after logging in. The modal pre-fills the user's account email and lets them choose which lists to subscribe to.
Changes
users/signals.py- setrequest.session["show_ml_post_auth_modal"]inuser_logged_inwhen user hasn't seen the modal yet (User.data["ml_post_auth_seen"]falsy)ak/views.py(HomepageView) - pop session flag inget_v3_context_data; mark user as seen immediately; inject modal list contextmailing_list/views.py- addPostAuthSubscribeView(login required); reuses existing subscription + confirmation-email logic; returns empty HTMX fragment to remove modal from DOMmailing_list/urls.py- register/mailing-list/post-auth-subscribe/templates/v3/includes/_mailing_list_post_auth_modal.html- new standalone modal partial; auto-opens via Alpinex-init+ CSS:target; reuses.mailing-list-modal__*classes for checkboxes/list itemsstatic/css/v3/mailing-list-card.css- add.ml-post-auth-modal__*classes for email row, body, footer row, and counttemplates/v3/homepage.html- conditionally include the modal whenshow_ml_post_auth_modalis in contextHow the "seen once" logic works
user_logged_insignal setsrequest.session["show_ml_post_auth_modal"] = True(only ifUser.data["ml_post_auth_seen"]is not set)HomepageView.get_v3_context_datapops the session key and immediately setsUser.data["ml_post_auth_seen"] = Trueon the User recordx-initopens it by setting the URL hashPostAuthSubscribeView, which creates PENDING subscription records and sends the confirmation email, then returns an empty response that HTMX uses to remove the modal from DOMRisks & Considerations
User.dataJSONField is used for theml_post_auth_seenflag - no migration needed since the field already exists and defaults to{}v3flag gate viaHomepageView.get_v3_context_data)Screenshots
Peer-review testing steps
v3flag is enableddocker compose exec web python manage.py shellSelf-review Checklist
Frontend
Summary by CodeRabbit
New Features
Bug Fixes
UX / Accessibility