Skip to content

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

Description

@herzog0

Context

versions.models.Review has exactly one writer - the boost.org results importer - and nothing else
in the codebase creates or updates those rows. So the Reviewer achievement and that importer are one
piece of work: the badge counts rows only the importer produces, and the natural moment to update the
grants is the moment the reviews change.

Two terms from the earlier engine ticket appear below. A backfill grants what a source supports and
the site is missing, and never removes anything. A reconcile is the two-way version, which also
removes grants a source no longer supports; it stays a deliberate human action and nothing here triggers
one.

The importer is broken. It scrapes https://www.boost.org/community/review_schedule.html, which
no longer exists, and matches tables by summary="Review Results", an attribute the replacement page
does not have. It therefore finds nothing and returns quietly.

The results now come from a regularly published Antora document,
boostorg/website-v2-docs/formal-reviews/modules/ROOT/pages/review-results.adoc, served at
https://www.boost.org/doc/formal-reviews/review-results.html. Three things about that page
break the old approach beyond the URL:

  1. The content is HTML-escaped inside an <iframe srcdoc="..."> wrapper.
  2. The section opens with a year-navigation table, and the old code takes the first table after
    the heading. Pointing the existing command at the new URL would import roughly two junk rows
    instead of the ~286 real reviews - and report success.
  3. boost.org serves the page with no charset, so requests falls back to ISO-8859-1 per the HTTP
    spec and every accented reviewer name is stored mojibaked.

Separately, the command has always created near-duplicate rows on every run, because its
update_or_create key is too narrow for the data.

Scope

The importer

  1. Point it at the current URL and extract the real document from the content iframe.
  2. Force UTF-8 decoding.
  3. Parse every result table in the section, not the first. Match on the header row and walk until
    the next h2, so the navigation table is rejected and each year's table is picked up.
  4. Replace the narrow update_or_create key with a normalized fingerprint - submission, submitter
    and review dates, accent-stripped, lowercased, punctuation removed - so a re-import updates in
    place, pre-existing duplicates collapse, and a library reviewed twice on different dates stays two
    records.
  5. Import oldest-first so the newest review gets the highest id, matching the ordering the admin and
    the public page use.
  6. Handle superseded results via the current markup.
  7. Fail loudly. An unparseable page must raise, not return quietly - this also runs from a task
    where nobody is watching the output.
  8. Time out the fetch, so a boost.org that accepts the connection and never answers cannot hold a
    worker for the life of the process.
  9. Move the command's existing --clean flag inside the import transaction and after the parse, so a
    failed scrape cannot leave the reviews deleted.
  10. Discard the achievements sourced from a review before deleting it as a duplicate. A grant points at
    its source row by table and id, with no database-level link back, so nothing stops the row being
    deleted underneath it and a bare delete leaves a grant still counting toward a threshold.
  11. Only store GitHub URLs as github_link, matched on hostname. Omit the key otherwise, so a
    re-import cannot erase a link entered by hand.
  12. Make the review changelist usable for this data: id, review-manager and scraped-review-manager
    columns, newest-first ordering, wider search, and whatever prefetching the results column needs
    to avoid a query per row.

The button

  1. A Celery task wrapper plus an Import Reviews from boost.org button on the review changelist,
    built on the existing task-button infrastructure. Gate it on delete permission rather than
    change - the command can merge duplicate reviews and delete the losing rows along with the
    achievements sourced from them - with a confirmation page that says what may be deleted in the
    same words as the button's help text.

The achievement

  1. A source iterator - the function the ingestion engine walks for one achievement - yielding
    (member, review) for every submitter that has a linked user.
    Submitters are CommitAuthor rows and only some are claimed by a member; an unlinked one is
    skipped rather than guessed at.
  2. Register it against its achievement, which also makes it selectable wherever a single source can be
    named - the commands and the admin buttons alike.
  3. Bring the grants into step at the end of the import task, for this source only: sweeping every
    source would walk the commit and library tables the import never touches. The additive sweep is the
    right one here, since removals are already handled by item 10.
  4. Tests: unlinked submitters are skipped; grants and badges produced end to end; the import task's call
    sequence; and one engine test that only becomes meaningful once a second source is wired - the admin's
    two-way sync must refuse to offer its Apply button when an achievement is missing from the seeded
    definitions.

Out of scope

  • Importing upcoming / scheduled reviews. The page still lists them; nothing consumes them.
  • Any change to versions.models.Review - the fields all exist already.
  • documentation_link. The submission cell also carries project pages and announcement posts, and
    guessing which is documentation is not this command's job.
  • Reconciling from the import task. Removals go through the achievement-discard path; a reassigned
    submitter leaves a stale grant until someone reconciles, which is the engine's deliberate split.
  • Improving how submitters are linked to members. That is upstream of this ticket.

Acceptance criteria

  • import_reviews populates Review and ReviewResult from the current page
  • All result tables are parsed and the navigation table rejected, verified against the live page
    (expect ~286 rows, 0 without a result)
  • Accented names round-trip correctly into both the raw fields and the FK lookup
  • A second run creates 0 reviews and deletes 0 rows
  • Pre-existing duplicates collapse, and the count is reported
  • Deleting a duplicate discards the achievements sourced from it
  • An unparseable page fails with an error instead of reporting success
  • --clean cannot leave the table empty after a failed scrape
  • Superseded results are marked as no longer most recent
  • The parser is tested against a captured sample page, not the network
  • The changelist button works, is gated on delete permission, and previews what may be deleted
  • A submitter with no linked user produces no grant, and gets one once they claim
  • The import task sweeps this source and only this source (tested on the call sequence)
  • Running the import twice does not change the grant count
  • Full suite green, pre-commit clean

Risks & considerations

Matching tables on an exact header row means a cosmetic upstream rename makes the command find zero
tables and raise. That is the trade worth taking: failing loudly beats importing nothing and
reporting success. Same for aborting on a malformed row - one bad year costs the whole import rather
than silently costing 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 apart.

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions