(Story #2440) Webpage Integration: Commit Email Addresses#2529
Open
herzog0 wants to merge 7 commits into
Open
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
9 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
users/tests/test_v3_profile_edit.py (1)
90-90: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake this fixture belong to another user.
other_users_emailcurrently hasclaimed_by=None, so the test does not prove another claimant’s verified email is excluded from this user’s profile.Proposed fix
- other_users_email = baker.make("libraries.CommitAuthorEmail", claim_verified=True) + other_user = baker.make("users.User") + other_users_email = baker.make( + "libraries.CommitAuthorEmail", + claim_verified=True, + claimed_by=other_user, + )🤖 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/tests/test_v3_profile_edit.py` at line 90, Update the other_users_email fixture in the profile edit test to assign it to a different user via claimed_by while keeping claim_verified=True. Ensure the fixture represents another claimant’s verified email so the profile exclusion behavior is exercised.libraries/tests/test_v3_commit_email_views.py (1)
398-441: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover resend authentication and claimant ownership.
Add tests proving logged-out users are redirected and one user cannot resend another user’s pending claim. These are the resend endpoint’s key authorization boundaries.
Suggested coverage
# --- resend --- +def test_v3_commit_email_resend_requires_login(tp): + claimant = baker.make("users.User") + commit_author_email = _pending_claim(claimant) + + response = tp.post( + tp.reverse("v3-commit-author-email-resend", pk=commit_author_email.pk) + ) + + tp.response_302(response) + assert "login" in response.url + + +def test_v3_commit_email_resend_refuses_other_users_email( + user, tp, mailoutbox +): + other_user = baker.make("users.User") + commit_author_email = _pending_claim(other_user) + old_hash = commit_author_email.claim_hash + + with tp.login(user): + response = tp.post( + tp.reverse( + "v3-commit-author-email-resend", + pk=commit_author_email.pk, + ) + ) + + tp.response_404(response) + commit_author_email.refresh_from_db() + assert commit_author_email.claimed_by == other_user + assert commit_author_email.claim_hash == old_hash + assert not mailoutbox🤖 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 `@libraries/tests/test_v3_commit_email_views.py` around lines 398 - 441, Add resend authorization tests alongside test_v3_commit_email_resend_sends_new_verification: verify a logged-out request to the v3-commit-author-email-resend endpoint redirects to login, and verify a different authenticated user cannot resend another user’s pending claim, returning the endpoint’s unauthorized/not-found response with no email sent. Reuse the existing pending-claim setup and mailoutbox assertions.
🤖 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 `@libraries/models.py`:
- Around line 215-220: Wrap claim creation, withdrawal, and verification
validation/mutation flows in transaction.atomic(). Lock the email row with
select_for_update() before updating claimed_by, claim_hash, or
claim_hash_expiration, and lock the author and sibling claims during
verification. Ensure all related state changes and conflict checks occur while
these locks are held, preventing concurrent transitions from overwriting or
becoming verified inconsistently.
- Around line 289-292: Update author_still_bound so attribution is preserved
only when another claim is active and belongs to the same claimant. Exclude
expired claims and claims owned by other users; do not treat
claim_hash__isnull=False alone as sufficient. Preserve the existing
self-exclusion and return false when no qualifying sibling claim exists.
In `@libraries/views.py`:
- Around line 760-765: Make the complete claim lifecycle atomic by centralizing
claim transitions in transactional model methods that lock and revalidate state.
In libraries/views.py lines 760-765, atomically lock the claim and its
author/sibling claims during verification before binding attribution; update
lines 698-700 and 821-829 to use the atomic acquisition method for legacy and V3
creation; serialize withdrawal at lines 847-855 and token rotation at lines
865-873 with verification and resend. Preserve eligibility, claimant, expiry,
token, and verification checks inside the locked operations.
- Around line 692-700: Update CommitAuthorEmailCreateView to inherit from
LoginRequiredMixin so anonymous POST requests are rejected before form
validation and trigger_verification_email executes. Preserve the existing
authenticated create flow and mixin behavior.
In `@static/css/v3/commit-email-card.css`:
- Around line 135-146: Update the .commit-email__spinner sizing so its 3px
border is included within the declared 24px dimensions, preserving the
documented 128x40 Submit-button footprint and preventing layout shift.
---
Nitpick comments:
In `@libraries/tests/test_v3_commit_email_views.py`:
- Around line 398-441: Add resend authorization tests alongside
test_v3_commit_email_resend_sends_new_verification: verify a logged-out request
to the v3-commit-author-email-resend endpoint redirects to login, and verify a
different authenticated user cannot resend another user’s pending claim,
returning the endpoint’s unauthorized/not-found response with no email sent.
Reuse the existing pending-claim setup and mailoutbox assertions.
In `@users/tests/test_v3_profile_edit.py`:
- Line 90: Update the other_users_email fixture in the profile edit test to
assign it to a different user via claimed_by while keeping claim_verified=True.
Ensure the fixture represents another claimant’s verified email so the profile
exclusion behavior is exercised.
🪄 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: 8e4e3981-3218-4102-ba43-938ced038154
📒 Files selected for processing (23)
config/test_settings.pyconfig/urls.pylibraries/constants.pylibraries/forms.pylibraries/migrations/0042_commitauthoremail_claimed_by.pylibraries/migrations/0043_backfill_commitauthoremail_claimed_by.pylibraries/models.pylibraries/tasks.pylibraries/tests/test_v3_commit_email_views.pylibraries/utils.pylibraries/views.pystatic/css/v3/commit-email-card.cssstatic/css/v3/components.csstemplates/includes/icon.htmltemplates/libraries/profile_confirm_email_address.htmltemplates/v3/includes/_commit_email_card.htmltemplates/v3/includes/_commit_email_card_body.htmltemplates/v3/libraries/commit_email_confirm.htmltemplates/v3/libraries/email/verify_commit_email.txttemplates/v3/user_profile_edit.htmlusers/forms.pyusers/tests/test_v3_profile_edit.pyusers/views.py
💤 Files with no reviewable changes (2)
- templates/libraries/profile_confirm_email_address.html
- users/forms.py
julhoang
self-requested a review
July 15, 2026 22:17
feat(2440): wire commit email card into v3 profile edit fix(2440): improve commit email add rows and error handling fix(2440): order commit emails verified first, then pending fix(2440): withdraw pending claim instead of deleting the email record feat(2440): bind commit email claims at verification via claimed_by feat(2440): v3 confirm pages for commit email verification link feat(2440): surface claim expiry in email and confirm pages fix(2440): show concrete expiry datetime on confirm page fix(2440): keep verification failure page generic fix(2440): anonymous wording when claimant has no display name fix(2440): single claimed-emails queryset so profile render can't clobber it feat(2440): second-person copy when the claimant opens their own link feat(2440): verify link only works in the claimant's own session feat(2440): hide expired pending claims from the commit email lists fix: isolate test cache from dev Redis so waffle flags survive test runs fix: require login on legacy commit email claim endpoint fix: only the claimant's active sibling claims keep legacy attribution fix: serialize claim lifecycle transitions with row locks fix: include spinner border inside its 24px box test: cover resend auth boundaries and foreign verified email exclusion fix: restore legacy verify screen and flow behind the v3 flag fix: restore legacy verification email when the v3 flag is off fix(2440): restore legacy claim flow verbatim, v3 gets its own form and ask method fix(2440): legacy ask also records claimed_by so claims survive the flag flip
herzog0
force-pushed
the
teo/2440-commit-emails
branch
from
July 16, 2026 13:21
bec3263 to
ec1f897
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
libraries/tests/test_v3_commit_email_views.py (1)
490-503: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify that resend delivers the refreshed token and expiry.
This test passes even if resend changes the database token but emails the old URL or leaves the original expiry unchanged, making a near-expiry resend unusable.
Proposed assertions
def test_v3_commit_email_resend_sends_new_verification(user, tp, mailoutbox): commit_author_email = _pending_claim(user) old_hash = commit_author_email.claim_hash + old_expiration = commit_author_email.claim_hash_expiration ... commit_author_email.refresh_from_db() assert commit_author_email.claim_hash != old_hash + assert commit_author_email.claim_hash_expiration > old_expiration assert len(mailoutbox) == 1 + assert str(commit_author_email.claim_hash) in mailoutbox[0].body + assert str(old_hash) not in mailoutbox[0].body🤖 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 `@libraries/tests/test_v3_commit_email_views.py` around lines 490 - 503, Extend test_v3_commit_email_resend_sends_new_verification to inspect the single message in mailoutbox and assert its verification URL contains the refreshed commit_author_email.claim_hash. Also capture the pre-resend expiry and assert the refreshed expiry changes appropriately, preserving the existing database-token and message-count assertions.
🤖 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 `@libraries/models.py`:
- Around line 306-317: Update the conflicting_claim branch in the commit-email
association method to return an explicit conflict outcome rather than a
successful/implicit return, while preserving the existing warning and avoiding
changes to author.user. Update the POST caller to detect that outcome and render
conflict-specific copy instead of confirmed=True, while keeping the successful
association path unchanged.
- Around line 273-278: Update V3CommitAuthorEmailResendView’s email dispatch
around ask_to_claim() to register the send_commit_author_email_verify_mail.delay
call with transaction.on_commit(), preserving its existing arguments. Ensure the
Celery task is enqueued only after the surrounding transaction successfully
commits, rather than while the row lock is held.
In `@libraries/tasks.py`:
- Line 507: Update the logging statement in the verification-email flow to
remove both the verification URL and recipient address from the log message.
Retain only a non-sensitive status message indicating that the verification
email is being sent.
In `@libraries/tests/test_v3_commit_email_views.py`:
- Around line 612-619: Scope CSRF assertions to the forms that submit the
protected actions: in libraries/tests/test_v3_commit_email_views.py:612-619,
locate the confirmation form and assert its markup contains the CSRF input; in
users/tests/test_v3_profile_edit.py:163-186, inspect every commit-email card
form and assert each form contains its own CSRF input rather than relying on a
page-wide assertion.
- Around line 637-666: Update the local _body helper to explicitly assert that
both the “mailing-list-confirm__header” start marker and the “</main>” end
marker are present before slicing; fail the test when either find result is -1,
then return the existing confirmation-component slice.
In `@libraries/views.py`:
- Around line 783-784: Update the verification flow around the feature-flag
branch and the corresponding logic at lines 800-803 so token verification is
determined by the token’s persisted flow type, not the current v3 flag state.
Ensure V3 tokens always use V3 verification, including the claimant-session POST
requirement, even after the flag is disabled; alternatively route V3 tokens
through a dedicated verification endpoint.
---
Nitpick comments:
In `@libraries/tests/test_v3_commit_email_views.py`:
- Around line 490-503: Extend test_v3_commit_email_resend_sends_new_verification
to inspect the single message in mailoutbox and assert its verification URL
contains the refreshed commit_author_email.claim_hash. Also capture the
pre-resend expiry and assert the refreshed expiry changes appropriately,
preserving the existing database-token and message-count assertions.
🪄 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: a4751a58-b307-45c7-be13-066930247e25
📒 Files selected for processing (22)
config/test_settings.pyconfig/urls.pylibraries/constants.pylibraries/forms.pylibraries/migrations/0042_commitauthoremail_claimed_by.pylibraries/migrations/0043_backfill_commitauthoremail_claimed_by.pylibraries/models.pylibraries/tasks.pylibraries/tests/test_v3_commit_email_views.pylibraries/utils.pylibraries/views.pystatic/css/v3/commit-email-card.cssstatic/css/v3/components.csstemplates/includes/icon.htmltemplates/v3/includes/_commit_email_card.htmltemplates/v3/includes/_commit_email_card_body.htmltemplates/v3/libraries/commit_email_confirm.htmltemplates/v3/libraries/email/verify_commit_email.txttemplates/v3/user_profile_edit.htmlusers/forms.pyusers/tests/test_v3_profile_edit.pyusers/views.py
💤 Files with no reviewable changes (1)
- users/forms.py
🚧 Files skipped from review as they are similar to previous changes (7)
- templates/v3/libraries/email/verify_commit_email.txt
- static/css/v3/components.css
- libraries/utils.py
- templates/includes/icon.html
- users/views.py
- config/test_settings.py
- static/css/v3/commit-email-card.css
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #2440
Summary & Context
Adds the "Commit email addresses" card to the v3 profile edit page so users can claim commit-author emails and aggregate their contribution history, and redesigns the claim flow so that public attribution (
CommitAuthor.user) is bound at verification instead of at ask time. A newCommitAuthorEmail.claimed_byfield records who asked; the verification link becomes a landing page naming the requester with an explicit Confirm button (no state change on GET). Pending claims can be withdrawn side-effect-free; verified ones are permanent.Changes
CommitAuthorEmail.claimed_byFK (migration0042+ backfill0043): in the v3 flow (ask_to_claim()), asking to claim records the claimant and token on the email row only;author.useris bound once at verification via the newverify_claim()(which never steals an author on which a different user holds a verified claim - conflict is logged instead). The legacy ask flow (trigger_verification_email(), form, create/resend views) is preserved - it still bindsauthor.userat ask time, and additionally recordsclaimed_byso flag-off claims carry over to v3VerifyCommitEmailViewreworked behind thev3flag: the link only works in the claimant's own authenticated session (proving a claim takes token + login), so nothing can ever happen on the claimant's behalf; GET renders a confirm page with no state change (prefetching mail scanners can't auto-verify), the claim completes on the explicit POST (also fixes the anonymous-click 500); any other visitor - logged out, or a different account - gets the same generic failure page as an invalid token, and a friendly "already verified" state shows to the claimant only. With the flag off, the pre-existing legacy behavior is preserved verbatim (GET verifies immediately and bindsauthor.userto the visitor, POST is 405, legacy template) - including its known flaws, which are only fixed under the flagtemplates/v3/libraries/commit_email_confirm.html), reusing the mailing-list confirm-page component (mailing-list-confirm.css+_button.html) for visual consistency; the legacy confirm template (libraries/profile_confirm_email_address.html) is kept untouched and routed viaget_template_names()+ waffle checkCOMMIT_EMAIL_CLAIM_MAX_AGE(24h) constant +format_durationhelper; behind the flag the verification email is a rendered text template (v3/libraries/email/verify_commit_email.txt) naming the requester and the expiry window, and the confirm page shows the concrete expiry datetime; the failure page stays deliberately generic (does not reveal whether a token exists or merely expired). With the flag off, the original plain text+HTML "Please verify your email address" email is sent unchanged (the task takes av3kwarg decided per-request viaflag_is_active)V3CommitAuthorEmailCreateView,V3CommitAuthorEmailWithdrawView,V3CommitAuthorEmailResendViewinlibraries/views.py, all HTMX-aware with non-JS redirect fallbacks; withdraw/resend refuse verified emails, other users' claims, and emails with no open ask (404)withdraw_claim()clears only the claim fields on the row (the email record is never deleted); unbindingauthor.usersurvives solely as a fallback for legacy rows bound at ask timeV3CommitAuthorEmailForm(the legacyCommitAuthorEmailFormis untouched): per-row claim state is checked first (verified yours/other, pending yours/other), expired or orphaned claims are re-claimable, and authors bound only by the matching heuristics no longer dead-end the real owner (a different user's verified sibling claim still blocks); the matched row is stored on the form, avoiding the legacyiexact-vs-exact 404 on case-differing inputclaimed_by; the legacy profile list stays keyed byauthor__userwhen the flag is off (a flag check inCurrentUserProfileView.get_context_datapicks the queryset)CommitAuthorEmail.claimed_by_user) becauseCurrentUserProfileView.get_context_datare-runs afterget_v3_edit_context(aV3Mixinre-entrancy quirk) and overwritescommit_email_addresses- with a shared queryset the overwrite is harmlesstemplates/v3/includes/_commit_email_card.htmland_commit_email_card_body.html; the body is returned wholesale by the endpoints for HTMX swap-in-place updates_field_text.htmland.btn.btn-primary, with an Alpine-managed "+ Add Another" that appends extra rows client-side (handed tohtmx.processso they submit via HTMX like the base row)htmx-requestclass; error state reuses the existing.field--errorstyles with the typed value preservedstatic/css/v3/commit-email-card.css(semantic tokens only) and the exact pixel-art reload icon from Figma added totemplates/includes/icon.htmlcsrf_tokenis forwarded explicitly through the{% include ... only %}chain (the stripped context otherwise rendered the card forms without CSRF inputs, causing 403s on submit)users/forms.py; profile edit context now provides the user's commit emailsconfig/test_settings.pynow uses local-memory caches so pytest runs stop sharing the dev server's Redis - previously every test run poisoned the waffle flag cache and deactivated thev3flag in the dev environment until it was re-saved in the adminauthor.user; attribution appears only after the inbox owner confirms. Behind thev3flag, the verify link requires an explicit button press (mail scanners and one-click habits no longer complete verification); with the flag off the legacy instant-verify-on-GET flow and email are preserved verbatim, so the anonymous-click 500 and scanner auto-verify remain possible there until v3 ships0042addsclaimed_by,0043backfills it fromauthor.userfor rows with a claim token (no bindings are removed); re-check numbering againstdevelopbefore mergeclaimed_by(invisible to legacy users), so claims made while the flag is off still show in the v3 card once the flag flips on/users/me/raisesNoReverseMatchwhenever a bound author has unverified sibling emails with no claim token - the legacy template reverses the resend URL withclaim_hash=None. This exists ondeveloptoday (sync-bound or multi-email authors) and is easy to reach during QA of this PR: verify a claim on a multi-email author under v3, then flip the flag off. Left as-is per the legacy-verbatim ruleconfig/urls.pyroutes use underscored path segments (commit_author_email_create_v3) to avoid collision with theboostversionslugconverterScreenshots
Peer-review and QA testing steps
Setup (all scenarios)
v3waffle flag and log inCommitAuthorEmailwith no linked user (e.g.select email from libraries_commitauthoremail ce join libraries_commitauthor a on ce.author_id = a.id where a.user_id is null limit 5;)Scenario A - Happy path: claim and verify
claimed_byis set whileauthor.useris still NULLauthor.useris now bound, and the claim can no longer be withdrawnScenario B - Manage a pending claim (resend / withdraw)
libraries_commitauthoremailwithclaim_hashcleared and the author unbound; the withdrawn link now shows the generic failure pageScenario C - Verify link in the wrong hands
Scenario D - Expired claim
(@kattyode let me know when you're testing this one, and I can help you out)
update libraries_commitauthoremail set claim_hash_expiration = now() - interval '1 hour' where email = '<email>';Scenario E - Error and conflict states
CommitAuthorEmailat all: an inline error renders under the field with the typed value preservedUSER@example.com): matches the existing row, no 404Scenario F - Legacy flow with the v3 flag off
v3waffle flag and claim an email via the legacy profile form: in the DBauthor.useris bound immediately (pre-existing ask-time binding) andclaimed_byis recorded so the claim survives a later flag flipauthor.userboundSelf-review Checklist
Frontend
Summary by CodeRabbit
New Features
Bug Fixes