feat(2447): implement profile edit (visibility, acc. details, email prefs)#2517
feat(2447): implement profile edit (visibility, acc. details, email prefs)#2517ycanales wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds country and profile-visibility fields, extends profile form validation and choices, restructures V3 profile editing into independent section forms, adds section-specific view handling, updates field and card styling, and introduces comprehensive tests. ChangesV3 profile edit multi-section update
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant CurrentUserProfileView
participant V3UserProfileForm
participant User
Browser->>CurrentUserProfileView: POST profile-account?edit=true with section button
CurrentUserProfileView->>CurrentUserProfileView: dispatch() checks authentication
CurrentUserProfileView->>CurrentUserProfileView: post_v3() selects submitted section
CurrentUserProfileView->>V3UserProfileForm: Validate section-owned fields
V3UserProfileForm-->>CurrentUserProfileView: Validation result
CurrentUserProfileView->>User: Save selected section when valid
CurrentUserProfileView-->>Browser: Re-render errors or redirect with success
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
users/tests/test_v3_profile_edit.py (1)
68-84: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd a True→False transition case for a hide toggle.
This test only exercises turning toggles on from their default (
False) state;hide_badges is Falseis asserted but never actually toggled off from a pre-existingTrue. Consider seedinguser.hide_badges = Truebefore the POST (withhide_achomitted) to confirm the section correctly unsets it.🤖 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` around lines 68 - 84, The v3 profile edit test only covers toggles being turned on from their default state, so it misses the True-to-False path for a hide toggle. Update the test in test_v3_update_profile_saves_visibility_toggles by seeding the user’s hide_badges to True before the POST, then omit the corresponding toggle field in the form data so the profile update flow can be verified to clear it. Keep the existing assertions on user.refresh_from_db() and add/adjust the final assertion to confirm hide_badges is unset after the update.users/views.py (1)
536-557: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutable class attribute flagged by Ruff (RUF012).
V3_EDIT_SECTIONSis a plain dict assigned as a class attribute; Ruff's RUF012 recommends annotating it astyping.ClassVar[dict[...]](or using a different construct) to make the shared, immutable-in-practice nature explicit and silence the linter.♻️ Suggested annotation
+from typing import ClassVar + - V3_EDIT_SECTIONS = { + V3_EDIT_SECTIONS: ClassVar[dict] = {🤖 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` around lines 536 - 557, V3_EDIT_SECTIONS is a mutable class-level dict and Ruff flags it under RUF012; update the declaration to make its shared, read-only intent explicit by annotating it with typing.ClassVar (or moving it to an immutable construct). Keep the fix centered on the V3_EDIT_SECTIONS constant in the view class so the linter recognizes it as a class attribute rather than an instance field.Source: Linters/SAST tools
🤖 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`:
- Line 180: The Country field in the user profile edit template is passing a
separate placeholder even though country_options already contains the empty
“Select” option, causing duplicate “Select” entries in the no-JS select. Update
the include for _field_combo.html in user_profile_edit.html to rely on
country_options alone and remove the placeholder argument, keeping the
selected/error wiring unchanged.
In `@users/forms.py`:
- Around line 255-265: The `clean_username` check in `users.forms` only
validates uniqueness at the form layer, so concurrent writes can still create
duplicate `User.display_name` values. Add a database-backed case-insensitive
uniqueness guarantee on `users.User` using a `UniqueConstraint` with
`Lower("display_name")`, and include the necessary migration/data cleanup to
resolve any existing duplicates before applying it. Also update the save path
for `User` so `IntegrityError` from the database is caught and surfaced as the
same “already taken” validation error, keeping the enforcement consistent in
both `clean_username` and the model save flow.
In `@users/tests/test_v3_profile_edit.py`:
- Around line 86-105: Add coverage for the omitted-checkbox path in the v3
profile edit tests: the current test for v3_update_details only checks the
checked state, but _save_v3_details_section reads booleans directly from
form.cleaned_data so absent checkbox keys should reset to False. Add a test
around test_v3_update_details_saves_account_fields (or a sibling test) that
starts with is_commit_author_name_overridden and indicate_last_login_method
already True, submits the profile-account form via tp.post without those two
keys, and asserts both fields become False after refresh_from_db.
---
Nitpick comments:
In `@users/tests/test_v3_profile_edit.py`:
- Around line 68-84: The v3 profile edit test only covers toggles being turned
on from their default state, so it misses the True-to-False path for a hide
toggle. Update the test in test_v3_update_profile_saves_visibility_toggles by
seeding the user’s hide_badges to True before the POST, then omit the
corresponding toggle field in the form data so the profile update flow can be
verified to clear it. Keep the existing assertions on user.refresh_from_db() and
add/adjust the final assertion to confirm hide_badges is unset after the update.
In `@users/views.py`:
- Around line 536-557: V3_EDIT_SECTIONS is a mutable class-level dict and Ruff
flags it under RUF012; update the declaration to make its shared, read-only
intent explicit by annotating it with typing.ClassVar (or moving it to an
immutable construct). Keep the fix centered on the V3_EDIT_SECTIONS constant in
the view class so the linter recognizes it as a class attribute rather than an
instance field.
🪄 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: 1dce707a-6b90-405a-a972-3619c7aeeb50
📒 Files selected for processing (10)
static/css/v3/card.csstemplates/v3/includes/_button.htmltemplates/v3/includes/_field_include.htmltemplates/v3/includes/_field_text.htmltemplates/v3/user_profile_edit.htmlusers/forms.pyusers/migrations/0022_user_country_user_hide_badges_and_more.pyusers/models.pyusers/tests/test_v3_profile_edit.pyusers/views.py
| {% include 'v3/includes/_field_include.html' with bound_field=user_profile_form.username %} | ||
| {% include 'v3/includes/_field_include.html' with bound_field=user_profile_form.email %} | ||
| {% include 'v3/includes/_field_include.html' with bound_field=user_profile_form.country %} | ||
| {% include 'v3/includes/_field_combo.html' with name=user_profile_form.country.name label='Country' options=country_options selected=user_profile_form.country.value placeholder='Select' error=user_profile_form.country.errors.0 %} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd _field_combo.html
cat templates/v3/includes/_field_combo.html 2>/dev/nullRepository: boostorg/website-v2
Length of output: 7378
Avoid passing a separate placeholder here
country_options already includes ("", "Select"), so placeholder='Select' makes the no-JS <select> render two identical “Select” entries. Drop one of them.
🤖 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 `@templates/v3/user_profile_edit.html` at line 180, The Country field in the
user profile edit template is passing a separate placeholder even though
country_options already contains the empty “Select” option, causing duplicate
“Select” entries in the no-JS select. Update the include for _field_combo.html
in user_profile_edit.html to rely on country_options alone and remove the
placeholder argument, keeping the selected/error wiring unchanged.
| def clean_username(self): | ||
| username = self.cleaned_data["username"] | ||
| if not username: | ||
| return username | ||
| existing = User.objects.filter(display_name__iexact=username) | ||
| if self._user is not None: | ||
| existing = existing.exclude(pk=self._user.pk) | ||
| if existing.exists(): | ||
| raise forms.ValidationError("This username is already taken") | ||
| return username | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files users/forms.py users/models.py users/migrations || true
printf '\n== users/forms.py (around clean_username) ==\n'
nl -ba users/forms.py | sed -n '230,290p'
printf '\n== users/models.py (relevant parts) ==\n'
nl -ba users/models.py | sed -n '1,260p'
printf '\n== search for display_name constraints / uniqueness / integrity handling ==\n'
rg -n "display_name|UniqueConstraint|unique=True|IntegrityError|Lower\(" users -g '!**/__pycache__/**'
printf '\n== migrations mentioning display_name ==\n'
rg -n "display_name|UniqueConstraint|unique=True|Lower\(" users/migrations -g '!**/__pycache__/**'Repository: boostorg/website-v2
Length of output: 1353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== users/forms.py (around clean_username) ==\n'
sed -n '240,285p' users/forms.py | cat -n
printf '\n== users/models.py (top + User model area) ==\n'
sed -n '1,260p' users/models.py | cat -n
printf '\n== users/migrations/0009_user_display_name.py ==\n'
sed -n '1,220p' users/migrations/0009_user_display_name.py | cat -n
printf '\n== users/migrations/0017_populate_users_display_name.py ==\n'
sed -n '1,220p' users/migrations/0017_populate_users_display_name.py | cat -n
printf '\n== search for IntegrityError / unique constraints / display_name ==\n'
grep -RIn --exclude-dir='__pycache__' -E 'display_name|UniqueConstraint|unique=True|IntegrityError|Lower\(' users | head -n 200Repository: boostorg/website-v2
Length of output: 19906
users/forms.py:255-265 — Add a DB-level uniqueness constraint for User.display_name
clean_username() only enforces this in the form, so concurrent saves can still create duplicate display names. Add a case-insensitive UniqueConstraint(Lower("display_name")) on users.User with a migration/data cleanup for existing duplicates, and handle IntegrityError on save so the “already taken” error is enforced by the database too.
🤖 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/forms.py` around lines 255 - 265, The `clean_username` check in
`users.forms` only validates uniqueness at the form layer, so concurrent writes
can still create duplicate `User.display_name` values. Add a database-backed
case-insensitive uniqueness guarantee on `users.User` using a `UniqueConstraint`
with `Lower("display_name")`, and include the necessary migration/data cleanup
to resolve any existing duplicates before applying it. Also update the save path
for `User` so `IntegrityError` from the database is caught and surfaced as the
same “already taken” validation error, keeping the enforcement consistent in
both `clean_username` and the model save flow.
…sibility # Conflicts: # users/forms.py # users/views.py
|
On removing the In the Update Details card, the field sits in an unbroken chain of definite-height containers: the grid stretches .user-profile__col to the row height, and from there card → Tagline and the other inputs never broke because they sit inside plain block divs, which make the parent height indefinite; there height: 100% silently fell back to auto. Before: After: I double-checked other views to confirm heights were not affected:
|
1 participant
Issue: #2447
Summary & Context
Wires the v3 edit-profile page (shell landed in Story 2437) so three of its cards actually validate and persist: profile visibility toggles, account details (username, country), and email preferences. Adds four new
Userfields plus a per-section form pattern so each card saves independently.Changes
<form>with a distinct named submit button (v3_update_profile,v3_update_details,v3_update_email_preferences);post_v3()dispatches on the button name and validates/saves only that section, so an unfinished card never blocks a completed one. Section wiring lives in theV3_EDIT_SECTIONSmap inusers/views.py.dispatch()override onCurrentUserProfileViewto enforce login and route v3 POSTs intopost()/post_v3()(the v3 mixin otherwise renders on every request and never reachespost), plusget_v3_edit_initial()/get_v3_edit_context()to seed the form from the current user.Usermodel (migration0022):country(django-countriesCountryField),hide_github_activity,hide_mailing_list_activity, andhide_badges(allBooleanField).User.display_name, with case-insensitive uniqueness inclean_username) and Country (User.country), plus the existingindicate_last_login_methodandis_commit_author_name_overriddentoggles. Country was previously an empty plain dropdown and is now wired to the existing searchable combo (_field_combo.html) populated from django-countries.hide_github/hide_ml/hide_ach).Preferences.notifications(JSONField) via the existing property accessors, using a 4-item choice list (V3_EMAIL_PREFERENCE_CHOICES, no Poll to match Figma); unlisted news types a user already had enabled (e.g. Poll) are preserved on save._button.htmlaccepts aname=so multiple submit buttons can coexist on one page,_field_text.html/_field_include.htmlsurface server-side errors and seed values on re-render, andcard.cssadds.card__formso a<form>can wrap part of a card without breaking its flex-column layout.users/tests/test_v3_profile_edit.py(11 tests) covering login-required GET/POST, current-value rendering, each section's save, blank-country clearing, duplicate-username inline error, email-prefs save, and the Poll-preservation edge case.Please list any potential risks or areas that need extra attention during review/testing
0022adds four columns to theUsertable; needs to run on deploy.email, the Commit email address card, Account connections, and the top of the Profile card (tagline, bio, links, avatar) are still UI-only scaffolding and do not persist yet. Theoverride_commit_author_emailtoggle also has no backing field. These are intentionally out of scope for this PR..field__controlheight is being overridden somewhere), so they look pill-shaped. Still being diagnosed.v3waffle flag.Screenshots
Self-review Checklist
Frontend
Summary by CodeRabbit
Summary by CodeRabbit