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

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:

src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py

@@ -34,6 +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, 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
@@ -61,6 +70,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 +158,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:

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,16 +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).
-# 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.
-_NEEDS_ATTENTION = (
-    "completion_status = 'not_started'"
-    " OR last_active_on <= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY)"
+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), '')"
@@ -102,6 +172,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
@@ -127,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.
@@ -146,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)
@@ -168,6 +241,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 +262,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. 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 = (
@@ -195,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}"
     )

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,23 +90,47 @@ 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,
     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,
             contract_id=contract_id,
             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,
-        )
+        ),
+        cutoff,
     )
     # Freshness first, so a refresh landing mid-request labels newer rows with
     # the older as_of rather than the reverse.