Story 2548: Webpage Integration -> Public Profile Routing - #2575
Story 2548: Webpage Integration -> Public Profile Routing#2575javiercoronadonarvaez wants to merge 31 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change replaces numeric profile URLs with persistent slug-based routing keys. It adds key lifecycle management, canonical redirects, profile sharing, linked user cards, and Boost profile-link preference across author displays. ChangesProfile routing and lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant URLconf
participant PublicUserProfileView
participant UserProfileRoutingKey
Browser->>URLconf: Request users/routing_key/
URLconf->>PublicUserProfileView: Pass routing_key
PublicUserProfileView->>UserProfileRoutingKey: Resolve profile key
UserProfileRoutingKey-->>PublicUserProfileView: Return canonical or superseded key
PublicUserProfileView-->>Browser: Render profile or return permanent redirect
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
javiercoronadonarvaez
changed the title
javiercoronadonarvaez
changed the title
There was a problem hiding this comment.
Hey @javiercoronadonarvaez, thanks for this!!
Slack thread feedback
From the Slack thread, we need two fixes regarding URL hydrations:
- There should be a "Share" button in the user's own profile view as well;
- Library Authors/Contributors cards should link the user to their boost profile URL on both the avatar and names, falling back to their Github url if they don't have a boost profile;
PR feedback
Regarding the backfill logic, there's something that comes to mind here: the backfill migration will run as soon as the new code gets deployed to prod, and new display name based URLs will only be generated if the user updates their display name while having the v3 flag on.
This creates a drift:
- Code is deployed → backfill happens;
- Beta users get the v3 flag → display name updates generate a new profile url (group A);
- Legacy users change their display name → no new profile url is generated (group B);
- v3 flag turned on for everybody;
Now group A correctly have their new profile url, and group B doesn't.
We can either accept that drift and assume new display name updates will generate the url, or we can extract the backfill function so we're able to run it on-demand as well. What are your thoughts?
|
@herzog0 thanks for those observations. There are two points to make here and they guided my solution implementation.
To address your observations, I've modified On extracting the backfill, it skips users who already have a key ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
users/tests/test_commands.py (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the first-element access.
Line 29 builds an entire list to read one element. Index the already-materialized
keyslist instead. Ruff reports this as RUF015.♻️ Proposed change
keys = list(user.profile_routing_keys.order_by("created")) - assert [k.routing_key for k in keys][0] == stale + assert keys[0].routing_key == stale assert keys[-1].routing_key.startswith("jane-smith-")🤖 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_commands.py` around lines 28 - 31, Update the first routing-key assertion in the test to index the already-materialized keys list directly, avoiding the redundant list comprehension while preserving the stale-key check and existing assertions.Source: Linters/SAST tools
users/models.py (2)
1077-1087: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving this receiver to
users/signals.py.The
Userdocstring states: "See ./signals.py for signals that relate to this model."users/signals.pyalready importsUserProfileRoutingKeyand callssync_for. Placing the creation hook there keeps all user signal handlers in one module.🤖 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 1077 - 1087, Move the create_profile_routing_key_for_user post_save receiver from users/models.py into users/signals.py, alongside the existing User signal handlers and using its existing UserProfileRoutingKey import. Preserve the receiver’s sender, raw/created guards, and mint_for behavior unchanged.
927-942: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
mint_forretries everyIntegrityError, not only key collisions.The
except IntegrityErrorblock treats any integrity failure as a collision. A foreign-key violation (for example, the user row was deleted concurrently) is retried five times and then reported asRuntimeError("Could not mint a unique profile routing key ..."). That message hides the real cause.Inspect the exception and re-raise when it is not a
routing_keyuniqueness conflict.🤖 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 927 - 942, The mint_for method should retry only IntegrityError exceptions caused by a routing_key uniqueness collision. Inspect the caught exception’s constraint or database error details, continue retrying for that specific conflict, and immediately re-raise other integrity failures such as foreign-key violations instead of converting them to RuntimeError.static/js/v3/share-profile.js (2)
47-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider announcing the copy result to screen readers.
setStatechanges the visible text and thearia-label. Assistive technology does not reliably announce anaria-labelchange on an element that already has focus. A visually hiddenaria-live="polite"region that receives "Copied" or "Copy failed" makes the result perceivable.🤖 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 `@static/js/v3/share-profile.js` around lines 47 - 64, Update setState to announce copy outcomes through a visually hidden aria-live="polite" region, writing the corresponding "Copied" or "Copy failed" message when state changes. Reuse or create a dedicated live-region element rather than relying on aria-label changes, while preserving the existing visible-label and reset behavior.
20-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHarden the
execCommandfallback. Production redirects HTTP to HTTPS, but local development useshttp://localhost:8000. Addreadonly, focus the textarea, and callsetSelectionRange(0, text.length)afterselect()to avoid iOS Safari keyboard and selection failures. Retain the fallback becausewriteTextcan also reject in a secure context.🤖 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 `@static/js/v3/share-profile.js` around lines 20 - 35, Harden fallbackCopy by making the temporary textarea readonly, focusing it before selection, and calling setSelectionRange(0, text.length) after select(). Preserve the existing execCommand fallback and cleanup behavior, including use when writeText rejects.users/migrations/0027_backfill_profile_routing_keys.py (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider inlining the key generator in this migration.
This migration imports
generate_routing_keyfromusers/utils.py. Historical migrations then depend on live application code. If the signature or the fallback behavior ofgenerate_routing_keychanges later, replaying migrations on a fresh database can fail or produce keys that differ from what the deployment produced. The file already pinsKEY_MAX_LENGTHlocally for exactly this reason.A local copy of the generator keeps the migration frozen in time.
🤖 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/migrations/0027_backfill_profile_routing_keys.py` around lines 3 - 7, Replace the live users.utils.generate_routing_key import in the migration with a local, frozen implementation of the generator, preserving the behavior and KEY_MAX_LENGTH value used when this migration was created. Update the migration’s call sites to use the local generator so replaying it does not depend on current application code.users/management/commands/sync_profile_routing_keys.py (1)
29-32: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the prefetched routing keys in the detection loop.
keys.current_for(user)performs one filtered, ordered query per active user. Add.prefetch_related("profile_routing_keys")and useuser.profile_routing_key. Useusers.iterator(chunk_size=1000)becauseprefetch_relatedrequires an explicit chunk size withiterator().🤖 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/management/commands/sync_profile_routing_keys.py` around lines 29 - 32, Update the queryset used before the detection loop to prefetch profile_routing_keys, iterate with users.iterator(chunk_size=1000), and replace keys.current_for(user) with the prefetched user.profile_routing_key while preserving the existing matches_display_name and stale collection logic.
🤖 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/utils.py`:
- Around line 505-516: The user enrichment in libraries/utils.py lines 505-516
must only replace an author’s profile_url when exactly one active, claimed User
matches the normalized GitHub username; avoid queryset iteration overwriting
ambiguous matches. Add coverage in libraries/tests/test_utils.py lines 430-488
with two active claimed users sharing a username, asserting the contributor
remains linked to GitHub.
- Line 594: Update the querysets feeding the author dictionaries in the
surrounding context builder to prefetch profile_routing_keys for authors and
maintainers, and make the top-contributors queryset use select_related("user")
plus prefetch_related("user__profile_routing_keys"). Add a query-count test
covering this context-building path to verify contributor rendering does not
issue per-row profile URL queries.
In `@news/services.py`:
- Around line 46-51: The news author card should use the User.profile_url
property so active unclaimed users with a known GitHub account link to GitHub;
update the profile_url assignment in news/services.py lines 46-51 accordingly.
Add an active unclaimed user with github_username and assert the GitHub URL in
news/tests/test_services.py lines 11-26.
In `@static/js/v3/share-profile.js`:
- Around line 74-79: Update flash in the share-profile feedback flow to store
the reset timeout ID on link and clear any existing timeout before setting the
new state. Assign the newly created setTimeout result back to that link property
so repeated clicks keep the “Copied” state visible for the full COPY_FEEDBACK_MS
interval.
In `@users/models.py`:
- Around line 602-610: Remove the database-writing call to
UserProfileRoutingKey.objects.mint_for from User.get_absolute_url. When
profile_routing_key is missing, use a non-persisting fallback that still
produces the expected profile-user URL, or ensure keys are minted earlier in an
explicit writable repair path; keep get_absolute_url itself read-only.
In `@users/tests/test_v3_profile_public.py`:
- Around line 232-238: Update
test_profile_user_route_does_not_shadow_the_literal_users_routes to verify
dispatch for the literal avatar path, not just URL generation: resolve or
request "/users/avatar/" and assert it maps to the "user-avatar" route while
preserving the existing profile-route assertions.
---
Nitpick comments:
In `@static/js/v3/share-profile.js`:
- Around line 47-64: Update setState to announce copy outcomes through a
visually hidden aria-live="polite" region, writing the corresponding "Copied" or
"Copy failed" message when state changes. Reuse or create a dedicated
live-region element rather than relying on aria-label changes, while preserving
the existing visible-label and reset behavior.
- Around line 20-35: Harden fallbackCopy by making the temporary textarea
readonly, focusing it before selection, and calling setSelectionRange(0,
text.length) after select(). Preserve the existing execCommand fallback and
cleanup behavior, including use when writeText rejects.
In `@users/management/commands/sync_profile_routing_keys.py`:
- Around line 29-32: Update the queryset used before the detection loop to
prefetch profile_routing_keys, iterate with users.iterator(chunk_size=1000), and
replace keys.current_for(user) with the prefetched user.profile_routing_key
while preserving the existing matches_display_name and stale collection logic.
In `@users/migrations/0027_backfill_profile_routing_keys.py`:
- Around line 3-7: Replace the live users.utils.generate_routing_key import in
the migration with a local, frozen implementation of the generator, preserving
the behavior and KEY_MAX_LENGTH value used when this migration was created.
Update the migration’s call sites to use the local generator so replaying it
does not depend on current application code.
In `@users/models.py`:
- Around line 1077-1087: Move the create_profile_routing_key_for_user post_save
receiver from users/models.py into users/signals.py, alongside the existing User
signal handlers and using its existing UserProfileRoutingKey import. Preserve
the receiver’s sender, raw/created guards, and mint_for behavior unchanged.
- Around line 927-942: The mint_for method should retry only IntegrityError
exceptions caused by a routing_key uniqueness collision. Inspect the caught
exception’s constraint or database error details, continue retrying for that
specific conflict, and immediately re-raise other integrity failures such as
foreign-key violations instead of converting them to RuntimeError.
In `@users/tests/test_commands.py`:
- Around line 28-31: Update the first routing-key assertion in the test to index
the already-materialized keys list directly, avoiding the redundant list
comprehension while preserving the stale-key check and existing assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 958d2632-0373-42e3-8128-3e17462029c7
📒 Files selected for processing (30)
config/urls.pycore/tests/test_user_card_component.pylibraries/mixins.pylibraries/models.pylibraries/tests/test_models.pylibraries/tests/test_utils.pylibraries/utils.pylibraries/views.pynews/services.pynews/tests/test_services.pynews/tests/test_views.pynews/views.pystatic/css/v3/user-card.cssstatic/js/v3/share-profile.jstemplates/v3/includes/_user_card.htmltemplates/v3/posts_list.htmltemplates/v3/user_profile_page.htmlusers/management/commands/sync_profile_routing_keys.pyusers/migrations/0026_userprofileroutingkey.pyusers/migrations/0027_backfill_profile_routing_keys.pyusers/mixins.pyusers/models.pyusers/signals.pyusers/tests/test_commands.pyusers/tests/test_profile_routing_key.pyusers/tests/test_v3_profile_edit.pyusers/tests/test_v3_profile_public.pyusers/utils.pyusers/views.pyversions/views.py
| ) | ||
|
|
||
| apply_collective_author_overrides(author_dicts) | ||
| apply_collective_author_overrides(prefer_boost_profile_links(author_dicts)) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Prefetch profile URL data before building author_dicts.
User.to_v3_profile_dict() and CommitAuthor.to_v3_profile_dict() resolve profile URLs before this call. The author, maintainer, and top-contributor querysets above do not preload routing keys, and the contributor queryset does not join user. This path can issue queries per rendered contributor.
Prefetch profile_routing_keys for authors and maintainers. Use select_related("user") and prefetch_related("user__profile_routing_keys") for top contributors. Add a query-count test for this context builder.
🤖 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/utils.py` at line 594, Update the querysets feeding the author
dictionaries in the surrounding context builder to prefetch profile_routing_keys
for authors and maintainers, and make the top-contributors queryset use
select_related("user") plus prefetch_related("user__profile_routing_keys"). Add
a query-count test covering this context-building path to verify contributor
rendering does not issue per-row profile URL queries.
There was a problem hiding this comment.
@javiercoronadonarvaez this is a good one to tackle
There was a problem hiding this comment.
Makes sense. Addressed now.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| # A deactivated author's profile 404s, so it is left unlinked. | ||
| "profile_url": ( | ||
| author.get_absolute_url() | ||
| if getattr(author, "is_active", False) | ||
| else None | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use User.profile_url for news author cards.
The active-account check sends an unclaimed user to /users/<routing-key>/. User.profile_url deliberately sends an unclaimed user to GitHub when known, because that public profile is a stub.
news/services.py#L46-L51: setprofile_urlfromauthor.profile_urlinstead of callingauthor.get_absolute_url().news/tests/test_services.py#L11-L26: add an active unclaimed user withgithub_usernameand assert that the card links to the GitHub profile.
📍 Affects 2 files
news/services.py#L46-L51(this comment)news/tests/test_services.py#L11-L26
🤖 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 `@news/services.py` around lines 46 - 51, The news author card should use the
User.profile_url property so active unclaimed users with a known GitHub account
link to GitHub; update the profile_url assignment in news/services.py lines
46-51 accordingly. Add an active unclaimed user with github_username and assert
the GitHub URL in news/tests/test_services.py lines 11-26.
There was a problem hiding this comment.
@javiercoronadonarvaez also a good one to join the batch
There was a problem hiding this comment.
Addressed now.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def get_absolute_url(self): | ||
| """This user's public profile page.""" | ||
| return reverse("profile-user", kwargs={"pk": self.pk}) | ||
| key = self.profile_routing_key | ||
| if key is None: | ||
| # Creation mints one and a migration backfilled the rest, so this | ||
| # only catches rows inserted around both (loaddata, bulk_create). | ||
| key = UserProfileRoutingKey.objects.mint_for(self) | ||
| self.profile_routing_key = key | ||
| return reverse("profile-user", kwargs={"routing_key": key.routing_key}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find get_absolute_url call sites for users, plus sitemap/feed generators that
# may run on read paths, and check for ATOMIC_REQUESTS / read-replica routing.
rg -nP --type=py -C3 '\bget_absolute_url\s*\(' -g '!**/migrations/**' -g '!**/tests/**'
rg -nP --type=py -C3 'Sitemap|Feed\b|DATABASE_ROUTERS|ATOMIC_REQUESTS' config/ users/ | head -80
rg -nP --type=html -C2 'get_absolute_url' templates/ | head -40Repository: boostorg/website-v2
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- tracked files ---'
git ls-files | sed -n '1,160p'
printf '%s\n' '--- target definition ---'
rg -n -C8 'def get_absolute_url|profile_routing_key|class UserProfileRoutingKey' users/models.py
printf '%s\n' '--- Python call sites ---'
rg -n -P -C3 '\bget_absolute_url\s*\(' --glob '*.py' --glob '!**/migrations/**' --glob '!**/tests/**' . || true
printf '%s\n' '--- template call sites ---'
rg -n -P -C3 'get_absolute_url' --glob '*.html' . || true
printf '%s\n' '--- routing and read-path configuration ---'
rg -n -P -C3 'Sitemap|Feed\b|DATABASE_ROUTERS|ATOMIC_REQUESTS|read.?only|replica|using\(' --glob '*.py' --glob '*.yml' --glob '*.yaml' --glob '*.ini' --glob '*.conf' . || trueRepository: boostorg/website-v2
Length of output: 45718
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- key manager and creation signal ---'
sed -n '892,945p' users/models.py
sed -n '1060,1100p' users/models.py
printf '%s\n' '--- routing-key migrations and repair commands ---'
rg -n -C5 'UserProfileRoutingKey|profile_routing_key|mint_for|backfill|repair' users --glob '*.py' --glob '!**/tests/**'
rg -n -C4 'UserProfileRoutingKey|profile_routing_key|mint_for|backfill|repair' core --glob '*.py' --glob '!**/tests/**'
printf '%s\n' '--- user profile URL consumers ---'
sed -n '1,150p' users/mixins.py
sed -n '1,155p' users/views.py
rg -n -P -C5 '\b(get_absolute_url|profile_url|profile-user)\b' users templates --glob '*.py' --glob '*.html' --glob '!**/tests/**'
printf '%s\n' '--- feed and sitemap implementations ---'
sed -n '1,120p' news/feeds.py
sed -n '1,100p' versions/feeds.py
rg -n -C5 'Sitemap|sitemaps|profile-user|UserProfile' . --glob '*.py' --glob '!**/tests/**' --glob '!**/migrations/**'
printf '%s\n' '--- database configuration ---'
rg -n -C6 'DATABASES|DATABASE_ROUTERS|ATOMIC_REQUESTS|CONN_MAX_AGE|OPTIONS' config docker-compose.yml kube --glob '*.py' --glob '*.yml' --glob '*.yaml' || true
printf '%s\n' '--- AST call-site summary ---'
python3 - <<'PY'
import ast
from pathlib import Path
for path in Path(".").rglob("*.py"):
if any(part in {"tests", "migrations"} for part in path.parts):
continue
try:
tree = ast.parse(path.read_text())
except (OSError, SyntaxError):
continue
for node in ast.walk(tree):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute):
if node.func.attr == "get_absolute_url":
print(f"{path}:{node.lineno}: {ast.unparse(node)}")
PYRepository: boostorg/website-v2
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -o pipefail
python3 - <<'PY'
import ast
from pathlib import Path
def owner(tree, node):
stack = []
result = []
for parent in ast.walk(tree):
for child in ast.iter_child_nodes(parent):
if child is node:
break
else:
continue
break
# Re-walk with an explicit ancestry stack.
def walk(n, ancestors):
if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)):
ancestors = ancestors + [n]
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute):
text = ast.unparse(n)
if n.func.attr in {"get_absolute_url", "mint_for"}:
print(
f"{path}:{n.lineno}: "
f"{' > '.join(a.name for a in ancestors)}: {text}"
)
for child in ast.iter_child_nodes(n):
walk(child, ancestors)
walk(tree, [])
for path in sorted(Path(".").rglob("*.py")):
if any(part in {"tests", "migrations"} for part in path.parts):
continue
try:
tree = ast.parse(path.read_text())
except (OSError, SyntaxError):
continue
# Keep the path available to the walker without executing repository code.
globals()["path"] = path
owner(tree, None)
PY
printf '%s\n' '--- exact sitemap/config references ---'
rg -n -i -C2 'sitemap|database_routers|atomic_requests|read.?only|read.?replica|replica' \
config users news versions kube docker-compose.yml --glob '*.py' --glob '*.yml' --glob '*.yaml' || true
printf '%s\n' '--- profile template consumers ---'
rg -n -P -C2 'profile_url|get_absolute_url|Share|user_profile_page|_user_profile' \
templates users --glob '*.html' --glob '*.py' --glob '!**/tests/**' | head -160Repository: boostorg/website-v2
Length of output: 18611
Keep get_absolute_url free of database writes.
UserProfileRoutingKey.objects.mint_for(self) performs an INSERT. The V3 profile context calls user.get_absolute_url() while rendering the Share link, and User.profile_url reaches it for claimed authors in read-only post views. A user created with bulk_create, fixture loading, or another path that bypasses the creation signal can therefore make a GET request fail on a read-only connection.
Use a non-minting fallback in get_absolute_url, or mint missing keys in an explicit writable repair path before serving these views.
🤖 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 602 - 610, Remove the database-writing call to
UserProfileRoutingKey.objects.mint_for from User.get_absolute_url. When
profile_routing_key is missing, use a non-persisting fallback that still
produces the expected profile-user URL, or ensure keys are minted earlier in an
explicit writable repair path; keep get_absolute_url itself read-only.
There was a problem hiding this comment.
@javiercoronadonarvaez this is an interesting one. Sometimes DevOps creates a read-only db replica which is the default db connection for GET requests. This is not the case for us though, as we use the same connection for both operations.
Don't bother with it though, I doubt this will change any time soon, and if it ever changes, it'll definitely go under a very well thought plan.
| @waffle.testutils.override_flag("v3", active=True) | ||
| def test_profile_user_route_does_not_shadow_the_literal_users_routes(user, tp): | ||
| """The slug converter matches "me" and "avatar", so those must resolve to | ||
| their own views rather than to a profile lookup.""" | ||
| with tp.login(user): | ||
| tp.response_200(tp.get("profile-account")) | ||
| assert tp.reverse("user-avatar") == "/users/avatar/" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the avatar route dispatch.
Line 238 only verifies URL generation. It does not verify that the slug route cannot capture /users/avatar/. Resolve /users/avatar/ and assert user-avatar, or issue a request to that route.
Proposed test update
+from django.urls import resolve
+
def test_profile_user_route_does_not_shadow_the_literal_users_routes(user, tp):
...
- assert tp.reverse("user-avatar") == "/users/avatar/"
+ assert resolve("/users/avatar/").view_name == "user-avatar"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @waffle.testutils.override_flag("v3", active=True) | |
| def test_profile_user_route_does_not_shadow_the_literal_users_routes(user, tp): | |
| """The slug converter matches "me" and "avatar", so those must resolve to | |
| their own views rather than to a profile lookup.""" | |
| with tp.login(user): | |
| tp.response_200(tp.get("profile-account")) | |
| assert tp.reverse("user-avatar") == "/users/avatar/" | |
| from django.urls import resolve | |
| @waffle.testutils.override_flag("v3", active=True) | |
| def test_profile_user_route_does_not_shadow_the_literal_users_routes(user, tp): | |
| """The slug converter matches "me" and "avatar", so those must resolve to | |
| their own views rather than to a profile lookup.""" | |
| with tp.login(user): | |
| tp.response_200(tp.get("profile-account")) | |
| assert resolve("/users/avatar/").view_name == "user-avatar" |
🤖 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_public.py` around lines 232 - 238, Update
test_profile_user_route_does_not_shadow_the_literal_users_routes to verify
dispatch for the literal avatar path, not just URL generation: resolve or
request "/users/avatar/" and assert it maps to the "user-avatar" route while
preserving the existing profile-route assertions.
There was a problem hiding this comment.
Tbh, I have no idea what this is all about
There was a problem hiding this comment.
@herzog0, the concern is only about the test.
/users/avatar/ can match two URL patterns:
- The literal avatar route:
/users/avatar/ - The new profile route:
/users/<slug:routing_key>/
The test currently verifies only URL generation:
tp.reverse("user-avatar") == "/users/avatar/"That does not verify which view handles an incoming request to /users/avatar/.
The test should resolve or request /users/avatar/ and verify that Django selects user-avatar, not the profile view. This protects against a future URL-order change that lets the profile slug route capture avatar.
🐇
You are interacting with an AI system.
There was a problem hiding this comment.
That makes sense and I've applied the proper fix.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Heya @javiercoronadonarvaez, this is awesome work!! The happy paths work great from my testing. I ran into a few edge cases and had some suggestions we might want to consider:
1. Adding more author profile URL prefetches
Three v3 post-card surfaces still resolve routing keys with one query per card. I'm wondering if we could add .prefetch_related("author__profile_routing_keys") to those querysets:
popular_entriesinak/homepage.py:110recent_entriesincore/views.py:311- in
news/views.py:261, updatingAUTHOR_PREFETCHto("author__maintainers", "author__profile_routing_keys")
2. Clearing the username from the routing key when an account is deleted
I created a user account with the display name "Julia Test Delete" and then triggered the deletion flow. Checking the routing_key column afterward, the name is still readable there.
Thinking about it, I wonder if we should just delete the routing keys for deleted users outright. Is there a strong reason to keep those records around if they're no longer accessible?
3. Profile links for unclaimed accounts
Right now, user cards for unclaimed accounts don't link to the Boost profile and fall back to GitHub instead. I think that's a great decision! That said, it looks like we're still generating routing keys for those accounts automatically. Should we consider skipping generation until the account is claimed?
4. Refreshing routing_key when an admin updates a user's name
This is an edge case that I don't expect to come up often, but if an admin updates a name manually from the admin panel, the user's link will go stale. Could we trigger a routing key refresh in the admin's save() method?
| queryset = LibraryVersion.objects.prefetch_related( | ||
| "authors", "library", "library__categories" | ||
| # author_details links the author's profile, which reads their routing | ||
| # keys. Prefetched so a page of libraries stays at one query for them. | ||
| "authors__profile_routing_keys", | ||
| "library", | ||
| "library__categories", | ||
| ).defer("data") |
There was a problem hiding this comment.
I think currently this queryset is not applying the author prefetch properly, which leads to cache-miss and a lot of redundant query per card rendered.
Suggestion for change:
queryset = LibraryVersion.objects.prefetch_related(
Prefetch(
"authors",
queryset=User.objects.order_by("pk").prefetch_related(
"profile_routing_keys"
),
),
"library",
"library__categories",
).defer("data")A bonus script to test before / after
docker compose exec -T web python manage.py shell <<'PY'
from django.db import connection
from django.db.models import Prefetch
from django.test.utils import CaptureQueriesContext
from libraries.models import LibraryVersion
from users.models import User
LIMIT = 50 # set to None for a full page (~190 libraries per version)
def count(prefetch, label):
qs = LibraryVersion.objects.prefetch_related(*prefetch).defer("data")
rows = list(qs[:LIMIT] if LIMIT else qs)
with CaptureQueriesContext(connection) as c:
details = [lv.author_details for lv in rows]
sqls = [q["sql"].lower() for q in c.captured_queries]
print(f"{label}: {len(rows)} cards -> {len(sqls)} queries "
f"({len([s for s in sqls if 'userprofileroutingkey' in s])} routing-key, "
f"{len([s for s in sqls if 'users_user' in s and 'routingkey' not in s])} user), "
f"{len([d for d in details if d['profile_url']])} linked")
count(["authors__profile_routing_keys", "library", "library__categories"], "current ")
count([Prefetch("authors", queryset=User.objects.order_by("pk").prefetch_related("profile_routing_keys")),
"library", "library__categories"], "with fix ")
PYThere was a problem hiding this comment.
Hey @julhoang! For the most part, I accepted your suggestions. Below are the answers to each one:
- Adding more author profile URL prefetches
Agree. Updated code.
- Clearing the username from the routing key when an account is deleted
Good catch. Addressed.
- Profile links for unclaimed accounts
I'd hold back here. We currently have 192 unclaimed accounts in the database. 189 of these have contrinution data. For example, Peter Dimov's account is unclaimed, but his profile lists 14 authored and 16 maintained libraries.
The backfill included in this PR already keyed all 192. To make it consistent with skipping generation, we would need to delete all of these, which would 404 189 real pages.
If you have something I may be missing, though, please let me know.
- Refreshing routing_key when an admin updates a user's name
Implemented related changes.
- CODE SNIPPET SUGGESTION
Noted and implemented.
There was a problem hiding this comment.
Thanks for all the updates @javiercoronadonarvaez 🙌
On the point about deleting the backfilled keys and 404-ing 189 real pages – I'm curious where the 404s would come from 🤔. My understanding is that we wouldn't generate those URLs, and the user cards wouldn't link to a profile for unclaimed accounts, so users wouldn't have a way to reach them. Please let me know if I'm missing a case!
There was a problem hiding this comment.
You're right about the 404s. My mistake. Nothing here is deployed yet, so if we skipped unclaimed accounts we'd exclude them from the backfill too and no URL would ever exist to break.
What's left of my concern is addressability, rather than discovery. Nobody would stumble onto those pages, that's true, but with no key there's no URL to link deliberately either: a support answer, a mailing-list reply, or a future "library authors" surface.
And it's four coordinated changes rather than one: skip minting on creation, exclude unclaimed from the backfill, mint when an account is claimed (or claiming leaves the user with no URL), and guard get_absolute_url()'s self-heal. Django admin renders a "View on site" link for any model with get_absolute_url, so staff clicking it on an unclaimed account would silently mint the very key we skipped.
I'd leave generation as-is here and file a follow-up. Nothing links to a stub profile, so we can drop those rows before v3 launches with zero user impact.
What do you think?
There was a problem hiding this comment.
The plan sounds great to me @javiercoronadonarvaez . Can you please ensure this part is captured in a follow-up ticket as well?
111 whose names are CJK, Cyrillic, or Koreanand slugify to empty. Every one of those is a non-English-speaking user. slugify(allow_unicode=True) with SlugField(allow_unicode=True) would give them readable keys, but it changes the column and the generator. Worth deciding as a follow-up.
There was a problem hiding this comment.
@henryajisegiri can you help us with this please? I refer in particular to the follow up ticket.
e8c848b to
a21e93f
Compare
There was a problem hiding this comment.
Hi @javiercoronadonarvaez , there's a couple of small requests:
build_all_contributorsis the only queryset left that is missing the user prefetch, would you mind adding that?- For the Libraries view, I think the GitHub profile URL fallback is not provided - should we add this so that it's always consistent across all "user profile" surfaces?
Also, I've never noticed this before, but once we start providing the GitHub fallback (via patch_commit_authors), all the avatars are populate too! 😲
| Without GitHub | With GitHub |
|---|---|
 |
 |
|
This is what the updated standard account creation (email and password required) process: StdSignupKeyCreation.movIt follows up from this point:
Fortunately, I was able to handle it with just one commit, so I cherry picked it from my new branch. How to test (as updated in PR description)
|
dbc39c1 to
6f132ff
Compare
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
Thanks for the latest fixes @javiercoronadonarvaez !! All good from my end :)
…e a Boost account
- Each row links a profile, which reads the user's routing keys. These are then prefetched, rather than fetched per card.
- Address GitHub Actions failure in tests
06ed734 to
c25a590
Compare
3 participants
v# Issue: #2548
Summary & Context
Gives every user a stable, unique public profile URL (
/users/jane-doe-k3f9/) that survives a rename, replacing/users/<pk>/, and links a user's name and avatar through to it from the v3 surfaces that render them.This PR stacks on
cy/2492-user-profile-public-view(#2536), which introduced the public profile view itself. It will retarget down the stack automatically as those PRs merge.localhost:8000/users/<routing-key>/. Please refer toHow to testto get the routing key. Linked names/avatars are visible onlocalhost:8000/news/andlocalhost:8000/libraries/latest/grid/.IMPORTANT: Regarding GitHub redirect fallback when the user's got no Boost profile, the following is the implemented resolution order per row:
Unclaimed accounts DO NOT link to a profile.
Changes
users_userprofileroutingkeytable (migration0026):routing_key(unique) →user, withcreated/modified. Append-only Table: rows are never updated or deleted, which is what lets a URL shared before a rename keep resolving.users/utils.py):slugify(display_name)truncated to fit, plus a 4-char random suffix (get_random_string). Names that slugify to nothing like non-Latin scripts, punctuation-only, or no name at all fall back touser-<8 chars>.mint_for()retries on the unique constraint inside a savepoint, so a collision can't fail aprofile save.
display_namehas no unique constraint and is written unvalidated by social login (users/signals.py), the legacy profile form, and admin, so it is neither unique nor stable enough to route on. The suffix also keeps a key from colliding with a reserved segment such asusers/me/.post_save, so a bareuser.save()never moves someone's public URL:post_savereceiver, alongside the existingLastSeen/Preferencesone.import_social_profile_data, the one point where that path sets a name._save_v3_details_section(v3 edit page) andupdate_profile(legacy profile page, added in review — see below)Rename and social use
sync_for(), which mints only when the base changes, since those handlers also fire for country, toggles, and second-provider links.0027): for existing users, batched, with in-memory collision checks.config/urls.py,users/views.py) :users/<slug:routing_key>/replacesusers/<int:pk>/. Superseded keys 301 to the canonical one. Unknown keys and deactivated owners 404.User.get_absolute_url()points at the newest key.User.profile_routing_keyorders in Python rather than withorder_by()soprefetch_relatedcovers pages linking many profiles.users/mixins.py,static/js/v3/share-profile.js): carries the real profile URL instead of"#"and copies it on click. It stays an<a href>, so with JS off it still navigates to the profile rather than doing nothing.Linking is opt-in via(that flag existed to keep contributor lists unlinked and was removed once they were linked — see Changes addressing review comments). Never applies to deactivated accounts (their profiles 404). Author routing keys are prefetched on every list queryset, so each page costs one query for them instead of one per card.to_v3_profile_dict(link_profile=True)_user_card.htmlgained an optionalprofile_url. The country flag stays outside the link.Around 50 new tests:71 new tests after the review changes:users/tests/test_profile_routing_key.py(32), plus additions to the public profile (6), profile edit (2), library models (3), a newnews/tests/test_services.py(3), andcore/tests/test_user_card_component.py(4). Full suite: 1198 passed, 43 skipped.test_profile_routing_key.py(34),users/tests/test_commands.py(8),test_v3_profile_public.py(7),libraries/tests/test_models.py(7),libraries/tests/test_utils.py(6),core/tests/test_user_card_component.py(4),news/tests/test_services.py(3),test_v3_profile_edit.py(2). Full suite: 1222 passed, 43 skipped.Sharebutton has been included in one's own profile, based on latest conversation./users/<pk>/is gone: this closes the user-enumeration vector that motivated keys, but any/users/1/-style link already shared stops working. v3 is flag-gated and unlaunched, so this should be safe.user-<8 chars>and these follow this breakdown:紺屋淳之介,Доминика Корнилова,서하영). Every one of those is a non-English-speaking user.slugify(allow_unicode=True)withSlugField(allow_unicode=True)would give them readable keys, but it changes the column and the generator. Worth deciding as a follow-up.import_social_profile_dataoverwritesdisplay_namefrom the provider, so an existing "J. Doe" who links a GitHub account named "Jane Doe" is renamed and gets a new canonical URL. The old one 301s so nothing breaks, but it happens without the user asking. Pre-existing behaviour, now with a URL consequence.homepage.html,news/list.html,libraries/detail.html): The profile route is v3-only and 404s with the flag off, so linking from v2 would produce dead links.news/tests/test_views.pygoes from 10 to 11. The prefetch adds one query per page and removes one per card, so the ceiling had to move up by one.developalready has0025_merge_20260729_1746,0025_user_deletion_extended_scrub, and0026_merge_20260805_1706. These two (0026,0027)will need renumbering to
0027/0028with dependencies updated once the stack merges and thisrebases onto develop.
Smaller notes for review:
/users/JANE-DOE-K3F9/404s. Keys are always generated lowercase and Share copies the exact URL.get_absolute_url()self-heals: if a user somehow has no key it mints one rather than raising. After the backfill, the only ways in areloaddata(which skips minting viaraw=True) andbulk_create(nothing bulk-creates users today). The alternative was a 500 on any page listing that user.Screenshots
Overall Functionality
OverallFunctionality.mov
Edit Username (
display_namein code) and redirect from past routing keyRedirectFromOldRoutingKey.mov
Redirect from one's own profile card in Posts section
RedirectFromOnesProfileCard.mov
Share Button
ShareButton.mov
Non Existing Routing Key, but existing User
NonExistingRoutingKey.mov
Deactivated Account
DeactivatedAccount.mov
Peer Testing
display_namein code). You should be redirected to the user's profile.display_namein code) and redirect from past routing key under Screenshots.Self-review Checklist
Frontend
Mostly backend; the UI surface is two link states on existing components plus the Share button.
:focus-visiblestyling added for thenew avatar link; the Share button updates its own
aria-labelwhen it copieslinked names and avatars are server-rendered
Changes addressing review comments
1. Share button on your own profile
Previously the owner saw Edit Profile instead of Share. Both now render, with Share rightmost and Edit Profile immediately to its left. The owner's Share copies the public profile URL, not
?edit=true, so what lands on the clipboard is what a visitor would open. No template or CSS change was needed: the button group is already a flex row with a gap, so the extra button inherits its spacing and right alignment.2. Contributors resolve by identity
Contributors are commit authors, not necessarily Boost members, so one rule can't cover them. Resolution order per row is the one in the IMPORTANT note above: claimed account → Boost profile; otherwise GitHub, from the account's own
github_usernameor from theCommitAuthorthatpatch_commit_authors()attaches; otherwise unlinked.Three things worth a reviewer's attention:
@example.comemails nobody can log into and a profile page that is an empty shell. Their GitHub page is the more useful destination.CommitAuthor.useris only populated by an exact email match (update_commit_authors_users), so a contributor who commits under a different address than they signed up with kept a GitHub link despite having a profile — this was the reported bug for Matt Borland and Alan de Freitas.prefer_boost_profile_links()resolves those by GitHub username, in one query per page.to_v3_profile_dict()'slink_profileflag is gone. It existed to keep contributor lists unlinked, which is exactly what this reverses, so every caller now wants the link.Measured on
/releases/1.82.0/: 86 contributor rows → 8 Boost profiles, 76 GitHub, 2 unlinked (those have neither an account nor a GitHub URL).One row can still resolve differently in two sections of the same page: a stub whose
CommitAuthorshares no email and no GitHub username with the account is only connectable by name, and matching on names risks merging distinct people. That's a one-row data fix (setCommitAuthor.user, or add the commit email to the account), not a rendering one.3. Routing-key drift on rename
Answering the backfill/flag-rollout comment directly:
RunPythonmigration, so it runs once at deploy over every row. After deployment every user has a key, regardless of flag state.sync_for()was wired only into the v3 edit handler, so renaming through the legacy profile form kept a URL built from the previous name. Fixed by calling it inupdate_profiletoo, so both forms behave identically at any flag state and the A/B sequencing can't arise.exclude(pk__in=already_keyed)) — precisely group B, who have one that's merely stale. Repairing them needs a sync, not a backfill.users/management/commands/sync_profile_routing_keys.py. It mints only where a key no longer matches its user's display name;--dry-runpreviews without writing; safe to re-run; skips deactivated users (their profile 404s); ignores cosmetic renames (" Jane Doe "slugifies to the same base). This covers the paths that bypass the views entirely: Django admin, and any future non-form write todisplay_name.current_for()andmatches_display_name()were split out ofsync_for()so--dry-runcan ask "would this change?" without duplicating the comparison.It is a manual ops tool, not scheduled. After the two fixes the only remaining trigger is a staff member renaming someone in Django admin — human and infrequent.
config/celery.pyhas precedent for nightly maintenance sweeps if the team would rather have automatic coverage, though it would want a bulk rewrite first (it currently costs one query per user).What it cannot do: rewrite history. A user who renamed during a partial-rollout window keeps their stale key; the command appends a current one and the stale URL 301s to it. Deleting keys would break links already shared, and a reused key could deliver a visitor to the wrong person's profile.
4. ruff
F402for _ in range(...)inmint_for()shadowedgettext_lazy as _, imported at the top ofusers/models.py. Renamed to_attempt. Worth noting the shadow was live rather than theoretical: any_("...")call inside that loop would have raisedTypeError.Additional fix: the signup form dropped the username
Not part of #2548 originally — agreed on the call to fold it in here, since the fix came to four files and a handful of lines rather than warranting its own PR.
That reaches into this ticket: the routing key is derived from
display_name, so new signups were minted auser-xxxxxxxxplaceholder URL instead of a readable one. Measured on two throwaway accounts:['user-nj7rliep', 'caramelo-ypck']— placeholder at signup, name-based key only after editing the profile 25 min later['javiercorps-88ke']— one key, correct from signupWhat changed (
d73e26b9):users/forms.py— the field is nowdisplay_namewithlabel="Username", so the page reads the same. Deliberately not calledusername: allauth manages that name, deletes it here, andBaseSignupForm.clean_usernamewould pull in validation for a model field that does not exist.templates/v3/accounts/signup.html—name="display_name",form.display_name.errors.0.users/account_adapters.py—save_userwrites it onto the user, calling super withcommit=Falseso the assignment lands before the first save. That ordering is what makes the routing key derive from the chosen name, since the key is minted by the creation receiver.users/tests/test_signup.py— two tests: the username reachesdisplay_name, and the minted key derives from it.How to test
Complete process
StdSignupKeyCreation.mov
http://localhost:8000/users/javiercorps-88ke/. Auser-xxxxxxxxURL means the name never reacheddisplay_nameand the fix has regressed.Summary by CodeRabbit