Story #2439: Manage Mailing Lists in the Profile Edit UI - #2538
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe profile view now renders mailing-list controls through ChangesMailing-list profile integration and local setup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
herzog0
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
users/views.py (1)
106-106: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid recomputing mailing-list context on v3 requests.
With
MailingListCardMixinbeforeV3Mixin, the v3 context flow re-entersCurrentUserProfileView.get_context_data(), causing the mailing-list state lookup and subscribed-ID query to execute twice per request. Cache the mixin context for the request or adjust the v3 context hook so this work runs once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@users/views.py` at line 106, Update the context flow involving MailingListCardMixin, V3Mixin, and CurrentUserProfileView.get_context_data so mailing-list state and subscribed-ID lookups execute only once for each v3 request. Cache the mixin-generated context per request or adjust the v3 context hook to reuse the existing result, while preserving the current context values.
🤖 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.
Nitpick comments:
In `@users/views.py`:
- Line 106: Update the context flow involving MailingListCardMixin, V3Mixin, and
CurrentUserProfileView.get_context_data so mailing-list state and subscribed-ID
lookups execute only once for each v3 request. Cache the mixin-generated context
per request or adjust the v3 context hook to reuse the existing result, while
preserving the current context values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3af821e-564f-4b10-b776-ce2c3d10791b
📒 Files selected for processing (2)
templates/v3/user_profile_edit.htmlusers/views.py
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Hi @herzog0 ! I'm having a bit of trouble testing this. Do you happen to know what's going on here?
The mailman-core and mailman-web containers in docker-compose.yml were disabled a few weeks back – I already uncommented them and run just rebuild and docker compose up but still see some errors:
|
Hi @herzog0 ! I was able to get the testing working, however I had to make a few code changes because I ran into an issue with the mailman host & lists. Here's the steps that I did:
I'm curious if my set up was wrong somehow that leads to me having to hardcode the domain, since I had already went though the original set up steps without having this override back when you implemented the first subscription ticket. 🤔 |
|
Hey @julhoang my apologies, I forgot to add the new test instructions for this here. |
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 `@docs/mailman/README.md`:
- Around line 53-59: Update the ngrok discussion in the documentation to explain
that changing its hostname changes the import-time MAILMAN_DOMAIN and managed
list IDs, so existing lists and subscriptions under the previous domain appear
missing; advise using a stable hostname or recreating the lists and restarting
all consumers after changing the URL.
🪄 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: 849db1b7-a15d-4982-b2a9-984f69bbd215
📒 Files selected for processing (3)
docs/mailman/README.mdenv.templatescripts/dev-mailman-helpers
💤 Files with no reviewable changes (1)
- env.template
d407aa4 to
eff31fc
Compare
There was a problem hiding this comment.
Hey @julhoang, I made small modifications to the helper script and the docker compose file, now things should be easier to test. I also added this doc file here. Let me know if you have any other questions!
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/mailman/local-testing.md`:
- Line 11: Update the fenced environment-variable example in the `.env`
documentation block to specify a language identifier, preferably dotenv (or
text), immediately after the opening backticks so it satisfies Markdownlint
MD040.
- Around line 60-68: The troubleshooting section should lead with a general
Mailman subscription failure rather than implying the API is unreachable, then
check the existing DNS resolution, configuration restart, and list-creation
steps in order. Add domain-mismatch diagnosis alongside missing lists, directing
readers to verify the requested list domain matches the domain configured in
Mailman.
- Around line 26-28: Update the initial “list lists” step in the local testing
instructions to state that an empty instance is expected to emit “No lists found
at $URL” on stderr and exit non-zero, rather than printing nothing. Keep the
subsequent create-lists and verification steps unchanged.
🪄 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: 61e38af8-f596-40bb-ae6c-2e99035e12a7
📒 Files selected for processing (5)
docker-compose.ymldocs/mailman/README.mddocs/mailman/local-testing.mdenv.templatescripts/dev-mailman-helpers
🚧 Files skipped from review as they are similar to previous changes (2)
- env.template
- scripts/dev-mailman-helpers
javiercoronadonarvaez
left a comment
javiercoronadonarvaez
left a comment
There was a problem hiding this comment.
LGTM! Great work as always Teo.
254f470 to
33c8b7b
Compare
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/user_profile_edit.html`:
- Around line 291-305: Replace the inert delete-account card in the profile
template with the existing conditional delete flow from the earlier
delete-account section, including its form action, dialog trigger, and
cancellation state when delete_permanently_at is set. Remove the original card
so only one delete-account card renders, and remove the left-column copy while
preserving the existing modal behavior.
🪄 Autofix
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: 08b18eec-d7d0-4a99-ab80-90b1820d237d
📒 Files selected for processing (2)
templates/v3/user_profile_edit.htmlusers/views.py
33c8b7b to
3993a79
Compare
docs: mailman docs docs: use network alias instead of tunnel docs: finish mailman docs docs: refactor docs
fix: correct empty-instance behavior in mailman list lists docs fix: add domain-mismatch diagnosis to mailman troubleshooting docs
f6ad703 to
f868eab
Compare
4 participants
Issue: #2439
Summary & Context
Adds mailing list management to the edit profile page. The existing subscribe/manage card (already used on the community, learn, library, and release pages) is reused as-is at the bottom of the left column, and the "Delete account" card moves to the bottom of the right column to make room for it.
⚠️ This implementation drifts from the Figma specification, but this is intentional and confirmed/accepted by Rob in this thread.
Changes
CurrentUserProfileViewnow includesMailingListCardMixin(users/views.py), so the edit page gets the samemailing_list_card_*context (subscribe/modal URLs, subscription state, list options) as every other page that embeds the card.templates/v3/user_profile_edit.html: the mailing-list card (_mailing_list_card.html) is now rendered at the bottom of the left column, right after the Profile card. No new markup, CSS, or JS - it's the exact same include and behavior (quick-subscribe form, "Manage your lists" modal, HTMX swap, no-JS PRG fallback) used elsewhere.mailing-list-card.cssis already loaded globally viacss/v3/components.cssinbase.html.Please list any potential risks or areas that need extra attention during review/testing
manage_urlfor this card resolves toprofile-account(this same edit page), so "Manage your lists" from other pages and the card's own state on this page both point back here - confirmed this doesn't create a redirect loop.autoInit is not defined) coming from the Biography WYSIWYG editor init script on this page - present before this change and not touched by it.Screenshots
Peer-review testing steps
First of all, checkout the new doc file in
docs/mailman/local-testing.md. After reading through that and settings things up, come back to the steps below (skipping what you've already done).v3Waffle flag and log in./users/me/?edit=true. Confirm the "Join the Boost Developers Mailing List" card renders at the bottom of the left column, right after the Profile card./community/.Self-review Checklist
Frontend
Summary by CodeRabbit
Summary by CodeRabbit
New Features
UI Changes
Documentation
Chores