Skip to content

[HnR-Autograder:6] Dashboard columns - #7446

Draft
gmorador-tribu wants to merge 3 commits into
refactor/students-table-variantsfrom
feat/checkpoint-dashboard-columns
Draft

[HnR-Autograder:6] Dashboard columns#7446
gmorador-tribu wants to merge 3 commits into
refactor/students-table-variantsfrom
feat/checkpoint-dashboard-columns

Conversation

@gmorador-tribu

Copy link
Copy Markdown
Contributor

The students table of a Hide & Reveal assignment repeats its metrics once per
phase, each under the header of its phase. When the assignment is also
auto-graded every phase gets its own grade plus a final grade; only the final
grade is synced, because the LMS gradebook has one grade per assignment.

Four commits, in order:

  1. GroupedActivityTable — the table with grouped column headers, assembled
    out of the frontend-shared primitives because DataTable cannot render a
    second header row. OrderableActivityTable becomes the facade which picks
    between the two, driven by the data: a column set which declares no group
    renders exactly as it does today, through DataTable.
  2. The checkpoint variants — plus a change to how the registry resolves one.
    Predicates which had to exclude each other's capabilities meant adding a
    variant edited the variants which came before it. Now a variant declares the
    capabilities it handles and the registry picks the one which declares exactly
    that set, so nothing has to be excluded and two variants can only claim the
    same assignment by declaring the same set.
  3. checkpoint_enabled in the dashboard API — emitted for every assignment
    rather than only when true, unlike auto_grading_config: the frontend reads
    it to choose between two table shapes, and an absent key would b
    indistinguishable from a backend which does not report it yet.
  4. Ordering the instructor email digest recipients — unrelated,
    purpose. That query had no ORDER BY, and adding the two dashboard API tests
    in this PR reshuffled the xdist distribution (the unit tests all share one
    database), which turned a test that had been passing by luck red
    this PR's pipeline fails. Happy to move it to its own PR off main if you'd
    rather keep this one focused.

Two things to read before reviewing:

  • None of this is reachable yet. The phase_metrics contract i
    the PoC in [HnR-Autograder:4] Annotations api #7445 and no backend emits it, so the checkpoint varia
    fall through to the "no phase reported" branch and only the tests exercise that
    code. That is deliberate.
  • **A real Hide & Reveal assignment looks exactly as it does today.
    student reports gets no columns, and while none is reported the table is the
    one the assignment would display without checkpoints. That is also the product
    rule — a section which is not defined gets no column — and it is
    this from rendering a grid of empty cells against the current backend.

Deliberately out of scope: column widths and the layout for mor
checkpoint (two checkpoints means twelve columns, and the headers stop fitting),
and the phase labels for more than one checkpoint, which cannot be derived yet
because the API reports a phase's position and never how many the a

# Emitted for every assignment rather than only when true: the
# dashboard picks the shape of its table off it, and an absent key
# would be indistinguishable from an older backend.
checkpoint_enabled=assignment.checkpoint_enabled,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This same field is added in PR #7445 (checkpoint_enabled on APIAssignment + this constructor + the same four full-dict assertions in assignment_test.py), so one of the two PRs has to drop it or we get a guaranteed conflict and two different comments explaining one field.
I'd suggest PR 7445 keeps it: it needs due_date on the same TypedDict anyway, so the two land together there. Then this PR drops js_config_types.py, views/dashboard/api/assignment.py and the test changes, and picks them up on rebase. Happy to do it the other way round if you'd rather

@Elimpizza
Elimpizza changed the base branch from refactor/students-table-variants to main August 24, 2026 14:00
@Elimpizza
Elimpizza changed the base branch from main to refactor/students-table-variants August 24, 2026 14:01
@Elimpizza
Elimpizza force-pushed the feat/checkpoint-dashboard-columns branch from 910669a to eb5a57b Compare August 24, 2026 14:04
@Elimpizza Elimpizza changed the title Feat/checkpoint dashboard columns [HnR-Autograer:6] Dashboard columns Aug 24, 2026
@Elimpizza
Elimpizza marked this pull request as draft August 24, 2026 14:09
@Elimpizza Elimpizza changed the title [HnR-Autograer:6] Dashboard columns [HnR-Autograder:6] Dashboard columns Aug 24, 2026
@Elimpizza
Elimpizza force-pushed the feat/checkpoint-dashboard-columns branch from eb5a57b to aa5259b Compare August 28, 2026 14:41
@Elimpizza
Elimpizza force-pushed the feat/checkpoint-dashboard-columns branch from aa5259b to 3ae153a Compare August 28, 2026 15:18
@Elimpizza
Elimpizza force-pushed the feat/checkpoint-dashboard-columns branch from 3ae153a to e270d18 Compare August 28, 2026 15:20
A Hide & Reveal assignment splits its activity into phases, and its table
repeats the same metrics once per phase. That needs a second header row
grouping the columns of each phase, which `DataTable` cannot render: it
builds its own single `<thead><tr>`, and its `children` prop renders
after the body.

So `GroupedActivityTable` assembles the same table out of the primitives
`frontend-shared` exports — `Table`/`TableHead`/`TableRow`/`TableCell`
plus `useOrderedRows` — and spans a group header over the run of adjacent
columns which declare it. It does not have the keyboard navigation, row
selection or confirm-on-double-click `DataTable` brings; the dashboard
does not use those in this table.

`OrderableActivityTable` becomes the facade which picks between the two,
so callers keep importing one component and the choice is driven by the
data: a column set which declares no group renders exactly as before,
through `DataTable`. `navigateOnConfirmRow` is passed only to that path
rather than spread into both, since the grouped table has no row
confirmation to hook into.

The column type gains two optional fields. `group` is the header a column
is displayed under; the label identifies the group, so two different
groups must not share one. `width` exists because `Table` lays out with
`table-fixed`, which divides the width evenly: a nine-column table gives
a formatted date less room than it needs, and `FormattedDate` does not
wrap. The widths go in a `<colgroup>` rather than on a header cell,
because with `table-fixed` the layout is taken from the first row — which
here is the group row, whose cells span several columns.
A Hide & Reveal assignment is graded per phase: the checkpoint reveals
delimit the phases and the last one closes at the due date. Its table
repeats the metrics once per phase, each under the header of its phase,
and when the assignment is also auto-graded every phase gets its grade
plus a final grade. Only the final grade is synced: the LMS gradebook has
one grade per assignment.

They are two variants rather than one because `gradesToSync` is read off
the module to decide whether the assignment grades at all, not called, so
a single module cannot make that decision at runtime.

The shape of the data follows the API this is being built against:
`phase_metrics` is a list with a 1-based `phase`, `ends_at`, the metrics
and an optional per-phase grade. `annotation_metrics` keeps holding the
totals, so the variants which know nothing about phases are unaffected.
Nothing emits `phase_metrics` yet, which is why the table degrades: a
phase no student reports gets no columns, and while none is reported the
table is exactly the one the assignment would display without
checkpoints. That is also the product rule — a section which is not
defined gets no column — and it is what keeps this from showing a grid of
empty cells against today's backend.

The phases are read off the students because that is where the API
reports them. The labels are not reported at all, only positions, so they
are derived: the last phase is the due date and the others are
checkpoints. That is correct for one checkpoint, which is what the MVP
has, and `MIN_PHASES` is what keeps the first phase from being called the
due date on the day after a reveal, before anybody has activity in the
next one. The comment on it says what would settle this for more than one.

Rows generate a field per phase, so the row type widens to an index
signature and the module interface becomes generic over it. The variants
whose fields are all known keep the checked row type, and every generated
key goes through `metricKey` so rows and columns cannot drift apart.

Registering the new variants also replaces the way the registry resolves
one. Predicates which had to exclude each other's capabilities meant
adding a variant edited the variants which came before it, which is the
opposite of what this registry is for. Now a variant declares the
capabilities it handles, the registry detects them in one place and picks
the variant which declares exactly that set. Nothing has to be excluded,
the order of the list stops mattering, and two variants can only claim
the same assignment by declaring the same set — an invariant a test
asserts about the registry rather than about a sample of assignments.
The query which picks the instructors to email had no `ORDER BY`, so
Postgres returned the rows in whatever physical order it found them in,
which depends on what else had been written to `user` before.

That made `test_it_sends_digests_for_instructors` pass or fail by luck: it
asserts the calls come out sorted by `h_userid`, which only held when the
heap happened to be laid out that way. The unit tests all share one
database (`tox.ini` points every worker at `lms_tests`) and run under
`--dist loadgroup`, so which tests a worker picks up — and therefore what
was written to `user` first — depends on the collected test list. Adding
the two dashboard API tests in this branch was enough to reshuffle it and
turn the test red, with no change to the code it covers.

Ordering in the query rather than in the test also makes the order the
digests go out in deterministic in production, which it was not.
@Elimpizza
Elimpizza force-pushed the feat/checkpoint-dashboard-columns branch from e270d18 to 660bee8 Compare September 1, 2026 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants