Skip to content

fix(remark): map indicator_ordinal → ordinal, or no feedback ever reaches a teacher - #342

Merged
hatafatif merged 1 commit into
developfrom
bd-43488-97789
Aug 21, 2026
Merged

fix(remark): map indicator_ordinal → ordinal, or no feedback ever reaches a teacher#342
hatafatif merged 1 commit into
developfrom
bd-43488-97789

Conversation

@hatafatif

Copy link
Copy Markdown
Collaborator

Reported by Hasnat, 2026-08-17:

There is also a breakage in the feedback flow: teachers are currently not receiving the feedback submitted by the principal.

Confirmed, and it is worse than reported — the principal's confirmation is lost too.

Root cause

remark-delivery.deps.js selected indicator_ordinal and returned the rows untouched:

.select('indicator_ordinal, score')
return data || [];            // { indicator_ordinal: 1, score: 3 }

remark-rubric.js:243 requires ordinal and throws on anything else. That computeS call sits at remark-delivery.service.js:100above both try/catch blocks — so the throw escaped submitRemark entirely, taking the narrative, the teacher's note and sendToPrincipal with it.

It looked healthy because scores persist before the failure point and the principal is shown SUCCESS before the async delivery runs.

remark-narrative-retry.worker.js:61 has always done this mapping. The same query, mapped in one file and not the other, is the entire bug — and the worker is scheduled nowhere (no Procfile entry, no npm script, no cron), so nothing drained the backlog either.

Verified end to end

Driving the real submitRemark:

row shape outcome
raw (indicator_ordinal) throws malformed score row (missing ordinal){teacher:0, principal:0, generated:0, marked:0}
mapped (ordinal) {teacher:1, principal:1, generated:1}, deliveryPending: false

Why the suite never caught it

remark-delivery.test.js:36 hand-writes the correct shape, and nothing exercised makeDeliveryDeps — the code that produces the wrong one. 177 tests green while zero notes were delivered.

New remark-delivery-deps.test.js pins that boundary: whatever loadScores returns must be something computeS accepts. Full remark suite 182/182.

Follow-ups (deliberately not in this PR)

  • 24h window: sends are free-form type: 'text' with no template fallback. A teacher who has not messaged the bot in 24h will hit Meta error 131047. A teacher being evaluated is exactly the person unlikely to have messaged recently — so this may move the failure downstream rather than remove it. Worth confirming via Axiom on responseData.error.code after this lands.
  • Retry worker is unscheduled: once wired, it will sweep the whole backlog and fire notes about weeks-old evaluations. Drain deliberately.

Closes: bd-43488

…ches a teacher

loadScores selected `indicator_ordinal` and returned the rows untouched. computeS
requires `ordinal` and throws on anything else, and that call sits ABOVE both of
submitRemark's try/catch blocks — so the throw escaped the whole function and took
the narrative, the teacher's coaching note AND the principal's confirmation with
it. Scores still persisted (written before the failure) and the principal still
saw the SUCCESS screen (returned before the async work), so the flow looked
healthy while no teacher had ever received a note.

remark-narrative-retry.worker.js has always done this mapping. The same query,
mapped in one file and not the other, is the whole bug — and the worker is
scheduled nowhere, so nothing drained the backlog either.

Verified end to end by driving the real submitRemark: with the raw row shape it
throws with teacher/principal/generated/marked all at 0; with the mapped shape
both sends fire and deliveryPending is false.

Tests: new remark-delivery-deps.test.js pins the boundary the two halves
disagreed about — nothing exercised makeDeliveryDeps before, which is why 177
tests passed while production sent nothing. Full remark suite 182/182.

Reported by Hasnat, 2026-08-17.

Closes: bd-43488
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