feat(b2b_dashboard): filter learner-progress by needs-attention

[daniellefrappier18]

What are the relevant tickets?

N/A — tracked in witan as tk-add-a-needs-attention-query-param-and-per-row-fi-71e3a4. Follows up #78 and unblocks the mit-learn filter UI (tk-add-the-needs-attention-filter-to-the-b2b-learne-225c49).

Description (What does it do?)

PR #78 added needs_attention_count to the LearnerProgressResponse envelope only, which supports a count tile but not a filter. This adds the two pieces mit-learn needs to build the "Needs attention" filter on the contract learner directory:

• A needs_attention query param on GET .../contracts/{contract_id}/learner-progress.
• A needs_attention field on each LearnerProgress row, consent-gated like the other outcome fields.

Both are built on the same expression needs_attention_count already uses, against a cutoff resolved once per request, so the row field, the filter and the count always apply the same rule on the same date.

This does not make the page and the count atomic, and isn't meant to. They are still two statements over a materialized view, so a refresh landing between them desynchronizes every count from the page — which is true today and independent of this PR. Only collapsing them into one statement would close that.

Filtering client-side was not an option: limit/offset paging and total_count are server-side, so narrowing one page in the browser yields wrong counts and wrong page math. Recomputing the rule client-side also gets the date basis wrong — CURRENT_DATE() resolves in the StarRocks cluster's timezone, and a browser-computed "today" can be a day off.

Implementation details

The rule lives in _needs_attention(cutoff), wrapped in COALESCE(..., FALSE). Two subtleties worth a reviewer's attention.

1. COALESCE, for three-valued logic.

A row with a grade but no tracked activity (last_active_on IS NULL, so completion_status = 'in_progress') makes the staleness comparison NULL, and FALSE OR NULL is NULL. The count's SUM(CASE WHEN ... THEN 1 ELSE 0 END) already folded that into its ELSE, so it was never visible. But the same expression in a WHERE clause behaves differently: an unguarded NULL is not FALSE, so without the guard that row would have

• fallen out of needs_attention=true and needs_attention=false alike, while still counting toward total_count — so the two directions would not partition the page, and
• serialized as needs_attention: null on a row whose outcomes_shared is true, contradicting the count that treats it as "no".

COALESCE(..., FALSE) settles it as "no", matching the documented intent ("no timestamp to judge quiet against") and making the expression two-valued for all three readers. needs_attention_count is unchanged — confirmed by evaluating both the old and new expressions over the same rows (identical results under both consent settings).

2. The cutoff is resolved once per request, not written as CURRENT_DATE(). (Raised by Copilot; fixed in a056224.)

The page and the count are two round trips. With CURRENT_DATE() in the SQL each evaluated it separately, so a request straddling midnight in the cluster's timezone could filter the page on one cutoff and count on another. The cutoff is CURRENT_DATE() - 30 days, so crossing midnight moves it forward and more learners qualify: a row could read needs_attention: false while needs_attention_count counted it, and with the filter on, total_count could exceed the set the page was drawn from.

The router now resolves it first with NEEDS_ATTENTION_CUTOFF_QUERY and threads it through. This is the pattern as_of already uses — read one database-derived value per request, hold it, use it everywhere — except it is never cached, because the failure it prevents is a date boundary and a cached cutoff would be wrong for exactly as long as the cache held it. The probe has no FROM, so StarRocks answers it without touching storage; a test pins that.

It is spliced as a literal rather than bound because it appears in the SELECT list, which precedes the WHERE clause the existing parameters bind to. _date_literal narrows the value to a datetime.date first, so the rendered text can only ever be 'YYYY-MM-DD'. It accepts date, datetime and an ISO-8601 string, since which of those a DATE column arrives as depends on the driver and the StarRocks build; datetime is truncated rather than passed through, because it subclasses date and rendering its isoformat unchanged would emit a time component.

Consent gating. The filter is gated exactly like the status filter: ({shared} AND [NOT] (...)). A withheld row matches neither direction, so filtering can't reveal the outcome the row is withholding. Unlike completion_status, there is no unknown escape hatch — a bool has no third member — so withheld rows are reachable only by omitting the filter. The param's description says so.

