diff --git a/openapi/specs/b2b_dashboard.yaml b/openapi/specs/b2b_dashboard.yaml index fb04e3f..c0a0a61 100644 --- a/openapi/specs/b2b_dashboard.yaml +++ b/openapi/specs/b2b_dashboard.yaml @@ -635,6 +635,98 @@ paths: application/json: schema: $ref: '#/components/schemas/HTTPValidationError' + /api/v1/analytics/organizations/{organization_id}/needs-attention: + get: + tags: + - needs-attention + summary: Learners who may need a nudge, counted once each, per contract + operationId: organizations_needs_attention_retrieve + parameters: + - name: organization_id + in: path + required: true + schema: + type: string + title: Organization Id + - name: limit + in: query + required: false + schema: + type: integer + maximum: 1000 + minimum: 1 + default: 100 + title: Limit + - name: offset + in: query + required: false + schema: + type: integer + minimum: 0 + default: 0 + title: Offset + responses: + '200': + description: Successful Response + content: + application/json: + schema: + $ref: '#/components/schemas/OrgAnalyticsResponse_ContractNeedsAttention_' + '422': + description: Validation Error + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPValidationError' + /api/v1/analytics/organizations/{organization_id}/contracts/{contract_id}/needs-attention: + get: + tags: + - needs-attention + summary: Learners who may need a nudge, counted once each, per contract + operationId: contracts_needs_attention_retrieve + parameters: + - name: organization_id + in: path + required: true + schema: + type: string + title: Organization Id + - name: contract_id + in: path + required: true + schema: + type: integer + title: Contract Id + - name: limit + in: query + required: false + schema: + type: integer + maximum: 1000 + minimum: 1 + default: 100 + title: Limit + - name: offset + in: query + required: false + schema: + type: integer + minimum: 0 + default: 0 + title: Offset + responses: + '200': + description: Successful Response + content: + application/json: + schema: + $ref: '#/components/schemas/OrgAnalyticsResponse_ContractNeedsAttention_' + '422': + description: Validation Error + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPValidationError' /api/v1/analytics/admin/contract-health: get: tags: @@ -1293,6 +1385,94 @@ components: ``monthly_active_learners`` across contracts can exceed the org''s own figure. Activity totals, being sums of events, do add up.' + ContractNeedsAttention: + properties: + contract_id: + type: integer + title: Contract Id + description: The contract's ID in MITx Online. + learners_considered: + type: integer + title: Learners Considered + description: Learners with an active enrollment under the contract. Deactivated + enrollments, such as after unenrolling or a refund, are left out. When + too few learners are in the group, the whole row is withheld to avoid + identifying them. + learners_needing_attention: + anyOf: + - type: integer + - type: 'null' + title: Learners Needing Attention + description: 'Of those learners, how many may need a nudge: they never started + a course, or their last recorded activity was at least 30 days ago. A + learner counts once however many of the contract''s courses they are behind + in. Learners who haven''t agreed to share their progress aren''t counted. + Withheld when too few learners are in the group to report without identifying + them.' + learners_outcomes_withheld: + anyOf: + - type: integer + - type: 'null' + title: Learners Outcomes Withheld + description: Of those learners, how many are left out of the count above + because they haven't agreed to share their progress. Withheld when too + few learners are in the group to report without identifying them. + type: object + required: + - contract_id + - learners_considered + - learners_needing_attention + - learners_outcomes_withheld + title: ContractNeedsAttention + description: 'Distinct learners needing attention — grain: org x contract. + + + The one model here that does not mirror a materialized view. It is computed + + at query time by ``learner_queries.needs_attention_aggregate`` over + + ``b2b_learner_records.mv_b2b_learner_enrollment``, which is what lets the + + KPI tile and the learner directory beneath it share a single + + needs-attention expression and a single per-request cutoff. + + + A column on ``mv_b2b_contract_utilization`` was the other candidate and was + + not chosen (decided 2026-10-02). It would have restated the 30-day rule and + + the completion-status CASE in dbt — two repos, two languages, no test that + + can see both — and frozen the cutoff at MV-refresh time, so the tile and the + + directory could disagree about the same learner for up to a refresh + + interval. The rule has already changed once since it was written. + + + Served from its own endpoint rather than folded into ``ContractUtilization`` + + because the MV behind it refreshes on its own schedule. One ``as_of`` per + + section is exactly what stops a lagging view from making another section + + look fresher than it is, so a client renders this tile''s freshness from + + this endpoint''s envelope, not from contract-utilization''s. + + + ``learners_considered`` is the primary cohort and gates the row. The other + + two counts are secondary and nulled on their own terms. As on + + ``ContractUtilization``, a published cohort beside a published subset of it + + still leaves the complement derivable (42 considered and 40 needing + + attention names 2 learners); that is the general across-column gap tracked + + separately, not something specific to this model.' ContractUtilization: properties: organization_key: @@ -2131,6 +2311,32 @@ components: - total_count - data title: OrgAnalyticsResponse[ContractMonthlyEngagementTrend] + OrgAnalyticsResponse_ContractNeedsAttention_: + properties: + organization_id: + type: string + title: Organization Id + as_of: + anyOf: + - type: string + format: date-time + - type: 'null' + title: As Of + total_count: + type: integer + title: Total Count + data: + items: + $ref: '#/components/schemas/ContractNeedsAttention' + type: array + title: Data + type: object + required: + - organization_id + - as_of + - total_count + - data + title: OrgAnalyticsResponse[ContractNeedsAttention] OrgAnalyticsResponse_ContractUtilization_: properties: organization_id: diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/app.py b/src/ol_analytics_api/tenants/b2b_dashboard/app.py index 5a90b59..f4af261 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/app.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/app.py @@ -34,7 +34,13 @@ from ol_analytics_api.core.errors import add_shared_error_handlers from ol_analytics_api.core.health import register_readiness_check from ol_analytics_api.tenants.b2b_dashboard.mitxonline_client import mitxonline_client -from ol_analytics_api.tenants.b2b_dashboard.routers import admin, contracts, learners, organizations +from ol_analytics_api.tenants.b2b_dashboard.routers import ( + admin, + contracts, + learners, + needs_attention, + organizations, +) # Names this tenant's readiness sub-path (/health/readiness/b2b_dashboard/). TENANT_NAME = "b2b_dashboard" @@ -69,6 +75,10 @@ def create_app() -> FastAPI: # literal segments, so the two cannot shadow each other either way. app.include_router(contracts.router) app.include_router(learners.router) + # Both of its routes hang off /organizations/{id}, one of them under + # /contracts/{id}, so it carries the org gate for both and adds the + # contract gate on the nested route alone. + app.include_router(needs_attention.router) app.include_router(admin.router) # Turn a saturated shared StarRocks pool into a fast 503 for this tenant's 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 0ed0e7c..5a4e251 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py @@ -5,6 +5,14 @@ suppress every one of them. Org-manager authorization and the contract gate still apply (routers/learners.py). +``needs_attention_aggregate`` is the one exception, and it goes the other way: +it returns no learner rows at all, only per-contract distinct-learner counts, +so it DOES go through that chokepoint and is floored like every other +aggregate in this tenant. It lives in this module rather than with the +materialized-view endpoints so that it can share ``_needs_attention`` and +``_COMPLETION_STATUS`` with the row-level queries above -- one rule and one +cutoff for the KPI tile and the learner directory alike. + Every query is a fixed template. The identifiers are this module's constants plus the validated schema name, and every caller-supplied value is a bound parameter. The only per-request variation is which of a fixed set of @@ -289,6 +297,102 @@ def learner_progress(filters: ProgressFilters, cutoff: datetime.date) -> Progres return ProgressQuery(page, count, tuple(params)) +@dataclass(frozen=True) +class AggregateQuery: + """``page`` takes ``params`` plus the anonymization floor, LIMIT and + OFFSET; ``count`` takes ``params`` plus the floor.""" + + page: str + count: str + params: tuple[Any, ...] + + +def needs_attention_aggregate( + organization_id: str, contract_id: int | None, cutoff: datetime.date +) -> AggregateQuery: + """Distinct learners needing attention, grouped by contract. + + The aggregate behind the dashboard's needs-attention KPI tile. It reads the + same learner-grain MV as ``learner_progress`` and reuses the same + ``_needs_attention`` expression against the same per-request cutoff, so the + tile and the directory beneath it cannot disagree about which learners are + quiet -- not even for a learner whose 30th quiet day is today. + + ``COUNT(DISTINCT CASE WHEN ... THEN learner_id END)`` is the point of the + whole query. ``learner_progress``'s ``needs_attention_count`` is a + ``SUM(CASE WHEN ...)`` over a learner x course-run grain, so a learner + stale in three of the contract's courses counts three times. That cannot + back a tile sitting beside ``active_learners``, which is itself a + distinct-learner count: two adjacent tiles would read as learner counts + while using different units. Here that learner counts once. + + ``contract_id`` narrows to one contract for the contract-scoped route; + ``None`` returns one row per contract the organization holds, which is what + the org-wide analytics view renders a card group from. + """ + table = f"{validate_sql_identifier(settings.learner_records_schema)}.{ENROLLMENT_MV}" + # The org predicate stays next to the contract one, as in learner_progress: + # contract ids are globally unique but not secret. + scope = ["sso_organization_id = %s"] + params: list[Any] = [organization_id] + if contract_id is not None: + scope.append("contract_id = %s") + params.append(contract_id) + # Active enrollments only, with no caller override, and that is what makes + # the tile and the directory count one population: learner_progress's + # `include_inactive` defaults to false, so a manager clicking through from + # the tile lands on exactly these learners. + scope.append("enrollment_is_active = TRUE") + records = ( + "SELECT user_global_id AS learner_id, contract_id," # noqa: S608 + f" {_COMPLETION_STATUS} AS completion_status, last_active_on" + f" FROM {table} WHERE {' AND '.join(scope)}" + ) + + shared = _outcomes_shared() + needs_attention = _needs_attention(cutoff) + # learners_considered is deliberately NOT consent-gated and the other two + # are, which mirrors learner_progress exactly: being enrolled is not an + # outcome, so it is disclosed, while anything about a learner's progress is + # gated. So the three do not sum -- a withheld learner is counted in the + # first and the third, never the second -- and the third is what stops a + # fail-closed stack from reading as "nobody needs attention". + aggregates = ( + "COUNT(DISTINCT learner_id) AS learners_considered," + f" COUNT(DISTINCT CASE WHEN {shared} AND ({needs_attention}) THEN learner_id END)" + " AS learners_needing_attention," + f" COUNT(DISTINCT CASE WHEN NOT {shared} THEN learner_id END)" + " AS learners_outcomes_withheld" + ) + # The primary-cohort gate, applied to the page AND the count, written once + # so the two cannot drift. suppress_small_cohorts would drop a sub-floor row + # anyway, but dropping it in Python AFTER the LIMIT is not equivalent: + # + # - A page whose rows are all sub-floor comes back empty beside a positive + # total_count, and the visible contracts behind it are reachable only by + # guessing an offset. OrgAnalyticsResponse tells clients to compare + # total_count against len(data) plus offset, which that breaks. + # - Varying the offset and watching which contracts surface reveals how + # many suppressed ones precede each visible one -- exactly the count of + # sub-floor cohorts that gating the count query exists to withhold. + # + # Gating in SQL is what makes the page and the count describe one set. + cohort_gate = "HAVING COUNT(DISTINCT learner_id) >= %s" + # Grouping by the grain key makes it unique per row, so it is also a + # deterministic ORDER BY for LIMIT/OFFSET paging. + page = ( + f"SELECT contract_id, {aggregates} FROM ({records}) records" # noqa: S608 + f" GROUP BY contract_id {cohort_gate}" + " ORDER BY contract_id LIMIT %s OFFSET %s" + ) + count = ( + "SELECT COUNT(*) AS total_count FROM (" # noqa: S608 + f"SELECT contract_id FROM ({records}) records" + f" GROUP BY contract_id {cohort_gate}) gated" + ) + return AggregateQuery(page, count, tuple(params)) + + @dataclass(frozen=True) class CourseRunsQuery: """``params`` binds ``count``; ``page`` takes ``params`` plus LIMIT and OFFSET.""" diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/models.py b/src/ol_analytics_api/tenants/b2b_dashboard/models.py index 2f6ca58..9edb317 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/models.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/models.py @@ -1,10 +1,16 @@ -"""Response schemas mirroring the 6 StarRocks B2B analytics materialized views. +"""Response schemas for this tenant's aggregate endpoints. -Column sets match the dbt models in `ol-data-platform`'s +Six of them mirror a StarRocks B2B analytics materialized view, and their +column sets match the dbt models in `ol-data-platform`'s `models/b2b_analytics/*.sql` (mitodl/ol-data-platform PR #2329) exactly. These are plain SQLModel (Pydantic) schemas, not ORM tables — StarRocks-side schema is owned by dbt, not by this service. +``ContractNeedsAttention`` is the exception: it is aggregated at query time in +this service rather than by dbt, for reasons its own docstring gives. What +makes it belong in this module is the ``cohort_policy`` below, not the MV it +doesn't have. + Every row model declares a ``cohort_policy`` (see core.anonymization): the distinct-entity counts subject to the k-anonymity floor and the derived values computed over them. The response layer nulls sub-floor secondary @@ -134,6 +140,64 @@ class ContractUtilization(SQLModel): ) +class ContractNeedsAttention(SQLModel): + """Distinct learners needing attention — grain: org x contract. + + The one model here that does not mirror a materialized view. It is computed + at query time by ``learner_queries.needs_attention_aggregate`` over + ``b2b_learner_records.mv_b2b_learner_enrollment``, which is what lets the + KPI tile and the learner directory beneath it share a single + needs-attention expression and a single per-request cutoff. + + A column on ``mv_b2b_contract_utilization`` was the other candidate and was + not chosen (decided 2026-10-02). It would have restated the 30-day rule and + the completion-status CASE in dbt — two repos, two languages, no test that + can see both — and frozen the cutoff at MV-refresh time, so the tile and the + directory could disagree about the same learner for up to a refresh + interval. The rule has already changed once since it was written. + + Served from its own endpoint rather than folded into ``ContractUtilization`` + because the MV behind it refreshes on its own schedule. One ``as_of`` per + section is exactly what stops a lagging view from making another section + look fresher than it is, so a client renders this tile's freshness from + this endpoint's envelope, not from contract-utilization's. + + ``learners_considered`` is the primary cohort and gates the row. The other + two counts are secondary and nulled on their own terms. As on + ``ContractUtilization``, a published cohort beside a published subset of it + still leaves the complement derivable (42 considered and 40 needing + attention names 2 learners); that is the general across-column gap tracked + separately, not something specific to this model. + """ + + cohort_policy: ClassVar[CohortPolicy] = CohortPolicy( + primary="learners_considered", + secondary=("learners_needing_attention", "learners_outcomes_withheld"), + ) + + contract_id: int = Field(description="The contract's ID in MITx Online.") + learners_considered: int = Field( + description=( + "Learners with an active enrollment under the contract. Deactivated enrollments, " + f"such as after unenrolling or a refund, are left out. {_ROW_WITHHELD}" + ) + ) + learners_needing_attention: int | None = Field( + description=( + "Of those learners, how many may need a nudge: they never started a course, or " + "their last recorded activity was at least 30 days ago. A learner counts once " + "however many of the contract's courses they are behind in. Learners who haven't " + f"agreed to share their progress aren't counted. {_WITHHELD}" + ) + ) + learners_outcomes_withheld: int | None = Field( + description=( + "Of those learners, how many are left out of the count above because they haven't " + f"agreed to share their progress. {_WITHHELD}" + ) + ) + + class EnrollmentCompletionFunnel(SQLModel): """mv_b2b_enrollment_completion_funnel — grain: org x contract x course_run. diff --git a/src/ol_analytics_api/tenants/b2b_dashboard/routers/needs_attention.py b/src/ol_analytics_api/tenants/b2b_dashboard/routers/needs_attention.py new file mode 100644 index 0000000..dff6120 --- /dev/null +++ b/src/ol_analytics_api/tenants/b2b_dashboard/routers/needs_attention.py @@ -0,0 +1,128 @@ +"""Distinct-learner needs-attention counts, at org x contract grain. + +Backs the needs-attention KPI tile on MIT Learn's B2B analytics page, beside +Seat utilization, Active learners and Completion rate. Those three come from +``mv_b2b_contract_utilization``; this one is aggregated here instead, over the +learner-grain MV the learner directory reads, so that the tile and the +directory share one needs-attention rule and one cutoff. See +``models.ContractNeedsAttention`` for why that was preferred to a dbt column. + +Two routes, because the dashboard renders a card group per contract on both +views: the org route returns one row per contract the organization holds, and +the contract route returns the single row for one contract. + +This is an aggregate, so unlike the learner-progress endpoint in the same +tenant it goes through core/db/query.py's anonymization chokepoint and carries +the k-anonymity floor. It has to: an unfloored small count sitting beside a +suppressed "—" in the next tile discloses by juxtaposition exactly what the +floor is there to hide. + +Mounted at /api/v1/analytics by tenants/b2b_dashboard/app.py. +""" + +from __future__ import annotations + +from typing import Annotated + +from fastapi import APIRouter, Depends + +from ol_analytics_api.core.db.client import starrocks_pool +from ol_analytics_api.core.db.query import fetch_and_suppress, fetch_visible_count +from ol_analytics_api.core.db.refresh_metadata import latest_refresh_timestamp +from ol_analytics_api.tenants.b2b_dashboard import learner_queries +from ol_analytics_api.tenants.b2b_dashboard.auth import ( + require_contract_in_org, + require_org_manager, +) +from ol_analytics_api.tenants.b2b_dashboard.config import settings +from ol_analytics_api.tenants.b2b_dashboard.models import ( + ContractNeedsAttention, + OrgAnalyticsResponse, +) +from ol_analytics_api.tenants.b2b_dashboard.pagination import Pagination, pagination + +router = APIRouter( + prefix="/organizations/{organization_id}", + tags=["needs-attention"], + dependencies=[Depends(require_org_manager)], +) + +_SUMMARY = "Learners who may need a nudge, counted once each, per contract" + + +async def _respond( + organization_id: str, contract_id: int | None, page: Pagination +) -> OrgAnalyticsResponse[ContractNeedsAttention]: + """Resolve the cutoff, run the aggregate and its gated count, suppress, wrap. + + The cutoff is read from the cluster once per request and never cached, for + the reason learner-progress does the same: the page and the count are two + round trips, so leaving ``CURRENT_DATE()`` in the SQL would let them + evaluate it either side of midnight and report a count the rows contradict. + A cached cutoff would be wrong for exactly as long as the cache held it, + which is worse than the bug — the whole value of the rule is being right at + a date boundary. Resolving it here rather than in the query builder also + means this endpoint and learner-progress agree within a request. + """ + cutoff = (await starrocks_pool.fetch_all(learner_queries.NEEDS_ATTENTION_CUTOFF_QUERY))[0][ + "cutoff" + ] + query = learner_queries.needs_attention_aggregate(organization_id, contract_id, cutoff) + # Freshness first, so a refresh landing mid-request labels newer rows with + # the older as_of rather than the reverse. This is the learner-enrollment + # MV's own refresh time, not contract-utilization's: a client showing this + # tile beside that endpoint's three must read each one's own as_of. + as_of = await latest_refresh_timestamp( + settings.learner_records_schema, learner_queries.ENROLLMENT_MV + ) + # The floor is bound twice on purpose: once into the page query's HAVING, so + # the page and total_count describe the same gated set, and once for + # fetch_and_suppress, which still nulls the sub-floor secondary counts + # within a surviving row. + rows = await fetch_and_suppress( + query.page, + (*query.params, settings.anonymization_floor, page.limit, page.offset), + ContractNeedsAttention, + settings.anonymization_floor, + ) + return OrgAnalyticsResponse( + organization_id=organization_id, + as_of=as_of, + total_count=await fetch_visible_count( + query.count, (*query.params, settings.anonymization_floor) + ), + data=rows, + ) + + +@router.get( + "/needs-attention", + response_model=OrgAnalyticsResponse[ContractNeedsAttention], + name="organization_needs_attention", + operation_id="organizations_needs_attention_retrieve", + summary=_SUMMARY, +) +async def organization_needs_attention( + *, organization_id: str, page: Annotated[Pagination, Depends(pagination)] +) -> OrgAnalyticsResponse[ContractNeedsAttention]: + return await _respond(organization_id, None, page) + + +@router.get( + "/contracts/{contract_id}/needs-attention", + response_model=OrgAnalyticsResponse[ContractNeedsAttention], + name="contract_needs_attention", + operation_id="contracts_needs_attention_retrieve", + summary=_SUMMARY, + # The org-manager gate on the router runs first, so a caller who manages no + # part of this org never reaches the probe that would tell them whether a + # given contract exists. + dependencies=[Depends(require_contract_in_org)], +) +async def contract_needs_attention( + *, + organization_id: str, + contract_id: int, + page: Annotated[Pagination, Depends(pagination)], +) -> OrgAnalyticsResponse[ContractNeedsAttention]: + return await _respond(organization_id, contract_id, page) diff --git a/tests/test_dashboard_needs_attention.py b/tests/test_dashboard_needs_attention.py new file mode 100644 index 0000000..555c8c5 --- /dev/null +++ b/tests/test_dashboard_needs_attention.py @@ -0,0 +1,537 @@ +"""Tests for the b2b_dashboard needs-attention aggregate endpoints. + +Two layers, and the split is deliberate. The sqlite tests EXECUTE the +production query strings — the real ``needs_attention_aggregate`` output, not a +rewritten copy — against rows in an in-memory database, because what this +endpoint exists for is a value property: one learner counts once however many +of a contract's courses they have gone quiet in. Asserting on generated SQL +text can never show that. The ASGI tests then pin the envelope, the scoping, +the suppression and the auth gates, stubbing the pool. + +The sqlite databases are ATTACHed under the schema name the query names, so +the production string runs verbatim rather than being edited to point at a bare +table. ``%s`` becomes ``?`` because that is the driver's placeholder dialect, +not part of what is under test. +""" + +import base64 +import datetime +import json +import sqlite3 +from unittest.mock import AsyncMock, patch + +import pytest +from httpx import ASGITransport, AsyncClient + +from ol_analytics_api.core.db.identifiers import validate_sql_identifier +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.config import settings +from ol_analytics_api.tenants.b2b_dashboard.models import ContractNeedsAttention + +ORG_ID = "11111111-1111-1111-1111-111111111111" +CONTRACT_ID = 101 +ORG_PATH = f"/api/v1/analytics/organizations/{ORG_ID}/needs-attention" +CONTRACT_PATH = f"/api/v1/analytics/organizations/{ORG_ID}/contracts/{CONTRACT_ID}/needs-attention" +_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) +# Bound into the page query's HAVING by the tests whose subject is the +# needs-attention rule rather than the k-anonymity floor: their fixtures are a +# handful of learners on purpose, and the real floor would gate them all out. +# The two tests that ARE about the floor bind it explicitly instead. +_NO_FLOOR = 1 + +# The MV columns the aggregate's inner select reads. Same shape as the +# learner-progress sqlite fixtures, plus the scoping and grain columns this +# query groups and filters on. +_MV_COLUMNS = ( + "sso_organization_id TEXT", + "contract_id INTEGER", + "user_global_id TEXT", + "enrollment_is_active INTEGER", + "certificate_is_revoked INTEGER", + "is_passing INTEGER", + "grade_value REAL", + "last_active_on TEXT", +) + + +def _enrollment( # noqa: PLR0913 + learner, + *, + contract_id=CONTRACT_ID, + organization_id=ORG_ID, + active=1, + revoked=1, + passing=0, + grade=None, + last_active_on=None, +): + """One row of mv_b2b_learner_enrollment. + + ``revoked=1`` is the default because _COMPLETION_STATUS reads an + *unrevoked* certificate as certified; 1 keeps a row out of that branch so + the grade and activity columns decide its status, which is what these + tests vary. + """ + return ( + organization_id, + contract_id, + learner, + active, + revoked, + passing, + grade, + last_active_on, + ) + + +def _db(rows): + """An in-memory stand-in for the learner-records schema, ATTACHed under the + name the production query actually uses, so that query needs no rewriting.""" + conn = sqlite3.connect(":memory:") + schema = validate_sql_identifier(settings.learner_records_schema) + conn.execute(f"ATTACH ':memory:' AS {schema}") + conn.execute( + f"CREATE TABLE {schema}.{learner_queries.ENROLLMENT_MV} ({', '.join(_MV_COLUMNS)})" + ) + conn.executemany( + f"INSERT INTO {schema}.{learner_queries.ENROLLMENT_MV} VALUES " # noqa: S608 + f"({', '.join('?' * len(_MV_COLUMNS))})", + rows, + ) + return conn + + +def _run(conn, query, params): + """Execute a production query string, translating only the placeholder + dialect (``%s`` -> ``?``).""" + return conn.execute(query.replace("%s", "?"), params) + + +@pytest.fixture +def _fail_open(monkeypatch): + """Consent fail-open, which is what every deployed stack sets. The code + default is fail-closed, under which every outcome -- needing attention + included -- is withheld and the count is 0 everywhere; the consent test + below pins both.""" + monkeypatch.setattr(settings, "consent_fail_open", True) + + +@pytest.mark.usefixtures("_fail_open") +def test_counts_each_learner_once_however_many_courses_they_are_quiet_in(): + # The defect this endpoint exists to fix. learner-progress's + # needs_attention_count is a SUM over a learner x course-run grain, so one + # learner stale in three of the contract's courses reads as 3 -- which + # cannot sit beside active_learners, a distinct-learner count. Executed + # against real rows, not asserted on SQL text, because the units are the + # whole point. + quiet = (_CUTOFF - datetime.timedelta(days=1)).isoformat() + rows = [ + # One learner, quiet in three of the contract's course runs. + *(_enrollment("learner-a", grade=0.4, last_active_on=quiet) for _ in range(3)), + # A second learner, quiet in one. + _enrollment("learner-b", grade=0.4, last_active_on=quiet), + # A third, active yesterday in two runs: not quiet in either. + *( + _enrollment( + "learner-c", + grade=0.4, + last_active_on=( + datetime.date(2026, 9, 15) - datetime.timedelta(days=1) + ).isoformat(), + ) + for _ in range(2) + ), + ] + conn = _db(rows) + query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) + [row] = _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)).fetchall() + + # 6 enrollments, 3 learners, 2 of them needing attention. + assert row == (CONTRACT_ID, 3, 2, 0) + + # And the same rows through learner-progress's enrollment-grain count, + # built from the production expression rather than a copy of it, to show + # the two really do disagree and by how much. 4 enrollments, 2 learners. + needs_attention = learner_queries._needs_attention(_CUTOFF) # noqa: SLF001 + enrollment_grain = _run( + conn, + "SELECT SUM(CASE WHEN TRUE AND" # noqa: S608 + f" ({needs_attention}) THEN 1 ELSE 0 END) FROM (SELECT *," + f" {learner_queries._COMPLETION_STATUS} AS completion_status" # noqa: SLF001 + f" FROM {settings.learner_records_schema}.{learner_queries.ENROLLMENT_MV})", + (), + ).fetchone()[0] + conn.close() + assert enrollment_grain == 4 + assert enrollment_grain != row[2] + + +@pytest.mark.usefixtures("_fail_open") +def test_projects_exactly_the_model_fields_in_order(): + # The hand-built analogue of test_column_contract's + # test_query_projects_exactly_model_fields. This query is not built by + # build_select, so nothing derives its projection from the model: a + # mistyped alias would surface only as a TypeError constructing + # ContractNeedsAttention on a live request. Executing it and reading the + # cursor's column names catches that, and proves the SQL parses. + conn = _db([_enrollment("learner-a")]) + query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) + cursor = _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)) + columns = [description[0] for description in cursor.description] + conn.close() + + assert columns == list(ContractNeedsAttention.model_fields) + + +def test_consent_gate_moves_learners_between_the_counts(monkeypatch): + # learners_considered is not consent-gated and the other two are, mirroring + # learner-progress: being enrolled is not an outcome, so it is disclosed, + # while anything about progress is gated. So a withheld learner is counted + # in the first and the third, never the second -- and the three never sum. + quiet = (_CUTOFF - datetime.timedelta(days=1)).isoformat() + conn = _db( + [ + _enrollment("learner-a", grade=0.4, last_active_on=quiet), + _enrollment("learner-b", grade=0.4, last_active_on=quiet), + ] + ) + + def aggregate(): + query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) + return _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)).fetchall() + + # The code default fails closed, so no learner's quietness is disclosed and + # both are reported as withheld instead. + assert type(settings)().consent_fail_open is False + assert aggregate() == [(CONTRACT_ID, 2, 0, 2)] + + monkeypatch.setattr(settings, "consent_fail_open", True) + assert aggregate() == [(CONTRACT_ID, 2, 2, 0)] + conn.close() + + +@pytest.mark.usefixtures("_fail_open") +def test_boundary_is_the_same_day_the_learner_directory_uses(): + # The tile and the directory must agree about a learner whose 30th quiet + # day is today, which is the reason this aggregate reuses + # ._needs_attention against a per-request cutoff instead of restating the + # rule. Day 30 counts ("at least 30 days ago" includes the 30th); day 29 + # does not. + conn = _db( + [ + _enrollment("day-30", grade=0.4, last_active_on=_CUTOFF.isoformat()), + _enrollment( + "day-29", + grade=0.4, + last_active_on=(_CUTOFF + datetime.timedelta(days=1)).isoformat(), + ), + # Never started: quiet by definition, with no timestamp involved. + _enrollment("never-started"), + # A grade but no tracked activity. The staleness comparison is NULL + # for this row; ._needs_attention's COALESCE settles it as "no", so + # it is counted in neither direction rather than vanishing. + _enrollment("graded-no-activity", grade=0.4), + ] + ) + query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) + [row] = _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)).fetchall() + conn.close() + + assert row == (CONTRACT_ID, 4, 2, 0) + + +@pytest.mark.usefixtures("_fail_open") +def test_inactive_enrollments_are_left_out_like_the_directory_default(): + # learner_progress's include_inactive defaults to false, so a manager + # clicking through from this tile sees only active enrollments. The tile + # must count that same population or it sends them to a shorter list. + quiet = (_CUTOFF - datetime.timedelta(days=1)).isoformat() + conn = _db( + [ + _enrollment("active", grade=0.4, last_active_on=quiet), + _enrollment("unenrolled", active=0, grade=0.4, last_active_on=quiet), + ] + ) + query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) + [row] = _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)).fetchall() + conn.close() + + assert row == (CONTRACT_ID, 1, 1, 0) + + +@pytest.mark.usefixtures("_fail_open") +def test_org_scope_returns_one_row_per_contract_and_never_crosses_orgs(): + quiet = (_CUTOFF - datetime.timedelta(days=1)).isoformat() + other_org = "22222222-2222-2222-2222-222222222222" + conn = _db( + [ + _enrollment("learner-a", contract_id=101, grade=0.4, last_active_on=quiet), + _enrollment("learner-b", contract_id=102, grade=0.4, last_active_on=quiet), + _enrollment("learner-b", contract_id=102), + # Same contract id under a different org. Contract ids are globally + # unique but not secret, so the org predicate is what keeps this + # out -- not the absence of a collision. + _enrollment("intruder", contract_id=101, organization_id=other_org), + ] + ) + query = learner_queries.needs_attention_aggregate(ORG_ID, None, _CUTOFF) + rows = _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)).fetchall() + conn.close() + + assert rows == [(101, 1, 1, 0), (102, 1, 1, 0)] + + +@pytest.mark.usefixtures("_fail_open") +def test_page_and_count_describe_the_same_gated_set(): + """Regression: the page used to be ungated while total_count was gated. + + Two sub-floor contracts sorting ahead of a visible one made the first page + come back empty beside a positive total_count, leaving the visible contract + reachable only by guessing an offset -- and offset-probing then revealed how + many suppressed contracts preceded it, which is the figure gating the count + exists to withhold. Both now carry the same HAVING. + """ + rows = [ + # 101 and 102 are sub-floor and sort first; 103 is visible. + *(_enrollment(f"l101-{n}", contract_id=101) for n in range(2)), + *(_enrollment(f"l102-{n}", contract_id=102) for n in range(2)), + *(_enrollment(f"l103-{n}", contract_id=103) for n in range(6)), + ] + conn = _db(rows) + query = learner_queries.needs_attention_aggregate(ORG_ID, None, _CUTOFF) + floor = settings.anonymization_floor + first_page = _run(conn, query.page, (*query.params, floor, 2, 0)).fetchall() + [(total,)] = _run(conn, query.count, (*query.params, floor)).fetchall() + conn.close() + + # The visible contract is on the first page, not behind two hidden ones. + assert [row[0] for row in first_page] == [103] + assert total == 1 + assert len(first_page) == total + + +@pytest.mark.usefixtures("_fail_open") +def test_count_query_applies_the_primary_cohort_floor(): + # suppress_small_cohorts drops rows whose learners_considered is below the + # floor after the query returns, so the count has to apply the same gate in + # SQL. An ungated COUNT would exceed anything paging can reach, and the + # difference would tell the caller how many sub-floor contracts their org + # has -- the disclosure the floor exists to prevent. + conn = _db( + [ + *(_enrollment(f"big-{n}", contract_id=101) for n in range(5)), + *(_enrollment(f"small-{n}", contract_id=102) for n in range(2)), + ] + ) + query = learner_queries.needs_attention_aggregate(ORG_ID, None, _CUTOFF) + page = _run(conn, query.page, (*query.params, 5, 100, 0)).fetchall() + [(total,)] = _run(conn, query.count, (*query.params, 5)).fetchall() + conn.close() + + # The sub-floor contract is gated out of both, so neither can disclose how + # many of them the org has. + assert [row[0] for row in page] == [101] + assert total == 1 + + +# --- ASGI layer ----------------------------------------------------------- + + +def _manager_header(organization_id=ORG_ID): + claims = {"sub": "kc-uuid-1", "organization": {"an-alias": {"id": organization_id}}} + return base64.b64encode(json.dumps(claims).encode()).decode() + + +class _FakePool: + """Answers the cutoff probe, the as_of probe, the contract gate, the gated + count and the aggregate itself, recording every call.""" + + def __init__(self, rows=(), total_count=0, *, contract_exists=True): + self.rows = list(rows) + self.total_count = total_count + self.contract_exists = contract_exists + self.calls = [] + + 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 "): + return [{"1": 1}] if self.contract_exists else [] + if "COUNT(*)" in query: + return [{"total_count": self.total_count}] + return self.rows + + def page_call(self): + return next(call for call in self.calls if call[0].endswith("LIMIT %s OFFSET %s")) + + def count_call(self): + return next(call for call in self.calls if "COUNT(*)" in call[0]) + + +def _row(**overrides): + return { + "contract_id": CONTRACT_ID, + "learners_considered": 42, + "learners_needing_attention": 11, + "learners_outcomes_withheld": 0, + **overrides, + } + + +@pytest.fixture +def app(): + return create_app() + + +@pytest.fixture(autouse=True) +def _clear_as_of_cache(): + _clear_cache() + yield + _clear_cache() + + +async def _get(app, pool, path=CONTRACT_PATH, *, is_manager=True, params=None): + with ( + patch("ol_analytics_api.core.db.client.starrocks_pool.fetch_all", new=pool.fetch_all), + patch( + "ol_analytics_api.tenants.b2b_dashboard.auth.mitxonline_client.is_org_manager", + new=AsyncMock(return_value=is_manager), + ), + ): + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + return await client.get(path, params=params, headers={"X-Userinfo": _manager_header()}) + + +async def test_envelope_carries_the_learner_mvs_own_freshness(app): + pool = _FakePool(rows=[_row()], total_count=1) + response = await _get(app, pool) + + assert response.status_code == 200 + body = response.json() + assert body["organization_id"] == ORG_ID + assert body["as_of"] == "2026-09-15T06:00:00" + assert body["total_count"] == 1 + assert body["data"] == [_row()] + # as_of is read from the learner-records schema, not the aggregate one. A + # client showing this tile beside contract-utilization's three must read + # each endpoint's own freshness; that is what the per-section as_of is for. + [as_of_params] = [params for query, params in pool.calls if "information_schema" in query] + assert as_of_params == (settings.learner_records_schema, learner_queries.ENROLLMENT_MV) + + +async def test_org_route_binds_the_org_and_the_contract_route_binds_both(app): + pool = _FakePool() + await _get(app, pool, path=ORG_PATH) + assert pool.page_call()[1] == (ORG_ID, settings.anonymization_floor, 100, 0) + assert pool.count_call()[1] == (ORG_ID, settings.anonymization_floor) + assert "contract_id = %s" not in pool.page_call()[0] + + pool = _FakePool() + await _get(app, pool) + assert pool.page_call()[1] == (ORG_ID, CONTRACT_ID, settings.anonymization_floor, 100, 0) + assert pool.count_call()[1] == (ORG_ID, CONTRACT_ID, settings.anonymization_floor) + # The org predicate is never dropped when the contract one is added. + assert "sso_organization_id = %s AND contract_id = %s" in pool.page_call()[0] + + +async def test_sub_floor_contract_is_withheld_whole(app): + # learners_considered is the primary cohort, so a contract with too few + # learners does not appear at all. + pool = _FakePool(rows=[_row(learners_considered=4, learners_needing_attention=4)]) + response = await _get(app, pool, path=ORG_PATH) + + assert response.status_code == 200 + assert response.json()["data"] == [] + + +async def test_sub_floor_needs_attention_count_is_nulled_not_the_row(app): + # The secondary counts are nulled on their own terms, so the tile renders a + # suppressed "--" beside a real learners_considered rather than the whole + # card disappearing. A count of exactly 0 is kept: it names no individual. + pool = _FakePool( + rows=[ + _row(learners_needing_attention=2), + _row(contract_id=102, learners_needing_attention=0), + ] + ) + response = await _get(app, pool, path=ORG_PATH) + + rows = response.json()["data"] + assert rows[0]["learners_considered"] == 42 + assert rows[0]["learners_needing_attention"] is None + assert rows[1]["learners_needing_attention"] == 0 + + +async def test_cutoff_is_resolved_once_and_shared_by_the_page_and_the_count(app): + # The page and the count are two round trips. If the cutoff were left as + # CURRENT_DATE() in the SQL they could evaluate it either side of midnight + # and report a count the rows contradict, so it is resolved once per + # request and spliced into both. + pool = _FakePool(rows=[_row()], total_count=1) + await _get(app, pool) + + probes = [ + query for query, _ in pool.calls if query == learner_queries.NEEDS_ATTENTION_CUTOFF_QUERY + ] + assert len(probes) == 1 + needs_attention = learner_queries._needs_attention(_CUTOFF) # noqa: SLF001 + assert needs_attention in pool.page_call()[0] + # The count query gates on the primary cohort only, so it carries the + # scope but not the predicate -- the two cannot disagree about the cutoff + # because only one of them applies it. + assert "COUNT(DISTINCT learner_id) >= %s" in pool.count_call()[0] + + +async def test_contract_not_in_org_is_403(app): + pool = _FakePool(contract_exists=False) + response = await _get(app, pool) + assert response.status_code == 403 + + +async def test_non_manager_is_403_on_both_routes(app): + for path in (ORG_PATH, CONTRACT_PATH): + response = await _get(app, _FakePool(), path=path, is_manager=False) + assert response.status_code == 403 + + +async def test_pagination_bounds_are_enforced(app): + pool = _FakePool() + response = await _get(app, pool, path=ORG_PATH, params={"limit": 10, "offset": 20}) + assert response.status_code == 200 + assert pool.page_call()[1] == (ORG_ID, settings.anonymization_floor, 10, 20) + + response = await _get(app, _FakePool(), path=ORG_PATH, params={"limit": 0}) + assert response.status_code == 422 + + +# --- governance ----------------------------------------------------------- +# +# ContractNeedsAttention is not in test_column_contract's _CASES: those cases +# are built from build_select, and this model's query is hand-written. The two +# checks below are the ones from that file which are about the model rather +# than the builder, so a suppressible model added outside build_select does not +# skip them. + + +def test_cohort_policy_columns_are_real_model_fields(): + # A typo'd cohort column would silently never suppress -- a governance bug, + # not a cosmetic one. + fields = set(ContractNeedsAttention.model_fields) + policy = ContractNeedsAttention.cohort_policy + assert policy.primary in fields + assert set(policy.secondary) <= fields + + +def test_every_field_is_described_in_the_published_schema(): + properties = ContractNeedsAttention.model_json_schema()["properties"] + for name, field in ContractNeedsAttention.model_fields.items(): + assert field.description, f"{name} has no description" + assert properties[name].get("description") == field.description diff --git a/tests/test_openapi_spec.py b/tests/test_openapi_spec.py index 2088582..8a76a73 100644 --- a/tests/test_openapi_spec.py +++ b/tests/test_openapi_spec.py @@ -64,6 +64,10 @@ def test_each_row_model_gets_its_own_response_schema(specs): "ContentEngagementDepth", "ContractMonthlyEngagementTrend", "ContractContentEngagementDepth", + # Not registered by either loop -- its two routes are written out in + # needs_attention.py -- but it parametrizes the same envelope, so a + # generated client must get its own typed row here too. + "ContractNeedsAttention", } assert envelopes == {f"OrgAnalyticsResponse_{model}_" for model in row_models} assert row_models <= set(schemas)