From 1ef6c066975000b64653ae6f69e4e17905850df2 Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Fri, 2 Oct 2026 14:24:25 -0400 Subject: [PATCH 1/2] feat(b2b_dashboard): distinct-learner needs-attention count per contract Adds /organizations/{org}/needs-attention and its contract-scoped sibling, returning COUNT(DISTINCT learner_id) over the needs-attention predicate at org x contract grain, k-anonymity floored like every other aggregate here. Backs the needs-attention KPI tile on MIT Learn's B2B analytics page. The existing needs_attention_count cannot: it is a SUM over a learner x course-run grain, so a learner stale in three courses counts three times, which cannot sit beside active_learners, itself a distinct-learner count. Aggregated in this service rather than as a column on mv_b2b_contract_utilization. A dbt column would have restated the 30-day rule and the completion-status CASE in a second repo and a second language, and frozen the cutoff at MV-refresh time, letting the tile and the learner directory disagree about the same learner for up to a refresh interval. Computing it from learner_queries._needs_attention against the same per-request cutoff means one rule and one cutoff for both. Served from its own endpoint rather than folded into contract-utilization because the MV behind it refreshes on its own schedule, and one as_of per section is what stops a lagging view from making another look fresher than it is. A client reads this tile's freshness from this envelope. learners_considered is the primary cohort and gates the row; learners_needing_attention and learners_outcomes_withheld are secondary and nulled on their own terms. Consent gates the latter two but not the first, mirroring learner-progress: being enrolled is not an outcome. The sqlite tests execute the production query strings against real rows, with the in-memory databases ATTACHed under the schema name the query names, since the distinct-learner property is about values and no assertion on SQL text can show it. Co-Authored-By: Claude Opus 5 --- openapi/specs/b2b_dashboard.yaml | 206 +++++++ .../tenants/b2b_dashboard/app.py | 12 +- .../tenants/b2b_dashboard/learner_queries.py | 94 ++++ .../tenants/b2b_dashboard/models.py | 68 ++- .../b2b_dashboard/routers/needs_attention.py | 124 +++++ tests/test_dashboard_needs_attention.py | 503 ++++++++++++++++++ tests/test_openapi_spec.py | 4 + 7 files changed, 1008 insertions(+), 3 deletions(-) create mode 100644 src/ol_analytics_api/tenants/b2b_dashboard/routers/needs_attention.py create mode 100644 tests/test_dashboard_needs_attention.py 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..661e665 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,92 @@ def learner_progress(filters: ProgressFilters, cutoff: datetime.date) -> Progres return ProgressQuery(page, count, tuple(params)) +@dataclass(frozen=True) +class AggregateQuery: + """``page`` takes ``params`` plus LIMIT and OFFSET; ``count`` takes + ``params`` plus the anonymization 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" + ) + # 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 + " GROUP BY contract_id ORDER BY contract_id LIMIT %s OFFSET %s" + ) + # The same primary-cohort gate build_count applies, for the same reason: + # suppress_small_cohorts drops sub-floor rows after the query returns, so an + # ungated COUNT would exceed anything paging can reach, and subtracting the + # rows the caller does receive would tell them exactly how many sub-floor + # contracts their org has. + count = ( + "SELECT COUNT(*) AS total_count FROM (" # noqa: S608 + f"SELECT contract_id FROM ({records}) records" + " GROUP BY contract_id HAVING COUNT(DISTINCT learner_id) >= %s) 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..ea8e8f1 --- /dev/null +++ b/src/ol_analytics_api/tenants/b2b_dashboard/routers/needs_attention.py @@ -0,0 +1,124 @@ +"""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 + ) + rows = await fetch_and_suppress( + query.page, + (*query.params, 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..1482013 --- /dev/null +++ b/tests/test_dashboard_needs_attention.py @@ -0,0 +1,503 @@ +"""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) + +# 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, 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, 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, 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, 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, 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, 100, 0)).fetchall() + conn.close() + + assert rows == [(101, 1, 1, 0), (102, 1, 1, 0)] + + +@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, 100, 0)).fetchall() + [(total,)] = _run(conn, query.count, (*query.params, 5)).fetchall() + conn.close() + + # The page itself is unfiltered -- Python suppression drops the small row -- + # but the count already excludes it. + assert [row[0] for row in page] == [101, 102] + 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, 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, 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, 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) From 4e95e644c2f77f20a6c202cebd7c8cb72d5e4f56 Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Fri, 2 Oct 2026 15:35:39 -0400 Subject: [PATCH 2/2] fix(b2b_dashboard): gate the needs-attention page on the primary cohort The page query was ungated while total_count carried the floor, so the two described different sets. 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 OrgAnalyticsResponse tells clients to page by comparing total_count against len(data) plus offset. Varying the offset and watching which contracts surfaced also revealed how many suppressed ones preceded each visible one, which is the figure gating the count exists to withhold. Both queries now share one HAVING, written once so they cannot drift. suppress_small_cohorts still runs, since it is what nulls the sub-floor secondary counts within a surviving row. Raised by Copilot on #89. The same page/count mismatch exists on the five MV-backed endpoints, where build_select emits no cohort gate and build_count does; that is a shared-machinery change and is tracked separately. Co-Authored-By: Claude Opus 5 --- .../tenants/b2b_dashboard/learner_queries.py | 28 ++++++--- .../b2b_dashboard/routers/needs_attention.py | 6 +- tests/test_dashboard_needs_attention.py | 60 +++++++++++++++---- 3 files changed, 71 insertions(+), 23 deletions(-) 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 661e665..5a4e251 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/learner_queries.py @@ -299,8 +299,8 @@ def learner_progress(filters: ProgressFilters, cutoff: datetime.date) -> Progres @dataclass(frozen=True) class AggregateQuery: - """``page`` takes ``params`` plus LIMIT and OFFSET; ``count`` takes - ``params`` plus the anonymization floor.""" + """``page`` takes ``params`` plus the anonymization floor, LIMIT and + OFFSET; ``count`` takes ``params`` plus the floor.""" page: str count: str @@ -364,21 +364,31 @@ def needs_attention_aggregate( 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 - " GROUP BY contract_id ORDER BY contract_id LIMIT %s OFFSET %s" + f" GROUP BY contract_id {cohort_gate}" + " ORDER BY contract_id LIMIT %s OFFSET %s" ) - # The same primary-cohort gate build_count applies, for the same reason: - # suppress_small_cohorts drops sub-floor rows after the query returns, so an - # ungated COUNT would exceed anything paging can reach, and subtracting the - # rows the caller does receive would tell them exactly how many sub-floor - # contracts their org has. count = ( "SELECT COUNT(*) AS total_count FROM (" # noqa: S608 f"SELECT contract_id FROM ({records}) records" - " GROUP BY contract_id HAVING COUNT(DISTINCT learner_id) >= %s) gated" + f" GROUP BY contract_id {cohort_gate}) gated" ) return AggregateQuery(page, count, tuple(params)) 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 index ea8e8f1..dff6120 100644 --- a/src/ol_analytics_api/tenants/b2b_dashboard/routers/needs_attention.py +++ b/src/ol_analytics_api/tenants/b2b_dashboard/routers/needs_attention.py @@ -75,9 +75,13 @@ async def _respond( 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, page.limit, page.offset), + (*query.params, settings.anonymization_floor, page.limit, page.offset), ContractNeedsAttention, settings.anonymization_floor, ) diff --git a/tests/test_dashboard_needs_attention.py b/tests/test_dashboard_needs_attention.py index 1482013..555c8c5 100644 --- a/tests/test_dashboard_needs_attention.py +++ b/tests/test_dashboard_needs_attention.py @@ -38,6 +38,11 @@ # 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 @@ -144,7 +149,7 @@ def test_counts_each_learner_once_however_many_courses_they_are_quiet_in(): ] conn = _db(rows) query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) - [row] = _run(conn, query.page, (*query.params, 100, 0)).fetchall() + [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) @@ -176,7 +181,7 @@ def test_projects_exactly_the_model_fields_in_order(): # 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, 100, 0)) + cursor = _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)) columns = [description[0] for description in cursor.description] conn.close() @@ -198,7 +203,7 @@ def test_consent_gate_moves_learners_between_the_counts(monkeypatch): def aggregate(): query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) - return _run(conn, query.page, (*query.params, 100, 0)).fetchall() + 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. @@ -234,7 +239,7 @@ def test_boundary_is_the_same_day_the_learner_directory_uses(): ] ) query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) - [row] = _run(conn, query.page, (*query.params, 100, 0)).fetchall() + [row] = _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)).fetchall() conn.close() assert row == (CONTRACT_ID, 4, 2, 0) @@ -253,7 +258,7 @@ def test_inactive_enrollments_are_left_out_like_the_directory_default(): ] ) query = learner_queries.needs_attention_aggregate(ORG_ID, CONTRACT_ID, _CUTOFF) - [row] = _run(conn, query.page, (*query.params, 100, 0)).fetchall() + [row] = _run(conn, query.page, (*query.params, _NO_FLOOR, 100, 0)).fetchall() conn.close() assert row == (CONTRACT_ID, 1, 1, 0) @@ -275,12 +280,41 @@ def test_org_scope_returns_one_row_per_contract_and_never_crosses_orgs(): ] ) query = learner_queries.needs_attention_aggregate(ORG_ID, None, _CUTOFF) - rows = _run(conn, query.page, (*query.params, 100, 0)).fetchall() + 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 @@ -295,13 +329,13 @@ def test_count_query_applies_the_primary_cohort_floor(): ] ) query = learner_queries.needs_attention_aggregate(ORG_ID, None, _CUTOFF) - page = _run(conn, query.page, (*query.params, 100, 0)).fetchall() + page = _run(conn, query.page, (*query.params, 5, 100, 0)).fetchall() [(total,)] = _run(conn, query.count, (*query.params, 5)).fetchall() conn.close() - # The page itself is unfiltered -- Python suppression drops the small row -- - # but the count already excludes it. - assert [row[0] for row in page] == [101, 102] + # 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 @@ -396,13 +430,13 @@ async def test_envelope_carries_the_learner_mvs_own_freshness(app): 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, 100, 0) + 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, 100, 0) + 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] @@ -472,7 +506,7 @@ 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, 10, 20) + 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