Story #2282: Implements Mailman mailing list subscription#2444
Conversation
1a44f32 to
18ed3a0
Compare
a715613 to
1f60148
Compare
julhoang
left a comment
There was a problem hiding this comment.
Hi @herzog0 , this is such a fantastic work! Thank you for all of your hard work and thoughtful considerations to put this PR together 🙌 !! There's a few concerns I want to share:
1. Stale pending records
If a user doesn't confirm within 7 days, it seems like we're still retaining the pending record in our DB indefinitely. Should we schedule a Celery task to clean up dead records past the confirmation window?
2. No rate limiting on the anonymous subscription card
It seems like there's currently no rate limit on the subscription card for anonymous users. With malicious intent, someone could use it to fire off a lot of emails (either across many addresses or repeatedly to the same one), which spams real people. IMO the bigger risk is email providers like Gmail will likely flag our sending domain as spam, which affects deliverability for every legitimate email we send (e.g. confirmations, notifications, etc.).
3. Duplicate email across users
I'm not fully clear on how Mailman handles subscription state on its end. Let's consider this scenario:
- User A (authenticated) tries to subscribe with
test@gmail.com→ DB now has User A inpendingstate with that email - User B (the real owner, authenticated) subscribes with their actual email
test@gmail.com→ confirms successfully → DB now has User B inactivestate with the same email
The screenshot shows our system currently accepts this, though I'm wondering:
- Does Mailman permit this on its side, or will the second subscription collide? (I tried to test for this but couldn't tell for sure)
- Should we add a uniqueness check on
(list_id, email)to prevent this from happening? 🤔
4. Adding Tests
Can we consider adding tests to ensure we can cover all possible scenarios?
All this being said, I understand this is already a large PR – if you'd prefer to spin any of these out as follow-up tasks, we can certainly do that too!
|
Great suggestions @julhoang! Thanks for the review. Everything should be addressed now. Only the rate limit that I thought would be good adding to both types of users (authenticated and anonymous). |
3b91b88 to
b3baaad
Compare
julhoang
left a comment
There was a problem hiding this comment.
Amazing work here @herzog0! This mailing list subscription definitely presents a lot of edge cases – I appreciate how thoroughly you've thought through the different user paths and the careful approach you took to handle them. This will make the feature much more robust 🙌
a917f3a to
2c48da6
Compare
jlchilders11
left a comment
There was a problem hiding this comment.
Few small questions/changes.
jlchilders11
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for those clean ups!
feat: add Mailman REST API client and config settings feat: add UserMailingListSubscription model feat: add mailing list subscribe/unsubscribe view and route feat: add subscribe result HTMX fragment and styles chore: add mailing list subscribe demo to component demo view chore: add mailman-members dev helper script fix: mailing list name chore: rename script feat: improve test script fix: hostname feat: wire up mailing list card feat: show success susbcribe card chore: request email verification feat: discard pending subscriptions when user unsub before verification feat: add email field to subscription model feat: add status field to subscription model feat: add pending logs to demo component feat: improve dev mm script feat: add boost owned email validation flow feat: Allow anonymous subscriptions and enhance pending state UI fix: csrf token injection fix: styles chore: improve subscription confirmation pages styles feat: improve subscription confirmation styles fix: centralize _CONFIRM_MAX_AGE value feat: improve email templates fix: font size fix: change doc chore: remove unused login url chore: add dev note to mailing list example card chore: update salt and add dev notes chore: centralize and improve subscription state logic fix: typography styles feat: no-js fix: guard mailing_list_state access and remove debug print fix: correct MAILMAN_LISTS defaults to use full list IDs fix: remove redundant buttons.css import and use _button.html in confirm templates fix: pass expiry_label to confirm_invalid when user not found fix: narrow exception handling in email sending to SMTPException and OSError fix: add novalidate and client-side email validation to mailing list card fix: replace build_absolute_uri with reverse for internal URLs revert: keep broad Exception catch in email sending feat: purge expired pending subscriptions via Celery beat feat: rate-limit anonymous subscription attempts per IP fix: add (list_id, email) uniqueness constraint to prevent duplicate subscriptions test: add view and task tests for mailing list subscription flows refactor: apply subscribe rate limit to all users, exempt staff and superusers fix: wrap IntegrityError catches in transaction.atomic savepoints
a705faa to
6c5482c
Compare
|
Here is part of a review from Claude. Not certain if it's accurate but worth checking. The "is it safe" question has a clear answer: mostly yes, but there is a real bug. Inside SubscribeView.post() (mailing_list/views.py lines 124–204), the unsubscribe branch does this: views.py Notice it calls MailmanClient().unsubscribe(email, list_id) where email is whatever the user typed into the form right now (line 125: email = request.POST.get("email", "").strip()), not sub.email (the address actually stored on the DB row, which is what they subscribed with). That means: suppose Alice is logged in, she previously subscribed alice@example.com, and on the manage page she retypes bob@example.com in the email field and unticks the box for the boost-developers list. The code will call Mailman to delete bob@example.com from that list. If Bob happens to be a real member (e.g. an old direct mailman subscriber from years ago), this will silently delete him. That's the kind of "delete a pre-existing user accidentally" thing you were worried about. It's the one place I'd actually call this out as needing a fix: What is the right thing to do? |
|
@coderabbitai full review |
|
I enabled coderabbit on the website-v2 repository. Experimental. The results might be nitpicky and don't always need to be followed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a full mailing list subscription system: a ChangesMailing List Subscription Feature
Sequence Diagram(s)sequenceDiagram
participant Browser
participant QuickSubscribeView
participant DB as UserMailingListSubscription
participant EmailBackend as Django Email
participant ConfirmView as ConfirmSubscriptionView
participant MailmanClient
participant Mailman as Mailman REST API
rect rgba(100, 149, 237, 0.5)
note over Browser,EmailBackend: Quick Subscribe (authenticated)
Browser->>QuickSubscribeView: POST /mailing-list/quick-subscribe/ (email, list_id)
QuickSubscribeView->>DB: INSERT UserMailingListSubscription (PENDING)
QuickSubscribeView->>EmailBackend: _send_confirmation_email (signed token, user_id)
EmailBackend-->>Browser: Confirmation email with /confirm/<token>/
QuickSubscribeView-->>Browser: HTMX card response (pending state)
end
rect rgba(144, 238, 144, 0.5)
note over Browser,Mailman: Confirmation
Browser->>ConfirmView: GET /mailing-list/confirm/<token>/
ConfirmView->>ConfirmView: Validate signed token (max-age check)
ConfirmView->>MailmanClient: subscribe(email, list_id)
MailmanClient->>Mailman: POST /3.1/members (pre_verified=True)
Mailman-->>MailmanClient: 200 OK / 409
ConfirmView->>DB: UPDATE status → ACTIVE
ConfirmView-->>Browser: confirm_success.html
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mailing_list/tests/test_views.py (1)
272-279:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce the “without DB record” contract in the anonymous confirm test.
The test name asserts no DB record should exist, but it currently only checks Mailman client invocation.
Suggested fix
`@pytest.mark.django_db` def test_confirm_anonymous_token_subscribes_without_db_record(client): @@ with patch("mailing_list.views.MailmanClient") as MockClient: response = client.get(url) assert response.status_code == 200 MockClient.return_value.subscribe.assert_called_once_with(EMAIL, LIST_ID) + assert not UserMailingListSubscription.objects.filter( + email=EMAIL, list_id=LIST_ID + ).exists()🤖 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/tests/test_views.py` around lines 272 - 279, The test function test_confirm_anonymous_token_subscribes_without_db_record claims in its name that it verifies subscription without a database record, but currently only validates that the Mailman client was invoked. Add an assertion after the response is obtained to verify that no corresponding database record was actually created. Query the relevant subscription model (using EMAIL and LIST_ID) and assert that no record exists in the database, ensuring the test enforces the contract implied by its name.
🧹 Nitpick comments (2)
mailing_list/models.py (1)
76-85: ⚡ Quick winAdd a composite index for pending-subscription purge queries.
purge_expired_pending_subscriptionsfilters bystatusandsubscribed_at; without an index this becomes a table scan as rows grow.♻️ Proposed migration-safe model update
class UserMailingListSubscription(models.Model): @@ class Meta: unique_together = [("user", "list_id"), ("list_id", "email")] + indexes = [ + models.Index(fields=["status", "subscribed_at"]), + ]🤖 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/models.py` around lines 76 - 85, The Subscription model is missing a composite database index on the status and subscribed_at fields, which causes the purge_expired_pending_subscriptions query to perform a table scan as the table grows. Add a composite index to the Meta class of the Subscription model by including an indexes list that creates an index on both the status and subscribed_at fields. This index should be defined alongside the existing unique_together constraint to optimize the filtering queries used in the purge operation.static/css/v3/mailing-list-card.css (1)
33-37: ⚡ Quick winRespect reduced-motion preferences for the spinner.
Add a
prefers-reduced-motionfallback so users who disable motion don’t get continuous rotation.Proposed fix
`@keyframes` mailing-list-card-spin { to { transform: rotate(360deg); } } + +@media (prefers-reduced-motion: reduce) { + .mailing-list-card__spinner { + animation: none; + } +}🤖 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 33 - 37, The mailing-list-card-spin keyframe animation does not respect users' motion accessibility preferences. Add a `@media` query block with prefers-reduced-motion: reduce to override the animation behavior for users who have disabled motion. Within this media query, either remove the animation from the element that uses mailing-list-card-spin (by setting animation: none) or provide a static alternative such as a fixed rotation without continuous spinning. This ensures users with motion sensitivity are not exposed to continuous rotation animations.
🤖 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 `@core/views.py`:
- Around line 2248-2249: The code assumes constants.MAILMAN_LISTS contains at
least one element when assigning to _demo_card_list_id, but this will crash with
an IndexError if the list is empty. Add a guard check before accessing
constants.MAILMAN_LISTS[0] to verify the list is not empty; only assign
_demo_card_list_id and set context["demo_mailman_lists"] if the list has
elements, otherwise handle the empty case appropriately (for example, by
skipping the assignment or using a default value).
In `@mailing_list/client.py`:
- Around line 58-63: The logger.info call for "subscribe failed" in the
subscribe failure block is logging the full response.text from the Mailman API
response, which may contain sensitive subscriber identifiers and other PII.
Remove the "response_text": response.text entry from the extra dictionary passed
to the logger.info call, keeping only the safe status_code field to avoid
exposing sensitive data in logs.
- Around line 110-117: The discard POST request in the _discard_pending() method
ignores the response status, allowing failures to be silently treated as success
and leaving pending subscriptions undeleted. Capture the response object
returned by requests.post and validate it similar to the GET request pattern
used earlier in the same function (lines 88-96): check response.ok and return
without error if the status is 404 (the request is already gone), but raise
MailmanAPIError with relevant error details for any other non-2xx status codes
to ensure the system detects and properly handles discard failures instead of
drifting into an invalid state.
In `@mailing_list/constants.py`:
- Around line 25-71: The MAILMAN_LISTS constant is currently derived from
MAILMAN_DOMAIN which comes from the REST API URL (MAILMAN_REST_API_URL),
coupling list identifiers to the API host. This causes issues when the API URL
and actual list domain differ (e.g., mailman-core:8001 vs lists.boost.org). To
fix this, introduce a separate configuration for the mailing list domain
(distinct from MAILMAN_DOMAIN used for API transport), use this new domain in
the build_mailing_list_address function instead of MAILMAN_DOMAIN, and define
MAILMAN_LISTS as an explicit configuration sourced from settings or environment
variables rather than deriving it from MAILMAN_REST_API_URL. Keep
MAILMAN_REST_API_URL and MAILMAN_DOMAIN reserved solely for API transport
purposes.
In `@mailing_list/mixins.py`:
- Line 9: The `_DEFAULT_LIST_ID` assignment directly accesses the first element
of `constants.MAILMAN_LISTS` without checking if the list is empty, causing an
IndexError at import time that breaks all code importing this mixin. Add a guard
to safely access the first element of `constants.MAILMAN_LISTS`, either by
checking its length before access or using a fallback/default value when the
list is empty or misconfigured. This ensures the import completes successfully
even if `MAILMAN_LISTS` is improperly configured.
In `@mailing_list/tests/test_views.py`:
- Around line 138-226: Add a new regression test function following the existing
authenticated quick subscribe tests to verify that the unsubscribe functionality
uses the persisted subscription email stored in the UserMailingListSubscription
record (sub.email) and never uses arbitrary form input. The test should create a
subscription with one email address, attempt an unsubscribe request with a
different email in the form data, and assert that the unsubscribe operation
succeeds and uses the correct persisted email from the subscription record, not
the form-provided email.
- Around line 31-35: The locmem_cache fixture configures the cache but does not
clear it between test runs, causing rate-limit throttle keys to persist and leak
across tests, making them order-dependent and flaky. Modify the fixture to clear
the cache after each test completes by adding teardown logic: use a yield
statement after configuring the cache settings, then add cleanup code after the
yield that clears the cache instance to ensure a fresh state for each test.
In `@mailing_list/views.py`:
- Line 245: The logger.info call in the quick subscribe request is logging the
raw email address as PII, which creates privacy and compliance concerns. Replace
the raw email variable in the log message with a hashed or redacted version of
the email address to maintain observability while protecting user privacy.
Additionally, search for any related error logs in the same function or nearby
code that also log email addresses and apply the same redaction/hashing
treatment to those as well.
- Around line 61-65: The _get_client_ip function trusts the X-Forwarded-For
header directly without validation, allowing clients to spoof different IP
addresses per request to bypass rate limiting. Modify the function to implement
a trusted proxy strategy: only use the X-Forwarded-For header if the request
originates from a known trusted proxy, otherwise fall back to REMOTE_ADDR. This
may require configuring a list of trusted proxies in your Django settings and
checking the request's actual origin before deciding which IP source to use.
- Around line 180-197: The code is calling MailmanClient().unsubscribe(email,
list_id) with a form-provided email parameter, which creates a security
vulnerability by allowing a logged-in user to unsubscribe another user's email
address. Instead of using the untrusted form input email, retrieve the
subscription object that was already filtered (the one checked for PENDING
status) and use its stored email address when calling
MailmanClient().unsubscribe(). If the subscription object exists with a
non-PENDING status, use its email attribute rather than the form input email for
the unsubscribe operation.
In `@templates/v3/includes/_field_text.html`:
- Line 90: The disabled attribute check on line 90 references `value` when
`validate_type` is any truthy value, but `value` is only defined when
`validate_type` equals "email". Replace the broad `validate_type` check with a
specific check for `validate_type == "email"` to ensure `value.trim()` is only
evaluated when that variable actually exists. This narrows the condition from
checking any truthy validate_type to only checking when the validator is
specifically the email validator.
In `@templates/v3/mailing_list/confirm_success.html`:
- Around line 20-24: The template currently always displays the success message
with the heading "Subscription confirmed" and the body text about subscription,
but this content should only be shown when there are actually confirmed items.
Wrap the entire success confirmation section (containing the
mailing-list-confirm__title heading and mailing-list-confirm__body paragraph
with the email and confirmed list references) in a conditional template tag that
checks if the confirmed list is not empty. This ensures the success message is
only displayed when subscriptions were actually confirmed, not when only errors
occurred.
---
Outside diff comments:
In `@mailing_list/tests/test_views.py`:
- Around line 272-279: The test function
test_confirm_anonymous_token_subscribes_without_db_record claims in its name
that it verifies subscription without a database record, but currently only
validates that the Mailman client was invoked. Add an assertion after the
response is obtained to verify that no corresponding database record was
actually created. Query the relevant subscription model (using EMAIL and
LIST_ID) and assert that no record exists in the database, ensuring the test
enforces the contract implied by its name.
---
Nitpick comments:
In `@mailing_list/models.py`:
- Around line 76-85: The Subscription model is missing a composite database
index on the status and subscribed_at fields, which causes the
purge_expired_pending_subscriptions query to perform a table scan as the table
grows. Add a composite index to the Meta class of the Subscription model by
including an indexes list that creates an index on both the status and
subscribed_at fields. This index should be defined alongside the existing
unique_together constraint to optimize the filtering queries used in the purge
operation.
In `@static/css/v3/mailing-list-card.css`:
- Around line 33-37: The mailing-list-card-spin keyframe animation does not
respect users' motion accessibility preferences. Add a `@media` query block with
prefers-reduced-motion: reduce to override the animation behavior for users who
have disabled motion. Within this media query, either remove the animation from
the element that uses mailing-list-card-spin (by setting animation: none) or
provide a static alternative such as a fixed rotation without continuous
spinning. This ensures users with motion sensitivity are not exposed to
continuous rotation animations.
🪄 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: 5792f33b-34dd-490a-ad0d-3231d7e1b94c
📒 Files selected for processing (35)
config/celery.pyconfig/settings.pyconfig/urls.pycore/views.pydocker-compose.ymlenv.templatelibraries/views.pymailing_list/client.pymailing_list/constants.pymailing_list/migrations/0007_usermailinglistsubscription.pymailing_list/mixins.pymailing_list/models.pymailing_list/tasks.pymailing_list/tests/test_tasks.pymailing_list/tests/test_views.pymailing_list/urls.pymailing_list/views.pyscripts/dev-mailman-helpersstatic/css/v3/components.cssstatic/css/v3/mailing-list-card.cssstatic/css/v3/mailing-list-confirm.cssstatic/css/v3/v3-examples-section.csstemplates/v3/community.htmltemplates/v3/examples/_v3_example_section.htmltemplates/v3/includes/_field_text.htmltemplates/v3/includes/_mailing_list_card.htmltemplates/v3/learn_page.htmltemplates/v3/libraries/library-subpage.htmltemplates/v3/mailing_list/_subscribe_result.htmltemplates/v3/mailing_list/_subscribe_success_card.htmltemplates/v3/mailing_list/confirm_invalid.htmltemplates/v3/mailing_list/confirm_success.htmltemplates/v3/mailing_list/email/confirm_subscription.txttemplates/v3/release_detail.htmlversions/views.py
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Issue: #2282
Summary & Context
Implements the mailing list subscribe/unsubscribe flow backed by the Mailman REST API. Users (authenticated or anonymous) can subscribe to Boost mailing lists via a V3 card component; subscription state is tracked in Django and confirmed via a signed email link before Mailman is called.
Changes
mailing_list/client.py) - thin wrapper around the Mailman REST API exposingsubscribe,unsubscribe,is_confirmed, and_discard_pending; allpre_*flags are set toTruebecause Django owns the confirmation stepUserMailingListSubscriptionmodel + migration - new model tracking per-user subscription state (pending/active) keyed on(user, list_id)with a corresponding migrationmailing_list/views.py):QuickSubscribeView- single-list subscribe for both authenticated and anonymous users; HTMX requests get an in-place card swap, non-HTMX requests get a PRG redirect back to the originating page with state encoded in query params (ml_state,ml_error,ml_email)SubscribeView- multi-list subscribe/unsubscribe for authenticated users (used in the demo page; will be reused for the profile modal)ConfirmSubscriptionView- handles the signed confirmation link, calls Mailman, and upgrades the DB record toactive; renders success/invalid pagesMailingListCardMixin(mailing_list/mixins.py) - injects card context into any CBV; reads DB state for authenticated users and falls back toml_state/ml_error/ml_emailquery params for the no-JS PRG flow; wired intoCommunityView,LearnPageView,ReleaseDetailView, andLibrarySubpageView_mailing_list_card.htmlwith state-aware rendering (default / pending / active / error), HTMX swap, and a plainaction/methodform fallback for no-JS; newconfirm_success.html,confirm_invalid.html,_subscribe_result.html,_subscribe_success_card.html, and plain-text confirmation email templatemailing-list-card.css(spinner, badge, form loading states) andmailing-list-confirm.css(full-page success/error confirmation layout)mailman-coreandmailman-webservices enabled indocker-compose.yml;MAILMAN_REST_API_URL,MAILMAN_DEV_API_URL, andMAILMAN_LISTSadded toenv.templatescripts/dev-mailman-helpers) - shell script for seeding local Mailman lists and inspecting subscription state during developmentRisks & Considerations
django.core.signingwith a 7-day TTL and a fixed salt embedded in the source. The salt is not a secret (signing is not encryption), but rotating it invalidates all outstanding tokens.QuickSubscribeViewis stateless for anonymous users - there is no server-side record until confirmation. If the email is already subscribed, a live Mailman API check is made; if the API is unreachable the duplicate-check is skipped silently and Mailman will return 409 on confirm (handled as a no-op).SubscribeViewis currently only wired to the demo page - it will need to be adapted when the multi-selection profile modal is built.mailman-coreandmailman-webDocker services now start by default. Teams that don't need Mailman locally can comment them out or usedocker compose up web db redis.Screenshots
Peer-review testing steps
env.templatevalues forMAILMAN_REST_API_URL,MAILMAN_DEV_API_URL, andMAILMAN_LISTSinto your.envdocker compose up- verifymailman-corestarts on port 8001scripts/dev-mailman-helpersto seed the three Boost lists in the local Mailman instance (choosecreate lists)Maildevinbox athttp://localhost:1080/#//community/and test the mailing list card as an anonymous user - enter an email and verify a confirmation email is sent to maildevhttp://localhost:8000/v3/demo/components/#mailing-list-subscribe-livesave. This will unsubscribe you from the list (you can fully test subscriptions in multiple lists in this component btw)No-JS flow (disable JavaScript in DevTools - Debugger tab - "Disable JavaScript")
Self-review Checklist
Frontend
Summary by CodeRabbit
/mailing-list/endpoints for subscribe, quick-subscribe, and confirmation.