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!
| to talk to a different Mailman instance: | ||
|
|
||
| client = MailmanClient() | ||
| client = MailmanClient(base_url="http://other:8001", user="u", password="p") |
There was a problem hiding this comment.
Excellent, this is exactly the functionality I was thinking of!
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. |
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