From 1e60960d03a94c15e67473b51bcceb19ef08e25d Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:20:13 -0300 Subject: [PATCH 01/20] refactor: let a Review name its own dedup fingerprint --- .../management/commands/import_reviews.py | 37 ++---------------- versions/models.py | 12 ++++++ versions/review_keys.py | 38 +++++++++++++++++++ 3 files changed, 53 insertions(+), 34 deletions(-) create mode 100644 versions/review_keys.py diff --git a/versions/management/commands/import_reviews.py b/versions/management/commands/import_reviews.py index bbea5f470..1000a782d 100644 --- a/versions/management/commands/import_reviews.py +++ b/versions/management/commands/import_reviews.py @@ -1,6 +1,4 @@ import html -import re -import unicodedata from urllib.parse import urlparse from bs4 import BeautifulSoup @@ -13,6 +11,7 @@ from badges.services import discard_source_achievements from libraries.models import CommitAuthor from versions.models import Review, ReviewResult +from versions.review_keys import review_key PAST_RESULTS_HEADING = "Past Review Results and Milestones" PAST_RESULTS_HEADER = ( @@ -101,7 +100,7 @@ def command(clean): existing_by_key = {} removed_duplicates = 0 for review in list(Review.objects.order_by("pk")): - key = _review_key( + key = review_key( review.submission, review.submitter_raw, review.review_dates ) if key in existing_by_key: @@ -118,7 +117,7 @@ def command(clean): ) for review_data, results in past_reviews: - key = _review_key( + key = review_key( review_data["submission"], review_data["submitter_raw"], review_data["review_dates"], @@ -283,41 +282,11 @@ def _parse_table(table): return reviews -def _normalize(value: str) -> str: - """Normalize a field for flexible matching. - - Strips accents, lowercases, and removes every non-alphanumeric character - (spaces, punctuation, and any other special characters) so that minor - spelling/formatting differences compare equal. Removing - rather than - collapsing to a space - is what lets mojibake survivors such as - ``Johan RÃ¥de`` match the correct ``Johan Råde`` (both become - ``johanrade``), alongside cases like ``Joaquín M López Muñoz`` vs - ``Joaquin M Lopez Munoz`` and ``boost::container::hub`` vs ``Boost Container Hub``. - """ - decomposed = unicodedata.normalize("NFKD", value or "") - without_accents = "".join(c for c in decomposed if not unicodedata.combining(c)) - return re.sub(r"[^a-z0-9]+", "", without_accents.lower()) - - def _is_github_link(link: str) -> bool: hostname = (urlparse(link).hostname or "").lower() return hostname == "github.com" or hostname.endswith(".github.com") -def _review_key(submission: str, submitter_raw: str, review_dates: str) -> tuple: - """A flexible, multi-field fingerprint used to deduplicate reviews. - - Dates are part of the key so that a library reviewed more than once (on - different dates) is kept as separate records, while the same review - re-imported with minor spelling changes collapses to one. - """ - return ( - _normalize(submission), - _normalize(submitter_raw), - _normalize(review_dates), - ) - - def _is_superseded(link, result_cell): """True if ``link`` is wrapped in a ````.""" node = link.parent diff --git a/versions/models.py b/versions/models.py index 7be0ab40b..a7425aef2 100755 --- a/versions/models.py +++ b/versions/models.py @@ -8,6 +8,7 @@ from .converters import to_url from .exceptions import BoostImportedDataException from .managers import VersionManager, VersionFileManager +from .review_keys import review_key from .utils.model_validators import validate_version_name_format User = get_user_model() @@ -321,6 +322,17 @@ class Review(models.Model): github_link = models.URLField(blank=True, default="") documentation_link = models.URLField(blank=True, default="") + @property + def dedup_key(self) -> str: + """The importer's fingerprint, as one string. + + What identifies a review to the achievement engine: stable across + re-imports that rewrite the row, unlike the primary key. + """ + return "|".join( + review_key(self.submission, self.submitter_raw, self.review_dates) + ) + def __str__(self) -> str: return self.submission diff --git a/versions/review_keys.py b/versions/review_keys.py new file mode 100644 index 000000000..f004054af --- /dev/null +++ b/versions/review_keys.py @@ -0,0 +1,38 @@ +"""Fingerprinting used to deduplicate reviews. + +Outside the import command so that a stored ``Review`` can name itself the same +way the importer does. +""" + +import re +import unicodedata + + +def normalize(value: str) -> str: + """Normalize a field for flexible matching. + + Strips accents, lowercases, and removes every non-alphanumeric character + (spaces, punctuation, and any other special characters) so that minor + spelling/formatting differences compare equal. Removing - rather than + collapsing to a space - is what lets mojibake survivors such as + ``Johan RÃ¥de`` match the correct ``Johan Råde`` (both become + ``johanrade``), alongside cases like ``Joaquín M López Muñoz`` vs + ``Joaquin M Lopez Munoz`` and ``boost::container::hub`` vs ``Boost Container Hub``. + """ + decomposed = unicodedata.normalize("NFKD", value or "") + without_accents = "".join(c for c in decomposed if not unicodedata.combining(c)) + return re.sub(r"[^a-z0-9]+", "", without_accents.lower()) + + +def review_key(submission: str, submitter_raw: str, review_dates: str) -> tuple: + """A flexible, multi-field fingerprint used to deduplicate reviews. + + Dates are part of the key so that a library reviewed more than once (on + different dates) is kept as separate records, while the same review + re-imported with minor spelling changes collapses to one. + """ + return ( + normalize(submission), + normalize(submitter_raw), + normalize(review_dates), + ) From 4e475dd2aee06f060a6acd07c8823b66e2cb467c Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:21:32 -0300 Subject: [PATCH 02/20] feat: add a dedup key to automatic achievement grants --- .../0004_userachievement_dedup_info.py | 41 +++++++++++++++++++ badges/models.py | 21 ++++++---- 2 files changed, 54 insertions(+), 8 deletions(-) create mode 100644 badges/migrations/0004_userachievement_dedup_info.py diff --git a/badges/migrations/0004_userachievement_dedup_info.py b/badges/migrations/0004_userachievement_dedup_info.py new file mode 100644 index 000000000..5db5d3046 --- /dev/null +++ b/badges/migrations/0004_userachievement_dedup_info.py @@ -0,0 +1,41 @@ +# Generated by Django 6.0.2 on 2026-08-19 14:21 + +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("badges", "0003_achievementsyncrun"), + ("contenttypes", "0002_remove_content_type_name"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.RemoveConstraint( + model_name="userachievement", + name="unique_automatic_user_achievement_source", + ), + migrations.AddField( + model_name="userachievement", + name="dedup_info", + field=models.CharField( + blank=True, + help_text="For automatic grants: the source's stable id for the evidence.", + max_length=200, + null=True, + verbose_name="dedup info", + ), + ), + migrations.AddConstraint( + model_name="userachievement", + constraint=models.UniqueConstraint( + condition=models.Q( + ("dedup_info__isnull", False), ("source_type", "automatic") + ), + fields=("user", "achievement", "dedup_info"), + name="unique_automatic_user_achievement_dedup", + ), + ), + ] diff --git a/badges/models.py b/badges/models.py index b61397563..a72718c4a 100644 --- a/badges/models.py +++ b/badges/models.py @@ -120,6 +120,16 @@ class UserAchievement(models.Model): # Big, not plain: the models these grants point at use BigAutoField keys. source_object_id = models.PositiveBigIntegerField(null=True, blank=True) source = GenericForeignKey("source_content_type", "source_object_id") + # The source's own name for the evidence, which a primary key is not: the + # commit importer deletes and re-creates rows, and one commit is stored once + # per library version covering it. Null for manual grants. + dedup_info = models.CharField( + _("dedup info"), + max_length=200, + null=True, + blank=True, + help_text=_("For automatic grants: the source's stable id for the evidence."), + ) granted_by = models.ForeignKey( settings.AUTH_USER_MODEL, @@ -142,14 +152,9 @@ class Meta: ordering = ("-created_at",) constraints = [ models.UniqueConstraint( - fields=[ - "user", - "achievement", - "source_content_type", - "source_object_id", - ], - condition=models.Q(source_type="automatic"), - name="unique_automatic_user_achievement_source", + fields=["user", "achievement", "dedup_info"], + condition=models.Q(source_type="automatic", dedup_info__isnull=False), + name="unique_automatic_user_achievement_dedup", ) ] From ed5c8c9ef943293591480d86f2c39dcaa3747d91 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:28:11 -0300 Subject: [PATCH 03/20] fix: store the dedup key as text, since a source key has no fixed width --- badges/migrations/0004_userachievement_dedup_info.py | 3 +-- badges/models.py | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/badges/migrations/0004_userachievement_dedup_info.py b/badges/migrations/0004_userachievement_dedup_info.py index 5db5d3046..018d28256 100644 --- a/badges/migrations/0004_userachievement_dedup_info.py +++ b/badges/migrations/0004_userachievement_dedup_info.py @@ -20,10 +20,9 @@ class Migration(migrations.Migration): migrations.AddField( model_name="userachievement", name="dedup_info", - field=models.CharField( + field=models.TextField( blank=True, help_text="For automatic grants: the source's stable id for the evidence.", - max_length=200, null=True, verbose_name="dedup info", ), diff --git a/badges/models.py b/badges/models.py index a72718c4a..c58f78e6e 100644 --- a/badges/models.py +++ b/badges/models.py @@ -123,9 +123,8 @@ class UserAchievement(models.Model): # The source's own name for the evidence, which a primary key is not: the # commit importer deletes and re-creates rows, and one commit is stored once # per library version covering it. Null for manual grants. - dedup_info = models.CharField( + dedup_info = models.TextField( _("dedup info"), - max_length=200, null=True, blank=True, help_text=_("For automatic grants: the source's stable id for the evidence."), From a3b3448cc7b323a8185478650c061741eb9abbb8 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:28:12 -0300 Subject: [PATCH 04/20] feat: match automatic grants on their source's dedup key --- badges/admin.py | 2 + badges/services.py | 72 +++++++++++++++++++---------------- badges/sources.py | 47 ++++++++++++++++------- badges/tests/test_sources.py | 16 ++++---- badges/tests/test_sync_log.py | 2 +- 5 files changed, 84 insertions(+), 55 deletions(-) diff --git a/badges/admin.py b/badges/admin.py index b90088e78..1a2f465e1 100644 --- a/badges/admin.py +++ b/badges/admin.py @@ -845,6 +845,7 @@ class UserAchievementAdmin( "user__display_name", "achievement__name", "grant_notes", + "dedup_info", ) autocomplete_fields = ("achievement", "user") readonly_fields = ( @@ -852,6 +853,7 @@ class UserAchievementAdmin( "invalidated_by", "invalidated_at", "granted_by", + "dedup_info", ) actions = ["invalidate", "revalidate"] add_fieldsets = ((None, {"fields": ("user", "achievement", "grant_notes")}),) diff --git a/badges/services.py b/badges/services.py index f82bd1742..c03a8759c 100644 --- a/badges/services.py +++ b/badges/services.py @@ -50,7 +50,7 @@ "the {rank} threshold of {threshold}." ) -# Rows per DELETE ... WHERE pk IN (...) and per bulk_create. The unmatched set can +# Rows per DELETE ... WHERE pk IN (...) and per bulk_create. The stored key set can # be as large as the achievement table. SYNC_BATCH_SIZE = 1000 @@ -355,26 +355,25 @@ def _sync_source( 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. + # Every stored grant, keyed the way its source names the evidence. A key the + # walk never yields is stale; a key it yields needs one row, so any surplus + # rows carrying it are stale too. 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" + # A list of rows per key, not one: rows written before a source was keyed + # share ``(user, NULL)``, and duplicate source rows can share a real key + # until a reconcile collapses them. + stored_keys = {} + for pk, user_id, dedup_info in stored.values_list( + "pk", "user_id", "dedup_info" ).iterator(chunk_size=2000): - unmatched.setdefault((user_id, content_type_id, object_id), []).append(pk) + stored_keys.setdefault((user_id, dedup_info), []).append(pk) scope = None if user_ids is None else set(user_ids) yielded = added = 0 changed = set() + seen = set() pending = {} def flush(): @@ -383,40 +382,48 @@ def flush(): if not pending: return if not dry_run: - # ignore_conflicts because ``unmatched`` is a snapshot: a concurrent + # ignore_conflicts because ``stored_keys`` 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) + # An inserted row is not stale, and a key repeated in a later batch must + # not be inserted a second time. + for key in pending: + stored_keys.setdefault(key, []) pending = {} - for user, source in sources.BACKFILL_ITERATORS[slug](): + for user, source, dedup_key in sources.BACKFILL_ITERATORS[slug](): yielded += 1 + if dedup_key is None: + # Without a key the engine cannot tell this grant from a new one, so + # every sweep would re-add it and every reconcile would remove it. + raise ValueError(f"Source '{slug}' yielded no dedup key for {source!r}.") # 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: + # since its key is never yielded and so 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 + # The scope is applied here as well as on ``stored_keys``: 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: + key = (user.pk, dedup_key) + seen.add(key) + if key in stored_keys: 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. + # ``pending`` is keyed, so an iterator naming the same evidence twice + # inside one batch counts it once, and ``flush`` carries the key over so + # that holds across batches too. if not add or key in pending: continue changed.add(user.pk) @@ -424,8 +431,9 @@ def flush(): user_id=user.pk, achievement=achievement, source_type=SourceType.AUTOMATIC, - source_content_type=content_type, + source_content_type=ContentType.objects.get_for_model(source), source_object_id=source.pk, + dedup_info=dedup_key, ) if len(pending) >= batch_size: flush() @@ -434,11 +442,11 @@ def 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 [] - ) + stale = [] + if remove: + for key, pks in stored_keys.items(): + surplus = pks if key not in seen else pks[1:] + stale.extend((pk, key[0]) for pk in surplus) if stale and not yielded and not allow_empty: logger.warning( "Refusing to remove %s stale grant(s) for '%s': the source yielded " @@ -454,7 +462,7 @@ def flush(): ) if remove: - changed.update(user_id for user_id, _, _ in unmatched) + changed.update(user_id for _, user_id in stale) recalculated = set() if stale and not dry_run: diff --git a/badges/sources.py b/badges/sources.py index c2a1d1261..34e3f902b 100644 --- a/badges/sources.py +++ b/badges/sources.py @@ -1,21 +1,25 @@ """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 +yielding ``(user, source_object, dedup_key)`` 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 +The dedup key is how the engine recognises a grant it has already made, so it is +the source's own name for the evidence rather than a row id, and it must not change +for the life of that evidence. It is required: an iterator that cannot name its +evidence cannot be reconciled. + +Two 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. +Both can still be granted by hand in the admin. Sub-libraries (``math/quaternion``, ``functional/hash``, and the rest of ``SUB_LIBRARIES``) are excluded from every library-shaped source. They are @@ -28,6 +32,15 @@ from libraries.constants import SUB_LIBRARIES +def _library_key(library): + """Name a library the way ``libraries.json`` does, or by its row if it cannot. + + ``Library.key`` is nullable, and a library without one still has to be named. + Its pk serves: nothing deletes and re-creates ``Library`` rows. + """ + return library.key or f"library-{library.pk}" + + def _iter_library_authoring(): """Yield (user, library) for every authorship of a parent library.""" from libraries.models import Library @@ -37,7 +50,7 @@ def _iter_library_authoring(): ) for library in libraries.iterator(chunk_size=500): for user in library.authors.all(): - yield user, library + yield user, library, _library_key(library) def _iter_library_maintenance(): @@ -61,7 +74,7 @@ def _iter_library_maintenance(): key = (user.pk, version.library_id) if key not in seen: seen.add(key) - yield user, version.library + yield user, version.library, _library_key(version.library) def _iter_library_versioning(): @@ -71,16 +84,24 @@ def _iter_library_versioning(): """ from libraries.models import LibraryVersion - versions = LibraryVersion.objects.exclude( - library__key__in=SUB_LIBRARIES - ).prefetch_related("authors") + versions = ( + LibraryVersion.objects.exclude(library__key__in=SUB_LIBRARIES) + .select_related("library", "version") + .prefetch_related("authors") + ) for version in versions.iterator(chunk_size=500): + key = f"{_library_key(version.library)}@{version.version.name}" for user in version.authors.all(): - yield user, version + yield user, version, key def _iter_code_commits(): - """Yield (user, commit) for every attributed commit.""" + """Yield (user, commit) for every attributed commit. + + Keyed on the sha and not the library, so the same commit stored once per + library version covering it, and once per library sharing the repository, + counts once. + """ from libraries.models import Commit commits = ( @@ -89,7 +110,7 @@ def _iter_code_commits(): .iterator(chunk_size=1000) ) for commit in commits: - yield commit.author.user, commit + yield commit.author.user, commit, commit.sha def _iter_library_review(): @@ -101,7 +122,7 @@ def _iter_library_review(): ): for commit_author in review.submitters.all(): if commit_author.user_id: - yield commit_author.user, review + yield commit_author.user, review, review.dedup_key BACKFILL_ITERATORS = { diff --git a/badges/tests/test_sources.py b/badges/tests/test_sources.py index 487acef08..a0e4f1bab 100644 --- a/badges/tests/test_sources.py +++ b/badges/tests/test_sources.py @@ -30,7 +30,7 @@ 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``. + double-counting - see ``unique_automatic_user_achievement_dedup``. """ achievement = Achievement.objects.get(slug="library-authoring") library = baker.make("libraries.Library") @@ -48,10 +48,9 @@ def test_an_automatic_grant_is_idempotent(plain_user): def test_iter_library_authoring(plain_user): """The authoring iterator yields each (author, library) pair.""" - library = baker.make("libraries.Library") + library = baker.make("libraries.Library", key="mp11") library.authors.add(plain_user) - pairs = list(sources._iter_library_authoring()) - assert (plain_user, library) in pairs + assert list(sources._iter_library_authoring()) == [(plain_user, library, "mp11")] def test_iter_library_authoring_skips_sub_libraries(plain_user): @@ -68,13 +67,12 @@ def test_iter_library_authoring_skips_sub_libraries(plain_user): def test_iter_library_maintenance_dedupes_versions(plain_user): """Maintaining many versions of one library yields a single pair.""" - library = baker.make("libraries.Library") + library = baker.make("libraries.Library", key="mp11") for _ in range(3): version = baker.make("libraries.LibraryVersion", library=library) version.maintainers.add(plain_user) - pairs = list(sources._iter_library_maintenance()) - assert pairs == [(plain_user, library)] + assert list(sources._iter_library_maintenance()) == [(plain_user, library, "mp11")] def test_iter_library_maintenance_skips_sub_libraries(plain_user): @@ -103,7 +101,7 @@ def test_iter_code_commits_skips_unlinked(plain_user): baker.make("libraries.Commit", author=unlinked) pairs = list(sources._iter_code_commits()) - assert [u for u, _ in pairs] == [plain_user] + assert [u for u, _, _ in pairs] == [plain_user] def test_iter_library_review_skips_unlinked(plain_user): @@ -114,4 +112,4 @@ def test_iter_library_review_skips_unlinked(plain_user): baker.make("libraries.CommitAuthor", user=None), ) pairs = list(sources._iter_library_review()) - assert [u for u, _ in pairs] == [plain_user] + assert [u for u, _, _ in pairs] == [plain_user] diff --git a/badges/tests/test_sync_log.py b/badges/tests/test_sync_log.py index 8d073ded0..c112bf6aa 100644 --- a/badges/tests/test_sync_log.py +++ b/badges/tests/test_sync_log.py @@ -196,7 +196,7 @@ def test_a_run_that_dies_part_way_is_recorded_as_failed(plain_user): commit = _commit(plain_user) def half_a_walk(): - yield plain_user, commit + yield plain_user, commit, commit.sha raise RuntimeError("the source went away") with patch.dict(sources.BACKFILL_ITERATORS, {SOURCE: half_a_walk}): From 43f60c2282c108d7fdf5ecabfcc9598980566b17 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:31:06 -0300 Subject: [PATCH 05/20] test: pin what keying a grant on its source buys --- badges/services.py | 20 ++--- badges/tests/fixtures.py | 7 +- badges/tests/test_dedup_keys.py | 154 ++++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 13 deletions(-) create mode 100644 badges/tests/test_dedup_keys.py diff --git a/badges/services.py b/badges/services.py index c03a8759c..36ef1a69b 100644 --- a/badges/services.py +++ b/badges/services.py @@ -355,15 +355,15 @@ def _sync_source( if user_ids is not None: stored = stored.filter(user_id__in=user_ids) - # Every stored grant, keyed the way its source names the evidence. A key the - # walk never yields is stale; a key it yields needs one row, so any surplus - # rows carrying it are stale too. 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. + # Every stored grant, keyed the way its source names the evidence. Whatever the + # walk never yields is stale, and a key it yields but cannot find here is a + # grant that does not exist yet, so one dict answers both halves. 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: rows written before a source was keyed - # share ``(user, NULL)``, and duplicate source rows can share a real key - # until a reconcile collapses them. + # A list of rows per key, not one: rows written before a source was keyed all + # share ``(user, NULL)``. A real key can only ever hold one row - + # ``unique_automatic_user_achievement_dedup`` says so. stored_keys = {} for pk, user_id, dedup_info in stored.values_list( "pk", "user_id", "dedup_info" @@ -445,8 +445,8 @@ def flush(): stale = [] if remove: for key, pks in stored_keys.items(): - surplus = pks if key not in seen else pks[1:] - stale.extend((pk, key[0]) for pk in surplus) + if key not in seen: + stale.extend((pk, key[0]) for pk in pks) if stale and not yielded and not allow_empty: logger.warning( "Refusing to remove %s stale grant(s) for '%s': the source yielded " diff --git a/badges/tests/fixtures.py b/badges/tests/fixtures.py index 85b1dd4c6..30d9a7587 100644 --- a/badges/tests/fixtures.py +++ b/badges/tests/fixtures.py @@ -31,18 +31,19 @@ } -def grant_from_source(user, achievement, source): +def grant_from_source(user, achievement, source, dedup_info=None): """Record one automatic grant pointing at ``source``. ``get_or_create`` rather than ``create``, so a test can assert that a repeat - is a no-op. + is a no-op. ``dedup_info`` is what the engine matches on; left out, the row + stands for one written before its source was keyed. """ return UserAchievement.objects.get_or_create( user=user, achievement=achievement, source_content_type=ContentType.objects.get_for_model(source), source_object_id=source.pk, - defaults={"source_type": SourceType.AUTOMATIC}, + defaults={"source_type": SourceType.AUTOMATIC, "dedup_info": dedup_info}, ) diff --git a/badges/tests/test_dedup_keys.py b/badges/tests/test_dedup_keys.py new file mode 100644 index 000000000..db5229f70 --- /dev/null +++ b/badges/tests/test_dedup_keys.py @@ -0,0 +1,154 @@ +"""Tests for the source key that identifies an automatic grant. + +A grant used to be identified by the row it pointed at, which the commit importer +churns and which exists once per library version covering a commit. These cover +what keying on the source's own name for the evidence buys instead. +""" + +import pytest +from django.core.management import call_command +from model_bakery import baker + +from badges import sources +from badges.models import Achievement, SourceType, 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 _commit(user, sha, library_version=None): + """One commit attributed to ``user``, optionally under a given version.""" + author = baker.make("libraries.CommitAuthor", user=user) + extra = {} if library_version is None else {"library_version": library_version} + return baker.make("libraries.Commit", author=author, sha=sha, **extra) + + +def _grants(user): + return UserAchievement.objects.filter( + user=user, achievement__slug="code-commits", source_type=SourceType.AUTOMATIC + ) + + +def test_one_sha_under_two_versions_grants_once(plain_user): + """The release ranges store a commit once per version they cover. + + Both rows are the same piece of work, so the member is credited once. + """ + library = baker.make("libraries.Library", key="mp11") + for name in ("1.88.0", "master"): + version = baker.make("libraries.LibraryVersion", library=library) + _commit(plain_user, "cafe1234", library_version=version) + + call_command("backfill_achievements", "--source", "code-commits") + + assert _grants(plain_user).count() == 1 + + +def test_reimporting_with_new_ids_changes_nothing(plain_user): + """The importer's clean mode deletes every commit row and re-inserts it. + + The shas survive that, so the grants do too: nothing to add, nothing stale. + """ + from libraries.models import Commit + + commit = _commit(plain_user, "cafe1234") + call_command("backfill_achievements", "--source", "code-commits") + grant = _grants(plain_user).get() + + author = commit.author + library_version = commit.library_version + Commit.objects.all().delete() + replacement = baker.make( + "libraries.Commit", + author=author, + library_version=library_version, + sha="cafe1234", + ) + assert replacement.pk != commit.pk + + call_command("reconcile_achievements", "--source", "code-commits") + + assert _grants(plain_user).get().pk == grant.pk + + +def test_unkeyed_duplicates_collapse_to_one(plain_user): + """One sha stored twice was credited twice before the key existed. + + Neither old row can be matched, so both go and one keyed grant replaces them. + """ + library = baker.make("libraries.Library", key="mp11") + achievement = Achievement.objects.get(slug="code-commits") + for _ in range(2): + commit = _commit( + plain_user, + "cafe1234", + library_version=baker.make("libraries.LibraryVersion", library=library), + ) + grant_from_source(plain_user, achievement, commit) + assert _grants(plain_user).count() == 2 + + call_command("reconcile_achievements", "--source", "code-commits") + + assert [g.dedup_info for g in _grants(plain_user)] == ["cafe1234"] + + +def test_a_grant_with_no_key_is_replaced(plain_user): + """Grants written before a source was keyed are replaced, not adopted. + + Which is what makes emptying the tables and backfilling the way to convert an + environment, rather than a healing pass nobody will run twice. + """ + achievement = Achievement.objects.get(slug="code-commits") + commit = _commit(plain_user, "cafe1234") + grant_from_source(plain_user, achievement, commit) + + call_command("reconcile_achievements", "--source", "code-commits") + + grant = _grants(plain_user).get() + assert grant.dedup_info == "cafe1234" + + +def test_a_source_that_names_nothing_fails_loudly(plain_user): + """A missing key would be added every sweep and removed every reconcile.""" + from unittest.mock import patch + + commit = _commit(plain_user, "cafe1234") + + def unkeyed(): + yield plain_user, commit, None + + with patch.dict(sources.BACKFILL_ITERATORS, {"code-commits": unkeyed}): + with pytest.raises(ValueError, match="no dedup key"): + call_command("backfill_achievements", "--source", "code-commits") + + +def test_source_key_formats(plain_user): + """The key format is a contract: changing one orphans every stored grant.""" + library = baker.make("libraries.Library", key="mp11") + version = baker.make( + "libraries.LibraryVersion", + library=library, + version=baker.make("versions.Version", name="boost-1.88.0"), + ) + version.authors.add(plain_user) + _commit(plain_user, "cafe1234") + review = baker.make( + "versions.Review", + submission="Boost.MP11", + submitter_raw="Peter Dimov", + review_dates="April 1-10, 2017", + ) + review.submitters.add(baker.make("libraries.CommitAuthor", user=plain_user)) + + assert [key for _, _, key in sources._iter_library_versioning()] == [ + "mp11@boost-1.88.0" + ] + assert [key for _, _, key in sources._iter_code_commits()] == ["cafe1234"] + assert [key for _, _, key in sources._iter_library_review()] == [ + "boostmp11|peterdimov|april1102017" + ] From 72309a0d2685987950eb940fc500ed188c5c9a3c Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:32:09 -0300 Subject: [PATCH 06/20] feat: classify which changed paths count as documentation --- libraries/doc_paths.py | 69 ++++++++++++++++++++++++++++ libraries/tests/test_doc_paths.py | 75 +++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 libraries/doc_paths.py create mode 100644 libraries/tests/test_doc_paths.py diff --git a/libraries/doc_paths.py b/libraries/doc_paths.py new file mode 100644 index 000000000..b08616387 --- /dev/null +++ b/libraries/doc_paths.py @@ -0,0 +1,69 @@ +"""Which changed file paths count as documentation. + +The commit importer counts these per commit, and the Documenter achievement is +derived from that count. Kept in one place so tightening the rules is a small diff +rather than a hunt. +""" + +from pathlib import PurePosixPath + +DOC_DIRECTORIES = frozenset({"doc", "docs"}) +DOC_SUFFIXES = frozenset({".adoc", ".qbk", ".rst", ".md"}) +# Build output, committed in some repos. It outnumbers the sources it was +# generated from by orders of magnitude. +GENERATED_DIRECTORIES = frozenset({"html"}) +IGNORED_DIRECTORIES = frozenset({".github"}) +IGNORED_SUFFIXES = frozenset({".json", ".yml", ".yaml", ".cmake", ".jam"}) +IGNORED_NAME_PREFIXES = ("Jamfile",) + + +def resolve_rename(path): + """Take the destination of a rename, which git reports as one path. + + Two shapes: ``doc/{old => new}/index.adoc``, and ``old.adoc => new.adoc`` + where the paths share no prefix. + """ + if "=>" not in path: + return path + if "{" in path and "}" in path: + before, rest = path.split("{", 1) + inner, after = rest.split("}", 1) + _, _, destination = inner.partition("=>") + return f"{before}{destination.strip()}{after}".replace("//", "/") + return path.split("=>", 1)[1].strip() + + +def is_doc_path(path): + """True if changing ``path`` counts as documentation work.""" + parts = PurePosixPath(path).parts + if not parts: + return False + directories, name = set(parts[:-1]), parts[-1] + if directories & (IGNORED_DIRECTORIES | GENERATED_DIRECTORIES): + return False + suffix = PurePosixPath(name).suffix.lower() + if suffix in IGNORED_SUFFIXES or name.startswith(IGNORED_NAME_PREFIXES): + return False + if directories & DOC_DIRECTORIES: + return True + if not directories and name.lower() == "readme.md": + # A README at the repository root introduces the library rather than + # documenting it. + return False + return suffix in DOC_SUFFIXES + + +def count_doc_files(stat_lines): + """Count the documentation files in ``git log --numstat`` output lines. + + A binary file reports ``-`` for its line counts and is classified on its path + like any other. + """ + total = 0 + for line in stat_lines: + fields = line.split("\t") + if len(fields) != 3: + continue + if is_doc_path(resolve_rename(fields[2].strip())): + total += 1 + return total diff --git a/libraries/tests/test_doc_paths.py b/libraries/tests/test_doc_paths.py new file mode 100644 index 000000000..541f29cc0 --- /dev/null +++ b/libraries/tests/test_doc_paths.py @@ -0,0 +1,75 @@ +"""Tests for the documentation path rules the Documenter achievement counts.""" + +import pytest + +from libraries.doc_paths import count_doc_files, is_doc_path, resolve_rename + +DOC_PATHS = [ + "doc/index.adoc", + "docs/index.adoc", + "doc/deep/nested/page.adoc", + "doc/img/diagram.png", + "doc/reference.qbk", + "guide.rst", + "CHANGELOG.md", + "example/README.md", + "doc/Makefile", +] + +NON_DOC_PATHS = [ + "README.md", + "include/boost/mp11/list.hpp", + "test/list_test.cpp", + "meta/libraries.json", + "doc/build.json", + "doc/html/index.html", + "doc/html/reference.adoc", + ".github/workflows/ci.yml", + "doc/appveyor.yml", + "doc/config.yaml", + "doc/CMakeLists.cmake", + "Jamfile.v2", + "doc/Jamfile", + "build/build.jam", +] + + +@pytest.mark.parametrize("path", DOC_PATHS) +def test_counts_as_documentation(path): + assert is_doc_path(path) is True + + +@pytest.mark.parametrize("path", NON_DOC_PATHS) +def test_does_not_count_as_documentation(path): + assert is_doc_path(path) is False + + +@pytest.mark.parametrize( + "reported,destination", + [ + ("doc/{old => new}/index.adoc", "doc/new/index.adoc"), + ("{ => doc}/index.adoc", "doc/index.adoc"), + ("doc/{sub => }/index.adoc", "doc/index.adoc"), + ("old.adoc => new.adoc", "new.adoc"), + ("doc/index.adoc", "doc/index.adoc"), + ], +) +def test_resolve_rename(reported, destination): + """git reports a rename as a single path, and the destination is what counts.""" + assert resolve_rename(reported) == destination + + +def test_count_doc_files_reads_numstat(): + """Binary files carry no line counts, and renames arrive as one path.""" + lines = [ + "12\t3\tdoc/index.adoc", + "-\t-\tdoc/img/diagram.png", + "40\t0\tinclude/boost/mp11/list.hpp", + "2\t2\tdoc/{old => new}/guide.adoc", + "1\t0\tREADME.md", + ] + assert count_doc_files(lines) == 3 + + +def test_count_doc_files_ignores_anything_that_is_not_a_stat_line(): + assert count_doc_files(["", "not a stat line", "1\t2"]) == 0 From 01d62a719d92dba9dbdf7970b4b823a3fdefe987 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:33:42 -0300 Subject: [PATCH 07/20] feat: read per-file stats from the commit log and count doc files --- libraries/github.py | 29 +++++++++++++- libraries/tests/test_github.py | 73 ++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/libraries/github.py b/libraries/github.py index 8eb6e87ee..056db6869 100644 --- a/libraries/github.py +++ b/libraries/github.py @@ -18,6 +18,7 @@ from versions.models import Version from .constants import CATEGORY_OVERRIDES +from .doc_paths import count_doc_files from .models import ( Category, Commit, @@ -46,6 +47,11 @@ ) - relativedelta(days=1) +# ``git log --numstat`` writes one of these per file changed, after the message. +# Merge commits produce none, so a merge never counts as documentation. +NUMSTAT_LINE = re.compile(r"^(?:\d+|-)\t(?:\d+|-)\t") + + @dataclass class ParsedCommit: email: str @@ -56,6 +62,7 @@ class ParsedCommit: is_merge: bool committed_at: timezone.datetime avatar_url: str | None = None + docs_files_changed: int = 0 @dataclass @@ -145,7 +152,16 @@ def get_commit_data_for_repo_versions(key, min_version=""): ) log_output = subprocess.run( - ["git", "--git-dir", str(git_dir), "log", f"{a}..{b}", "--date", "iso"], + [ + "git", + "--git-dir", + str(git_dir), + "log", + f"{a}..{b}", + "--date", + "iso", + "--numstat", + ], capture_output=True, ) commits = log_output.stdout.decode() @@ -155,7 +171,15 @@ def get_commit_data_for_repo_versions(key, min_version=""): email = groups["email"].strip() sha = groups["sha"].strip() is_merge = bool(groups.get("merge", False)) - message = groups["message"].strip("\n") + # The message group runs to the next commit header, so the file + # stats land at the end of it and have to come back out before + # anything is stored. git indents every message line by four + # spaces, so an unindented stat line cannot be message text. + lines = groups["message"].strip("\n").split("\n") + stat_lines = [line for line in lines if NUMSTAT_LINE.match(line)] + message = "\n".join( + line for line in lines if not NUMSTAT_LINE.match(line) + ).strip("\n") message = "\n".join( [m[4:] if m.startswith(" ") else m for m in message.split("\n")] ) @@ -169,6 +193,7 @@ def get_commit_data_for_repo_versions(key, min_version=""): committed_at=committed_at, is_merge=is_merge, version=b, + docs_files_changed=count_doc_files(stat_lines), ) diff --git a/libraries/tests/test_github.py b/libraries/tests/test_github.py index 92a1e4063..de106d257 100644 --- a/libraries/tests/test_github.py +++ b/libraries/tests/test_github.py @@ -388,3 +388,76 @@ def test_parse_boostdep_artifact( library__key="numeric/conversion", version__name="boost-1.85.0" ) assert lv.dependencies.count() == 1 + + +GIT_LOG_WITH_NUMSTAT = """commit abc123 +Author: Peter Dimov +Date: 2024-01-02 10:00:00 +0000 + + Document the list algorithms + + With a second paragraph. + +12\t3\tdoc/index.adoc +-\t-\tdoc/img/diagram.png +40\t0\tinclude/boost/mp11/list.hpp + +commit def456 +Merge: 111 222 +Author: Someone Else +Date: 2024-01-03 10:00:00 +0000 + + Merge pull request #1 + +commit fff999 +Author: Peter Dimov +Date: 2024-01-04 10:00:00 +0000 + + Fix a typo in the header + +40\t0\tinclude/boost/mp11/list.hpp +""" + + +@pytest.fixture +def fake_git(monkeypatch): + """Answer the importer's git calls without a clone.""" + + def run(args, **kwargs): + completed = MagicMock() + completed.args = args + completed.stdout = b"" + completed.stderr = b"" + if args[1] == "clone": + completed.stderr = b"Cloning into bare repository" + elif "log" in args: + completed.stdout = GIT_LOG_WITH_NUMSTAT.encode() + elif "diff" in args: + completed.stdout = b" 3 files changed, 52 insertions(+), 3 deletions(-)" + return completed + + monkeypatch.setattr("libraries.github.subprocess.run", run) + + +@pytest.mark.django_db +def test_commit_parse_counts_doc_files(fake_git): + """Per-file stats are read, and the message is not polluted by them.""" + from libraries.github import ParsedCommit, get_commit_data_for_repo_versions + + baker.make(Library, key="mp11", github_url="https://github.com/boostorg/mp11") + + commits = [ + item + for item in get_commit_data_for_repo_versions("mp11") + if isinstance(item, ParsedCommit) + ] + + assert [(c.sha, c.docs_files_changed) for c in commits] == [ + ("abc123", 2), + ("def456", 0), + ("fff999", 0), + ] + assert commits[0].message == ( + "Document the list algorithms\n\nWith a second paragraph." + ) + assert commits[1].is_merge is True From 81c8314377674116f92d2e6679293f4f9cfc4496 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:34:34 -0300 Subject: [PATCH 08/20] feat: store the doc file count on each commit --- libraries/github.py | 9 ++++++- .../0044_commit_docs_files_changed.py | 18 +++++++++++++ libraries/models.py | 4 +++ libraries/tests/test_github.py | 25 +++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 libraries/migrations/0044_commit_docs_files_changed.py diff --git a/libraries/github.py b/libraries/github.py index 056db6869..d891ba5b6 100644 --- a/libraries/github.py +++ b/libraries/github.py @@ -528,6 +528,7 @@ def handle_commit(commit: ParsedCommit): message=commit.message, committed_at=commit.committed_at, is_merge=commit.is_merge, + docs_files_changed=commit.docs_files_changed, ) except KeyError: @@ -566,7 +567,13 @@ def handle_version_diff_stat(diff: VersionDiffStat): Commit.objects.bulk_create( commits, update_conflicts=True, - update_fields=["author", "message", "committed_at", "is_merge"], + update_fields=[ + "author", + "message", + "committed_at", + "is_merge", + "docs_files_changed", + ], unique_fields=["library_version", "sha"], ) LibraryVersion.objects.bulk_update( diff --git a/libraries/migrations/0044_commit_docs_files_changed.py b/libraries/migrations/0044_commit_docs_files_changed.py new file mode 100644 index 000000000..17031008a --- /dev/null +++ b/libraries/migrations/0044_commit_docs_files_changed.py @@ -0,0 +1,18 @@ +# Generated by Django 6.0.2 on 2026-08-19 14:34 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("libraries", "0043_libraryversion_website_adoc_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="commit", + name="docs_files_changed", + field=models.PositiveIntegerField(default=0), + ), + ] diff --git a/libraries/models.py b/libraries/models.py index d3dc57672..21077fc5f 100644 --- a/libraries/models.py +++ b/libraries/models.py @@ -433,6 +433,10 @@ class Commit(models.Model): message = models.TextField(default="") committed_at = models.DateTimeField(db_index=True) is_merge = models.BooleanField(default=False) + # A count rather than a flag, so weighting a commit by how much it documented + # stays possible without another import. Zero for merges, which change no + # files of their own. + docs_files_changed = models.PositiveIntegerField(default=0) class Meta: constraints = [ diff --git a/libraries/tests/test_github.py b/libraries/tests/test_github.py index de106d257..f6943b518 100644 --- a/libraries/tests/test_github.py +++ b/libraries/tests/test_github.py @@ -461,3 +461,28 @@ def test_commit_parse_counts_doc_files(fake_git): "Document the list algorithms\n\nWith a second paragraph." ) assert commits[1].is_merge is True + + +@pytest.mark.django_db +def test_reimport_heals_the_doc_count_without_moving_the_row(fake_git): + """The non-destructive path updates matched rows in place. + + Which is what lets commits imported before the field existed gain a count + without any grant pointing at them being disturbed. + """ + from libraries.models import Commit + + library = baker.make( + Library, key="mp11", github_url="https://github.com/boostorg/mp11" + ) + version = baker.make(LibraryVersion, library=library) + version.version.name = "master" + version.version.save() + stale = baker.make( + Commit, library_version=version, sha="abc123", docs_files_changed=0 + ) + + LibraryUpdater().update_commits(library) + + stale.refresh_from_db() + assert stale.docs_files_changed == 2 From 14a5db97962952c16726b25139236e814a427c4a Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:35:48 -0300 Subject: [PATCH 09/20] fix: discard grants for commits a clean re-import deletes --- libraries/github.py | 7 ++++++- libraries/tests/test_github.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/libraries/github.py b/libraries/github.py index d891ba5b6..4260c504d 100644 --- a/libraries/github.py +++ b/libraries/github.py @@ -29,6 +29,7 @@ LibraryVersion, PullRequest, ) +from badges.services import discard_source_achievements from core.githubhelper import GithubAPIClient, GithubDataParser from .utils import generate_fake_email, parse_boostdep_artifact, parse_date @@ -563,7 +564,11 @@ def handle_version_diff_stat(diff: VersionDiffStat): with transaction.atomic(): if clean: - Commit.objects.filter(library_version__library=library).delete() + # The rows come back with new ids, and a grant names its source by + # id with no link back, so the grants have to go with them. + doomed = Commit.objects.filter(library_version__library=library) + discard_source_achievements(Commit, doomed.values_list("pk", flat=True)) + doomed.delete() Commit.objects.bulk_create( commits, update_conflicts=True, diff --git a/libraries/tests/test_github.py b/libraries/tests/test_github.py index f6943b518..b1e313deb 100644 --- a/libraries/tests/test_github.py +++ b/libraries/tests/test_github.py @@ -486,3 +486,35 @@ def test_reimport_heals_the_doc_count_without_moving_the_row(fake_git): stale.refresh_from_db() assert stale.docs_files_changed == 2 + + +@pytest.mark.django_db +def test_clean_reimport_discards_the_grants_it_orphans( + fake_git, catalogue, django_user_model +): + """A wiped commit takes its grants with it, whatever source they came from. + + Left behind, they count toward a threshold forever: a reconcile can only match + a grant against what the iterator yields, and a deleted row yields nothing. + """ + from badges.models import Achievement, UserAchievement + from badges.tests.fixtures import grant_from_source + from libraries.models import Commit + + user = django_user_model.objects.create_user( + email="committer@example.com", password="x" + ) + library = baker.make( + Library, key="mp11", github_url="https://github.com/boostorg/mp11" + ) + version = baker.make(LibraryVersion, library=library) + version.version.name = "master" + version.version.save() + commit = baker.make(Commit, library_version=version, sha="abc123") + grant_from_source( + user, Achievement.objects.get(slug="code-commits"), commit, dedup_info="abc123" + ) + + LibraryUpdater().update_commits(library, clean=True) + + assert not UserAchievement.objects.filter(source_object_id=commit.pk).exists() From b6266bec4787957cfb68ddc506f04368d7b54180 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:37:32 -0300 Subject: [PATCH 10/20] feat: derive the Documenter achievement from doc-touching commits --- badges/sources.py | 22 ++++++++++++++++++++++ badges/tests/test_admin_badge_config.py | 3 ++- docs/badges-admin.md | 4 ++-- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/badges/sources.py b/badges/sources.py index 34e3f902b..3d925d138 100644 --- a/badges/sources.py +++ b/badges/sources.py @@ -113,6 +113,27 @@ def _iter_code_commits(): yield commit.author.user, commit, commit.sha +def _iter_documentation(): + """Yield (user, commit) for every attributed commit that touched docs. + + Merges are excluded explicitly as well as by their empty file list: merging + someone else's documentation is not writing it. + """ + from libraries.models import Commit + + commits = ( + Commit.objects.filter( + docs_files_changed__gt=0, + is_merge=False, + author__user__isnull=False, + ) + .select_related("author__user") + .iterator(chunk_size=1000) + ) + for commit in commits: + yield commit.author.user, commit, commit.sha + + def _iter_library_review(): """Yield (user, review) for every review submission with a linked user.""" from versions.models import Review @@ -131,6 +152,7 @@ def _iter_library_review(): AchievementSlug.LIBRARY_VERSIONING: _iter_library_versioning, AchievementSlug.CODE_COMMITS: _iter_code_commits, AchievementSlug.LIBRARY_REVIEW: _iter_library_review, + AchievementSlug.DOCUMENTATION: _iter_documentation, } # Derived, so the CLI choices can never drift from the wired iterators. diff --git a/badges/tests/test_admin_badge_config.py b/badges/tests/test_admin_badge_config.py index 159dc3cb3..4f39a0d9a 100644 --- a/badges/tests/test_admin_badge_config.py +++ b/badges/tests/test_admin_badge_config.py @@ -498,8 +498,9 @@ def test_the_changelist_flags_an_unwired_source(client, super_user, catalogue): 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["documenter"]) is True assert admin_class.source_wired(rows["regular"]) is False + assert admin_class.source_wired(rows["publisher"]) is False def test_the_changelist_counts_each_holder_once_and_skips_revocations( diff --git a/docs/badges-admin.md b/docs/badges-admin.md index 84b7ac61b..38b485621 100644 --- a/docs/badges-admin.md +++ b/docs/badges-admin.md @@ -13,7 +13,7 @@ when to run it. | Page | What it is for | What you do there | | --- | --- | --- | | **Achievements** | The catalogue of the eight achievement types: library authoring, library versioning, library maintenance, code commits, library review, documentation, mailing list and publisher. | Mostly read-only. The slug is the join key to the code that feeds a type, so it freezes once the row exists. Add a type only for a genuinely new achievement - a manual-only type needs no code, an automatic one needs a new source (a deploy). | -| **Badges** | One row per badge (Library Author, Version Author, Maintainer, Commits Master, Reviewer, Documenter, Regular, Publisher): the achievement that feeds it, its live ladder of thresholds, how many members hold it and whether an automatic source feeds it. | Opening a badge is one form: its description plus all active tiers, saved together. **This is where thresholds are changed** - see "Changing a badge's tiers" below. The Automatic column marks the sources that self-refresh; the other three badges only ever move on a manual grant. | +| **Badges** | One row per badge (Library Author, Version Author, Maintainer, Commits Master, Reviewer, Documenter, Regular, Publisher): the achievement that feeds it, its live ladder of thresholds, how many members hold it and whether an automatic source feeds it. | Opening a badge is one form: its description plus all active tiers, saved together. **This is where thresholds are changed** - see "Changing a badge's tiers" below. The Automatic column marks the sources that self-refresh; the other two badges only ever move on a manual grant. | | **Badge tiers** | The history behind the ladder. Deliberately hidden from this index - tiers are configured on the badge page - and reached through a badge's "N retired tier(s)" link. | Recovery only: retired rows are listed here, and the Reactivate action undoes a mistaken retirement. The rows themselves are immutable. | | **User achievements** | One row per achievement a member has earned. Automatic grants link to the row that justified them; manual ones carry the granting admin and their note. | The grant, correction and sync surface: add a manual grant (note required), invalidate or revalidate a grant (audited), and run the Backfill and Reconcile jobs. | | **User badges** | The derived badge state, one row per badge a member holds or has held. Read-only, because only the recalculation service writes it. | Filter by Held / Revoked or by revocation source, follow the user link to the per-member page, revoke or reinstate a badge (audited), and run the Recalculate job. | @@ -33,7 +33,7 @@ this section emails a member. | **Reconcile** | Two-way, the only operation that removes: it adds what a source now supports and deletes the stored grants it no longer does (a commit reassigned to another author, a news post deleted, a maintainer dropped, an account deleted), then recalculates the members that moved. You see a preview of the changes before anything runs. Manual grants are never touched. | "Reconcile achievements" button on the User achievements page (scoped like backfill), the per-member page, or `manage.py reconcile_achievements` with `--dry-run`, `--user`, `--source` or `--remove-only`. Requires the delete permission. | Not scheduled. Run it after an upstream data correction that backfill cannot undo - a fixed attribution, deleted content, a dropped maintainer, a deleted account - scoped to the member or source you meant to fix. A source that reads empty is treated as broken and nothing is removed; overriding that needs `--allow-empty` from the shell. | | **Recalculate** | Rebuilds badges from the achievements already on record: awards every tier whose threshold is met and cascade-revokes every one that has fallen below it. No achievement is added, removed or changed - the safe thing to run after editing a badge's thresholds. Idempotent. | "Recalculate badges" button on the User badges page (whole table, in the background), or `manage.py recalculate_badges`. | After editing a badge's thresholds, after fixing data, after restoring a dump. Safe at any time. | | **Recalculate / Reconcile this member** | The same two jobs for one member, run synchronously so the result is visible on the page you are already on. | Buttons at the bottom of the per-member page. | For a support request about one member. The page itself says which is needed: "held below its threshold" or "grants already reach X" call for a recalculate; a source disagreement calls for a reconcile. | -| **Manual grant** | Grants an achievement by hand for something no source can see (Documenter, Regular, and any special case). A note is required and the granting admin is recorded, and the member is not notified. | Add on the User achievements page, or "Grant an achievement" on the per-member page. | Whenever a member earned something the sources cannot derive. Backfill and reconcile never touch it. | +| **Manual grant** | Grants an achievement by hand for something no source can see (Regular, and any special case). A note is required and the granting admin is recorded, and the member is not notified. | Add on the User achievements page, or "Grant an achievement" on the per-member page. | Whenever a member earned something the sources cannot derive. Backfill and reconcile never touch it. | | **Invalidate / Revalidate** | Corrects a grant in place: invalidation soft-deletes it with a required note (who, when, why - the row stays for the audit trail), and the badge follows - cascade-revoked if the count drops below its threshold. Revalidate undoes it, clears the trail, and re-awards. | Select rows on the User achievements page and pick the action. | When a grant was wrong but must stay on record. There is no hard delete. | | **Revoke / Reinstate** | The badge-side override: revoke takes a badge away with a required note; it survives every recalculation, and only reinstate brings it back. Reinstate skips cascade-revoked badges - their count is still below the threshold. | Select rows on the User badges page and pick the action. | When a member should not show a badge regardless of count - a policy decision, a dispute, a misused event badge. Never for correcting data: that is what invalidate and reconcile are for. | From ef4924123fcfdcc92b9b286385834a68fd0fe86d Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Wed, 19 Aug 2026 11:40:31 -0300 Subject: [PATCH 11/20] test: cover the documentation source end to end --- badges/tests/test_dedup_keys.py | 40 +++++++++++++++++++++++++++++++++ badges/tests/test_sources.py | 21 +++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/badges/tests/test_dedup_keys.py b/badges/tests/test_dedup_keys.py index db5229f70..caaf76936 100644 --- a/badges/tests/test_dedup_keys.py +++ b/badges/tests/test_dedup_keys.py @@ -152,3 +152,43 @@ def test_source_key_formats(plain_user): assert [key for _, _, key in sources._iter_library_review()] == [ "boostmp11|peterdimov|april1102017" ] + + +def test_a_doc_commit_earns_the_documenter_badge(plain_user): + """End to end: a doc commit grants and awards, a code-only commit does not.""" + from badges.models import UserBadge + + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author, sha="d0c1", docs_files_changed=3) + baker.make("libraries.Commit", author=author, sha="c0de", docs_files_changed=0) + + call_command("backfill_achievements", "--source", "documentation") + + grants = UserAchievement.objects.filter( + user=plain_user, achievement__slug="documentation" + ) + assert [g.dedup_info for g in grants] == ["d0c1"] + assert UserBadge.objects.filter( + user=plain_user, badge__label="documenter", revoked_at__isnull=True + ).exists() + + +def test_backfilling_documentation_twice_grants_once(plain_user): + """The second sweep of the weekly pipeline must not double anyone's count.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author, sha="d0c1", docs_files_changed=3) + + call_command("backfill_achievements", "--source", "documentation") + call_command("backfill_achievements", "--source", "documentation") + + assert ( + UserAchievement.objects.filter( + user=plain_user, achievement__slug="documentation" + ).count() + == 1 + ) + + +def test_documentation_is_selectable_as_a_source(): + """The CLI choices are derived from what is wired, so this cannot drift.""" + assert "documentation" in sources.AUTOMATIC_SLUGS diff --git a/badges/tests/test_sources.py b/badges/tests/test_sources.py index a0e4f1bab..2eb0b0561 100644 --- a/badges/tests/test_sources.py +++ b/badges/tests/test_sources.py @@ -113,3 +113,24 @@ def test_iter_library_review_skips_unlinked(plain_user): ) pairs = list(sources._iter_library_review()) assert [u for u, _, _ in pairs] == [plain_user] + + +def test_iter_documentation_skips_commits_that_touched_no_docs(plain_user): + """Only commits with a doc file to their name are yielded.""" + author = baker.make("libraries.CommitAuthor", user=plain_user) + documented = baker.make("libraries.Commit", author=author, docs_files_changed=2) + baker.make("libraries.Commit", author=author, docs_files_changed=0) + + assert list(sources._iter_documentation()) == [ + (plain_user, documented, documented.sha) + ] + + +def test_iter_documentation_skips_merges_and_unlinked_authors(plain_user): + """A merge did not write the docs, and an unclaimed author is nobody yet.""" + linked = baker.make("libraries.CommitAuthor", user=plain_user) + unlinked = baker.make("libraries.CommitAuthor", user=None) + baker.make("libraries.Commit", author=linked, docs_files_changed=1, is_merge=True) + baker.make("libraries.Commit", author=unlinked, docs_files_changed=1) + + assert list(sources._iter_documentation()) == [] From e18aa9e3118c04df073cdbf9359238a5663e116d Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Thu, 20 Aug 2026 15:08:57 -0300 Subject: [PATCH 12/20] fix: end a parsed commit at the next header, not the next word --- libraries/github.py | 5 ++++- libraries/tests/test_github.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/libraries/github.py b/libraries/github.py index 4260c504d..caeb95884 100644 --- a/libraries/github.py +++ b/libraries/github.py @@ -85,7 +85,10 @@ def get_commit_data_for_repo_versions(key, min_version=""): parser = re.compile( r"^commit (?P\w+)(?:\n(?PMerge).*)?\nAuthor: (?P[^\<]+)" r"\s+\<(?P[^\>]+)\>\nDate:\s+(?P.*)\n(?P(.|\n)+?)" - r"(?=(commit|\Z))", + # Anchored: the word "commit" is ordinary text in a message body and an + # ordinary word in a path, and an unanchored lookahead ends the match at + # the first of either - dropping the file stats that follow it. + r"(?=^commit |\Z)", flags=re.MULTILINE, ) re.compile( diff --git a/libraries/tests/test_github.py b/libraries/tests/test_github.py index b1e313deb..e7c4ad6c6 100644 --- a/libraries/tests/test_github.py +++ b/libraries/tests/test_github.py @@ -416,6 +416,18 @@ def test_parse_boostdep_artifact( Fix a typo in the header 40\t0\tinclude/boost/mp11/list.hpp + +commit eee888 +Author: Peter Dimov +Date: 2024-01-05 10:00:00 +0000 + + Rework the guide + + This commit rewrites doc/commit.adoc and the tutorial. + +3\t1\tdoc/commit.adoc +7\t0\tdoc/tutorial.adoc +2\t2\tsrc/commit_log.cpp """ @@ -456,11 +468,17 @@ def test_commit_parse_counts_doc_files(fake_git): ("abc123", 2), ("def456", 0), ("fff999", 0), + ("eee888", 2), ] assert commits[0].message == ( "Document the list algorithms\n\nWith a second paragraph." ) assert commits[1].is_merge is True + # "commit" as a word in the body and in a path: both used to end the match + # early, taking the file stats with them and reading as no documentation. + assert commits[3].message == ( + "Rework the guide\n\nThis commit rewrites doc/commit.adoc and the tutorial." + ) @pytest.mark.django_db From e67191ba214e14fbef81a9bca38d1c712b817740 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Thu, 20 Aug 2026 15:10:43 -0300 Subject: [PATCH 13/20] fix: keep badges through a clean commit re-import --- badges/services.py | 46 ++++++++++++++++++++++-- libraries/github.py | 23 +++++++++--- libraries/tests/test_github.py | 64 +++++++++++++++++++++++++++------- 3 files changed, 114 insertions(+), 19 deletions(-) diff --git a/badges/services.py b/badges/services.py index 36ef1a69b..dc2960b6a 100644 --- a/badges/services.py +++ b/badges/services.py @@ -11,8 +11,9 @@ 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. +that source in both directions, ``discard_source_achievements``, for source rows +about to be deleted outright, and ``relink_source_achievements``, for rows about to +be replaced by the same evidence under new ids. 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``. @@ -138,6 +139,47 @@ def discard_source_achievements(model, object_ids): recalculate_badges(user_id, achievement_id) +def relink_source_achievements(model, ids_by_key): + """Re-point automatic grants at rows re-created under the same dedup key. + + A caller that deletes and re-inserts the same evidence - the commit importer + running destructively - leaves every pointer to it dangling, because a generic + foreign key carries no referential integrity. The dedup key is what survives + that swap, so the link is rebuilt from it rather than the grant thrown away: + discarding a grant revokes the badge it justifies, records that revocation + permanently, and re-earns the badge with today's date on the next sync, so a + re-import would rewrite history that nothing actually changed. + + No grant appears or disappears, so no count moves and nothing is recalculated. + + Args: + model: The model the grants point at. + ids_by_key: The new row id for each dedup key, as the source names it. + + Returns: + How many grants were re-pointed. + """ + if not ids_by_key: + return 0 + content_type = ContentType.objects.get_for_model(model) + grants = UserAchievement.objects.filter( + source_content_type=content_type, + source_type=SourceType.AUTOMATIC, + dedup_info__in=list(ids_by_key), + ).only("pk", "dedup_info", "source_object_id") + moved = [] + for grant in grants.iterator(chunk_size=2000): + object_id = ids_by_key[grant.dedup_info] + if object_id != grant.source_object_id: + grant.source_object_id = object_id + moved.append(grant) + if moved: + UserAchievement.objects.bulk_update( + moved, ["source_object_id"], batch_size=SYNC_BATCH_SIZE + ) + return len(moved) + + class SourceSync(NamedTuple): """What syncing one source found, and what it was allowed to do about it. diff --git a/libraries/github.py b/libraries/github.py index caeb95884..787839cce 100644 --- a/libraries/github.py +++ b/libraries/github.py @@ -29,7 +29,7 @@ LibraryVersion, PullRequest, ) -from badges.services import discard_source_achievements +from badges.services import discard_source_achievements, relink_source_achievements from core.githubhelper import GithubAPIClient, GithubDataParser from .utils import generate_fake_email, parse_boostdep_artifact, parse_date @@ -566,11 +566,10 @@ def handle_version_diff_stat(diff: VersionDiffStat): assert_never() with transaction.atomic(): + doomed_ids = [] if clean: - # The rows come back with new ids, and a grant names its source by - # id with no link back, so the grants have to go with them. doomed = Commit.objects.filter(library_version__library=library) - discard_source_achievements(Commit, doomed.values_list("pk", flat=True)) + doomed_ids = list(doomed.values_list("pk", flat=True)) doomed.delete() Commit.objects.bulk_create( commits, @@ -584,6 +583,22 @@ def handle_version_diff_stat(diff: VersionDiffStat): ], unique_fields=["library_version", "sha"], ) + if clean: + # The same commits are back under new ids, and a grant names its + # evidence by sha, so the pointers are rebuilt rather than the + # grants dropped: dropping them would revoke the badges they + # justify and re-earn them dated today on the next sync. + relink_source_achievements( + Commit, + dict( + Commit.objects.filter( + library_version__library=library + ).values_list("sha", "pk") + ), + ) + # Whatever still points into the deleted ids is evidence that did + # not come back, so those grants really are stale. + discard_source_achievements(Commit, doomed_ids) LibraryVersion.objects.bulk_update( library_version_updates, ["insertions", "deletions", "files_changed"], diff --git a/libraries/tests/test_github.py b/libraries/tests/test_github.py index e7c4ad6c6..e997b47a8 100644 --- a/libraries/tests/test_github.py +++ b/libraries/tests/test_github.py @@ -506,16 +506,10 @@ def test_reimport_heals_the_doc_count_without_moving_the_row(fake_git): assert stale.docs_files_changed == 2 -@pytest.mark.django_db -def test_clean_reimport_discards_the_grants_it_orphans( - fake_git, catalogue, django_user_model -): - """A wiped commit takes its grants with it, whatever source they came from. - - Left behind, they count toward a threshold forever: a reconcile can only match - a grant against what the iterator yields, and a deleted row yields nothing. - """ - from badges.models import Achievement, UserAchievement +def _library_with_granted_commit(django_user_model, sha): + """A member holding a code-commits grant for one commit of one library.""" + from badges.models import Achievement + from badges.services import recalculate_badges from badges.tests.fixtures import grant_from_source from libraries.models import Commit @@ -528,11 +522,55 @@ def test_clean_reimport_discards_the_grants_it_orphans( version = baker.make(LibraryVersion, library=library) version.version.name = "master" version.version.save() - commit = baker.make(Commit, library_version=version, sha="abc123") - grant_from_source( - user, Achievement.objects.get(slug="code-commits"), commit, dedup_info="abc123" + commit = baker.make(Commit, library_version=version, sha=sha) + achievement = Achievement.objects.get(slug="code-commits") + grant_from_source(user, achievement, commit, dedup_info=sha) + recalculate_badges(user.pk, achievement.pk) + return library, commit + + +@pytest.mark.django_db +def test_clean_reimport_keeps_the_badges_it_would_have_revoked( + fake_git, catalogue, django_user_model +): + """Evidence that comes back keeps its grant, so no badge moves. + + The grant is re-pointed at the new row instead of deleted. Deleting it would + cascade-revoke the badge it justifies, record that revocation permanently, and + re-earn it dated today once the next sync put the grant back - rewriting + history for a re-import in which nothing actually changed. + """ + from badges.models import UserAchievement, UserBadge + from libraries.models import Commit + + library, commit = _library_with_granted_commit(django_user_model, "abc123") + badges = set(UserBadge.objects.values_list("pk", "awarded_at", "revoked_at")) + assert badges, "nothing was awarded, so the assertion below proves nothing" + + LibraryUpdater().update_commits(library, clean=True) + + grant = UserAchievement.objects.get(dedup_info="abc123") + assert grant.source_object_id == Commit.objects.get(sha="abc123").pk + assert grant.source_object_id != commit.pk + assert ( + set(UserBadge.objects.values_list("pk", "awarded_at", "revoked_at")) == badges ) + +@pytest.mark.django_db +def test_clean_reimport_discards_grants_for_commits_that_do_not_return( + fake_git, catalogue, django_user_model +): + """A wiped commit the repository no longer reports takes its grants with it. + + Left behind, they count toward a threshold forever: a reconcile can only match + a grant against what the iterator yields, and a deleted row yields nothing. + """ + from badges.models import UserAchievement + + library, commit = _library_with_granted_commit(django_user_model, "deadbeef") + LibraryUpdater().update_commits(library, clean=True) + assert not UserAchievement.objects.filter(dedup_info="deadbeef").exists() assert not UserAchievement.objects.filter(source_object_id=commit.pk).exists() From 74356c2d7deb974b2568b32865286c1555b89ee9 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Thu, 20 Aug 2026 16:14:59 -0300 Subject: [PATCH 14/20] test: walk every wired source twice and require nothing to move --- badges/tests/test_dedup_keys.py | 173 +++++++++++++++++++++++++++++++- 1 file changed, 172 insertions(+), 1 deletion(-) diff --git a/badges/tests/test_dedup_keys.py b/badges/tests/test_dedup_keys.py index caaf76936..be23e4965 100644 --- a/badges/tests/test_dedup_keys.py +++ b/badges/tests/test_dedup_keys.py @@ -10,7 +10,8 @@ from model_bakery import baker from badges import sources -from badges.models import Achievement, SourceType, UserAchievement +from badges.models import Achievement, SourceType, UserAchievement, UserBadge +from badges.services import recalculate_badges, sync_source from badges.tests.fixtures import grant_from_source pytestmark = pytest.mark.django_db @@ -192,3 +193,173 @@ def test_backfilling_documentation_twice_grants_once(plain_user): def test_documentation_is_selectable_as_a_source(): """The CLI choices are derived from what is wired, so this cannot drift.""" assert "documentation" in sources.AUTOMATIC_SLUGS + + +@pytest.fixture +def every_source(plain_user): + """One piece of evidence for each of the six wired sources, all one member. + + A round-trip test is only worth anything if the source actually yielded + something, so the tests assert on the backfill rather than trusting this. + """ + library = baker.make("libraries.Library", key="mp11") + library.authors.add(plain_user) + version = baker.make( + "libraries.LibraryVersion", + library=library, + version=baker.make("versions.Version", name="boost-1.88.0"), + ) + version.authors.add(plain_user) + version.maintainers.add(plain_user) + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make( + "libraries.Commit", + author=author, + library_version=version, + sha="cafe1234", + docs_files_changed=2, + is_merge=False, + ) + review = baker.make( + "versions.Review", + submission="Boost.MP11", + submitter_raw="Peter Dimov", + review_dates="April 1-10, 2017", + ) + review.submitters.add(author) + + +@pytest.mark.parametrize("slug", sources.AUTOMATIC_SLUGS) +def test_a_second_walk_recognises_everything_the_first_wrote(slug, every_source): + """Walk a source twice over unchanged data and nothing may move. + + What this catches is a key the walk cannot reproduce - one built from a clock, + a counter, or an unstable ordering rather than from the evidence itself. The + count would climb on every sweep and the reconcile behind it would delete what + it failed to recognise, neither of them raising anything. + + What it does *not* catch is a key that is merely fragile, a row id being the + obvious one: nothing re-creates rows inside a single test, so a row id looks + perfectly stable here. That property belongs to the two sources whose rows the + importers actually delete and insert again - see + ``test_reimporting_with_new_ids_changes_nothing`` for commits and + ``test_reimporting_a_review_keeps_its_grant`` for reviews. + + Parametrised over the wired sources, so a source added later is covered by + having been added rather than by somebody remembering. + """ + achievement = Achievement.objects.get(slug=slug) + + backfill = sync_source(slug, achievement, remove=False) + assert backfill.added > 0, f"the fixture fed '{slug}' nothing to grant" + + again = sync_source(slug, achievement, dry_run=True) + + assert (again.added, again.removed) == (0, 0) + + +def test_replacing_an_unkeyed_grant_never_moves_the_badge(plain_user): + """Converting an environment must not disturb the badges on the way through. + + The walk inserts before it deletes, so the count never dips below the + threshold and the tier is neither revoked nor re-earned. If that order ever + changes, every member converted would have their award date reset to the day + of the deploy. + """ + achievement = Achievement.objects.get(slug="code-commits") + commit = _commit(plain_user, "cafe1234") + grant_from_source(plain_user, achievement, commit) + recalculate_badges(plain_user.pk, achievement.pk) + badges = set(UserBadge.objects.values_list("pk", "awarded_at", "revoked_at")) + assert badges, "nothing was awarded, so the assertion below proves nothing" + + call_command("reconcile_achievements", "--source", "code-commits") + + assert _grants(plain_user).get().dedup_info == "cafe1234" + assert ( + set(UserBadge.objects.values_list("pk", "awarded_at", "revoked_at")) == badges + ) + + +def test_one_sha_feeds_two_achievements_independently(plain_user): + """Code Commits and Documenter both name a commit by its sha. + + The key is unique per achievement, not globally, so one doc-touching commit + earns both. If those two are ever merged into one achievement, this is the + test that says so. + """ + author = baker.make("libraries.CommitAuthor", user=plain_user) + baker.make("libraries.Commit", author=author, sha="d0c1", docs_files_changed=3) + + call_command("backfill_achievements", "--source", "code-commits") + call_command("backfill_achievements", "--source", "documentation") + + grants = UserAchievement.objects.filter(user=plain_user, dedup_info="d0c1") + assert set(grants.values_list("achievement__slug", flat=True)) == { + "code-commits", + "documentation", + } + + +def test_a_scoped_reconcile_leaves_other_members_alone(plain_user, super_user): + """The per-member admin button reconciles one member, not the table. + + The scope is applied to the stored rows as well as inside the walk, and only + the first of those stops another member's grants being read as stale - every + key outside the scope is simply absent from the comparison. + """ + achievement = Achievement.objects.get(slug="code-commits") + _commit(super_user, "beef0001") + call_command("backfill_achievements", "--source", "code-commits") + untouched = set( + UserAchievement.objects.filter(user=super_user).values_list("pk", "dedup_info") + ) + assert untouched, "the other member holds nothing, so this proves nothing" + + orphan = _commit(plain_user, "dead0002") + grant_from_source(plain_user, achievement, orphan, dedup_info="dead0002") + orphan.delete() + + sync_source("code-commits", achievement, user_ids=[plain_user.pk]) + + assert not _grants(plain_user).exists() + assert ( + set( + UserAchievement.objects.filter(user=super_user).values_list( + "pk", "dedup_info" + ) + ) + == untouched + ) + + +def test_reimporting_a_review_keeps_its_grant(plain_user): + """A review's fingerprint survives the row being deleted and imported again. + + Reviews are the only source besides commits whose rows are genuinely + re-created - ``import_reviews --clean`` empties the table first. This covers + the key rather than the command: the command also discards its own grants + before deleting, which is a separate question from whether the key holds. + """ + from versions.models import Review + + author = baker.make("libraries.CommitAuthor", user=plain_user) + fields = { + "submission": "Boost.MP11", + "submitter_raw": "Peter Dimov", + "review_dates": "April 1-10, 2017", + } + baker.make("versions.Review", **fields).submitters.add(author) + call_command("backfill_achievements", "--source", "library-review") + grant = UserAchievement.objects.get(achievement__slug="library-review") + + Review.objects.all().delete() + replacement = baker.make("versions.Review", **fields) + replacement.submitters.add(author) + assert replacement.pk != grant.source_object_id + + call_command("reconcile_achievements", "--source", "library-review") + + assert ( + UserAchievement.objects.get(achievement__slug="library-review").pk == grant.pk + ) From 3eecf36f8b3e1e179fb0783ca8ec1bb6745239ce Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Thu, 20 Aug 2026 16:15:26 -0300 Subject: [PATCH 15/20] test: pin what re-pointing a grant may and may not touch --- badges/tests/test_relink.py | 120 ++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 badges/tests/test_relink.py diff --git a/badges/tests/test_relink.py b/badges/tests/test_relink.py new file mode 100644 index 000000000..e5746b3a9 --- /dev/null +++ b/badges/tests/test_relink.py @@ -0,0 +1,120 @@ +"""Tests for re-pointing a grant at evidence that was created again. + +The commit importer's destructive mode deletes every row for a library and inserts +the same commits back under new ids. Discarding the grants instead of re-pointing +them revokes the badges they justify, records those revocations permanently, and +re-earns the badges dated today on the next sync - so a re-import would rewrite +history that nothing actually changed. +""" + +import pytest +from django.contrib.contenttypes.models import ContentType +from model_bakery import baker + +from badges.models import Achievement, SourceType, UserAchievement +from badges.services import relink_source_achievements +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 _commit(user, sha): + """One commit attributed to ``user``.""" + author = baker.make("libraries.CommitAuthor", user=user) + return baker.make("libraries.Commit", author=author, sha=sha) + + +def test_a_grant_follows_its_evidence_to_the_new_row(plain_user): + """The sha is what survives the swap, so the pointer is rebuilt from it.""" + from libraries.models import Commit + + achievement = Achievement.objects.get(slug="code-commits") + old = _commit(plain_user, "cafe1234") + grant_from_source(plain_user, achievement, old, dedup_info="cafe1234") + replacement = baker.make("libraries.Commit", author=old.author, sha="cafe1234") + assert replacement.pk != old.pk + + assert relink_source_achievements(Commit, {"cafe1234": replacement.pk}) == 1 + + grant = UserAchievement.objects.get(dedup_info="cafe1234") + assert grant.source_object_id == replacement.pk + + +def test_a_manual_grant_is_left_where_the_admin_put_it(plain_user): + """The filter is on source type as well as key. + + An admin's row is not the engine's to move, even in the odd case where it + carries a key: only automatic grants are derived from a source. + """ + from libraries.models import Commit + + commit = _commit(plain_user, "cafe1234") + manual = UserAchievement.objects.create( + user=plain_user, + achievement=Achievement.objects.get(slug="code-commits"), + source_type=SourceType.MANUAL, + source_content_type=ContentType.objects.get_for_model(Commit), + source_object_id=commit.pk, + dedup_info="cafe1234", + ) + + assert relink_source_achievements(Commit, {"cafe1234": commit.pk + 1_000}) == 0 + + manual.refresh_from_db() + assert manual.source_object_id == commit.pk + + +def test_a_grant_for_another_model_is_left_alone(plain_user): + """Keyed on content type too, because key strings are per source. + + Nothing stops a review fingerprint colliding with a sha one day, and a + commit re-import has no business touching a review's grant either way. + """ + from libraries.models import Commit + + review = baker.make( + "versions.Review", + submission="Boost.MP11", + submitter_raw="Peter Dimov", + review_dates="April 1-10, 2017", + ) + grant_from_source( + plain_user, + Achievement.objects.get(slug="library-review"), + review, + dedup_info="cafe1234", + ) + + assert relink_source_achievements(Commit, {"cafe1234": 999_999}) == 0 + + assert UserAchievement.objects.get(dedup_info="cafe1234").source_object_id == ( + review.pk + ) + + +def test_a_pointer_that_has_not_moved_is_not_rewritten(plain_user): + """A non-destructive import re-creates nothing, so there is nothing to move.""" + from libraries.models import Commit + + commit = _commit(plain_user, "cafe1234") + grant_from_source( + plain_user, + Achievement.objects.get(slug="code-commits"), + commit, + dedup_info="cafe1234", + ) + + assert relink_source_achievements(Commit, {"cafe1234": commit.pk}) == 0 + + +def test_an_empty_map_touches_the_database_not_at_all(django_assert_num_queries): + """The importer calls this on every clean run, including ones with no grants.""" + from libraries.models import Commit + + with django_assert_num_queries(0): + assert relink_source_achievements(Commit, {}) == 0 From 3ec4cf085681ddf735ed1331823681fd3e1b6c9d Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Thu, 20 Aug 2026 16:15:36 -0300 Subject: [PATCH 16/20] test: cover awkward commit logs and repeated imports --- libraries/tests/test_github.py | 186 +++++++++++++++++++++++++++++++++ 1 file changed, 186 insertions(+) diff --git a/libraries/tests/test_github.py b/libraries/tests/test_github.py index e997b47a8..c09098fd4 100644 --- a/libraries/tests/test_github.py +++ b/libraries/tests/test_github.py @@ -574,3 +574,189 @@ def test_clean_reimport_discards_grants_for_commits_that_do_not_return( assert not UserAchievement.objects.filter(dedup_info="deadbeef").exists() assert not UserAchievement.objects.filter(source_object_id=commit.pk).exists() + + +def _parse_log(monkeypatch, log): + """Run the importer's parser over ``log`` without cloning anything. + + Separate from ``fake_git`` so a test can supply the log shape it is about, + rather than adding to a fixture every other test also reads. + """ + + def run(args, **kwargs): + completed = MagicMock() + completed.args = args + completed.stdout = b"" + completed.stderr = b"" + if args[1] == "clone": + completed.stderr = b"Cloning into bare repository" + elif "log" in args: + completed.stdout = log.encode() + elif "diff" in args: + completed.stdout = b" 1 file changed, 1 insertion(+)" + return completed + + monkeypatch.setattr("libraries.github.subprocess.run", run) + from libraries.github import ParsedCommit, get_commit_data_for_repo_versions + + baker.make(Library, key="mp11", github_url="https://github.com/boostorg/mp11") + return [ + item + for item in get_commit_data_for_repo_versions("mp11") + if isinstance(item, ParsedCommit) + ] + + +def _log_entry(body, stats): + """One commit as ``git log --numstat`` writes it, message body already indented.""" + return ( + "commit abc123\n" + "Author: Peter Dimov \n" + "Date: 2024-01-02 10:00:00 +0000\n" + "\n" + body + "\n" + stats + ) + + +@pytest.mark.django_db +@pytest.mark.parametrize( + ("body", "stats", "doc_files", "message"), + [ + pytest.param("", "1\t0\tdoc/a.adoc\n", 1, "", id="no-message-at-all"), + pytest.param( + " commit abc1234 was reverted\n", + "1\t0\tdoc/a.adoc\n", + 1, + "commit abc1234 was reverted", + id="body-line-that-reads-like-a-header", + ), + pytest.param( + " Move the guide\n", + "2\t2\tdoc/{old => new}/guide.adoc\n", + 1, + "Move the guide", + id="rename-inside-a-shared-prefix", + ), + pytest.param( + " Rename it\n", + "1\t1\told.adoc => doc/new.adoc\n", + 1, + "Rename it", + id="rename-with-no-shared-prefix", + ), + pytest.param( + " Add a diagram\n", + "-\t-\tdoc/img/a.png\n", + 1, + "Add a diagram", + id="binary-file-reports-dashes", + ), + pytest.param( + " Fix the title\n", + "1\t0\tdoc/my guide.adoc\n", + 1, + "Fix the title", + id="path-containing-a-space", + ), + pytest.param( + " Drop the old build\n", + "0\t80\tdoc/html/index.html\n0\t4\tdoc/Jamfile.v2\n", + 0, + "Drop the old build", + id="generated-output-and-build-files-do-not-count", + ), + ], +) +def test_the_parser_survives_awkward_log_shapes( + monkeypatch, body, stats, doc_files, message +): + """The message and the file stats share one region and must not bleed. + + The indented-header case is the one that matters most: the lookahead ending a + commit is anchored to column zero precisely because git indents every message + line by four spaces, so a body can say the word and still be a body. + """ + commits = _parse_log(monkeypatch, _log_entry(body, stats)) + + assert len(commits) == 1 + assert commits[0].docs_files_changed == doc_files + assert commits[0].message == message + + +@pytest.mark.django_db +def test_a_commit_touching_thousands_of_files_is_counted_in_full(monkeypatch): + """The stats are read line by line, so a huge commit has no special case.""" + stats = "".join(f"1\t0\tdoc/page{n}.adoc\n" for n in range(1_200)) + + commits = _parse_log(monkeypatch, _log_entry(" Import the manual\n", stats)) + + assert commits[0].docs_files_changed == 1_200 + assert commits[0].message == "Import the manual" + + +@pytest.mark.django_db +def test_a_second_non_destructive_import_changes_nothing(fake_git): + """The nightly task re-walks ranges it has already imported, every night.""" + from libraries.models import Commit + + library = baker.make( + Library, key="mp11", github_url="https://github.com/boostorg/mp11" + ) + version = baker.make(LibraryVersion, library=library) + version.version.name = "master" + version.version.save() + + LibraryUpdater().update_commits(library) + first = set( + Commit.objects.values_list("pk", "sha", "message", "docs_files_changed") + ) + assert first, "nothing was imported, so the assertion below proves nothing" + + LibraryUpdater().update_commits(library) + + assert ( + set(Commit.objects.values_list("pk", "sha", "message", "docs_files_changed")) + == first + ) + + +@pytest.mark.django_db +def test_two_destructive_imports_in_a_row_leave_the_badges_alone( + fake_git, catalogue, django_user_model +): + """Once is not enough: the second run re-points grants the first one moved.""" + from badges.models import UserAchievement, UserBadge + + library, _ = _library_with_granted_commit(django_user_model, "abc123") + badges = set(UserBadge.objects.values_list("pk", "awarded_at", "revoked_at")) + assert badges, "nothing was awarded, so the assertion below proves nothing" + + for _ in range(2): + LibraryUpdater().update_commits(library, clean=True) + + assert UserAchievement.objects.filter(dedup_info="abc123").count() == 1 + assert ( + set(UserBadge.objects.values_list("pk", "awarded_at", "revoked_at")) == badges + ) + + +@pytest.mark.django_db +def test_the_stored_message_is_no_longer_cut_short_at_the_word_commit(fake_git): + """The parser fix changes a field the site renders, so it is pinned here. + + Every message in the database was truncated at the first lowercase "commit" + before this, which is why none of them contained the word. + """ + from libraries.models import Commit + + library = baker.make( + Library, key="mp11", github_url="https://github.com/boostorg/mp11" + ) + version = baker.make(LibraryVersion, library=library) + version.version.name = "master" + version.version.save() + + LibraryUpdater().update_commits(library) + + assert Commit.objects.get(sha="eee888").message == ( + "Rework the guide\n\nThis commit rewrites doc/commit.adoc and the tutorial." + ) From b140f4f9aa62382437eea87c62531c80acd91c9f Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Thu, 20 Aug 2026 16:16:17 -0300 Subject: [PATCH 17/20] test: pin the review fingerprint both callers share --- versions/tests/test_review_keys.py | 66 ++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 versions/tests/test_review_keys.py diff --git a/versions/tests/test_review_keys.py b/versions/tests/test_review_keys.py new file mode 100644 index 000000000..fec90ee64 --- /dev/null +++ b/versions/tests/test_review_keys.py @@ -0,0 +1,66 @@ +"""Tests for the review fingerprint, now that two callers depend on it. + +``import_reviews`` uses it to recognise a review it has already imported. +``Review.dedup_key`` uses it to name that review to the achievement engine. They +are two call sites of one function on purpose: if they ever disagree about what +counts as the same review, review grants duplicate and nothing says so. +""" + +import pytest +from model_bakery import baker + +from versions.review_keys import normalize, review_key + +pytestmark = pytest.mark.django_db + + +def _review(**overrides): + """A stored review, overriding any of the three fingerprinted fields.""" + fields = { + "submission": "Boost.MP11", + "submitter_raw": "Peter Dimov", + "review_dates": "April 1-10, 2017", + } + fields.update(overrides) + return baker.make("versions.Review", **fields) + + +def test_a_review_names_itself_the_way_the_importer_matches_it(): + """The model's key is the importer's fingerprint joined, not a second rule.""" + review = _review() + + assert review.dedup_key == "|".join( + review_key(review.submission, review.submitter_raw, review.review_dates) + ) + + +def test_two_spellings_the_importer_would_collapse_share_one_key(): + """The importer keeps one row for these, so the engine must grant once. + + Accents and punctuation are stripped, which is what lets a re-import with + tidied-up names match the review already on record. + """ + accented = _review(submitter_raw="Joaquín M López Muñoz") + plain = _review(submitter_raw="Joaquin M Lopez Munoz") + + assert accented.dedup_key == plain.dedup_key + + +def test_a_second_review_on_different_dates_keeps_its_own_key(): + """Dates are in the fingerprint so a library reviewed twice counts twice.""" + first = _review(review_dates="April 1-10, 2017") + second = _review(review_dates="June 1-10, 2021") + + assert first.dedup_key != second.dedup_key + + +def test_the_separator_cannot_appear_inside_a_component(): + """Which is what makes joining on a pipe unambiguous rather than lossy. + + Normalisation drops every non-alphanumeric character, so no field can smuggle + a separator in and make two different reviews produce one key. + """ + review = _review(submission="a|b", submitter_raw="c|d", review_dates="e|f") + + assert review.dedup_key == "ab|cd|ef" + assert normalize("a|b|c") == "abc" From 53cd8bbc1ab596c28010a91fa07f77217e42c840 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Thu, 20 Aug 2026 16:30:06 -0300 Subject: [PATCH 18/20] fix: keep reviewer badges through a clean review re-import --- .../management/commands/import_reviews.py | 33 +++++++++++++--- versions/tests/test_commands.py | 38 +++++++++++++++++++ 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/versions/management/commands/import_reviews.py b/versions/management/commands/import_reviews.py index 1000a782d..14fc37fcd 100644 --- a/versions/management/commands/import_reviews.py +++ b/versions/management/commands/import_reviews.py @@ -8,7 +8,10 @@ from django.core.management.base import CommandError from django.db import transaction -from badges.services import discard_source_achievements +from badges.services import ( + discard_source_achievements, + relink_source_achievements, +) from libraries.models import CommitAuthor from versions.models import Review, ReviewResult from versions.review_keys import review_key @@ -81,12 +84,12 @@ def command(clean): reviews_created = results_created = 0 # Import everything in a transaction with transaction.atomic(): + doomed_ids = [] if clean: - # Parse before touching stored data, then make the grant discard, - # review deletion, and replacement one atomic operation. - discard_source_achievements( - Review, Review.objects.values_list("pk", flat=True) - ) + # Parse before touching stored data, then make the deletion and its + # replacement one atomic operation. The grants are settled after the + # replacement rows exist, not before: see the end of this block. + doomed_ids = list(Review.objects.values_list("pk", flat=True)) delete_output = Review.objects.all().delete() click.secho(f"Deleted {delete_output}\n", fg="yellow") @@ -104,6 +107,11 @@ def command(clean): review.submission, review.submitter_raw, review.review_dates ) if key in existing_by_key: + # The survivor is the same review, so a grant naming it by + # fingerprint follows the survivor instead of being thrown away. + relink_source_achievements( + Review, {"|".join(key): existing_by_key[key].pk} + ) discard_source_achievements(Review, [review.pk]) review.delete() removed_duplicates += 1 @@ -140,6 +148,19 @@ def command(clean): ) results_created += int(created) + if clean: + # The same reviews are back under new ids, and a grant names its + # evidence by fingerprint, so the pointers are rebuilt rather than + # the grants dropped: dropping them would revoke the Reviewer badges + # they justify and re-earn them dated today on the next sync. + relink_source_achievements( + Review, + {"|".join(key): review.pk for key, review in existing_by_key.items()}, + ) + # Whatever still points into the deleted ids is evidence that did not + # come back, so those grants really are stale. + discard_source_achievements(Review, doomed_ids) + click.secho("\nFinished importing reviews", fg="green") click.secho( f"Created {reviews_created} reviews and {results_created} results", fg="green" diff --git a/versions/tests/test_commands.py b/versions/tests/test_commands.py index a5230217d..55d4f00c2 100644 --- a/versions/tests/test_commands.py +++ b/versions/tests/test_commands.py @@ -555,3 +555,41 @@ def test_import_reviews_fails_when_the_heading_is_missing(capsys): call_command("import_reviews") assert "Could not find review result tables under" in capsys.readouterr().err + + +@pytest.mark.django_db +def test_import_reviews_clean_keeps_the_badges_it_would_have_revoked( + review_results_page, catalogue +): + """A clean re-import replaces the rows; it does not un-earn the badge. + + The fingerprint survives the swap, so the grant is re-pointed at the new row. + Discarding it instead would cascade-revoke the Reviewer badge, record that + revocation permanently, and re-earn it dated today on the next sync. + """ + from badges.models import Achievement, UserAchievement, UserBadge + from badges.services import recalculate_badges + from badges.tests.fixtures import grant_from_source + + user = baker.make("users.User") + submitter = baker.make( + "libraries.CommitAuthor", user=user, name="Christian Mazakas" + ) + achievement = Achievement.objects.get(slug="library-review") + call_command("import_reviews") + review = Review.objects.get(submission="boost::container::hub") + review.submitters.add(submitter) + grant, _ = grant_from_source(user, achievement, review, review.dedup_key) + recalculate_badges(user.pk, achievement.pk) + badges = set(UserBadge.objects.values_list("pk", "awarded_at", "revoked_at")) + assert badges, "nothing was awarded, so the assertion below proves nothing" + + call_command("import_reviews", "--clean") + + replacement = Review.objects.get(submission="boost::container::hub") + assert replacement.pk != review.pk + survivor = UserAchievement.objects.get(pk=grant.pk) + assert survivor.source_object_id == replacement.pk + assert ( + set(UserBadge.objects.values_list("pk", "awarded_at", "revoked_at")) == badges + ) From 65843572a785fe57e118d76820bc62aeb9ed4c6f Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Fri, 21 Aug 2026 11:06:55 -0300 Subject: [PATCH 19/20] fix: chain the docs-count migration onto the merged libraries leaf --- ..._docs_files_changed.py => 0045_commit_docs_files_changed.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename libraries/migrations/{0044_commit_docs_files_changed.py => 0045_commit_docs_files_changed.py} (83%) diff --git a/libraries/migrations/0044_commit_docs_files_changed.py b/libraries/migrations/0045_commit_docs_files_changed.py similarity index 83% rename from libraries/migrations/0044_commit_docs_files_changed.py rename to libraries/migrations/0045_commit_docs_files_changed.py index 17031008a..c68881983 100644 --- a/libraries/migrations/0044_commit_docs_files_changed.py +++ b/libraries/migrations/0045_commit_docs_files_changed.py @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("libraries", "0043_libraryversion_website_adoc_and_more"), + ("libraries", "0044_merge_20260814_1905"), ] operations = [ From 82dd126b85dc8ae7f4190c3f22cd802cfa004cd6 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Fri, 21 Aug 2026 13:29:19 -0300 Subject: [PATCH 20/20] fix: chain the docs-count migration after the claim unbind --- ..._docs_files_changed.py => 0046_commit_docs_files_changed.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename libraries/migrations/{0045_commit_docs_files_changed.py => 0046_commit_docs_files_changed.py} (83%) diff --git a/libraries/migrations/0045_commit_docs_files_changed.py b/libraries/migrations/0046_commit_docs_files_changed.py similarity index 83% rename from libraries/migrations/0045_commit_docs_files_changed.py rename to libraries/migrations/0046_commit_docs_files_changed.py index c68881983..2816f8f1f 100644 --- a/libraries/migrations/0045_commit_docs_files_changed.py +++ b/libraries/migrations/0046_commit_docs_files_changed.py @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("libraries", "0044_merge_20260814_1905"), + ("libraries", "0045_unbind_unverified_commit_author_claims"), ] operations = [