diff --git a/openapi/specs/b2b_dashboard.yaml b/openapi/specs/b2b_dashboard.yaml index 2091fd7..c22e12c 100644 --- a/openapi/specs/b2b_dashboard.yaml +++ b/openapi/specs/b2b_dashboard.yaml @@ -649,13 +649,14 @@ components: in_progress: type: integer title: In Progress - description: Matching enrollments with a nonzero grade so far that isn't - yet passing, other than those already counted as certified or passed above. + description: Matching enrollments with a nonzero grade that isn't yet passing, + or with no grade yet but some activity in the course, other than those + already counted as certified or passed above. not_started: type: integer title: Not Started - description: Matching enrollments with no certificate and no grade recorded - yet. + description: Matching enrollments with no certificate, no grade, and no + activity in the course yet. type: object required: - certified @@ -673,13 +674,15 @@ components: enrollment falls into exactly one, in the order below (certificate beats - grade beats no grade), so summing the four plus ``outcomes_withheld_count`` + grade beats activity beats neither), so summing the four plus - always equals ``total_count``. An unrevoked certificate always wins even + ``outcomes_withheld_count`` always equals ``total_count``. An unrevoked - when the same enrollment also carries a passing or in-progress grade, + certificate always wins even when the same enrollment also carries a - which is why each field''s own description calls out what it excludes. + passing or in-progress grade, which is why each field''s own description + + calls out what it excludes. These counts are never suppressed for small cohorts, unlike the @@ -1561,8 +1564,9 @@ components: format: date - type: 'null' title: Last Active On - description: The last day the learner did anything in the course. Not available - yet, so always empty for now. + 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. type: object required: - learner_id @@ -1611,6 +1615,12 @@ components: $ref: '#/components/schemas/CompletionStatusCounts' description: How many of those enrollments are in each stage of completion. Enrollments with hidden progress aren't counted in any stage. + needs_attention_count: + type: integer + title: Needs Attention Count + description: 'How many of those enrollments need attention: the learner + never started, or their last recorded activity was at least 30 days ago. + Enrollments with hidden progress aren''t counted.' data: items: $ref: '#/components/schemas/LearnerProgress' @@ -1624,6 +1634,7 @@ components: - total_count - outcomes_withheld_count - completion_status_counts + - needs_attention_count - data title: LearnerProgressResponse description: 'The org envelope (``organization_id``, ``as_of``, ``total_count``, 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 fb00fc5..8e8fe94 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/learner_models.py @@ -20,14 +20,20 @@ everything in ``_OUTCOME_FIELDS`` is. - ``completion_status``: an unrevoked certificate is ``certified``. A revoked certificate doesn't count, and the status then follows the grade, so it can - read ``passed``, ``in_progress`` or ``not_started``. Until learner-grain - activity data lands, ``in_progress`` means a nonzero grade. -- ``last_active_on`` is NULL for every row until activity data lands, whatever - ``outcomes_shared`` says. + read ``passed``, ``in_progress`` or ``not_started``. ``in_progress`` means a + nonzero grade or any tracked activity. +- ``last_active_on`` is NULL, whatever ``outcomes_shared`` says, until the + learner has any tracked activity. - ``outcomes_withheld_count`` counts the rows in ``total_count`` whose ``outcomes_shared`` is false. ``completion_status_counts`` buckets the rest by status; the two together add up to ``total_count``, since ``CompletionStatus`` is exhaustive and its branches don't overlap. +- ``needs_attention_count`` overlaps ``completion_status_counts`` rather than + adding to it: a learner needs attention if they never started, or if + their last recorded activity was at least 30 days ago, so the same row + 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. """ from __future__ import annotations @@ -138,8 +144,8 @@ class LearnerProgress(BaseModel): ) last_active_on: datetime.date | None = Field( description=( - "The last day the learner did anything in the course. Not available yet, so always " - "empty for now." + f"The last day the learner did anything in the course. Empty if they haven't yet. " + f"{_HIDDEN}" ) ) @@ -157,10 +163,11 @@ class CompletionStatusCounts(BaseModel): Each field counts a disjoint slice of the matching enrollments: every enrollment falls into exactly one, in the order below (certificate beats - grade beats no grade), so summing the four plus ``outcomes_withheld_count`` - always equals ``total_count``. An unrevoked certificate always wins even - when the same enrollment also carries a passing or in-progress grade, - which is why each field's own description calls out what it excludes. + grade beats activity beats neither), so summing the four plus + ``outcomes_withheld_count`` always equals ``total_count``. An unrevoked + certificate always wins even when the same enrollment also carries a + passing or in-progress grade, which is why each field's own description + calls out what it excludes. These counts are never suppressed for small cohorts, unlike the ``cohort_policy``-gated aggregates elsewhere in b2b_analytics (e.g. @@ -180,12 +187,15 @@ class CompletionStatusCounts(BaseModel): ) in_progress: int = Field( description=( - "Matching enrollments with a nonzero grade so far that isn't yet passing, other " - "than those already counted as certified or passed above." + "Matching enrollments with a nonzero grade that isn't yet passing, or with no grade " + "yet but some activity in the course, other than those already counted as certified " + "or passed above." ) ) not_started: int = Field( - description="Matching enrollments with no certificate and no grade recorded yet." + description=( + "Matching enrollments with no certificate, no grade, and no activity in the course yet." + ) ) @@ -213,4 +223,11 @@ class LearnerProgressResponse(BaseModel): "hidden progress aren't counted in any stage." ) ) + needs_attention_count: int = Field( + description=( + "How many of those enrollments need attention: the learner never started, or their " + "last recorded activity was at least 30 days ago. Enrollments with hidden progress " + "aren't counted." + ) + ) data: list[LearnerProgress] = Field(description="This page of enrollments.") 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 754ea4d..48a963c 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py @@ -34,17 +34,28 @@ # Matches the b2b_learner_records tenant. An unrevoked certificate is certified # without requiring is_passing, since production has unrevoked certificates with -# is_passing false (ol-data-platform#2669). Until activity data lands, -# "in progress" can only mean a nonzero grade. +# is_passing false (ol-data-platform#2669). in_progress must match +# mv_b2b_learner.courses_in_progress: a nonzero grade or any tracked activity +# (ol-data-platform#2693). _COMPLETION_STATUS = ( "CASE" " WHEN certificate_is_revoked = FALSE THEN 'certified'" " WHEN is_passing = TRUE THEN 'passed'" - " WHEN grade_value > 0 THEN 'in_progress'" + " WHEN grade_value > 0 OR last_active_on IS NOT NULL THEN 'in_progress'" " ELSE 'not_started'" " 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)" +) + # 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), '')" @@ -72,6 +83,7 @@ "letter_grade", "certificate_issued_on", "certificate_is_revoked", + "last_active_on", ) @@ -127,7 +139,7 @@ def learner_progress(filters: ProgressFilters) -> ProgressQuery: " courserun_readable_id, courserun_title, courserun_start_on, courserun_end_on," " enrollment_created_on AS enrolled_on, enrollment_is_active, enrollment_mode," f" {_COMPLETION_STATUS} AS completion_status, is_passing, grade_value AS grade," - " letter_grade, certificate_issued_on, certificate_is_revoked" + " letter_grade, certificate_issued_on, certificate_is_revoked, last_active_on" f" FROM {table} WHERE {' AND '.join(scope)}" ) @@ -164,7 +176,6 @@ 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), - "NULL AS last_active_on", ] ) page = ( @@ -178,7 +189,9 @@ def learner_progress(filters: ProgressFilters) -> ProgressQuery: count = ( "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" {status_sums}," + f" SUM(CASE WHEN {shared} AND ({_NEEDS_ATTENTION}) THEN 1 ELSE 0 END)" + " AS needs_attention_count" f" FROM ({records}) records{where}" ) return ProgressQuery(page, count, tuple(params)) 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 9d0b197..e8423ff 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/routers/learners.py @@ -110,5 +110,6 @@ async def learner_progress( # noqa: PLR0913 passed=int(counts["passed"] or 0), certified=int(counts["certified"] or 0), ), + needs_attention_count=int(counts["needs_attention_count"] or 0), data=[LearnerProgress(**row) for row in rows], ) diff --git a/tests/test_dashboard_learner_progress.py b/tests/test_dashboard_learner_progress.py index 75a93c3..c1098a9 100644 --- a/tests/test_dashboard_learner_progress.py +++ b/tests/test_dashboard_learner_progress.py @@ -10,6 +10,7 @@ import datetime import json import re +import sqlite3 from unittest.mock import AsyncMock, patch import pytest @@ -75,6 +76,7 @@ def __init__( "in_progress": 0, "passed": 0, "certified": 0, + "needs_attention_count": 0, **(status_counts or {}), } self.contract_exists = contract_exists @@ -220,6 +222,118 @@ async def test_completion_status_counts_share_the_response_filters(app): ) +async def test_in_progress_also_counts_tracked_activity(app): + pool = _FakePool() + await _get(app, pool) + assert "grade_value > 0 OR last_active_on IS NOT NULL THEN 'in_progress'" in pool.page_call()[0] + + +async def test_needs_attention_count_reported_from_the_count_query(app): + pool = _FakePool(status_counts={"needs_attention_count": 7}) + response = await _get(app, pool) + + 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))" + " THEN 1 ELSE 0 END) AS needs_attention_count" in count_query + ) + + +async def test_needs_attention_count_shares_the_response_filters(app): + pool = _FakePool() + await _get(app, pool, params={"completion_status": ["passed"]}) + count_query, _ = pool.count_call() + # Same query, same WHERE clause as total_count and the status buckets. + assert count_query.count("WHERE") == 2 + assert "needs_attention_count" in count_query + + +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()}'" + ) + + +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 + # 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 + 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, None, (today - datetime.timedelta(days=29)).isoformat()), # active 29 days ago + (1, 0, None, cutoff.isoformat()), # active exactly 30 days ago + ], + ) + rows = conn.execute( + "SELECT completion_status," # noqa: S608 + f" ({needs_attention}) AS needs_attention FROM" + f" (SELECT *, {learner_queries._COMPLETION_STATUS} AS completion_status FROM enrollment)" # noqa: SLF001 + ).fetchall() + conn.close() + + assert 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 + ] + + +def test_needs_attention_count_respects_the_consent_gate(monkeypatch): + # The same rows, but through the full SUM(CASE WHEN shared AND (...)) + # aggregate, with consent fail-closed (the default, so every row's + # outcome -- including needs-attention -- is withheld) and fail-open. + 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, None, (today - datetime.timedelta(days=29)).isoformat()), # active 29 days ago + (1, 0, None, cutoff.isoformat()), # active exactly 30 days ago + ], + ) + + def count(shared): + query = ( + f"SELECT SUM(CASE WHEN {shared} AND ({needs_attention}) THEN 1 ELSE 0 END) FROM" # noqa: S608 + f" (SELECT *, {learner_queries._COMPLETION_STATUS} AS completion_status" # noqa: SLF001 + " FROM enrollment)" + ) + return conn.execute(query).fetchone()[0] + + assert type(settings)().consent_fail_open is False + assert count(learner_queries._outcomes_shared()) == 0 # noqa: SLF001 + + monkeypatch.setattr(settings, "consent_fail_open", True) + assert count(learner_queries._outcomes_shared()) == 2 # noqa: SLF001 + conn.close() + + 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 @@ -286,7 +400,7 @@ 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) ) - for name in ("completion_status", "is_passing", "grade", "letter_grade"): + 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