Skip to content

feat(assessment): she can untick questions and get the paper again (bd-60023) - #595

Merged
hatafatif merged 2 commits into
developfrom
bd-60023-08396
Sep 4, 2026
Merged

feat(assessment): she can untick questions and get the paper again (bd-60023)#595
hatafatif merged 2 commits into
developfrom
bd-60023-08396

Conversation

@hatafatif

Copy link
Copy Markdown
Collaborator

What this adds

The generated paper arrives finished and unchangeable. If two questions are wrong for her class, the only remedy today is to build another paper from scratch. This adds the review layer that V1.2.6 has had columns for since the day it was written — exam_json, original_exam_json, selected_question_ids, edited_at — and which nothing was ever built on.

After the document lands she gets a Flow listing her questions, all ticked. She unticks what she doesn't want, taps once, and the same paper comes back without them. No model call — the questions already exist, so it's a filter, a render and a send.

The three things that carry the risk

Risk How it's handled
Ids moving when the tree changes A question is addressed by path (seen.objective.MCQs.0), never printed position. A test pins the traversal against the renderer's collectQuestions, because her ticks are numbered against what the paper printed.
Losing ticks across a page turn Meta renders max 20 checkbox options; real staging papers came back at 10, 20, 28 and 64 questions. Ticks live in the session and merge per page, so a submit from page 1 can't drop what she never scrolled to.
NULL vs [] collapsing NULL = never chose = whole paper. [] = unticked everything, refused on the screen where she can still fix it, not rendered as a blank exam.

Two things Meta only told us when asked

Both found by uploading to a throwaway draft Flow and reading the validation errors — the file now validates with zero:

  • init-value is not allowed on CheckboxGroup. Preselection is init-values on the Form, keyed by component name. (Every guessed spelling — init-values, value, selected, default-value on the component — was refused by name.)
  • A screen may not route to itself. REVIEW → REVIEW is rejected as a routing loop; re-renders come back as data_exchange, which is not a routing-model edge.

A latent bug the tests caught

This module's job entry point is async function process, and that declaration shadows the global across the whole file — so process.env inside it is the job argument's undefined .env. The offer would have been a silent no-op in production. Found because a test asserted the send, not by reading the code.

Gate

  • 206 tests pass across 11 assessment suites (16 new selection, 10 revision, 12 review-screen, 4 delivery).
  • npm test exits 1 on this branch and exits 1 identically on a clean origin/develop — same 16 suites, same 34 offender files, byte-for-byte. Nothing here is new debt.
  • No migration: every column written already exists in V1.2.6.

Red-first throughout; each behaviour above failed before it passed.

Closes: bd-60023

The paper arrives finished and unchangeable. If two questions are wrong for
her class, the only remedy is to build another paper from scratch and hope.
The schema has anticipated this since V1.2.6 — exam_json as the living
document, original_exam_json frozen beside it, selected_question_ids, and
edited_at — and nothing was ever built on those columns. This builds it.

After the document lands she is offered a Flow that lists her questions,
every one ticked. She unticks what she does not want, taps once, and the
same paper comes back without them. No model call: the questions already
exist, so this is a filter, a render and a send, and it answers in seconds
rather than the minute generation takes.

Three things carry the risk, and each has tests:

  · A question is addressed by its PATH in the tree (seen.objective.MCQs.0),
    never by its printed position. Untick question 2 and everything after it
    renumbers, so a stored "question 4" would come to mean a different
    question than the one she chose. A test pins this traversal against the
    renderer's, because her ticks are numbered against what the paper printed.

  · Her ticks live in the session, not in the form. Meta renders at most 20
    checkbox options and real papers on staging came back at 10, 20, 28 and
    64 questions, so the screen pages — and the form only ever knows the
    twenty rows on screen. Merging per page is what stops a submit from an
    early page silently dropping every question she never scrolled to.

  · NULL and [] stay distinct. NULL is "she never chose", which is the whole
    paper; [] is "she unticked every one", which is refused on the screen
    where she can still fix it, rather than rendered as a blank exam.

Two things the Flow JSON only told us when Meta was asked. `init-value` is
not allowed on a CheckboxGroup — preselection is `init-values` on the Form,
keyed by component name — and a screen may not route to itself, so REVIEW
routes only to SUBMITTED and re-renders come back as data_exchange. Both
were found by uploading to a throwaway draft Flow and reading the errors;
the file now validates with zero of them.

One latent bug found by the tests rather than by reading: this module's job
entry point is `async function process`, and that declaration shadows the
global across the whole file, so `process.env` here is the job argument's
undefined `.env`. The offer would have been a silent no-op in production.

Gate: 206 tests pass across 11 assessment suites. `npm test` exits 1 on
this branch and exits 1 identically on a clean origin/develop — same 16
suites, same 34 offender files — so nothing here is new debt.

Closes: bd-60023
This repo is public and the source-hygiene guard forbids bd-/BUG-/FEAT-
references in shipped code. Two comments in the assessment services carried
one. They predate this branch, but the edit above moved one of them onto a
new line, which the guard reads offender-by-offender and so counts as new.

Removing the refs is the fix rather than widening the guard's allowlist: the
sentences say the same thing without them, and an allowlist entry would
outlive the reason for it.
@hatafatif

Copy link
Copy Markdown
Collaborator Author

CI note — test (22) failure is inherited, not introduced.

The baseline gate fails on this PR. It also fails on develop's own head (af9bd331) and on the last six merges into develop. I compared the two runs' NEW offenders in already-red suites blocks directly:

PR #595 develop head af9bd331
New offenders 100 101
Assessment files flagged none
Present on PR but not develop none

The offenders are in coaching-messages.js, ux-strings.js, observe-visit-flow.handler.js, text-message.handler.js and column-completeness — none of them touched here.

The first run of this PR did surface one offender of mine: assessment-orchestrator.service.js:155, a pre-existing (bd-60015) comment that my edit shifted onto a new line, which the offender-by-offender guard reads as new. Fixed in 7daf3607 by removing the ref from both assessment services rather than widening the allowlist — this repo is public and the rule is right.

Locally I also ran the gate against a clean origin/develop worktree: exit 1 there too, same 16 suites and 34 offender files, byte-for-byte identical to this branch.

Assessment suites on this branch: 206 passing across 11 suites.

@hatafatif
hatafatif merged commit accc89d into develop Sep 4, 2026
6 of 7 checks passed
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.

1 participant