Story #2586 :: Task: Wire the Library Maintenance achievement, counting libraries and not versions - #2587
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:
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 |
32fb541 to
07a2b1c
Compare
07a2b1c to
d3c392d
Compare
d3c392d to
1696c2c
Compare
1696c2c to
a253833
Compare
| def test_iter_library_maintenance_dedupes_versions(plain_user): | ||
| """Maintaining many versions of one library yields a single pair.""" | ||
| library = baker.make("libraries.Library") | ||
| for _ in range(3): | ||
| version = baker.make("libraries.LibraryVersion", library=library) | ||
| version.maintainers.add(plain_user) | ||
|
|
||
| pairs = list(sources._iter_library_maintenance()) | ||
| assert pairs == [(plain_user, library)] |
There was a problem hiding this comment.
Nit suggestion: Perhaps we can update this test to add another library here, and assert that pairs == [(plain_user, library), (plain_user, another library)] to ensure we can cover that common case as well!
a253833 to
2703076
Compare
2703076 to
53d58b6
Compare
53d58b6 to
6319f28
Compare
6319f28 to
6f3b5a0
Compare
Issue: #2586
teo/2541-source-library-authoringSummary & Context
Wires the Library Maintenance achievement. The one thing worth reviewing here is the grain:
maintainers are recorded per
LibraryVersion, but the badge counts libraries maintained(1 / 2 / 5 / 10 / 20), so the iterator deduplicates across versions and yields the
Libraryasthe achievement source.
Without that, a maintainer of one library with 40 releases would hold diamond.
Changes
_iter_library_maintenancewalksLibraryVersionwith its maintainers prefetched and yields(user, library)once per(user, library)pair, tracking what it has already seen.BACKFILL_ITERATORS.test_iter_library_maintenance_dedupes_versions- three versions of one library yield exactlyone pair.
test_backfill_library_maintenance- two versions, one grant, one badge.(user, library)pair,bounded by the number of maintainer relationships, not by the number of versions - a few
thousand entries at Boost's scale.
Library, not theLibraryVersion. This iswhat makes the grant idempotent under
unique_automatic_user_achievement_sourceand what letsreconciliation match it. An earlier iteration of this feature recorded versions and had to be
cleaned up with a data migration; a fresh install never creates those rows.
reconcile, the two-way command, not on a backfill. That is by design.
Screenshots
n/a - no UI.
Peer-review testing steps
The dedup is basically the whole PR, and it is visible on a changelist.
The source is wired.
/admin/badges/badge/- the Maintainer row's Automatic column is atick and its ladder reads
1 / 2 / 5 / 10 / 20.Make yourself a maintainer of one library, several times over.
/admin/libraries/libraryversion/, filter by a single library, then open three of its versionsand add your own user to Maintainers on each.
Backfill.
/admin/badges/userachievement/-> Backfill achievements with Source set toLibrary Maintenance.
Three versions, one grant. Filter that changelist by Achievement: Library Maintenance: you
have exactly one row, and its Source column links to the library, not to any of the
three versions. Without the dedup this step is where you would see three.
/admin/badges/userbadge/shows Maintainer / Bronze; the per-member page (your name, linked from either badge changelist)
reads one valid grant and 1 to go to Silver.
A second library does move the count. Add yourself to Maintainers on one version of a
different library and press Backfill again: two grants, and Maintainer / Silver is
awarded. This is what proves the dedup is per (member, library) and not just "one grant per member".
Removal is a reconcile, and it demotes. Drop yourself from all three versions of the first
library. Backfill changes nothing. Then Reconcile achievements with Source on Library
Maintenance: the preview reports one removal, Apply, and the Silver row is revoked with
Count at revocation 1 while Bronze stays held.
/admin/badges/achievementsyncrun/shows the runwith Removed 1, and the revoked badge's notes name it.
Sanity-check against real data. Backfill All sources on a production copy, then filter
/admin/badges/userbadge/to Badge: Maintainer and Rank: Diamond. Every name there must genuinelymaintain 20 or more libraries - filter
/admin/badges/userachievement/by Achievement: LibraryMaintenance, search their email, and count the rows, whose Source links must all be distinct
libraries. Without the dedup, anyone maintaining a single long-lived library would be sitting in that
Diamond list, which is the failure this PR exists to prevent.
Self-review Checklist
Backend