From da634c2279e578fb6b9738f9209df3411f546314 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Fri, 31 Jul 2026 19:05:37 -0300 Subject: [PATCH 1/4] refactor: rewrite the boost.org review results import --- versions/admin.py | 29 +- .../management/commands/import_reviews.py | 320 ++++++++++---- versions/tasks.py | 6 + .../tests/files/review-results-sample.html | 61 +++ versions/tests/test_admin.py | 62 +++ versions/tests/test_commands.py | 415 +++++++++++++++++- versions/tests/test_tasks.py | 10 + 7 files changed, 812 insertions(+), 91 deletions(-) create mode 100644 versions/tests/files/review-results-sample.html create mode 100644 versions/tests/test_admin.py diff --git a/versions/admin.py b/versions/admin.py index cb80a340e..abcb32142 100755 --- a/versions/admin.py +++ b/versions/admin.py @@ -135,15 +135,36 @@ class ResultInline(admin.StackedInline): @admin.register(models.Review) class ReviewAdmin(admin.ModelAdmin): - list_display = ["submission", "review_dates", "get_results"] - search_fields = ["submission"] + list_display = [ + "id", + "submission", + "review_dates", + "get_results", + "get_review_manager", + "get_scraped_review_manager", + ] + ordering = ["-id"] + search_fields = ["submission", "review_manager_raw", "review_manager__name"] inlines = [ResultInline] def get_results(self, obj): - return " | ".join(obj.results.values_list("short_description", flat=True)) + return " | ".join(result.short_description for result in obj.results.all()) + + @admin.display(description="Review manager", ordering="review_manager__name") + def get_review_manager(self, obj): + return obj.review_manager or "" + + @admin.display(description="Scraped review manager", ordering="review_manager_raw") + def get_scraped_review_manager(self, obj): + return obj.review_manager_raw def get_queryset(self, request: HttpRequest) -> QuerySet: - return super().get_queryset(request).prefetch_related("results") + return ( + super() + .get_queryset(request) + .select_related("review_manager") + .prefetch_related("results") + ) @admin.register(models.ReviewResult) diff --git a/versions/management/commands/import_reviews.py b/versions/management/commands/import_reviews.py index 8798dad9f..bbea5f470 100644 --- a/versions/management/commands/import_reviews.py +++ b/versions/management/commands/import_reviews.py @@ -1,14 +1,28 @@ +import html +import re +import unicodedata +from urllib.parse import urlparse + from bs4 import BeautifulSoup import djclick as click import requests -from django.contrib.auth import get_user_model +from django.core.management.base import CommandError from django.db import transaction +from badges.services import discard_source_achievements from libraries.models import CommitAuthor from versions.models import Review, ReviewResult -User = get_user_model() +PAST_RESULTS_HEADING = "Past Review Results and Milestones" +PAST_RESULTS_HEADER = ( + "Submission", + "Submitter", + "Review Manager", + "Review/Release Dates", + "Result", +) +REQUEST_TIMEOUT_SECONDS = 30 @click.command() @@ -17,44 +31,108 @@ ) def command(clean): """Import Boost library reviews from boost.org table data""" - if clean: - delete_output = Review.objects.all().delete() - - click.secho(f"Deleted {delete_output}\n", fg="yellow") - click.secho("Starting review import from boost.org\n", fg="green") - url = "https://www.boost.org/community/review_schedule.html" - response = requests.get(url) - - soup = BeautifulSoup(response.text, "html.parser") + url = "https://www.boost.org/doc/formal-reviews/review-results.html" + # Timed out because this now runs from a Celery task as well as by hand, and a + # boost.org that accepts the connection and never answers would otherwise hold + # a worker for as long as the process lives. + response = requests.get(url, timeout=REQUEST_TIMEOUT_SECONDS) + # boost.org serves the review-results page with `Content-Type: text/html` + # and no charset, so `requests` falls back to ISO-8859-1 per the HTTP spec + # and the actual UTF-8 body gets mojibake-decoded (e.g. "Joaquín M López + # Muñoz" turns into "Joaquín M López Muñoz"). Force UTF-8 so accented + # names round-trip cleanly into the raw fields and FK lookup. + response.encoding = "utf-8" + + # Raise rather than return on an unparseable page: this command runs from a + # Celery task, and a quiet return would report success on an empty scrape. + inner = _extract_inner_doc(response.text) + if inner is None: + raise CommandError(f"Could not find review content in {url}") + + # Locate every yearly results table in the section. The live page starts the + # section with a navigation table, so position alone cannot distinguish the + # result data from the surrounding page furniture. + heading = inner.find( + lambda t: t.name in ("h1", "h2", "h3", "h4") + and t.get_text(strip=True) == PAST_RESULTS_HEADING + ) + result_tables = _result_tables_under(heading) if heading else [] + if not result_tables: + raise CommandError( + f'Could not find review result tables under "{PAST_RESULTS_HEADING}" ' + f"in {url}" + ) - # Parse both tables - scheduled_review_table = soup.find("table", summary="Formal Review Schedule") - past_review_table = soup.find("table", summary="Review Results") + past_reviews = [ + review + for result_table in result_tables + for review in _parse_table(result_table) + ] - if not scheduled_review_table or not past_review_table: - click.secho("Could not find review tables in page content", fg="red", err=True) - return + click.echo(f"Found {len(past_reviews)} past reviews") - upcoming_reviews = _parse_table(scheduled_review_table) - past_reviews = _parse_table(past_review_table, past_results=True) - - click.echo( - f"Found {len(upcoming_reviews)} upcoming and {len(past_reviews)} past reviews" - ) + # The page lists newest first. Create oldest-first so the newest review gets + # the highest id; combined with the `-id` ordering on the admin and the + # public past-reviews page, that keeps the newest reviews at the top, exactly + # as they appear on the website. + past_reviews.reverse() reviews_created = results_created = 0 # Import everything in a transaction with transaction.atomic(): - # Create or update past reviews + 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) + ) + delete_output = Review.objects.all().delete() + click.secho(f"Deleted {delete_output}\n", fg="yellow") + + # Build a fingerprint -> Review map of what already exists so re-imports + # update rows in place instead of creating near-duplicates. The + # fingerprint is flexible (case/accents/punctuation are normalized away) + # and spans multiple fields (submission + submitter + dates), so a row + # whose spelling shifted slightly between imports still matches, while + # genuinely distinct reviews - e.g. a library re-reviewed on different + # dates - stay separate. Any pre-existing duplicates are collapsed. + existing_by_key = {} + removed_duplicates = 0 + for review in list(Review.objects.order_by("pk")): + key = _review_key( + review.submission, review.submitter_raw, review.review_dates + ) + if key in existing_by_key: + discard_source_achievements(Review, [review.pk]) + review.delete() + removed_duplicates += 1 + else: + existing_by_key[key] = review + + if removed_duplicates: + click.secho( + f"Removed {removed_duplicates} pre-existing duplicate reviews", + fg="yellow", + ) + for review_data, results in past_reviews: - review, created = Review.objects.update_or_create( - submission=review_data["submission"], - submitter_raw=review_data["submitter_raw"], - defaults=review_data, + key = _review_key( + review_data["submission"], + review_data["submitter_raw"], + review_data["review_dates"], ) - reviews_created += int(created) + review = existing_by_key.get(key) + if review is None: + review = Review.objects.create(**review_data) + existing_by_key[key] = review + reviews_created += 1 + else: + for field, value in review_data.items(): + setattr(review, field, value) + review.save() + for result in results: _, created = ReviewResult.objects.update_or_create( review=review, @@ -63,15 +141,6 @@ def command(clean): ) results_created += int(created) - # Create or update upcoming reviews - for review_data, _ in upcoming_reviews: - _, created = Review.objects.update_or_create( - submission=review_data["submission"], - submitter_raw=review_data["submitter_raw"], - defaults=review_data, - ) - reviews_created += int(created) - click.secho("\nFinished importing reviews", fg="green") click.secho( f"Created {reviews_created} reviews and {results_created} results", fg="green" @@ -88,7 +157,7 @@ def command(clean): submitter_names = _parse_raw_names(review.submitter_raw) for name in submitter_names: submitter = CommitAuthor.objects.filter(name=name).first() - if submitter: + if submitter and not review.submitters.filter(pk=submitter.pk).exists(): review.submitters.add(submitter) users_linked += 1 click.echo(f"Linked submitter {submitter} to {review.submission}") @@ -103,9 +172,9 @@ def command(clean): if manager_names: name = manager_names[0] manager = CommitAuthor.objects.filter(name=name).first() - if manager: + if manager and review.review_manager_id != manager.pk: review.review_manager = manager - review.save() + review.save(update_fields=["review_manager"]) managers_linked += 1 click.echo(f"Linked manager {manager} to {review.submission}") @@ -117,69 +186,148 @@ def command(clean): click.secho("\nDone!", fg="green") -def _parse_table(table, past_results=False): - """Parse a review table and return review data""" +def _extract_inner_doc(page_html): + """Return the parsed inner document embedded in the page's content iframe. + + The review-results page is an Antora/AsciiDoc doc whose real content is + HTML-escaped inside an `` + + diff --git a/versions/tests/test_admin.py b/versions/tests/test_admin.py new file mode 100644 index 000000000..ac849bacd --- /dev/null +++ b/versions/tests/test_admin.py @@ -0,0 +1,62 @@ +import pytest +from django.contrib.admin.sites import AdminSite +from django.test import RequestFactory +from django.urls import reverse +from model_bakery import baker + +from versions.admin import ReviewAdmin +from versions.models import Review + + +@pytest.mark.django_db +def test_review_admin_changelist_shows_review_manager_columns(client, super_user): + client.force_login(super_user) + manager = baker.make("libraries.CommitAuthor", name="Marshall Clow") + baker.make( + "versions.Review", + submission="Boost.Linked", + review_manager=manager, + review_manager_raw="Marshall Clow", + ) + baker.make( + "versions.Review", + submission="Boost.Unlinked", + review_manager=None, + review_manager_raw="Someone Unlinked", + ) + + response = client.get(reverse("admin:versions_review_changelist")) + + assert response.status_code == 200 + content = response.content.decode() + # Resolved FK is rendered via CommitAuthor.__str__ (name). + assert "Marshall Clow" in content + # Raw column is always shown, even when the FK is unlinked. + assert "Someone Unlinked" in content + # Raw column is labelled as "Scraped review manager" to disambiguate it + # from the resolved FK column. + assert "Scraped review manager" in content + + +@pytest.mark.django_db +def test_review_admin_results_use_prefetched_objects( + super_user, django_assert_num_queries +): + review = baker.make("versions.Review") + baker.make( + "versions.ReviewResult", + review=review, + short_description="Accepted", + ) + baker.make( + "versions.ReviewResult", + review=review, + short_description="Released", + ) + request = RequestFactory().get("/") + request.user = super_user + model_admin = ReviewAdmin(Review, AdminSite()) + prefetched = model_admin.get_queryset(request).get(pk=review.pk) + + with django_assert_num_queries(0): + assert model_admin.get_results(prefetched) == "Accepted | Released" diff --git a/versions/tests/test_commands.py b/versions/tests/test_commands.py index 3dbf38374..a5230217d 100644 --- a/versions/tests/test_commands.py +++ b/versions/tests/test_commands.py @@ -1,10 +1,16 @@ -from unittest.mock import patch +import html +from pathlib import Path +from unittest.mock import Mock, patch import pytest +from click.exceptions import Exit from django.core.management import call_command from model_bakery import baker from core.models import RenderedContent +from versions.models import Review, ReviewResult + +REVIEW_RESULTS_FIXTURE = Path(__file__).parent / "files" / "review-results-sample.html" @pytest.fixture @@ -142,3 +148,410 @@ def test_generate_whats_new_requires_an_action(): with pytest.raises(Exception): # djclick raises UsageError; pytest treats it as failure. call_command("generate_whats_new") + + +@pytest.fixture +def review_results_page(): + """Mock requests.get to serve the review-results fixture page.""" + html = REVIEW_RESULTS_FIXTURE.read_text() + with patch("versions.management.commands.import_reviews.requests.get") as mock_get: + mock_get.return_value = Mock(text=html) + yield mock_get + + +@pytest.mark.django_db +def test_import_reviews_imports_past_table_only(review_results_page): + """Every yearly result table is imported; navigation and schedule are skipped.""" + call_command("import_reviews") + + submissions = set(Review.objects.values_list("submission", flat=True)) + assert submissions == { + "Boost 1.91.0 Released", + "boost::container::hub", + "Parser", + } + # The Current Schedule row must not be imported. + assert not Review.objects.filter(submission="Upcoming Library").exists() + # The section navigation table must not be imported either. + assert not Review.objects.filter(submission="2026").exists() + + +@pytest.mark.django_db +def test_import_reviews_creates_milestone_with_notes(review_results_page): + call_command("import_reviews") + + milestone = Review.objects.get(submission="Boost 1.91.0 Released") + assert milestone.submitter_raw == "-" + assert milestone.review_manager_raw == "Marshall Clow" + assert milestone.review_dates == "April 22, 2026" + assert milestone.github_link == "" + + result = milestone.results.get() + assert result.short_description == "Notes" + assert result.is_most_recent is True + assert ( + result.announcement_link + == "https://www.boost.org/users/history/version_1_91_0.html" + ) + + +@pytest.mark.django_db +def test_import_reviews_captures_github_link(review_results_page): + call_command("import_reviews") + + review = Review.objects.get(submission="boost::container::hub") + assert review.github_link == "https://github.com/joaquintides/hub" + result = review.results.get() + assert result.short_description == "Accepted" + assert result.is_most_recent is True + + +@pytest.mark.django_db +def test_import_reviews_ignores_a_non_github_submission_link(review_results_page): + """A submission linking somewhere other than GitHub stores no link at all.""" + call_command("import_reviews") + + review = Review.objects.get(submission="Parser") + assert review.github_link == "" + assert review.documentation_link == "" + + +@pytest.mark.django_db +def test_import_reviews_never_touches_a_curated_documentation_link(review_results_page): + """The field belongs to whoever filled it in, source link or not. + + Asserted on the one row whose submission cell *does* carry a link, because + that is the case a scraper writing the field would overwrite. + """ + existing = baker.make( + Review, + submission="Parser", + submitter_raw="Zach Laine", + review_dates="February 19, 2024 - February 28, 2024", + documentation_link="https://example.com/curated-parser-docs", + ) + + call_command("import_reviews") + + existing.refresh_from_db() + assert existing.documentation_link == "https://example.com/curated-parser-docs" + + +@pytest.mark.django_db +def test_import_reviews_preserves_links_when_source_has_none(review_results_page): + existing = baker.make( + Review, + submission="Boost 1.91.0 Released", + submitter_raw="-", + review_dates="April 22, 2026", + github_link="https://github.com/boostorg/boost", + documentation_link="https://example.com/curated-release-notes", + ) + + call_command("import_reviews") + + existing.refresh_from_db() + assert existing.github_link == "https://github.com/boostorg/boost" + assert existing.documentation_link == "https://example.com/curated-release-notes" + + +@pytest.mark.django_db +def test_import_reviews_marks_superseded_result_not_recent(review_results_page): + """A line-through result is superseded; the bare anchor is current.""" + call_command("import_reviews") + + review = Review.objects.get(submission="Parser") + superseded = review.results.get(short_description="Pending") + current = review.results.get(short_description="Conditionally Accepted") + + assert superseded.is_most_recent is False + assert superseded.announcement_link == "https://lists.boost.org/pending" + assert current.is_most_recent is True + assert current.announcement_link == "https://lists.boost.org/conditional" + + +@pytest.mark.django_db +def test_import_reviews_links_submitters_and_managers(review_results_page): + submitter = baker.make("libraries.CommitAuthor", name="Joaquin M Lopez Munoz") + manager = baker.make("libraries.CommitAuthor", name="Marshall Clow") + + call_command("import_reviews") + + review = Review.objects.get(submission="boost::container::hub") + assert list(review.submitters.all()) == [submitter] + + milestone = Review.objects.get(submission="Boost 1.91.0 Released") + assert milestone.review_manager == manager + + +@pytest.mark.django_db +def test_import_reviews_reports_only_new_user_links(review_results_page, capsys): + baker.make("libraries.CommitAuthor", name="Joaquin M Lopez Munoz") + baker.make("libraries.CommitAuthor", name="Marshall Clow") + + call_command("import_reviews") + first_output = capsys.readouterr().out + call_command("import_reviews") + second_output = capsys.readouterr().out + + assert "Linked 1 submitters and 2 managers" in first_output + assert "Linked 0 submitters and 0 managers" in second_output + + +@pytest.mark.django_db +def test_import_reviews_clean_deletes_existing(review_results_page): + stale = baker.make(Review, submission="Stale", submitter_raw="Someone") + baker.make(ReviewResult, review=stale, short_description="Old") + + call_command("import_reviews", "--clean") + + assert not Review.objects.filter(submission="Stale").exists() + assert Review.objects.count() == 3 + + +@pytest.mark.django_db +def test_import_reviews_clean_rolls_back_delete_when_replacement_fails( + review_results_page, + catalogue, +): + from badges.models import Achievement, UserAchievement + from badges.tests.fixtures import grant_from_source + + stale = baker.make(Review, submission="Stale", submitter_raw="Someone") + user = baker.make("users.User") + grant, _ = grant_from_source( + user, Achievement.objects.get(slug="library-review"), stale + ) + + with patch.object( + Review.objects, "create", side_effect=RuntimeError("write failed") + ): + with pytest.raises(RuntimeError, match="write failed"): + call_command("import_reviews", "--clean") + + assert Review.objects.filter(pk=stale.pk).exists() + assert UserAchievement.objects.filter(pk=grant.pk).exists() + + +@pytest.mark.django_db +def test_import_reviews_is_idempotent(review_results_page): + """Running the import twice does not create duplicate reviews.""" + call_command("import_reviews") + call_command("import_reviews") + + assert Review.objects.count() == 3 + # The Parser row still has exactly its two results, not four. + parser = Review.objects.get(submission="Parser") + assert parser.results.count() == 2 + + +@pytest.mark.django_db +def test_import_reviews_matches_existing_despite_spelling(review_results_page): + """A near-duplicate (accents/punctuation) is updated, not duplicated.""" + existing = baker.make( + Review, + submission="boost::container::hub", + # Accented spelling that differs from the ASCII fixture row. + submitter_raw="Joaquín M López Muñoz", + review_dates="April 16, 2026 - April 26, 2026", + ) + + call_command("import_reviews") + + assert Review.objects.count() == 3 + existing.refresh_from_db() + # The same record was updated to the page's spelling. + assert existing.submitter_raw == "Joaquin M Lopez Munoz" + assert Review.objects.filter(submission="boost::container::hub").count() == 1 + + +@pytest.mark.django_db +def test_import_reviews_collapses_preexisting_duplicates(review_results_page): + """Pre-existing rows with the same fingerprint are collapsed to one.""" + for _ in range(2): + baker.make( + Review, + submission="Parser", + submitter_raw="Zach Laine", + review_dates="February 19, 2024 - February 28, 2024", + ) + + call_command("import_reviews") + + assert Review.objects.filter(submission="Parser").count() == 1 + assert Review.objects.count() == 3 + + +@pytest.mark.django_db +def test_import_reviews_dedupes_across_special_characters(review_results_page): + """A DB row differing only by special characters/case dedupes against the page. + + Mirrors the real ``Johan Råde`` vs mojibake ``Johan RÃ¥de`` case: the special + characters are stripped before comparison, so the existing row is matched + and updated instead of duplicated. + """ + existing = baker.make( + Review, + submission="BOOST::CONTAINER::HUB!!", + submitter_raw="joaquin, m. lopez-muñoz", + review_dates="April 16, 2026 - April 26, 2026", + ) + + call_command("import_reviews") + + assert Review.objects.filter(submission__icontains="hub").count() == 1 + existing.refresh_from_db() + assert existing.submission == "boost::container::hub" + assert existing.submitter_raw == "Joaquin M Lopez Munoz" + + +@pytest.mark.django_db +def test_import_reviews_orders_newest_first_by_id(review_results_page): + """The newest row on the page gets the highest id (newest-first under -id).""" + call_command("import_reviews") + + newest = Review.objects.get(submission="Boost 1.91.0 Released") # first on page + oldest = Review.objects.get(submission="Parser") # last on page + assert newest.id > oldest.id + + +class _CharsetlessResponse: + """Minimal stand-in for the real boost.org response. + + The live server sends ``Content-Type: text/html`` with no charset, which + makes ``requests`` fall back to ISO-8859-1 per RFC 2616 even though the + body is actually UTF-8. Reading ``.text`` against that default mojibakes + every accented character. + """ + + def __init__(self, content_bytes: bytes): + self.content = content_bytes + self.encoding = "ISO-8859-1" + + @property + def text(self) -> str: + return self.content.decode(self.encoding) + + +@pytest.mark.django_db +def test_import_reviews_decodes_utf8_when_server_omits_charset(): + """Accented names survive an ISO-8859-1-defaulting response intact.""" + inner_html = ( + "

Past Review Results and Milestones

" + "" + " " + "" + " " + "" + "
SubmissionSubmitterReview ManagerReview/Release DatesResult
Foo-Joaquín M López MuñozApril 22, 2026Released
" + ) + outer_html = ( + f'" + ) + response = _CharsetlessResponse(outer_html.encode("utf-8")) + existing = baker.make( + Review, + submission="Foo", + submitter_raw="-", + review_manager_raw="Joaquín M López Muñoz", + review_dates="April 22, 2026", + ) + stale_result = baker.make( + ReviewResult, + review=existing, + short_description="Released", + is_most_recent=False, + announcement_link="https://example.com/stale", + ) + + with patch("versions.management.commands.import_reviews.requests.get") as mock_get: + mock_get.return_value = response + call_command("import_reviews") + + review = Review.objects.get(submission="Foo") + assert review.review_manager_raw == "Joaquín M López Muñoz" + # Mojibake artefacts must not survive into the stored data. + assert "Ã" not in review.review_manager_raw + assert "Â" not in review.review_manager_raw + stale_result.refresh_from_db() + assert stale_result.is_most_recent is True + assert stale_result.announcement_link == "" + + +@pytest.mark.django_db +def test_import_reviews_discards_grants_for_collapsed_duplicates( + review_results_page, catalogue +): + """Deleting a duplicate review must not leave an achievement counting it. + + ``UserAchievement`` points at its source through a generic FK, so nothing in + the database stops a grant from outliving the review that justified it. + """ + from badges.models import Achievement, UserAchievement, UserBadge + from badges.tests.fixtures import grant_from_source + + reviews = [ + baker.make( + Review, + submission="Parser", + submitter_raw="Zach Laine", + review_dates="February 19, 2024 - February 28, 2024", + ) + for _ in range(2) + ] + user = baker.make("users.User") + achievement = Achievement.objects.get(slug="library-review") + for review in reviews: + grant_from_source(user, achievement, review) + assert UserAchievement.objects.count() == 2 + assert UserBadge.objects.filter(user=user, revoked_at__isnull=True).count() == 2 + + call_command("import_reviews") + + assert Review.objects.filter(submission="Parser").count() == 1 + assert UserAchievement.objects.count() == 1 + # Reviewer tiers are 1/2/3/4/5, so dropping to one valid grant revokes silver. + assert UserBadge.objects.filter(user=user, revoked_at__isnull=True).count() == 1 + + +@pytest.mark.django_db +def test_import_reviews_clean_discards_grants(review_results_page, catalogue): + """--clean wipes every review, so it must wipe their grants too.""" + from badges.models import Achievement, UserAchievement + from badges.tests.fixtures import grant_from_source + + review = baker.make(Review, submission="Old", submitter_raw="Someone") + user = baker.make("users.User") + grant_from_source(user, Achievement.objects.get(slug="library-review"), review) + + call_command("import_reviews", "--clean") + + assert not UserAchievement.objects.filter(source_object_id=review.pk).exists() + + +@pytest.mark.django_db +def test_import_reviews_fails_when_the_page_has_no_iframe(capsys): + """A silent return would let the Celery task report a successful no-op.""" + existing = baker.make(Review, submission="Existing", submitter_raw="Someone") + with patch("versions.management.commands.import_reviews.requests.get") as mock_get: + mock_get.return_value = Mock(text="redesigned") + with pytest.raises(Exit): + call_command("import_reviews", "--clean") + + assert "Could not find review content" in capsys.readouterr().err + assert Review.objects.filter(pk=existing.pk).exists() + + +@pytest.mark.django_db +def test_import_reviews_fails_when_the_heading_is_missing(capsys): + """The table is located by heading, so a renamed heading must not pass.""" + page = REVIEW_RESULTS_FIXTURE.read_text().replace( + "Past Review Results and Milestones", "Archive" + ) + with patch("versions.management.commands.import_reviews.requests.get") as mock_get: + mock_get.return_value = Mock(text=page) + with pytest.raises(Exit): + call_command("import_reviews") + + assert "Could not find review result tables under" in capsys.readouterr().err diff --git a/versions/tests/test_tasks.py b/versions/tests/test_tasks.py index 0d04e0d77..737dbbb1f 100644 --- a/versions/tests/test_tasks.py +++ b/versions/tests/test_tasks.py @@ -100,3 +100,13 @@ def test_import_version_race_condition(tag_mock: MagicMock, *args): assert rm.latest_version is not None # Ensure that that latest version is not our previously created version assert rm.latest_version != v + + +@patch("versions.tasks.call_command") +def test_import_reviews_task_runs_the_import_command(mock_call): + """The task exists so the admin can start the scrape off-request.""" + from versions.tasks import import_reviews_task + + import_reviews_task() + + assert [c.args for c in mock_call.call_args_list] == [("import_reviews",)] From 7ff9d70233d8de82a03dc731cb2f1179430df494 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Mon, 3 Aug 2026 17:17:21 -0300 Subject: [PATCH 2/4] feat: add an admin button that imports review results --- versions/admin.py | 61 +++++++++++++++++++++++++++++- versions/tests/test_admin.py | 73 ++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 2 deletions(-) diff --git a/versions/admin.py b/versions/admin.py index abcb32142..63ba0c861 100755 --- a/versions/admin.py +++ b/versions/admin.py @@ -5,11 +5,67 @@ from django.urls import path from django.utils.html import format_html, format_html_join +from core.admin_buttons import TaskButton, TaskButtonAdminMixin from libraries.tasks import import_new_versions_tasks from . import models from .models import Version -from .tasks import dispatch_whats_new +from .tasks import dispatch_whats_new, import_reviews_task + + +def _import_reviews_preview(_request, _value): + """Static warning for the destructive parts of review reconciliation.""" + return { + "title": "Import reviews from boost.org", + "summary": ( + "This re-scrapes the published formal-review results and updates the " + "stored reviews to match. Existing reviews with the same normalized " + "identity are updated instead of copied." + ), + "rows": ( + { + "label": "Duplicate reviews", + "detail": ( + "Duplicate stored reviews are merged; the duplicate rows, " + "their results, and achievements sourced from them are deleted." + ), + "warning": True, + }, + { + "label": "Review results", + "detail": ( + "Published results are created or updated for every imported " + "review." + ), + "warning": False, + }, + ), + "warning": ( + "Continue only if boost.org is the source you intend to reconcile " + "against." + ), + "can_apply": True, + } + + +IMPORT_REVIEWS_BUTTON = TaskButton( + name="import_reviews", + label="Import Reviews from boost.org", + task=import_reviews_task, + success_message="Reviews are being imported from boost.org in the background.", + busy_message=( + "A review import is already queued or running; not starting another one." + ), + permission="versions.delete_review", + confirm=_import_reviews_preview, + description=( + "Re-scrapes the formal-review results published on boost.org and updates " + "the reviews and results below, matching each against what is already " + "stored rather than adding a near-duplicate. Duplicate reviews, their " + "results, and achievements sourced from them may be deleted; Reviewer " + "achievements are brought into step afterwards." + ), +) class VersionFileInline(admin.StackedInline): @@ -134,7 +190,7 @@ class ResultInline(admin.StackedInline): @admin.register(models.Review) -class ReviewAdmin(admin.ModelAdmin): +class ReviewAdmin(TaskButtonAdminMixin, admin.ModelAdmin): list_display = [ "id", "submission", @@ -146,6 +202,7 @@ class ReviewAdmin(admin.ModelAdmin): ordering = ["-id"] search_fields = ["submission", "review_manager_raw", "review_manager__name"] inlines = [ResultInline] + task_buttons = (IMPORT_REVIEWS_BUTTON,) def get_results(self, obj): return " | ".join(result.short_description for result in obj.results.all()) diff --git a/versions/tests/test_admin.py b/versions/tests/test_admin.py index ac849bacd..ec012b17f 100644 --- a/versions/tests/test_admin.py +++ b/versions/tests/test_admin.py @@ -1,5 +1,9 @@ +from unittest.mock import patch + import pytest +from django.contrib.auth.models import Permission from django.contrib.admin.sites import AdminSite +from django.core.cache import cache from django.test import RequestFactory from django.urls import reverse from model_bakery import baker @@ -8,6 +12,75 @@ from versions.models import Review +@pytest.fixture(autouse=True) +def _clear_task_button_locks(): + """The import button debounces through the cache; isolate tests.""" + cache.clear() + + +@pytest.mark.django_db +def test_review_admin_import_button_enqueues_task(client, super_user): + client.force_login(super_user) + + with patch("versions.admin.import_reviews_task.delay") as mock_delay: + preview = client.post(reverse("admin:versions_review_import_reviews")) + response = client.post( + reverse("admin:versions_review_import_reviews"), {"apply": "1"} + ) + + assert preview.status_code == 200 + preview_body = preview.content.decode() + assert "Duplicate stored reviews are merged" in preview_body + assert "their results" in preview_body + mock_delay.assert_called_once_with() + # Redirects back to the Review changelist. + assert response.status_code == 302 + + +@pytest.mark.django_db +def test_review_admin_import_button_ignores_get(client, super_user): + """Importing rewrites every Review row, so a link prefetch must not start it.""" + client.force_login(super_user) + + with patch("versions.admin.import_reviews_task.delay") as mock_delay: + response = client.get(reverse("admin:versions_review_import_reviews")) + + mock_delay.assert_not_called() + assert response.status_code == 302 + + +@pytest.mark.django_db +def test_review_admin_changelist_shows_import_button(client, super_user): + client.force_login(super_user) + + response = client.get(reverse("admin:versions_review_changelist")) + + assert response.status_code == 200 + assert reverse("admin:versions_review_import_reviews").encode() in response.content + assert b"Duplicate reviews, their results" in response.content + + +@pytest.mark.django_db +def test_review_admin_import_requires_delete_permission(client): + staff = baker.make("users.User", email="review-staff@example.com", is_staff=True) + staff.user_permissions.add( + *Permission.objects.filter( + content_type__app_label="versions", + codename__in=("view_review", "change_review"), + ) + ) + client.force_login(staff) + + changelist = client.get(reverse("admin:versions_review_changelist")) + with patch("versions.admin.import_reviews_task.delay") as mock_delay: + response = client.post(reverse("admin:versions_review_import_reviews")) + + assert changelist.status_code == 200 + assert changelist.context["task_buttons"] == [] + assert response.status_code == 403 + mock_delay.assert_not_called() + + @pytest.mark.django_db def test_review_admin_changelist_shows_review_manager_columns(client, super_user): client.force_login(super_user) From 84c4ee84f9ebe65083ed8b104f5db2d09e3f1670 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Fri, 31 Jul 2026 19:20:35 -0300 Subject: [PATCH 3/4] feat: derive the library review achievement from review submitters --- badges/sources.py | 13 +++++++++++++ badges/tests/test_admin.py | 15 +++++++++++++++ badges/tests/test_commands.py | 12 ++++++++++++ badges/tests/test_sources.py | 11 +++++++++++ versions/tasks.py | 3 +++ versions/tests/test_tasks.py | 9 ++++++--- 6 files changed, 60 insertions(+), 3 deletions(-) diff --git a/badges/sources.py b/badges/sources.py index 18067d3d1..c2a1d1261 100644 --- a/badges/sources.py +++ b/badges/sources.py @@ -92,11 +92,24 @@ def _iter_code_commits(): yield commit.author.user, commit +def _iter_library_review(): + """Yield (user, review) for every review submission with a linked user.""" + from versions.models import Review + + for review in Review.objects.prefetch_related("submitters__user").iterator( + chunk_size=500 + ): + for commit_author in review.submitters.all(): + if commit_author.user_id: + yield commit_author.user, review + + BACKFILL_ITERATORS = { AchievementSlug.LIBRARY_AUTHORING: _iter_library_authoring, AchievementSlug.LIBRARY_MAINTENANCE: _iter_library_maintenance, AchievementSlug.LIBRARY_VERSIONING: _iter_library_versioning, AchievementSlug.CODE_COMMITS: _iter_code_commits, + AchievementSlug.LIBRARY_REVIEW: _iter_library_review, } # Derived, so the CLI choices can never drift from the wired iterators. diff --git a/badges/tests/test_admin.py b/badges/tests/test_admin.py index b872a90ff..98a17553c 100644 --- a/badges/tests/test_admin.py +++ b/badges/tests/test_admin.py @@ -1179,6 +1179,21 @@ def test_reconcile_preview_refuses_an_empty_source( assert UserAchievement.objects.filter(user=plain_user).count() == 1 +def test_reconcile_preview_blocks_an_incomplete_catalogue( + client, super_user, plain_user, commit_by_someone_else, stale_commit_grant +): + """An unseeded slug is what the command refuses outright, so say so first.""" + Achievement.objects.filter(slug="library-review").delete() + client.force_login(super_user) + + response = client.post(reverse(RECONCILE_URL)) + + body = response.content.decode() + assert "the catalogue is incomplete" in body + assert "Run migrations first" in body + assert 'name="apply"' not in body + + def test_reconcile_button_needs_more_than_the_change_permission( client, plain_user, commit_by_someone_else, stale_commit_grant ): diff --git a/badges/tests/test_commands.py b/badges/tests/test_commands.py index 016041591..296de077c 100644 --- a/badges/tests/test_commands.py +++ b/badges/tests/test_commands.py @@ -121,6 +121,18 @@ def test_backfill_library_versioning(plain_user): ) +def test_backfill_library_review(plain_user): + """Backfill grants the reviewer achievement from linked review submitters.""" + review = baker.make("versions.Review") + review.submitters.add(baker.make("libraries.CommitAuthor", user=plain_user)) + + call_command("backfill_achievements", "--source", "library-review") + + assert UserBadge.objects.filter( + user=plain_user, badge__achievement__slug="library-review" + ).exists() + + def test_backfill_fails_loudly_on_an_explicit_unseeded_source(plain_user): """A named source with no Achievement row is a deploy bug, not a skip.""" Achievement.objects.filter(slug=AchievementSlug.CODE_COMMITS).delete() diff --git a/badges/tests/test_sources.py b/badges/tests/test_sources.py index ef46a81d0..487acef08 100644 --- a/badges/tests/test_sources.py +++ b/badges/tests/test_sources.py @@ -104,3 +104,14 @@ def test_iter_code_commits_skips_unlinked(plain_user): pairs = list(sources._iter_code_commits()) assert [u for u, _ in pairs] == [plain_user] + + +def test_iter_library_review_skips_unlinked(plain_user): + """Review submitters without a linked user are skipped.""" + review = baker.make("versions.Review") + review.submitters.add( + baker.make("libraries.CommitAuthor", user=plain_user), + baker.make("libraries.CommitAuthor", user=None), + ) + pairs = list(sources._iter_library_review()) + assert [u for u, _ in pairs] == [plain_user] diff --git a/versions/tasks.py b/versions/tasks.py index e9f33f1e4..442b08dda 100644 --- a/versions/tasks.py +++ b/versions/tasks.py @@ -503,6 +503,9 @@ def import_library_versions(version_name, token=None, version_type="tag"): def import_reviews_task(): """Imports Boost formal-review results and milestones from boost.org.""" call_command("import_reviews") + # Reviews are the only source of the library-review achievement, and this is + # the only thing that writes them, so it owns keeping the grants in step. + call_command("backfill_achievements", "--source", "library-review") @app.task diff --git a/versions/tests/test_tasks.py b/versions/tests/test_tasks.py index 737dbbb1f..42c09a191 100644 --- a/versions/tests/test_tasks.py +++ b/versions/tests/test_tasks.py @@ -103,10 +103,13 @@ def test_import_version_race_condition(tag_mock: MagicMock, *args): @patch("versions.tasks.call_command") -def test_import_reviews_task_runs_the_import_command(mock_call): - """The task exists so the admin can start the scrape off-request.""" +def test_import_reviews_task_backfills_the_review_source(mock_call): + """Reviews are the only source of library-review, so this task owns it.""" from versions.tasks import import_reviews_task import_reviews_task() - assert [c.args for c in mock_call.call_args_list] == [("import_reviews",)] + assert [c.args for c in mock_call.call_args_list] == [ + ("import_reviews",), + ("backfill_achievements", "--source", "library-review"), + ] From 75f6e054afa41c1bfd735b77eb8676b9d5123f59 Mon Sep 17 00:00:00 2001 From: "Teodoro B. Mendes" Date: Tue, 4 Aug 2026 14:09:02 -0300 Subject: [PATCH 4/4] fix: name the admin behind a review-import backfill --- versions/admin.py | 1 + versions/tasks.py | 12 +++++++++--- versions/tests/test_admin.py | 2 +- versions/tests/test_tasks.py | 4 +++- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/versions/admin.py b/versions/admin.py index 63ba0c861..b30cddba0 100755 --- a/versions/admin.py +++ b/versions/admin.py @@ -57,6 +57,7 @@ def _import_reviews_preview(_request, _value): "A review import is already queued or running; not starting another one." ), permission="versions.delete_review", + pass_actor=True, confirm=_import_reviews_preview, description=( "Re-scrapes the formal-review results published on boost.org and updates " diff --git a/versions/tasks.py b/versions/tasks.py index 442b08dda..e887a9346 100644 --- a/versions/tasks.py +++ b/versions/tasks.py @@ -500,12 +500,18 @@ def import_library_versions(version_name, token=None, version_type="tag"): @app.task -def import_reviews_task(): - """Imports Boost formal-review results and milestones from boost.org.""" +def import_reviews_task(actor_id=None): + """Imports Boost formal-review results and milestones from boost.org. + + ``actor_id`` is the admin who started the import, which the sync log records so + a Reviewer badge that moves with a re-import can be traced back to it. + """ call_command("import_reviews") # Reviews are the only source of the library-review achievement, and this is # the only thing that writes them, so it owns keeping the grants in step. - call_command("backfill_achievements", "--source", "library-review") + call_command( + "backfill_achievements", "--source", "library-review", actor_id=actor_id + ) @app.task diff --git a/versions/tests/test_admin.py b/versions/tests/test_admin.py index ec012b17f..80e58185a 100644 --- a/versions/tests/test_admin.py +++ b/versions/tests/test_admin.py @@ -32,7 +32,7 @@ def test_review_admin_import_button_enqueues_task(client, super_user): preview_body = preview.content.decode() assert "Duplicate stored reviews are merged" in preview_body assert "their results" in preview_body - mock_delay.assert_called_once_with() + mock_delay.assert_called_once_with(actor_id=super_user.pk) # Redirects back to the Review changelist. assert response.status_code == 302 diff --git a/versions/tests/test_tasks.py b/versions/tests/test_tasks.py index 42c09a191..9d82d6854 100644 --- a/versions/tests/test_tasks.py +++ b/versions/tests/test_tasks.py @@ -107,9 +107,11 @@ def test_import_reviews_task_backfills_the_review_source(mock_call): """Reviews are the only source of library-review, so this task owns it.""" from versions.tasks import import_reviews_task - import_reviews_task() + import_reviews_task(actor_id=7) assert [c.args for c in mock_call.call_args_list] == [ ("import_reviews",), ("backfill_achievements", "--source", "library-review"), ] + # The admin who pressed the button, so the sync log can name them. + assert mock_call.call_args_list[-1].kwargs == {"actor_id": 7}