Ticket #2539 :: Task: Track documentation contributions and derive the Documenter achievement from them - #2641
Ticket #2539 :: Task: Track documentation contributions and derive the Documenter achievement from them #2641herzog0 wants to merge 24 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds stable deduplication keys for automatic achievements, tracks documentation changes during Git imports, wires documentation commits to the Documenter badge, and centralizes review fingerprint generation. ChangesBadge deduplication and documentation sources
Review identity reuse
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to This change adds automatic documentation achievement tracking and related persistence updates; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant GitRepository
participant libraries.github
participant libraries.doc_paths
participant Commit
participant badges.sources
participant badges.services
participant UserAchievement
GitRepository->>libraries.github: Return git log --numstat output
libraries.github->>libraries.doc_paths: Count documentation files
libraries.doc_paths-->>libraries.github: Return documentation-file count
libraries.github->>Commit: Store docs_files_changed
badges.sources->>badges.services: Yield user, commit, and commit SHA
badges.services->>UserAchievement: Create or update keyed grant
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description is comprehensive and follows the required structure. It explains the purpose, changes, risks, testing steps, and QA expectations. It omits the template's self-review checklist, but the detailed testing information makes this non-critical. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
27ed0df to
657de79
Compare
657de79 to
8451733
Compare
8451733 to
82dd126
Compare
82dd126 to
5fe652c
Compare
5fe652c to
221fe98
Compare
221fe98 to
67de1b6
Compare
| relink_source_achievements( | ||
| Commit, | ||
| dict( | ||
| Commit.objects.filter( | ||
| library_version__library=library | ||
| ).values_list("sha", "pk") | ||
| ), | ||
| ) |
There was a problem hiding this comment.
| relink_source_achievements( | |
| Commit, | |
| dict( | |
| Commit.objects.filter( | |
| library_version__library=library | |
| ).values_list("sha", "pk") | |
| ), | |
| ) | |
| relink_source_achievements( | |
| Commit, | |
| dict( | |
| Commit.objects.filter( | |
| library_version__library=library | |
| ).values_list("sha", "pk") | |
| ), | |
| doomed_ids, | |
| ) |
Hey Teo, I found this bug:
Some commits are stored more than once, under different libraries. The code says so itself, in badges/sources.py. The same commit is stored "once per library version covering it, and once per library sharing the repository."
One commit name then, can appear in two rows belonging to two different libraries. The re-pointing step looks grants up purely by commit name. It never checks whether the grant was one of the broken ones. It also grabs grants that were pointing at a healthy row in a completely different library, and moves those too.
Proposed fix:
When the code re-points grants, it already knows the list of slots it just deleted (doomed_ids). It should require both conditions: matching commit name and pointing at one of the deleted slots, thus only repairing grants that actually broke and leaving grants belonging to other libraries alone.
This fix is probably not just constrained to this file, but I believe it's the main one.
Bellow is a concrete example that Claude created to help me understand:
Walkthrough
Commit
a1b2c3is stored twice:
slot library sha row 500 numeric/conversiona1b2c3row 900 numeric/intervala1b2c3Alice has one badge grant, and it points at row 900 — the copy under
numeric/interval.Step 1 — an admin re-imports
numeric/conversion. Row 500 is deleted and
comes back as row 1500. Row 900 is untouched; a different library was
re-imported, so nothing happened to it.Step 2 — the re-pointing runs. It searches for grants whose sha is
a1b2c3
and finds Alice's. It does not notice that her grant was pointing at row 900,
which is fine and still there. It moves her grant to row 1500.Nothing visibly breaks yet. But Alice's grant is now attached to the
numeric/conversioncopy of the commit, when it should still be attached to the
numeric/intervalcopy.Step 3 — months later,
numeric/conversionis re-imported again, and this
time its version range no longer includes commita1b2c3. So that commit does
not come back undernumeric/conversion.Step 4 — the cleanup step runs. It deletes grants still pointing at slots
that did not come back. Alice's grant points at row 1500, which did not come
back, so her grant is deleted.But row 900 is still sitting there under
numeric/interval, still recording
that Alice wrote that commit. The evidence never went away — the grant was just
moved onto the wrong copy of it back in step 2.The result for Alice: her commit count drops by one for no real reason. If
that tips her below a threshold, the system takes her badge away and writes a
permanent "revoked" entry in the audit trail.
There was a problem hiding this comment.
I'm sorry to bring this up, because it's not part of your changes, but I think it makes sense to at least flag it because the discard_source_achievements function is called in libraries/github.py:601 .
The bug:
discard_source_achievements sends every row id it was given in a single database statement, with no batching.
Proposed fix:
Simply chunk all calls in SYNC_BATCH_SIZE slices. Only 'discard_source_achievements' is lagging. The relink_source_achievements function, which sits up above the former in github.py incorporates batching, for example:
if moved:
UserAchievement.objects.bulk_update(
moved, ["source_object_id"], batch_size=SYNC_BATCH_SIZE
)| ) | ||
| # Whatever still points into the deleted ids is evidence that did | ||
| # not come back, so those grants really are stale. | ||
| discard_source_achievements(Commit, doomed_ids) |
There was a problem hiding this comment.
update_commits decides which versions to rebuild using min_version, but decides which rows to delete without it. The delete is therefore wider than the rebuild.
Before this PR, and in reference to discard_source_achievements , a clean import with a floor deleted commit rows below the floor and left the grants pointing at nothing. That was untidy, but recoverable. A later full import brought the commits back and the counts still held.
Now, however, the grants pointing into those deleted ids are deleted too, and recalculate_badges runs on every affected member. The commit rows are gone from the table, so no later import restores them.
One great example from Claude:
Say someone runs
update_commits.delay(clean=True, min_version="boost-1.85.0").
Measured against your local database, here is what that does to one library:
Geometry commit rows stored today 9,309 covered by versions ≥ boost-1.85.0 291 deleted and never rebuilt 9,018 Step 1 — the delete.
doomedselects all 9,309 rows, ignoring the floor.
Their ids go intodoomed_ids. All 9,309 are deleted.Step 2 — the rebuild. Only versions at or above boost-1.85.0 are in
library_versions, so only 291 rows are re-created. The other 9,018 commits are
simply gone from the table.Step 3 — the relink. It can only re-point grants whose commit came back —
291 rows' worth. Every grant derived from the other 9,018 still points at a
deleted id.Step 4 — the discard.
discard_source_achievements(Commit, doomed_ids)
deletes all of those grants and recalculates the badges they supported.The result for a contributor. Bob's Commits Master count for this library
falls from 300 to a handful. That drops him below his tier threshold, so
recalculate_badgesrevokes his Gold badge and writes a permanent revocation
row in the audit trail.
Fortunately, 'update_commits' is not run with both the clean and min_version arguments in our codebase, so it would require a perhaps relatively simple fix: scope the delete to the versions actually being rebuilt.
doomed = Commit.objects.filter(library_version__in=library_versions.values())
javiercoronadonarvaez
left a comment
javiercoronadonarvaez
left a comment
There was a problem hiding this comment.
Hey @herzog0 all testing was performed accurately, but left a few comments.
Let me know what you think. Overall, it's pretty much ready, so great work on this one.
67de1b6 to
f8e174c
Compare
jlchilders11
left a comment
jlchilders11
left a comment
There was a problem hiding this comment.
Pre approving pending Javiers findings, but I did not find any additional errors, and this seems sound from a design perspective.
update_commits picked the versions to rebuild with min_version but deleted without it, so the delete was always wider than the rebuild. A run with a floor emptied the whole library and re-created only the versions at or above it: for geometry that is 9,309 rows deleted and 291 rebuilt. That used to leave grants pointing at nothing, which a later full import repaired. It no longer does. discard_source_achievements now deletes those grants and recalculates the badges they justified, and neither half comes back on its own - a backfill can only credit evidence that exists, and the commit rows are gone from the table until someone runs an import with no floor. No caller passes both arguments today: the admin button passes clean only, import_commits has no floor option, and release_tasks passes a floor with clean defaulting to false. The combination is reachable by hand-invoking the task, which is enough for the delete and the rebuild to disagree. Scoped with the same predicate that builds library_versions rather than with that dict's values, so it stays identical to current behaviour when min_version is empty and does not depend on LibraryVersion having one row per version name, which nothing enforces.
This branch moved the identity of an automatic grant off the source row and onto the source's own name for the evidence: sources.py says the key "is the source's own name for the evidence rather than a row id", the constraint moved from (user, achievement, content_type, object_id) to (user, achievement, dedup_info), and _library_key falls back to a pk only because nothing re-creates Library rows. discard_source_achievements was the one place left deciding a deletion from the old identity. It deleted every grant pointing into the row ids a caller was removing, which is only the same question where a key has exactly one row. A commit is stored once per library version covering it and once per library sharing the repository, so a sha routinely has several rows and a grant points at whichever one the sweep happened to see first. A clean re-import of one library that no longer reports that sha therefore deleted a grant whose evidence was still sitting in the table under another library, revoked the badge it justified, and left it to be earned again dated later by the next backfill - the exact churn relink_source_achievements exists to prevent. Callers whose keys map to one row keep the old behaviour by passing nothing, so import_reviews is unchanged: its fingerprints are collapsed to one row per key during the import. Survivors are read excluding the ids being discarded, so the check is also correct for a caller that discards before deleting. The relink is deliberately left matching on the key alone. Under this identity the foreign key is provenance, not identity - only the admin's source column reads it, never the count - so moving it between two rows carrying the same sha changes nothing that is counted or displayed, and its breadth is what lets a full clean run pull a pointer back onto a live row. Narrowing it to the ids just deleted would harden a field the model deliberately stopped treating as identity, and would strand pointers orphaned by earlier runs. Also corrects the docstring on the discard test, which argued that a reconcile could not clean up an abandoned grant. It can - an unyielded key reads as stale. The reason the importer has to discard inline is that nothing runs a reconcile: release_tasks runs backfill_achievements, which only adds, and the admin's clean re-import runs no sweep at all.
discard_source_achievements sent every row id in one statement while _sync_source, three functions below it, honours SYNC_BATCH_SIZE for exactly this shape of work. The constant's own comment describes what this function was doing: "Rows per DELETE ... WHERE pk IN (...)". Nothing breaks at today's sizes - the largest library holds 9,309 commit rows - but the cost is not only the parameter list. post_delete is connected to UserAchievement, so the queryset delete cannot take Django's fast path: it materialises every matching row and dispatches a signal per row, and the pair scan above it reads the same rows again. That is precisely why the stale delete in _sync_source batches. The pairs are accumulated across chunks and recalculated after the last delete rather than per chunk. Recalculating per chunk would give a member with grants in two chunks two identical answers, which is the batching this module exists to hold in place. The survivor subquery is deliberately not chunked. A key whose only other row sits in a later chunk is not a survivor, so narrowing that read to the chunk would spare grants whose evidence is on its way out.
3 participants
Issue: #2539
teo/badges-docsSummary & Context
Makes Documenter an automatic achievement. Nothing in the site recorded who wrote
documentation, so the badge could only be granted by hand. The commit importer already clones
every library repo, so asking git for per-file statistics in the same call costs one flag and no
extra API traffic. Each commit now carries a count of the documentation files it touched, and the
achievement counts commits with a non-zero count.
Getting there needed one fix first. An automatic grant identified its evidence by the row it
pointed at, and commit row ids are not stable: the importer deletes and re-creates them, and the
same commit is stored once per release range covering it. So grants were being orphaned and counts
inflated. Grants now carry the source's own id instead (a commit sha, a library key, a review
fingerprint). That part is invisible on screen but is the reason the new source can be trusted.
Changes
dedup_infocolumn and are matched on it instead of on a row id.Reviewcan produce its own dedup fingerprint, which the import command used to keep private.Commit.no key, so a reconcile replaces them rather than adopting them. Nothing is in production; staging
and QA should have their badge tables emptied and rebuilt. I (Teo) will be doing that.
like a regression.
every grant it fed. Cheap to change now, expensive after launch.
again, so doc work predating this shows as zero until then.
it safe for achievements; whether the button should stop doing that is an open question on the
ticket.
doc path. Those two should be settled together before a production backfill.
Screenshots
n/a - no UI. The badge already renders wherever badges render.
Peer-review testing steps
Setup:
just load_production_data,just migrate,docker compose up.The rules. In
just manage shell:A real import.
just manage import_commits --key mp11, then:Spot-check one sha against the repo:
git show --numstat <sha>and count the doc paths yourself.Backfill.
just manage backfill_achievements --source documentation, then run it asecond time. The second run must report
added 0.Pk churn is a non-event. Note the code-commits and documentation grant counts, then
just manage import_commits --key mp11 --cleanand backfill both sources again. Countsunchanged, and no grant left pointing at a deleted row:
Reconcile.
just manage reconcile_achievements --dry-run. Over freshly backfilleddata every source reports
added 0, removed 0.QA: Documenter badge
The Documenter badge could only ever be given by hand, because nothing in the site recorded who
wrote documentation. It is now worked out automatically from the commits: each commit is checked
for documentation files, and the badge counts the commits that touched some.
Two things must be true before any of this will show results, and neither is something you can do
yourself:
commit is read in, so every commit already in the database reads zero until a full re-import has
run. On a database copied from production that is all of them.
not carry the new identifier and will be replaced rather than reused.
Teo does both of these. Confirm with him that they have been done before you start. If you run
a backfill first, you will correctly get zero grants and it will look like the feature does not
work.
Related: after that re-import, counts on some existing badges will go down. That is the correct
number arriving after a bug that inflated them - not a regression.
What this change does
a review fingerprint - instead of pointing at a database row that can be replaced. There is a new
read-only Dedup info field showing it.
the thing that earned it.
What it does not do
separately.
Before you start
/admin/badges/achievement/or
/admin/badges/badge/and findDocumenter.
Test 1: Documentation is a source you can pick
/admin/badges/userachievement/.Expected: Documentation is in the list. Pick it and press Backfill achievements.
Expected: the status line finishes in place, without a reload.
If you get zero grants, do not report it straight away. There are two ordinary reasons, and
both are listed at the top of this document: the commit history has not been re-imported, or the
commits belong to people whose email is not linked to a member account. Check with Teo before
treating it as a failure.
Test 2: the achievements say where they came from
/admin/badges/userachievement/,filter Achievement to Documentation.
Expected: it opens the commit that earned it, and the commit message reads like documentation
work.
Expected:
Test 3: you can search by commit id
/admin/badges/userachievement/.Expected: it finds that achievement. This did not work before this change.
Test 4: the badge was awarded
/admin/badges/userbadge/.Expected: the members from Test 2 hold Documenter at whatever level their count reaches.
Expected: it shows how many valid achievements they have and how many more they need for the
next level, in plain words.
Test 5: running it twice changes nothing (the most important check)
/admin/badges/achievementsyncrun/.Expected: two documentation rows, the second one with Added 0, both naming you under
Triggered by.
This is what stops the weekly job doubling everybody's count every time it runs. If the second
run adds anything at all, stop and report it immediately with both run numbers.
Test 6: re-importing commits does not disturb anybody's badges
This is the whole reason for the new Dedup info field, and it is worth doing carefully.
write down: how many achievements they have, what level they hold, and the Awarded at date on
the badge.
Expected, all four:
A badge that disappeared, or an award date that jumped to today, is a serious bug here. Report
it with the member's email and both sets of numbers.
Test 7: hand-given achievements are left alone
/admin/badges/userachievement/→ Add, grant Documentation to any member by hand, with a note.
Expected: the hand-given achievement is still there, still shows your note, and still names you
as the person who granted it. Automatic runs never overwrite a manual decision.
Test 8: the badge shows on the profile
https://www.cppal-dev.boost.org/.
Expected: the Documenter badge appears in their badges list along with the rest.
/admin/users/user/, tick Hide badges,save, and open their profile again in a logged-out window.
Expected: no badges at all on their public profile. Untick it afterwards.
Test 9: commit messages got longer
A side effect worth knowing about, because it is visible and it is not a bug.
Commit messages were previously being cut short in one specific case. After a re-import, roughly
1,500 commits will show their full message where they used to show a truncated one.
Expected: longer, complete commit messages on
/admin/libraries/commit/. What would bea bug is the opposite: a message with stray numbers, tabs or file names stuck on the end of it.
Report that if you see it.
Things that look wrong but are meant to be that way
any documentation file. That wording is still being settled - do not report it.
Reporting anything you find
Please include:
/admin/badges/achievementsyncrun/.commit.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation