Skip to content

(Story #2440) Webpage Integration: Commit Email Addresses - #2529

Open
herzog0 wants to merge 1 commit into
developfrom
teo/2440-commit-emails
Open

(Story #2440) Webpage Integration: Commit Email Addresses#2529
herzog0 wants to merge 1 commit into
developfrom
teo/2440-commit-emails

Conversation

@herzog0

@herzog0 herzog0 commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

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 new CommitAuthorEmail.claimed_by field 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

  • New CommitAuthorEmail.claimed_by FK (migration 0042 + backfill 0043): in the v3 flow (ask_to_claim()), asking to claim records the claimant and token on the email row only; author.user is bound once at verification via the new verify_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 binds author.user at ask time, and additionally records claimed_by so flag-off claims carry over to v3
  • VerifyCommitEmailView reworked behind the v3 flag: 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 binds author.user to the visitor, POST is 405, legacy template) - including its known flaws, which are only fixed under the flag
  • Confirm/success/error pages are v3 (templates/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 via get_template_names() + waffle check
  • Expiry mirrors the mailing-list flow: COMMIT_EMAIL_CLAIM_MAX_AGE (24h) constant + format_duration helper; 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 a v3 kwarg decided per-request via flag_is_active)
  • New v3 endpoints V3CommitAuthorEmailCreateView, V3CommitAuthorEmailWithdrawView, V3CommitAuthorEmailResendView in libraries/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); unbinding author.user survives solely as a fallback for legacy rows bound at ask time
  • New V3CommitAuthorEmailForm (the legacy CommitAuthorEmailForm is 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 legacy iexact-vs-exact 404 on case-differing input
  • All v3 claim displays and guards keyed by claimed_by; the legacy profile list stays keyed by author__user when the flag is off (a flag check in CurrentUserProfileView.get_context_data picks the queryset)
  • The card lists only emails the user actually claimed (verified, or pending with an open, unexpired ask), ordered verified first; a bound author's other imported emails are not shown. Expired asks are never swept or cleared - they just stop rendering, their token is already dead everywhere, and the next ask by anyone overwrites the claim fields. The queryset lives in one place (CommitAuthorEmail.claimed_by_user) because CurrentUserProfileView.get_context_data re-runs after get_v3_edit_context (a V3Mixin re-entrancy quirk) and overwrites commit_email_addresses - with a shared queryset the overwrite is harmless
  • New card templates templates/v3/includes/_commit_email_card.html and _commit_email_card_body.html; the body is returned wholesale by the endpoints for HTMX swap-in-place updates
  • Row model per Figma: verified emails render as read-only field rows with no controls; pending emails show inline "Pending" plus resend (reload) and withdraw (close) icon buttons; the add row reuses _field_text.html and .btn.btn-primary, with an Alpine-managed "+ Add Another" that appends extra rows client-side (handed to htmx.process so they submit via HTMX like the base row)
  • Loading state: the Submit button swaps to a spinner via the form's htmx-request class; error state reuses the existing .field--error styles with the typed value preserved
  • New static/css/v3/commit-email-card.css (semantic tokens only) and the exact pixel-art reload icon from Figma added to templates/includes/icon.html
  • csrf_token is forwarded explicitly through the {% include ... only %} chain (the stripped context otherwise rendered the card forms without CSRF inputs, causing 403s on submit)
  • Removed dead placeholder formset code from users/forms.py; profile edit context now provides the user's commit emails
  • Test infra fix (unrelated to the feature but hit while testing it): config/test_settings.py now 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 the v3 flag in the dev environment until it was re-saved in the admin

‼️ Risks & Considerations ‼️

  • Behavior change: asking to claim no longer binds author.user; attribution appears only after the inbox owner confirms. Behind the v3 flag, 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 ships
  • Verification policy follows the current flag, not the token's origin (CodeRabbit flagged this; deliberate): if v3 is rolled back while claims are pending, their outstanding links verify on GET like any legacy link. Consistent with the rule that flag off means legacy behavior verbatim, and the exposure equals what production has today; persisting a per-token flow type would need a legacy-visible migration
  • Migrations: 0042 adds claimed_by, 0043 backfills it from author.user for rows with a claim token (no bindings are removed); re-check numbering against develop before merge
  • Legacy surfaces are untouched: the legacy form, create/resend views, profile list keying, ask flow, verify flow, and email all behave exactly as the pre-branch code when the flag is off. The single delta is that the legacy ask now also records claimed_by (invisible to legacy users), so claims made while the flag is off still show in the v3 card once the flag flips on
  • Known pre-existing legacy crash (deliberately not fixed): with the flag off, /users/me/ raises NoReverseMatch whenever a bound author has unverified sibling emails with no claim token - the legacy template reverses the resend URL with claim_hash=None. This exists on develop today (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 rule
  • The "+ Add Another" extra rows are client-side only; unsubmitted values in other rows are discarded when a row's submit swaps the card body
  • config/urls.py routes use underscored path segments (commit_author_email_create_v3) to avoid collision with the boostversionslug converter

Screenshots

image image image image

Peer-review and QA testing steps

Setup (all scenarios)

  1. Enable the v3 waffle flag and log in
  2. Visit http://localhost:8000/users/me/?edit=true and find the "Commit email addresses" card
  3. Pick an email that exists in CommitAuthorEmail with 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

  1. Submit the email: the row appears with "Pending", a verification email arrives (check maildev), and in the DB claimed_by is set while author.user is still NULL
  2. Open the link from the mail (same session): a page says "You asked to link...", with the concrete expiry datetime, and nothing changes until you press Confirm
  3. Press Confirm: success page; reload the profile - the row renders read-only with no icons, author.user is now bound, and the claim can no longer be withdrawn

Scenario B - Manage a pending claim (resend / withdraw)

  1. Claim an email as in Scenario A, but don't confirm
  2. Click the reload icon: a new verification email is sent with a fresh link; the previous link stops working (the token is regenerated)
  3. Click the X icon: the claim is withdrawn (the row disappears) but the record remains in libraries_commitauthoremail with claim_hash cleared and the author unbound; the withdrawn link now shows the generic failure page

Scenario C - Verify link in the wrong hands

  1. Claim an email but don't confirm; copy the verification link
  2. Open it logged out: generic "Invalid or expired link" page, nothing changes
  3. Open it logged in as a different user: same generic failure page, nothing changes (no information about the claim leaks)
  4. Confirm the claim as the requester, then open the link again as the requester: a friendly "already been verified" page; as anyone else: still the generic failure page

Scenario D - Expired claim
(@kattyode let me know when you're testing this one, and I can help you out)

  1. Claim an email, then backdate it in the DB: update libraries_commitauthoremail set claim_hash_expiration = now() - interval '1 hour' where email = '<email>';
  2. Reload the profile: the pending row no longer renders
  3. Open the verification link: generic failure page (no hint that it expired vs. never existed)
  4. Submit the same email again (any user): the claim is re-created and a fresh link works

Scenario E - Error and conflict states

  1. Submit an email that isn't in CommitAuthorEmail at all: an inline error renders under the field with the typed value preserved
  2. Submit an email another user already verified: inline error saying it's taken
  3. Submit an email another user has a pending claim on: inline error reporting the pending state (their ask is not overwritten while unexpired)
  4. Submit a claimed email with different casing (e.g. USER@example.com): matches the existing row, no 404

Scenario F - Legacy flow with the v3 flag off

  1. Disable the v3 waffle flag and claim an email via the legacy profile form: in the DB author.user is bound immediately (pre-existing ask-time binding) and claimed_by is recorded so the claim survives a later flag flip
  2. Check maildev: the email is the original plain "Please verify your email address" one (with HTML alternative), not the v3 template
  3. Open the link while logged in: the legacy confirmation page renders and the claim is verified immediately on GET (no confirm button), with author.user bound
  4. Re-enable the flag afterwards

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 a V3 commit email management experience in user profiles.
    • Users can add, verify, resend, and withdraw commit email addresses.
    • Added pending and verified status indicators with responsive HTMX interactions.
    • Verification emails now include expiration details and requester information.
    • Added safeguards to prevent conflicting claims and protect verification details.
  • Bug Fixes

    • Isolated test caching to prevent shared development cache state.
    • Preserved legacy commit email behavior when V3 is disabled.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 43.53% 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
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.
Title check ✅ Passed The title is concise and accurately summarizes the main change: adding commit email addresses integration.
Description check ✅ Passed The description matches the required template and includes the issue, summary, changes, risks, screenshots, and checklist.
✨ 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/2440-commit-emails

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.

❤️ Share

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

@herzog0 herzog0 linked an issue Jul 15, 2026 that may be closed by this pull request
9 tasks

@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: 5

🧹 Nitpick comments (2)
users/tests/test_v3_profile_edit.py (1)

90-90: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Make this fixture belong to another user.

other_users_email currently has claimed_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 win

Cover 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

📥 Commits

Reviewing files that changed from the base of the PR and between e17725d and 60fc0d0.

📒 Files selected for processing (23)
  • config/test_settings.py
  • config/urls.py
  • libraries/constants.py
  • libraries/forms.py
  • libraries/migrations/0042_commitauthoremail_claimed_by.py
  • libraries/migrations/0043_backfill_commitauthoremail_claimed_by.py
  • libraries/models.py
  • libraries/tasks.py
  • libraries/tests/test_v3_commit_email_views.py
  • libraries/utils.py
  • libraries/views.py
  • static/css/v3/commit-email-card.css
  • static/css/v3/components.css
  • templates/includes/icon.html
  • templates/libraries/profile_confirm_email_address.html
  • templates/v3/includes/_commit_email_card.html
  • templates/v3/includes/_commit_email_card_body.html
  • templates/v3/libraries/commit_email_confirm.html
  • templates/v3/libraries/email/verify_commit_email.txt
  • templates/v3/user_profile_edit.html
  • users/forms.py
  • users/tests/test_v3_profile_edit.py
  • users/views.py
💤 Files with no reviewable changes (2)
  • templates/libraries/profile_confirm_email_address.html
  • users/forms.py

Comment thread libraries/models.py Outdated
Comment thread libraries/models.py Outdated
Comment thread libraries/views.py Outdated
Comment thread libraries/views.py Outdated
Comment thread static/css/v3/commit-email-card.css
@julhoang
julhoang self-requested a review July 15, 2026 22:17
@herzog0
herzog0 force-pushed the teo/2440-commit-emails branch from bec3263 to ec1f897 Compare July 16, 2026 13:21

@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: 6

🧹 Nitpick comments (1)
libraries/tests/test_v3_commit_email_views.py (1)

490-503: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Verify 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

📥 Commits

Reviewing files that changed from the base of the PR and between 60fc0d0 and ec1f897.

📒 Files selected for processing (22)
  • config/test_settings.py
  • config/urls.py
  • libraries/constants.py
  • libraries/forms.py
  • libraries/migrations/0042_commitauthoremail_claimed_by.py
  • libraries/migrations/0043_backfill_commitauthoremail_claimed_by.py
  • libraries/models.py
  • libraries/tasks.py
  • libraries/tests/test_v3_commit_email_views.py
  • libraries/utils.py
  • libraries/views.py
  • static/css/v3/commit-email-card.css
  • static/css/v3/components.css
  • templates/includes/icon.html
  • templates/v3/includes/_commit_email_card.html
  • templates/v3/includes/_commit_email_card_body.html
  • templates/v3/libraries/commit_email_confirm.html
  • templates/v3/libraries/email/verify_commit_email.txt
  • templates/v3/user_profile_edit.html
  • users/forms.py
  • users/tests/test_v3_profile_edit.py
  • users/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

Comment thread libraries/models.py
Comment thread libraries/models.py
Comment thread libraries/tasks.py Outdated
Comment thread libraries/tests/test_v3_commit_email_views.py Outdated
Comment thread libraries/tests/test_v3_commit_email_views.py
Comment thread libraries/views.py
@herzog0
herzog0 force-pushed the teo/2440-commit-emails branch 2 times, most recently from c42ed80 to 54a8ac5 Compare July 20, 2026 12:54

@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: 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 `@libraries/forms.py`:
- Around line 651-661: Update the open-claim validation around has_open_claim to
guard claim_hash_expiration before invoking expiration logic, treating a null
expiration as expired or otherwise not an active claim. Preserve the existing
verification-email and claimant checks, and ensure claimed_by population cannot
leave an active claim with a null expiration.
- Around line 639-649: Update the ownership check in the claim_verified
validation branch to compare self.user directly with
commit_author_email.author.user, removing the claimant fallback from the owner
determination. Preserve the existing messages and validation behavior for
matching and non-matching users.

In `@templates/v3/includes/_commit_email_card_body.html`:
- Around line 29-30: Move the field-email label and its associated markup into
the same conditional that renders the base add row, identified by the not
commit_email_addresses or commit_email_form.errors check. Ensure no label with
for="field-email" is emitted when that input is absent, and remove or update the
stale “always-rendered” comment.
🪄 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: 9be51288-0106-4058-b35d-83e946efd6ce

📥 Commits

Reviewing files that changed from the base of the PR and between 377b3ef and b522a35.

📒 Files selected for processing (22)
  • config/test_settings.py
  • config/urls.py
  • libraries/constants.py
  • libraries/forms.py
  • libraries/migrations/0042_commitauthoremail_claimed_by.py
  • libraries/migrations/0043_backfill_commitauthoremail_claimed_by.py
  • libraries/models.py
  • libraries/tasks.py
  • libraries/tests/test_v3_commit_email_views.py
  • libraries/utils.py
  • libraries/views.py
  • static/css/v3/commit-email-card.css
  • static/css/v3/components.css
  • templates/includes/icon.html
  • templates/v3/includes/_commit_email_card.html
  • templates/v3/includes/_commit_email_card_body.html
  • templates/v3/libraries/commit_email_confirm.html
  • templates/v3/libraries/email/verify_commit_email.txt
  • templates/v3/user_profile_edit.html
  • users/forms.py
  • users/tests/test_v3_profile_edit.py
  • users/views.py
🚧 Files skipped from review as they are similar to previous changes (14)
  • templates/v3/user_profile_edit.html
  • templates/includes/icon.html
  • config/test_settings.py
  • static/css/v3/components.css
  • templates/v3/libraries/email/verify_commit_email.txt
  • libraries/utils.py
  • users/views.py
  • libraries/constants.py
  • config/urls.py
  • static/css/v3/commit-email-card.css
  • libraries/tasks.py
  • users/tests/test_v3_profile_edit.py
  • libraries/views.py
  • libraries/models.py

Comment thread libraries/forms.py
Comment thread libraries/forms.py
Comment thread templates/v3/includes/_commit_email_card_body.html Outdated

@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.

Super awesome work @herzog0 ! I only have 1 tiny nit for the logger 🙌

Comment thread libraries/tasks.py Outdated

@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.

Woops forgot to select Approval :)

@julhoang

Copy link
Copy Markdown
Collaborator

@herzog0 Oh also just to double-check, will the No-JS support of this card be handled here: #2509 ? I noticed that the + Add button doesn't work without JS right now.

@jlchilders11
jlchilders11 self-requested a review July 27, 2026 14:41
@herzog0

herzog0 commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

@herzog0 Oh also just to double-check, will the No-JS support of this card be handled here: #2509 ? I noticed that the + Add button doesn't work without JS right now.

Good call @julhoang. Addressed. Now we hide the "+Add Another" button when js is disabled and always add an empty form row.

@boostorg boostorg deleted a comment from coderabbitai Bot Jul 27, 2026
Base automatically changed from cy/2447-profile-visibility to develop July 28, 2026 17:27
@herzog0
herzog0 force-pushed the teo/2440-commit-emails branch from 774a4e1 to e7f4a63 Compare July 28, 2026 18:31
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
fix(2440): address coderabbit feedback

fix(2440): render generic failure page for malformed verify tokens

fix(2440): always render the add-email row per Figma and for no-JS

fix(2440): associate the Email label with the add-row input

fix(2440): use space token for 8px margins in commit email card

fix(2440): scope ask_to_claim save to the claim fields

fix(2440): show commit email add row only when empty or on add

fix(2440): update commit email tests for conditional add row

chore: hide legacy PII exposing log

fix(2440): render a no-js commit email add row, hide + Add Another

fix(2440): keep the no-js commit email row out of htmx swaps

chore(2440): extract the shared commit email add row partial

fix(2440): avoid dangling commit email label when no add row renders
@herzog0
herzog0 force-pushed the teo/2440-commit-emails branch from e7f4a63 to 8e62850 Compare July 28, 2026 18:31

@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.

I just want to say thanks for tackling this, there was a lot of considerations behind this huge rework! I'm pre approving because the work looks good, but there is one corner case I'd like to consider. It can be resolved by admin action, but I'd prefer to avoid that as much as possible.

Comment thread libraries/forms.py
if self.user is not None and claimant == self.user:
msg = "A verification email is already pending for this address."
else:
msg = "This email address has a verification pending by another user."

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.

Could someone create a hostile interaction by repeatedly false claiming an email, even if they could never verify it? Since we only allow a single claimant at a time, you could hold someone elses email hostage with a fairly simple script.

@herzog0 herzog0 Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Potentially, yes. I thought about this as well, but ended up like this:

Path 1) We add a locked time window between claims from the same user (their record stays there even after expiry, so we can use that to confirm a re-attempt), this would give time for the real claimant to get their email;

Path 2) No locks, if this ever happens to the real claimant, they will reach out for support, who will take a more direct approach to fixing it (maybe we should have a ban user feature, and maybe the admin panel already offers something like this);

But then, it feels to me that Path #1 will always end up in Path #2.
The expiry is in 7 days, and real claimants won't know that, so they'll reach out to support anyways. Even better if they do that, as admins will be able to spot any smart fellas trying to be funny.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thus, I left the feature like this. Let me know if you have some parallel thoughts about it!

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.

Hmm... I think you are correct that we will ultimately probably need admin action to resolve bad actors in this case. Without a better solution I'm okay with putting a pin in that, but I do think that is a good future upgrade.

@herzog0

herzog0 commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

@kattyode this one depends on #2554 too, so let's consider it QA-blocked until that's resolved ok?
cc @henryajisegiri

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.

Webpage Integration: Commit Email Addresses

3 participants