Repository navigation
feat!: count active enrollments, so the API agrees with Open edX - #390
Merged
Merged
Conversation
Every enrollment number has a defensible alternative depending on which filters
you apply, which is why a count taken from Open edX directly never matches the
one this API reports. For course-v1:moe+CS104+2024 the database yields 77,984
raw rows, 76,962 active enrollments (what Open edX reports) and 76,246 after
this API's filters, and nothing in the response explained the 716 difference.
`breakdown=1` now returns `enrollments_count_breakdown` next to
`enrollments_count`, with one entry per filter stage in the order the filters
are applied:
all_rows -> active_enrollment -> active_user -> excluding_platform_staff
-> in_visible_courses -> excluding_course_staff
The first stage is the raw count Open edX reports; the last always equals
`enrollments_count`. The drop between any two adjacent stages is attributable
to exactly one rule, so a discrepancy can be explained from the response alone
instead of by reading the source or querying the database.
The whole breakdown is computed in a single pass with conditional aggregation.
One query per stage would multiply the cost of a query that is already heavy
enough to sit behind a waffle flag.
Opt-in: without `breakdown=1` the response shape is byte-identical, so existing
consumers are unaffected. Only `enrollments` is supported for now; the other
stats need their own stage definitions and the cached certificate path cannot
produce a funnel at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`breakdown=1` now covers `learners` and `courses` as well as `enrollments`.
learners: in_tenant -> active_user -> excluding_platform_staff
-> excluding_course_staff
The base stage is deliberately the tenant-scoped population rather than every
user on the installation. Tenant membership for a learner is decided by signup
source, not by enrolment, so an unscoped first stage would report the whole
platform and tell the caller nothing.
courses: all_courses -> in_visible_courses
Short by nature, because catalog visibility is the only filter between them.
The gap is exactly the `hidden_courses` statistic, which the test asserts.
The courses stage condition is written out rather than reusing the
`course_is_visible` annotation, so it does not depend on the removable
annotation machinery holding across a values()/annotate() regrouping.
Not supported, and documented as such: `certificates` (the default path reads a
pre-aggregated cache with no intermediate stages), `hidden_courses`,
`learning_hours`, `unique_learners`.
Tests assert the same two invariants as the enrollments breakdown rather than
hardcoding stage counts: the last stage equals the stat's own value, and stages
never increase. For courses there is a third: all_courses minus
in_visible_courses must equal the hidden-course count.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tenant-level breakdown did not cover the case that prompted it. The
question that keeps being asked is about a single course — for
course-v1:moe+CS104+2024, Open edX reports 76,962 and this API reports 76,246 —
and per-course counts come from /courses/v1/courses/, not from total_counts,
which has no course filter at all.
CourseDetailsSerializer now carries an optional `enrolled_count_breakdown` with
the same stages as the tenant funnel:
all_rows -> active_enrollment -> active_user -> excluding_platform_staff
-> excluding_course_staff
`active_enrollment` is the figure Open edX reports for the course, and
`excluding_course_staff` is `enrolled_count` itself, so the difference between
the two systems is readable from a single course row.
It uses the existing optional-field mechanism rather than a new parameter, so
the field is omitted entirely unless `optional_field_tags=enrolled_count_breakdown`
is passed, and the annotations that back it are only added to the queryset in
that case. Each stage costs one correlated subquery per course, which is why it
is not computed by default.
Also collapses the learners import in views/statistics.py, which isort rejected
in the previous run; the single line is 117 characters against a 120 limit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 100% coverage gate flagged details/courses.py:104 as unreached. The branch was genuinely dead: no breakdown stage passes `exclude_course_staff`, because the final stage of the breakdown is `enrolled_count` itself, which is annotated separately and already carries that exclusion. Removed rather than tested. A parameter that exists only to satisfy a coverage gate is worse than no parameter, and the docstring now records why no stage needs one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
no-self-use on the serializer getter and too-many-arguments on get_courses_queryset, both suppressed the way this codebase already suppresses them (38 and 3 existing instances in the same two files respectively). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BREAKING. Every published count now reports every matching row, instead of excluding deactivated accounts, platform staff, course-team staff and hidden courses. Requested so that the numbers stop disagreeing with what Open edX reports on its own screens. Changed: per-course enrolled_count and active_count, the tenant enrollments, learners and courses statistics, the live certificate count, and the shared user/course gate behind the learner, enrollment and payment lists. Removed the per-course enrolled_count_breakdown added earlier in this branch, which existed to explain a gap that raw counts remove. A kill switch comes with it. The waffle flag fx_dashboard.legacy_filtered_counts restores the previous behaviour without a redeploy. It is off by default, so raw counts are what ships; activating it reverts every count in one step. Given the size of the shift this is the difference between a bad day and an outage. Recorded deviation: this was implemented against my recommendation, and the figures it produces match neither the previous API nor Open edX exactly. For course-v1:moe+CS104+2024 the raw count is 77,984, where Open edX reports 76,962 and the previous API reported 76,246. The 1,022 difference against Open edX is un-enrolled rows (is_active=0), which raw counting includes and Open edX's own screens do not. The stage that matches Open edX exactly is active_enrollment, not all rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
flake8 E303. Left behind when the per-course breakdown annotations were removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mypy call-arg. Left behind when get_courses_queryset lost the parameter along with the per-course breakdown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Switches the default counting rule from "every row" to "every row whose own is_active is true, with the FutureX-specific exclusions dropped". Counting every row reported more than Open edX rather than the same figure, because it included un-enrolled learners: for course-v1:moe+CS104+2024 that was 77,984 against Open edX's 76,962. Keeping is_active and dropping only the platform-staff, course-team and catalog-visibility exclusions lands on 76,962 exactly, which is the point of the change. The rule is now uniform: keep each row's own is_active, drop filters that exclude rows for what they are related to. - enrollments keep is_active, and no longer drop rows for the enrolled user being deactivated, staff, or a member of the course team - learners keep the account's is_active, and no longer drop staff - certificates keep user__is_active, and no longer drop staff or hidden courses - courses no longer drop hidden ones Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Raw counting is now the default, so the tests that assert the old filtered
numbers no longer describe the default. They are not obsolete though: they
describe the rollback path, which is the thing most worth keeping tested. Each
now runs under override_flag('fx_dashboard.legacy_filtered_counts'), so it keeps
guarding exactly the behaviour the kill switch restores, and a new test asserts
the default differs from it and never reports fewer rows.
Also removes the tenant-level breakdown entirely. Its defining invariant was
that the last stage equals the reported count, and raw counting broke that: for
tenant 1 the count became 20 while the stage still read 16. It was built to
explain a gap that this change closes by construction, so it is deleted rather
than repaired.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
flake8 W391 in two files. The test job never ran on the previous push: quality failed first and fail-fast cancelled it, so the test state after pinning the exclusion tests to the legacy flag is still unverified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…fault The three remaining failures were the full-response assertions on the headline statistics endpoint, which encode the filtered numbers. They now run under the legacy flag, keeping the rollback path covered in detail. Pinning them alone would have left the shipping default untested on the endpoint consumers actually read, so a new test asserts the default differs from the legacy behaviour and never reports fewer than it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Inserting the new raw-default test placed it between the enable_heavy_queries decorator and test_all_stats, so the new test took the decorator and test_all_stats lost it. That made its certificates_count read 0, since without the flag the count is served from the CourseStat cache, which is empty in tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unused get_user_model, Count, Q and check_staff_exist_queryset imports in statistics/learners.py, and a too-many-arguments suppression on get_courses_queryset that stopped being needed when the breakdown parameter was removed. pylint reports that last one as a useless suppression, which is the right call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pylint wrong-import-order. The script that pinned the exclusion tests to the legacy flag appended the import after the first-party block instead of with the other third-party imports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collapsed the two-name import in details/courses.py, which fits on one line at 116 characters, and moved the toggles import above the helpers block in statistics/courses.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… counts Restores breakdown=1 on total_counts for enrollments, learners and courses. I had removed it because raw counting broke its invariant, but removing it was the wrong call: with raw counts the breakdown is more useful, not less, because it carries the number the API now reports and the number it used to report in the same response. The invariant is repaired rather than dropped. Which stage equals the count now depends on the counting mode - by default the stage that keeps only the row's own is_active, and with fx_dashboard.legacy_filtered_counts active the last, fully filtered stage. The test asserts that in both modes, so the two cannot drift apart again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes breakdown=1 and the per-stage functions behind it, for enrollments, learners and courses. It existed to explain why enrollments_count disagreed with Open edX. It no longer does: enrollments_count now counts active enrollments, which is the same population Open edX reports, so there is no gap left for the breakdown to account for. Keeping it would mean maintaining an explanation of something that does not happen. The response shape is unchanged for anyone who was not passing breakdown=1, which the default-shape test already covered before this removal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AliAlfaifi
changed the title
Contributor
Author
|
Title and description rewritten — they described the opposite of what this PR now does. It opened as "expose a per-stage breakdown" to explain why the API and Open edX disagreed. The direction then changed twice: first to reporting every row, which overshot (77,984 against Open edX's 76,962, because it counted un-enrolled learners), and finally to counting active enrollments, which lands on 76,962 exactly. The breakdown was added, extended to learners and courses, removed, restored, and removed again once the gap it explained no longer existed. That is why there are 18 commits for a +128/-17 diff. Squash on merge. The final diff is the thing to review; the commit history mostly documents a direction that was abandoned. Both checks are green. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the published counts report the same population Open edX reports on its own screens.
The problem
For
course-v1:moe+CS104+2024, Open edX reports 76,962 enrollments and this API reported 76,246. The 716 difference was FutureX-specific filtering, verified against the production replica:Every dashboard number carried a similar gap, and explaining it required reading the source or querying MySQL.
The change
Counts now keep each row’s own
is_activeand drop the FutureX-specific exclusions — platform staff, course-team staff, and catalog visibility. That is exactly the population Open edX counts, so CS104 now reads 76,962 on both systems.Applied to:
enrolled_countandactive_countenrollments,learnersandcoursesstatisticsBreaking
Every published count goes up. The dashboard, CSV exports and every integration shift at once. At
spscthe learner count roughly doubles, because 24,259 of its 70,920 enrolled accounts are deactivated and 22,873 of those never logged in.Numbers will also stop matching previously published reports. This aligns the API with Open edX going forward, not with its own history.
Rollback
Waffle flag
fx_dashboard.legacy_filtered_counts— off by default. Activating it restores every previous number without a redeploy. Worth having someone able to flip it during the deploy window.Tests
The 33 tests that asserted the filtered numbers were not rewritten. They describe the behaviour the kill switch restores, which is the thing most worth keeping covered, so each now runs under
override_flag(...legacy_filtered_counts). Two new tests assert the shipped default differs from legacy and never reports fewer rows, so the default is covered on both the statistics functions and thetotal_countsendpoint.Not included
certificates,hidden_courses,learning_hoursandunique_learnerskeep their current behaviour. The default certificate count is served from the pre-aggregatedCourseStatcache, which has no unfiltered variant to fall back to.