Story #2608 :: Task: Fix the formal-review importer and derive the Reviewer achievement from it

[herzog0]

Issue: #2608

⚠️ Base branch is teo/2541-source-library-versioning

Summary & Context

Everything about formal reviews, in one place: the boost.org results importer, the admin button
that runs it, and the Reviewer achievement derived from what it imports. They are one context -
versions.models.Review is written by exactly one thing, and that thing is this importer, so a
badge counting reviews and the command that produces them are not really separable.

• Figma link: n/a - admin and management command only
• Source page: https://www.boost.org/doc/formal-reviews/review-results.html
• Link to components/page: /admin/versions/review/

Changes

The importer, rewritten

import_reviews was scraping a page boost.org no longer serves, and creating near-duplicate rows
on every run. The URL changes:

Was	https://www.boost.org/community/review_schedule.html (gone - no usable tables)
Now	https://www.boost.org/doc/formal-reviews/review-results.html

The new page is generated from
boostorg/website-v2-docs/formal-reviews/modules/ROOT/pages/review-results.adoc, so its shape
follows that document rather than the old hand-written HTML. Open it before reviewing - almost
every change below follows from something visible on it.

The release-blocking part: the section now opens with a year-navigation table, and the old code
took the first table after the heading. On the live page that means the import would write about
two junk rows instead of the 286 real reviews, and report success.

• Parse every result table, not the first. Header-tuple match plus a stop at the next h2.
  Verified against the live page: 27 tables in the section, 26 match, 286 rows parse, 0 without a
  result; the rejected one is the navigation table.
• Extract the real document. The content is HTML-escaped inside <iframe srcdoc="...">; the
  content iframe is the one with the longest srcdoc.
• Force UTF-8. boost.org serves the page with no charset, so requests falls back to
  ISO-8859-1 per the HTTP spec and mojibakes every accented name (Joaquín M López Muñoz ->
  Joaquín M López Muñoz).
• Fingerprint deduplication. A review is identified by normalized (submission + submitter +
  dates), so a re-import updates in place instead of adding a near-duplicate, and pre-existing
  duplicates collapse.
• Fail loudly. An unparseable page raises CommandError rather than returning quietly, which
  matters now that this also runs from a task.
• Timeout on the fetch (30s), and --clean moved inside the import transaction and after
  the parse, so a failed scrape can no longer leave the reviews deleted.
• Achievement safety: deleting a duplicate review calls discard_source_achievements first. A
  grant reaches its source through a generic FK with no referential integrity, so a bare delete
  would leave one still counting toward a threshold.
• Only GitHub URLs are stored as github_link, matched on hostname. The key is omitted
  otherwise, so a re-import can never erase a link entered by hand.
• ReviewAdmin: id, review-manager and scraped-review-manager columns, -id ordering, wider
  search, and the select_related / prefetch_related the results column needs.

The admin button

An Import Reviews from boost.org button on the review changelist, built on the task-button
mixin from PR 2. Gated on delete permission rather than change, because the command can merge
duplicate reviews and delete the losing rows along with the achievements sourced from them, and
with a confirmation page that says what may be deleted in the same words as the button's help
text.

The Reviewer achievement

• _iter_library_review yields (member, review) for every submitter that has a linked user.
  Submitters are CommitAuthor rows and only some are claimed; an unlinked one is skipped rather
  than guessed at.
• Registered, which also adds the slug to the commands' --source choices.
• import_reviews_task runs backfill_achievements --source library-review after the import,
  scoped to this one source so it does not walk the commit and library tables the task never
  touches.
• Tests: unlinked submitters are skipped, end-to-end backfill, the task's exact call sequence, and
  test_reconcile_preview_blocks_an_incomplete_catalogue, which needs a wired source to mean
  anything - it deletes this achievement's catalogue row to prove the reconcile preview refuses to
  offer Apply against an incomplete catalogue.

‼️ Risks & Considerations ‼️

• The dedup pass deletes rows on its first run. Run it against a copy of production data and
  check the reported count before running it for real.
• The header match is an exact five-column tuple. A cosmetic upstream rename ("Review /
  Release Dates") makes the command find zero tables and raise. That is deliberate: failing loudly
  beats importing nothing and reporting success. Same for aborting the run on a row with fewer
  than five cells - one malformed year costs the whole import rather than one table.
• The fingerprint is deliberately flexible, so two genuinely distinct reviews of the same library
  on the same dates by the same submitter would collapse. There are none today, and the dates
  component is what keeps re-reviews separate.
• The task backfills, it does not reconcile. An import that removes a review handles its
  grants through discard_source_achievements; one that reassigns a submitter leaves a stale
  grant until someone reconciles. That is the engine's deliberate split.
• Thresholds here are the lowest in the catalogue (1 / 2 / 3 / 4 / 5), because a formal review
  submission is far rarer than a commit. Worth sanity-checking against real data before the first
  production backfill.
• An unclaimed reviewer gets no grant, and gets one whenever they claim - right behaviour, but it
  means the count moves for reasons unrelated to reviewing.

Screenshots

Before	After

Peer-review testing steps

Everything runs from the admin button, which is the path production will use. Setup:
just load_production_data, just migrate, docker compose up. Open
https://www.boost.org/doc/formal-reviews/review-results.html in a tab first - most of these checks
are "does the admin agree with that page".

The import

1. On /admin/versions/review/, note the row count in the paginator before doing anything. On a
   production copy this is the pre-existing set, duplicates included.
2. Press Import Reviews from boost.org. Read the confirmation page first: it must warn that
   duplicate reviews, their results, and achievements sourced from them may be deleted. Continue, and
   watch the status line reach Finished in place.
3. Count. The paginator now reads ~286, and the count drops on a database that had duplicates.
4. Dedup. Order by Submission and scan for adjacent identical submissions carrying the same
   dates - there should be none. Press the button a second time: the count does not move, which is the
   fingerprint doing its job.
5. Encoding. Search Joaqu. The row must read Joaquín M López Muñoz, not Joaquín.
6. The two manager columns. Find a row where Review manager is empty but Scraped review manager
   has text: the FK is only filled when the scraped name matches a CommitAuthor exactly, and the raw
   text is kept either way. Add a CommitAuthor with that exact name at
   /admin/libraries/commitauthor/add/ and re-import: the resolved column fills in on the next run and
   the raw column is untouched.
7. A hand-entered GitHub link survives. Open a review, put a non-GitHub URL in github_link, save,
   and re-import: the value is still there, because only GitHub hostnames are written.
8. It fails loudly. Temporarily point the url in versions/management/commands/import_reviews.py
   at a page with no result tables and press the button again. The status line must read Failed with
   the CommandError message, and /admin/versions/review/ must still have every row. Put the URL
   back.
9. Permissions. As a staff user with change but not delete on Review, the button must not
   render on the changelist, and POSTing its URL must be refused. A GET on that URL must enqueue
   nothing.

The Reviewer badge

10. /admin/badges/badge/ - the Reviewer row's Automatic column is now a tick, the ladder reads
    1 / 2 / 3 / 4 / 5, and Holders is non-zero after the import. The import ran the backfill itself;
    nothing else had to be pressed.
11. /admin/badges/achievementsyncrun/ - a library-review row with trigger admin and your
    name under Triggered by, because the button hands the actor down to the backfill it runs. A
    second import adds another row with Added 0.
12. /admin/badges/userachievement/ filtered to Achievement: Library Review - each Source column
    links to the review row that justified it. Click one and confirm the submitter is the member on the
    grant.
13. An unclaimed submitter gets nothing, and gets it on claiming. Open a review and read its
    Submitters, then find one of them at /admin/libraries/commitauthor/ whose User field is
    empty. That person holds no Reviewer grant, which is deliberate - an unclaimed CommitAuthor is
    skipped rather than guessed at. Point the User field at a test member, save, then press Backfill
    achievements with Source = Library Review on the grant changelist. That member now has a
    Reviewer grant and Bronze, the per-member page states the count in words, and nothing else moves.
14. A reassignment needs a reconcile. Point that same CommitAuthor at a different member and
    press Backfill: the first member's grant stays, because backfill only adds. Then Reconcile
    achievements with Source = Library Review: the preview reports one addition and one removal,
    and applying it moves both the grant and the badge. That split between the two commands is the
    behaviour called out under Risks, worth seeing once rather than taking on trust.

Backend

• Parser tested against a captured sample page, not the network
• Verified against the live boost.org page with a read-only probe

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 88adad8e-2430-4505-8121-8db026817a72

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}