diff --git a/badges/admin.py b/badges/admin.py new file mode 100644 index 000000000..b90088e78 --- /dev/null +++ b/badges/admin.py @@ -0,0 +1,1390 @@ +"""Django admin for the achievements and badges system. + +Superusers (and anyone with the relevant per-model permissions) can manage +achievement types, configure badge tiers, manually grant achievements, and +invalidate / revoke with a required audit note. +""" + +from django.contrib import admin, messages +from django.contrib.admin import helpers +from django.contrib.auth import get_user_model +from django.core.exceptions import PermissionDenied, ValidationError +from django.db.models import Count, Prefetch, Q +from django.forms.models import BaseInlineFormSet +from django.http import HttpResponseRedirect +from django.shortcuts import get_object_or_404, render +from django.urls import NoReverseMatch, path, reverse +from django.utils import timezone +from django.utils.html import format_html +from django.utils.text import Truncator + +from badges.enums import TierRank +from badges.forms import NotesActionForm +from badges.models import ( + RANK_LADDER_ORDER, + Achievement, + AchievementSyncRun, + Badge, + BadgeTier, + RevocationSource, + SourceType, + SyncTrigger, + UserAchievement, + UserBadge, + ladder_order_error, +) +from badges.services import ( + achievement_pairs, + deactivate_tier, + reactivate_tier, + recalculate_badges, + recalculate_many, + replace_tier, + sync_source, +) +from badges.sources import AUTOMATIC_SLUGS +from badges.summary import user_badge_summary +from badges.tasks import ( + backfill_achievements_task, + recalculate_all_badges_task, + reconcile_achievements_task, +) +from core.admin_buttons import TaskButton, TaskButtonAdminMixin + +# Derived from the wired iterators, so the options cannot drift from what the +# commands will accept. The slugs match the catalogue's names. +SOURCE_CHOICES = tuple( + (slug, slug.replace("-", " ").title()) for slug in sorted(AUTOMATIC_SLUGS) +) + +RECONCILE_PERMISSION = "badges.delete_userachievement" + +# How many items a message names before it stops and counts the rest. Enough to +# recognise what was skipped, few enough that a whole selection cannot become the +# page. +MESSAGE_ITEM_LIMIT = 3 + +# The same idea for a confirmation page, which has the room for a longer list but +# not for an unbounded one: "select all" on a changelist is the whole table. +CONFIRM_LIST_LIMIT = 10 + +# Wraps the object tools in the left-aligned cluster below the page title. The +# task-button template extends the same one, so a page that grows a button later +# keeps the layout it already had. +ADMIN_ACTIONS_CHANGE_LIST = "admin/admin_actions_change_list.html" + + +def name_a_few(items, limit=MESSAGE_ITEM_LIMIT): + """Name the first few of ``items`` and count the rest. + + A message that interpolates a whole selection grows with the data behind it: + one bronze tier can hold thousands of badges, and naming them all turns a + sentence into a wall of text nobody reads to the end. + """ + items = [str(item) for item in items] + if len(items) <= limit: + return ", ".join(items) + return f"{', '.join(items[:limit])} and {len(items) - limit} more" + + +def reconcile_results(slugs, user_ids=None, dry_run=False, actor=None): + """Sync each named source, skipping any the catalogue has no row for. + + Returns ``(results, unseeded)``. The management command refuses to run at all + on an unseeded slug; an admin page has to be gentler than that, because a page + that raises is worse than one that says the catalogue is incomplete. + """ + achievements = { + achievement.slug: achievement + for achievement in Achievement.objects.filter(slug__in=slugs) + } + results = [ + sync_source( + slug, + achievements[slug], + user_ids=user_ids, + dry_run=dry_run, + trigger=SyncTrigger.ADMIN, + actor=actor, + ) + for slug in sorted(slugs) + if slug in achievements + ] + return results, sorted(set(slugs) - set(achievements)) + + +def reconcile_apply(slugs, user_ids=None, actor=None): + """Sync the named sources for real, then recalculate the members that moved. + + ``sync_source`` recalculates the members it deleted from, chunk by chunk, and + leaves the rest to its caller. ``outstanding`` is that rest: here, the members + who gained a grant, whose ``bulk_create`` sent no signal. + """ + results, _ = reconcile_results(slugs, user_ids=user_ids, actor=actor) + achievements = { + achievement.slug: achievement.pk + for achievement in Achievement.objects.filter( + slug__in=[result.slug for result in results] + ) + } + for result in results: + for user_id in result.outstanding(): + recalculate_badges(user_id, achievements[result.slug]) + return ( + sum(result.added for result in results), + sum(result.removed for result in results if not result.refused), + ) + + +def reconcile_preview(slugs, user_ids=None, scope_label=""): + """Dry-run the named sources and describe what a real run would change. + + Shared by the changelist button and the per-member page so that both show the + same numbers, arrived at the same way. The walk is the expensive part and it + happens in the request: measured at a few seconds against a full copy of the + Boost data, where the commits table is the long pole. + """ + results, unseeded = reconcile_results(slugs, user_ids=user_ids, dry_run=True) + rows = [ + {"label": result.slug, "detail": result.describe(), "warning": result.refused} + for result in results + ] + rows += [ + { + "label": slug, + "detail": "No achievement row: the catalogue is incomplete.", + "warning": True, + } + for slug in unseeded + ] + + added = sum(result.added for result in results) + removed = sum(result.removed for result in results if not result.refused) + refused = [result.slug for result in results if result.refused] + where = scope_label or "every member" + + if added or removed: + changes = [] + if added: + changes.append(f"add {added} automatic achievement(s)") + if removed: + changes.append(f"remove {removed}") + summary = ( + f"This would {' and '.join(changes)} for {where}. Manual grants are not " + "touched. Badges follow in both directions: a tier left below its " + "threshold is revoked as a cascade, and one back above it is re-earned." + ) + elif refused: + summary = f"Nothing can be removed for {where} - see the warning below." + else: + summary = ( + f"Nothing to reconcile for {where}: every automatic achievement already " + "agrees with its source." + ) + + warning = "" + if unseeded: + warning = ( + "Not safe to run: no achievement row for " + f"{', '.join(unseeded)}. Run migrations first." + ) + elif refused: + warning = ( + f"{', '.join(refused)} yielded nothing at all, which is more likely a " + "broken import than a source that is genuinely empty, so those grants " + "are left alone. Use the reconcile_achievements command with " + "--allow-empty if the emptiness is real." + ) + + return { + "title": "Reconcile achievements", + "summary": summary, + "rows": rows, + "warning": warning, + # Nothing to apply is not a decision worth offering, and an incomplete + # catalogue is one the command would refuse anyway. + "can_apply": bool(added or removed) and not unseeded, + } + + +def _reconcile_button_preview(request, slug): + """Dry-run whichever sources the changelist button was pointed at.""" + return reconcile_preview([slug] if slug else AUTOMATIC_SLUGS) + + +BACKFILL_BUTTON = TaskButton( + name="backfill", + label="Backfill achievements", + task=backfill_achievements_task, + success_message="Achievements are being backfilled in the background.", + busy_message="A backfill is already queued or running; not starting another one.", + argument="slug", + choice_label="Source", + choices=SOURCE_CHOICES, + all_label="All sources", + pass_actor=True, + description=( + "Grants the automatic achievements the sources support and this site is " + "missing, then awards any badge that reaches its threshold. It only ever " + "adds, so it is safe to run at any time; this is what runs itself after " + "each release. Use Reconcile instead if an achievement needs removing." + ), +) +RECONCILE_BUTTON = TaskButton( + name="reconcile", + label="Reconcile achievements", + task=reconcile_achievements_task, + success_message=( + "Achievements are being reconciled with their sources in the background." + ), + busy_message=( + "A reconciliation is already queued or running; not starting another one." + ), + argument="slug", + choice_label="Source", + choices=SOURCE_CHOICES, + all_label="All sources", + pass_actor=True, + # This one deletes rows, which the change permission does not cover. + permission=RECONCILE_PERMISSION, + confirm=_reconcile_button_preview, + description=( + "Makes the automatic achievements agree with their sources in both " + "directions: it adds the ones a source now supports and removes the ones it " + "no longer does, such as a commit reassigned to another author or a news " + "post deleted. Badges follow either way. Manually granted achievements are " + "never touched, and you see what would change before anything does." + ), +) +RECALCULATE_BUTTON = TaskButton( + name="recalculate", + label="Recalculate badges", + task=recalculate_all_badges_task, + success_message="Badges are being recalculated in the background.", + busy_message=( + "A recalculation is already queued or running; not starting another one." + ), + description=( + "Rebuilds every member's badges from the achievements already on record: it " + "awards a tier whose threshold is met and revokes one that has fallen below " + "it. No achievement is added, removed or changed, so this is the safe thing " + "to run after editing a badge's thresholds." + ), +) + + +def user_summary_url(user_id): + """The per-user badge page, which three admins link to.""" + return reverse("admin:badges_userbadge_user_summary", args=[user_id]) + + +class BadgeStatusFilter(admin.SimpleListFilter): + """Held or revoked, which is the only question anyone asks of ``revoked_at``. + + Filtering the field directly gives Django's date filter ("Past 7 days", "This + year"), which answers a question nobody has. + """ + + title = "status" + parameter_name = "status" + + def lookups(self, request, model_admin): + """The two states a badge can be in.""" + return (("held", "Held"), ("revoked", "Revoked")) + + def queryset(self, request, queryset): + """Partition on the revocation timestamp.""" + if self.value() == "held": + return queryset.filter(revoked_at__isnull=True) + if self.value() == "revoked": + return queryset.filter(revoked_at__isnull=False) + return queryset + + +@admin.register(Achievement) +class AchievementAdmin(admin.ModelAdmin): + """CRUD admin for achievement types, with the slug frozen after creation. + + The slug is the join key between an ``Achievement`` row and the code that + refers to it (``badges.sources.BACKFILL_ITERATORS``, keyed by + ``AchievementSlug``). Renaming one would silently detach its backfill source + with no error anywhere, so it is only editable on the add form. + """ + + change_list_template = ADMIN_ACTIONS_CHANGE_LIST + list_display = ("name", "slug", "badge", "grants", "created_at") + search_fields = ("name", "slug", "description") + prepopulated_fields = {"slug": ("name",)} + ordering = ("name",) + + def get_queryset(self, request): + """The wiring columns, without a query per row.""" + return ( + super() + .get_queryset(request) + .prefetch_related("badges") + .annotate( + grant_count=Count( + "user_achievements", + filter=Q(user_achievements__is_valid=True), + ) + ) + ) + + @admin.display(description="Badge") + def badge(self, obj): + """The badge this type feeds, if any. + + An achievement with no badge accumulates grants that can never become + anything, which is invisible from anywhere else in the admin. + """ + labels = [badge.get_label_display() for badge in obj.badges.all()] + return ", ".join(labels) if labels else "None - awards nothing" + + @admin.display(description="Valid grants", ordering="grant_count") + def grants(self, obj): + """How many valid grants exist, which is what thresholds count.""" + return obj.grant_count + + def has_delete_permission(self, request, obj=None): + """Never delete an achievement type. + + With badges awarded it is a ``ProtectedError`` dead end anyway. Without + them it cascades: every grant for the type is destroyed, and the wired + backfill source loses the row it needs. Retire the badge's tiers instead. + """ + return False + + def get_readonly_fields(self, request, obj=None): + """Freeze the slug once the achievement exists.""" + if obj is None: + return () + return ("slug",) + + def get_prepopulated_fields(self, request, obj=None): + """Only prepopulate on the add form, where the slug is still editable.""" + if obj is None: + return self.prepopulated_fields + return {} + + +class ActiveBadgeTierInlineFormSet(BaseInlineFormSet): + """Validate the ladder this request is about to save, not the stored one. + + Two things can only be judged across the whole submitted set: a rank claimed by + two rows, and the thresholds' ordering. Shifting every rung up is legal even + though each rung passes through a value that collides with a sibling's stored + threshold, so the per-row model check is handed over here. + """ + + def add_fields(self, form, index): + """Tell each row that this formset owns the ladder ordering check.""" + super().add_fields(form, index) + form.instance.ladder_checked_by_caller = True + + def clean(self): + """Add field errors before the conditional database constraint can fire.""" + super().clean() + if any(self.errors): + return + + seen = set() + submitted = {} + for form in self.forms: + if not form.cleaned_data or form.cleaned_data.get("DELETE"): + continue + rank = form.cleaned_data.get("rank") + if not rank: + continue + if rank in seen: + form.add_error( + "rank", + f"Only one active {TierRank(rank).label} tier is allowed " + "for a badge.", + ) + seen.add(rank) + threshold = form.cleaned_data.get("threshold") + if threshold is not None: + submitted[rank] = threshold + + for form in self.forms: + if not form.cleaned_data or form.cleaned_data.get("DELETE"): + continue + rank = form.cleaned_data.get("rank") + threshold = form.cleaned_data.get("threshold") + if not rank or threshold is None: + continue + error = ladder_order_error( + rank, threshold, {r: t for r, t in submitted.items() if r != rank} + ) + if error: + form.add_error("threshold", error) + + +def _skip_protected_delete_check(form): + """Stand in for Django's ``hand_clean_DELETE`` on the tier inline. + + Bound as a method on the form class the admin builds per request, which is why + it takes the form rather than reading one. + """ + return None + + +class ActiveBadgeTierInline(admin.TabularInline): + """A badge's live ladder, edited in place. + + Tiers are append-only records, so an edit here never updates the row: + ``BadgeAdmin.save_formset`` retires the old tier and creates a replacement, + and removing a row retires it. Both are what preserve the members who + already reached the old threshold - see ``badges.services.replace_tier``. + """ + + model = BadgeTier + formset = ActiveBadgeTierInlineFormSet + fields = ("rank", "threshold") + # One blank row, because the "Add another" link needs JavaScript. Capped at + # the number of ranks, which is also the point at which the constraint on + # (badge, rank) would start rejecting additions. + extra = 1 + max_num = len(TierRank) + verbose_name = "active tier" + verbose_name_plural = "active tiers" + + def get_formset(self, request, obj=None, **kwargs): + """Drop the inline's protected-delete check: nothing here is deleted. + + Django refuses a ticked delete whose row is referenced by a protected + foreign key, and lists every referencing object while it does it. Here that + is ``UserBadge.tier``, so the check refuses exactly the retirements this + page exists for - a tier nobody has earned is the only one it lets + through - and reports it as a message naming every holder. + + ``BadgeAdmin.save_formset`` retires instead of deleting, and + ``BadgeTierAdmin.get_deleted_objects`` is the same removal on the tier's + own page. + """ + formset = super().get_formset(request, obj, **kwargs) + formset.form.hand_clean_DELETE = _skip_protected_delete_check + return formset + + def get_queryset(self, request): + """The live ladder only; retired tiers are linked from the badge form.""" + return ( + super() + .get_queryset(request) + .filter(is_active=True) + .order_by(RANK_LADDER_ORDER) + ) + + +@admin.register(BadgeTier) +class BadgeTierAdmin(admin.ModelAdmin): + """The tier record, kept for history and recovery rather than for tuning. + + Tiers are configured on the badge page. What is left here is what that page + deliberately does not show: the retired rows, and the ``reactivate`` action + that undoes a mistaken retirement. Rows stay immutable - ``rank`` and + ``threshold`` cannot be edited, because updating one in place would revoke + the members who reached the old threshold. + """ + + change_list_template = ADMIN_ACTIONS_CHANGE_LIST + list_display = ("badge", "rank", "threshold", "is_active", "deactivated_at") + list_filter = ("is_active", "rank", "badge") + search_fields = ("badge__label", "rank") + autocomplete_fields = ("badge",) + actions = ["reactivate"] + + def get_ordering(self, request): + """Group by badge, then up the ladder, then oldest threshold first. + + The model's default ordering is by threshold alone, which interleaves + every badge's bronze row. Threshold is also not the ladder on this page in + particular: it is the one that shows retired rows, so a badge that has been + retuned has a bronze at 1 and a bronze at 6 sitting either side of its gold. + + Returned from here rather than set as ``ordering``, because the admin + system check validates that attribute against real model fields and + ``rank_order`` is an annotation. + """ + return ("badge__label", "rank_order", "threshold") + + def get_queryset(self, request): + """Annotate the ladder position, then order on it. + + Not ``super().get_queryset()`` plus an annotation: ``ModelAdmin`` applies + the ordering itself, and ``order_by`` validates a plain name against the + queryset it is handed, so the annotation has to exist first. + """ + return ( + self.model._default_manager.get_queryset() + .annotate(rank_order=RANK_LADDER_ORDER) + .order_by(*self.get_ordering(request)) + ) + + def get_model_perms(self, request): + """Keep this off the index: tiers are configured on the badge page. + + Two entry points for the same thing is the confusion this layer removes. + URLs, the badge page's retired-tier link and the ``reactivate`` action + all keep working; only the index and sidebar listings drop it. + """ + return {} + + def get_readonly_fields(self, request, obj=None): + """Lock rank/threshold once the tier exists; status is always derived.""" + if obj is None: + return ("is_active", "deactivated_at", "deactivated_by") + return ( + "badge", + "rank", + "threshold", + "is_active", + "deactivated_at", + "deactivated_by", + ) + + def get_deleted_objects(self, objs, request): + """Report no cascade - deletion is soft, so nothing is actually removed. + + Without this, the protected ``UserBadge`` references would block the + delete confirmation page before the soft delete can run. + """ + return [str(obj) for obj in objs], {}, set(), [] + + def delete_model(self, request, obj): + """Soft-delete a single tier.""" + deactivate_tier(obj, actor=request.user) + + def delete_queryset(self, request, queryset): + """Soft-delete tiers selected via the bulk delete action.""" + for tier in queryset: + deactivate_tier(tier, actor=request.user) + + @admin.action(description="Reactivate selected retired tiers") + def reactivate(self, request, queryset): + """Undo a retirement, which the change form cannot do. + + A retired tier's form has no editable fields, so without this a mistaken + retirement can only be undone by adding a replacement tier - which leaves + the original's badges behind and duplicates the rank. + """ + reactivated, refused = 0, [] + for tier in queryset.filter(is_active=False): + try: + reactivate_tier(tier) + except ValidationError: + refused.append(str(tier)) + else: + reactivated += 1 + self.message_user(request, f"Reactivated {reactivated} tier(s).") + if refused: + self.message_user( + request, + f"Skipped {len(refused)} tier(s) whose rank already has an active " + f"tier: {name_a_few(refused)}. Retire the replacement first.", + level=messages.WARNING, + ) + + +@admin.register(Badge) +class BadgeAdmin(admin.ModelAdmin): + """The configuration page for a badge: its achievement and its ladder. + + A badge, its description and all five of its tiers are one form and one + save. What is *not* an admin action is inventing a new category: ``label`` + is constrained to ``badges.enums.BadgeLabel`` because the label chooses the + display asset, so an empty-looking dropdown means every category is already + in use, not that something is broken. A genuinely new one needs an enum + member, which is a deploy. + """ + + change_list_template = ADMIN_ACTIONS_CHANGE_LIST + list_display = ("label", "achievement", "ladder", "holders", "source_wired") + list_filter = ("label",) + search_fields = ("label", "achievement__name") + autocomplete_fields = ("achievement",) + inlines = [ActiveBadgeTierInline] + + def get_queryset(self, request): + """Everything the health columns read, without a query per row. + + ``to_attr`` rather than filtering ``tiers`` in place, so the inline's own + queryset is unaffected. + """ + return ( + super() + .get_queryset(request) + .select_related("achievement") + .prefetch_related( + Prefetch( + "tiers", + queryset=BadgeTier.objects.filter(is_active=True).order_by( + RANK_LADDER_ORDER + ), + to_attr="active_tiers", + ) + ) + .annotate( + holder_count=Count( + "user_badges__user", + filter=Q(user_badges__revoked_at__isnull=True), + distinct=True, + ) + ) + ) + + @admin.display(description="Ladder") + def ladder(self, obj): + """The live thresholds, bronze to diamond. + + A badge with no active tiers is the silent misconfiguration: it is + wired, it looks complete, and it can never award anything. + """ + if not obj.active_tiers: + return "No tiers - awards nothing" + return " / ".join(str(tier.threshold) for tier in obj.active_tiers) + + @admin.display(description="Holders", ordering="holder_count") + def holders(self, obj): + """Members currently holding any tier of this badge, counted once each.""" + return obj.holder_count + + @admin.display(boolean=True, description="Automatic") + def source_wired(self, obj): + """Whether a backfill iterator feeds this badge's achievement. + + ``documentation`` and ``mailing-list`` deliberately have none, so they + only ever move on a manual grant. That is worth seeing on the page + rather than knowing. + """ + return obj.achievement.slug in AUTOMATIC_SLUGS + + def save_formset(self, request, form, formset, change): + """Apply the append-only tier rules: an edit replaces, a delete retires. + + ``formset.save(commit=False)`` fills in ``new_objects``, + ``changed_objects`` and ``deleted_objects`` without writing or deleting + anything, which is what lets a removed row become a retirement instead. + The whole request is already wrapped in a transaction by + ``ModelAdmin.changeform_view``. + """ + if formset.model is not BadgeTier: + super().save_formset(request, form, formset, change) + return + + formset.save(commit=False) + for tier in formset.deleted_objects: + deactivate_tier(tier, actor=request.user) + self.message_user( + request, + f"Retired {tier.get_rank_display()} (>= {tier.threshold}). It " + "no longer awards badges; the members who earned it keep it.", + ) + # The inline exposes only rank and threshold, and both are append-only, + # so every changed row is a replacement rather than an update. + for tier, _changed_fields in formset.changed_objects: + retired, replacement = replace_tier(tier, actor=request.user) + self.message_user( + request, + f"Retired {retired.get_rank_display()} (>= {retired.threshold}) " + f"and created {replacement.get_rank_display()} " + f"(>= {replacement.threshold}). Members who already earned " + f"{retired.get_rank_display()} keep it; the new threshold " + "applies from now on.", + ) + for tier in formset.new_objects: + tier.save() + + def has_delete_permission(self, request, obj=None): + """Never delete a badge; retire its tiers instead. + + ``UserBadge.tier`` is protected, so this is a dead end once anything has + been awarded, and a silent cascade over the tiers when it has not. + """ + return False + + def get_fields(self, request, obj=None): + """The retired-tier link needs a badge to scope itself to.""" + fields = ["label", "achievement", "description"] + if obj is not None: + fields.append("retired_tiers") + return fields + + def get_readonly_fields(self, request, obj=None): + """Freeze the achievement once the badge exists. + + Repointing a badge at a different achievement would leave every awarded + ``UserBadge`` derived from a count that no longer feeds it, and nothing + recalculates the members of the achievement it used to track. + """ + if obj is None: + return () + return ("achievement", "retired_tiers") + + @admin.display(description="Retired tiers") + def retired_tiers(self, obj): + """A link out to the history the live ladder deliberately hides. + + A second inline for the retired rows would be the obvious thing, but two + inlines of the same model share a formset prefix and collide. + """ + count = obj.tiers.filter(is_active=False).count() + if not count: + return "None." + url = ( + f"{reverse('admin:badges_badgetier_changelist')}" + f"?is_active__exact=0&badge__id__exact={obj.pk}" + ) + return format_html( + '{} retired tier(s) - kept because members still ' + "hold the badges earned against them.", + url, + count, + ) + + +class NotesActionMixin: + """The confirmation page shared by the actions that require an audit note. + + Invalidating an achievement and revoking a badge are different writes, but + both are "explain yourself first, then apply to the selection", and both used + to restate the same eight-key template context - which is exactly the kind of + pair that drifts. + """ + + def notes_action(self, request, queryset, *, title, action, submit_label, apply): + """Collect a required note, then hand it to ``apply``. + + ``apply(notes)`` does the write and reports its own count, because what + counts as applied differs: one of the two saves row by row so the + ``post_save`` signal runs, the other updates in bulk. + + Returns ``None`` once the note is in and the work is done, which is how an + admin action says "go back to the changelist". + """ + if "apply" in request.POST: + form = NotesActionForm(request.POST) + if form.is_valid(): + apply(form.cleaned_data["notes"]) + return None + else: + form = NotesActionForm() + + opts = self.model._meta + # Only the listing is capped. ``objects`` still carries the whole selection + # into the hidden fields the POST reads it back from, and "select all" on a + # changelist is every row in the table. + listed = list(queryset[:CONFIRM_LIST_LIMIT]) + return render( + request, + "admin/badges/notes_action.html", + { + "title": title, + "objects": queryset, + "listed_objects": listed, + "unlisted_count": max(queryset.count() - len(listed), 0), + "form": form, + "action": action, + "action_checkbox_name": helpers.ACTION_CHECKBOX_NAME, + "opts": opts, + "submit_label": submit_label, + # Named rather than left as "../": the action posts to the + # changelist, so a relative hop lands on the app index instead of + # back where the admin came from. + "cancel_url": reverse( + f"admin:{opts.app_label}_{opts.model_name}_changelist" + ), + }, + ) + + +class MemberSummaryLinkMixin: + """A ``user`` column leading to the per-user badge page. + + On both of these changelists the question about a row is almost always a + question about the member rather than the row. The row itself stays reachable + through the first column, which is what the changelist links by default. + """ + + @admin.display(description="User", ordering="user__email") + def user_link(self, obj): + """The member, linking to why they hold what they hold.""" + return format_html( + '{}', user_summary_url(obj.user_id), obj.user + ) + + +@admin.register(UserAchievement) +class UserAchievementAdmin( + MemberSummaryLinkMixin, NotesActionMixin, TaskButtonAdminMixin, admin.ModelAdmin +): + """Admin for per-user achievement grants. + + Manual creation auto-populates ``source_type`` and ``granted_by``, and requires + a note. An existing row is then a record: the ``invalidate`` and ``revalidate`` + actions are the only way to change its state, both of which recalculate badges + through the ``post_save`` signal. ``invalidate`` collects a required audit note + of its own. + """ + + task_buttons = (BACKFILL_BUTTON, RECONCILE_BUTTON) + list_display = ( + "achievement", + "user_link", + "source_type", + "source_link", + "grant_note", + "is_valid", + "created_at", + ) + list_filter = ("is_valid", "source_type", "achievement") + list_select_related = ("achievement", "user") + search_fields = ( + "user__email", + "user__display_name", + "achievement__name", + "grant_notes", + ) + autocomplete_fields = ("achievement", "user") + readonly_fields = ( + "created_at", + "invalidated_by", + "invalidated_at", + "granted_by", + ) + actions = ["invalidate", "revalidate"] + add_fieldsets = ((None, {"fields": ("user", "achievement", "grant_notes")}),) + + def has_delete_permission(self, request, obj=None): + """Invalidation is a soft delete on purpose; keep the audit trail.""" + return False + + def get_queryset(self, request): + """Prefetch the generic source so the column costs one query per type.""" + return super().get_queryset(request).prefetch_related("source") + + @admin.display(description="Source") + def source_link(self, obj): + """The row that justified an automatic grant. + + Without this, 138 commit grants are 138 identical lines and there is no + way to see what any of them came from. Not every source model is + registered in the admin - ``news.Entry`` is not - so an unreachable one + falls back to its own label. + """ + source = obj.source + if source is None: + return "-" + meta = source._meta + try: + url = reverse( + f"admin:{meta.app_label}_{meta.model_name}_change", args=[source.pk] + ) + except NoReverseMatch: + return str(source) + return format_html('{}', url, source) + + @admin.display(description="Note") + def grant_note(self, obj): + """The reason a manual grant was given, truncated to stay scannable. + + Sits beside ``source_link`` because the two answer the same question from + opposite ends: an automatic grant is explained by the row it came from, a + manual one only by whoever typed it. Truncated rather than omitted, because + the alternative is opening every row to find out why it exists. + """ + return Truncator(obj.grant_notes).chars(60) or "-" + + def get_form(self, request, obj=None, **kwargs): + """Require the note on a manual grant, and only there. + + Not ``blank=False`` on the model: that would also bind the automatic rows, + which are created by ``bulk_create`` (so unvalidated anyway) and already + have a source record explaining them. Mutating ``base_fields`` is safe + because ``modelform_factory`` builds a new class per call. + """ + form = super().get_form(request, obj, **kwargs) + if obj is None: + form.base_fields["grant_notes"].required = True + return form + + def get_fieldsets(self, request, obj=None): + """Collect only what a manual grant means. + + ``save_model`` forces ``source_type`` and ``granted_by``, and the generic + foreign key belongs to automatic grants: a manual row pointing at a source + is not covered by the uniqueness constraint, so the same source would + count twice. + """ + if obj is None: + return self.add_fieldsets + return super().get_fieldsets(request, obj) + + def get_readonly_fields(self, request, obj=None): + """Make an existing grant a read-only record. + + Moving a grant to another user or achievement recalculates only the pair + it moved *to*, so the pair it left keeps a badge nothing will revoke. + State changes go through the ``invalidate`` action instead, which records + who did it and why. + + ``grant_notes`` is the one field deliberately left editable: correcting the + wording of a reason changes no badge state, and the rule above exists to + protect badge state rather than to freeze the row for its own sake. + """ + if obj is None: + return self.readonly_fields + return self.readonly_fields + ( + "user", + "achievement", + "is_valid", + "invalidation_notes", + "source_type", + "source_content_type", + "source_object_id", + ) + + def save_model(self, request, obj, form, change): + """Mark admin-created grants as manual and record the granting admin.""" + if not change: + obj.source_type = SourceType.MANUAL + obj.granted_by = request.user + super().save_model(request, obj, form, change) + + @admin.action(description="Invalidate selected achievements (with note)") + def invalidate(self, request, queryset): + """Soft-invalidate achievements after collecting a required note.""" + # Narrowed before the confirmation page too, so it never lists rows the + # action would skip and then report having invalidated nothing. + queryset = queryset.filter(is_valid=True) + if not queryset.exists(): + self.message_user( + request, + "Nothing to invalidate: every selected achievement is already " + "invalid.", + level=messages.WARNING, + ) + return None + + def apply(notes): + """Save row by row, so the ``post_save`` signal revokes the badges.""" + count = 0 + for achievement in queryset: + achievement.is_valid = False + achievement.invalidated_by = request.user + achievement.invalidated_at = timezone.now() + achievement.invalidation_notes = notes + achievement.save() + count += 1 + self.message_user(request, f"Invalidated {count} achievement(s).") + + return self.notes_action( + request, + queryset, + title="Invalidate achievements", + action="invalidate", + submit_label="Invalidate", + apply=apply, + ) + + @admin.action(description="Revalidate selected achievements") + def revalidate(self, request, queryset): + """Undo an invalidation, clearing its audit trail. + + The counterpart to ``invalidate``, and the reason the change form does not + expose ``is_valid``: flipping it there would leave the row reading + "invalidated by X" while counting toward a threshold again. Saves one row + at a time so the post_save signal re-awards the badges. + """ + count = 0 + for achievement in queryset.filter(is_valid=False): + achievement.is_valid = True + achievement.invalidated_by = None + achievement.invalidated_at = None + achievement.invalidation_notes = "" + achievement.save() + count += 1 + self.message_user(request, f"Revalidated {count} achievement(s).") + + +@admin.register(UserBadge) +class UserBadgeAdmin( + MemberSummaryLinkMixin, NotesActionMixin, TaskButtonAdminMixin, admin.ModelAdmin +): + """Read-only admin for derived badge state. + + Badges are awarded and revoked by the recalculation service, so rows are + neither added nor edited here. The ``revoke`` action lets an admin revoke one + after collecting a required note; it does not touch any ``UserAchievement`` + records. Manual revocations are never re-earned by recalculation - use the + ``reinstate`` action to undo one. + """ + + task_buttons = (RECALCULATE_BUTTON,) + list_display = ( + "badge", + "user_link", + "tier", + "is_held", + "hidden_by_member", + "awarded_at", + "revoked_at", + ) + list_filter = ("badge", "tier__rank", BadgeStatusFilter, "revocation_source") + list_select_related = ("badge", "user", "tier") + search_fields = ("user__email", "user__display_name", "badge__label") + readonly_fields = ( + "badge", + "user", + "tier", + "awarded_at", + "revoked_by", + "revoked_at", + "revocation_source", + "revocation_notes", + "count_at_revocation", + ) + actions = ["revoke", "reinstate"] + + def get_urls(self): + """Register the per-user page ahead of the ``/`` catch-all.""" + summary = [ + path( + "user-summary//", + self.admin_site.admin_view(self.user_summary_view), + name="badges_userbadge_user_summary", + ) + ] + return summary + super().get_urls() + + def user_summary_view(self, request, user_id): + """Why one member does or does not show each badge. + + The four causes of a missing badge live in three changelists otherwise, + two of them only as arithmetic against a threshold. Every url the page + renders is built here rather than in the template. + + ``admin_site.admin_view`` only asks whether the caller is staff. The page + reads awarded badges *and* achievement grants, so it needs view + permission on both. + """ + if not ( + self.has_view_permission(request) + and request.user.has_perm("badges.view_userachievement") + ): + raise PermissionDenied + + member = get_object_or_404(get_user_model(), pk=user_id) + if request.method == "POST": + if request.POST.get("action") == "reconcile": + return self._reconcile_member(request, member) + # Anything else is the recalculate form. Defaulting to it is safe + # because it is the idempotent one: an unrecognised action costs a + # recalculation, not a deletion. + return self._recalculate_member(request, member) + + grants_changelist = reverse("admin:badges_userachievement_changelist") + rows = [ + { + "row": row, + "grants_url": ( + f"{grants_changelist}?user__id__exact={member.pk}" + f"&achievement__id__exact={row.achievement.pk}" + ), + } + for row in user_badge_summary(member) + ] + context = { + **self.admin_site.each_context(request), + "title": f"Badges for {member}", + "member": member, + "rows": rows, + "opts": self.model._meta, + "index_url": reverse("admin:index"), + "changelist_url": reverse("admin:badges_userbadge_changelist"), + "member_admin_url": self._member_admin_url(request, member), + "recalculate_url": user_summary_url(member.pk), + "can_recalculate": self.has_change_permission(request), + "reconcile_url": user_summary_url(member.pk), + "can_reconcile": request.user.has_perm(RECONCILE_PERMISSION), + "grant_url": ( + f"{reverse('admin:badges_userachievement_add')}?user={member.pk}" + ), + "can_grant": request.user.has_perm("badges.add_userachievement"), + } + return render(request, "admin/badges/user_summary.html", context) + + def _member_admin_url(self, request, member): + """This member's own admin page, for the questions this page cannot answer. + + Support arrives here from a badge and often leaves needing the account: + the email to reply to, whether it is active, what else they have. Empty + rather than offered when the caller could not open it, on the same + principle as the task buttons - a control nobody can use is not a control. + + Read off the user model's own meta rather than hardcoded, and tolerant of + an unregistered one, exactly as ``UserAchievementAdmin.source_link`` is. + """ + meta = get_user_model()._meta + allowed = request.user.has_perm( + f"{meta.app_label}.view_{meta.model_name}" + ) or request.user.has_perm(f"{meta.app_label}.change_{meta.model_name}") + if not allowed: + return "" + try: + return reverse( + f"admin:{meta.app_label}_{meta.model_name}_change", args=[member.pk] + ) + except NoReverseMatch: + return "" + + def _recalculate_member(self, request, member): + """Reconcile every one of this member's badges, synchronously. + + One member is at most a handful of achievement types at five queries + each, so a Celery task would buy nothing and cost the admin the ability + to see the result on the page they are already looking at. + """ + if not self.has_change_permission(request): + raise PermissionDenied + count = recalculate_many(achievement_pairs(user_ids=[member.pk])) + self.message_user( + request, f"Recalculated {count} achievement type(s) for this member." + ) + return HttpResponseRedirect(user_summary_url(member.pk)) + + def _reconcile_member(self, request, member): + """Preview, then on a second POST apply, this member's source disagreements. + + Synchronous for the same reason ``_recalculate_member`` is - the admin + wants the outcome on the page they are already looking at - and the choice + costs less than it looks: walking every source is the price whether the run + is scoped to one member or not, and it is the same walk the preview just + did. + + It can delete, so it is gated on ``delete_userachievement`` rather than on + the change permission that guards the rest of this page. + """ + if not request.user.has_perm(RECONCILE_PERMISSION): + raise PermissionDenied + + if "apply" not in request.POST: + return render( + request, + "admin/dry_run_confirm.html", + { + **self.admin_site.each_context(request), + "opts": self.model._meta, + "title": f"Reconcile achievements for {member}", + "preview": reconcile_preview( + AUTOMATIC_SLUGS, + user_ids=[member.pk], + scope_label=str(member), + ), + "form_action": user_summary_url(member.pk), + "hidden_fields": [{"name": "action", "value": "reconcile"}], + "submit_label": "Reconcile this member", + "cancel_url": user_summary_url(member.pk), + }, + ) + + added, removed = reconcile_apply( + AUTOMATIC_SLUGS, user_ids=[member.pk], actor=request.user + ) + self.message_user( + request, + f"Reconciled this member with their sources: added {added} and removed " + f"{removed} automatic achievement(s).", + ) + return HttpResponseRedirect(user_summary_url(member.pk)) + + def has_add_permission(self, request): + """Badges are derived. Grant the achievement behind one instead. + + A hand-made row has no achievements supporting it, so the next + recalculation cascade-revokes it and the badge silently disappears. + """ + return False + + def has_delete_permission(self, request, obj=None): + """Revocation is a soft delete on purpose; keep the audit trail.""" + return False + + @admin.display(boolean=True, description="Held", ordering="revoked_at") + def is_held(self, obj): + """Whether the member currently holds this badge.""" + return obj.is_active + + @admin.display(boolean=True, description="Hidden", ordering="user__hide_badges") + def hidden_by_member(self, obj): + """Whether the member has turned badge display off on their profile. + + Two of the reasons a badge does not appear are visible here; the other two + - manual and cascade revocation - are the ``revocation_source`` column. + """ + return obj.user.hide_badges + + @admin.action(description="Revoke selected badges (with note)") + def revoke(self, request, queryset): + """Directly revoke badges after collecting a required note.""" + # Narrowed before the confirmation page, like ``invalidate``, so it never + # lists a badge it would skip and then report having revoked nothing. + queryset = queryset.filter(revoked_at__isnull=True) + if not queryset.exists(): + self.message_user( + request, + "Nothing to revoke: every selected badge is already revoked.", + level=messages.WARNING, + ) + return None + + def apply(notes): + """One update: ``UserBadge`` has no signals to run per row.""" + count = queryset.update( + revoked_at=timezone.now(), + revoked_by=request.user, + revocation_notes=notes, + revocation_source=RevocationSource.MANUAL, + ) + self.message_user(request, f"Revoked {count} badge(s).") + + return self.notes_action( + request, + queryset, + title="Revoke badges", + action="revoke", + submit_label="Revoke", + apply=apply, + ) + + @admin.action(description="Reinstate selected manually revoked badges") + def reinstate(self, request, queryset): + """Clear revocation on manually revoked badges, undoing a revoke action. + + Cascade revocations are skipped: they mean the achievement count is + below the tier threshold, so reinstating one would award a badge the + user has not earned. Nothing would take it away again either - per-pair + recalculation only runs when a ``UserAchievement`` changes, and the + achievement behind a cascade revocation is already invalid. + """ + eligible = queryset.filter( + revoked_at__isnull=False, revocation_source=RevocationSource.MANUAL + ) + skipped = queryset.filter(revoked_at__isnull=False).count() - eligible.count() + count = eligible.update( + revoked_at=None, + revoked_by=None, + revocation_notes="", + revocation_source="", + count_at_revocation=None, + ) + self.message_user(request, f"Reinstated {count} badge(s).") + if skipped: + self.message_user( + request, + f"Skipped {skipped} cascade-revoked badge(s): their achievement " + "count is below the tier threshold. Grant or revalidate the " + "achievements instead.", + level=messages.WARNING, + ) + + +class AppliedFilter(admin.SimpleListFilter): + """The refusal guard, filtered in the same words the column reads in. + + Over ``refused`` itself, so the sidebar does not ask about the state the table + stopped naming. + """ + + title = "applied" + parameter_name = "applied" + + def lookups(self, request, model_admin): + """Yes and no, phrased as answers to "did this run apply its changes".""" + return (("yes", "Yes"), ("no", "No - refused")) + + def queryset(self, request, queryset): + """Map the answer back onto the stored ``refused`` flag.""" + if self.value() == "yes": + return queryset.filter(refused=False) + if self.value() == "no": + return queryset.filter(refused=True) + return queryset + + +@admin.register(AchievementSyncRun) +class AchievementSyncRunAdmin(admin.ModelAdmin): + """Read-only history of backfill and reconcile runs. + + This is what a cascade revocation note points at. When a member asks where + their badge went, the run named in that note says what changed the count, when, + and whether a person or the weekly pipeline started it. + + Both flag columns read the way the admin's icons do, which is the opposite of + the way the fields behind them are stored: a green tick is a run that went + well. Stored as the exceptional case (``refused``, ``error``) because that is + what the sync writes and what a log is for; shown as the normal one, because a + table of red crosses against runs that all went fine trains an admin to ignore + the column that matters. + """ + + list_display = ( + "id", + "source_slug", + "mode", + "trigger", + "triggered_by", + "started_at", + "added", + "removed", + "members_changed", + "applied", + "succeeded", + ) + list_filter = ("mode", "trigger", AppliedFilter, "source_slug") + list_select_related = ("triggered_by",) + search_fields = ("source_slug", "triggered_by__email") + date_hierarchy = "started_at" + readonly_fields = tuple( + field.name for field in AchievementSyncRun._meta.fields if field.name != "id" + ) + + @admin.display(boolean=True, description="Succeeded", ordering="error") + def succeeded(self, obj): + """Whether the run finished, which its counts alone cannot say. + + Three states rather than two, because "not failed" covers both a run that + went well and one still going: ``None`` renders as the admin's grey + question mark, and a run in flight has no error yet only because it has not + reached the end. A crashed reconcile has already removed the grants it got + through, so the cross is what separates "nothing to do" from "stopped + early, re-run it". + """ + if obj.finished_at is None: + return None + return not obj.error + + @admin.display(boolean=True, description="Applied", ordering="-refused") + def applied(self, obj): + """Whether the run acted, or declined to because its source looked broken. + + A cross is the safety guard having fired: the source yielded nothing at all + while stale grants existed, which is indistinguishable from a failed import + and would otherwise revoke every badge that source feeds, so nothing was + removed. Only reachable for a reconcile - a backfill removes nothing and + therefore has nothing to refuse. + """ + return not obj.refused + + def has_add_permission(self, request): + """Runs are recorded by the sync itself, never entered by hand.""" + return False + + def has_change_permission(self, request, obj=None): + """A run is history; editing one would defeat the point of keeping it.""" + return False + + def has_delete_permission(self, request, obj=None): + """Revocation notes point at these rows, so deleting one orphans a note.""" + return False diff --git a/badges/forms.py b/badges/forms.py new file mode 100644 index 000000000..b18688d70 --- /dev/null +++ b/badges/forms.py @@ -0,0 +1,20 @@ +"""Forms for the badges admin.""" + +from django import forms +from django.utils.translation import gettext_lazy as _ + + +class NotesActionForm(forms.Form): + """Intermediate form for admin actions that require an audit note. + + Used by the ``UserAchievement`` invalidation and ``UserBadge`` revocation + actions, both of which require a non-empty note explaining the change. + """ + + notes = forms.CharField( + label=_("Notes"), + widget=forms.Textarea(attrs={"rows": 4, "cols": 60}), + required=True, + strip=True, + help_text=_("Required. Explain why this action is being taken."), + ) diff --git a/badges/management/arguments.py b/badges/management/arguments.py new file mode 100644 index 000000000..78d97b519 --- /dev/null +++ b/badges/management/arguments.py @@ -0,0 +1,68 @@ +"""Shared arguments for the badge management commands.""" + +from argparse import ArgumentTypeError + +from django.contrib.auth import get_user_model + +from badges.models import SyncTrigger + + +def positive_integer(value): + """Parse a strictly positive integer for ``argparse``.""" + parsed = int(value) + if parsed <= 0: + raise ArgumentTypeError("must be a positive integer") + return parsed + + +def add_sync_log_arguments(parser): + """Register the options describing how a run was started. + + Both are for the sync log, and both are how a caller that is not a person at a + shell says so: the release pipeline sets the trigger, and a changelist button + names the admin behind it. + """ + parser.add_argument( + "--trigger", + choices=SyncTrigger.values, + default=None, + help=( + "How this run was started, recorded in the sync log (default: admin " + "when --triggered-by names somebody, otherwise command)." + ), + ) + parser.add_argument( + "--triggered-by", + dest="actor_id", + type=positive_integer, + metavar="USER_ID", + help="Primary key of the person who started this run, for the sync log.", + ) + + +def resolve_sync_log(options, stderr): + """``(trigger, actor)`` describing where this run came from. + + A named person means somebody pressed a button, which is why a caller passing + ``--triggered-by`` need not also state the trigger. An explicit one still wins, + so the release pipeline can label its own sweep. + + An id that resolves to nobody is reported and otherwise ignored: attribution is + worth less than the run itself, so a member deleted between a button press and + the worker collecting the job costs the log a name, not the sweep. The trigger + still says a person started it, because one did. + """ + actor_id = options["actor_id"] + trigger = options["trigger"] or ( + SyncTrigger.ADMIN if actor_id else SyncTrigger.COMMAND + ) + if actor_id is None: + return trigger, None + + actor = get_user_model().objects.filter(pk=actor_id).first() + if actor is None: + stderr.write( + f"No member with id {actor_id}: the sync log will not record who " + "started this run." + ) + return trigger, actor diff --git a/badges/management/commands/backfill_achievements.py b/badges/management/commands/backfill_achievements.py new file mode 100644 index 000000000..629301a1d --- /dev/null +++ b/badges/management/commands/backfill_achievements.py @@ -0,0 +1,106 @@ +"""Backfill automatic achievements from existing Boost data. + +Walks each wired source and creates the grants it yields that the database is +missing, then recalculates badges once per affected (user, achievement) pair. + +**Additive only.** This is ``services.sync_source`` with ``remove=False``, so it +can never undo an attribution, which is what makes it safe for the weekly +pipeline to run unattended. To remove the grants a source has stopped supporting, +use ``reconcile_achievements``. +""" + +from django.core.management.base import BaseCommand, CommandError + +from badges import sources +from badges.management.arguments import ( + add_sync_log_arguments, + positive_integer, + resolve_sync_log, +) +from badges.models import Achievement +from badges.services import SYNC_BATCH_SIZE, recalculate_badges, sync_source + + +class Command(BaseCommand): + """Create automatic UserAchievement rows from historical data.""" + + help = "Backfill automatic achievements from existing data." + + def add_arguments(self, parser): + """Register CLI options.""" + parser.add_argument( + "--source", + dest="slugs", + action="append", + choices=sources.AUTOMATIC_SLUGS, + help="Backfill only this source slug; repeat for several (default: all).", + ) + parser.add_argument( + "--batch-size", + type=positive_integer, + default=SYNC_BATCH_SIZE, + help=f"Rows per bulk_create batch (default: {SYNC_BATCH_SIZE}).", + ) + add_sync_log_arguments(parser) + + def handle(self, *args, **options): + """Run the backfill for the requested source(s).""" + explicit = bool(options["slugs"]) + slugs = options["slugs"] or sources.AUTOMATIC_SLUGS + batch_size = options["batch_size"] + trigger, actor = resolve_sync_log(options, self.stderr) + + achievements = { + achievement.slug: achievement + for achievement in Achievement.objects.filter(slug__in=slugs) + } + missing = sorted(set(slugs) - set(achievements)) + if missing: + # A named source that is missing is a deploy bug, so fail on it. A + # scheduled sweep instead reports it and keeps going: one unseeded + # slug must not cost the other five sources their backfill. + if explicit: + raise CommandError( + "No Achievement row for wired source(s): " + f"{', '.join(missing)}. Run migrations to seed the catalogue." + ) + self.stderr.write( + "Skipping wired source(s) with no Achievement row: " + f"{', '.join(missing)}. Run migrations to seed the catalogue." + ) + unseeded = set(missing) + slugs = [slug for slug in slugs if slug not in unseeded] + if not slugs: + raise CommandError("No wired source has an Achievement row.") + + dirty_pairs = set() + for slug in slugs: + achievement = achievements[slug] + result = sync_source( + slug, + achievement, + remove=False, + batch_size=batch_size, + trigger=trigger, + actor=actor, + ) + # Only the members who actually gained a row, so a repeat run - the + # weekly one - does not recalculate every pair in the system. + # ``outstanding`` is every one of them here, this run being additive, + # but reading it from the result means no caller has to know that. + dirty_pairs.update( + (user_id, achievement.pk) for user_id in result.outstanding() + ) + # ``describe()`` rather than a line of its own, so this and the + # reconcile command and the admin's preview cannot reach different + # conclusions about the same numbers. It can never report a removal + # here: ``remove=False`` leaves the stale set empty. + self.stdout.write(f" {result.slug}: {result.describe()}") + + for user_id, achievement_id in dirty_pairs: + recalculate_badges(user_id, achievement_id) + self.stdout.write( + self.style.SUCCESS( + f"Done. Recalculated {len(dirty_pairs)} (user, achievement) pair(s)." + ) + ) diff --git a/badges/management/commands/reconcile_achievements.py b/badges/management/commands/reconcile_achievements.py new file mode 100644 index 000000000..ecbcc17aa --- /dev/null +++ b/badges/management/commands/reconcile_achievements.py @@ -0,0 +1,188 @@ +"""Make automatic achievements agree with the sources they came from. + +Two-way, unlike ``backfill_achievements``, which only ever adds. One walk of each +source creates the grants it yields that are missing and deletes the stored grants +it no longer yields - a commit re-assigned to another author, a maintainer dropped +from a library, a news entry unpublished. Manual grants are never touched, and +badges follow in both directions: a tier below its threshold is cascade-revoked, and +one back above it is re-earned. + +Scope it and rehearse it: ``--dry-run`` reports without writing, ``--user`` and +``--source`` keep the blast radius to what you meant to fix, and ``--remove-only`` +leaves the additive half out when removal is all you want. +""" + +from django.contrib.auth import get_user_model +from django.core.management.base import BaseCommand, CommandError + +from badges import sources +from badges.management.arguments import ( + add_sync_log_arguments, + positive_integer, + resolve_sync_log, +) +from badges.models import Achievement +from badges.services import SYNC_BATCH_SIZE, recalculate_badges, sync_source + +User = get_user_model() + + +class Command(BaseCommand): + """Sync automatic UserAchievement rows against the sources they derive from.""" + + help = "Add and remove automatic achievements so they match their sources." + + def add_arguments(self, parser): + """Register CLI options.""" + parser.add_argument( + "--source", + dest="slugs", + action="append", + choices=sources.AUTOMATIC_SLUGS, + help="Reconcile only this source slug; repeat for several (default: all).", + ) + parser.add_argument( + "--user", + dest="users", + action="append", + metavar="EMAIL_OR_ID", + help=( + "Restrict the run to this member, by email or primary key; " + "repeat for several (default: every member)." + ), + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Report what would change and write nothing.", + ) + parser.add_argument( + "--remove-only", + action="store_true", + help=( + "Delete stale grants without creating missing ones, which is what " + "this command did before it was two-way." + ), + ) + parser.add_argument( + "--allow-empty", + action="store_true", + help=( + "Delete even when a source yields no rows at all. Without this, an " + "empty source is treated as broken rather than as evidence that " + "every grant it feeds is stale." + ), + ) + parser.add_argument( + "--batch-size", + type=positive_integer, + default=SYNC_BATCH_SIZE, + help=f"Rows per insert and delete batch (default: {SYNC_BATCH_SIZE}).", + ) + add_sync_log_arguments(parser) + + def handle(self, *args, **options): + """Sync the requested source(s) and report what changed.""" + dry_run = options["dry_run"] + slugs = options["slugs"] or sources.AUTOMATIC_SLUGS + user_ids = self._resolve_users(options["users"]) + trigger, actor = resolve_sync_log(options, self.stderr) + + achievements = { + achievement.slug: achievement + for achievement in Achievement.objects.filter(slug__in=slugs) + } + missing = sorted(set(slugs) - set(achievements)) + if missing: + # Unlike the backfill, an unseeded slug is never merely skipped: it + # has no stored grants either, so there is nothing to reconcile and + # nothing to lose by insisting the catalogue is intact first. + raise CommandError( + "No Achievement row for wired source(s): " + f"{', '.join(missing)}. Run migrations to seed the catalogue." + ) + + if dry_run: + self.stdout.write("Dry run: nothing will be written.") + + results = [ + sync_source( + slug, + achievements[slug], + user_ids=user_ids, + add=not options["remove_only"], + dry_run=dry_run, + allow_empty=options["allow_empty"], + batch_size=options["batch_size"], + trigger=trigger, + actor=actor, + ) + for slug in slugs + ] + + # Two sets, because they answer different questions. ``dirty_pairs`` is + # what moved, which is what the summary line reports. ``pending_pairs`` is + # what still needs recalculating: the run already did that for every member + # it deleted from, chunk by chunk, and repeating them would be another pass + # per member for the same answer. + dirty_pairs = set() + pending_pairs = set() + for result in results: + self.stdout.write(f" {result.slug}: {result.describe()}") + achievement_id = achievements[result.slug].pk + dirty_pairs.update((user_id, achievement_id) for user_id in result.changed) + pending_pairs.update( + (user_id, achievement_id) for user_id in result.outstanding() + ) + + if not dry_run: + for user_id, achievement_id in pending_pairs: + recalculate_badges(user_id, achievement_id) + + added = sum(result.added for result in results) + removed = sum(result.removed for result in results if not result.refused) + lead = "Would add" if dry_run else "Added" + tail = "remove" if dry_run else "removed" + self.stdout.write( + self.style.SUCCESS( + f"Done. {lead} {added} and {tail} {removed} grant(s) across " + f"{len(dirty_pairs)} (user, achievement) pair(s)." + ) + ) + refused = [result.slug for result in results if result.refused] + if refused: + # Loud, and on stderr: a refusal means a source read empty, which is + # a data problem outliving this command. + self.stderr.write( + "Refused to remove anything for: " + f"{', '.join(refused)}. The source(s) yielded nothing while grants " + "exist. Investigate the source, or pass --allow-empty if the " + "emptiness is real." + ) + + def _resolve_users(self, identifiers): + """Turn ``--user`` values into primary keys, or fail listing the strays. + + Accepts an email or a primary key because both are how a member gets + named in practice - an email from a bug report, an id from an admin URL. + """ + if not identifiers: + return None + + user_ids = set() + unknown = [] + for identifier in identifiers: + lookup = ( + {"pk": int(identifier)} + if identifier.isdigit() + else {"email__iexact": identifier} + ) + pk = User.objects.filter(**lookup).values_list("pk", flat=True).first() + if pk is None: + unknown.append(identifier) + else: + user_ids.add(pk) + + if unknown: + raise CommandError(f"No such member(s): {', '.join(unknown)}.") + return user_ids diff --git a/badges/migrations/0003_achievementsyncrun.py b/badges/migrations/0003_achievementsyncrun.py new file mode 100644 index 000000000..6c2560eb7 --- /dev/null +++ b/badges/migrations/0003_achievementsyncrun.py @@ -0,0 +1,115 @@ +# Generated by Django 6.0.2 on 2026-08-03 20:44 + +import django.db.models.deletion +import django.utils.timezone +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("badges", "0002_seed_achievements_and_badges"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name="AchievementSyncRun", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "source_slug", + models.CharField(max_length=255, verbose_name="source slug"), + ), + ( + "mode", + models.CharField( + choices=[("backfill", "Backfill"), ("reconcile", "Reconcile")], + max_length=20, + verbose_name="mode", + ), + ), + ( + "trigger", + models.CharField( + choices=[ + ("command", "Command"), + ("admin", "Admin"), + ("pipeline", "Release pipeline"), + ], + default="command", + max_length=20, + verbose_name="trigger", + ), + ), + ( + "started_at", + models.DateTimeField( + default=django.utils.timezone.now, verbose_name="started at" + ), + ), + ( + "finished_at", + models.DateTimeField( + blank=True, null=True, verbose_name="finished at" + ), + ), + ( + "added", + models.PositiveIntegerField(default=0, verbose_name="grants added"), + ), + ( + "removed", + models.PositiveIntegerField( + default=0, verbose_name="grants removed" + ), + ), + ( + "members_changed", + models.PositiveIntegerField( + default=0, verbose_name="members changed" + ), + ), + ( + "refused", + models.BooleanField( + default=False, + help_text="The source yielded nothing, so stale grants were left alone.", + verbose_name="refused", + ), + ), + ( + "error", + models.TextField( + blank=True, + help_text="What the run raised, where it did not finish. Deletions are chunked rather than wrapped in one transaction, so a run that died part way left the grants it had already removed removed.", + verbose_name="error", + ), + ), + ( + "triggered_by", + models.ForeignKey( + blank=True, + help_text="The admin who started this run, where a person started it.", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="+", + to=settings.AUTH_USER_MODEL, + ), + ), + ], + options={ + "verbose_name": "achievement sync run", + "ordering": ("-started_at",), + }, + ), + ] diff --git a/badges/models.py b/badges/models.py index 0a5e49426..b61397563 100644 --- a/badges/models.py +++ b/badges/models.py @@ -376,3 +376,75 @@ def __str__(self): def is_active(self): """Whether this badge is currently held (not revoked).""" return self.revoked_at is None + + +class SyncMode(models.TextChoices): + """Which half of the sync a run was allowed to do.""" + + BACKFILL = "backfill", _("Backfill") + RECONCILE = "reconcile", _("Reconcile") + + +class SyncTrigger(models.TextChoices): + """What started a sync run.""" + + COMMAND = "command", _("Command") + ADMIN = "admin", _("Admin") + PIPELINE = "pipeline", _("Release pipeline") + + +class AchievementSyncRun(models.Model): + """One row per source per backfill or reconcile run. + + A cascade revocation can say that a member's count fell below a threshold, but + not what moved the count. This is the record it names: what ran, when, how it + was started, and how many grants it added or removed. Without it, a member + losing a badge after an upstream data correction is unexplainable. + + Dry runs are not recorded. A preview writes nothing, and the reconcile + confirmation page previews every source each time it is opened. + """ + + source_slug = models.CharField(_("source slug"), max_length=255) + mode = models.CharField(_("mode"), max_length=20, choices=SyncMode) + trigger = models.CharField( + _("trigger"), + max_length=20, + choices=SyncTrigger, + default=SyncTrigger.COMMAND, + ) + triggered_by = models.ForeignKey( + settings.AUTH_USER_MODEL, + on_delete=models.SET_NULL, + null=True, + blank=True, + related_name="+", + help_text=_("The admin who started this run, where a person started it."), + ) + started_at = models.DateTimeField(_("started at"), default=timezone.now) + finished_at = models.DateTimeField(_("finished at"), null=True, blank=True) + added = models.PositiveIntegerField(_("grants added"), default=0) + removed = models.PositiveIntegerField(_("grants removed"), default=0) + members_changed = models.PositiveIntegerField(_("members changed"), default=0) + refused = models.BooleanField( + _("refused"), + default=False, + help_text=_("The source yielded nothing, so stale grants were left alone."), + ) + error = models.TextField( + _("error"), + blank=True, + help_text=_( + "What the run raised, where it did not finish. Deletions are chunked " + "rather than wrapped in one transaction, so a run that died part way " + "left the grants it had already removed removed." + ), + ) + + class Meta: + ordering = ("-started_at",) + verbose_name = _("achievement sync run") + + def __str__(self): + """Human-readable label.""" + return f"{self.get_mode_display()} of '{self.source_slug}' #{self.pk}" diff --git a/badges/services.py b/badges/services.py index 4d589f8ab..f82bd1742 100644 --- a/badges/services.py +++ b/badges/services.py @@ -5,26 +5,40 @@ invariant always holds, and it is idempotent. Concurrent runs for the same (user, achievement) are not serialised: two -overlapping recalculations can leave the badge reflecting the earlier of their -two counts. Any later event for the pair, or a full ``recalculate_badges`` run, -repairs it. +overlapping recalculations can leave the badge reflecting the earlier of their two +counts. Any later event for the pair, or a full ``recalculate_badges`` run, repairs +it. + +This module also owns the achievement-side writes that feed recalculation: +``sync_source``, which makes the stored automatic grants for one source agree with +that source in both directions, and ``discard_source_achievements``, for source +rows about to be deleted outright. + +Both delete in bulk, and both recalculate their own members rather than leaving it +to the ``post_delete`` signal, which fires per row: see ``owns_recalculation``. """ import contextvars import logging from contextlib import contextmanager +from typing import NamedTuple from django.contrib.contenttypes.models import ContentType -from django.db import transaction +from django.db import DatabaseError, transaction from django.db.models import Prefetch from django.utils import timezone +from badges import sources from badges.enums import rank_order from badges.models import ( Achievement, + AchievementSyncRun, Badge, BadgeTier, RevocationSource, + SourceType, + SyncMode, + SyncTrigger, UserAchievement, UserBadge, ) @@ -36,16 +50,25 @@ "the {rank} threshold of {threshold}." ) +# Rows per DELETE ... WHERE pk IN (...) and per bulk_create. The unmatched set can +# be as large as the achievement table. +SYNC_BATCH_SIZE = 1000 + +# A database error carries the statement that failed, which for a batch of a +# thousand rows is the whole batch. Enough to identify the fault, not the payload. +SYNC_ERROR_MAX_LENGTH = 2000 + _revocation_cause = contextvars.ContextVar("badge_revocation_cause", default=None) +_owns_recalculation = contextvars.ContextVar("badge_owns_recalculation", default=False) @contextmanager def revocation_cause(description): """Name what is about to change achievement counts, for the audit trail. - A cascade revocation records only arithmetic, which tells support that a - count fell but not what moved it. Anything that changes grants in bulk should - wrap the work in this so the note says which operation was responsible. + A cascade revocation records only arithmetic, which tells support that a count + fell but not what moved it. Anything changing grants in bulk should wrap the + work in this so the note says which operation was responsible. """ token = _revocation_cause.set(description) try: @@ -54,6 +77,35 @@ def revocation_cause(description): _revocation_cause.reset(token) +@contextmanager +def owns_recalculation(): + """Take over recalculating from the ``post_delete`` signal for this block. + + The signal fires per row, and a member's badges are derived from a count + rather than adjusted by a delta, so deleting ten of one member's grants + recalculates the same answer ten times. A bulk delete that knows which members + it touched can do it once each instead, which is what this suspends the signal + for. + + Only the delete side needs it. Grants are inserted with ``bulk_create``, which + does not send ``post_save`` at all. + + The contract is the name: inside this block, deleting a grant no longer keeps + the member's badges honest, so the block itself has to. Not a way to make a + delete cheaper, a way to move the same work somewhere it can be batched. + """ + token = _owns_recalculation.set(True) + try: + yield + finally: + _owns_recalculation.reset(token) + + +def recalculation_is_owned(): + """Whether a caller has taken responsibility for recalculating, for signals.""" + return _owns_recalculation.get() + + def discard_source_achievements(model, object_ids): """Delete automatic grants pointing at the given rows and recalculate. @@ -61,7 +113,8 @@ def discard_source_achievements(model, object_ids): referential integrity, so deleting a source row on its own leaves a grant still counting toward a threshold. Call this first. - The caller is expected to own the transaction. + Atomic in itself, so the grants and the badges they justify move together + whether or not the caller has a transaction of its own around the source rows. """ object_ids = list(object_ids) if not object_ids: @@ -71,9 +124,375 @@ def discard_source_achievements(model, object_ids): 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) + # The pairs are known before the delete, so the per-row signal can only reach + # the same answer once per row instead of once per pair. + # + # Atomic here as well as at the caller: deleting a grant without recalculating + # it in the same transaction leaves a member holding a badge nothing supports, + # and leaves nothing behind to notice it by. Cheap to guarantee locally rather + # than depend on every future caller reading the paragraph above. + with transaction.atomic(): + with owns_recalculation(): + grants.delete() + for user_id, achievement_id in pairs: + recalculate_badges(user_id, achievement_id) + + +class SourceSync(NamedTuple): + """What syncing one source found, and what it was allowed to do about it. + + ``yielded`` counts the whole iterator, before any user scope is applied and + before deactivated members are dropped. That is what lets ``refused`` tell + "this member authored no commits any more" apart from "the commits table read + empty and something upstream is broken". + + ``changed`` is the members whose grants moved, which is what a caller + recalculates. It is populated on a dry run too, where it says who *would* + change and must not be recalculated. + + ``recalculated`` is the part of ``changed`` this run has already brought up to + date, which is every member it deleted from: those are recalculated per chunk + so that a run dying half way leaves the badges it got to correct. A caller + recalculating ``changed - recalculated`` does exactly the work still owing. + Empty on a dry run, and empty for an additive run, which deletes nothing and + whose ``bulk_create`` sends no signal. + """ + + slug: str + yielded: int + added: int + removed: int + changed: frozenset + applied: bool + refused: bool + recalculated: frozenset = frozenset() + run_id: int | None = None + + def outstanding(self): + """The members a caller still has to recalculate after this run.""" + return self.changed - self.recalculated + + def describe(self): + """One sentence about what this source's sync found. + + On the tuple rather than in each caller, so the commands' console lines and + the admin's confirmation page cannot reach different conclusions about the + same numbers. + """ + if self.refused: + return ( + f"REFUSED - the source yielded nothing, so {self.removed} grant(s) " + "were left alone" + ) + if not (self.added or self.removed): + return f"nothing to change ({self.yielded} yielded by the source)" + parts = [] + if self.added: + parts.append(f"{'added' if self.applied else 'add'} {self.added}") + if self.removed: + parts.append(f"{'removed' if self.applied else 'remove'} {self.removed}") + lead = "" if self.applied else "would " + return ( + f"{lead}{' and '.join(parts)} grant(s) " + f"across {len(self.changed)} member(s)" + ) + + +def sync_source( + slug, + achievement, + *, + user_ids=None, + add=True, + remove=True, + dry_run=False, + allow_empty=False, + batch_size=SYNC_BATCH_SIZE, + trigger=SyncTrigger.COMMAND, + actor=None, +): + """Make the stored automatic grants for one source agree with that source. + + One walk answers both halves: a pair the source yields with no row behind it is + created, and a stored row the source never yields is stale and deleted. + ``backfill_achievements`` is this with ``remove=False``, which is why it cannot + undo anything and why the weekly pipeline is safe to point at it. + + Manual grants are never touched, and an *invalidated* automatic row is matched + like any other, so an admin's judgement is never overwritten by a re-add. An + automatic row with no source pointer counts as stale: nothing can create one and + it can never match what an iterator yields. + + Deactivated members are skipped whatever the source says about them, so a + deleted account is granted nothing and loses what it holds on the next + reconcile. + + Stale grants are **deleted, not invalidated**: the uniqueness constraint on + automatic grants ignores ``is_valid``, so a tombstone would permanently block + the grant from being re-created if the attribution came back. + + Badges are recalculated here only for the members this run *deleted* from, once + per member per chunk, because the chunked delete is only crash-safe if the + badges move with it. Everything else is left to the caller, which knows whether + it is looking at one member or the whole table and whether a dry run means there + is nothing to do: see ``SourceSync.outstanding``. + + Args: + slug: A key of ``sources.BACKFILL_ITERATORS``. + achievement: The ``Achievement`` that ``slug`` feeds. + user_ids: Restrict both halves to these members. The iterator is still + walked in full - there is no way to ask it about one member - but no + other member's grants are created or deleted. + add: Create the grants the source yields and the database is missing. + remove: Delete the stored grants the source did not yield. + dry_run: Report what would change, writing nothing. + allow_empty: Delete even when the iterator yielded nothing at all. Off by + default: see ``refused`` below. + batch_size: Rows per ``bulk_create`` and per ``DELETE ... IN``. + trigger: What started this run, for the sync log. + actor: The admin who started it, where a person did. + + Returns: + A ``SourceSync``. ``refused`` is set when the iterator yielded no pairs + while stale grants exist, which is indistinguishable from a broken source + and would otherwise revoke every badge the source feeds. Nothing is + deleted in that case unless ``allow_empty`` says so; the additive half is + unaffected, there being nothing to add. + + Raises: + Whatever the source iterator or the writes raise, after naming it on the + run so a half-finished reconcile is not left looking like one still in + progress. + """ + run = None + if not dry_run: + run = AchievementSyncRun.objects.create( + source_slug=slug, + mode=SyncMode.RECONCILE if remove else SyncMode.BACKFILL, + trigger=trigger, + triggered_by=actor, + ) + # Named on every badge this run revokes, so support can follow a vanished badge + # back to the operation that moved the count. + with revocation_cause(None if run is None else str(run)): + try: + return _sync_source( + slug, + achievement, + run=run, + user_ids=user_ids, + add=add, + remove=remove, + dry_run=dry_run, + allow_empty=allow_empty, + batch_size=batch_size, + ) + except Exception as exc: + # An unfinished row is otherwise indistinguishable from a run still in + # flight, and the deletes are chunked rather than transactional, so a + # half-done reconcile has already revoked badges that this row is the + # only record of. Re-raised: the command still exits non-zero and the + # task still fails. + _fail_run(run, exc) + raise + + +def _fail_run(run, exc): + """Record that a run died, without letting the bookkeeping hide why. + + ``finished_at`` is stamped as well as ``error``: the run stopped, and leaving it + open would keep reading as in flight. Best effort - a failure here must never + replace the exception on its way out, so a connection left unusable by the + original error costs a log line and nothing else. + """ + if run is None: + return + run.error = f"{type(exc).__name__}: {exc}"[:SYNC_ERROR_MAX_LENGTH] + run.finished_at = timezone.now() + try: + run.save(update_fields=["error", "finished_at"]) + except DatabaseError: + logger.exception("Could not record the failure of %s", run) + + +def _finish_run(run, result): + """Record what a run did and hand the result back with its id attached.""" + if run is None: + return result + run.added = result.added + run.removed = 0 if result.refused else result.removed + run.members_changed = len(result.changed) + run.refused = result.refused + run.finished_at = timezone.now() + run.save( + update_fields=[ + "added", + "removed", + "members_changed", + "refused", + "finished_at", + ] + ) + return result._replace(run_id=run.pk) + + +def _sync_source( + slug, + achievement, + *, + run, + user_ids, + add, + remove, + dry_run, + allow_empty, + batch_size, +): + """The walk itself. See ``sync_source``, which owns the run log around it.""" + stored = UserAchievement.objects.filter( + achievement=achievement, source_type=SourceType.AUTOMATIC + ) + if user_ids is not None: + stored = stored.filter(user_id__in=user_ids) + + # Every stored key, keyed by what the iterator can reconstruct and valued by + # the rows carrying it. Whatever survives the walk is stale, and a key the + # walk cannot find here is a grant that does not exist yet - so one dict + # answers both halves and the walk needs no per-batch lookup of its own. + # Bounded by this achievement's row count rather than by the source's, so a + # scoped run holds one member's grants in memory and not every commit. + # + # A list of rows per key, not one: the source pointer is nullable, so several + # automatic rows can share ``(user, NULL, NULL)``, and one slot per key would + # clear all but the last of them per run. A key with a real pointer can only + # ever hold one row - ``unique_automatic_user_achievement_source`` says so. + unmatched = {} + for pk, user_id, content_type_id, object_id in stored.values_list( + "pk", "user_id", "source_content_type_id", "source_object_id" + ).iterator(chunk_size=2000): + unmatched.setdefault((user_id, content_type_id, object_id), []).append(pk) + + scope = None if user_ids is None else set(user_ids) + yielded = added = 0 + changed = set() + pending = {} + + def flush(): + """Insert the batch built so far and count it as added.""" + nonlocal added, pending + if not pending: + return + if not dry_run: + # ignore_conflicts because ``unmatched`` is a snapshot: a concurrent + # run of this same function may have inserted the row since. + UserAchievement.objects.bulk_create( + list(pending.values()), ignore_conflicts=True + ) + added += len(pending) + pending = {} + + for user, source in sources.BACKFILL_ITERATORS[slug](): + yielded += 1 + # A deactivated account is skipped for every source at once, rather than + # in each iterator, so a source wired later cannot forget the rule. It + # sits after ``yielded`` on purpose: the refusal below asks whether the + # source read empty, and "everyone it named is gone" is not that. + # + # Skipping is also what removes the grants such an account already holds, + # since its key stays in ``unmatched`` and reads as stale. That matters: + # deleting an account scrubs its grants, but the libraries and commits + # they derive from name it still, so without this the next sweep would + # award them all back. + if not user.is_active: + continue + # The scope is applied here as well as on ``unmatched``: an out-of-scope + # member's key is absent from it, which on the additive side is + # indistinguishable from a grant that needs creating. + if scope is not None and user.pk not in scope: + continue + content_type = ContentType.objects.get_for_model(source) + key = (user.pk, content_type.pk, source.pk) + if unmatched.pop(key, None) is not None: + continue + # ``pending`` is keyed, so an iterator that yields the same pair twice + # inside one batch counts it once. Across a flush the unique constraint + # is what catches it, and only the count is then optimistic. + if not add or key in pending: + continue + changed.add(user.pk) + pending[key] = UserAchievement( + user_id=user.pk, + achievement=achievement, + source_type=SourceType.AUTOMATIC, + source_content_type=content_type, + source_object_id=source.pk, + ) + if len(pending) >= batch_size: + flush() + flush() + + # Paired with the member each row belongs to, so a chunk can recalculate the + # members it just emptied without going back to the database to ask who they + # were. + stale = ( + [(pk, user_id) for (user_id, _, _), pks in unmatched.items() for pk in pks] + if remove + else [] + ) + if stale and not yielded and not allow_empty: + logger.warning( + "Refusing to remove %s stale grant(s) for '%s': the source yielded " + "nothing at all. Pass allow_empty to override.", + len(stale), + slug, + ) + return _finish_run( + run, + SourceSync( + slug, yielded, added, len(stale), frozenset(changed), not dry_run, True + ), + ) + + if remove: + changed.update(user_id for user_id, _, _ in unmatched) + + recalculated = set() + if stale and not dry_run: + # One transaction per chunk, and never one for the whole run: a run that + # dies half way is then simply a run to repeat, because every chunk either + # happened with its recalculations or did not happen at all. + # + # The pairing is what makes that true, and it is not optional. Deleting a + # grant outside the same transaction that recalculates it leaves a member + # holding a badge their count no longer supports, *and* leaves no trace of + # it: a second reconcile finds their grants already gone, so it reports + # nothing changed and recalculates nobody. Only a full recalculation would + # find them. + for start in range(0, len(stale), batch_size): + chunk = stale[start : start + batch_size] + members = {user_id for _, user_id in chunk} + with transaction.atomic(): + with owns_recalculation(): + UserAchievement.objects.filter( + pk__in=[pk for pk, _ in chunk] + ).delete() + for user_id in members: + recalculate_badges(user_id, achievement.pk) + recalculated.update(members) + + return _finish_run( + run, + SourceSync( + slug, + yielded, + added, + len(stale), + frozenset(changed), + not dry_run, + False, + recalculated=frozenset(recalculated), + ), + ) def deactivate_tier(tier, actor=None): diff --git a/badges/signals.py b/badges/signals.py index d3e375d67..eb11e4300 100644 --- a/badges/signals.py +++ b/badges/signals.py @@ -1,7 +1,9 @@ """Signals that keep badge state in step with achievement and tier changes. Creating, invalidating or deleting a ``UserAchievement`` all move a member's valid -count, so all three funnel into ``recalculate_badges``. +count, so all three funnel into ``recalculate_badges``. Row at a time, which is the +right granularity for the admin and for a shell, and the wrong one for a bulk +delete: those take the work over through ``services.owns_recalculation``. A ``BadgeTier`` change affects every member of that achievement type, so it goes to a task instead of the request. It can only ever add badges: recalculation @@ -13,7 +15,7 @@ from django.dispatch import receiver from badges.models import Badge, BadgeTier, UserAchievement -from badges.services import recalculate_badges +from badges.services import recalculate_badges, recalculation_is_owned from badges.tasks import recalculate_achievement_task @@ -37,7 +39,14 @@ def recalculate_on_achievement_save(sender, instance, created, raw, **kwargs): @receiver(post_delete, sender=UserAchievement) def recalculate_on_achievement_delete(sender, instance, **kwargs): - """Recalculate when an achievement row is hard-deleted.""" + """Recalculate when an achievement row is hard-deleted. + + Stands down for a bulk delete that has taken the job on itself: this fires per + row, and one member losing ten grants needs one recalculation, not ten. + """ + if recalculation_is_owned(): + return + recalculate_badges(instance.user_id, instance.achievement_id) diff --git a/badges/sources.py b/badges/sources.py new file mode 100644 index 000000000..8ca568775 --- /dev/null +++ b/badges/sources.py @@ -0,0 +1,42 @@ +"""Automatic achievement sources. + +Maps an automatic achievement type to the model it derives from, as an iterator +yielding ``(user, source_object)`` pairs over all historical data. There are +deliberately **no live signals**: this data is processed in batch by +``backfill_achievements`` and ad hoc by manual admin grants, so automatic +achievements are not real-time. + +Three of the catalogue's eight types have no automatic source, there being no clean +per-record, per-user source for them here: + +* ``documentation`` - no model tracks documentation contributions per user. +* ``mailing-list`` (Regular) - posts live in the external Hyperkitty database, + which stores aggregate counts rather than per-post rows. +* ``publisher`` - news post storage is being reworked, so an iterator written + against the current models would not survive it. + +All three can still be granted by hand in the admin. +""" + +from badges.enums import AchievementSlug + + +def _iter_code_commits(): + """Yield (user, commit) for every attributed commit.""" + from libraries.models import Commit + + commits = ( + Commit.objects.filter(author__user__isnull=False) + .select_related("author__user") + .iterator(chunk_size=1000) + ) + for commit in commits: + yield commit.author.user, commit + + +BACKFILL_ITERATORS = { + AchievementSlug.CODE_COMMITS: _iter_code_commits, +} + +# Derived, so the CLI choices can never drift from the wired iterators. +AUTOMATIC_SLUGS = [slug.value for slug in BACKFILL_ITERATORS] diff --git a/badges/summary.py b/badges/summary.py new file mode 100644 index 000000000..60652708e --- /dev/null +++ b/badges/summary.py @@ -0,0 +1,256 @@ +"""Why one member does or does not show a badge. + +An admin diagnostic, deliberately separate from ``badges.display``: that module +is the public-profile rendering layer and answers "what do we show", while this +one answers "why", including for the badges a member does *not* have. The four +causes of a missing badge - below threshold, cascade-revoked, manually revoked, +hidden by the member - live in three different changelists otherwise, and two of +them are only visible as arithmetic against a threshold. + +Everything is read in a fixed number of queries, so the cost does not grow with +the number of achievement types an admin has created. +""" + +from dataclasses import dataclass + +from django.db.models import Count, Prefetch, Q + +from badges.enums import rank_order +from badges.models import ( + RANK_LADDER_ORDER, + Achievement, + Badge, + BadgeTier, + RevocationSource, + UserAchievement, + UserBadge, +) + + +@dataclass(frozen=True) +class AchievementRow: + """One achievement, one badge it feeds, and the member's state against it.""" + + achievement: Achievement + badge: Badge | None + valid_grants: int + invalid_grants: int + held: UserBadge | None + next_tier: BadgeTier | None + revoked: list[UserBadge] + reason: str + + @property + def gap(self): + """Valid grants still needed to reach ``next_tier``, if there is one.""" + if self.next_tier is None: + return None + return max(self.next_tier.threshold - self.valid_grants, 0) + + +def user_badge_summary(user): + """One row per achievement type and badge, with why the member shows it. + + An achievement that feeds no badge still gets a row: its grants accumulate + and can never become anything, which is worth seeing. An achievement that + feeds several gets one row each, because each badge has its own ladder and + its own answer. + """ + counts = { + row["achievement_id"]: row + for row in UserAchievement.objects.filter(user=user) + .values("achievement_id") + .annotate( + valid=Count("pk", filter=Q(is_valid=True)), + invalid=Count("pk", filter=Q(is_valid=False)), + ) + } + achievements = Achievement.objects.prefetch_related( + Prefetch( + "badges", + queryset=Badge.objects.prefetch_related( + Prefetch( + "tiers", + # Up the ladder, which is the order the page presents them in. + queryset=BadgeTier.objects.filter(is_active=True).order_by( + RANK_LADDER_ORDER + ), + to_attr="active_tiers", + ) + ), + ) + ) + awarded = {} + # ``revoked_by`` is joined because a manual revocation names the admin who + # made it, which would otherwise be a query per revoked badge. + for user_badge in UserBadge.objects.filter(user=user).select_related( + "badge", "tier", "revoked_by" + ): + awarded.setdefault(user_badge.badge_id, []).append(user_badge) + + rows = [] + for achievement in achievements: + grants = counts.get(achievement.pk, {}) + valid = grants.get("valid", 0) + invalid = grants.get("invalid", 0) + badges = list(achievement.badges.all()) + if not badges: + rows.append(_row(user, achievement, None, valid, invalid, [])) + continue + for badge in badges: + rows.append( + _row( + user, + achievement, + badge, + valid, + invalid, + awarded.get(badge.pk, []), + ) + ) + return rows + + +def _row(user, achievement, badge, valid, invalid, user_badges): + """Assemble one row from the member's badge rows for a single badge.""" + held = _highest_held(user_badges) + revoked = sorted( + (row for row in user_badges if row.revoked_at is not None), + key=lambda row: row.revoked_at, + reverse=True, + ) + next_tier = _next_tier(badge, held, user_badges) + return AchievementRow( + achievement=achievement, + badge=badge, + valid_grants=valid, + invalid_grants=invalid, + held=held, + next_tier=next_tier, + revoked=revoked, + reason=_reason(user, badge, valid, held, next_tier, revoked), + ) + + +def _highest_held(user_badges): + """The best tier the member currently holds for one badge. + + Ranked by ``TierRank`` order, not by threshold. Retiring a tier keeps the + badges awarded against it, so a badge can hold a retired gold at 3 next to a + live bronze at 6 and the higher threshold is then the *lower* rank. + Threshold only breaks ties between two rows of the same rank. + """ + active = [row for row in user_badges if row.revoked_at is None] + if not active: + return None + return max(active, key=lambda row: (rank_order(row.tier.rank), row.tier.threshold)) + + +def _next_tier(badge, held, user_badges): + """The lowest rung the member can still climb to, by rank. + + Keyed on the member's *rank*, not on their grant count. Counting from the + count picks the tier with the lowest unmet threshold, which after a retuning + is a rank the member has already passed: shift a whole ladder up by five and + a gold holder's "next" rank becomes bronze, which is not a rung anyone climbs + to from gold. Their next rung is platinum, and the threshold is then the + answer to "how many do I need", not the question. + + Ranks with no active tier are skipped rather than reported as unreachable, so + a badge whose silver has been retired sends a bronze holder to gold. + + A manually revoked rank is skipped too, because recalculation will not give it + back however many grants arrive - ``services._award_tier`` refuses to - so it + is not the rung anyone is waiting for. A cascade revocation is left in place: + that one does come back on its own once the count recovers. + """ + if badge is None: + return None + floor = -1 if held is None else rank_order(held.tier.rank) + blocked = { + row.tier_id + for row in user_badges + if row.revoked_at is not None + and row.revocation_source == RevocationSource.MANUAL + } + above = [ + tier + for tier in badge.active_tiers + if rank_order(tier.rank) > floor and tier.pk not in blocked + ] + return min(above, key=lambda tier: rank_order(tier.rank), default=None) + + +def _reason(user, badge, valid, held, next_tier, revoked): + """Plain English for the state the row is in. + + Ordered by what a support request is actually asking. A configuration fault + outranks anything about the member, because no answer about the member is + meaningful while the badge cannot award at all. + """ + if badge is None: + return "No badge is configured for this achievement." + if not badge.active_tiers: + return "The badge has no active tiers, so it awards nothing." + if held is not None: + return _held_reason(user, held, valid) + if revoked: + return _revoked_reason(revoked, valid) + # Past the two branches above the member has no badge rows at all for this + # badge, so nothing is blocking the bottom of the ladder and ``next_tier`` is + # the lowest active tier rather than None. + if valid < next_tier.threshold: + return ( + f"Not earned - {valid} of {next_tier.threshold} for " + f"{next_tier.get_rank_display()}." + ) + # The threshold for the next rung is already met and no badge row exists, so + # recalculation has not run since the grants arrived. Only reachable if + # something wrote grants without firing the signals - a bulk insert, raw + # SQL, a restored dump. Naming the highest rung the count reaches says what + # recalculating would actually hand out, which the next rung alone does not. + reached = max( + (tier for tier in badge.active_tiers if valid >= tier.threshold), + key=lambda tier: rank_order(tier.rank), + ) + return ( + f"Not earned, but {valid} valid grants already reaches " + f"{reached.get_rank_display()}. Recalculate to award it." + ) + + +def _held_reason(user, held, valid): + """Why a held badge does or does not reach the member's profile. + + The stale case comes first: ``hide_badges`` is stated once at the top of the + page already, whereas a badge held below its own threshold is an + inconsistency nothing else on the page names. + """ + rank = held.tier.get_rank_display() + since = held.awarded_at.date() + if valid < held.tier.threshold: + return ( + f"Held since {since} ({rank}), but only {valid} valid grants against " + f"a threshold of {held.tier.threshold}. Recalculate to reconcile." + ) + if user.hide_badges: + return "Held, but hidden - the member has turned badge display off." + return f"Held since {since} ({rank})." + + +def _revoked_reason(revoked, valid): + """Describe the revocation that is actually keeping the badge away. + + A manual revocation survives recalculation and a cascade revocation does + not, so a manual one is the blocker whenever both are present. Within either + group the lowest threshold is the one nearest to coming back. + """ + manual = [ + row for row in revoked if row.revocation_source == RevocationSource.MANUAL + ] + row = min(manual or revoked, key=lambda candidate: candidate.tier.threshold) + if row.revocation_source == RevocationSource.MANUAL: + who = row.revoked_by or "an admin whose account is gone" + note = row.revocation_notes or "no note recorded" + return f"Revoked by {who} on {row.revoked_at.date()}: {note}" + return f"Revoked automatically - {valid} valid grants, needs {row.tier.threshold}." diff --git a/badges/tasks.py b/badges/tasks.py index 35707d965..43d29bab5 100644 --- a/badges/tasks.py +++ b/badges/tasks.py @@ -1,19 +1,59 @@ """Background badge work. -``recalculate_achievement_task`` is narrow on purpose: a configuration change only -affects one achievement type, so there is no reason to sweep the whole table, and -no reason to make the request that changed it wait. +The command wrappers exist so the admin changelist buttons can start a long-running +command on a worker instead of holding the request open. +``recalculate_achievement_task`` is narrower: a configuration change only affects +one achievement type, so there is no reason to sweep the whole table. """ import logging from celery import shared_task +from django.core.management import call_command from badges.services import achievement_pairs, recalculate_many logger = logging.getLogger(__name__) +@shared_task +def backfill_achievements_task(slug=None, actor_id=None): + """Run the ``backfill_achievements`` management command off-request. + + No slug sweeps every wired source, which is what the unscoped admin button + wants. A slug narrows the run to that one source, so a newly wired iterator can + be backfilled without walking every commit in the database again. + """ + options = {"actor_id": actor_id} + if slug is not None: + options["slugs"] = [slug] + call_command("backfill_achievements", **options) + + +@shared_task +def recalculate_all_badges_task(): + """Run the ``recalculate_badges`` management command off-request.""" + call_command("recalculate_badges") + + +@shared_task +def reconcile_achievements_task(slug=None, user_id=None, actor_id=None): + """Run the ``reconcile_achievements`` command off-request. + + ``--allow-empty`` is deliberately not reachable from here. A source that reads + empty is refused, and overriding that refusal is a decision for someone at a + shell who has looked at why it is empty, not for a button. + """ + options = {"actor_id": actor_id} + if slug: + options["slugs"] = [slug] + if user_id: + # A string because that is what argparse would have handed the command, + # and what its email-or-id resolution expects. + options["users"] = [str(user_id)] + call_command("reconcile_achievements", **options) + + @shared_task def recalculate_achievement_task(achievement_id): """Recalculate every (user, achievement) pair for one achievement type.""" diff --git a/badges/templates/admin/badges/notes_action.html b/badges/templates/admin/badges/notes_action.html new file mode 100644 index 000000000..ca2410a66 --- /dev/null +++ b/badges/templates/admin/badges/notes_action.html @@ -0,0 +1,36 @@ +{% extends "admin/base_site.html" %} +{% load i18n static %} + +{% block extrastyle %} + {{ block.super }} + + +{% endblock %} + +{% block content %} +

{% blocktranslate %}A note is required for this action. It will be recorded for audit purposes.{% endblocktranslate %}

+ + + +
+ {% csrf_token %} + {% for obj in objects %} + + {% endfor %} + + + {{ form.as_p }} + + +
+{% endblock %} diff --git a/badges/templates/admin/badges/user_summary.html b/badges/templates/admin/badges/user_summary.html new file mode 100644 index 000000000..8f4f71026 --- /dev/null +++ b/badges/templates/admin/badges/user_summary.html @@ -0,0 +1,127 @@ +{% extends "admin/base_site.html" %} +{% load i18n static %} + +{% block extrastyle %} + {{ block.super }} + + +{% endblock %} + +{% block breadcrumbs %} + +{% endblock %} + +{% block content %} + {% if member_admin_url %} + + {% endif %} + + {% if member.hide_badges %} + + {% endif %} + +
+

{% translate "Achievements and badges" %}

+ + + + + + + + + + + + + + {% for item in rows %} + + + + + + + + + + {% empty %} + + + + {% endfor %} + +
{% translate "Badge" %}{% translate "Achievement" %}{% translate "Valid grants" %}{% translate "Invalid" %}{% translate "Current rank" %}{% translate "Next rank" %}{% translate "State" %}
{{ item.row.badge|default:"-" }}{{ item.row.achievement }}{{ item.row.valid_grants }}{{ item.row.invalid_grants }} + {% if item.row.held %} + {{ item.row.held.tier.get_rank_display }} (≥ {{ item.row.held.tier.threshold }}) + {% else %} + {% translate "None" %} + {% endif %} + + {% if item.row.next_tier %} + {{ item.row.next_tier.get_rank_display }} (≥ {{ item.row.next_tier.threshold }}), + {% blocktranslate with gap=item.row.gap %}{{ gap }} to go{% endblocktranslate %} + {% else %} + {% translate "None" %} + {% endif %} + + {{ item.row.reason }} + {% if item.row.revoked %} +
    + {% for entry in item.row.revoked %} +
  • + {% blocktranslate with rank=entry.tier.get_rank_display source=entry.get_revocation_source_display date=entry.revoked_at|date:"Y-m-d" %}{{ rank }} revoked {{ date }} ({{ source }}){% endblocktranslate %} +
  • + {% endfor %} +
+ {% endif %} +
{% translate "No achievement types are configured." %}
+
+ +
+ {% if can_recalculate %} +
+
+ {% csrf_token %} + + +
+

+ {% blocktranslate %}Rebuilds their badges from the achievements above, awarding any tier they now meet and revoking any they no longer do. Changes no achievements.{% endblocktranslate %} +

+
+ {% endif %} + {% if can_reconcile %} +
+
+ {% csrf_token %} + + +
+

+ {% blocktranslate %}Rebuilds the achievements themselves from their sources, adding what a source now supports and removing what it no longer does, then the badges. Manual grants are left alone, and you see the changes first.{% endblocktranslate %} +

+
+ {% endif %} + {% if can_grant %} +
+ {% translate "Grant an achievement" %} +

+ {% blocktranslate %}Adds one achievement by hand, for something no source can see. Reconciling never removes it.{% endblocktranslate %} +

+
+ {% endif %} +
+{% endblock %} diff --git a/badges/tests/fixtures.py b/badges/tests/fixtures.py index a9876bf88..85b1dd4c6 100644 --- a/badges/tests/fixtures.py +++ b/badges/tests/fixtures.py @@ -6,6 +6,7 @@ import pytest from django.contrib.contenttypes.models import ContentType +from django.core.management import call_command from model_bakery import baker from badges.enums import BadgeLabel, TierRank @@ -112,6 +113,42 @@ def plain_user(db): return baker.make("users.User", email="badge-user@example.com") +@pytest.fixture +def commit_by_someone_else(catalogue, super_user): + """A live attributed commit belonging to another member, already recorded. + + ``sync_source`` refuses to act on a source that yields nothing at all, so a + test about one member's stale grants has to leave the source yielding somebody. + Only the tests of the refusal itself do without this. + + Backfilled, so this member is fully *in step* with the source: otherwise a + two-way run would have a grant to create here, and every test using this + fixture would be quietly asserting against that as well. + """ + author = baker.make("libraries.CommitAuthor", user=super_user) + commit = baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + return commit + + +@pytest.fixture +def stale_commit_grant(catalogue, plain_user): + """Give ``plain_user`` a commits achievement, then break its attribution. + + Reproduces what reconciliation exists for: the ``Commit`` row survives, the + ``UserAchievement`` row survives, and the two no longer agree because the + ``CommitAuthor`` stopped pointing at the member. Returns the commit. + """ + author = baker.make("libraries.CommitAuthor", user=plain_user) + commit = baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + assert UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + + author.user = None + author.save() + return commit + + @pytest.fixture def grant_achievement(db): """Return a helper that creates valid UserAchievement rows. diff --git a/badges/tests/test_admin.py b/badges/tests/test_admin.py new file mode 100644 index 000000000..b872a90ff --- /dev/null +++ b/badges/tests/test_admin.py @@ -0,0 +1,1271 @@ +"""Tests for the badges admin actions and manual-grant behaviour.""" + +from unittest.mock import Mock, patch + +import pytest +from django.contrib.admin import helpers +from django.contrib.admin.sites import AdminSite +from django.contrib.auth.models import Permission +from django.core.cache import cache +from django.core.management import call_command +from django.test import RequestFactory +from django.urls import reverse +from django.utils import timezone +from model_bakery import baker + +from badges.admin import UserAchievementAdmin, UserBadgeAdmin +from badges.enums import TierRank +from badges.models import ( + Achievement, + BadgeTier, + RevocationSource, + SourceType, + UserAchievement, + UserBadge, +) +from badges.services import deactivate_tier +from badges.tests.fixtures import grant_from_source + +pytestmark = pytest.mark.django_db + + +@pytest.fixture(autouse=True) +def _clear_task_button_locks(): + """The task buttons debounce through the cache; isolate tests from each other.""" + cache.clear() + + +def test_manual_create_sets_source_type_and_granted_by(achievement, super_user): + """save_model marks admin-created grants as manual and records the admin.""" + admin = UserAchievementAdmin(UserAchievement, AdminSite()) + request = RequestFactory().post("/") + request.user = super_user + obj = UserAchievement(achievement=achievement, user=super_user) + + admin.save_model(request, obj, form=None, change=False) + + obj.refresh_from_db() + assert obj.source_type == SourceType.MANUAL + assert obj.granted_by == super_user + + +def test_invalidate_action_requires_notes( + client, super_user, plain_user, achievement, grant_achievement +): + """Invalidation with an empty note does not change the achievement.""" + rows = grant_achievement(plain_user, achievement, count=1) + client.force_login(super_user) + url = reverse("admin:badges_userachievement_changelist") + + client.post( + url, + { + "action": "invalidate", + helpers.ACTION_CHECKBOX_NAME: [rows[0].pk], + "apply": "1", + "notes": "", + }, + ) + + rows[0].refresh_from_db() + assert rows[0].is_valid is True + + +def test_invalidate_action_with_notes( + client, super_user, plain_user, achievement, grant_achievement +): + """Invalidation records the admin, timestamp and note.""" + rows = grant_achievement(plain_user, achievement, count=1) + client.force_login(super_user) + url = reverse("admin:badges_userachievement_changelist") + + client.post( + url, + { + "action": "invalidate", + helpers.ACTION_CHECKBOX_NAME: [rows[0].pk], + "apply": "1", + "notes": "Duplicate record", + }, + ) + + rows[0].refresh_from_db() + assert rows[0].is_valid is False + assert rows[0].invalidated_by == super_user + assert rows[0].invalidated_at is not None + assert rows[0].invalidation_notes == "Duplicate record" + + +@pytest.mark.parametrize( + "changelist,action,selected", + [ + ("admin:badges_userachievement_changelist", "invalidate", "grant"), + ("admin:badges_userbadge_changelist", "revoke", "badge"), + ], +) +def test_notes_page_is_laid_out_and_cancels_back_to_the_list( + client, + super_user, + plain_user, + badge, + achievement, + grant_achievement, + changelist, + action, + selected, +): + """The third confirmation page of its kind, and the one that was left behind. + + ``.submit-row`` is only a flex bar because of the admin's own ``forms.css``, + which ``base_site.html`` does not load - so without it the submit and Cancel + stack with no spacing and Cancel keeps the admin's link underline. Cancel is + also an explicit url: this page is posted to from the changelist, so ``../`` + would land on the app index rather than back on the list. + """ + grant_achievement(plain_user, achievement, count=1) + row = ( + UserAchievement.objects.get(user=plain_user) + if selected == "grant" + else UserBadge.objects.get(user=plain_user, tier__rank=TierRank.BRONZE) + ) + client.force_login(super_user) + changelist_url = reverse(changelist) + + body = client.post( + changelist_url, + {"action": action, helpers.ACTION_CHECKBOX_NAME: [row.pk]}, + ).content.decode() + + assert "admin/css/forms.css" in body + assert "css/admin/controls.css" in body + assert f'href="{changelist_url}" class="button cancel-link"' in body + + +def test_source_column_links_a_registered_source(client, super_user, plain_user): + """An automatic grant must show what it came from.""" + achievement = baker.make("badges.Achievement", slug="code-commits") + author = baker.make("libraries.CommitAuthor", user=plain_user) + commit = baker.make("libraries.Commit", author=author) + grant_from_source(plain_user, achievement, commit) + client.force_login(super_user) + + response = client.get(reverse("admin:badges_userachievement_changelist")) + + assert reverse("admin:libraries_commit_change", args=[commit.pk]).encode() in ( + response.content + ) + + +def test_source_column_falls_back_for_an_unregistered_source( + client, super_user, plain_user +): + """news.Entry has no admin, so the column shows its label instead of 500ing.""" + achievement = baker.make("badges.Achievement", slug="publisher") + entry = baker.make("news.Entry", author=plain_user, title="A published post") + grant_from_source(plain_user, achievement, entry) + client.force_login(super_user) + + response = client.get(reverse("admin:badges_userachievement_changelist")) + + assert response.status_code == 200 + assert b"A published post" in response.content + + +def test_source_column_is_blank_for_a_manual_grant( + client, super_user, plain_user, achievement, grant_achievement +): + """A manual grant has no source row to point at.""" + grant_achievement(plain_user, achievement, count=1) + client.force_login(super_user) + + response = client.get(reverse("admin:badges_userachievement_changelist")) + + assert response.status_code == 200 + + +def test_revalidate_action_clears_the_audit_fields( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Revalidating restores the count and leaves no stale invalidation trail.""" + rows = grant_achievement(plain_user, achievement, count=1) + rows[0].is_valid = False + rows[0].invalidated_by = super_user + rows[0].invalidated_at = timezone.now() + rows[0].invalidation_notes = "Duplicate record" + rows[0].save() + assert not UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + client.force_login(super_user) + + client.post( + reverse("admin:badges_userachievement_changelist"), + {"action": "revalidate", helpers.ACTION_CHECKBOX_NAME: [rows[0].pk]}, + ) + + rows[0].refresh_from_db() + assert rows[0].is_valid is True + assert rows[0].invalidated_by is None + assert rows[0].invalidated_at is None + assert rows[0].invalidation_notes == "" + assert UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + + +def test_invalidate_action_reports_an_already_invalid_selection( + client, super_user, plain_user, achievement, grant_achievement +): + """The confirmation page must not list rows the action would skip.""" + rows = grant_achievement(plain_user, achievement, count=1) + rows[0].is_valid = False + rows[0].save() + client.force_login(super_user) + + response = client.post( + reverse("admin:badges_userachievement_changelist"), + {"action": "invalidate", helpers.ACTION_CHECKBOX_NAME: [rows[0].pk]}, + follow=True, + ) + + assert "Nothing to invalidate" in response.content.decode() + + +def test_revoke_action_reports_an_already_revoked_selection( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """The confirmation page must not list badges the action would skip.""" + grant_achievement(plain_user, achievement, count=1) + bronze = UserBadge.objects.get( + user=plain_user, badge=badge, tier__rank=TierRank.BRONZE + ) + bronze.revoked_at = timezone.now() + bronze.revocation_source = RevocationSource.MANUAL + bronze.save() + client.force_login(super_user) + + response = client.post( + reverse("admin:badges_userbadge_changelist"), + {"action": "revoke", helpers.ACTION_CHECKBOX_NAME: [bronze.pk]}, + follow=True, + ) + + assert "Nothing to revoke" in response.content.decode() + + +def test_revoke_action_requires_notes( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Revoking a badge with an empty note leaves it active.""" + grant_achievement(plain_user, achievement, count=1) + bronze = UserBadge.objects.get( + user=plain_user, badge=badge, tier__rank=TierRank.BRONZE + ) + client.force_login(super_user) + url = reverse("admin:badges_userbadge_changelist") + + client.post( + url, + { + "action": "revoke", + helpers.ACTION_CHECKBOX_NAME: [bronze.pk], + "apply": "1", + "notes": "", + }, + ) + + bronze.refresh_from_db() + assert bronze.revoked_at is None + + +def test_revoke_action_with_notes( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Revoking a badge records the admin, timestamp and note.""" + grant_achievement(plain_user, achievement, count=1) + bronze = UserBadge.objects.get( + user=plain_user, badge=badge, tier__rank=TierRank.BRONZE + ) + client.force_login(super_user) + url = reverse("admin:badges_userbadge_changelist") + + client.post( + url, + { + "action": "revoke", + helpers.ACTION_CHECKBOX_NAME: [bronze.pk], + "apply": "1", + "notes": "Awarded by mistake", + }, + ) + + bronze.refresh_from_db() + assert bronze.revoked_at is not None + assert bronze.revoked_by == super_user + assert bronze.revocation_notes == "Awarded by mistake" + assert bronze.revocation_source == RevocationSource.MANUAL + + +def test_reinstate_action_clears_manual_revocation( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Reinstating undoes the revocation without pretending it was earned again.""" + grant_achievement(plain_user, achievement, count=1) + bronze = UserBadge.objects.get( + user=plain_user, badge=badge, tier__rank=TierRank.BRONZE + ) + originally_awarded_at = timezone.datetime(2025, 3, 7, 14, 30, tzinfo=timezone.UTC) + bronze.awarded_at = originally_awarded_at + bronze.revoked_at = timezone.now() + bronze.revoked_by = super_user + bronze.revocation_notes = "Awarded by mistake" + bronze.revocation_source = RevocationSource.MANUAL + bronze.save() + client.force_login(super_user) + url = reverse("admin:badges_userbadge_changelist") + + client.post( + url, + {"action": "reinstate", helpers.ACTION_CHECKBOX_NAME: [bronze.pk]}, + ) + + bronze.refresh_from_db() + assert bronze.revoked_at is None + assert bronze.revoked_by is None + assert bronze.revocation_notes == "" + assert bronze.revocation_source == "" + assert bronze.awarded_at == originally_awarded_at + + +def test_reinstate_action_refuses_a_cascade_revocation( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Reinstating a cascade revocation would award an unearned badge.""" + rows = grant_achievement(plain_user, achievement, count=1) + bronze = UserBadge.objects.get( + user=plain_user, badge=badge, tier__rank=TierRank.BRONZE + ) + + rows[0].is_valid = False + rows[0].invalidated_by = super_user + rows[0].save() + bronze.refresh_from_db() + assert bronze.revocation_source == RevocationSource.CASCADE + + client.force_login(super_user) + response = client.post( + reverse("admin:badges_userbadge_changelist"), + {"action": "reinstate", helpers.ACTION_CHECKBOX_NAME: [bronze.pk]}, + follow=True, + ) + + bronze.refresh_from_db() + assert bronze.revoked_at is not None + assert bronze.revocation_source == RevocationSource.CASCADE + assert "Skipped 1 cascade-revoked badge(s)" in response.content.decode() + + +def test_userbadge_status_filter_partitions_the_rows( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Held and revoked, rather than Django's date filter on revoked_at.""" + grant_achievement(plain_user, achievement, count=1) + held = UserBadge.objects.get(user=plain_user) + revoked = baker.make( + UserBadge, + badge=badge, + user=super_user, + tier=badge.tiers.get(rank=TierRank.SILVER), + revoked_at=timezone.now(), + ) + client.force_login(super_user) + url = reverse("admin:badges_userbadge_changelist") + + def ids(query): + """The primary keys the changelist returns for one filter query.""" + return set( + client.get(f"{url}{query}") + .context["cl"] + .queryset.values_list("pk", flat=True) + ) + + assert ids("?status=held") == {held.pk} + assert ids("?status=revoked") == {revoked.pk} + + +def test_userbadge_changelist_shows_a_hidden_profile( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """ "My badge is missing" is usually hide_badges, so make it visible.""" + grant_achievement(plain_user, achievement, count=1) + plain_user.hide_badges = True + plain_user.save(update_fields=["hide_badges"]) + admin_class = UserBadgeAdmin(UserBadge, AdminSite()) + + row = UserBadge.objects.get(user=plain_user) + + assert admin_class.is_held(row) is True + assert admin_class.hidden_by_member(row) is True + + +def test_tier_delete_is_a_soft_delete(client, super_user, badge): + """Deleting a tier in the admin deactivates it instead of removing it.""" + silver = badge.tiers.get(rank=TierRank.SILVER) + client.force_login(super_user) + url = reverse("admin:badges_badgetier_delete", args=[silver.pk]) + + client.post(url, {"post": "yes"}) + + silver.refresh_from_db() # row still exists + assert silver.is_active is False + assert silver.deactivated_by == super_user + assert silver.deactivated_at is not None + + +def test_reactivate_action_restores_a_retired_tier(client, super_user, badge): + """A retired tier's change form has no fields, so an action is the only undo.""" + silver = badge.tiers.get(rank=TierRank.SILVER) + deactivate_tier(silver, actor=super_user) + client.force_login(super_user) + + client.post( + reverse("admin:badges_badgetier_changelist"), + {"action": "reactivate", helpers.ACTION_CHECKBOX_NAME: [silver.pk]}, + ) + + silver.refresh_from_db() + assert silver.is_active is True + assert silver.deactivated_at is None + + +def test_reactivate_action_refuses_a_replaced_tier(client, super_user, badge): + """Reactivating would break the one-active-tier-per-rank constraint.""" + silver = badge.tiers.get(rank=TierRank.SILVER) + deactivate_tier(silver, actor=super_user) + baker.make(BadgeTier, badge=badge, rank=TierRank.SILVER, threshold=9) + client.force_login(super_user) + + response = client.post( + reverse("admin:badges_badgetier_changelist"), + {"action": "reactivate", helpers.ACTION_CHECKBOX_NAME: [silver.pk]}, + follow=True, + ) + + silver.refresh_from_db() + assert silver.is_active is False + assert "Retire the replacement first" in response.content.decode() + + +def test_reactivate_names_a_few_of_the_tiers_it_skipped_and_counts_the_rest( + client, super_user, badge +): + """A skipped list grows with the selection; the sentence must not. + + Four rivals rather than three, because three is where the message stops + naming them. + """ + retired = [] + for rank, threshold in ( + (TierRank.BRONZE, 11), + (TierRank.SILVER, 13), + (TierRank.GOLD, 15), + (TierRank.PLATINUM, 17), + ): + tier = badge.tiers.filter(rank=rank).first() or baker.make( + BadgeTier, badge=badge, rank=rank, threshold=threshold - 1 + ) + deactivate_tier(tier, actor=super_user) + baker.make(BadgeTier, badge=badge, rank=rank, threshold=threshold) + retired.append(tier) + client.force_login(super_user) + + response = client.post( + reverse("admin:badges_badgetier_changelist"), + { + "action": "reactivate", + helpers.ACTION_CHECKBOX_NAME: [tier.pk for tier in retired], + }, + follow=True, + ) + + body = response.content.decode() + assert "Skipped 4 tier(s)" in body + assert "and 1 more" in body + assert not BadgeTier.objects.filter( + pk__in=[tier.pk for tier in retired], is_active=True + ).exists() + + +def test_the_notes_page_lists_ten_of_the_selection_and_counts_the_rest( + client, super_user, plain_user, achievement, grant_achievement +): + """ "Select all" is the whole table, and this page would otherwise be it. + + The listing is capped; the hidden fields that carry the selection into the + second POST are not, or the action would apply to ten of them. + """ + rows = grant_achievement(plain_user, achievement, count=12) + client.force_login(super_user) + + response = client.post( + reverse("admin:badges_userachievement_changelist"), + { + "action": "invalidate", + helpers.ACTION_CHECKBOX_NAME: [row.pk for row in rows], + }, + ) + + body = response.content.decode() + # Every row reads the same, so counting the label counts the list items. + assert body.count(str(rows[0])) == 10 + assert "... and 2 more." in body + assert body.count(f'name="{helpers.ACTION_CHECKBOX_NAME}"') == 12 + assert UserAchievement.objects.filter(is_valid=True).count() == 12 + + +def test_tier_changelist_groups_a_badge_ladder_together(client, super_user, catalogue): + """Ordering by threshold alone interleaves every badge's bronze row.""" + client.force_login(super_user) + + response = client.get(reverse("admin:badges_badgetier_changelist")) + + labels = [row.badge.label for row in response.context["cl"].result_list] + assert labels == sorted(labels) + + +def test_tier_threshold_is_readonly_on_change(client, super_user, badge): + """The change form locks rank/threshold so the record can't be rewritten.""" + bronze = badge.tiers.get(rank=TierRank.BRONZE) + client.force_login(super_user) + url = reverse("admin:badges_badgetier_change", args=[bronze.pk]) + + response = client.get(url) + form_fields = response.context["adminform"].form.fields + assert "threshold" not in form_fields + assert "rank" not in form_fields + + +def test_revoke_action_does_not_touch_achievements( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Direct badge revocation must not alter UserAchievement rows.""" + grant_achievement(plain_user, achievement, count=1) + bronze = UserBadge.objects.get( + user=plain_user, badge=badge, tier__rank=TierRank.BRONZE + ) + client.force_login(super_user) + url = reverse("admin:badges_userbadge_changelist") + + client.post( + url, + { + "action": "revoke", + helpers.ACTION_CHECKBOX_NAME: [bronze.pk], + "apply": "1", + "notes": "x", + }, + ) + + assert ( + UserAchievement.objects.filter( + user=plain_user, achievement=achievement, is_valid=True + ).count() + == 1 + ) + + +@pytest.mark.parametrize( + "url_name", + ["admin:badges_achievement_delete", "admin:badges_badge_delete"], +) +def test_configuration_rows_cannot_be_deleted( + client, super_user, badge, achievement, url_name +): + """Deleting a type or a badge destroys grants or dead-ends on PROTECT.""" + client.force_login(super_user) + target = achievement if "achievement" in url_name else badge + + response = client.get(reverse(url_name, args=[target.pk])) + + assert response.status_code == 403 + + +def test_awarded_rows_cannot_be_added_or_hard_deleted( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """UserBadge is derived, and both audit tables soft-delete by design.""" + grant_achievement(plain_user, achievement, count=1) + awarded = UserBadge.objects.get(user=plain_user) + grant = UserAchievement.objects.get(user=plain_user) + client.force_login(super_user) + + assert client.get(reverse("admin:badges_userbadge_add")).status_code == 403 + assert ( + client.get( + reverse("admin:badges_userbadge_delete", args=[awarded.pk]) + ).status_code + == 403 + ) + assert ( + client.get( + reverse("admin:badges_userachievement_delete", args=[grant.pk]) + ).status_code + == 403 + ) + + +def test_userachievement_add_form_collects_only_the_grant(client, super_user): + """The add form must not offer the fields save_model overrides.""" + client.force_login(super_user) + + response = client.get(reverse("admin:badges_userachievement_add")) + + assert list(response.context["adminform"].form.fields) == [ + "user", + "achievement", + "grant_notes", + ] + + +def test_manual_grant_requires_a_note(client, super_user, plain_user, achievement): + """A grant with no source must say why it exists, or it does not exist. + + The asymmetry this closes: invalidating an achievement has always demanded a + note, while granting one by hand demanded nothing - and the manual grant is the + case with no source row to explain it. + """ + client.force_login(super_user) + + response = client.post( + reverse("admin:badges_userachievement_add"), + {"user": plain_user.pk, "achievement": achievement.pk, "grant_notes": " "}, + ) + + assert response.status_code == 200 + assert "grant_notes" in response.context["adminform"].form.errors + assert not UserAchievement.objects.filter(user=plain_user).exists() + + +def test_manual_grant_records_the_note(client, super_user, plain_user, achievement): + """The note is stored on the row alongside the admin who typed it.""" + client.force_login(super_user) + + client.post( + reverse("admin:badges_userachievement_add"), + { + "user": plain_user.pk, + "achievement": achievement.pk, + "grant_notes": "Chaired the Boost.Asio review, which no source sees.", + }, + ) + + grant = UserAchievement.objects.get(user=plain_user) + assert grant.grant_notes == "Chaired the Boost.Asio review, which no source sees." + assert grant.source_type == SourceType.MANUAL + assert grant.granted_by == super_user + + +def test_changelist_shows_the_note_truncated( + client, super_user, plain_user, achievement, grant_achievement +): + """The reason is readable from the changelist, without opening the row.""" + grant = grant_achievement(plain_user, achievement, count=1)[0] + grant.grant_notes = "Ran the release train " + "for a very long time " * 10 + grant.save(update_fields=["grant_notes"]) + client.force_login(super_user) + + body = client.get( + reverse("admin:badges_userachievement_changelist") + ).content.decode() + + assert "Ran the release train" in body + assert grant.grant_notes not in body + + +def test_a_note_is_searchable( + client, super_user, plain_user, achievement, grant_achievement +): + """Finding every grant made for one reason is a search, not a scroll.""" + kept, other = grant_achievement(plain_user, achievement, count=2) + kept.grant_notes = "Compensating for the reassigned commits" + kept.save(update_fields=["grant_notes"]) + client.force_login(super_user) + + response = client.get( + reverse("admin:badges_userachievement_changelist"), {"q": "reassigned commits"} + ) + + assert [row.pk for row in response.context["cl"].result_list] == [kept.pk] + assert other.pk not in [row.pk for row in response.context["cl"].result_list] + + +def test_existing_rows_are_read_only( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """A grant and a badge are records: state changes go through the actions. + + ``grant_notes`` is the single exception, and is asserted exactly rather than + loosely: rewording a reason moves no badge, but anything *else* becoming + editable here does, so the list stays pinned. + """ + grant_achievement(plain_user, achievement, count=1) + grant = UserAchievement.objects.get(user=plain_user) + awarded = UserBadge.objects.get(user=plain_user) + client.force_login(super_user) + + for url, editable in ( + ( + reverse("admin:badges_userachievement_change", args=[grant.pk]), + ["grant_notes"], + ), + (reverse("admin:badges_userbadge_change", args=[awarded.pk]), []), + ): + response = client.get(url) + assert response.status_code == 200 + assert list(response.context["adminform"].form.fields) == editable + + +def test_badge_achievement_is_frozen_after_creation(client, super_user, badge): + """Repointing a badge would orphan every UserBadge awarded against it.""" + client.force_login(super_user) + + response = client.get(reverse("admin:badges_badge_change", args=[badge.pk])) + + assert "achievement" not in response.context["adminform"].form.fields + + +@pytest.mark.parametrize( + "changelist,url_name,task", + [ + ( + "admin:badges_userachievement_changelist", + "admin:badges_userachievement_backfill", + "backfill_achievements_task", + ), + ( + "admin:badges_userbadge_changelist", + "admin:badges_userbadge_recalculate", + "recalculate_all_badges_task", + ), + ], +) +def test_each_changelist_offers_its_own_task_button( + client, super_user, changelist, url_name, task +): + """The wiring: this changelist offers this button, which starts this task. + + Everything the button does *as* a button - POST only, permission-gated, one + click one job, the status of the last run - belongs to the mixin and is + covered in ``core/tests/test_admin_buttons.py``. + """ + client.force_login(super_user) + url = reverse(url_name) + + assert url.encode() in client.get(reverse(changelist)).content + + with patch(f"badges.admin.{task}.delay") as mock_delay: + response = client.post(url) + + # Which arguments travel with the job is asserted where they matter: the + # actor in the sync log tests, the scope below. + mock_delay.assert_called_once() + assert response.status_code == 302 + + +def test_user_summary_page_renders(client, super_user, plain_user, catalogue): + """Every achievement type is answered for, whether or not it was earned.""" + client.force_login(super_user) + + response = client.get( + reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + ) + + assert response.status_code == 200 + body = response.content.decode() + for name in Achievement.objects.values_list("name", flat=True): + assert name in body + + +def test_user_summary_explains_a_mixed_state( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """A held tier, a revoked tier and the gap to the next one, on one page.""" + grant_achievement(plain_user, achievement, count=3) + silver = UserBadge.objects.get(user=plain_user, tier__rank=TierRank.SILVER) + silver.revoked_at = timezone.now() + silver.revoked_by = super_user + silver.revocation_notes = "Duplicate reviews." + silver.revocation_source = RevocationSource.MANUAL + silver.save() + client.force_login(super_user) + + response = client.get( + reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + ) + + body = response.content.decode() + assert "Held since" in body + # The unreached tier and the gap to it, which is arithmetic anywhere else. + assert "Gold (≥ 5)" in body + assert "2 to go" in body + # ISO, matching the dates the state text builds in Python. + assert f"Silver revoked {timezone.localdate()} (Manual)" in body + + +def test_user_summary_explains_each_action_separately( + client, super_user, plain_user, catalogue +): + """Every action carries its own help text, not one paragraph for all three. + + Recalculate and Reconcile differ only in whether they touch achievements at + all, which is not a distinction either label makes. + """ + client.force_login(super_user) + + body = client.get( + reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + ).content.decode() + + assert body.count('class="submit-row-action"') == 3 + assert body.count('

') == 3 + + +def test_user_summary_links_to_the_member_s_own_admin_page( + client, super_user, plain_user, catalogue +): + """Support arrives from a badge and leaves needing the account behind it. + + The page answers "why this badge", never "who is this": the email to reply to + and whether the account is even active are one click away on the user page, + which was otherwise reachable only by retyping the id into another changelist. + """ + client.force_login(super_user) + + body = client.get( + reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + ).content.decode() + + assert reverse("admin:users_user_change", args=[plain_user.pk]) in body + + +def test_user_summary_hides_the_account_link_from_staff_who_cannot_open_it( + client, db, plain_user, catalogue +): + """A control the caller cannot use is not offered, as with the task buttons. + + Badge permissions and user permissions are granted separately, so a support + account can legitimately reach this page and get a 403 from that link. + """ + staff = baker.make("users.User", email="no-user-perm@example.com", is_staff=True) + for codename in ("view_userbadge", "view_userachievement"): + staff.user_permissions.add( + Permission.objects.get(codename=codename, content_type__app_label="badges") + ) + client.force_login(staff) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + body = client.get(url).content.decode() + + assert reverse("admin:users_user_change", args=[plain_user.pk]) not in body + + staff.user_permissions.add( + Permission.objects.get(codename="view_user", content_type__app_label="users") + ) + body = client.get(url).content.decode() + + assert reverse("admin:users_user_change", args=[plain_user.pk]) in body + + +def test_user_summary_requires_view_permission_on_both_models( + client, db, plain_user, catalogue +): + """The page shows grants as well as badges, so staff alone is not enough.""" + staff = baker.make("users.User", email="summary-staff@example.com", is_staff=True) + client.force_login(staff) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + assert client.get(url).status_code == 403 + + staff.user_permissions.add( + Permission.objects.get( + codename="view_userbadge", content_type__app_label="badges" + ) + ) + assert client.get(url).status_code == 403 + + staff.user_permissions.add( + Permission.objects.get( + codename="view_userachievement", content_type__app_label="badges" + ) + ) + assert client.get(url).status_code == 200 + + +def test_user_summary_recalculate_requires_post( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """A link prefetch or a restored tab must not rewrite badge state.""" + grant_achievement(plain_user, achievement, count=1) + user_badge = UserBadge.objects.get(user=plain_user) + # A bulk delete: no post_delete receivers, so the badge is left stale. + UserAchievement.objects.filter(user=plain_user)._raw_delete(using="default") + client.force_login(super_user) + + client.get(reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk])) + + user_badge.refresh_from_db() + assert user_badge.revoked_at is None + + +def test_user_summary_recalculate_fixes_a_stale_badge( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """The button reconciles this member without touching the whole table.""" + grant_achievement(plain_user, achievement, count=1) + user_badge = UserBadge.objects.get(user=plain_user) + UserAchievement.objects.filter(user=plain_user)._raw_delete(using="default") + client.force_login(super_user) + + response = client.post( + reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]), + follow=True, + ) + + user_badge.refresh_from_db() + assert user_badge.revoked_at is not None + assert "Recalculated 1 achievement type(s)" in response.content.decode() + + +def test_user_summary_recalculate_requires_change_permission( + client, db, plain_user, badge, achievement, grant_achievement +): + """Reading the page is not authorisation to rewrite badge state.""" + grant_achievement(plain_user, achievement, count=1) + user_badge = UserBadge.objects.get(user=plain_user) + UserAchievement.objects.filter(user=plain_user)._raw_delete(using="default") + staff = baker.make("users.User", email="viewer@example.com", is_staff=True) + staff.user_permissions.set( + Permission.objects.filter( + codename__in=["view_userbadge", "view_userachievement"], + content_type__app_label="badges", + ) + ) + client.force_login(staff) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + assert client.get(url).context["can_recalculate"] is False + assert client.post(url).status_code == 403 + + user_badge.refresh_from_db() + assert user_badge.revoked_at is None + + +def test_user_summary_grant_link_prefills_the_user( + client, super_user, plain_user, catalogue +): + """The grant form lands with the member already chosen.""" + client.force_login(super_user) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + grant_url = client.get(url).context["grant_url"] + + assert grant_url.endswith(f"?user={plain_user.pk}") + response = client.get(grant_url) + assert response.status_code == 200 + assert response.context["adminform"].form.initial["user"] == str(plain_user.pk) + + +def test_user_summary_grant_counts_link_to_the_filtered_changelist( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """The valid-grant count is a way in to the rows behind it.""" + grant_achievement(plain_user, achievement, count=2) + client.force_login(super_user) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + grants_url = client.get(url).context["rows"][0]["grants_url"] + + response = client.get(grants_url) + assert response.status_code == 200 + assert response.context["cl"].result_count == 2 + + +def test_user_summary_404s_for_an_unknown_member(client, super_user, db): + """A stale link is a 404, not a crash.""" + client.force_login(super_user) + + response = client.get(reverse("admin:badges_userbadge_user_summary", args=[9999])) + + assert response.status_code == 404 + + +def test_user_admin_links_to_the_badge_summary(client, super_user, plain_user): + """The user record is where support starts, so the way in is from there.""" + client.force_login(super_user) + + response = client.get(reverse("admin:users_user_change", args=[plain_user.pk])) + + assert ( + reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + in response.content.decode() + ) + + +def test_user_admin_add_form_has_no_dead_badge_link(client, super_user): + """There is nothing to summarise before the user exists.""" + client.force_login(super_user) + + response = client.get(reverse("admin:users_user_add")) + + assert response.status_code == 200 + assert "user-summary" not in response.content.decode() + + +@pytest.mark.parametrize( + "url_name", + [ + "admin:badges_userbadge_changelist", + "admin:badges_userachievement_changelist", + ], +) +def test_changelists_link_the_member_to_their_summary( + client, super_user, plain_user, badge, achievement, grant_achievement, url_name +): + """Both changelists reach the per-user page through the user column.""" + grant_achievement(plain_user, achievement, count=1) + client.force_login(super_user) + + response = client.get(reverse(url_name)) + + body = response.content.decode() + assert reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) in body + # The row itself is still reachable through the changelist's first column. + assert "field-user_link" in body + + +RECONCILE_URL = "admin:badges_userachievement_reconcile" +RECONCILE_TASK = "badges.admin.reconcile_achievements_task.delay" + + +def _staff_with(email, *codenames): + """A staff account holding exactly the named badges permissions.""" + staff = baker.make("users.User", email=email, is_staff=True) + staff.user_permissions.set( + Permission.objects.filter( + codename__in=codenames, content_type__app_label="badges" + ) + ) + return staff + + +def _stale_grant_for(user): + """Give ``user`` a commits achievement and then break its attribution.""" + author = baker.make("libraries.CommitAuthor", user=user) + baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + author.user = None + author.save() + + +def test_reconcile_button_previews_before_deleting_anything( + client, super_user, plain_user, commit_by_someone_else, stale_commit_grant +): + """The first POST is a dry run rendered as a page, not a job.""" + client.force_login(super_user) + + response = client.post(reverse(RECONCILE_URL)) + + body = response.content.decode() + assert response.status_code == 200 + assert "would remove 1 grant(s)" in body + assert 'name="apply"' in body + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + assert UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + + +def test_reconcile_button_enqueues_the_task_only_on_apply( + client, super_user, plain_user, commit_by_someone_else, stale_commit_grant +): + """The preview's own submit is what starts the job.""" + client.force_login(super_user) + + with patch(RECONCILE_TASK, return_value=Mock(id="a-task-id")) as delay: + response = client.post(reverse(RECONCILE_URL), {"apply": "1"}, follow=True) + + delay.assert_called_once_with(actor_id=super_user.pk) + assert "being reconciled with their sources" in response.content.decode() + + +def test_reconcile_preview_carries_the_chosen_source_into_the_apply( + client, super_user, plain_user, commit_by_someone_else, stale_commit_grant +): + """A scoped preview must apply the same scope it previewed.""" + client.force_login(super_user) + + response = client.post(reverse(RECONCILE_URL), {"slug": "code-commits"}) + assert 'value="code-commits"' in response.content.decode() + + with patch(RECONCILE_TASK, return_value=Mock(id="a-task-id")) as delay: + client.post(reverse(RECONCILE_URL), {"slug": "code-commits", "apply": "1"}) + + delay.assert_called_once_with(slug="code-commits", actor_id=super_user.pk) + + +def test_reconcile_preview_offers_no_apply_when_everything_agrees( + client, super_user, plain_user, commit_by_someone_else +): + """A preview with nothing to do is not a decision worth offering.""" + client.force_login(super_user) + + response = client.post(reverse(RECONCILE_URL)) + + body = response.content.decode() + assert "Nothing to reconcile" in body + assert 'name="apply"' not in body + + +def test_reconcile_preview_reports_grants_it_would_create( + client, super_user, plain_user, commit_by_someone_else +): + """The additive half shows up in the preview too, not only the destructive one.""" + baker.make( + "libraries.Commit", author=baker.make("libraries.CommitAuthor", user=plain_user) + ) + client.force_login(super_user) + + response = client.post(reverse(RECONCILE_URL), {"slug": "code-commits"}) + + body = response.content.decode() + assert "would add 1 grant(s)" in body + assert 'name="apply"' in body + assert not UserAchievement.objects.filter(user=plain_user).exists() + + +def test_member_reconcile_restores_a_grant_the_source_supports_again( + client, super_user, plain_user, commit_by_someone_else, stale_commit_grant +): + """Unbind the author, reconcile, rebind, reconcile: the badge comes back. + + The step that a one-directional prune left no way to take, and the reason the + per-member control is two-way. + """ + client.force_login(super_user) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + client.post(url, {"action": "reconcile", "apply": "1"}) + assert not UserAchievement.objects.filter(user=plain_user).exists() + bronze = UserBadge.objects.get(user=plain_user, tier__rank="bronze") + assert bronze.revocation_source == RevocationSource.CASCADE + + author = stale_commit_grant.author + author.user = plain_user + author.save() + + response = client.post(url, {"action": "reconcile", "apply": "1"}, follow=True) + + assert "added 1 and removed 0" in response.content.decode() + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + bronze.refresh_from_db() + assert bronze.revoked_at is None + + +def test_reconcile_preview_refuses_an_empty_source( + client, super_user, plain_user, stale_commit_grant +): + """An empty source is reported, explained, and not offered as applyable.""" + client.force_login(super_user) + + response = client.post(reverse(RECONCILE_URL), {"slug": "code-commits"}) + + body = response.content.decode() + assert "REFUSED" in body + assert "--allow-empty" in body + assert 'name="apply"' not in body + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + + +def test_reconcile_button_needs_more_than_the_change_permission( + client, plain_user, commit_by_someone_else, stale_commit_grant +): + """Deleting achievements is not something ``change`` authorises.""" + staff = _staff_with( + "changer@example.com", "view_userachievement", "change_userachievement" + ) + client.force_login(staff) + + body = client.get( + reverse("admin:badges_userachievement_changelist") + ).content.decode() + assert "Backfill achievements" in body + assert "Reconcile achievements" not in body + + assert client.post(reverse(RECONCILE_URL)).status_code == 403 + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + + +def test_reconcile_status_endpoint_needs_the_delete_permission(client, db): + """The state of a job is not offered to someone who cannot start it.""" + staff = _staff_with( + "status-changer@example.com", "view_userachievement", "change_userachievement" + ) + client.force_login(staff) + + assert client.get(reverse(f"{RECONCILE_URL}_status")).status_code == 403 + + +def test_member_page_offers_a_reconcile_preview( + client, super_user, plain_user, commit_by_someone_else, stale_commit_grant +): + """The per-member control previews exactly the way the changelist one does.""" + client.force_login(super_user) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + assert client.get(url).context["can_reconcile"] is True + + response = client.post(url, {"action": "reconcile"}) + + body = response.content.decode() + assert "would remove 1 grant(s)" in body + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + + +def test_member_reconcile_applies_and_cascades_the_badge( + client, super_user, plain_user, commit_by_someone_else, stale_commit_grant +): + """The second POST does the work synchronously and reports what it did.""" + client.force_login(super_user) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + response = client.post(url, {"action": "reconcile", "apply": "1"}, follow=True) + + assert "added 0 and removed 1" in response.content.decode() + assert not UserAchievement.objects.filter(user=plain_user).exists() + assert not UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + + +def test_member_reconcile_leaves_every_other_member_alone( + client, super_user, plain_user, commit_by_someone_else, stale_commit_grant +): + """It is the page for one member, so it is a run for one member.""" + other = baker.make("users.User", email="other-stale@example.com") + _stale_grant_for(other) + client.force_login(super_user) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + client.post(url, {"action": "reconcile", "apply": "1"}) + + assert not UserAchievement.objects.filter(user=plain_user).exists() + assert UserAchievement.objects.filter(user=other).exists() + + +def test_member_reconcile_needs_the_delete_permission( + client, plain_user, commit_by_someone_else, stale_commit_grant +): + """Change permission runs the recalculation on this page, not the deletion.""" + staff = _staff_with( + "member-changer@example.com", + "view_userbadge", + "view_userachievement", + "change_userbadge", + ) + client.force_login(staff) + url = reverse("admin:badges_userbadge_user_summary", args=[plain_user.pk]) + + assert client.get(url).context["can_reconcile"] is False + assert client.post(url, {"action": "reconcile"}).status_code == 403 + assert UserAchievement.objects.filter(user=plain_user).count() == 1 diff --git a/badges/tests/test_admin_badge_config.py b/badges/tests/test_admin_badge_config.py new file mode 100644 index 000000000..159dc3cb3 --- /dev/null +++ b/badges/tests/test_admin_badge_config.py @@ -0,0 +1,624 @@ +"""Tests for the badge page as the single tier-configuration surface.""" + +import pytest +from django.contrib.admin.sites import AdminSite +from django.urls import reverse +from django.utils import timezone +from model_bakery import baker + +from badges.admin import AchievementAdmin, BadgeAdmin +from badges.enums import TierRank +from badges.models import Achievement, Badge, BadgeTier, UserBadge +from badges.services import deactivate_tier + +pytestmark = pytest.mark.django_db + +PREFIX = "tiers" + + +def _row(tier, threshold=None, rank=None, delete=False): + """One bound inline row for an existing tier, optionally edited.""" + row = { + "id": str(tier.pk), + "rank": rank or tier.rank, + "threshold": str(tier.threshold if threshold is None else threshold), + } + if delete: + row["DELETE"] = "on" + return row + + +def _post_ladder(client, badge, rows): + """POST the badge change form with ``rows`` as its complete ladder.""" + data = { + "label": badge.label, + "description": badge.description, + f"{PREFIX}-TOTAL_FORMS": str(len(rows)), + f"{PREFIX}-INITIAL_FORMS": str(sum(1 for row in rows if row.get("id"))), + f"{PREFIX}-MIN_NUM_FORMS": "0", + f"{PREFIX}-MAX_NUM_FORMS": str(len(TierRank)), + } + for index, row in enumerate(rows): + for name, value in row.items(): + data[f"{PREFIX}-{index}-{name}"] = value + return client.post( + reverse("admin:badges_badge_change", args=[badge.pk]), data, follow=True + ) + + +def _ladder_rows(badge): + """Every tier of a badge as ``(rank, threshold, is_active)``.""" + return sorted( + badge.tiers.values_list("rank", "threshold", "is_active"), + key=lambda row: (TierRank(row[0]).order, row[1]), + ) + + +def test_editing_a_threshold_retires_and_replaces_the_tier( + client, super_user, badge, achievement +): + """An in-place update would revoke the members who met the old threshold.""" + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + client.force_login(super_user) + + _post_ladder(client, badge, [_row(bronze, threshold=2), _row(silver), _row(gold)]) + + bronze.refresh_from_db() + assert bronze.threshold == 1 + assert bronze.is_active is False + assert bronze.deactivated_by == super_user + assert bronze.deactivated_at is not None + replacement = badge.tiers.get(rank=TierRank.BRONZE, is_active=True) + assert replacement.threshold == 2 + assert replacement.pk != bronze.pk + + +def test_editing_a_threshold_keeps_existing_holders( + client, + super_user, + plain_user, + badge, + achievement, + grant_achievement, + django_capture_on_commit_callbacks, +): + """The grandfathering guard, exercised through the form an admin uses.""" + grant_achievement(plain_user, achievement, count=5) + gold = badge.tiers.get(rank=TierRank.GOLD) + held = UserBadge.objects.get(user=plain_user, tier=gold) + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + client.force_login(super_user) + + with django_capture_on_commit_callbacks(execute=True): + _post_ladder( + client, badge, [_row(bronze), _row(silver), _row(gold, threshold=10)] + ) + + held.refresh_from_db() + assert held.revoked_at is None + + +def test_editing_a_threshold_awards_newly_qualifying_members( + client, + super_user, + plain_user, + badge, + achievement, + grant_achievement, + django_capture_on_commit_callbacks, +): + """Lowering a threshold takes effect without anyone running a sweep.""" + grant_achievement(plain_user, achievement, count=3) + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + assert not UserBadge.objects.filter(user=plain_user, tier=gold).exists() + client.force_login(super_user) + + # Silver comes down with gold: the ladder has to stay ordered as submitted. + with django_capture_on_commit_callbacks(execute=True): + _post_ladder( + client, + badge, + [_row(bronze), _row(silver, threshold=2), _row(gold, threshold=3)], + ) + + awarded = UserBadge.objects.get( + user=plain_user, tier__rank=TierRank.GOLD, tier__is_active=True + ) + assert awarded.revoked_at is None + + +def test_a_threshold_out_of_ladder_order_is_refused( + client, super_user, badge, achievement +): + """Silver may not be dragged onto bronze, and nothing is written when it is.""" + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + client.force_login(super_user) + + response = _post_ladder( + client, badge, [_row(bronze), _row(silver, threshold=1), _row(gold)] + ) + + assert "threshold of the rank below this one" in response.content.decode() + assert _ladder_rows(badge) == [ + (TierRank.BRONZE.value, 1, True), + (TierRank.SILVER.value, 3, True), + (TierRank.GOLD.value, 5, True), + ] + + +def test_shifting_the_whole_ladder_up_is_accepted( + client, super_user, badge, achievement +): + """Every rung moves at once, so each one briefly collides with a stored value. + + Judged against the submitted ladder this is legal, and it is the edit staff + actually make when a badge turns out to be too easy across the board. + """ + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + client.force_login(super_user) + + _post_ladder( + client, + badge, + [ + _row(bronze, threshold=5), + _row(silver, threshold=10), + _row(gold, threshold=15), + ], + ) + + assert sorted( + badge.tiers.filter(is_active=True).values_list("rank", "threshold"), + key=lambda row: TierRank(row[0]).order, + ) == [ + (TierRank.BRONZE.value, 5), + (TierRank.SILVER.value, 10), + (TierRank.GOLD.value, 15), + ] + + +def test_deleting_a_tier_row_in_the_inline_retires_it( + client, super_user, badge, achievement +): + """Removing a row is a retirement; the UserBadge rows behind it survive.""" + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + client.force_login(super_user) + + _post_ladder(client, badge, [_row(bronze), _row(silver, delete=True), _row(gold)]) + + silver.refresh_from_db() + assert silver.is_active is False + assert silver.deactivated_by == super_user + assert badge.tiers.count() == 3 + + +def test_retiring_a_tier_the_members_have_earned_is_not_refused( + client, + super_user, + plain_user, + badge, + achievement, + grant_achievement, + django_capture_on_commit_callbacks, +): + """The inline's own delete check would refuse this, and name every holder. + + Django validates a ticked delete by collecting what the row protects, and + ``UserBadge.tier`` protects it, so the only retirement it lets through is one + nobody has earned - the case that needs no protecting. The retirement itself + is what preserves those holders. + """ + grant_achievement(plain_user, achievement, count=5) + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + held = UserBadge.objects.get(user=plain_user, tier=bronze) + client.force_login(super_user) + + with django_capture_on_commit_callbacks(execute=True): + response = _post_ladder( + client, badge, [_row(bronze, delete=True), _row(silver), _row(gold)] + ) + + assert "protected related objects" not in response.content.decode() + bronze.refresh_from_db() + assert bronze.is_active is False + assert bronze.deactivated_by == super_user + held.refresh_from_db() + assert held.revoked_at is None + + +def test_adding_a_tier_row_in_the_inline_creates_and_awards_it( + client, + super_user, + plain_user, + badge, + achievement, + grant_achievement, + django_capture_on_commit_callbacks, +): + """A whole ladder is one page: a fourth rank needs no second changelist.""" + grant_achievement(plain_user, achievement, count=10) + rows = [_row(tier) for tier in badge.tiers.order_by("threshold")] + rows.append({"id": "", "rank": TierRank.PLATINUM, "threshold": "10"}) + client.force_login(super_user) + + with django_capture_on_commit_callbacks(execute=True): + _post_ladder(client, badge, rows) + + platinum = badge.tiers.get(rank=TierRank.PLATINUM) + assert platinum.is_active is True + assert platinum.threshold == 10 + assert UserBadge.objects.filter( + user=plain_user, tier=platinum, revoked_at=None + ).exists() + + +def test_two_new_sibling_tiers_cannot_submit_the_same_rank( + client, super_user, badge, achievement +): + """Cross-form validation returns the page instead of a constraint error.""" + rows = [_row(tier) for tier in badge.tiers.order_by("threshold")] + rows.extend( + [ + {"id": "", "rank": TierRank.DIAMOND, "threshold": "7"}, + {"id": "", "rank": TierRank.DIAMOND, "threshold": "9"}, + ] + ) + client.force_login(super_user) + + response = _post_ladder(client, badge, rows) + + assert response.status_code == 200 + assert "Only one active Diamond tier is allowed for a badge." in ( + response.content.decode() + ) + assert not badge.tiers.filter(rank=TierRank.DIAMOND).exists() + + +def test_delete_and_readd_same_rank_stays_rejected( + client, super_user, badge, achievement +): + """Replacing a rank uses an edit; delete-and-readd remains unsupported.""" + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + rows = [ + _row(bronze), + _row(silver, delete=True), + _row(gold), + {"id": "", "rank": TierRank.SILVER, "threshold": "4"}, + ] + client.force_login(super_user) + + response = _post_ladder(client, badge, rows) + + assert response.status_code == 200 + assert "An active Silver tier already exists for this badge." in ( + response.content.decode() + ) + silver.refresh_from_db() + assert silver.is_active is True + assert list(badge.tiers.filter(rank=TierRank.SILVER)) == [silver] + + +def test_a_badge_and_its_whole_ladder_are_one_save(client, super_user, achievement): + """Creating a badge from scratch must not need a second page.""" + client.force_login(super_user) + rows = [ + {"id": "", "rank": rank, "threshold": str(index + 1)} + for index, rank in enumerate(TierRank) + ] + data = { + "label": "documenter", + "achievement": str(achievement.pk), + "description": "Docs.", + f"{PREFIX}-TOTAL_FORMS": str(len(rows)), + f"{PREFIX}-INITIAL_FORMS": "0", + f"{PREFIX}-MIN_NUM_FORMS": "0", + f"{PREFIX}-MAX_NUM_FORMS": str(len(TierRank)), + } + for index, row in enumerate(rows): + for name, value in row.items(): + data[f"{PREFIX}-{index}-{name}"] = value + + client.post(reverse("admin:badges_badge_add"), data, follow=True) + + tiers = BadgeTier.objects.filter(badge__label="documenter") + assert tiers.count() == len(TierRank) + assert set(tiers.values_list("is_active", flat=True)) == {True} + + +def test_the_replacement_message_names_both_tiers( + client, super_user, badge, achievement +): + """ "Bronze was changed" is not what happened, and the difference matters.""" + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + client.force_login(super_user) + + response = _post_ladder( + client, badge, [_row(bronze, threshold=2), _row(silver), _row(gold)] + ) + + body = response.content.decode() + assert "Retired Bronze (>= 1) and created Bronze (>= 2)" in body + assert "Members who already earned Bronze keep it" in body + + +def test_the_retirement_message_says_the_tier_was_not_deleted( + client, super_user, badge, achievement +): + """The row survives, and so do the badges awarded against it.""" + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + client.force_login(super_user) + + response = _post_ladder( + client, badge, [_row(bronze), _row(silver, delete=True), _row(gold)] + ) + + assert "Retired Silver (>= 3)" in response.content.decode() + + +def test_the_threshold_column_explains_that_changes_are_not_retroactive( + client, super_user, badge, achievement +): + """Tabular inlines render field help text once, as the column tooltip.""" + client.force_login(super_user) + + response = client.get(reverse("admin:badges_badge_change", args=[badge.pk])) + + assert "members who already reached the old threshold keep" in ( + response.content.decode() + ) + + +def test_the_inline_shows_the_live_ladder_in_rank_order( + client, super_user, badge, achievement +): + """Retired tiers stay out of the way, and bronze reads before diamond.""" + deactivate_tier(badge.tiers.get(rank=TierRank.SILVER)) + # Thresholds deliberately out of ladder order, so an ordering by threshold + # alone would put diamond first. + baker.make(BadgeTier, badge=badge, rank=TierRank.DIAMOND, threshold=2) + client.force_login(super_user) + + response = client.get(reverse("admin:badges_badge_change", args=[badge.pk])) + + formset = response.context["inline_admin_formsets"][0].formset + assert [form.instance.rank for form in formset.initial_forms] == [ + TierRank.BRONZE, + TierRank.GOLD, + TierRank.DIAMOND, + ] + + +def test_the_badge_page_links_its_retired_tiers(client, super_user, badge, achievement): + """Retired rows stay reachable without cluttering the live ladder.""" + deactivate_tier(badge.tiers.get(rank=TierRank.SILVER)) + client.force_login(super_user) + + response = client.get(reverse("admin:badges_badge_change", args=[badge.pk])) + + expected = ( + f"{reverse('admin:badges_badgetier_changelist')}" + f"?is_active__exact=0&badge__id__exact={badge.pk}" + ) + assert expected in response.content.decode() + + +def test_the_badge_page_says_so_when_nothing_is_retired( + client, super_user, badge, achievement +): + """A link to an empty changelist would read as a broken one.""" + client.force_login(super_user) + + response = client.get(reverse("admin:badges_badge_change", args=[badge.pk])) + + body = response.content.decode() + assert "is_active__exact=0" not in body + assert "None." in body + + +def test_the_retired_tier_link_filters_to_one_badge( + client, super_user, badge, achievement +): + """The filtered changelist has to accept both lookups, not redirect to ?e=1.""" + retired = badge.tiers.get(rank=TierRank.SILVER) + deactivate_tier(retired) + other = baker.make("badges.Badge", label="documenter", achievement=achievement) + deactivate_tier( + baker.make(BadgeTier, badge=other, rank=TierRank.BRONZE, threshold=1) + ) + client.force_login(super_user) + + response = client.get( + f"{reverse('admin:badges_badgetier_changelist')}" + f"?is_active__exact=0&badge__id__exact={badge.pk}" + ) + + assert response.status_code == 200 + assert set(response.context["cl"].queryset.values_list("pk", flat=True)) == { + retired.pk + } + + +def _changelist_rows(client): + """Badge changelist rows keyed by label, with the health columns prefetched.""" + response = client.get(reverse("admin:badges_badge_changelist")) + assert response.status_code == 200 + return {row.label: row for row in response.context["cl"].result_list} + + +def test_the_changelist_shows_the_ladder_in_rank_order( + client, super_user, badge, achievement +): + """Thresholds do not have to ascend with rank, so ordering by them lies.""" + baker.make(BadgeTier, badge=badge, rank=TierRank.DIAMOND, threshold=2) + client.force_login(super_user) + + rows = _changelist_rows(client) + + assert BadgeAdmin(Badge, AdminSite()).ladder(rows[badge.label]) == "1 / 3 / 5 / 2" + + +def test_the_changelist_flags_a_badge_that_can_never_award( + client, super_user, achievement +): + """A badge with no active tiers looks complete and does nothing.""" + baker.make("badges.Badge", label="documenter", achievement=achievement) + client.force_login(super_user) + + rows = _changelist_rows(client) + + assert BadgeAdmin(Badge, AdminSite()).ladder(rows["documenter"]) == ( + "No tiers - awards nothing" + ) + + +def test_the_changelist_flags_an_unwired_source(client, super_user, catalogue): + """Two of the eight badges only ever move on a manual grant.""" + client.force_login(super_user) + admin_class = BadgeAdmin(Badge, AdminSite()) + + rows = _changelist_rows(client) + + assert admin_class.source_wired(rows["commits_master"]) is True + assert admin_class.source_wired(rows["documenter"]) is False + assert admin_class.source_wired(rows["regular"]) is False + + +def test_the_changelist_counts_each_holder_once_and_skips_revocations( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Three tiers held by one member is one holder, and a revoked badge is none.""" + grant_achievement(plain_user, achievement, count=5) # bronze + silver + gold + other = baker.make("users.User", email="revoked-holder@example.com") + grant_achievement(other, achievement, count=1) + UserBadge.objects.filter(user=other).update(revoked_at=timezone.now()) + client.force_login(super_user) + + rows = _changelist_rows(client) + + assert BadgeAdmin(Badge, AdminSite()).holders(rows[badge.label]) == 1 + + +def _achievement_rows(client): + """Achievement changelist rows keyed by slug.""" + response = client.get(reverse("admin:badges_achievement_changelist")) + assert response.status_code == 200 + return {row.slug: row for row in response.context["cl"].result_list} + + +def test_the_achievement_changelist_names_the_badge_it_feeds( + client, super_user, badge, achievement +): + """Which badge a type drives is otherwise only visible from the badge side.""" + client.force_login(super_user) + + rows = _achievement_rows(client) + + assert AchievementAdmin(Achievement, AdminSite()).badge(rows[achievement.slug]) == ( + "Maintainer" + ) + + +def test_the_achievement_changelist_flags_a_type_with_no_badge( + client, super_user, achievement +): + """Grants against a badgeless type accumulate and can never award.""" + client.force_login(super_user) + + rows = _achievement_rows(client) + + assert AchievementAdmin(Achievement, AdminSite()).badge(rows[achievement.slug]) == ( + "None - awards nothing" + ) + + +def test_the_achievement_changelist_counts_only_valid_grants( + client, super_user, plain_user, badge, achievement, grant_achievement +): + """Thresholds count valid grants, so that is the number worth showing.""" + rows_granted = grant_achievement(plain_user, achievement, count=3) + rows_granted[0].is_valid = False + rows_granted[0].save() + client.force_login(super_user) + + rows = _achievement_rows(client) + + assert ( + AchievementAdmin(Achievement, AdminSite()).grants(rows[achievement.slug]) == 2 + ) + + +def test_badge_tiers_are_hidden_from_the_admin_index(client, super_user, badge): + """One entry point for configuring a ladder, not two.""" + client.force_login(super_user) + + index = client.get(reverse("admin:index")) + changelist = client.get(reverse("admin:badges_badgetier_changelist")) + + listed = [ + model["object_name"] + for app in index.context["app_list"] + if app["app_label"] == "badges" + for model in app["models"] + ] + assert "Badge" in listed + assert "BadgeTier" not in listed + assert changelist.status_code == 200 + + +def test_a_full_ladder_offers_no_blank_row(client, super_user, catalogue): + """The blank row exists because "Add another" needs JS; five ranks is the cap.""" + full = Badge.objects.get(label="commits_master") + client.force_login(super_user) + + response = client.get(reverse("admin:badges_badge_change", args=[full.pk])) + + formset = response.context["inline_admin_formsets"][0].formset + assert len(formset.initial_forms) == len(TierRank) + assert formset.extra_forms == [] + + +def test_the_inline_refuses_a_second_active_tier_for_a_rank( + client, super_user, badge, achievement +): + """The constraint is reported as a form error, not a 500.""" + bronze = badge.tiers.get(rank=TierRank.BRONZE) + silver = badge.tiers.get(rank=TierRank.SILVER) + gold = badge.tiers.get(rank=TierRank.GOLD) + client.force_login(super_user) + + response = _post_ladder( + client, + badge, + [ + _row(bronze), + _row(silver), + _row(gold), + {"id": "", "rank": TierRank.BRONZE, "threshold": "9"}, + ], + ) + + assert "An active Bronze tier already exists" in response.content.decode() + assert _ladder_rows(badge) == [ + (TierRank.BRONZE, 1, True), + (TierRank.SILVER, 3, True), + (TierRank.GOLD, 5, True), + ] diff --git a/badges/tests/test_commands.py b/badges/tests/test_commands.py index ec92892bd..c56969539 100644 --- a/badges/tests/test_commands.py +++ b/badges/tests/test_commands.py @@ -1,11 +1,18 @@ -"""Tests for the recalculate_badges management command.""" +"""Tests for the backfill, recalculate and reconcile management commands.""" + +import re import pytest -from django.core.management import call_command +from django.core.management import call_command, load_command_class +from django.core.management.base import CommandError +from django.utils import timezone +from model_bakery import baker from badges.enums import AchievementSlug from badges.models import ( Achievement, + AchievementSyncRun, + RevocationSource, SourceType, UserAchievement, UserBadge, @@ -19,6 +26,82 @@ def _catalogue(catalogue): """Seed the real achievement catalogue for every test in this module.""" +def test_backfill_grants_achievement_and_badge(plain_user): + """Backfill turns existing source data into achievements and badges.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) # no live signal grants it + assert UserAchievement.objects.count() == 0 + + call_command("backfill_achievements", "--source", "code-commits") + + assert ( + UserAchievement.objects.filter( + user=plain_user, achievement__slug="code-commits" + ).count() + == 1 + ) + assert UserBadge.objects.filter( + user=plain_user, badge__achievement__slug="code-commits" + ).exists() # bronze threshold is 1 + + +def test_backfill_is_idempotent(plain_user): + """Running backfill twice does not create duplicate rows.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + + call_command("backfill_achievements", "--source", "code-commits") + call_command("backfill_achievements", "--source", "code-commits") + + assert ( + UserAchievement.objects.filter( + user=plain_user, achievement__slug="code-commits" + ).count() + == 1 + ) + + +def test_backfill_skips_recalculation_without_new_rows(plain_user): + """A re-run with no new source data does not recalculate badges.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + UserBadge.objects.all().delete() # would be restored by a recalculation + + call_command("backfill_achievements", "--source", "code-commits") + + assert not UserBadge.objects.exists() + + +def test_backfill_fails_loudly_on_an_explicit_unseeded_source(plain_user): + """A named source with no Achievement row is a deploy bug, not a skip.""" + Achievement.objects.filter(slug=AchievementSlug.CODE_COMMITS).delete() + + with pytest.raises(CommandError, match="code-commits"): + call_command("backfill_achievements", "--source", "code-commits") + + +def test_backfill_fails_when_no_source_is_seeded(plain_user): + """Nothing to back fill at all is still worth a non-zero exit.""" + Achievement.objects.all().delete() + + with pytest.raises(CommandError, match="No wired source"): + call_command("backfill_achievements") + + +@pytest.mark.parametrize( + "command_name", ["backfill_achievements", "reconcile_achievements"] +) +@pytest.mark.parametrize("batch_size", ["0", "-1"]) +def test_sync_commands_reject_non_positive_batch_sizes(command_name, batch_size): + """Invalid batch sizes fail in argument parsing, before either command runs.""" + command = load_command_class("badges", command_name) + parser = command.create_parser("manage.py", command_name) + + with pytest.raises(CommandError, match="must be a positive integer"): + parser.parse_args(["--batch-size", batch_size]) + + def _grant(user, slug): """One valid manual grant, which is all a threshold of 1 needs. @@ -77,3 +160,363 @@ def test_recalculate_revokes_badges_whose_achievements_are_gone(plain_user): badge.refresh_from_db() assert badge.revoked_at is not None + + +def test_reconcile_deletes_a_grant_the_source_no_longer_yields( + plain_user, commit_by_someone_else, stale_commit_grant +): + """The case the command exists for: attribution moved, the grant did not.""" + call_command("reconcile_achievements", "--source", "code-commits") + + assert not UserAchievement.objects.filter(user=plain_user).exists() + revoked = UserBadge.objects.get( + user=plain_user, badge__achievement__slug="code-commits", tier__rank="bronze" + ) + assert revoked.revoked_at is not None + assert revoked.revocation_source == "cascade" + + +def test_reconcile_removes_a_grant_whose_source_row_is_gone( + plain_user, commit_by_someone_else, stale_commit_grant +): + """A deleted source row leaves the same dangling grant, and is cleaned up too. + + The generic foreign key carries no referential integrity, so nothing else + notices. ``discard_source_achievements`` covers the callers that delete rows + deliberately; this covers everything that did not. + """ + stale_commit_grant.delete() + + call_command("reconcile_achievements", "--source", "code-commits") + + assert not UserAchievement.objects.filter(user=plain_user).exists() + + +def test_reconcile_leaves_manual_grants_alone( + plain_user, commit_by_someone_else, stale_commit_grant +): + """A manual grant has no source to disagree with, so it must survive.""" + achievement = Achievement.objects.get(slug=AchievementSlug.CODE_COMMITS) + manual = UserAchievement.objects.create( + user=plain_user, achievement=achievement, source_type=SourceType.MANUAL + ) + + call_command("reconcile_achievements", "--source", "code-commits") + + assert UserAchievement.objects.filter(pk=manual.pk).exists() + assert not UserAchievement.objects.filter( + user=plain_user, source_type=SourceType.AUTOMATIC + ).exists() + # The manual grant is still worth one achievement, which is still bronze. + assert UserBadge.objects.filter( + user=plain_user, badge__achievement__slug="code-commits", revoked_at=None + ).exists() + + +def test_reconcile_leaves_an_attributed_grant_alone(plain_user): + """A source that still yields the member changes nothing, however often.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + badge = UserBadge.objects.get(user=plain_user, tier__rank="bronze") + + call_command("reconcile_achievements", "--source", "code-commits") + call_command("reconcile_achievements", "--source", "code-commits") + + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + badge.refresh_from_db() + assert badge.revoked_at is None + + +def test_reconcile_dry_run_writes_nothing( + plain_user, commit_by_someone_else, stale_commit_grant, capsys +): + """A dry run reports the stale grant and leaves it, and the badge, in place.""" + call_command("reconcile_achievements", "--source", "code-commits", "--dry-run") + + output = capsys.readouterr().out + assert "Dry run" in output + assert "would remove 1 grant(s)" in output + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + assert UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + + +def test_reconcile_scopes_to_the_named_member( + plain_user, commit_by_someone_else, stale_commit_grant +): + """``--user`` must not clean up a member it was not pointed at.""" + other = baker.make("users.User", email="other-stale@example.com") + other_author = baker.make("libraries.CommitAuthor", user=other) + baker.make("libraries.Commit", author=other_author) + call_command("backfill_achievements", "--source", "code-commits") + other_author.user = None + other_author.save() + + call_command("reconcile_achievements", "--user", plain_user.email) + + assert not UserAchievement.objects.filter(user=plain_user).exists() + assert UserAchievement.objects.filter(user=other).exists() + + +def test_reconcile_scoped_to_a_member_does_not_add_for_anyone_else( + plain_user, stale_commit_grant +): + """``--user`` bounds the additive half too, not only the deletions. + + A member outside the scope is absent from the stored keys, which on the + additive side looks exactly like a grant that needs creating - so the walk has + to know about the scope as well. + """ + other = baker.make("users.User", email="other-missing@example.com") + baker.make( + "libraries.Commit", author=baker.make("libraries.CommitAuthor", user=other) + ) + + call_command( + "reconcile_achievements", "--source", "code-commits", "--user", plain_user.email + ) + + assert not UserAchievement.objects.filter(user=plain_user).exists() + assert not UserAchievement.objects.filter(user=other).exists() + + +def test_reconcile_accepts_a_member_by_primary_key( + plain_user, commit_by_someone_else, stale_commit_grant +): + """An id is as good as an email, because an admin URL only carries the id.""" + call_command("reconcile_achievements", "--user", str(plain_user.pk)) + + assert not UserAchievement.objects.filter(user=plain_user).exists() + + +def test_reconcile_rejects_an_unknown_member(plain_user): + """A typo in ``--user`` must not silently widen the run to everybody.""" + with pytest.raises(CommandError, match=re.escape("nobody@example.com")): + call_command("reconcile_achievements", "--user", "nobody@example.com") + + +def test_reconcile_refuses_a_source_that_yields_nothing( + plain_user, stale_commit_grant, capsys +): + """An empty source is a broken source until proven otherwise. + + Without this, one failed import would revoke every badge the source feeds. + """ + call_command("reconcile_achievements", "--source", "code-commits") + + captured = capsys.readouterr() + assert "REFUSED" in captured.out + assert "--allow-empty" in captured.err + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + assert UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + + +def test_backfill_grants_a_deactivated_member_nothing(plain_user): + """A deactivated account is skipped however loudly the source names it. + + Deleting an account scrubs its grants but leaves the commits and libraries + behind it whole, so without this the next sweep would award them all back. + """ + plain_user.is_active = False + plain_user.save(update_fields=["is_active"]) + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + + call_command("backfill_achievements", "--source", "code-commits") + + assert not UserAchievement.objects.filter(user=plain_user).exists() + assert not UserBadge.objects.filter(user=plain_user).exists() + + +def test_reconcile_takes_back_what_a_deactivated_member_holds( + plain_user, commit_by_someone_else +): + """Grants earned before deactivation read as stale, and the badge is revoked. + + Which is what cleans up an account deleted before this rule existed, and an + account whose deletion never scrubbed the grants in the first place. + """ + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + badge = UserBadge.objects.get(user=plain_user, tier__rank="bronze") + + plain_user.is_active = False + plain_user.save(update_fields=["is_active"]) + call_command("reconcile_achievements", "--source", "code-commits") + + assert not UserAchievement.objects.filter(user=plain_user).exists() + badge.refresh_from_db() + assert badge.revoked_at is not None + assert badge.revocation_source == "cascade" + + +def test_a_source_naming_only_deactivated_members_is_not_an_empty_source(plain_user): + """The refusal asks whether the source read empty, not who survived it. + + A source that named nobody at all is a broken import. A source that named one + member who has since been deactivated is working exactly as it should, and its + stale grant has to go. + """ + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + plain_user.is_active = False + plain_user.save(update_fields=["is_active"]) + + call_command("reconcile_achievements", "--source", "code-commits") + + run = AchievementSyncRun.objects.first() + assert run.refused is False + assert run.removed == 1 + assert not UserAchievement.objects.filter(user=plain_user).exists() + + +def test_reconcile_allow_empty_overrides_the_refusal(plain_user, stale_commit_grant): + """The emptiness is sometimes real, and then the operator says so.""" + call_command("reconcile_achievements", "--source", "code-commits", "--allow-empty") + + assert not UserAchievement.objects.filter(user=plain_user).exists() + assert not UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + + +def test_reconcile_does_not_refuse_a_source_with_nothing_to_do(plain_user, capsys): + """An empty source with no stale grants is not a refusal, it is a no-op.""" + call_command("reconcile_achievements", "--source", "code-commits") + + captured = capsys.readouterr() + assert "nothing to change" in captured.out + assert "REFUSED" not in captured.out + assert captured.err == "" + + +def test_reconcile_creates_the_grants_a_source_supports(plain_user): + """The additive half: a source that gained a pair gets a row and a badge.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + assert not UserAchievement.objects.filter(user=plain_user).exists() + + call_command("reconcile_achievements", "--source", "code-commits") + + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + assert UserBadge.objects.filter( + user=plain_user, badge__achievement__slug="code-commits", revoked_at=None + ).exists() + + +def test_reconcile_restores_a_grant_whose_source_supports_it_again( + plain_user, commit_by_someone_else, stale_commit_grant +): + """Unbind an author, reconcile, rebind, reconcile: the badge comes back. + + The whole reason this command is two-way. A one-directional prune leaves no + way to undo itself, so a mistaken unbinding was permanent. + """ + call_command("reconcile_achievements", "--source", "code-commits") + assert not UserAchievement.objects.filter(user=plain_user).exists() + bronze = UserBadge.objects.get(user=plain_user, tier__rank="bronze") + assert bronze.revocation_source == RevocationSource.CASCADE + + author = stale_commit_grant.author + author.user = plain_user + author.save() + + call_command("reconcile_achievements", "--source", "code-commits") + + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + bronze.refresh_from_db() + assert bronze.revoked_at is None + + +def test_reconcile_does_not_undo_a_manual_revocation( + plain_user, commit_by_someone_else +): + """An admin's deliberate revocation outlives the achievement being re-created.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + bronze = UserBadge.objects.get(user=plain_user, tier__rank="bronze") + bronze.revoked_at = timezone.now() + bronze.revocation_source = RevocationSource.MANUAL + bronze.save() + # The grant goes missing the way a bulk path loses one: no post_delete, so the + # revocation is not overwritten with a cascade before the run being tested. + UserAchievement.objects.filter(user=plain_user)._raw_delete(using="default") + + call_command("reconcile_achievements", "--source", "code-commits") + + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + bronze.refresh_from_db() + assert bronze.revoked_at is not None + assert bronze.revocation_source == RevocationSource.MANUAL + + +def test_reconcile_adds_and_removes_in_one_run(plain_user, commit_by_someone_else): + """Both halves at once, for different members, off one walk of the source.""" + gaining = baker.make("users.User", email="gaining@example.com") + baker.make( + "libraries.Commit", author=baker.make("libraries.CommitAuthor", user=gaining) + ) + losing_author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=losing_author) + call_command("backfill_achievements", "--source", "code-commits") + UserAchievement.objects.filter(user=gaining).delete() + losing_author.user = None + losing_author.save() + + call_command("reconcile_achievements", "--source", "code-commits") + + assert UserAchievement.objects.filter(user=gaining).count() == 1 + assert not UserAchievement.objects.filter(user=plain_user).exists() + + +def test_reconcile_clears_every_sourceless_automatic_grant( + plain_user, commit_by_someone_else +): + """All of them, not one per run: they collapse to a single key. + + An automatic row with no source pointer can never be matched against anything + an iterator yields, so each is stale. They also share one key - the pointer is + nullable, so nothing distinguishes them - and keying the stored rows one-to-one + left every duplicate behind for a later run that would never come. + """ + achievement = Achievement.objects.get(slug=AchievementSlug.CODE_COMMITS) + for _ in range(3): + UserAchievement.objects.create( + user=plain_user, achievement=achievement, source_type=SourceType.AUTOMATIC + ) + + call_command("reconcile_achievements", "--source", "code-commits") + + assert not UserAchievement.objects.filter(user=plain_user).exists() + + +def test_reconcile_remove_only_skips_the_additive_half(plain_user): + """The one-directional behaviour is still reachable, by asking for it.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + + call_command("reconcile_achievements", "--source", "code-commits", "--remove-only") + + assert not UserAchievement.objects.filter(user=plain_user).exists() + + +def test_backfill_never_removes_anything(plain_user, commit_by_someone_else): + """The weekly pipeline's command must stay additive whatever else changes.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author) + call_command("backfill_achievements", "--source", "code-commits") + author.user = None + author.save() + + call_command("backfill_achievements", "--source", "code-commits") + + assert UserAchievement.objects.filter(user=plain_user).count() == 1 + + +def test_reconcile_fails_on_an_unseeded_source(plain_user): + """No Achievement row means the catalogue is broken, which is not a skip.""" + Achievement.objects.filter(slug=AchievementSlug.CODE_COMMITS).delete() + + with pytest.raises(CommandError, match="code-commits"): + call_command("reconcile_achievements", "--source", "code-commits") diff --git a/badges/tests/test_recalculation_batching.py b/badges/tests/test_recalculation_batching.py new file mode 100644 index 000000000..da4fc9ef3 --- /dev/null +++ b/badges/tests/test_recalculation_batching.py @@ -0,0 +1,255 @@ +"""Tests for who recalculates after a bulk delete, and how many times. + +A member's badges are derived from a count rather than adjusted by a delta, so +recalculating once after ten deletions reaches the same answer as recalculating +after each one. The ``post_delete`` signal cannot know that: it fires per row. A +bulk delete that knows which members it touched therefore takes the job on itself, +and these tests hold that arrangement in place - both halves of it, since a +suppression that outlives its block would leave badges silently over-awarded. +""" + +from unittest.mock import patch + +import pytest +from django.core.management import call_command +from model_bakery import baker + +from badges import services, signals +from badges.enums import TierRank +from badges.models import AchievementSyncRun, UserAchievement, UserBadge +from badges.services import recalculate_badges + +pytestmark = pytest.mark.django_db + +SOURCE = "code-commits" + + +@pytest.fixture(autouse=True) +def _catalogue(catalogue): + """Seed the real achievement catalogue for every test in this module.""" + + +def _spy_on(target): + """Count calls to ``recalculate_badges`` through one module's reference. + + Each module binds the function at import time, so the counting has to name the + reference under test: ``badges.signals`` for the per-row signal, and + ``badges.services`` for the sync's own batched calls. + """ + return patch(f"{target}.recalculate_badges", side_effect=recalculate_badges) + + +def _member_with_commits(email, count): + """A member holding ``count`` attributed commits, and their author row.""" + user = baker.make("users.User", email=email) + author = baker.make("libraries.CommitAuthor", user=user) + for _ in range(count): + baker.make("libraries.Commit", author=author) + return user, author + + +def _orphan(author): + """Break the attribution, leaving the member's grants stale.""" + author.user = None + author.save() + + +def test_a_bulk_delete_recalculates_once_per_member_not_once_per_row(): + """Five stale grants for one member are one recalculation, not five. + + The regression this guards: ``QuerySet.delete()`` sends ``post_delete`` per + row, so before the sync took ownership this member was recalculated five times + over, at about seven queries each, for an answer that only the last one + decided. + """ + member, author = _member_with_commits("five-commits@example.com", 5) + # Somebody the source still yields, so the run is not refused for reading empty. + _member_with_commits("still-committing@example.com", 1) + call_command("backfill_achievements", "--source", SOURCE) + assert UserAchievement.objects.filter(user=member).count() == 5 + _orphan(author) + + with _spy_on("badges.signals") as from_signal, _spy_on( + "badges.services" + ) as batched: + call_command("reconcile_achievements", "--source", SOURCE) + + assert not UserAchievement.objects.filter(user=member).exists() + assert from_signal.call_count == 0 + assert batched.call_count == 1 + assert batched.call_args.args[0] == member.pk + + +def test_the_command_does_not_repeat_what_the_run_already_did(): + """The final pass covers what the run left owing, not what it finished. + + Both call sites used to recalculate every member in ``changed``, on the belief + that the signal had done the removals - which it had, and then they did them + again. ``outstanding`` is what makes that division of labour real. + """ + member, author = _member_with_commits("outstanding@example.com", 2) + _member_with_commits("still-committing@example.com", 1) + call_command("backfill_achievements", "--source", SOURCE) + _orphan(author) + + with _spy_on("badges.management.commands.reconcile_achievements") as final_pass: + call_command("reconcile_achievements", "--source", SOURCE) + + assert final_pass.call_count == 0 + + +def test_the_summary_still_counts_the_members_a_removal_touched(capsys): + """What moved and what is left to do are different questions. + + The command recalculates what the run left owing, and reports what the run + changed. Reading the summary off the first would tell an admin who just removed + twenty grants that it happened across no members at all. + """ + _, author = _member_with_commits("counted@example.com", 20) + _member_with_commits("still-committing@example.com", 1) + call_command("backfill_achievements", "--source", SOURCE) + _orphan(author) + capsys.readouterr() + + call_command("reconcile_achievements", "--source", SOURCE) + + output = capsys.readouterr().out + assert "removed 20 grant(s) across 1 member(s)" in output + assert "across 1 (user, achievement) pair(s)" in output + + +def test_a_delete_outside_a_bulk_run_still_recalculates(): + """The signal is suspended for a block, not disabled. + + Without this the suite would pass on a guard that leaked: an ad-hoc delete in a + shell, or any future caller, would silently leave a badge awarded against + grants that no longer exist. + """ + member, _ = _member_with_commits("ad-hoc@example.com", 1) + call_command("backfill_achievements", "--source", SOURCE) + assert UserBadge.objects.filter(user=member, revoked_at=None).exists() + + with _spy_on("badges.signals") as from_signal: + UserAchievement.objects.filter(user=member).delete() + + assert from_signal.call_count == 1 + assert not UserBadge.objects.filter(user=member, revoked_at=None).exists() + + +def test_the_guard_is_released_even_when_the_delete_raises(): + """A failure inside the block must not leave the signal suspended. + + Contextvars are per-task, so a leak would not cross into another request, but + it would silently disarm every later delete in this one. + """ + with pytest.raises(RuntimeError): + with services.owns_recalculation(): + raise RuntimeError("boom") + + assert services.recalculation_is_owned() is False + + +def test_a_run_that_dies_is_a_run_to_repeat(): + """Each chunk deletes and recalculates together, or does neither. + + Two things would break this. Collecting every member and recalculating at the + end of the run means a crash leaves nobody recalculated. Recalculating per + chunk but outside the chunk's transaction means a crash leaves that chunk's + members holding badges their count no longer supports - and *silently*, because + a second reconcile sees their grants already gone, reports nothing changed, and + recalculates nobody. Only a full recalculation would ever find them. + + So: the run dies recalculating the second chunk. The first member is settled, + the second is untouched rather than half-done, the run says it failed, and + re-running it finishes the job. + """ + first, first_author = _member_with_commits("first-chunk@example.com", 1) + second, second_author = _member_with_commits("second-chunk@example.com", 1) + _member_with_commits("still-committing@example.com", 1) + call_command("backfill_achievements", "--source", SOURCE) + _orphan(first_author) + _orphan(second_author) + + calls = [] + + def die_on_the_second_chunk(user_id, achievement_id, **kwargs): + calls.append(user_id) + if len(calls) > 1: + raise RuntimeError("the worker went away") + return recalculate_badges(user_id, achievement_id, **kwargs) + + with patch.object(services, "recalculate_badges", die_on_the_second_chunk): + with pytest.raises(RuntimeError): + # One member per chunk, so the failure lands between two members + # rather than inside one member's rows. + call_command( + "reconcile_achievements", "--source", SOURCE, "--batch-size", "1" + ) + + settled, rolled_back = (first, second) if calls[0] == first.pk else (second, first) + assert not UserAchievement.objects.filter(user=settled).exists() + assert not UserBadge.objects.filter(user=settled, revoked_at=None).exists() + # Neither deleted nor recalculated: the chunk went back the way it came, so the + # grant and the badge it justifies still agree with each other. + assert UserAchievement.objects.filter(user=rolled_back).exists() + assert UserBadge.objects.filter(user=rolled_back, revoked_at=None).exists() + assert AchievementSyncRun.objects.get(source_slug=SOURCE, error__gt="").error + + call_command("reconcile_achievements", "--source", SOURCE) + + assert not UserAchievement.objects.filter(user=rolled_back).exists() + assert not UserBadge.objects.filter(user=rolled_back, revoked_at=None).exists() + + +def test_discarding_a_source_row_recalculates_once_per_pair(): + """The same arrangement on the path that deletes a source object. + + ``discard_source_achievements`` knows its pairs before it deletes, so the + per-row signal could only reach the same answer more slowly. + """ + member, _ = _member_with_commits("discarded@example.com", 4) + call_command("backfill_achievements", "--source", SOURCE) + commits = list( + UserAchievement.objects.filter(user=member).values_list( + "source_object_id", flat=True + ) + ) + from libraries.models import Commit + + with _spy_on("badges.signals") as from_signal, _spy_on( + "badges.services" + ) as batched: + services.discard_source_achievements(Commit, commits) + + assert not UserAchievement.objects.filter(user=member).exists() + assert from_signal.call_count == 0 + assert batched.call_count == 1 + + +def test_a_tier_the_member_no_longer_qualifies_for_is_still_revoked(): + """The end-to-end promise, independent of who does the recalculating. + + Everything above is about how many times the count is read. This is about the + answer: a member whose grants go below a threshold loses the badge, and the + revocation still names the run that moved the count. + """ + member, author = _member_with_commits("revoked@example.com", 3) + _member_with_commits("still-committing@example.com", 1) + call_command("backfill_achievements", "--source", SOURCE) + assert UserBadge.objects.filter( + user=member, tier__rank=TierRank.BRONZE, revoked_at=None + ).exists() + _orphan(author) + + call_command("reconcile_achievements", "--source", SOURCE) + + revoked = UserBadge.objects.get(user=member, tier__rank=TierRank.BRONZE) + assert revoked.revoked_at is not None + assert revoked.count_at_revocation == 0 + run = AchievementSyncRun.objects.get(source_slug=SOURCE, removed=3) + assert f"#{run.pk}" in revoked.revocation_notes + + +def test_the_signal_module_and_the_service_agree_on_the_guard(): + """``signals`` reads the guard through the function, not a copy of its value.""" + assert signals.recalculation_is_owned is services.recalculation_is_owned diff --git a/badges/tests/test_seed_data.py b/badges/tests/test_seed_data.py index 4a5210431..50787963a 100644 --- a/badges/tests/test_seed_data.py +++ b/badges/tests/test_seed_data.py @@ -3,7 +3,7 @@ ``Achievement.slug`` is an open field by design (admins may add manual-only types), so the slugs the codebase hard-codes are only safe if something checks they still exist. These tests are that check: they cover the seams between -``badges.enums`` and ``badges.seed_data``. +``badges.enums``, ``badges.seed_data`` and ``badges.sources``. """ import os @@ -15,6 +15,7 @@ from django.db.models import Count from model_bakery import baker +from badges import sources from badges.enums import AchievementSlug, BadgeLabel, TierRank from badges.models import Achievement, Badge, BadgeTier from badges.seed_data import SEED_CATALOGUE, seed_catalogue @@ -92,6 +93,18 @@ def test_thresholds_increase_with_rank(): assert ordered == sorted(ordered), slug +def test_every_wired_source_has_a_catalogue_entry(): + """A backfill iterator without an achievement type can never grant.""" + assert set(sources.BACKFILL_ITERATORS) <= set(SEED_SLUGS) + + +def test_automatic_slugs_are_derived_from_the_iterators(): + """The CLI --source choices cannot drift from the wired iterators.""" + assert sources.AUTOMATIC_SLUGS == [ + slug.value for slug in sources.BACKFILL_ITERATORS + ] + + @pytest.mark.django_db def test_seed_catalogue_creates_the_whole_taxonomy(catalogue): """Seeding produces one achievement and badge per entry, with five tiers.""" diff --git a/badges/tests/test_sources.py b/badges/tests/test_sources.py new file mode 100644 index 000000000..31f34ef86 --- /dev/null +++ b/badges/tests/test_sources.py @@ -0,0 +1,57 @@ +"""Tests for the backfill iterators and the uniqueness they rely on. + +There are no live signals; ingestion happens via the backfill command (see +test_commands.py) and manual grants. These tests cover the iterator logic +(filtering) and the constraint that makes re-running a backfill a no-op. +""" + +import pytest +from model_bakery import baker + +from badges import sources +from badges.models import Achievement, UserAchievement +from badges.tests.fixtures import grant_from_source + +pytestmark = pytest.mark.django_db + + +@pytest.fixture(autouse=True) +def _catalogue(catalogue): + """Seed the real achievement catalogue for every test in this module.""" + + +def test_catalogue_seeded(): + """The catalogue helper populates the achievement registry.""" + assert Achievement.objects.filter(slug="library-authoring").exists() + assert Achievement.objects.filter(slug="code-commits").exists() + + +def test_an_automatic_grant_is_idempotent(plain_user): + """Granting the same (user, achievement, source) twice creates one row. + + Which is what lets the weekly backfill re-walk every source without + double-counting - see ``unique_automatic_user_achievement_source``. + """ + achievement = Achievement.objects.get(slug="library-authoring") + library = baker.make("libraries.Library") + + _, created_first = grant_from_source(plain_user, achievement, library) + _, created_second = grant_from_source(plain_user, achievement, library) + + assert created_first is True + assert created_second is False + assert ( + UserAchievement.objects.filter(user=plain_user, achievement=achievement).count() + == 1 + ) + + +def test_iter_code_commits_skips_unlinked(plain_user): + """Only commits whose author has a linked user are yielded.""" + linked = baker.make("libraries.CommitAuthor", user=plain_user) + unlinked = baker.make("libraries.CommitAuthor", user=None) + baker.make("libraries.Commit", author=linked) + baker.make("libraries.Commit", author=unlinked) + + pairs = list(sources._iter_code_commits()) + assert [u for u, _ in pairs] == [plain_user] diff --git a/badges/tests/test_summary.py b/badges/tests/test_summary.py new file mode 100644 index 000000000..fc92cffcd --- /dev/null +++ b/badges/tests/test_summary.py @@ -0,0 +1,302 @@ +"""Tests for the per-user badge summary service.""" + +import pytest +from django.utils import timezone +from model_bakery import baker + +from badges.enums import BadgeLabel, TierRank +from badges.models import ( + Achievement, + Badge, + BadgeTier, + RevocationSource, + UserAchievement, + UserBadge, +) +from badges.services import deactivate_tier +from badges.summary import user_badge_summary +from badges.tests.fixtures import ONE_PER_RANK, set_ladder, shift_ladder + + +def _rows_by_achievement(user): + """The summary keyed by achievement slug, for rows that name one badge.""" + return {row.achievement.slug: row for row in user_badge_summary(user)} + + +def _manually_revoke(user_badge, actor, note): + """Revoke the way the admin's revoke action does.""" + user_badge.revoked_at = timezone.now() + user_badge.revoked_by = actor + user_badge.revocation_notes = note + user_badge.revocation_source = RevocationSource.MANUAL + user_badge.save() + + +def test_summary_covers_every_achievement_type(catalogue, plain_user): + """Every type appears, in achievement-name order, even with no grants.""" + rows = user_badge_summary(plain_user) + + assert len(rows) == Achievement.objects.count() == 8 + assert [row.achievement.name for row in rows] == sorted( + Achievement.objects.values_list("name", flat=True) + ) + assert all(row.badge is not None for row in rows) + assert all(row.valid_grants == 0 for row in rows) + + +def test_summary_reports_the_gap_to_the_next_tier( + badge, achievement, plain_user, grant_achievement +): + """Two of the three needed for silver leaves a gap of one.""" + grant_achievement(plain_user, achievement, count=2) + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.valid_grants == 2 + assert row.next_tier.rank == TierRank.SILVER + assert row.next_tier.threshold == 3 + assert row.gap == 1 + assert row.held.tier.rank == TierRank.BRONZE + + +def test_summary_names_the_next_rank_up_after_every_threshold_shifts( + badge, achievement, plain_user, grant_achievement +): + """A gold holder's next rung is platinum, whatever the thresholds became. + + The reported bug. Three grants make the member gold under 1/2/3/4/5; adding + five to every rung leaves those three grants meeting nothing at all, so the + lowest *unmet threshold* is the new bronze at six. Bronze is not a rung + anybody climbs to from gold - platinum is, and it needs nine. + """ + set_ladder(badge, ONE_PER_RANK) + grant_achievement(plain_user, achievement, count=3) + assert _rows_by_achievement(plain_user)["code-contribution"].held.tier.rank == ( + TierRank.GOLD + ) + + shift_ladder(badge, 5) + + row = _rows_by_achievement(plain_user)["code-contribution"] + assert row.held.tier.rank == TierRank.GOLD + assert row.next_tier.rank == TierRank.PLATINUM + assert row.next_tier.threshold == 9 + assert row.gap == 6 + + +def test_summary_reads_the_current_rank_by_rank_not_by_threshold( + badge, achievement, plain_user, grant_achievement +): + """A bronze earned at a higher threshold than an older gold is still bronze. + + Recalculation no longer creates rows like this, but a database written before + it stopped - or a restored dump - still holds them, and ranking the member's + badges by threshold would report this one as a promotion to bronze. + """ + set_ladder(badge, ONE_PER_RANK) + grant_achievement(plain_user, achievement, count=3) + shift_ladder(badge, 5) + new_bronze = badge.tiers.get(rank=TierRank.BRONZE, is_active=True) + baker.make(UserBadge, user=plain_user, badge=badge, tier=new_bronze) + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert new_bronze.threshold == 6 + assert row.held.tier.rank == TierRank.GOLD + assert row.held.tier.threshold == 3 + + +def test_summary_skips_a_manually_revoked_rank_when_naming_the_next_one( + badge, achievement, plain_user, grant_achievement, super_user +): + """A rank recalculation refuses to give back is not the rung anyone awaits. + + The member holds bronze and had silver taken away by hand. Silver's threshold + is met and will stay met, and no number of new grants brings it back, so the + next rung they can actually reach is gold. + """ + grant_achievement(plain_user, achievement, count=3) + _manually_revoke( + UserBadge.objects.get(user=plain_user, tier__rank=TierRank.SILVER), + super_user, + "Duplicate reviews.", + ) + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.held.tier.rank == TierRank.BRONZE + assert row.next_tier.rank == TierRank.GOLD + assert row.gap == 2 + + +def test_summary_reports_a_manual_revocation_with_its_note( + badge, achievement, plain_user, grant_achievement, super_user +): + """The reason names who revoked it, when, and why.""" + grant_achievement(plain_user, achievement, count=1) + _manually_revoke( + UserBadge.objects.get(user=plain_user), super_user, "Spam account." + ) + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.held is None + assert len(row.revoked) == 1 + assert str(super_user) in row.reason + assert "Spam account." in row.reason + + +def test_summary_reports_a_cascade_revocation_with_the_count( + badge, achievement, plain_user, grant_achievement +): + """A cascade revocation is explained as a count against a threshold.""" + grant_achievement(plain_user, achievement, count=1) + grant = UserAchievement.objects.get(user=plain_user) + grant.is_valid = False + grant.save() + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.held is None + assert row.valid_grants == 0 + assert row.invalid_grants == 1 + assert row.reason == "Revoked automatically - 0 valid grants, needs 1." + + +def test_summary_prefers_a_manual_revocation_over_a_cascade( + badge, achievement, plain_user, grant_achievement, super_user +): + """A manual revocation survives recalculation, so it is the real blocker.""" + grant_achievement(plain_user, achievement, count=3) + bronze = UserBadge.objects.get(user=plain_user, tier__rank=TierRank.BRONZE) + _manually_revoke(bronze, super_user, "Under review.") + grant = UserAchievement.objects.filter(user=plain_user).first() + grant.is_valid = False + grant.save() + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert {entry.revocation_source for entry in row.revoked} == { + RevocationSource.MANUAL, + RevocationSource.CASCADE, + } + assert "Under review." in row.reason + + +def test_summary_reports_hidden_badges( + badge, achievement, plain_user, grant_achievement +): + """A held badge the member has switched off is not a missing badge.""" + grant_achievement(plain_user, achievement, count=1) + plain_user.hide_badges = True + plain_user.save() + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.held is not None + assert row.reason == ("Held, but hidden - the member has turned badge display off.") + + +def test_summary_reports_a_badge_with_no_active_tiers(badge, plain_user): + """The misconfiguration that makes a badge unawardable.""" + for tier in badge.tiers.all(): + deactivate_tier(tier) + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.next_tier is None + assert row.gap is None + assert row.reason == "The badge has no active tiers, so it awards nothing." + + +def test_summary_reports_an_achievement_with_no_badge(achievement, plain_user): + """Grants that can never become anything still get a row.""" + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.badge is None + assert row.reason == "No badge is configured for this achievement." + + +def test_summary_reports_a_badge_held_below_its_threshold( + badge, achievement, plain_user, grant_achievement +): + """A badge whose grants vanished without a recalculation is flagged.""" + grant_achievement(plain_user, achievement, count=1) + # A bulk delete: no post_delete receivers, so nothing revokes the badge. + UserAchievement.objects.filter(user=plain_user)._raw_delete(using="default") + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.held is not None + assert "only 0 valid grants against a threshold of 1" in row.reason + assert "Recalculate" in row.reason + + +def test_summary_reports_grants_that_were_never_awarded(badge, achievement, plain_user): + """Grants meeting every threshold with no badge row is the inverse stale case.""" + # bulk_create sends no post_save, so no recalculation runs. + UserAchievement.objects.bulk_create( + [UserAchievement(user=plain_user, achievement=achievement) for _ in range(5)] + ) + + row = _rows_by_achievement(plain_user)["code-contribution"] + + assert row.held is None + # Holding nothing puts the member at the bottom of the ladder, so the next + # rung is bronze even though its threshold - and every other - is already met. + assert row.next_tier.rank == TierRank.BRONZE + assert row.gap == 0 + assert row.reason == ( + "Not earned, but 5 valid grants already reaches Gold. " + "Recalculate to award it." + ) + + +def test_summary_gives_an_achievement_a_row_per_badge( + badge, achievement, plain_user, grant_achievement +): + """Two badges over one achievement have separate ladders and answers.""" + second = baker.make(Badge, label=BadgeLabel.REGULAR, achievement=achievement) + baker.make(BadgeTier, badge=second, rank=TierRank.BRONZE, threshold=10) + grant_achievement(plain_user, achievement, count=1) + + rows = [row for row in user_badge_summary(plain_user) if row.badge is not None] + + assert len(rows) == 2 + by_label = {row.badge.label: row for row in rows} + assert by_label[BadgeLabel.MAINTAINER].held is not None + assert by_label[BadgeLabel.REGULAR].held is None + assert by_label[BadgeLabel.REGULAR].gap == 9 + + +@pytest.mark.parametrize("count", [1, 8]) +def test_summary_query_count_is_flat(db, plain_user, django_assert_num_queries, count): + """The cost does not grow with the number of achievement types.""" + for label in list(BadgeLabel)[:count]: + achievement = baker.make(Achievement, name=label.label, slug=label.value) + new_badge = baker.make(Badge, label=label, achievement=achievement) + baker.make(BadgeTier, badge=new_badge, rank=TierRank.BRONZE, threshold=1) + + with django_assert_num_queries(5): + rows = user_badge_summary(plain_user) + + assert len(rows) == count + + +def test_summary_query_count_survives_revocations( + badge, + achievement, + plain_user, + grant_achievement, + super_user, + django_assert_num_queries, +): + """Naming the revoking admin must not cost a query per revoked badge.""" + grant_achievement(plain_user, achievement, count=3) + for user_badge in UserBadge.objects.filter(user=plain_user): + _manually_revoke(user_badge, super_user, "Under review.") + + with django_assert_num_queries(5): + rows = user_badge_summary(plain_user) + + assert "Revoked by" in rows[0].reason diff --git a/badges/tests/test_sync_log.py b/badges/tests/test_sync_log.py new file mode 100644 index 000000000..8d073ded0 --- /dev/null +++ b/badges/tests/test_sync_log.py @@ -0,0 +1,251 @@ +"""Tests for the sync run log, the record a revoked badge points at.""" + +import re +from unittest.mock import patch + +import pytest +from django.core.management import call_command +from django.urls import reverse +from django.utils import timezone +from model_bakery import baker + +from badges import sources +from badges.admin import reconcile_apply, reconcile_preview +from badges.models import ( + AchievementSyncRun, + SyncMode, + SyncTrigger, + UserAchievement, + UserBadge, +) +from badges.tasks import backfill_achievements_task, reconcile_achievements_task + +pytestmark = pytest.mark.django_db + +SOURCE = "code-commits" + + +@pytest.fixture(autouse=True) +def _catalogue(catalogue): + """Seed the real achievement catalogue for every test in this module.""" + + +def _commit(user): + """One commit attributed to ``user``, which the badge counts.""" + author = baker.make("libraries.CommitAuthor", user=user) + return baker.make("libraries.Commit", author=author) + + +def test_a_backfill_is_recorded(plain_user): + """Every real run leaves a row saying what it did.""" + _commit(plain_user) + + call_command("backfill_achievements", "--source", SOURCE) + + run = AchievementSyncRun.objects.get(source_slug=SOURCE) + assert run.mode == SyncMode.BACKFILL + assert run.trigger == SyncTrigger.COMMAND + assert run.added == 1 + assert run.removed == 0 + assert run.members_changed == 1 + assert run.finished_at is not None + + +def test_a_reconcile_is_recorded_as_such(plain_user): + """The mode distinguishes the run that can delete from the one that cannot.""" + call_command("reconcile_achievements", "--source", SOURCE) + + assert AchievementSyncRun.objects.get(source_slug=SOURCE).mode == SyncMode.RECONCILE + + +def test_the_trigger_records_the_release_pipeline(plain_user): + """Whether a person or the weekly job did this is the first thing support asks.""" + _commit(plain_user) + + call_command( + "backfill_achievements", "--source", SOURCE, "--trigger", SyncTrigger.PIPELINE + ) + + assert ( + AchievementSyncRun.objects.get(source_slug=SOURCE).trigger + == SyncTrigger.PIPELINE + ) + + +def test_an_admin_reconcile_records_who_ran_it(plain_user, super_user): + """A reconcile started from the admin names the admin who started it.""" + reconcile_apply([SOURCE], actor=super_user) + + run = AchievementSyncRun.objects.get(source_slug=SOURCE) + assert run.trigger == SyncTrigger.ADMIN + assert run.triggered_by == super_user + + +@pytest.mark.parametrize( + "task,mode", + [ + (backfill_achievements_task, SyncMode.BACKFILL), + (reconcile_achievements_task, SyncMode.RECONCILE), + ], +) +def test_a_run_started_from_a_button_names_who_pressed_it(super_user, task, mode): + """The button hands its task the caller, which has to survive the trip. + + Both changelist buttons run on a worker, so the request the admin made is over + long before the log row is written. Without the id travelling with the job, the + two paths that do the most damage are the two the log cannot attribute. + """ + task(slug=SOURCE, actor_id=super_user.pk) + + run = AchievementSyncRun.objects.get(source_slug=SOURCE, mode=mode) + assert run.trigger == SyncTrigger.ADMIN + assert run.triggered_by == super_user + + +def test_an_explicit_trigger_beats_the_one_an_actor_implies(plain_user, super_user): + """A caller that states its trigger keeps it, actor or no actor.""" + _commit(plain_user) + + call_command( + "backfill_achievements", + "--source", + SOURCE, + "--trigger", + SyncTrigger.PIPELINE, + "--triggered-by", + str(super_user.pk), + ) + + run = AchievementSyncRun.objects.get(source_slug=SOURCE) + assert run.trigger == SyncTrigger.PIPELINE + assert run.triggered_by == super_user + + +def test_a_run_whose_actor_no_longer_exists_still_happens(plain_user): + """Attribution is worth less than the sweep it describes.""" + _commit(plain_user) + + call_command( + "backfill_achievements", "--source", SOURCE, "--triggered-by", "123456789" + ) + + run = AchievementSyncRun.objects.get(source_slug=SOURCE) + assert run.triggered_by is None + assert run.added == 1 + + +def test_a_dry_run_is_not_recorded(plain_user): + """A preview writes nothing, and the confirmation page previews on every open.""" + _commit(plain_user) + + reconcile_preview([SOURCE]) + + assert not AchievementSyncRun.objects.exists() + + +def test_a_revoked_badge_names_the_run_that_removed_its_grants(plain_user): + """The whole point: from a lost badge to the operation that caused it.""" + commit = _commit(plain_user) + # Somebody else's commit, so the source is not empty afterwards: an iterator + # that yields nothing at all is refused rather than believed. + _commit(baker.make("users.User", email="other-committer@example.com")) + call_command("backfill_achievements", "--source", SOURCE) + assert UserBadge.objects.filter(user=plain_user, revoked_at=None).exists() + + # The upstream correction: the commit history this badge rested on is gone. + commit.delete() + call_command("reconcile_achievements", "--source", SOURCE) + + assert not UserAchievement.objects.filter(user=plain_user).exists() + revoked = UserBadge.objects.get(user=plain_user, revoked_at__isnull=False) + run = AchievementSyncRun.objects.get(source_slug=SOURCE, mode=SyncMode.RECONCILE) + assert f"#{run.pk}" in revoked.revocation_notes + assert revoked.count_at_revocation == 0 + assert run.removed == 1 + + +def test_a_refused_run_records_that_it_removed_nothing(plain_user): + """A source reading empty is logged as refused, not as a successful no-op.""" + commit = _commit(plain_user) + second = baker.make("libraries.Commit", author=commit.author) + call_command("backfill_achievements", "--source", SOURCE) + assert UserAchievement.objects.filter(user=plain_user).count() == 2 + # Every commit gone at once is what a broken import looks like, not a member + # who stopped contributing. Both grants are stale, and the refusal has to hold + # for all of them rather than for the last one walked. + second.delete() + commit.delete() + + call_command("reconcile_achievements", "--source", SOURCE) + + run = AchievementSyncRun.objects.filter(mode=SyncMode.RECONCILE).latest( + "started_at" + ) + assert run.refused is True + assert run.removed == 0 + assert UserAchievement.objects.filter(user=plain_user).count() == 2 + + +def test_a_run_that_dies_part_way_is_recorded_as_failed(plain_user): + """A crashed run must not read as one still in flight. + + The counts cannot say it: a run that died before writing anything looks exactly + like a run with nothing to do. Since the deletes are chunked, a half-finished + reconcile has already revoked badges that this row is the only record of. + """ + commit = _commit(plain_user) + + def half_a_walk(): + yield plain_user, commit + raise RuntimeError("the source went away") + + with patch.dict(sources.BACKFILL_ITERATORS, {SOURCE: half_a_walk}): + # Re-raised rather than swallowed, so the command still exits non-zero and + # a task still fails instead of reporting a clean run. + with pytest.raises(RuntimeError): + call_command("backfill_achievements", "--source", SOURCE) + + run = AchievementSyncRun.objects.get(source_slug=SOURCE) + assert run.error == "RuntimeError: the source went away" + assert run.finished_at is not None + assert run.added == 0 + assert run.refused is False + + +def _flags(client, super_user): + """The two flag icons of the single run on the changelist, in column order.""" + client.force_login(super_user) + body = client.get(reverse("admin:badges_achievementsyncrun_changelist")).content + return re.findall(rb"icon-(yes|no|unknown)\.svg", body) + + +@pytest.mark.parametrize( + "finished_at,error,refused,expected", + [ + (True, "", False, [b"yes", b"yes"]), + (True, "RuntimeError: the source went away", False, [b"yes", b"no"]), + (True, "", True, [b"no", b"yes"]), + (False, "", False, [b"yes", b"unknown"]), + ], + ids=["clean", "died", "refused", "in-flight"], +) +def test_the_log_shows_a_tick_for_the_run_that_went_well( + client, super_user, finished_at, error, refused, expected +): + """Stored as the exception, read as the norm. + + The fields say ``refused`` and ``error``, so a run that did exactly what it was + asked used to be two red crosses - the icon an admin scanning for trouble stops + on. A run still in flight is the third state the pair of booleans could not + express: it has no error only because it has not finished. + """ + baker.make( + AchievementSyncRun, + source_slug=SOURCE, + mode=SyncMode.BACKFILL, + finished_at=timezone.now() if finished_at else None, + error=error, + refused=refused, + ) + + assert _flags(client, super_user) == expected diff --git a/badges/tests/test_tasks.py b/badges/tests/test_tasks.py new file mode 100644 index 000000000..12e60a96d --- /dev/null +++ b/badges/tests/test_tasks.py @@ -0,0 +1,46 @@ +"""Tests for the badges Celery tasks.""" + +import pytest + +from badges.models import AchievementSyncRun, SyncMode, SyncTrigger, UserAchievement +from badges.sources import AUTOMATIC_SLUGS +from badges.tasks import backfill_achievements_task, reconcile_achievements_task + +pytestmark = pytest.mark.django_db + + +def test_backfill_task_sweeps_every_source_by_default(catalogue, capsys): + """No argument is the whole database, which is what the weekly run wants.""" + backfill_achievements_task() + + output = capsys.readouterr().out + for slug in AUTOMATIC_SLUGS: + assert f"{slug}:" in output + + +def test_reconcile_task_scopes_its_run_to_one_member( + stale_commit_grant, commit_by_someone_else, plain_user, super_user +): + """The wrappers are the only place the commands' option names are spelled. + + ``slug`` becomes ``slugs``, ``user_id`` becomes a one-element list of strings + for the command's email-or-id resolution, and ``actor_id`` becomes both the + run's actor and, through it, its trigger. A wrong name here is a TypeError + inside a worker, on the path an admin reaches by hand from a member's page. + + Called for real rather than against a patched ``call_command``: asserting the + forwarded keywords would pin the spelling without proving the command accepts + it, which is the half that breaks. + """ + reconcile_achievements_task( + slug="code-commits", user_id=plain_user.pk, actor_id=super_user.pk + ) + + assert not UserAchievement.objects.filter(user=plain_user).exists() + # Out of scope, so its stale-or-not is never considered. + assert UserAchievement.objects.filter(user=super_user).exists() + run = AchievementSyncRun.objects.get(mode=SyncMode.RECONCILE) + assert run.source_slug == "code-commits" + assert run.removed == 1 + assert run.triggered_by == super_user + assert run.trigger == SyncTrigger.ADMIN diff --git a/core/admin_buttons.py b/core/admin_buttons.py new file mode 100644 index 000000000..ef2eb63b7 --- /dev/null +++ b/core/admin_buttons.py @@ -0,0 +1,383 @@ +"""Changelist buttons that enqueue a Celery task. + +In one place because each button needs the same four things: a POST (so a link +prefetch or a restored tab cannot start a full-table job), a CSRF token, a short +cache lock so a double-click does not queue the work twice, and a real permission +check - ``admin_site.admin_view`` only asks whether the caller is staff. + +The lock stores the enqueued task's id rather than a boolean, so it can answer "is +that job still running" instead of "was this pressed recently". Three keys: + +* ``:job:`` - the last run of *this* job, blocking another while + that run is demonstrably executing. Keyed by the argument, so a run scoped to + one source does not refuse a run for another. +* ``:job::recent`` - a few seconds, so one click is one job. Also + the grace period a queued task gets to be collected: past it a ``PENDING`` task + stops blocking, because a task no worker will collect must not wedge the button. +* ```` - the last run whatever it targeted, read only to render state. + +The trade is deliberate: a duplicate run of an idempotent job is recoverable, a +button locked for ten minutes because a worker died is not. + +State is rendered with the page, so it is correct without JavaScript; the status +view exists so HTMX can re-fetch that fragment while the job runs. + +A button whose job deletes rows sets ``permission`` and ``confirm``. Both are +opt-in: one that only rewrites derived state needs neither. +""" + +import logging +from dataclasses import dataclass +from typing import Any + +from celery.result import AsyncResult +from django.contrib import messages +from django.core.cache import cache +from django.core.exceptions import PermissionDenied +from django.http import HttpResponseRedirect +from django.shortcuts import render +from django.urls import path, reverse + +from config.celery import app as celery_app + +logger = logging.getLogger(__name__) + +TASK_BUTTON_COOLDOWN_SECONDS = 600 +TASK_BUTTON_CLICK_FLOOR_SECONDS = 30 +TASK_BUTTON_POLL_INTERVAL = "every 5s" + +# A task in one of these has been collected by a worker and is not finished. +# Anything else is either terminal or not yet collected. +RUNNING_STATES = frozenset({"STARTED", "RETRY"}) +FINISHED_STATES = frozenset({"SUCCESS", "FAILURE", "REVOKED"}) + +# Celery's own state names, which mean something to Celery and nothing to the +# person who pressed the button. +TASK_STATE_LABELS = { + "PENDING": "Queued", + "STARTED": "Running", + "RETRY": "Retrying", + "SUCCESS": "Finished", + "FAILURE": "Failed", + "REVOKED": "Cancelled", +} +UNKNOWN_STATE_LABEL = "Status unavailable" + + +@dataclass(frozen=True) +class TaskButton: + """One changelist button and the task it enqueues. + + ``choices`` makes the button a scoped run: the pairs become a select beside it + and the chosen value reaches the task as ``argument``. Choosing nothing runs the + task with no arguments. + + ``permission`` names what authorises the job, for a task doing something the + model's change permission does not cover - a sweep that deletes rows wants + ``delete_``. Unset, change permission is the gate. + + ``confirm`` interposes a preview between the click and the enqueue. Called as + ``confirm(request, value)``, it returns the preview context: ``title``, + ``summary``, ``rows`` (``label``, ``detail``, ``warning``), ``warning`` and + ``can_apply``. The task is enqueued only when the preview's submit comes back. + + ``description`` renders under the button as help text. Say what the job changes + and what it leaves alone: two of these differ only in whether they can remove + anything, and the person choosing is not the person who wrote them. + + ``pass_actor`` sends the caller's primary key to the task as ``actor_id``, for a + job that records who asked for it. Opt-in because the task has to accept the + keyword, and because a job whose audit trail says nothing gains nothing from it. + """ + + name: str + label: str + task: Any + success_message: str + busy_message: str + argument: str = "" + choice_label: str = "" + choices: tuple = () + all_label: str = "All" + permission: str = "" + confirm: Any = None + description: str = "" + pass_actor: bool = False + + def __post_init__(self): + """Refuse a select whose value would go nowhere.""" + if self.choices and not self.argument: + raise ValueError( + f"TaskButton {self.name!r} has choices but no argument to pass " + "the chosen value as." + ) + + +def task_status(task_id): + """``(state, error)`` for ``task_id``, or ``None`` if the backend cannot say. + + A result backend is configured in every environment this admin runs in, but + an unreachable one must not turn into a 500 on a changelist, and a state it + cannot report must not be rendered as fact. Callers treat ``None`` as "assume + the job is still running", which is the behaviour these buttons had before + they tracked task ids at all. + """ + try: + result = AsyncResult(task_id, app=celery_app) + state = result.state + return state, str(result.result) if state == "FAILURE" else "" + except Exception: + logger.warning("Could not read Celery state for task %s", task_id) + return None + + +class TaskButtonAdminMixin: + """Adds ``task_buttons`` to a ``ModelAdmin`` changelist. + + Set ``task_buttons`` to a tuple of ``TaskButton``. Each one gets its own + admin view, reachable only by POST, and is rendered as a submit button by + ``admin/task_buttons_change_list.html``. Subclasses that need their own + changelist template should extend that one. + """ + + change_list_template = "admin/task_buttons_change_list.html" + task_buttons = () + task_button_cooldown_seconds = TASK_BUTTON_COOLDOWN_SECONDS + task_button_click_floor_seconds = TASK_BUTTON_CLICK_FLOOR_SECONDS + + def get_urls(self): + """Register a POST-only view and a status view per button, first.""" + custom = [] + for button in self.task_buttons: + custom += [ + path( + f"{button.name}/", + self.admin_site.admin_view(self._task_button_view(button)), + name=self._task_button_url_name(button), + ), + path( + f"{button.name}/status/", + self.admin_site.admin_view(self._task_status_view(button)), + name=f"{self._task_button_url_name(button)}_status", + ), + ] + return custom + super().get_urls() + + def changelist_view(self, request, extra_context=None): + """Pass the resolved button urls and their last run to the template. + + Nothing is passed to a caller who could not use it, so the buttons are not + rendered as dead controls. The status is rendered here rather than fetched, + so it is right on a plain page load and polling is only an enhancement. + """ + buttons = [ + { + "url": reverse(f"admin:{self._task_button_url_name(button)}"), + "label": button.label, + "argument": button.argument, + "choice_label": button.choice_label, + "choices": button.choices, + "all_label": button.all_label, + "description": button.description, + "field_id": f"task-button-{button.name}-{button.argument}", + "status": self._task_button_status(button), + } + for button in self.task_buttons + if self._task_button_allows(request, button) + ] + extra_context = {**(extra_context or {}), "task_buttons": buttons} + return super().changelist_view(request, extra_context) + + def _task_button_allows(self, request, button): + """Whether this caller may start ``button``'s job. + + Per button, because they do not all cost the same: the change permission + is enough to rewrite derived state, and not enough to delete rows. + """ + if button.permission: + return request.user.has_perm(button.permission) + return self.has_change_permission(request) + + def _task_button_url_name(self, button): + """The admin url name for ``button``, which every other key derives from.""" + opts = self.model._meta + return f"{opts.app_label}_{opts.model_name}_{button.name}" + + def _task_button_cache_key(self, button): + """The key holding this button's last run, whatever it was scoped to. + + Read for display only. The button asks ``_task_button_job_key`` whether + it may start something. + """ + return f"admin-task-button:{self._task_button_url_name(button)}" + + def _task_button_job_key(self, button, value=""): + """The key holding the id of the last run of one particular job. + + Keyed by the chosen argument, so a backfill of one source does not refuse + a backfill of another - they are different jobs that happen to share a + button. + """ + return f"{self._task_button_cache_key(button)}:job:{value or 'all'}" + + def _task_button_is_busy(self, button, value=""): + """Whether this job is still running, or was started a moment ago.""" + key = self._task_button_job_key(button, value) + if cache.get(f"{key}:recent") is not None: + return True + task_id = cache.get(key) + if task_id is None: + return False + status = task_status(task_id) + return status is None or status[0] in RUNNING_STATES + + def _task_button_status(self, button): + """What to say about ``button``'s last run, if there was one. + + ``label`` is empty when nothing has been started recently, which the + template renders as nothing at all rather than as "no status". + """ + status = { + "url": reverse(f"admin:{self._task_button_url_name(button)}_status"), + "name": button.label, + "scope": "", + "label": "", + "error": "", + "poll": False, + "poll_interval": TASK_BUTTON_POLL_INTERVAL, + } + last = cache.get(self._task_button_cache_key(button)) + task_id = last.get("task_id") if isinstance(last, dict) else None + if not task_id: + # Nothing has run, or the key still holds a payload from a deploy that + # stored a different shape under it. Reading that as "no status" costs + # one status line; indexing into it would take the changelist down. + return status + result = task_status(task_id) + status["scope"] = last.get("scope", "") + if result is None: + # Nothing to poll for: a backend that cannot answer now will not + # answer in five seconds either. + status["label"] = UNKNOWN_STATE_LABEL + return status + state, error = result + status["label"] = TASK_STATE_LABELS.get(state, state) + status["error"] = error + status["poll"] = state not in FINISHED_STATES + return status + + def _task_status_view(self, button): + """Build the view that renders ``button``'s status on its own.""" + + def view(request): + """Render the status fragment, for an HTMX poll or a plain GET.""" + # Same gate as the button: a caller who is not offered the control is + # not offered the state of the job behind it either. + if not self._task_button_allows(request, button): + raise PermissionDenied + return render( + request, + "admin/task_button_status.html", + {"status": self._task_button_status(button)}, + ) + + return view + + def _task_button_view(self, button): + """Build the view that enqueues ``button``'s task.""" + + def view(request): + """Enqueue the task on POST, then send the caller back to the list.""" + opts = self.model._meta + redirect_url = reverse( + f"admin:{opts.app_label}_{opts.model_name}_changelist" + ) + if request.method != "POST": + return HttpResponseRedirect(redirect_url) + if not self._task_button_allows(request, button): + raise PermissionDenied + + choices = dict(button.choices) + value = request.POST.get(button.argument, "") if choices else "" + # ``call_command`` does not enforce an argument's ``choices``, so an + # unvetted value would only fail inside the worker, where nobody is + # looking. + if value and value not in choices: + self.message_user( + request, + "That is not one of the available options; nothing has been " + "started.", + level=messages.WARNING, + ) + return HttpResponseRedirect(redirect_url) + + if self._task_button_is_busy(button, value): + self.message_user(request, button.busy_message, level=messages.WARNING) + return HttpResponseRedirect(redirect_url) + + # Checked after the busy test, so nobody reads a preview of a job that + # was never going to start, and before the enqueue, which the preview's + # own submit is what authorises. + if button.confirm and "apply" not in request.POST: + preview = button.confirm(request, value) + return render( + request, + "admin/dry_run_confirm.html", + { + **self.admin_site.each_context(request), + "opts": opts, + "title": preview.get("title") or button.label, + "preview": preview, + "form_action": "", + # The empty value is the "all" option, and passing it back + # as a hidden field would be the same as leaving it out. + "hidden_fields": ( + [{"name": button.argument, "value": value}] if value else [] + ), + "submit_label": button.label, + "cancel_url": redirect_url, + }, + ) + + job_key = self._task_button_job_key(button, value) + # Claimed before the enqueue, so two simultaneous clicks cannot both + # get through, and released again if the enqueue fails: a broker that + # is down would otherwise hold the button for the whole cooldown. + if not cache.add( + f"{job_key}:recent", True, self.task_button_click_floor_seconds + ): + self.message_user(request, button.busy_message, level=messages.WARNING) + return HttpResponseRedirect(redirect_url) + kwargs = {button.argument: value} if value else {} + if button.pass_actor: + kwargs["actor_id"] = request.user.pk + try: + result = button.task.delay(**kwargs) + except Exception: + cache.delete(f"{job_key}:recent") + logger.exception("Could not enqueue %s", button.name) + self.message_user( + request, + "Could not queue the job: the task queue is not reachable. " + "Nothing has been started.", + level=messages.ERROR, + ) + return HttpResponseRedirect(redirect_url) + + # ``str`` because this is written to a cache that has to serialise it, + # and a task id is a string in every real case. + task_id = str(result.id) + scope = choices.get(value, "") + cache.set(job_key, task_id, self.task_button_cooldown_seconds) + cache.set( + self._task_button_cache_key(button), + {"task_id": task_id, "scope": scope}, + self.task_button_cooldown_seconds, + ) + message = button.success_message + if scope: + message = f"{message} Limited to {scope}." + self.message_user(request, message) + return HttpResponseRedirect(redirect_url) + + return view diff --git a/core/tests/test_admin_buttons.py b/core/tests/test_admin_buttons.py new file mode 100644 index 000000000..f866e7c60 --- /dev/null +++ b/core/tests/test_admin_buttons.py @@ -0,0 +1,440 @@ +"""Tests for the shared changelist task buttons. + +This module owns the *mixin's* contract - POST only, permission-gated, locked +against a double click, honest about the state of the last run. Which button +enqueues which task is the wiring each app asserts for itself. + +Exercised through the badges admin, which is where the real buttons live: a +test-only ``ModelAdmin`` would need its own model, its own registration and its +own url namespace to assert anything about urls and permissions. +""" + +from unittest.mock import Mock, patch + +import pytest +from django.contrib.auth.models import Permission +from django.core.cache import cache +from django.urls import reverse +from django.utils.html import escape +from model_bakery import baker + +from badges.admin import BACKFILL_BUTTON, RECONCILE_BUTTON +from badges.sources import AUTOMATIC_SLUGS +from core.admin_buttons import TASK_BUTTON_COOLDOWN_SECONDS + +pytestmark = pytest.mark.django_db + +BACKFILL_URL = "admin:badges_userachievement_backfill" +STATUS_URL = "admin:badges_userachievement_backfill_status" +CHANGELIST_URL = "admin:badges_userachievement_changelist" +TASK_PATH = "badges.admin.backfill_achievements_task.delay" + +# Both live buttons, so the contract is asserted on more than the single one the +# rest of this module drives. The scoped and status-rendering behaviour is only +# exercised through the backfill button, which is the one that has choices. The +# third element is whether the button names the caller to its task. +BUTTONS = [ + ("admin:badges_userachievement_backfill", "backfill_achievements_task", True), + ("admin:badges_userbadge_recalculate", "recalculate_all_badges_task", False), +] + +# The last run of the button, for display; and the last run of one of its jobs, +# which is what decides whether another may start. +LAST_RUN_KEY = "admin-task-button:badges_userachievement_backfill" +JOB_KEY = f"{LAST_RUN_KEY}:job:all" + + +@pytest.fixture(autouse=True) +def _clear_task_button_locks(): + """The buttons lock through the cache; isolate tests from each other.""" + cache.clear() + + +def _result(task_id="a-task-id"): + """A stand-in for what ``.delay()`` returns.""" + return Mock(id=task_id) + + +def _state(state): + """Patch the Celery state of every task id for the duration of a block.""" + return patch("core.admin_buttons.AsyncResult", return_value=Mock(state=state)) + + +def _running_job(task_id="running-task"): + """Record ``task_id`` as this button's last run of the unscoped job.""" + cache.set(JOB_KEY, task_id, TASK_BUTTON_COOLDOWN_SECONDS) + cache.set( + LAST_RUN_KEY, + {"task_id": task_id, "scope": ""}, + TASK_BUTTON_COOLDOWN_SECONDS, + ) + + +def _bare_staff(email): + """A staff account holding no badges permissions at all.""" + return baker.make("users.User", email=email, is_staff=True) + + +@pytest.mark.parametrize("url_name,task,names_actor", BUTTONS) +def test_button_still_swallows_a_double_click( + client, super_user, url_name, task, names_actor +): + """Two immediate posts enqueue once, whatever the worker is doing.""" + client.force_login(super_user) + + with patch(f"badges.admin.{task}.delay", return_value=_result()) as delay: + client.post(reverse(url_name)) + client.post(reverse(url_name)) + + delay.assert_called_once_with( + **({"actor_id": super_user.pk} if names_actor else {}) + ) + + +def test_button_does_not_enqueue_after_losing_the_click_floor_claim(client, super_user): + """A concurrent request that wins the atomic claim is the only enqueue.""" + client.force_login(super_user) + + with ( + patch("core.admin_buttons.cache.add", return_value=False), + patch(TASK_PATH) as delay, + ): + response = client.post(reverse(BACKFILL_URL), follow=True) + + delay.assert_not_called() + assert "not starting another one" in response.content.decode() + + +@pytest.mark.parametrize("url_name,task,_names_actor", BUTTONS) +def test_button_ignores_a_get(client, super_user, url_name, task, _names_actor): + """A GET must never start the job: link prefetchers and history restores do. + + The button is a POST form, so a GET means something other than a click. + """ + client.force_login(super_user) + + with patch(f"badges.admin.{task}.delay") as delay: + response = client.get(reverse(url_name)) + + delay.assert_not_called() + assert response.status_code == 302 + + +@pytest.mark.parametrize("url_name,task,_names_actor", BUTTONS) +def test_button_requires_change_permission(client, db, url_name, task, _names_actor): + """Staff alone is not authorisation to rewrite every row of a table. + + ``admin_site.admin_view`` only checks ``is_staff``, so without this a support + account with no badges permissions could start a full-database job. + """ + client.force_login(_bare_staff("plain-staff@example.com")) + + with patch(f"badges.admin.{task}.delay") as delay: + response = client.post(reverse(url_name)) + + assert response.status_code == 403 + delay.assert_not_called() + + +def test_button_is_not_rendered_without_permission(client, db): + """A button the caller cannot use must not be offered.""" + staff = _bare_staff("viewer-staff@example.com") + staff.user_permissions.add( + Permission.objects.get( + codename="view_userachievement", content_type__app_label="badges" + ) + ) + client.force_login(staff) + + response = client.get(reverse(CHANGELIST_URL)) + + assert response.status_code == 200 + assert response.context["task_buttons"] == [] + + +def test_button_refuses_while_the_task_is_running(client, super_user): + """A job a worker has picked up and not finished blocks a second run.""" + client.force_login(super_user) + _running_job() + + with _state("STARTED"), patch(TASK_PATH) as delay: + response = client.post(reverse(BACKFILL_URL), follow=True) + + delay.assert_not_called() + assert "not starting another one" in response.content.decode() + + +def test_button_allows_a_second_run_once_the_task_is_ready(client, super_user): + """A finished job does not hold the button for the rest of the cooldown. + + The regression test for the dead lock: before the id was tracked, any press + took the button out for ten minutes even if the work took two seconds. + """ + client.force_login(super_user) + _running_job("finished-task") + + with _state("SUCCESS"), patch(TASK_PATH, return_value=_result()) as delay: + client.post(reverse(BACKFILL_URL)) + + delay.assert_called_once_with(actor_id=super_user.pk) + + +def test_button_allows_a_second_run_once_a_queued_task_is_stale(client, super_user): + """A task no worker ever collected must not wedge the button. + + ``PENDING`` is both "queued a moment ago" and "queued at a broker nothing is + listening to". Past the click floor the two are indistinguishable, so the + button reopens rather than staying locked for the full cooldown. + """ + client.force_login(super_user) + _running_job("orphaned-task") + + with _state("PENDING"), patch(TASK_PATH, return_value=_result()) as delay: + client.post(reverse(BACKFILL_URL)) + + delay.assert_called_once_with(actor_id=super_user.pk) + + +def test_button_records_the_task_id(client, super_user): + """The lock holds the id of the job it started, not a boolean.""" + client.force_login(super_user) + + with patch(TASK_PATH, return_value=_result("the-new-task")): + client.post(reverse(BACKFILL_URL)) + + assert cache.get(JOB_KEY) == "the-new-task" + assert cache.get(LAST_RUN_KEY) == {"task_id": "the-new-task", "scope": ""} + + +def test_button_falls_back_to_the_cooldown_without_a_result_backend(client, super_user): + """A state the backend cannot report is treated as "still running". + + Degrading to the behaviour these buttons had before is the safe direction: a + button that refuses is recoverable, a second full-table job started on a + guess is not. + """ + client.force_login(super_user) + _running_job("unknowable-task") + + with patch("core.admin_buttons.AsyncResult", side_effect=OSError("no backend")): + with patch(TASK_PATH) as delay: + response = client.post(reverse(BACKFILL_URL), follow=True) + + delay.assert_not_called() + assert "not starting another one" in response.content.decode() + + +def test_changelist_reports_a_running_task(client, super_user): + """The state of the last run is on the page, in words, next to the button.""" + client.force_login(super_user) + _running_job() + + with _state("STARTED"): + body = client.get(reverse(CHANGELIST_URL)).content.decode() + + assert "Backfill achievements: Running" in body + + +def test_changelist_reports_a_failed_task(client, super_user): + """A failure says so, and says what the failure was.""" + client.force_login(super_user) + _running_job("failed-task") + failed = Mock(state="FAILURE", result=RuntimeError("no source is wired")) + + with patch("core.admin_buttons.AsyncResult", return_value=failed): + body = client.get(reverse(CHANGELIST_URL)).content.decode() + + assert "Failed" in body + assert "no source is wired" in body + + +def test_changelist_says_nothing_before_the_first_run(client, super_user): + """No run is no status, rather than a status of nothing.""" + client.force_login(super_user) + + body = client.get(reverse(CHANGELIST_URL)).content.decode() + + assert "task-button-status" not in body + + +def test_changelist_ignores_a_lock_from_an_older_deploy(client, super_user): + """The key used to hold a boolean; finding one must not break the page.""" + client.force_login(super_user) + cache.set(LAST_RUN_KEY, True, TASK_BUTTON_COOLDOWN_SECONDS) + + response = client.get(reverse(CHANGELIST_URL)) + + assert response.status_code == 200 + assert "task-button-status" not in response.content.decode() + + +def test_changelist_ignores_a_payload_of_the_wrong_shape(client, super_user): + """A dict is not enough: it has to be a dict this deploy can read. + + The rename of a key would otherwise reach the changelist as a 500 for every + admin, for as long as the entry from the previous deploy lives. + """ + client.force_login(super_user) + cache.set(LAST_RUN_KEY, {"job_id": "a-task"}, TASK_BUTTON_COOLDOWN_SECONDS) + + response = client.get(reverse(CHANGELIST_URL)) + + assert response.status_code == 200 + assert "task-button-status" not in response.content.decode() + + +def test_changelist_polls_only_while_the_task_runs(client, super_user): + """A finished job stops the polling, by rendering nothing to poll with.""" + client.force_login(super_user) + _running_job("a-task") + status_url = reverse(STATUS_URL) + + with _state("STARTED"): + running = client.get(reverse(CHANGELIST_URL)).content.decode() + with _state("SUCCESS"): + finished = client.get(reverse(CHANGELIST_URL)).content.decode() + + assert f'hx-get="{status_url}"' in running + assert "hx-get" not in finished + assert "Finished" in finished + + +def test_status_endpoint_renders_without_js(client, super_user): + """A plain GET of the fragment returns what the poll would return. + + Which is the same thing the changelist renders inline, so the page is right + with JavaScript off and the polling only saves a reload. + """ + client.force_login(super_user) + _running_job() + + with _state("STARTED"): + fragment = client.get(reverse(STATUS_URL)).content.decode() + changelist = client.get(reverse(CHANGELIST_URL)).content.decode() + + assert "Backfill achievements: Running" in fragment + assert fragment.strip() in changelist + + +def test_status_endpoint_reports_an_unreadable_backend(client, super_user): + """A state the backend cannot report is not rendered as a state.""" + client.force_login(super_user) + _running_job("unknowable-task") + + with patch("core.admin_buttons.AsyncResult", side_effect=OSError("no backend")): + body = client.get(reverse(STATUS_URL)).content.decode() + + assert "Status unavailable" in body + assert "hx-get" not in body + + +def test_status_endpoint_requires_permission(client, db): + """Staff who are not offered the button are not offered its status either.""" + client.force_login(_bare_staff("status-staff@example.com")) + _running_job() + + assert client.get(reverse(STATUS_URL)).status_code == 403 + + +def test_button_does_not_lock_when_the_enqueue_fails(client, super_user): + """A broker that is down must not take the button out for ten minutes.""" + client.force_login(super_user) + + with patch(TASK_PATH, side_effect=OSError("broker down")): + response = client.post(reverse(BACKFILL_URL), follow=True) + + assert "task queue is not reachable" in response.content.decode() + assert cache.get(JOB_KEY) is None + assert cache.get(f"{JOB_KEY}:recent") is None + + with patch(TASK_PATH, return_value=_result()) as delay: + client.post(reverse(BACKFILL_URL)) + + delay.assert_called_once_with(actor_id=super_user.pk) + + +def test_changelist_offers_every_wired_source(client, super_user): + """The select is built from the wired iterators, plus running them all.""" + client.force_login(super_user) + + body = client.get(reverse(CHANGELIST_URL)).content.decode() + + assert '' in body + for slug in AUTOMATIC_SLUGS: + assert f'

", tools_start)] + + assert '