Story #2493 - Add achievement and badge models with badge recalculation - #2569
Story #2493 - Add achievement and badge models with badge recalculation#2569herzog0 wants to merge 8 commits into
Conversation
📝 WalkthroughWalkthroughAdds the ChangesBadges domain
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UserAchievement
participant badges_signals
participant badges_tasks
participant badges_services
participant UserBadge
UserAchievement->>badges_signals: save or delete achievement
badges_signals->>badges_tasks: schedule achievement recalculation
badges_tasks->>badges_services: recalculate_many(achievement_pairs)
badges_services->>UserBadge: award, restore, or revoke badge tiers
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🧹 Nitpick comments (15)
badges/models.py (2)
157-162: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider
PROTECTinstead ofCASCADEforBadge.achievement.
UserBadge.tierusesPROTECTto keep the record of why a member earned a badge. That protection is bypassed by this chain: deleting anAchievementcascades toBadge, then toBadgeTierand to everyUserBadgerow, including revoked rows kept for audit.Achievementis admin-editable, so a single admin delete can erase the badge history.
PROTECTonBadge.achievement(and onBadgeTier.badge) makes the failure loud instead of destructive.♻️ Proposed change
achievement = models.ForeignKey( Achievement, - on_delete=models.CASCADE, + on_delete=models.PROTECT, related_name="badges", help_text=_("Which achievement type feeds this badge."), )
UserAchievement.achievementcascading is a separate decision; the fact rows arguably follow the achievement type. Confirm the intended deletion policy before changing both.🤖 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 `@badges/models.py` around lines 157 - 162, Update the Badge.achievement ForeignKey to use Django’s PROTECT deletion policy instead of CASCADE, preserving badge, tier, and UserBadge audit history when an Achievement is deleted. Do not change UserAchievement.achievement; handle only the Badge.achievement relationship shown in the diff.
331-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
UniqueConstraintfor the composite uniqueness rule.In Django 6.0,
UniqueConstraintis the recommended replacement forunique_together. Sincebadges/models.pyalready usesUniqueConstraintelsewhere, move this rule toMeta.constraintsand regeneratebadges/migrations/0001_initial.pyso the model schema appears asAddConstraintinstead ofAlterUniqueTogether.♻️ Proposed change
class Meta: ordering = ("-awarded_at",) - unique_together = ("badge", "user", "tier") + constraints = [ + models.UniqueConstraint( + fields=["badge", "user", "tier"], + name="unique_user_badge_tier", + ) + ]🤖 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 `@badges/models.py` around lines 331 - 333, Replace unique_together in the affected model’s Meta with a matching UniqueConstraint in Meta.constraints, preserving the badge/user/tier composite uniqueness. Regenerate badges/migrations/0001_initial.py so the schema uses AddConstraint rather than AlterUniqueTogether, following the existing UniqueConstraint pattern in badges/models.py.badges/tests/fixtures.py (1)
102-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote: the
badgefixture collides withcatalogueonMAINTAINER.The
badgefixture creates aBadgeLabel.MAINTAINERbadge with tiers 1/3/5. Thecataloguefixture also seeds MAINTAINER, with 1/2/5/10/20, andbadges/tests/test_catalogue.py(Lines 110 and 125) reaches for the MAINTAINER rows by label.No test in this PR requests both fixtures, so nothing fails today. If a future test requests both,
seed_cataloguefinds the existing badge and bronze/silver/gold rows and skips them, and the resulting ladder is a silent mix of both fixtures. Using a label that the catalogue does not seed is not possible, because the catalogue covers everyBadgeLabel. A short warning in the fixture docstring prevents the trap.🤖 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 `@badges/tests/fixtures.py` around lines 102 - 109, Update the badge fixture docstring to warn that its MAINTAINER badge and 1/3/5 tiers collide with the catalogue fixture’s seeded MAINTAINER data, causing a mixed ladder if both fixtures are requested. Keep the fixture behavior unchanged and document that tests should avoid combining these fixtures.badges/migrations/0001_initial.py (1)
361-373: 🚀 Performance & Scalability | 🔵 TrivialConsider covering indexes for the recalculation count query.
Badge recalculation counts valid
UserAchievementrows per (user, achievement) and then readsUserBadgerows per (user, badge). The schema provides single-column FK indexes only, plus the partial unique index on automatic rows, whose leading column order does not serve auser + achievement + is_validcount.Row counts are small today, so this is a forward-looking suggestion. If recalculation runs per achievement event, add composite indexes when the table grows:
class Meta: indexes = [ models.Index(fields=["user", "achievement", "is_valid"]), ]and, on
UserBadge, an index on("user", "badge").🤖 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 `@badges/migrations/0001_initial.py` around lines 361 - 373, Extend the migration’s schema changes to add covering composite indexes for recalculation lookups: index UserAchievement by user, achievement, and is_valid, and index UserBadge by user and badge. Use the models.Index definitions in the relevant model Meta configurations or equivalent migration operations, preserving the existing unique constraint.badges/tests/test_models.py (1)
62-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the revoked branch and the
active()queryset.The test asserts
is_activefor an un-revoked badge only. Neitheris_active is Falseafter revocation norUserBadge.objects.active()is covered, and the revocation semantics carry the whole soft-delete design.💚 Proposed additional tests
def test_user_badge_is_active_property(badge, plain_user): tier = badge.tiers.get(rank=TierRank.BRONZE) ub = UserBadge.objects.create(badge=badge, user=plain_user, tier=tier) assert ub.is_active is True + + +def test_user_badge_is_inactive_once_revoked(badge, plain_user): + """Setting revoked_at flips is_active and removes the row from active().""" + tier = badge.tiers.get(rank=TierRank.BRONZE) + ub = UserBadge.objects.create(badge=badge, user=plain_user, tier=tier) + + ub.revoked_at = timezone.now() + ub.revocation_source = RevocationSource.MANUAL + ub.save() + + ub.refresh_from_db() + assert ub.is_active is False + assert not UserBadge.objects.active().filter(pk=ub.pk).exists()The new test needs
RevocationSourcein the import at Line 10.🤖 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 `@badges/tests/test_models.py` around lines 62 - 65, Extend test_user_badge_is_active_property to revoke the created UserBadge using RevocationSource, assert is_active is False afterward, and verify UserBadge.objects.active() excludes the revoked record while retaining an unrevoked record. Add RevocationSource to the test imports and cover both active-queryset behavior and revocation semantics.badges/catalogue.py (1)
145-155: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueOptional: detect a badge whose achievement no longer matches the catalogue.
get_or_create(label=label, ...)keeps an existingBadgerow untouched, which is the documented intent. It also means aBadgethat points at a differentAchievementthan the catalogue declares stays silently wrong, and recalculation then counts the wrong achievement.Consider logging a warning when the existing row disagrees, so the mismatch surfaces on the next deploy instead of as wrong badge counts.
♻️ Proposed change
badge, _created = badge_model.objects.get_or_create( label=label, defaults={"achievement": achievement, "description": description}, ) + if badge.achievement_id != achievement.pk: + logger.warning( + "Badge %s points at achievement %s, catalogue expects %s.", + label, + badge.achievement_id, + achievement.pk, + )The migration runs this with historical models, so keep the check read-only and never repoint the row automatically.
🤖 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 `@badges/catalogue.py` around lines 145 - 155, After get_or_create in the badge catalogue setup, detect when the existing Badge’s achievement differs from the catalogue’s achievement and log a warning identifying the mismatch. Keep the check read-only: do not update or repoint the Badge, and preserve the existing tier creation behavior.badges/tests/test_services.py (2)
297-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the function-local imports to the module level.
Four imports sit inside test bodies.
BadgeTierat Line 322 shadows the module-level import at Line 10 with the same symbol, which adds no value.ProtectedError,BadgeLabel, andValidationErrorhave no circular-import reason to stay local.Also applies to: 321-322, 380-380
🤖 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 `@badges/tests/test_services.py` at line 297, Move the function-local imports of ProtectedError, BadgeLabel, ValidationError, and BadgeTier in the affected tests to the module-level imports, removing the redundant local BadgeTier shadowing while preserving the existing test behavior.
305-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe final assertion is trivially true without the
badgefixture.This test requests
achievementandplain_userbut notbadge. NoBadgeorBadgeTierexists for the achievement, soUserBadge.objects.exists()returnsFalseregardless of whatrecalculate_badgesdoes. The test proves that stale ids do not raise, but not that they write nothing.Add the
badgefixture so the achievement has active tiers. The assertion then covers the documented claim that a missing user counts zero achievements.💚 Proposed change
-def test_recalculation_noop_when_user_or_achievement_gone(achievement, plain_user): +def test_recalculation_noop_when_user_or_achievement_gone( + achievement, badge, plain_user +): """Stale ids are a no-op rather than a crash. A missing user simply counts zero achievements; a missing achievement has nothing to reconcile against, so it logs and returns. """ + assert badge.tiers.filter(is_active=True).exists() recalculate_badges(99999999, achievement.pk)🤖 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 `@badges/tests/test_services.py` around lines 305 - 314, Add the existing badge fixture to test_recalculation_noop_when_user_or_achievement_gone so the achievement has active tiers before invoking recalculate_badges. Keep the stale-ID calls and final UserBadge.objects.exists() assertion unchanged, ensuring the test verifies no records are written when the user is missing.badges/tests/test_signals.py (1)
94-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that no task is queued, not only that the tier row is gone.
The docstring states that a tier cascaded away with its badge has no achievement left to visit. The assertion checks only that the
BadgeTierrow was deleted, which the cascade guarantees regardless of the handler. The early-return branch atbadges/signals.pyLine 62 is therefore not covered.Capture the callback list and assert it is empty.
💚 Proposed assertion
- with django_capture_on_commit_callbacks(execute=True): + with django_capture_on_commit_callbacks(execute=True) as callbacks: Badge.objects.filter(pk=other.pk).delete() + assert callbacks == [] assert not BadgeTier.objects.filter(pk=tier.pk).exists()🤖 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 `@badges/tests/test_signals.py` around lines 94 - 97, Update the deletion test around django_capture_on_commit_callbacks to capture the queued callbacks, then assert the callback list is empty after deleting the badge tier. Keep the existing BadgeTier deletion assertion, and ensure this exercises the early-return path in the badge signal handler.badges/tasks.py (1)
13-16: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a time limit and a retry policy to the sweep task.
The task visits every
(user, achievement)pair for one achievement, so its runtime grows with the table. The bare@shared_taskdecorator applies no soft time limit, so a slow sweep can hold a worker slot indefinitely. The task is idempotent, so a retry is safe.Logging the returned count also helps confirm that a queued sweep actually ran, because the count currently reaches the result backend only.
♻️ Proposed task options
-@shared_task -def recalculate_achievement_task(achievement_id): - """Recalculate every (user, achievement) pair for one achievement type.""" - return recalculate_many(achievement_pairs([achievement_id])) +@shared_task( + autoretry_for=(Exception,), + retry_backoff=True, + max_retries=3, + soft_time_limit=600, + time_limit=660, +) +def recalculate_achievement_task(achievement_id): + """Recalculate every (user, achievement) pair for one achievement type.""" + count = recalculate_many(achievement_pairs([achievement_id])) + logger.info( + "Recalculated %s pair(s) for achievement %s.", count, achievement_id + ) + return countMatch the limits and the retry settings to the conventions used by the other task modules in the project.
🤖 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 `@badges/tasks.py` around lines 13 - 16, Update the shared_task configuration on recalculate_achievement_task to add a soft time limit and the project’s standard retry policy, matching conventions used by other task modules. Capture the return value from recalculate_many and log the completed sweep count before returning it, preserving the existing task result.badges/services.py (1)
49-59: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider wrapping the delete and recalculation in one transaction.
grants.delete()commits before the loop runs. If the process fails partway through the loop, some badge rows stay above their threshold with no achievement rows behind them. A singletransaction.atomic()block around the delete and the loop keeps the two writes consistent.Also note that
QuerySet.delete()firespost_delete, andrecalculate_on_achievement_deleteinbadges/signals.pyalready recalculates each pair. The explicit loop is therefore a second, idempotent pass. Keep it if you want protection against future fast-delete paths, otherwise it can go.♻️ Proposed change to make the discard atomic
content_type = ContentType.objects.get_for_model(model) - grants = UserAchievement.objects.filter( - source_content_type=content_type, source_object_id__in=object_ids - ) - pairs = set(grants.values_list("user_id", "achievement_id")) - grants.delete() - for user_id, achievement_id in pairs: - recalculate_badges(user_id, achievement_id) + with transaction.atomic(): + grants = UserAchievement.objects.filter( + source_content_type=content_type, source_object_id__in=object_ids + ) + pairs = set(grants.values_list("user_id", "achievement_id")) + grants.delete() + for user_id, achievement_id in pairs: + recalculate_badges(user_id, achievement_id)🤖 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 `@badges/services.py` around lines 49 - 59, Wrap the grants.delete() call and the subsequent recalculate_badges loop in a single transaction.atomic() block within the surrounding function. Preserve the existing pair collection and explicit recalculation unless intentionally removing the redundant pass, ensuring partial deletion and recalculation cannot leave inconsistent badge state.badges/management/commands/recalculate_badges.py (1)
21-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing the scoping filters that
achievement_pairsalready supports.
achievement_pairsacceptsachievement_idsanduser_ids, andbadges/tests/test_services.pycovers both. The command always sweeps the whole table. Adding--userand--achievementoptions lets an operator repair one member or one achievement type without a full sweep.♻️ Proposed options
help = "Recalculate UserBadge state for every (user, achievement) pair." + def add_arguments(self, parser): + """Allow a scoped rebuild instead of a full sweep.""" + parser.add_argument("--user", type=int, action="append", dest="user_ids") + parser.add_argument( + "--achievement", type=int, action="append", dest="achievement_ids" + ) + def handle(self, *args, **options): """Recalculate each distinct (user, achievement) pair worth visiting.""" - count = recalculate_many(achievement_pairs()) + count = recalculate_many( + achievement_pairs( + achievement_ids=options["achievement_ids"], + user_ids=options["user_ids"], + ) + )🤖 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 `@badges/management/commands/recalculate_badges.py` around lines 21 - 26, Update the handle method to accept --user and --achievement options, pass their values as user_ids and achievement_ids to achievement_pairs, and preserve the current full-table recalculation when either option is omitted. Keep the existing success output and recalculate_many flow intact.badges/signals.py (1)
57-65: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDeduplicate the queued sweeps per transaction.
Each
BadgeTiersave or delete queues onerecalculate_achievement_taskfor the whole achievement. A bulk tier edit therefore queues one full-achievement sweep per tier. The catalogue migration in this PR creates forty tiers in one transaction, so forty sweeps are queued at commit, and each one visits every(user, achievement)pair for its achievement.
replace_tierinbadges/services.pyalso produces two saves per replacement, so a single threshold edit queues two sweeps.The work is idempotent, so this is throughput, not correctness. Collect the achievement ids in a per-transaction set and queue one task per id at commit.
♻️ Sketch of a per-transaction dedupe
+def _queue_once(achievement_id): + """Queue at most one sweep per achievement per transaction.""" + conn = transaction.get_connection() + pending = getattr(conn, "_badges_pending_sweeps", None) + if pending is None: + pending = conn._badges_pending_sweeps = set() + + def flush(ids=pending): + conn._badges_pending_sweeps = None + for achievement_pk in ids: + recalculate_achievement_task.delay(achievement_pk) + + transaction.on_commit(flush) + pending.add(achievement_id) + + `@receiver`([post_save, post_delete], sender=BadgeTier) def recalculate_on_tier_change(sender, instance, **kwargs): @@ if achievement_id is None: # the badge cascaded away with the tier return - transaction.on_commit(lambda: recalculate_achievement_task.delay(achievement_id)) + _queue_once(achievement_id)Confirm the attribute lifetime against the connection object before adopting this exact form.
🤖 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 `@badges/signals.py` around lines 57 - 65, Update the BadgeTier save/delete signal flow around recalculate_achievement_task to collect achievement IDs in a per-transaction set associated with the database connection, rather than registering one on_commit callback per change. Register a single commit callback that queues one recalculation task for each collected ID, and verify the set is scoped and reset correctly across transactions.badges/tests/test_commands.py (2)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the enum values instead of the raw strings.
_grantpassessource_type="manual"as a literal, whilebadges/enums.pydefinesSourceTypefor this field. Line 40 asserts against the literal"library-authoring", while Line 34 grants throughAchievementSlug.LIBRARY_AUTHORING. If the slug value changes, the grant follows the enum but the assertion does not, and the test fails for the wrong reason.♻️ Proposed change
-from badges.enums import AchievementSlug +from badges.enums import AchievementSlug, SourceType @@ return UserAchievement.objects.create( user=user, achievement=Achievement.objects.get(slug=slug), - source_type="manual", + source_type=SourceType.MANUAL, ) @@ assert UserBadge.objects.filter( - user=plain_user, badge__achievement__slug="library-authoring" + user=plain_user, + badge__achievement__slug=AchievementSlug.LIBRARY_AUTHORING, ).exists()Confirm the
SourceTypemember name inbadges/enums.pybefore applying this.Also applies to: 39-41
🤖 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 `@badges/tests/test_commands.py` around lines 25 - 29, Update the test helper _grant and the related assertions to use the appropriate SourceType enum member from badges/enums.py instead of raw "manual" and "library-authoring" strings, confirming the exact member name before applying the change. Keep the existing achievement slug behavior unchanged.
69-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
_raw_deletein the tests that simulate signal-freeUserAchievementdeletion.QuerySet._raw_deleteis a private Django API, so these tests can break on Django changes. Use a dedicated test helper that clearspost_deletetemporarily and calls normaldelete(), or use supported raw SQL insidedjango.test.TestCase.🤖 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 `@badges/tests/test_commands.py` around lines 69 - 70, Replace the private QuerySet._raw_delete usage in the signal-free UserAchievement deletion setup in badges/tests/test_commands.py lines 69-70 and badges/tests/test_services.py line 448 with a supported approach: add or reuse a dedicated test helper that temporarily disconnects post_delete receivers and invokes normal delete(), or use supported raw SQL through django.test.TestCase. Apply the same behavior at both sites while ensuring post_delete receivers are restored afterward.
🤖 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 `@badges/models.py`:
- Around line 128-141: The Meta constraints for the badge model must enforce
that automatic grants include both source_content_type and source_object_id. Add
a CheckConstraint alongside unique_automatic_user_achievement_source requiring
both fields to be non-null whenever source_type is "automatic", and update the
corresponding migration to create the same database constraint.
In `@badges/services.py`:
- Around line 182-214: Serialize recalculations for each (user_id, achievement)
pair inside recalculate_badges’s atomic transaction before reading valid_count
or held rows. Use the established PostgreSQL advisory-lock approach keyed by
both identifiers, or lock the existing user-badge rows with select_for_update
while ensuring the no-row first-award case is also serialized; keep all
derived-state reads and award/revoke writes within that protected section.
---
Nitpick comments:
In `@badges/catalogue.py`:
- Around line 145-155: After get_or_create in the badge catalogue setup, detect
when the existing Badge’s achievement differs from the catalogue’s achievement
and log a warning identifying the mismatch. Keep the check read-only: do not
update or repoint the Badge, and preserve the existing tier creation behavior.
In `@badges/management/commands/recalculate_badges.py`:
- Around line 21-26: Update the handle method to accept --user and --achievement
options, pass their values as user_ids and achievement_ids to achievement_pairs,
and preserve the current full-table recalculation when either option is omitted.
Keep the existing success output and recalculate_many flow intact.
In `@badges/migrations/0001_initial.py`:
- Around line 361-373: Extend the migration’s schema changes to add covering
composite indexes for recalculation lookups: index UserAchievement by user,
achievement, and is_valid, and index UserBadge by user and badge. Use the
models.Index definitions in the relevant model Meta configurations or equivalent
migration operations, preserving the existing unique constraint.
In `@badges/models.py`:
- Around line 157-162: Update the Badge.achievement ForeignKey to use Django’s
PROTECT deletion policy instead of CASCADE, preserving badge, tier, and
UserBadge audit history when an Achievement is deleted. Do not change
UserAchievement.achievement; handle only the Badge.achievement relationship
shown in the diff.
- Around line 331-333: Replace unique_together in the affected model’s Meta with
a matching UniqueConstraint in Meta.constraints, preserving the badge/user/tier
composite uniqueness. Regenerate badges/migrations/0001_initial.py so the schema
uses AddConstraint rather than AlterUniqueTogether, following the existing
UniqueConstraint pattern in badges/models.py.
In `@badges/services.py`:
- Around line 49-59: Wrap the grants.delete() call and the subsequent
recalculate_badges loop in a single transaction.atomic() block within the
surrounding function. Preserve the existing pair collection and explicit
recalculation unless intentionally removing the redundant pass, ensuring partial
deletion and recalculation cannot leave inconsistent badge state.
In `@badges/signals.py`:
- Around line 57-65: Update the BadgeTier save/delete signal flow around
recalculate_achievement_task to collect achievement IDs in a per-transaction set
associated with the database connection, rather than registering one on_commit
callback per change. Register a single commit callback that queues one
recalculation task for each collected ID, and verify the set is scoped and reset
correctly across transactions.
In `@badges/tasks.py`:
- Around line 13-16: Update the shared_task configuration on
recalculate_achievement_task to add a soft time limit and the project’s standard
retry policy, matching conventions used by other task modules. Capture the
return value from recalculate_many and log the completed sweep count before
returning it, preserving the existing task result.
In `@badges/tests/fixtures.py`:
- Around line 102-109: Update the badge fixture docstring to warn that its
MAINTAINER badge and 1/3/5 tiers collide with the catalogue fixture’s seeded
MAINTAINER data, causing a mixed ladder if both fixtures are requested. Keep the
fixture behavior unchanged and document that tests should avoid combining these
fixtures.
In `@badges/tests/test_commands.py`:
- Around line 25-29: Update the test helper _grant and the related assertions to
use the appropriate SourceType enum member from badges/enums.py instead of raw
"manual" and "library-authoring" strings, confirming the exact member name
before applying the change. Keep the existing achievement slug behavior
unchanged.
- Around line 69-70: Replace the private QuerySet._raw_delete usage in the
signal-free UserAchievement deletion setup in badges/tests/test_commands.py
lines 69-70 and badges/tests/test_services.py line 448 with a supported
approach: add or reuse a dedicated test helper that temporarily disconnects
post_delete receivers and invokes normal delete(), or use supported raw SQL
through django.test.TestCase. Apply the same behavior at both sites while
ensuring post_delete receivers are restored afterward.
In `@badges/tests/test_models.py`:
- Around line 62-65: Extend test_user_badge_is_active_property to revoke the
created UserBadge using RevocationSource, assert is_active is False afterward,
and verify UserBadge.objects.active() excludes the revoked record while
retaining an unrevoked record. Add RevocationSource to the test imports and
cover both active-queryset behavior and revocation semantics.
In `@badges/tests/test_services.py`:
- Line 297: Move the function-local imports of ProtectedError, BadgeLabel,
ValidationError, and BadgeTier in the affected tests to the module-level
imports, removing the redundant local BadgeTier shadowing while preserving the
existing test behavior.
- Around line 305-314: Add the existing badge fixture to
test_recalculation_noop_when_user_or_achievement_gone so the achievement has
active tiers before invoking recalculate_badges. Keep the stale-ID calls and
final UserBadge.objects.exists() assertion unchanged, ensuring the test verifies
no records are written when the user is missing.
In `@badges/tests/test_signals.py`:
- Around line 94-97: Update the deletion test around
django_capture_on_commit_callbacks to capture the queued callbacks, then assert
the callback list is empty after deleting the badge tier. Keep the existing
BadgeTier deletion assertion, and ensure this exercises the early-return path in
the badge signal handler.
🪄 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: c0b2e490-014d-4d6a-a39b-f0057aa1e953
📒 Files selected for processing (26)
badges/__init__.pybadges/apps.pybadges/catalogue.pybadges/enums.pybadges/management/__init__.pybadges/management/commands/__init__.pybadges/management/commands/recalculate_badges.pybadges/migrations/0001_initial.pybadges/migrations/0002_seed_achievements_and_badges.pybadges/migrations/__init__.pybadges/models.pybadges/services.pybadges/signals.pybadges/tasks.pybadges/tests/__init__.pybadges/tests/fixtures.pybadges/tests/test_catalogue.pybadges/tests/test_commands.pybadges/tests/test_enums.pybadges/tests/test_models.pybadges/tests/test_services.pybadges/tests/test_signals.pyconfig/settings.pyconftest.pyusers/migrations/0026_remove_user_badges_delete_badge.pyusers/models.py
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)
badges/services.py (2)
73-76: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftSuppress per-row recalculation during bulk source disposal.
grants.delete()emitspost_deletefor everyUserAchievement.badges/signals.pyrecalculates each row, then this loop recalculates each distinct pair again. Deleting many grants for one pair causes repeated identical reconciliation work.Add a scoped signal-suppression mechanism for this bulk operation. Recalculate the collected distinct pairs once after deletion.
🤖 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 `@badges/services.py` around lines 73 - 76, Update the bulk disposal flow around grants.delete() in the relevant service function to use a scoped suppression mechanism for UserAchievement post-delete recalculation, preventing badges/signals.py from reconciling each deleted row. Keep collecting distinct user_id/achievement_id pairs, perform the deletion within the suppression scope, then invoke recalculate_badges once for each collected pair afterward.
108-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the replacement before creating it.
BadgeTier.objects.create()bypassesBadgeTier.clean(), soreplace_tier()persists only against the one-active-rank database constraint. Add a validated bulk replacement API that checks the complete proposed ladder, including existing active siblings, before retiring/creating tiers.🤖 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 `@badges/services.py` around lines 108 - 121, The replace_tier flow must validate the complete proposed badge ladder before mutating storage, rather than relying on BadgeTier.objects.create and only the database constraint. Add or reuse a validated bulk replacement API that includes existing active sibling tiers and the replacement values, invoke it from replace_tier before deactivate_tier, and preserve the atomic retire/create behavior only after validation succeeds.
🤖 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 `@badges/services.py`:
- Around line 73-76: Update the bulk disposal flow around grants.delete() in the
relevant service function to use a scoped suppression mechanism for
UserAchievement post-delete recalculation, preventing badges/signals.py from
reconciling each deleted row. Keep collecting distinct user_id/achievement_id
pairs, perform the deletion within the suppression scope, then invoke
recalculate_badges once for each collected pair afterward.
- Around line 108-121: The replace_tier flow must validate the complete proposed
badge ladder before mutating storage, rather than relying on
BadgeTier.objects.create and only the database constraint. Add or reuse a
validated bulk replacement API that includes existing active sibling tiers and
the replacement values, invoke it from replace_tier before deactivate_tier, and
preserve the atomic retire/create behavior only after validation succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf2123c6-3113-4d25-9ea1-d194608ceaf1
📒 Files selected for processing (13)
badges/catalogue.pybadges/enums.pybadges/management/commands/recalculate_badges.pybadges/migrations/0001_initial.pybadges/models.pybadges/services.pybadges/signals.pybadges/tasks.pybadges/tests/fixtures.pybadges/tests/test_commands.pybadges/tests/test_models.pybadges/tests/test_services.pybadges/tests/test_signals.py
🚧 Files skipped from review as they are similar to previous changes (9)
- badges/tasks.py
- badges/management/commands/recalculate_badges.py
- badges/enums.py
- badges/migrations/0001_initial.py
- badges/signals.py
- badges/catalogue.py
- badges/tests/test_signals.py
- badges/tests/fixtures.py
- badges/tests/test_commands.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
badges/seed_data.py (1)
135-151: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFreeze the data used by migration
0002.
badges/migrations/0002_seed_achievements_and_badges.pycallsseed_cataloguefrom this mutable module. Django replays migrations on fresh databases, so later edits toSEED_CATALOGUEchange the output of the historical migration. Fresh and upgraded databases can then diverge. A later replacement migration can also fail to find the threshold it expects.Keep an immutable copy of the initial catalogue in migration
0002, or call a versioned helper that will not change. Use a new data migration for every catalogue change.Also applies to: 1-18
🤖 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 `@badges/seed_data.py` around lines 135 - 151, Freeze the catalogue consumed by seed_catalogue in migration 0002 by moving the initial data into an immutable migration-local constant or versioned helper, rather than reading the mutable SEED_CATALOGUE. Keep seed_catalogue’s existing idempotent creation behavior and require future catalogue changes to use new data migrations.
🤖 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 `@badges/tests/test_seed_data.py`:
- Around line 27-29: Update the import guard test around SEED_DATA_IMPORTERS,
IMPORT_FORMS, and its scanning logic to parse Python files with ast.Import and
ast.ImportFrom nodes instead of searching raw text. Resolve relative imports
such as from .seed_data and from . import seed_data against the badges package,
then assert the resolved module is not an offender while ignoring comments and
docstrings.
---
Outside diff comments:
In `@badges/seed_data.py`:
- Around line 135-151: Freeze the catalogue consumed by seed_catalogue in
migration 0002 by moving the initial data into an immutable migration-local
constant or versioned helper, rather than reading the mutable SEED_CATALOGUE.
Keep seed_catalogue’s existing idempotent creation behavior and require future
catalogue changes to use new data migrations.
🪄 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: 17d1eb4d-f110-4c1b-a30a-9f0a37bdeee5
📒 Files selected for processing (5)
badges/enums.pybadges/migrations/0002_seed_achievements_and_badges.pybadges/seed_data.pybadges/tests/fixtures.pybadges/tests/test_seed_data.py
🚧 Files skipped from review as they are similar to previous changes (2)
- badges/migrations/0002_seed_achievements_and_badges.py
- badges/tests/fixtures.py
| SEED_DATA_IMPORTERS = ("badges/seed_data.py", "badges/migrations/", "badges/tests/") | ||
| IMPORT_FORMS = ("badges.seed_data", "from badges import seed_data") | ||
| UNSEARCHED_DIRS = {".git", ".venv", "venv", "node_modules", "__pycache__", "media"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Parse imports instead of scanning raw text.
The guard only searches absolute import forms. A runtime module can use from .seed_data import ... or from . import seed_data and pass this test. The substring scan can also flag comments and docstrings.
Parse ast.Import and ast.ImportFrom nodes, and resolve relative imports from the badges package before asserting that no offenders exist.
Also applies to: 42-58
🤖 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 `@badges/tests/test_seed_data.py` around lines 27 - 29, Update the import guard
test around SEED_DATA_IMPORTERS, IMPORT_FORMS, and its scanning logic to parse
Python files with ast.Import and ast.ImportFrom nodes instead of searching raw
text. Resolve relative imports such as from .seed_data and from . import
seed_data against the badges package, then assert the resolved module is not an
offender while ignoring comments and docstrings.
1 participant
Issue: #2493
Summary & Context
The data model for achievements and badges, plus the single function allowed to derive badge
state from it. Nothing in this PR is user-visible: no admin, no ingestion, no rendering. It is
the layer everything else in the stack stands on.
The rule the whole feature rests on is that
UserAchievementis the only fact andUserBadgeis derived from it.
services.recalculate_badgesis the only writer ofUserBadge, it bothawards and revokes so the count-vs-threshold invariant always holds, and it is idempotent.
Changes
badgesapp:Achievement,UserAchievement,Badge,BadgeTier,UserBadge, with theaudit fields (
granted_by,invalidated_by,revoked_by, notes) and the constraints thatmake repeated ingestion safe - notably
unique_automatic_user_achievement_sourceandunique_active_badgetier_per_rank.badges/enums.py:AchievementSlug,BadgeLabel,TierRank.TierRankis ordered bydeclaration and that order is the primary ladder; thresholds are only the arithmetic behind
one rung, and are not comparable across badges or even reliably within one.
badges/catalogue.py+ migration0002: the eight achievements, eight badges and forty tiersfrom the Boost mapping spreadsheet. A bootstrap fixture, not a live source of truth - a re-seed
never overwrites a threshold staff have tuned, and never resurrects a rank they retired.
badges/services.py:recalculate_badges,achievement_pairs,recalculate_many, the tierlifecycle (
deactivate_tier/reactivate_tier/replace_tier) anddiscard_source_achievements.badges/signals.py: achievement create / invalidate / delete recalculate the one member; atier change goes through
recalculate_achievement_taskbecause it affects everybody.recalculate_badgesmanagement command.users.Badgemodel and itsUser.badgesM2M (migrationusers/0026). It has to be in this PR:UserBadge.related_name="badges"collides with the oldaccessor, and Django's system check fails while both exist.
discard_source_achievements(used by Setup Forum using django-machina #2, the review importer) and thegrant_from_sourcetest helper (used by Setup Forum using django-machina #2 and Version app and models #3). Their docstrings forward-referencebadges.sources, which arrives in Version app and models #3. That is the stack, not a mistake.awarded against it, and
replace_tierexists so retuning a threshold never revokes a memberwho only ever met the old one.
test_grandfathering_threshold_change_does_not_revokeandtest_deactivating_tier_preserves_existing_user_badgespin it. Please do not "fix" it.blocks nothing here, but it must be settled before the first production backfill (PR Option for users to receive an email for forum thread replies quickly #13),
because backfill awards badges against whatever thresholds are live at that moment.
users.Badgehad no rows in production and no reader other than the placeholderUser.badge_url, which this PR leaves alone.Screenshots
A summary of the data model
Peer-review testing steps
Then in a shell (
just manage shell), check the catalogue seeded:You do not call
recalculate_badgesto exercise it. Creating aUserAchievementfires itthrough
post_save, so the test is "make grants, watch badges appear". Paste the whole block:Keep adding grants and the rank climbs. Five things worth poking at:
just manage recalculate_badgestwice changes no rows the second time.awarding it; revalidating brings it back.
thresholdin place makes the holder lose the badge. That is thewhole reason
replace_tierexists and the field is read-only in PR version app, test and black fixes #6.(
test_recalculation_refuses_a_rank_below_one_already_held).achievement_pairsunions grants and badges - a bulkupdate()fires no signal, sobadges survive until a rebuild. Same for a deleted
UserBadgerow: the rebuild restores it.2, 5 and 3 in the shell, in that order - 3 retunes the ladder, so it has to be last. Each block
leaves the state it started with, so 2 and 5 can be re-run freely:
The supported path is
replace_tier(tier), which retires the old rung and adds the replacement soholders keep what they earned -
test_replace_tier_keeps_the_old_tiers_holders.Three things that look like bugs and are not:
recalculate_badgestakes ids, and a stale onecounts zero rather than raising; a tier change needs a running worker (it goes through a task on
commit, because a tier affects everybody); and bulk paths skip the signals by design, which is why
PR #3's engine recalculates explicitly.
The contested decisions are written down as tests rather than prose -
test_recalculation_refuses_a_rank_below_one_already_held,test_grandfathering_threshold_change_does_not_revoke,test_deactivating_tier_preserves_existing_user_badges,test_manual_revocation_survives_recalculation,test_re_earn_after_cascade_revocation_when_count_sufficient,test_achievement_pairs_includes_badge_only_pairs, andtest_recalculation_cost_does_not_grow_with_tiersfor the one performance claim.Not testable here, on purpose: no admin, so nothing can be granted through a UI; no ingestion, so
every grant is one you typed; no display, so the profile keeps its placeholder medal. And
discard_source_achievementsand thegrant_from_sourcetest helper have no callers in this PR -they arrive with #2 and #3.
Self-review Checklist
Backend
makemigrations --checkcleanSummary by CodeRabbit
New Features
Bug Fixes
Tests