From d61691d62441b08c0975fd5d85fcb27785ce505a Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Wed, 30 Sep 2026 12:03:33 -0400 Subject: [PATCH 1/2] feat(b2b_dashboard): filter learner-progress by needs-attention PR #78 added `needs_attention_count` to the response envelope only, so mit-learn could show the number but not filter the directory down to it. Client-side filtering is not an option: `limit`/`offset` and `total_count` are server-side, so narrowing one page in the browser yields wrong counts and wrong page math. Adds both halves, built on the one `_NEEDS_ATTENTION` expression the count already uses, so a row, the filter and the count can never disagree: - `needs_attention` query param on `learner_progress`, consent-gated like the status filter -- a withheld row matches neither direction, so the filter can't reveal the outcome it is withholding. - `needs_attention` on the `LearnerProgress` row model, gated by the same `_OUTCOME_FIELDS` validator. This stops mit-learn recomputing the rule client-side, where it gets the date basis wrong: `CURRENT_DATE()` resolves in the StarRocks cluster timezone, and a browser-computed "today" can differ by a day. `_NEEDS_ATTENTION` is now wrapped in `COALESCE(..., FALSE)`. A row with a grade but no tracked activity makes the staleness comparison NULL. The count's `SUM(CASE WHEN ...)` already folded that into its ELSE, but an unguarded NULL in a WHERE clause is not FALSE, so such a row would have fallen out of `needs_attention=true` and `needs_attention=false` alike while still counting toward `total_count`, and would have serialized as `needs_attention: null` on a row whose outcomes are shared. The count's result is unchanged. `<=` is kept over PR #78's prose `<`: "at least 30 days ago" includes the 30th day, which is what the existing boundary test pins. The comment now says so. Needing attention is deliberately not a `CompletionStatusFilter` member: it cuts across the four statuses rather than partitioning them, so folding it in would break the "exactly one bucket per row" property `CompletionStatusCounts` rests on. Co-Authored-By: Claude Opus 5 --- openapi/specs/b2b_dashboard.yaml | 23 +++ .../tenants/b2b_dashboard/learner_models.py | 15 ++ .../tenants/b2b_dashboard/learner_queries.py | 27 +++- .../tenants/b2b_dashboard/routers/learners.py | 18 +++ tests/test_dashboard_learner_progress.py | 143 +++++++++++++++++- 5 files changed, 222 insertions(+), 4 deletions(-) diff --git a/openapi/specs/b2b_dashboard.yaml b/openapi/specs/b2b_dashboard.yaml index 96da131..fb04e3f 100644 --- a/openapi/specs/b2b_dashboard.yaml +++ b/openapi/specs/b2b_dashboard.yaml @@ -520,6 +520,20 @@ paths: description: Exact match. Narrows to one course run, e.g. the module filter. title: Courserun Readable Id description: Exact match. Narrows to one course run, e.g. the module filter. + - name: needs_attention + in: query + required: false + schema: + anyOf: + - type: boolean + - type: 'null' + description: Keep only the learners who need attention, or only those who + don't. Cuts the same rows `needs_attention_count` counts. Rows with withheld + outcomes match neither, so omit this to see them. + title: Needs Attention + description: Keep only the learners who need attention, or only those who + don't. Cuts the same rows `needs_attention_count` counts. Rows with withheld + outcomes match neither, so omit this to see them. - name: include_inactive in: query required: false @@ -1694,6 +1708,14 @@ components: description: The last day the learner did anything in the course. Empty if they haven't yet. Hidden if the learner hasn't agreed to share their progress. + needs_attention: + anyOf: + - type: boolean + - type: 'null' + title: Needs Attention + description: 'Whether this learner may need a nudge: they never started + the course, or their last recorded activity was at least 30 days ago. + Hidden if the learner hasn''t agreed to share their progress.' type: object required: - learner_id @@ -1714,6 +1736,7 @@ components: - certificate_issued_on - certificate_is_revoked - last_active_on + - needs_attention title: LearnerProgress description: One learner's enrollment in one course run under the contract. LearnerProgressResponse: diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py b/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py index 7978536..b17b6be 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py @@ -34,6 +34,14 @@ can be ``in_progress`` and also counted here. A grade-only ``in_progress`` row with no ``last_active_on`` has no recorded activity to judge stale, so it isn't counted either. +- ``needs_attention`` is the per-row form of that same rule, from the one + ``learner_queries._NEEDS_ATTENTION`` expression the count is built on, so a + row can never disagree with the count it is summarized by. Read it rather + than recomputing it client-side: the 30-day cutoff is evaluated against + ``CURRENT_DATE()`` in the StarRocks cluster's timezone, which a browser's + own "today" can be a day off from. It is consent-gated, so it is NULL + exactly when the other outcome fields are; it is never NULL otherwise, not + even on a row with no ``last_active_on``. """ from __future__ import annotations @@ -61,6 +69,7 @@ def _assume_utc(value: datetime.datetime) -> datetime.datetime: "certificate_issued_on", "certificate_is_revoked", "last_active_on", + "needs_attention", ) _HIDDEN = "Hidden if the learner hasn't agreed to share their progress." @@ -148,6 +157,12 @@ class LearnerProgress(BaseModel): f"{_HIDDEN}" ) ) + needs_attention: bool | None = Field( + description=( + "Whether this learner may need a nudge: they never started the course, or their " + f"last recorded activity was at least 30 days ago. {_HIDDEN}" + ) + ) @model_validator(mode="after") def _gate_outcomes(self) -> Self: diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py b/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py index 0d6109d..623703f 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py @@ -49,12 +49,22 @@ # A learner needs attention if they never started, or if their last recorded # activity was at least 30 days ago (product definition, Danielle Frappier). +# `<=` is deliberate: "at least 30 days ago" includes the 30th day itself, and +# test_needs_attention_boundary_is_computed_from_real_rows pins that day. +# # A NULL last_active_on on a non-not_started row (grade but no tracked # activity) doesn't match the staleness branch -- there's no timestamp to -# judge quiet against. +# judge quiet against. COALESCE settles that as "no" instead of NULL, which +# makes the expression two-valued. That matters now that all three readers +# share it: SUM(CASE WHEN ...) already folded NULL into its ELSE, but an +# unguarded NULL in a WHERE clause is not FALSE, so such a row would fall out +# of `needs_attention=true` AND `needs_attention=false` alike, and project +# `needs_attention: null` on a row whose outcomes are shared. _NEEDS_ATTENTION = ( + "COALESCE(" "completion_status = 'not_started'" " OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY)" + ", FALSE)" ) # Upstream stores "" rather than NULL for learners who never set a name. Null @@ -102,6 +112,7 @@ class ProgressFilters: search: str | None = None completion_statuses: tuple[str, ...] = () courserun_readable_id: str | None = None + needs_attention: bool | None = None include_inactive: bool = False sort: SortKey = SortKey.FULL_NAME descending: bool = False @@ -168,6 +179,14 @@ def learner_progress(filters: ProgressFilters) -> ProgressQuery: if "unknown" in filters.completion_statuses: alternatives.append(f"NOT {shared}") predicates.append(f"({' OR '.join(alternatives)})") + if filters.needs_attention is not None: + # Consent-gated like the status filter, and for the same reason: a + # withheld row matches neither direction, so filtering can't reveal the + # outcome the row is withholding. There is no `unknown` escape hatch + # here as there is for status -- a bool has no third member -- so + # withheld rows are reachable only by leaving this filter off. + negate = "" if filters.needs_attention else "NOT " + predicates.append(f"({shared} AND {negate}({_NEEDS_ATTENTION}))") where = f" WHERE {' AND '.join(predicates)}" if predicates else "" direction = "DESC" if filters.descending else "ASC" @@ -181,6 +200,12 @@ def learner_progress(filters: ProgressFilters) -> ProgressQuery: *_COLUMNS, f"{shared} AS outcomes_shared", *(f"CASE WHEN {shared} THEN {name} END AS {name}" for name in _OUTCOMES), + # Derived here rather than joining _OUTCOMES, which are columns: + # this reads `records.completion_status`, an alias that only + # resolves in this outer select. It is the same _NEEDS_ATTENTION + # the count and the filter use, so a row, the page it came on and + # needs_attention_count can never disagree about it. + f"CASE WHEN {shared} THEN {_NEEDS_ATTENTION} END AS needs_attention", ] ) page = ( diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py b/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py index 2c29cbf..926f1bc 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py @@ -45,6 +45,13 @@ ) +# CompletionStatus plus `unknown` for the withheld rows. A docstring here would +# render into the published spec as the parameter's description, so this stays a +# comment: needing attention is deliberately not a member, because it cuts +# across these four instead of partitioning them (a stale `in_progress` row is +# both). Folding it in would break the "exactly one bucket per row" property +# CompletionStatusCounts rests on, so it is the separate `needs_attention` +# filter below. class CompletionStatusFilter(StrEnum): NOT_STARTED = "not_started" IN_PROGRESS = "in_progress" @@ -83,6 +90,16 @@ async def learner_progress( # noqa: PLR0913 description="Exact match. Narrows to one course run, e.g. the module filter.", ), ] = None, + needs_attention: Annotated[ + bool | None, + Query( + description=( + "Keep only the learners who need attention, or only those who don't. " + "Cuts the same rows `needs_attention_count` counts. Rows with withheld " + "outcomes match neither, so omit this to see them." + ) + ), + ] = None, include_inactive: Annotated[ bool, Query(description="Include deactivated enrollments (unenrolled, refunded).") ] = False, @@ -96,6 +113,7 @@ async def learner_progress( # noqa: PLR0913 search=search, completion_statuses=tuple(status.value for status in completion_status or ()), courserun_readable_id=courserun_readable_id, + needs_attention=needs_attention, include_inactive=include_inactive, sort=sort, descending=descending, diff --git a/tests/test_dashboard_learner_progress.py b/tests/test_dashboard_learner_progress.py index ff26d2b..8359b2a 100644 --- a/tests/test_dashboard_learner_progress.py +++ b/tests/test_dashboard_learner_progress.py @@ -18,7 +18,7 @@ from ol_analytics_api.core.db.refresh_metadata import _clear_cache from ol_analytics_api.main import create_app -from ol_analytics_api.tenants.b2b_dashboard import learner_queries +from ol_analytics_api.tenants.b2b_dashboard import learner_models, learner_queries from ol_analytics_api.tenants.b2b_dashboard.config import settings from ol_analytics_api.tenants.b2b_dashboard.learner_models import ( CompletionStatusCounts, @@ -57,6 +57,7 @@ def _row(**overrides): "certificate_issued_on": None, "certificate_is_revoked": None, "last_active_on": None, + "needs_attention": None, **overrides, } @@ -235,8 +236,8 @@ async def test_needs_attention_count_reported_from_the_count_query(app): assert response.json()["needs_attention_count"] == 7 count_query, _ = pool.count_call() assert ( - "SUM(CASE WHEN FALSE AND (completion_status = 'not_started'" - " OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY))" + "SUM(CASE WHEN FALSE AND (COALESCE(completion_status = 'not_started'" + " OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY), FALSE))" " THEN 1 ELSE 0 END) AS needs_attention_count" in count_query ) @@ -280,6 +281,7 @@ def test_needs_attention_boundary_is_computed_from_real_rows(): (1, 0, None, None), # never started (1, 0, None, (today - datetime.timedelta(days=29)).isoformat()), # active 29 days ago (1, 0, None, cutoff.isoformat()), # active exactly 30 days ago + (1, 0, 0.5, None), # graded, but no tracked activity at all ], ) rows = conn.execute( @@ -293,6 +295,9 @@ def test_needs_attention_boundary_is_computed_from_real_rows(): ("not_started", 1), # never started: needs attention ("in_progress", 0), # active 29 days ago: still recent ("in_progress", 1), # active exactly 30 days ago: needs attention + # No timestamp to judge quiet against, so: no. Not NULL -- see + # test_needs_attention_is_two_valued_so_the_filter_partitions_rows. + ("in_progress", 0), ] @@ -334,6 +339,122 @@ def count(shared): conn.close() +def test_needs_attention_is_two_valued_so_the_filter_partitions_rows(): + # A row with a grade but no tracked activity makes the staleness + # comparison NULL. Unguarded that is neither true nor false, so the row + # would fall out of `needs_attention=true` and `needs_attention=false` + # alike while still counting toward total_count, and would serialize as + # `needs_attention: null` on a row whose outcomes are shared. COALESCE in + # ._NEEDS_ATTENTION settles it as false; this pins that every shared row + # lands in exactly one direction of the filter. + today = datetime.date.today() # noqa: DTZ011 - the boundary is date-only + cutoff = today - datetime.timedelta(days=30) + needs_attention = _needs_attention_sql(cutoff) + + conn = sqlite3.connect(":memory:") + conn.execute( + "CREATE TABLE enrollment (certificate_is_revoked INTEGER, is_passing INTEGER," + " grade_value REAL, last_active_on TEXT)" + ) + conn.executemany( + "INSERT INTO enrollment VALUES (?, ?, ?, ?)", + [ + (1, 0, None, None), # never started + (1, 0, 0.5, None), # graded, but no tracked activity at all + (1, 0, 0.5, (today - datetime.timedelta(days=29)).isoformat()), # active 29 days ago + (1, 0, 0.5, cutoff.isoformat()), # active exactly 30 days ago + ], + ) + records = ( + f"(SELECT *, {learner_queries._COMPLETION_STATUS} AS completion_status FROM enrollment)" # noqa: S608, SLF001 + ) + + def matching(*, direction): + negate = "" if direction else "NOT " + return conn.execute( + f"SELECT COUNT(*) FROM {records} WHERE (TRUE AND {negate}({needs_attention}))" # noqa: S608 + ).fetchone()[0] + + counted = conn.execute( + f"SELECT SUM(CASE WHEN TRUE AND ({needs_attention}) THEN 1 ELSE 0 END) FROM {records}" # noqa: S608 + ).fetchone()[0] + nulls = conn.execute( + f"SELECT COUNT(*) FROM {records} WHERE ({needs_attention}) IS NULL" # noqa: S608 + ).fetchone()[0] + selected = (matching(direction=True), matching(direction=False)) + conn.close() + + assert nulls == 0 + # The two directions partition the four rows: none lost, none double-counted. + assert selected == (2, 2) + # And `true` selects exactly the rows needs_attention_count counts. + assert selected[0] == counted + + +async def test_needs_attention_filter_is_consent_gated_in_both_directions(app): + for value, negate in (("true", ""), ("false", "NOT ")): + pool = _FakePool() + await _get(app, pool, params={"needs_attention": value}) + expected = ( + f"(FALSE AND {negate}(COALESCE(completion_status = 'not_started'" + " OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY), FALSE)))" + ) + # The `FALSE AND` is the consent gate: a withheld row matches neither + # direction, so the filter can't reveal the outcome it withholds. + assert expected in pool.page_call()[0] + assert expected in pool.count_call()[0] + + +async def test_needs_attention_filter_is_omitted_when_not_given(app): + # Absent means "don't filter", which is the only way to see withheld rows. + # The row projection still carries the expression; only the predicate goes. + pool = _FakePool() + await _get(app, pool) + page_query = pool.page_call()[0] + assert "FALSE AND (COALESCE" not in page_query + assert "FALSE AND NOT (COALESCE" not in page_query + assert page_query.count("WHERE") == 1 + + +async def test_needs_attention_filter_narrows_the_counts_with_the_page(app): + # Same WHERE on both, so needs_attention_count and the status buckets + # describe the filtered set the page came from, not the contract. + pool = _FakePool() + await _get(app, pool, params={"needs_attention": "true", "completion_status": ["in_progress"]}) + count_query, _ = pool.count_call() + assert count_query.count("WHERE") == 2 + assert "completion_status IN (%s)" in count_query + assert "needs_attention_count" in count_query + + +async def test_needs_attention_row_field_reuses_the_count_expression(app): + # One expression behind the row, the filter and the count, so mit-learn + # never has to recompute the 30-day rule against a browser's own "today". + pool = _FakePool() + await _get(app, pool) + page_query = pool.page_call()[0] + assert ( + f"CASE WHEN FALSE THEN {learner_queries._NEEDS_ATTENTION} END AS needs_attention" # noqa: SLF001 + in page_query + ) + assert learner_queries._NEEDS_ATTENTION in pool.count_call()[0] # noqa: SLF001 + + +async def test_needs_attention_row_field_is_served_and_withheld_with_consent(app): + pool = _FakePool( + rows=[_row(outcomes_shared=1, completion_status="not_started", needs_attention=1)] + ) + assert (await _get(app, pool)).json()["data"][0]["needs_attention"] is True + + pool = _FakePool(rows=[_row(outcomes_shared=0, needs_attention=1)]) + assert (await _get(app, pool)).json()["data"][0]["needs_attention"] is None + + +async def test_non_boolean_needs_attention_is_rejected(app): + response = await _get(app, _FakePool(), params={"needs_attention": "stale"}) + assert response.status_code == 422 + + def test_completion_status_buckets_are_mutually_exclusive_and_exhaustive(): # Each row's completion_status is exactly one CASE branch # (learner_queries._COMPLETION_STATUS), so the four buckets never overlap @@ -424,6 +545,22 @@ def test_every_outcome_column_is_consent_gated_in_the_query(): ) for name in ("completion_status", "is_passing", "grade", "letter_grade", "last_active_on"): assert f"CASE WHEN FALSE THEN {name} END AS {name}" in query.page + # needs_attention is derived, not selected, so the same gate wraps an + # expression rather than a column name. + assert ( + f"CASE WHEN FALSE THEN {learner_queries._NEEDS_ATTENTION} END AS needs_attention" # noqa: SLF001 + in query.page + ) + + +def test_the_model_gates_every_outcome_the_query_projects(): + # The two lists are maintained apart (learner_models._OUTCOME_FIELDS is the + # second check the query change can't defeat), so pin them in step: an + # outcome added to the query alone would reach a manager ungated. + assert set(learner_models._OUTCOME_FIELDS) == { # noqa: SLF001 + *learner_queries._OUTCOMES, # noqa: SLF001 + "needs_attention", + } @pytest.mark.parametrize( From a0562240b615c8f99978eec73ec4648387be279c Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Wed, 30 Sep 2026 13:28:58 -0400 Subject: [PATCH 2/2] fix(b2b_dashboard): resolve the needs-attention cutoff once per request Addresses Copilot's review on #84. The page and the count are two statements. With `CURRENT_DATE()` left in the SQL, each evaluated it independently, 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 wrong page math this endpoint exists to avoid. `learner_queries` no longer writes `CURRENT_DATE()` into either data query. The router resolves the cutoff first, with `NEEDS_ATTENTION_CUTOFF_QUERY`, and passes it to the one `_needs_attention(cutoff)` expression behind the row field, the filter and the count. This is the pattern `as_of` already uses: read one database-derived value per request, hold it, use it everywhere. Unlike `as_of` it is never cached -- the failure it prevents is a date boundary, so a cached cutoff would be wrong for exactly as long as the cache held it. The probe has no FROM clause, so StarRocks answers it without touching storage; a test pins that. The cutoff 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 -- binding it would mean prepending it to a tuple the page and count queries share. `_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, because which of those a DATE column arrives as depends on the driver and the StarRocks build, and rejecting the other two would turn a type surprise into a 500 on every request. `datetime` is truncated rather than passed through: it subclasses `date`, so rendering its isoformat unchanged would emit a time component and change the comparison. Written bare rather than as `DATE '...'` so it parses in sqlite too. That removes the string substitution the sqlite tests needed: they now execute the production expression verbatim. This does not make the page and count atomic, and is not meant to. They remain two statements over a materialized view, so an MV refresh landing between them still desynchronizes every count from the page. Only a single statement would close that, which is a separate change. The PR wording is corrected to claim what is true: one rule and one cutoff, not one snapshot. Co-Authored-By: Claude Opus 5 --- .../tenants/b2b_dashboard/learner_models.py | 17 +-- .../tenants/b2b_dashboard/learner_queries.py | 112 ++++++++++++++---- .../tenants/b2b_dashboard/routers/learners.py | 15 ++- tests/test_dashboard_learner_progress.py | 92 +++++++++++--- 4 files changed, 188 insertions(+), 48 deletions(-) diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py b/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py index b17b6be..d3cafeb 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py @@ -34,14 +34,15 @@ can be ``in_progress`` and also counted here. A grade-only ``in_progress`` row with no ``last_active_on`` has no recorded activity to judge stale, so it isn't counted either. -- ``needs_attention`` is the per-row form of that same rule, from the one - ``learner_queries._NEEDS_ATTENTION`` expression the count is built on, so a - row can never disagree with the count it is summarized by. Read it rather - than recomputing it client-side: the 30-day cutoff is evaluated against - ``CURRENT_DATE()`` in the StarRocks cluster's timezone, which a browser's - own "today" can be a day off from. It is consent-gated, so it is NULL - exactly when the other outcome fields are; it is never NULL otherwise, not - even on a row with no ``last_active_on``. +- ``needs_attention`` is the per-row form of that same rule, built from the + one ``learner_queries._needs_attention`` expression the count is built on, + against a cutoff the router resolves on the cluster once per request. So a + row and the count it is summarized by apply the same rule on the same date, + even though they arrive on separate round trips. Read it rather than + recomputing it client-side: the cutoff is 30 days before the StarRocks + cluster's today, which a browser's own "today" can be a day off from. It is + consent-gated, so it is NULL exactly when the other outcome fields are; it + is never NULL otherwise, not even on a row with no ``last_active_on``. """ from __future__ import annotations diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py b/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py index 623703f..0ed0e7c 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py @@ -11,6 +11,15 @@ predicates and orderings is included, and code chooses that, not input. That is the justification for each ``# noqa: S608`` below. +The one spliced value is the needs-attention cutoff, and it is not +caller-supplied: the router reads it from StarRocks with +``NEEDS_ATTENTION_CUTOFF_QUERY``, and ``_date_literal`` narrows it to a +``datetime.date`` before rendering, so the text it produces can only ever be +``'YYYY-MM-DD'``. It is spliced rather than bound because it appears in the +SELECT list, which precedes the WHERE clause the existing parameters bind to; +binding it would mean prepending it to a tuple the page and count queries +share. + Each query is two layers, as in the b2b_learner_records tenant. The inner ``records`` select scopes to the organization and contract and derives ``completion_status``; the outer select applies consent, search and the status @@ -23,6 +32,7 @@ from __future__ import annotations +import datetime from dataclasses import dataclass from enum import StrEnum from typing import Any @@ -47,26 +57,76 @@ " END" ) -# A learner needs attention if they never started, or if their last recorded -# activity was at least 30 days ago (product definition, Danielle Frappier). -# `<=` is deliberate: "at least 30 days ago" includes the 30th day itself, and -# test_needs_attention_boundary_is_computed_from_real_rows pins that day. -# -# A NULL last_active_on on a non-not_started row (grade but no tracked -# activity) doesn't match the staleness branch -- there's no timestamp to -# judge quiet against. COALESCE settles that as "no" instead of NULL, which -# makes the expression two-valued. That matters now that all three readers -# share it: SUM(CASE WHEN ...) already folded NULL into its ELSE, but an -# unguarded NULL in a WHERE clause is not FALSE, so such a row would fall out -# of `needs_attention=true` AND `needs_attention=false` alike, and project -# `needs_attention: null` on a row whose outcomes are shared. -_NEEDS_ATTENTION = ( - "COALESCE(" - "completion_status = 'not_started'" - " OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY)" - ", FALSE)" +NEEDS_ATTENTION_QUIET_DAYS = 30 + +# Resolves the cutoff on the StarRocks cluster, whose timezone is the one the +# rule is defined in -- not this process's, and not a browser's. +NEEDS_ATTENTION_CUTOFF_QUERY = ( + f"SELECT DATE_SUB(CURRENT_DATE(), INTERVAL {NEEDS_ATTENTION_QUIET_DAYS} DAY) AS cutoff" ) + +def _date_literal(value: object) -> str: + """``value`` as a quoted SQL date literal, ``'YYYY-MM-DD'``. + + Splicing is safe only because this narrows to a real calendar date first: + the rendered text comes from a ``datetime.date``, never from the input + string. Anything it cannot read as a date raises rather than reaching the + query -- see the module docstring. + + It accepts ``date``, ``datetime`` and an ISO-8601 string because the value + crosses the DB driver, and which of those a DATE column arrives as depends + on the driver and the StarRocks build. Rejecting the other two would turn + a type surprise into a 500 on every request. ``datetime`` is truncated + rather than passed through: it subclasses ``date``, so rendering its + isoformat unchanged would emit ``YYYY-MM-DDTHH:MM:SS`` and change the + comparison. + + Written bare rather than as ``DATE '...'`` so the expression parses in + sqlite too, which is what lets the tests execute the production string + verbatim instead of rewriting it. StarRocks casts an ISO-8601 literal to + DATE to compare it against a DATE column. + """ + if isinstance(value, datetime.datetime): + value = value.date() + elif isinstance(value, str): + value = datetime.date.fromisoformat(value) + if type(value) is not datetime.date: + message = f"needs-attention cutoff must be a date, got {type(value).__name__}" + raise TypeError(message) + return f"'{value.isoformat()}'" + + +def _needs_attention(cutoff: datetime.date) -> str: + """A learner needs attention if they never started, or if their last + recorded activity was on or before ``cutoff`` (30 days before the + cluster's today). + + ``<=`` is deliberate: "at least 30 days ago" includes the 30th day itself, + and test_needs_attention_boundary_is_computed_from_real_rows pins that day. + + A NULL last_active_on on a non-not_started row (grade but no tracked + activity) doesn't match the staleness branch -- there's no timestamp to + judge quiet against. COALESCE settles that as "no" instead of NULL, which + makes the expression two-valued. That matters because all three readers + share it: SUM(CASE WHEN ...) already folded NULL into its ELSE, but an + unguarded NULL in a WHERE clause is not FALSE, so such a row would fall out + of ``needs_attention=true`` AND ``needs_attention=false`` alike, and + project ``needs_attention: null`` on a row whose outcomes are shared. + + The cutoff is passed in rather than written as ``CURRENT_DATE()`` so that + the page and count statements -- two round trips, and so two evaluations -- + cannot straddle midnight and disagree about which learners are quiet. The + caller resolves it once per request, as it already does for ``as_of``. + """ + return ( + "COALESCE(" + "completion_status = 'not_started'" + f" OR last_active_on <= {_date_literal(cutoff)}" + ", FALSE)" + ) + + # Upstream stores "" rather than NULL for learners who never set a name. Null # blank names so they sort with the missing ones instead of before every name. _BLANK_AS_NULL_NAME = "NULLIF(TRIM(full_name), '')" @@ -138,7 +198,7 @@ def _contains_pattern(term: str) -> str: return f"%{escaped}%" -def learner_progress(filters: ProgressFilters) -> ProgressQuery: +def learner_progress(filters: ProgressFilters, cutoff: datetime.date) -> ProgressQuery: table = f"{validate_sql_identifier(settings.learner_records_schema)}.{ENROLLMENT_MV}" # The org predicate stays next to the contract one: contract ids are # globally unique but not secret. @@ -157,6 +217,8 @@ def learner_progress(filters: ProgressFilters) -> ProgressQuery: ) shared = _outcomes_shared() + # One expression, one cutoff, for the row field, the filter and the count. + needs_attention = _needs_attention(cutoff) predicates: list[str] = [] if filters.search: pattern = _contains_pattern(filters.search) @@ -186,7 +248,7 @@ def learner_progress(filters: ProgressFilters) -> ProgressQuery: # here as there is for status -- a bool has no third member -- so # withheld rows are reachable only by leaving this filter off. negate = "" if filters.needs_attention else "NOT " - predicates.append(f"({shared} AND {negate}({_NEEDS_ATTENTION}))") + predicates.append(f"({shared} AND {negate}({needs_attention}))") where = f" WHERE {' AND '.join(predicates)}" if predicates else "" direction = "DESC" if filters.descending else "ASC" @@ -202,10 +264,10 @@ def learner_progress(filters: ProgressFilters) -> ProgressQuery: *(f"CASE WHEN {shared} THEN {name} END AS {name}" for name in _OUTCOMES), # Derived here rather than joining _OUTCOMES, which are columns: # this reads `records.completion_status`, an alias that only - # resolves in this outer select. It is the same _NEEDS_ATTENTION - # the count and the filter use, so a row, the page it came on and - # needs_attention_count can never disagree about it. - f"CASE WHEN {shared} THEN {_NEEDS_ATTENTION} END AS needs_attention", + # resolves in this outer select. Same expression and same cutoff + # as the filter and the count, so the three cannot disagree about + # which learners are quiet. + f"CASE WHEN {shared} THEN {needs_attention} END AS needs_attention", ] ) page = ( @@ -220,7 +282,7 @@ def learner_progress(filters: ProgressFilters) -> ProgressQuery: "SELECT COUNT(*) AS total_count," # noqa: S608 f" SUM(CASE WHEN {shared} THEN 0 ELSE 1 END) AS outcomes_withheld_count," f" {status_sums}," - f" SUM(CASE WHEN {shared} AND ({_NEEDS_ATTENTION}) THEN 1 ELSE 0 END)" + f" SUM(CASE WHEN {shared} AND ({needs_attention}) THEN 1 ELSE 0 END)" " AS needs_attention_count" f" FROM ({records}) records{where}" ) diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py b/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py index 926f1bc..085c369 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py @@ -106,6 +106,18 @@ async def learner_progress( # noqa: PLR0913 sort: learner_queries.SortKey = learner_queries.SortKey.FULL_NAME, descending: bool = False, ) -> LearnerProgressResponse: + # Resolve the needs-attention cutoff on the cluster once, before either + # data query. The page and the count are two statements, so leaving + # CURRENT_DATE() in the SQL would let them evaluate it either side of + # midnight and disagree about which learners are quiet -- a row reading + # `false` while needs_attention_count counted it, and a `total_count` the + # page's own filter contradicts. Resolved per request and never cached: + # the failure this prevents is a date boundary, so a cached cutoff would + # be wrong for exactly as long as the cache held it. This is the one + # non-deterministic expression in the tenant's SQL. + cutoff = (await starrocks_pool.fetch_all(learner_queries.NEEDS_ATTENTION_CUTOFF_QUERY))[0][ + "cutoff" + ] query = learner_queries.learner_progress( learner_queries.ProgressFilters( organization_id=organization_id, @@ -117,7 +129,8 @@ async def learner_progress( # noqa: PLR0913 include_inactive=include_inactive, sort=sort, descending=descending, - ) + ), + cutoff, ) # Freshness first, so a refresh landing mid-request labels newer rows with # the older as_of rather than the reverse. diff --git a/tests/test_dashboard_learner_progress.py b/tests/test_dashboard_learner_progress.py index 8359b2a..95148d4 100644 --- a/tests/test_dashboard_learner_progress.py +++ b/tests/test_dashboard_learner_progress.py @@ -30,6 +30,10 @@ CONTRACT_ID = 101 PATH = f"/api/v1/analytics/organizations/{ORG_ID}/contracts/{CONTRACT_ID}/learner-progress" _AS_OF = datetime.datetime(2026, 9, 15, 6, 0) # noqa: DTZ001 - StarRocks returns naive UTC +# What the cluster answers NEEDS_ATTENTION_CUTOFF_QUERY with: 30 days before _AS_OF's +# date. Fixed, so the SQL these tests assert on is stable. +_CUTOFF = datetime.date(2026, 8, 16) +_CUTOFF_SQL = "'2026-08-16'" def _manager_header(organization_id=ORG_ID): @@ -85,6 +89,8 @@ def __init__( async def fetch_all(self, query, params=()): self.calls.append((query, params)) + if query == learner_queries.NEEDS_ATTENTION_CUTOFF_QUERY: + return [{"cutoff": _CUTOFF}] if "information_schema" in query: return [{"as_of": _AS_OF}] if query.startswith("SELECT 1 "): @@ -237,7 +243,7 @@ async def test_needs_attention_count_reported_from_the_count_query(app): count_query, _ = pool.count_call() assert ( "SUM(CASE WHEN FALSE AND (COALESCE(completion_status = 'not_started'" - " OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY), FALSE))" + f" OR last_active_on <= {_CUTOFF_SQL}, FALSE))" " THEN 1 ELSE 0 END) AS needs_attention_count" in count_query ) @@ -252,18 +258,16 @@ async def test_needs_attention_count_shares_the_response_filters(app): def _needs_attention_sql(cutoff): - # sqlite has no DATE_SUB/INTERVAL syntax, so swap in the one computed - # literal StarRocks would evaluate server-side. Every column, CASE branch - # and comparison operator below this is the real production string. - return learner_queries._NEEDS_ATTENTION.replace( # noqa: SLF001 - "DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY)", f"'{cutoff.isoformat()}'" - ) + # The production string, verbatim -- no substitution. The cutoff is + # resolved on the cluster and spliced as a date literal, which sqlite + # parses too, so these tests execute exactly what StarRocks would. + return learner_queries._needs_attention(cutoff) # noqa: SLF001 def test_needs_attention_boundary_is_computed_from_real_rows(): # test_needs_attention_count_reported_from_the_count_query pins the SQL # text; this actually runs learner_queries._COMPLETION_STATUS and - # ._NEEDS_ATTENTION against rows in sqlite, so a day-30 regression (or a + # ._needs_attention against rows in sqlite, so a day-30 regression (or a # reverted `<=`) fails here even though _FakePool never evaluates a WHERE # clause on its own. today = datetime.date.today() # noqa: DTZ011 - the boundary is date-only @@ -345,7 +349,7 @@ def test_needs_attention_is_two_valued_so_the_filter_partitions_rows(): # would fall out of `needs_attention=true` and `needs_attention=false` # alike while still counting toward total_count, and would serialize as # `needs_attention: null` on a row whose outcomes are shared. COALESCE in - # ._NEEDS_ATTENTION settles it as false; this pins that every shared row + # ._needs_attention settles it as false; this pins that every shared row # lands in exactly one direction of the filter. today = datetime.date.today() # noqa: DTZ011 - the boundary is date-only cutoff = today - datetime.timedelta(days=30) @@ -397,7 +401,7 @@ async def test_needs_attention_filter_is_consent_gated_in_both_directions(app): await _get(app, pool, params={"needs_attention": value}) expected = ( f"(FALSE AND {negate}(COALESCE(completion_status = 'not_started'" - " OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY), FALSE)))" + f" OR last_active_on <= {_CUTOFF_SQL}, FALSE)))" ) # The `FALSE AND` is the consent gate: a withheld row matches neither # direction, so the filter can't reveal the outcome it withholds. @@ -434,10 +438,70 @@ async def test_needs_attention_row_field_reuses_the_count_expression(app): await _get(app, pool) page_query = pool.page_call()[0] assert ( - f"CASE WHEN FALSE THEN {learner_queries._NEEDS_ATTENTION} END AS needs_attention" # noqa: SLF001 + f"CASE WHEN FALSE THEN {learner_queries._needs_attention(_CUTOFF)} END AS needs_attention" # noqa: SLF001 in page_query ) - assert learner_queries._NEEDS_ATTENTION in pool.count_call()[0] # noqa: SLF001 + assert learner_queries._needs_attention(_CUTOFF) in pool.count_call()[0] # noqa: SLF001 + + +async def test_cutoff_is_resolved_once_on_the_cluster_and_shared_by_both_statements(app): + # The page and the count are two round trips. With CURRENT_DATE() left in + # the SQL each would evaluate it separately, so a request straddling + # midnight in the cluster's timezone would filter the page on one cutoff + # and count on another -- an off-by-one between a row's needs_attention + # and needs_attention_count, and a total_count the page contradicts. + pool = _FakePool() + await _get(app, pool, params={"needs_attention": "true"}) + + queries = [query for query, _ in pool.calls] + assert queries.count(learner_queries.NEEDS_ATTENTION_CUTOFF_QUERY) == 1 + # Resolved before either statement is built, like as_of. + cutoff_at = queries.index(learner_queries.NEEDS_ATTENTION_CUTOFF_QUERY) + assert cutoff_at < queries.index(pool.page_call()[0]) + assert cutoff_at < queries.index(pool.count_call()[0]) + + for query in (pool.page_call()[0], pool.count_call()[0]): + assert "CURRENT_DATE" not in query + assert _CUTOFF_SQL in query + + +def test_cutoff_query_reads_no_table(): + # No FROM clause, so StarRocks answers it without touching storage. That + # is what makes the extra round trip per request cheap enough to prefer + # over caching a value whose whole purpose is to be correct at a boundary. + assert " FROM " not in learner_queries.NEEDS_ATTENTION_CUTOFF_QUERY + assert str(learner_queries.NEEDS_ATTENTION_QUIET_DAYS) in ( + learner_queries.NEEDS_ATTENTION_CUTOFF_QUERY + ) + + +@pytest.mark.parametrize( + "value", + [ + _CUTOFF, + # Which of these a DATE column arrives as depends on the driver and + # the StarRocks build, so all three normalize to the same literal + # rather than 500ing the endpoint on a type surprise. + datetime.datetime(2026, 8, 16, 9, 30), # noqa: DTZ001 - StarRocks returns naive + "2026-08-16", + ], +) +def test_cutoff_literal_normalizes_every_shape_the_driver_can_return(value): + # datetime is the one that matters: it subclasses date, so rendering its + # isoformat unchanged would emit a time component and change the + # comparison. It is truncated, not passed through. + assert learner_queries._date_literal(value) == _CUTOFF_SQL # noqa: SLF001 + + +@pytest.mark.parametrize( + ("value", "error"), + [(None, TypeError), (20260816, TypeError), ("16/08/2026", ValueError)], +) +def test_cutoff_literal_refuses_what_it_cannot_read_as_a_date(value, error): + # The cutoff is spliced, not bound, so nothing that isn't a real calendar + # date may reach the query text. + with pytest.raises(error): + learner_queries._date_literal(value) # noqa: SLF001 async def test_needs_attention_row_field_is_served_and_withheld_with_consent(app): @@ -541,14 +605,14 @@ def test_models_null_outcomes_a_row_carries_when_not_shared(): def test_every_outcome_column_is_consent_gated_in_the_query(): query = learner_queries.learner_progress( - learner_queries.ProgressFilters(organization_id=ORG_ID, contract_id=CONTRACT_ID) + learner_queries.ProgressFilters(organization_id=ORG_ID, contract_id=CONTRACT_ID), _CUTOFF ) for name in ("completion_status", "is_passing", "grade", "letter_grade", "last_active_on"): assert f"CASE WHEN FALSE THEN {name} END AS {name}" in query.page # needs_attention is derived, not selected, so the same gate wraps an # expression rather than a column name. assert ( - f"CASE WHEN FALSE THEN {learner_queries._NEEDS_ATTENTION} END AS needs_attention" # noqa: SLF001 + f"CASE WHEN FALSE THEN {learner_queries._needs_attention(_CUTOFF)} END AS needs_attention" # noqa: SLF001 in query.page )