Row projection. needs_attention is derived in the outer select rather than added to _OUTCOMES, because it reads records.completion_status, an alias that only resolves there. It is added to learner_models._OUTCOME_FIELDS so the model validator nulls it too, and a new test pins those two lists in step so an outcome added to the query alone can't reach a manager ungated.

<= vs <. PR #78's description said the rule used <; the merged code uses <=. The code is correct — "at least 30 days ago" includes the 30th day, which is what the pre-existing boundary test already pinned — so the code is unchanged and the comment now states the intent explicitly. I've also corrected #78's description.

Not a CompletionStatusFilter member. Needing attention cuts across the four statuses rather than partitioning them (a stale in_progress row is both), so folding it in would break the "exactly one bucket per row" property CompletionStatusCounts rests on. It stays a separate boolean param.

How can this be tested?

Local checks, all passing on this branch:

uv run ruff check .                    # All checks passed
uv run ruff format --check .           # 75 files already formatted
uv run mypy src                        # no issues in 45 source files
uv run bin/generate-openapi-spec --check
uv run pytest                          # 331 passed

14 new tests in tests/test_dashboard_learner_progress.py, including two that execute the real _COMPLETION_STATUS and _needs_attention strings against rows in sqlite rather than asserting on SQL text:

• test_needs_attention_is_two_valued_so_the_filter_partitions_rows — the case above: asserts no row evaluates to NULL, that true/false partition the rows, and that true selects exactly what the count counts.
• test_needs_attention_boundary_is_computed_from_real_rows (extended) — adds the grade-but-no-activity row to the day-29/day-30 boundary cases.

The rest cover consent gating in both directions, the predicate being absent when the param is omitted, page and count sharing one WHERE clause, the row field reusing the count's expression, the field being withheld without consent, and a non-boolean value returning 422.

Plus coverage that the cutoff is resolved exactly once per request, reaches both statements, and that no CURRENT_DATE() survives in either; and that _date_literal normalizes every shape the driver might hand back and refuses anything it can't read as a date.

Not exercised against a live StarRocks instance. These tests mock the DB layer (per the test module's docstring), so COALESCE-over-boolean is verified in sqlite, not StarRocks. Two things a reviewer should check against a real mv_b2b_learner_enrollment — same caveat as #78:

• the generated SQL runs, and the bare 'YYYY-MM-DD' literal compares correctly against the DATE column;
• what Python type the driver returns for NEEDS_ATTENTION_CUTOFF_QUERY. _date_literal handles date, datetime and str, so any of those is fine — this is to confirm it isn't a fourth thing.

Since the cutoff is now a literal rather than DATE_SUB(CURRENT_DATE(), ...), the sqlite tests no longer rewrite the SQL to run it — they execute the production expression verbatim.

The generated predicate, for reference (consent fail-open):

WHERE (TRUE AND (COALESCE(completion_status = 'not_started'
      OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY), FALSE)))

Additional Context

• The OpenAPI spec diff is confined to the new query param and the new row field. CompletionStatusFilter gained an explanatory comment rather than a docstring, deliberately: a docstring on that enum renders into the published spec as the parameter's description, which put internal RST notes in front of API consumers.
• needs_attention is a required field on LearnerProgress (nullable, no default), consistent with the other outcome fields. Consumers deserializing the row model strictly will see a new key.

🤖 Generated with Claude Code

[github-actions]

OpenAPI Changes

Show/hide changes

## Changes for b2b_dashboard.yaml:
2 changes: 0 error, 0 warning, 2 info
info	[new-optional-request-parameter] at head/openapi/specs/b2b_dashboard.yaml
	in API GET /api/v1/analytics/organizations/{organization_id}/contracts/{contract_id}/learner-progress
		added the new optional `query` request parameter `needs_attention`

info	[response-required-property-added] at head/openapi/specs/b2b_dashboard.yaml
	in API GET /api/v1/analytics/organizations/{organization_id}/contracts/{contract_id}/learner-progress
		added the required property `data/items/needs_attention` to the response with the `200` status



## Changes for b2b_learner_records.yaml:
No changes detected

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

[Copilot]

Copilot review overview

🔵 Needs a closer look

The cutoff probe and date-literal comparison still need validation against live StarRocks and its driver.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Use one request-consistent cutoff date for data and count queries