Skip to content

Story #2282: Implements Mailman mailing list subscription#2444

Merged
herzog0 merged 18 commits into
developfrom
teo/2282-mailing-list
Jun 22, 2026
Merged

Story #2282: Implements Mailman mailing list subscription#2444
herzog0 merged 18 commits into
developfrom
teo/2282-mailing-list

Conversation

@herzog0

@herzog0 herzog0 commented May 15, 2026

Copy link
Copy Markdown
Collaborator

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

  • Mailman API client (mailing_list/client.py) - thin wrapper around the Mailman REST API exposing subscribe, unsubscribe, is_confirmed, and _discard_pending; all pre_* flags are set to True because Django owns the confirmation step
  • UserMailingListSubscription model + migration - new model tracking per-user subscription state (pending / active) keyed on (user, list_id) with a corresponding migration
  • Three new views (mailing_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 to active; renders success/invalid pages
  • MailingListCardMixin (mailing_list/mixins.py) - injects card context into any CBV; reads DB state for authenticated users and falls back to ml_state / ml_error / ml_email query params for the no-JS PRG flow; wired into CommunityView, LearnPageView, ReleaseDetailView, and LibrarySubpageView
  • V3 templates - updated _mailing_list_card.html with state-aware rendering (default / pending / active / error), HTMX swap, and a plain action/method form fallback for no-JS; new confirm_success.html, confirm_invalid.html, _subscribe_result.html, _subscribe_success_card.html, and plain-text confirmation email template
  • V3 CSS - new mailing-list-card.css (spinner, badge, form loading states) and mailing-list-confirm.css (full-page success/error confirmation layout)
  • Docker + env - mailman-core and mailman-web services enabled in docker-compose.yml; MAILMAN_REST_API_URL, MAILMAN_DEV_API_URL, and MAILMAN_LISTS added to env.template
  • Dev helper script (scripts/dev-mailman-helpers) - shell script for seeding local Mailman lists and inspecting subscription state during development

Risks & Considerations

  • The confirmation token uses django.core.signing with 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.
  • QuickSubscribeView is 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).
  • SubscribeView is currently only wired to the demo page - it will need to be adapted when the multi-selection profile modal is built.
  • mailman-core and mailman-web Docker services now start by default. Teams that don't need Mailman locally can comment them out or use docker compose up web db redis.

Screenshots

image image image image

Peer-review testing steps

  1. Copy env.template values for MAILMAN_REST_API_URL, MAILMAN_DEV_API_URL, and MAILMAN_LISTS into your .env
  2. docker compose up - verify mailman-core starts on port 8001
  3. Run scripts/dev-mailman-helpers to seed the three Boost lists in the local Mailman instance (choose create lists)
  4. Open your Maildev inbox at http://localhost:1080/#/
  5. Log out
  6. Visit /community/ and test the mailing list card as an anonymous user - enter an email and verify a confirmation email is sent to maildev
  7. Copy and paste the confirmation link in another tab - verify the success page shows the correct list name
  8. Go back to the community page, refresh it. You'll notice the card goes back to the default state, because we aren't storing any information about the subscription state for logged out users
  9. Try entering the same email, you should get an error message in the card
  10. Now log in
  11. Navigate to http://localhost:8000/v3/demo/components/#mailing-list-subscribe-live
  12. Write down the same email and uncheck all the boxes. Hit save. This will unsubscribe you from the list (you can fully test subscriptions in multiple lists in this component btw)
  13. Go back to the community page and repeat steps 6 and 7 (while logged in this time)
  14. Right after submitting the form in step 6 you should see the card transition to "pending" state
  15. Copy and paste the confirmation link in another tab - verify the success page shows the correct list name
  16. Go back to the community page, refresh it. You'll notice the card now transitions to the "subscribed" state

No-JS flow (disable JavaScript in DevTools - Debugger tab - "Disable JavaScript")

  1. Repeat steps 5-9 with JS disabled - the form should submit normally, redirect back to the community page, and show the correct card state (pending or error) without any in-place swap
  2. Click the confirmation link - the success page should render as usual
  3. Log in and repeat steps 13-16 with JS disabled - after submitting, the page reloads with the pending card; after confirmation and a manual refresh, the card shows the subscribed state

Self-review Checklist

  • Tag at least one team member from each team to review this PR
  • Link this PR to the related GitHub Project ticket

Frontend

  • UI implementation matches Figma design
  • Tested in light and dark mode
  • Responsive / mobile verified
  • Accessibility checked (keyboard navigation, etc.)
  • Ensure design tokens are used for colors, spacing, typography, etc. – No hardcoded values
  • Test without JavaScript (if applicable)
  • No console errors or warnings

Summary by CodeRabbit

  • New Features
    • Added mailing-list subscription flows with signed confirmation links, including pending/active subscription handling.
    • Introduced /mailing-list/ endpoints for subscribe, quick-subscribe, and confirmation.
  • UI Improvements
    • Added state-driven mailing-list cards across supported V3 pages, including HTMX-powered subscribe result updates and dedicated confirm success/invalid pages.
  • Bug Fixes
    • Improved authenticated vs anonymous behavior, confirmation sending, rate limiting, and duplicate handling.
  • Tests
    • Added coverage for subscribe/confirm flows, rate limiting, and purging expired pending subscriptions.
  • Chores
    • Scheduled daily cleanup of expired pending subscriptions.
  • Configuration
    • Added Mailman REST API version configuration and enabled Mailman services in local Docker compose.

@herzog0 herzog0 requested review from jlchilders11 and julhoang May 18, 2026 15:26
@herzog0 herzog0 marked this pull request as ready for review May 18, 2026 15:27
@herzog0 herzog0 linked an issue May 18, 2026 that may be closed by this pull request
@herzog0 herzog0 force-pushed the teo/2282-mailing-list branch 2 times, most recently from 1a44f32 to 18ed3a0 Compare May 20, 2026 23:47
@herzog0 herzog0 requested review from julioest and ycanales May 21, 2026 00:05
@herzog0 herzog0 force-pushed the teo/2282-mailing-list branch 2 times, most recently from a715613 to 1f60148 Compare May 22, 2026 14:56

@julhoang julhoang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in pending state with that email
  • User B (the real owner, authenticated) subscribes with their actual email test@gmail.com → confirms successfully → DB now has User B in active state with the same email
Image

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!

Comment thread core/views.py Outdated
Comment thread config/settings.py Outdated
Comment thread templates/v3/mailing_list/confirm_invalid.html Outdated
Comment thread templates/v3/mailing_list/confirm_success.html Outdated
Comment thread mailing_list/views.py
Comment thread mailing_list/views.py Outdated
Comment thread mailing_list/views.py
Comment thread templates/v3/mailing_list/confirm_invalid.html Outdated
Comment thread templates/v3/mailing_list/confirm_success.html Outdated
Comment thread templates/v3/includes/_mailing_list_card.html
@herzog0 herzog0 requested a review from julhoang May 26, 2026 15:16
@herzog0

herzog0 commented May 26, 2026

Copy link
Copy Markdown
Collaborator Author

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).
Also I have a comment about the error catching above. Let me know your thoughts!

@herzog0 herzog0 force-pushed the teo/2282-mailing-list branch 2 times, most recently from 3b91b88 to b3baaad Compare May 27, 2026 15:06

@julhoang julhoang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙌

@herzog0 herzog0 force-pushed the teo/2282-mailing-list branch 2 times, most recently from a917f3a to 2c48da6 Compare May 29, 2026 17:05

@jlchilders11 jlchilders11 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few small questions/changes.

Comment thread config/settings.py Outdated
Comment thread mailing_list/mixins.py Outdated
Comment thread mailing_list/mixins.py Outdated
Comment thread mailing_list/client.py
@herzog0 herzog0 requested a review from jlchilders11 June 3, 2026 15:27

@jlchilders11 jlchilders11 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for those clean ups!

Comment thread mailing_list/client.py
herzog0 added 6 commits June 8, 2026 11:47
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
@herzog0 herzog0 force-pushed the teo/2282-mailing-list branch from a705faa to 6c5482c Compare June 8, 2026 14:48
@sdarwin

sdarwin commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

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
Lines 175-194

        for list_id in to_unsubscribe:
            sub = UserMailingListSubscription.objects.filter(
                user=request.user, list_id=list_id
            ).first()
            if sub and sub.status == SubscriptionStatus.PENDING:
                sub.delete()
                unsubscribed.append(list_id)
                continue
            try:
                MailmanClient().unsubscribe(email, list_id)
                UserMailingListSubscription.objects.filter(
                    user=request.user, list_id=list_id
                ).delete()
                unsubscribed.append(list_id)
            except MailmanAPIError as exc:
                logger.error(
                    "Mailman unsubscribe error for %s/%s: %s", email, list_id, exc
                )
                errors.append(list_id)

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?
The whole point of the fix is "never trust the form-submitted email for a destructive Mailman operation." That principle says: if we don't have a stored sub.email to anchor to, we have nothing safe to unsubscribe, so we should do nothing in that case, not fall back to the POST email. So the actual fix looks more like this:

for list_id in to_unsubscribe:
    sub = UserMailingListSubscription.objects.filter(
        user=request.user, list_id=list_id
    ).first()
    if not sub:
        # Row vanished between computing `current` and now — nothing safe to do.
        continue
    if sub.status == SubscriptionStatus.PENDING:
        sub.delete()
        unsubscribed.append(list_id)
        continue
    try:
        MailmanClient().unsubscribe(sub.email, list_id)
        sub.delete()
        unsubscribed.append(list_id)
    except MailmanAPIError as exc:
        logger.error(
            "Mailman unsubscribe error for %s/%s: %s", sub.email, list_id, exc
        )
        errors.append(list_id)

@sdarwin

sdarwin commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

@coderabbitai full review

@sdarwin

sdarwin commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

I enabled coderabbit on the website-v2 repository. Experimental. The results might be nitpicky and don't always need to be followed.
It did not trigger here so far, but I created a duplicate PR of this one at https://github.com/sdarwin/website-v2/pull/1 see the results over there.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 99de299f-8d88-4797-9f7f-fe4efc25549a

📥 Commits

Reviewing files that changed from the base of the PR and between 6679879 and 140a143.

📒 Files selected for processing (1)
  • core/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/views.py

📝 Walkthrough

Walkthrough

Adds a full mailing list subscription system: a UserMailingListSubscription Django model, a MailmanClient REST API wrapper, three subscription views (SubscribeView, QuickSubscribeView, ConfirmSubscriptionView) with HTMX support and signed confirmation tokens, a MailingListCardMixin wired into five existing page views, state-driven card templates, a Celery purge task, comprehensive test coverage, and Docker/dev tooling for Mailman services.

Changes

Mailing List Subscription Feature

Layer / File(s) Summary
Data model, constants, and settings foundation
mailing_list/models.py, mailing_list/migrations/0007_usermailinglistsubscription.py, mailing_list/constants.py, config/settings.py, env.template
Introduces SubscriptionStatus choices and UserMailingListSubscription model with unique_together constraints and migration. Restructures mailing list constants around MailingLists enum with dynamic domain derivation from MAILMAN_REST_API_URL. Adds MAILMAN_REST_API_VERSION setting and Mailman env vars for Docker/host API URLs and list domains.
Mailman REST API client
mailing_list/client.py
Adds MailmanAPIError and MailmanClient wrapping the Mailman REST API: subscribe (pre-verified POST, 409 no-op), is_confirmed (GET member), _discard_pending (GET/POST discard), and unsubscribe (GET + DELETE with pending cleanup).
Subscription views, URL wiring, and email helper
mailing_list/urls.py, mailing_list/views.py, config/urls.py
Routes /subscribe/, /quick-subscribe/, and /confirm/<token>/ to class-based views. Implements bulk authenticated subscription delta, single-list HTMX/PRG quick-subscribe with rate limiting, signed-token confirmation with MailmanClient.subscribe and ACTIVE status update, and _send_confirmation_email helper.
MailingListCardMixin and page-view integration
mailing_list/mixins.py, core/views.py, libraries/views.py, versions/views.py
Adds MailingListCardMixin with get_subscription_state_count_and_email (pending-over-active priority) and get_context_data injecting card context. Integrates the mixin into CommunityView, LearnPageView, LibraryDetail, VersionDetail, and populates V3ComponentDemoView with demo data and user subscriptions.
State-driven card templates and CSS
templates/v3/includes/_mailing_list_card.html, templates/v3/mailing_list/*, templates/v3/community.html, templates/v3/learn_page.html, templates/v3/libraries/library-subpage.html, templates/v3/release_detail.html, templates/v3/examples/_v3_example_section.html, templates/v3/includes/_field_text.html, static/css/v3/mailing-list-card.css, static/css/v3/mailing-list-confirm.css, static/css/v3/v3-examples-section.css
Rewrites the card template into active/pending/form/error rendering modes with HTMX. Adds success card, subscribe result fragment, confirm success/invalid pages, confirmation email template, and context-variable-passing updates across community/learn/library/release page includes. Adds card and confirm-page CSS with spinner, badge, responsive styles, and demo HTMX form section. Fixes email field validation binding.
Purge task and Celery Beat schedule
mailing_list/tasks.py, config/celery.py
Adds purge_expired_pending_subscriptions Celery task deleting 7-day-old PENDING subscription rows with logging. Schedules it daily at 3:45 AM in Celery Beat.
Task and view tests
mailing_list/tests/test_tasks.py, mailing_list/tests/test_views.py
Tests the purge task (expired/recent pending, active retention), anonymous quick-subscribe flow (email sending, already-subscribed, rate limiting, staff bypass), authenticated quick-subscribe (PENDING creation, idempotency, duplicate rejection), and confirm endpoint (token activation, invalid/expired HTTP 400, anonymous path).
Docker Compose and dev helper script
docker-compose.yml, scripts/dev-mailman-helpers
Activates mailman-core and mailman-web Docker Compose services. Adds an interactive Bash dev script using fzf for listing, creating, inspecting members, confirming pending subscriptions, and deleting Mailman lists against the local REST API.

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
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related issues

  • [Backend Integration] Mailing List Auto Subscription #2282: Implements the comprehensive mailing list subscription system with all acceptance criteria including the data model, Mailman REST API client, subscription views with email confirmation workflows, and state-driven card component wired across multiple pages.

  • Webpage Integration: Manage Mailing List #2439: Establishes foundational mailing list subscription infrastructure (MailmanClient, UserMailingListSubscription, subscription views) that a profile page manage-subscriptions feature would depend on and reuse.

Possibly related PRs

  • boostorg/website-v2#2446: Modifies core/views.py LearnPageView and templates/v3/learn_page.html—the same files updated in this PR when wiring MailingListCardMixin into the learn page.

Suggested reviewers

  • julhoang
  • jlchilders11

Poem

🐇 Hippity-hoppity, emails take flight,
A token is signed in the middle of night.
PENDING to ACTIVE with one little click,
The Mailman confirms it — surprisingly quick!
The bunny subscribes to the Boost mailing list,
No more of those newsletters ever be missed! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 46.30% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title directly and specifically describes the main implementation—Mailman mailing list subscription functionality—which aligns with the primary objective of the changeset.
Description check ✅ Passed The pull request description is comprehensive and well-structured, covering summary/context, detailed changes across all major components, risks/considerations, peer-review testing steps, and a completed self-review checklist aligned with the repository template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch teo/2282-mailing-list

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Enforce 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 win

Add a composite index for pending-subscription purge queries.

purge_expired_pending_subscriptions filters by status and subscribed_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 win

Respect reduced-motion preferences for the spinner.

Add a prefers-reduced-motion fallback 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

📥 Commits

Reviewing files that changed from the base of the PR and between 00a8e72 and 68b0572.

📒 Files selected for processing (35)
  • config/celery.py
  • config/settings.py
  • config/urls.py
  • core/views.py
  • docker-compose.yml
  • env.template
  • libraries/views.py
  • mailing_list/client.py
  • mailing_list/constants.py
  • mailing_list/migrations/0007_usermailinglistsubscription.py
  • mailing_list/mixins.py
  • mailing_list/models.py
  • mailing_list/tasks.py
  • mailing_list/tests/test_tasks.py
  • mailing_list/tests/test_views.py
  • mailing_list/urls.py
  • mailing_list/views.py
  • scripts/dev-mailman-helpers
  • static/css/v3/components.css
  • static/css/v3/mailing-list-card.css
  • static/css/v3/mailing-list-confirm.css
  • static/css/v3/v3-examples-section.css
  • templates/v3/community.html
  • templates/v3/examples/_v3_example_section.html
  • templates/v3/includes/_field_text.html
  • templates/v3/includes/_mailing_list_card.html
  • templates/v3/learn_page.html
  • templates/v3/libraries/library-subpage.html
  • templates/v3/mailing_list/_subscribe_result.html
  • templates/v3/mailing_list/_subscribe_success_card.html
  • templates/v3/mailing_list/confirm_invalid.html
  • templates/v3/mailing_list/confirm_success.html
  • templates/v3/mailing_list/email/confirm_subscription.txt
  • templates/v3/release_detail.html
  • versions/views.py

Comment thread core/views.py
Comment thread mailing_list/client.py Outdated
Comment thread mailing_list/client.py
Comment thread mailing_list/constants.py
Comment thread mailing_list/mixins.py Outdated
Comment thread mailing_list/views.py
Comment thread mailing_list/views.py
Comment thread mailing_list/views.py Outdated
Comment thread templates/v3/includes/_field_text.html Outdated
Comment thread templates/v3/mailing_list/confirm_success.html Outdated
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
{}

@kattyode kattyode left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Approved

@herzog0 herzog0 merged commit 32786c7 into develop Jun 22, 2026
5 checks passed
@herzog0 herzog0 deleted the teo/2282-mailing-list branch June 22, 2026 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Backend Integration] Mailing List Auto Subscription

6 participants