Story 2443: User Profile Integration – User Roles - #2527
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:
📝 WalkthroughWalkthroughChangesAdds dynamic library-scoped and internal profile roles, persisted role selections, validation, administrative controls, recomputation triggers, opt-out behavior, and updated v3 profile and author rendering. Displayed profile role system
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProfilePage
participant CurrentUserProfileView
participant V3UserProfileForm
participant CurrentUserSerializer
participant User
ProfilePage->>CurrentUserProfileView: request edit context
CurrentUserProfileView->>User: compute role options
CurrentUserProfileView->>V3UserProfileForm: seed offered role choices
ProfilePage->>CurrentUserSerializer: submit encoded role
CurrentUserSerializer->>User: validate and persist role selection
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
julhoang
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@users/tasks.py`:
- Around line 176-197: Update the stale-selection cleanup around stale_ids so it
cannot overwrite a concurrent legitimate selection: retain each stale user’s
exact displayed_profile_role and displayed_profile_role_library values, then
perform the clearing update inside the transaction only when both fields still
match those snapshot values. Keep the existing role-resolution updates unchanged
and ensure concurrent changes cause the conditional clear to affect no rows.
In `@users/tests/test_profile_role.py`:
- Around line 313-324: Strengthen the unauthorized-role rejection tests,
including test_post_rejects_role_for_unheld_library and the corresponding test
around the second reported range, by first assigning the user an eligible
displayed profile role for library before submitting the tampered role. After
the POST, assert the original role and library ID remain unchanged, proving the
request is rejected rather than ignored.
🪄 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: ff96fd6c-a85d-4211-8cda-0b492fe0003b
📒 Files selected for processing (17)
ak/homepage.pyconfig/celery.pycore/views.pylibraries/tasks.pynews/models.pynews/views.pystatic/css/v3/user-profile-page.csstemplates/v3/posts_list.htmltemplates/v3/user_profile_edit.htmlusers/admin.pyusers/forms.pyusers/migrations/0022_user_displayed_profile_role_and_more.pyusers/models.pyusers/profile_cards.pyusers/tasks.pyusers/tests/test_profile_role.pyusers/views.py
💤 Files with no reviewable changes (1)
- users/profile_cards.py
e212783 to
1871c93
Compare
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
Looking pretty good! Tested all cases and they're looking great.
Only one thing I noticed: there's no way to de-select the role if I want it.
I understand this is not originally in the ticket, so I'm approving!
Let's discuss internally.
Sorry, I think there's something else I didn't consider in the first review, let me finish double checking
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
Heya! I was finally able to reproduce the issue I mentioned last week and get the step-by-step instructions for you to reproduce.
TL;DR
The field resolved_profile_role is a cache, and can become stale/invalid after running recompute_displayed_profile_roles.
If the role is not applicable to the user anymore, and they don't have valid options to set in their role field in the profile, then resolved_profile_role is filled into the selector while it's disabled (cause the user has no other valid roles to apply, by the fresh data pulled from the tables and ignoring what's cached, so the field is locked).
If you then update any unrelated field and hit save, you'll get an error, because the form still submits the cached invalid role in the request, which the backend correctly validates and sees the role is invalid.
More info the code.
| resolved_profile_role = models.CharField( | ||
| max_length=64, | ||
| blank=True, | ||
| default="", | ||
| editable=False, | ||
| help_text=_( | ||
| "Auto-derived top library role, recomputed on import. Used only when " | ||
| "the user has set neither displayed_profile_role nor internal_role." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
About the issue I mentioned in the review.
Importante note
About the time window when the error might happen, there are two:
- Super low criticality → happens if the
recompute_displayed_profile_rolesfunction gets delayed after the other tasks run (it clears the cached field at the end, so if this function runs pretty close to the ingestion updates, we're fine); - Mid criticality → if an admin manually revokes the role in the admin panel, then the user will get an error until the next day when the task runs again;
Reproducing the error
Data setup
- Find a safe candidate - zero real library eligibility (so the dropdown renders disabled) and no linked social account (so the edit page doesn't 500 — a separate, unrelated local-env gap I hit while
testing this):
SELECT u.id, u.email, u.is_active, u.claimed
FROM users_user u
WHERE u.claimed IS TRUE
AND u.is_active IS TRUE
AND u.id NOT IN (SELECT user_id FROM libraries_library_authors)
AND u.id NOT IN (SELECT user_id FROM libraries_libraryversion_maintainers)
AND u.id NOT IN (SELECT user_id FROM libraries_commitauthor WHERE user_id IS NOT NULL)
AND u.id NOT IN (SELECT user_id FROM socialaccount_socialaccount)
ORDER BY u.id
LIMIT 20;- Force the stale state on the chosen account:
UPDATE users_user
SET resolved_profile_role = 'Contributor' -- or 'Author' / 'Maintainer'
WHERE email = '<chosen-email>';- Make sure they can actually log in (only needed if the account has no known password/unverified email):
-- Known password → plaintext "foobarone"
UPDATE users_user
SET password = 'pbkdf2_sha256$1200000$UrzH9CXDMIhtHEmReBUuX4$c1qlT3Qsqarwc2tQ/SuCz0xKzfPatfTuQOABNYiXg5E='
WHERE email = '<chosen-email>';
-- Confirm the primary email is verified (allauth blocks unverified accounts)
SELECT email, verified, "primary" FROM account_emailaddress WHERE email = '<chosen-email>';
-- If verified = false:
UPDATE account_emailaddress SET verified = true WHERE email = '<chosen-email>';- Verify the setup before testing:
SELECT id, email, resolved_profile_role, displayed_profile_role,
displayed_profile_role_library_id, internal_role, is_active, claimed
FROM users_user
WHERE email = '<chosen-email>'; resolved_profile_role should show the value you set; displayed_profile_role / internal_role should be empty.
- Revert once done:
UPDATE users_user SET resolved_profile_role = '' WHERE email = '<chosen-email>';Steps to reproduce
- Run query 1, pick an id/email from the results.
- Run query 2 on that account.
- Run query 3 if the account's password/verification is unknown.
- Run query 4 to confirm the row looks right.
- Log in as that account (
<chosen-email> / foobaroneif query 3 was used). - Go to
/users/me/?edit=true- "Your Role" should render disabled/locked with the "Contribute to a library to unlock a role" placeholder (but in my tests it's actually showing an empty field, still locked). - Open DevTools → Network before saving.
- Change any unrelated field (e.g. add a GitHub link) and click Save Changes.
- Inspect the PATCH to
/api/v1/users/me/: the payload contains "role": "Contributor:" (or whichever role was set), the response is 400 "You cannot select that role," and the unrelated field does not save. - Run query 5 to clean up.
There was a problem hiding this comment.
A possible route to fix this is to never resubmit a value for a field the server told it not to offer. That won't fix the root cause but at least the user won't experience any issues anymore.
There was a problem hiding this comment.
Thanks for catching this issue @herzog0 !
Following your suggestion, I've updated _field_dropdown.html so it never submits a value when the field is disabled (i.e. in this case role is omitted from the request).
Now for the dropdown's initial seed value: it now checks the stored/cached role against the real-time eligibility list from get_role_options(). If the stored role isn't in that list (i.e. we know the cache is stale), we don't pre-select an option to be on the safe side. Then the role will be set if either user picks a valid role themselves, or the cached value gets reconciled on the next daily recompute. I think this should mitigate the issue you found above. Does this approach seems reasonable to you too?
Commit ref: ee1852b
1871c93 to
ee1852b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
users/serializers.py (1)
79-83: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize bare Slack Member IDs to canonical URLs.
The validation logic intentionally accepts bare Member IDs (via
is_valid_slack_link), but fails to normalize them into the canonical URL before saving. If a user bypasses the frontend and submits a bare ID, it will be saved as-is, violating the database contract described inmodels.py("Slack stores the full canonical CPPLang Slack profile URL") and resulting in broken profile links.Normalize the value to the canonical URL if it's a bare Member ID.
🐛 Proposed fix to normalize the Member ID
- slack_value = value.get(V3ProfileLinkChoices.SLACK) - if slack_value and not is_valid_slack_link(slack_value): - field_errors[V3ProfileLinkChoices.SLACK] = ( - "Please enter a valid CPPLang profile URL or Member ID." - ) + slack_value = value.get(V3ProfileLinkChoices.SLACK) + if slack_value: + if not is_valid_slack_link(slack_value): + field_errors[V3ProfileLinkChoices.SLACK] = ( + "Please enter a valid CPPLang profile URL or Member ID." + ) + elif not is_url(slack_value): + value[V3ProfileLinkChoices.SLACK] = f"{SLACK_PROFILE_URL_PREFIX}{slack_value}"🤖 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/serializers.py` around lines 79 - 83, Update the Slack handling near is_valid_slack_link in the serializer to convert accepted bare Member IDs into the canonical CPPLang Slack profile URL before saving. Preserve validation behavior for invalid values and leave already-canonical URLs unchanged.templates/v3/user_profile_edit.html (1)
546-559: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove early return to prevent dropping concurrent form errors.
Returning early here skips processing potential concurrent validation errors (like an invalid
roleselection) and bypasses throwing an error. As a result, the form's overallsaveStatusis never updated to'error', masking the failure state from the main UI indicator.Instead of returning, delete the
profile_linksobject fromdataafter mapping the inline errors, and throw an aggregate error at the end to correctly update the form state.🐛 Proposed fix
// Per-link validation errors (e.g. the backend secure-URL/Slack // checks) come back keyed by link type; route them to their own // field instead of the banner next to the Save button. const linkErrors = data.profile_links; + let hasLinkErrors = false; if (linkErrors && typeof linkErrors === 'object' && !Array.isArray(linkErrors)) { Object.entries(linkErrors).forEach(([type, msg]) => { this.errors[type] = Array.isArray(msg) ? msg[0] : msg; }); - return; + hasLinkErrors = true; + delete data.profile_links; } const fieldErrors = Object.values(data).flat().filter(Boolean); - throw new Error(fieldErrors.length ? fieldErrors.join(' ') : `Save failed (${res.status})`); + if (fieldErrors.length) { + throw new Error(fieldErrors.join(' ')); + } + if (hasLinkErrors) { + throw new Error("Please correct the link errors below."); + } + throw new Error(`Save failed (${res.status})`); } this.saveStatus = 'saved';🤖 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` around lines 546 - 559, Update the profile_links validation handling in the save error flow to remove the early return after mapping per-link errors. After populating this.errors, delete profile_links from data, then continue collecting remaining fieldErrors and throw the aggregate error so saveStatus is set to 'error' for concurrent validation failures.
🧹 Nitpick comments (1)
users/models.py (1)
359-368: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
displayed_profile_rolechoices are broader than the field's contract.
choices=ProfileRoleallows the full enum, including internal C++ Alliance titles, even though this field is documented as "Library role the user has chosen to feature."internal_role(Line 380-386) explicitly restricts its choices toProfileRole.internal_roles(); this field should mirror that pattern and restrict toProfileRole.library_roles(). Currently_effective_role()happens to guard against a mis-set internal value at read time (Line 551), but the field itself accepts invalid states (e.g. viafull_clean()or a raw admin edit), which is a data-integrity gap.♻️ Proposed fix
displayed_profile_role = models.CharField( max_length=64, - choices=ProfileRole, + choices=[ + (r.value, r.label) + for r in ProfileRole + if r.value in ProfileRole.library_roles() + ], blank=True, default="",🤖 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/models.py` around lines 359 - 368, Update the displayed_profile_role field to use ProfileRole.library_roles() for its choices, matching the field’s library-role contract and the restriction used by internal_role. Leave the field’s default, blank behavior, and other configuration unchanged.
🤖 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.
Outside diff comments:
In `@templates/v3/user_profile_edit.html`:
- Around line 546-559: Update the profile_links validation handling in the save
error flow to remove the early return after mapping per-link errors. After
populating this.errors, delete profile_links from data, then continue collecting
remaining fieldErrors and throw the aggregate error so saveStatus is set to
'error' for concurrent validation failures.
In `@users/serializers.py`:
- Around line 79-83: Update the Slack handling near is_valid_slack_link in the
serializer to convert accepted bare Member IDs into the canonical CPPLang Slack
profile URL before saving. Preserve validation behavior for invalid values and
leave already-canonical URLs unchanged.
---
Nitpick comments:
In `@users/models.py`:
- Around line 359-368: Update the displayed_profile_role field to use
ProfileRole.library_roles() for its choices, matching the field’s library-role
contract and the restriction used by internal_role. Leave the field’s default,
blank behavior, and other configuration unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 496bc7c5-ce35-4018-b0ad-5b9168fb7197
📒 Files selected for processing (19)
ak/homepage.pyconfig/celery.pycore/views.pylibraries/tasks.pynews/models.pynews/views.pystatic/css/v3/user-profile-page.csstemplates/v3/includes/_field_dropdown.htmltemplates/v3/posts_list.htmltemplates/v3/user_profile_edit.htmlusers/admin.pyusers/forms.pyusers/migrations/0023_user_displayed_profile_role_and_more.pyusers/models.pyusers/profile_cards.pyusers/serializers.pyusers/tasks.pyusers/tests/test_profile_role.pyusers/views.py
💤 Files with no reviewable changes (1)
- users/profile_cards.py
🚧 Files skipped from review as they are similar to previous changes (10)
- templates/v3/posts_list.html
- static/css/v3/user-profile-page.css
- config/celery.py
- core/views.py
- news/models.py
- libraries/tasks.py
- users/forms.py
- users/admin.py
- news/views.py
- users/tasks.py
julioest
left a comment
julioest
left a comment
There was a problem hiding this comment.
Approving this one, awesome work, Julia! Dug into this locally and it holds up really well Just one thing, nothing blocking.
#2517 also adds a 0023 migration off 0022, so the two collide once they both land. I actually hit it locally, with both files present makemigrations --check bails with "Conflicting migrations detected, multiple leaf nodes ... (0023_user_country..., 0023_user_displayed...)." Your code is totally fine, it's just that whoever merges second will need to bump to 0024
|
Thanks for the heads-up @julioest !! I'll keep an eye out for migration conflicts |
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 `@users/models.py`:
- Around line 274-280: Update NO_PUBLIC_ROLE_LABEL to replace the en dash with
an ASCII hyphen, keeping the label text and meaning otherwise unchanged so it
satisfies Ruff’s RUF001 check.
🪄 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: 8bde1d29-f120-408e-a34c-582ba46d148e
📒 Files selected for processing (6)
users/forms.pyusers/migrations/0023_user_displayed_profile_role_and_more.pyusers/models.pyusers/serializers.pyusers/tests/test_profile_role.pyusers/views.py
🚧 Files skipped from review as they are similar to previous changes (4)
- users/migrations/0023_user_displayed_profile_role_and_more.py
- users/views.py
- users/serializers.py
- users/forms.py
|
@julhoang Works great, I was able to switch roles and verify |
|
Great work Julia, testing went good 👍 /users/me/Visible with role visible and opted out: PostsRole visible: Opted-out: Edit profile: no public role
I'll stay tuned to this integrates cleanly onto my public profile PR. |
f2a5177 to
4df420f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
users/models.py (1)
371-380: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRestrict
displayed_profile_rolechoices to library roles only.
internal_roleexplicitly filters itschoicestoProfileRole.internal_roles(), butdisplayed_profile_rolepasses the fullProfileRoleenum, letting an internal title (e.g."ceo") be assigned to this field at the model/admin level even though it's documented as "Library role the user has chosen to feature."_effective_role()happens to guard against this (its first branch checksin ProfileRole.library_roles()), so there's no current display bug, but the field's own contract doesn't enforce it — a future admin form or bulk update could silently store an invalid combination.🛡️ Proposed fix (restrict choices; note this needs a follow-up migration for the `choices` change)
displayed_profile_role = models.CharField( max_length=64, - choices=ProfileRole, + choices=[(r.value, r.label) for r in ProfileRole if r.value in ProfileRole.library_roles()], blank=True,🤖 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/models.py` around lines 371 - 380, Update the displayed_profile_role field to use only ProfileRole.library_roles() for its choices, matching the field’s documented contract and the filtering used by internal_role. Preserve its existing blank and default behavior, and add the required migration for the choices change.users/views.py (1)
395-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the typographic apostrophe to satisfy Ruff RUF001.
Same issue category already fixed elsewhere in this PR for
NO_PUBLIC_ROLE_LABEL.Proposed fix
ctx["bio"] = ( user.biography - or "Add a short bio to tell the community who you are, what you work on, or what you’re passionate about." + or "Add a short bio to tell the community who you are, what you work on, or what you're passionate about." )🤖 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 395 - 398, Update the fallback biography string in the ctx["bio"] assignment to replace the typographic apostrophe with a plain ASCII apostrophe, matching the existing RUF001 fixes such as NO_PUBLIC_ROLE_LABEL.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 `@users/tasks.py`:
- Around line 146-202: The recomputation snapshot in the task containing the
valid-role queries must be serialized to prevent older results overwriting newer
eligibility. Acquire the task’s shared execution lock before building valid,
held_any, and stale_rows, and keep the related updates within that serialized
flow; do not rely on the existing compare-and-swap, which only protects
displayed choices.
---
Nitpick comments:
In `@users/models.py`:
- Around line 371-380: Update the displayed_profile_role field to use only
ProfileRole.library_roles() for its choices, matching the field’s documented
contract and the filtering used by internal_role. Preserve its existing blank
and default behavior, and add the required migration for the choices change.
In `@users/views.py`:
- Around line 395-398: Update the fallback biography string in the ctx["bio"]
assignment to replace the typographic apostrophe with a plain ASCII apostrophe,
matching the existing RUF001 fixes such as NO_PUBLIC_ROLE_LABEL.
🪄 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: f097ac41-602c-4dd9-b953-480ea6ab72f6
📒 Files selected for processing (19)
ak/homepage.pyconfig/celery.pycore/views.pylibraries/tasks.pynews/models.pynews/views.pystatic/css/v3/user-profile-page.csstemplates/v3/includes/_field_dropdown.htmltemplates/v3/posts_list.htmltemplates/v3/user_profile_edit.htmlusers/admin.pyusers/forms.pyusers/migrations/0024_user_displayed_profile_role_and_more.pyusers/models.pyusers/profile_cards.pyusers/serializers.pyusers/tasks.pyusers/tests/test_profile_role.pyusers/views.py
💤 Files with no reviewable changes (1)
- users/profile_cards.py
🚧 Files skipped from review as they are similar to previous changes (10)
- templates/v3/posts_list.html
- templates/v3/includes/_field_dropdown.html
- core/views.py
- static/css/v3/user-profile-page.css
- ak/homepage.py
- users/forms.py
- users/admin.py
- news/views.py
- users/serializers.py
- users/tests/test_profile_role.py
| # (user_id, library_id) pairs per role: author / maintainer / contributor. | ||
| valid = { | ||
| ProfileRole.AUTHOR.value: set( | ||
| Library.authors.through.objects.values_list("user_id", "library_id") | ||
| ), | ||
| ProfileRole.MAINTAINER.value: set( | ||
| LibraryVersion.maintainers.through.objects.values_list( | ||
| "user_id", "libraryversion__library_id" | ||
| ).distinct() | ||
| ), | ||
| ProfileRole.CONTRIBUTOR.value: set( | ||
| Commit.objects.filter(author__user_id__isnull=False) | ||
| .values_list("author__user_id", "library_version__library_id") | ||
| .distinct() | ||
| ), | ||
| } | ||
| held_any = {role: {uid for uid, _ in pairs} for role, pairs in valid.items()} | ||
|
|
||
| # Each user's highest-precedence role. Iterating high-to-low, setdefault | ||
| # keeps the first (highest) seen. | ||
| top = {} | ||
| for role in ProfileRole.library_role_precedence(): | ||
| for uid in held_any[role]: | ||
| top.setdefault(uid, role) | ||
| by_role = defaultdict(list) | ||
| for uid, role in top.items(): | ||
| by_role[role].append(uid) | ||
|
|
||
| # Clear explicit user choices the latest import has revoked, so `.role` | ||
| # falls through instead of displaying a role the user no longer holds. | ||
| stale_rows = [] # (uid, role, lib_id) as read in this snapshot | ||
| choosers = User.objects.exclude(displayed_profile_role="").values_list( | ||
| "id", "displayed_profile_role", "displayed_profile_role_library_id" | ||
| ) | ||
| for uid, role, lib_id in choosers: | ||
| if lib_id: | ||
| ok = (uid, lib_id) in valid.get(role, set()) | ||
| else: | ||
| ok = uid in held_any.get(role, set()) | ||
| if not ok: | ||
| stale_rows.append((uid, role, lib_id)) | ||
|
|
||
| cleared_choices = 0 | ||
| with transaction.atomic(): | ||
| User.objects.exclude(id__in=top).exclude(resolved_profile_role="").update( | ||
| resolved_profile_role="" | ||
| ) | ||
| for role, ids in by_role.items(): | ||
| User.objects.filter(id__in=ids).update(resolved_profile_role=role) | ||
| # Compare-and-swap: only clear rows still matching the snapshot values, | ||
| # so a selection the user saved after the snapshot survives. | ||
| for uid, role, lib_id in stale_rows: | ||
| cleared_choices += User.objects.filter( | ||
| id=uid, | ||
| displayed_profile_role=role, | ||
| displayed_profile_role_library_id=lib_id, | ||
| ).update(displayed_profile_role="", displayed_profile_role_library=None) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize recomputation snapshots.
Line 146 reads eligibility before any execution lock. Overlapping tasks can write snapshots out of order: an older task can restore resolved_profile_role="Author" after a newer task clears it following revoked eligibility. The compare-and-swap only protects explicit selections.
Serialize this task with a shared lock and take the snapshot after acquiring it (or version the source snapshot and conditionally apply writes).
🤖 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/tasks.py` around lines 146 - 202, The recomputation snapshot in the
task containing the valid-role queries must be serialized to prevent older
results overwriting newer eligibility. Acquire the task’s shared execution lock
before building valid, held_any, and stale_rows, and keep the related updates
within that serialized flow; do not rely on the existing compare-and-swap, which
only protects displayed choices.
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
Heya! Thanks for the fixes :)
All looking good to me
4df420f to
88aa0b4
Compare
4 participants
Issue: #2443
Summary & Context
Adds a real, user-controlled displayed profile role to the User model. A user features a library role they actually hold (Author / Maintainer / Contributor, optionally scoped to a library like "Boost.Beast Author"); staff assign internal C++ Alliance titles (e.g. "Board Member") in the admin. Users can also opt out entirely with No Public Role, which hides their role on every profile surface except their own profile page. The role is resolved on read with no per-author N+1.
Changes
Profile role model & source of truth
ProfileRoleenum with two groups: library roles (Author / Maintainer / Contributor, user-selectable) and internal C++ Alliance titles (admin-only).User:displayed_profile_role+displayed_profile_role_library(FK) – the library role the user chose to feature, optionally scoped to one library.internal_role– the staff-assigned C++ Alliance title.resolved_profile_role– the auto-derived default (maintained by the recompute task, see below).hide_public_role– explicit opt-out; when set, the user's role is hidden everywhere except their own profile page.User.rolethe single source of truth, resolved by precedence: chosen role → internal title → auto-derived role, and empty when the user has opted out.User.public_rolefor the user's own profile page: it ignores the opt-out and always shows the best available role (so the profile page never goes role-less even when the user hides it elsewhere).UniqueConstraint.Role selection (edit page)
hide_public_role, clears any featured library role, and removes the role from feeds, cards, and every other surface — while the user's own profile page still shows their best available role._field_combo.html) with a native<select>no-JS fallback.<form>saved by the existing Save Changes button. The new save handler re-validates the choice against the user's own options, so they can never feature a role they don't hold; re-selecting a real role clears the opt-out.Admin: assign C++ Alliance titles
EmailUserAdminForm: labelsinternal_roleas "C++ Alliance title" and, when a singular title is already taken, blocks the save with an error naming the current holder (the DB constraint is the hard backstop).Role resolution & performance
.rolereads the storedresolved_profile_rolecolumn directly, so a feed of N posts resolves every author's role with no per-author query.recompute_displayed_profile_roles) refreshes that column after each library/commit import, plus a daily backstop.select_related(...)for the scoped-role FK on the feed, homepage, and post-detail querysets.resolved_profile_rolecolumn, so nothing latency-sensitive needs the matview.Cleanup
users/profile_cards.py(user_profile_card); news cards useEntry.author.to_v3_profile_dict(), andto_v3_post_card_dict()drops its now-unusedauthor_roleargument.Peer-Testing Guidelines
Setup
docker compose exec web python manage.py migrate, thendocker compose restart celery-worker celery-beat.docker compose exec web python manage.py shell -c "from users.tasks import recompute_displayed_profile_roles as t; t()".Find accounts that hold a role
3. Most accounts have no library contributions, so first find ones that do — run this SQL to list users with at least one eligible role:
Exercise the role dropdown
5. Open http://localhost:8000/users/me/?edit=true. If your account has no library contributions, "Your Role" is disabled with the "Contribute to a library to unlock a role" hint. To see it populated (and pick/save a role), impersonate one of the eligible users from step 3 — ⭐️ there's a method to do this locally, reach out to me for details. ⭐️
Exercise "No Public Role"
6. As one of those eligible users, pick a role and save, then reopen the dropdown and choose "No Public Role" and save. Confirm the role no longer appears in the posts feed / user cards for that account, but their own profile page (http://localhost:8000/users/me/) still shows a role (the best available one). Re-select a real role and confirm it reappears everywhere.
Admin: C++ Alliance titles
7. In the Users admin, open your account and set a C++ Alliance title — the "Derived library roles" panel shows the eligible roles read-only. Then assign a singular title (e.g. CEO) to a second user — the save is blocked, naming the current holder.
Screenshots
Example of more roles filled out
Self-review Checklist
Frontend
Summary by CodeRabbit
New Features
Bug Fixes