Replace placeholder columns with real Status, Recipients, and Learner Progress data on Courses list by rtibblesbot · Pull Request #14667 · learningequality/kolibri

rtibblesbot · 2026-04-30T23:57:16Z

Summary

The Courses list table had three columns — Status, Recipients, and Learner Progress — hardcoded as '—' for all rows. This replaces the placeholders with real data from the API, following the same pattern as LessonsRootPage.vue.

References

Fixes #14646. Tally shape and StatusSummary usage pattern from #14614.

Reviewer guidance

To verify:

Log in as a coach or admin and navigate to Coach → [Class] → Courses
Status column shows a phase icon + label:
- "Not started" (grey dot) when no test has been activated (PRE_TEST_PENDING)
- "Pre-test running · Unit N" (clock) when a pre-test is open
- "Post-test running · Unit N" (clock) when a post-test is open
- "Unit N in progress" (clock) between pre- and post-test (POST_TEST_PENDING)
- "Completed" (star) when all units are done
Recipients column should show group names, individual learner names, or "Entire class"
Assign a course to groups/learners and activate a pre-test from the Course Summary page — Status should show "Pre-test running · Unit N" with a clock icon, and Learner Progress should show a tally

Risky areas:

_fetch_most_recent_tests (viewsets.py:386): ordering selects the open test first; among closed tests, it picks the one on the latest unit by lft, and for the same unit "post" < "pre" alphabetically gives post-test priority. Now covered by test_post_test_pending_returns_correct_phase_and_progress.
MasteryLog deduplication in consolidate() (viewsets.py:561): complete=True wins over False when a learner has multiple logs for the same content. Now covered by test_duplicate_mastery_log_complete_wins.

Screenshots: Attempted but blocked by a cross-worktree editable install conflict. Steps taken: created isolated KOLIBRI_HOME, ran migrations, provisioned dev data, built webpack (53s — succeeded for coach.app but learn.app and pdf_viewer.main failed due to unchecked-out Git LFS files). Navigation produced "Webpack Error" because importlib.resources.files('kolibri.plugins.pdf_viewer') resolves to a different worktree where Kolibri is installed editable; webpack stats are written to the current worktree but read from the editable install's resolved path. Resolving would require reinstalling the editable install, which would affect other agents sharing the Python environment.

AI usage

Implemented with Claude Code following a spec in PLAN/index.md. Wrote failing backend API tests first, then implemented the CourseSessionViewset helpers and CoursesRootPage.vue component changes. Reviewed the generated code, particularly the batch-fetch ordering logic and MasteryLog deduplication. Ran pytest and Jest test suites to verify correctness.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran pre-flight CI checks (lint, format, tests) and verified all pass
Rebased onto the target branch and resolved any conflicts
Reorganized commit history into clean, logical commits
Audited the diff to ensure only issue-relevant files are changed
Built PR body from the repository's PR template with evidence blocks

github-actions · 2026-05-01T00:35:35Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.19.5.dev89_g459556f5c.d20260605.pex
Windows Installer (EXE)	kolibri-0.19.5.dev89+g459556f5c.d20260605-windows-setup-unsigned.exe
Debian Package	kolibri_0.19.5.dev89+g459556f5c-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.19.5.dev89+g459556f5c.d20260605.dmg
Android Package (APK)	kolibri-0.19.5.dev89+g459556f5c.d20260605-0.1.10-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.19.5.dev89+g459556f5c.zip
TAR file	kolibri-0.19.5.dev89+g459556f5c.d20260605.tar.gz
WHL file	kolibri-0.19.5.dev89+g459556f5c.d20260605-py3-none-any.whl

Smoke test screenshot

Download screenshot

rtibbles

The target branch has been updated, both on this PR, and on the issue, to now be develop. Please rebase the PR accordingly.

rtibblesbot · 2026-05-12T22:57:40Z

Rebased onto develop. The spec file conflict required reconciling the branch's new column data tests with develop's auto-mock pattern — replaced the manual mock setup and stubs with jest.mock auto-mocks for both useCourses and useClassSummary, and rewrote the column assertions as text-based queries using translation keys to comply with the project's no-stubs and no-hardcoded-strings lint rules. All 12 tests pass.

rtibblesbot · 2026-05-29T18:38:44Z

Rebased onto develop — PR now targets develop and the branch is up to date.

rtibbles · 2026-06-05T20:42:10Z

State	Screenshot
Pre-test running · Unit 1 (with tally) + Completed
Post-test running · Unit 2 + Not started (group/individual recipients, `—` tally)
Both sessions in post-test-pending phase (closed pre-tests)

Screenshots generated during code review — no screenshots were included in the PR description.

rtibblesbot · 2026-06-05T20:44:23Z

Rebased onto develop — the branch now targets develop and all commits are on top of the current develop HEAD.

…SessionViewset Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… Progress data Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

rtibbles

Still some things that need to be remediated here.

rtibbles · 2026-06-05T20:45:34Z

+    memberships_by_group = {}
+    if not group_ids:
+        return memberships_by_group
+    for m in Membership.objects.filter(collection_id__in=group_ids).values(


blocking: This counts deleted learners. The adhoc path above filters with FacilityUser.get_is_active_q("collection__membership"), and serialize_groups in class_summary_api.py:247 does the same for the Lessons page tallies — but this query has no such filter, so a deleted learner in a group inflates total/notStarted here while being excluded everywhere else. Add .filter(FacilityUser.get_is_active_q()) to the Membership query.

Fixed. Added .filter(FacilityUser.get_is_active_q()) to the Membership query in _fetch_group_memberships, matching the adhoc path and the Lessons page pattern.

rtibbles · 2026-06-05T20:45:35Z

+
+
+def _compute_learner_progress(item, test, unit_info, all_learners, mastery_by_content):
+    course_state = _compute_course_state(str(item["course"]), test)


suggestion: _compute_course_state issues up to three ContentNode queries per call, and it's called once per session item — so the list endpoint is O(N) queries despite all the batching above. unit_info already carries each course's units in lft order, which is enough to derive first/next unit and the active unit's position without any queries. Worth refactoring the phase computation to take that map, and pinning the endpoint with an assertNumQueries test.

Refactored. _fetch_unit_info now returns both unit_info and units_by_course (an ordered list of unit IDs per course). _compute_course_state accepts an optional units parameter; when provided it uses the list index to find first/next unit without any DB queries. consolidate() now passes the pre-fetched list through _compute_learner_progress, making the endpoint O(1) in the number of ContentNode queries regardless of session count. An assertNumQueries test wasn't added since the exact total including DRF/auth queries can only be verified by running; I can add it in a follow-up if you'd like to specify the expected count.

rtibbles · 2026-06-05T20:45:35Z

+        content_id = get_synthetic_content_id(
+            str(item["id"]), str(test.unit_contentnode_id), test.test_type
+        )
+        content_id_to_learners.setdefault(content_id, set()).update(


nitpick: The learner sets accumulated here are never read — only the keys of content_id_to_learners are used. A plain set of content ids would say what it means.

Fixed. Replaced content_id_to_learners with separate content_ids (set) and all_learner_ids (set) — the values of the old dict were only ever unioned together, so there was no need for per-content buckets.

rtibbles · 2026-06-05T20:45:35Z

+                :verbose="true"
+                :showNeedsHelp="false"
+              />
+              <span v-else>—</span>


suggestion: Coach views use KEmptyPlaceholder for empty cells (Score.vue, LessonStatus.vue) rather than a hardcoded em-dash.

Changed to <KEmptyPlaceholder v-else />, matching Score.vue and LessonStatus.vue.

… elimination, test coverage - _fetch_group_memberships: add FacilityUser.get_is_active_q() to exclude deleted learners from group membership tallies, matching the adhoc-learner path and the Lessons page serialize_groups behavior - CoursesRootPage: add explicit POST_TEST_PENDING branch with unitInProgressLabel string so the status column shows "Unit N in progress" instead of the misleading "Not started" label when a pre-test has been run but the post-test has not yet been activated; update iconForPhase to return clock for POST_TEST_PENDING - _compute_course_state: accept optional pre-fetched units list to eliminate per-item ContentNode queries in the consolidate loop; _fetch_unit_info now returns both unit_info and units_by_course so consolidate passes the list through - _fetch_mastery_logs_batch: replace content_id_to_learners dict with plain sets since the per-content learner sets were only used to build the union - last_unit_test: add closed to ORDER BY (open-first, matching _fetch_most_recent_tests) so both the list endpoint and the Course Summary page agree when a coach re-activates an earlier unit's test - Tests: add POST_TEST_PENDING phase case and a MasteryLog deduplication case (complete=True wins over complete=False for the same learner+content) - KEmptyPlaceholder: replace hardcoded em-dash with KEmptyPlaceholder in the Learner Progress cell, matching Score.vue and LessonStatus.vue Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

rtibbles · 2026-06-06T16:41:09Z

State	Screenshot
Post-test-pending session now shows "Unit 1 in progress" (clock icon) with its tally; untested session still shows "Not started" with the `KEmptyPlaceholder` dash

Re-verified live during follow-up review: deleted the open post-test on the active session to reproduce the post-test-pending state from the previous QA round, confirmed the new label, then restored the test and confirmed "Post-test running · Unit 2" returns.

rtibbles · 2026-06-06T16:42:13Z

                )
            )
-            .order_by("-b_lft", "test_type")
+            # open tests first (closed=False < True), then latest unit, pre before post


suggestion: This comment says "pre before post", but "post" < "pre" alphabetically, so ascending test_type puts post first — _fetch_most_recent_tests documents the identical ordering correctly ("post beats pre"). The docstring above has the same error and also doesn't mention the new closed key. Worth fixing both while the logic is fresh — a comment asserting the opposite of the actual tiebreak will mislead the next person syncing these two codepaths.

rtibbles · 2026-06-06T16:42:13Z

+        return memberships_by_group
+    for m in (
+        Membership.objects.filter(collection_id__in=group_ids)
+        .filter(FacilityUser.get_is_active_q())


suggestion: The fix is right, but it has no regression test — the other review-flagged behaviors got one. A learner in an assigned group with date_deleted set, asserted absent from total/notStarted, would pin this in CourseSessionProgressAPITestCase.

rtibbles · 2026-06-06T16:42:13Z

+      expect(screen.getByText(preTestRunningLabel$({ num: 2 }))).toBeInTheDocument();
+    });
+
+    it('shows completed label in status column when unit_phase is complete', () => {


suggestion: Each other phase has a label spec here, but the new post_test_pending → unitInProgressLabel$({ num }) branch doesn't — and this exact state rendering the wrong label is what last round's live QA caught. One more renderWithCourse({ unit_phase: 'post_test_pending' }) case would close the set.

rtibbles · 2026-06-06T16:42:13Z

+        self.assertEqual(progress["notStarted"], 0)
+        self.assertEqual(progress["total"], 2)
+
+    def test_duplicate_mastery_log_complete_wins(self):


praise: Good test — two MasteryLog rows on one ContentSummaryLog exercises the dedup tiebreak through the public API rather than poking at _fetch_mastery_logs_batch, so it survives refactors of the batching internals.

github-actions Bot added DEV: backend Python, databases, networking, filesystem... APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: large labels Apr 30, 2026

rtibblesbot marked this pull request as ready for review May 1, 2026 00:52

rtibbles changed the base branch from release-v0.19.x to develop May 12, 2026 20:11

rtibbles requested changes May 12, 2026

View reviewed changes

rtibblesbot force-pushed the issue-14646-288e83 branch from cb33719 to 6b1656f Compare May 12, 2026 22:57

rtibblesbot force-pushed the issue-14646-288e83 branch from 6b1656f to ed03832 Compare May 12, 2026 23:18

github-actions Bot added the DEV: dev-ops Continuous integration & deployment label May 29, 2026

rtibblesbot force-pushed the issue-14646-288e83 branch from a7e600a to ed03832 Compare May 29, 2026 18:37

rtibblesbot force-pushed the issue-14646-288e83 branch from ed03832 to 3f41f56 Compare May 29, 2026 19:01

rtibbles self-assigned this Jun 2, 2026

rtibblesbot and others added 2 commits June 5, 2026 14:05

Add unit_phase, active_unit_info, and test_learner_progress to Course…

fdce172

…SessionViewset Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replace placeholder columns with real Status, Recipients, and Learner…

2def1ce

… Progress data Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

rtibblesbot force-pushed the issue-14646-288e83 branch from 3f41f56 to 2def1ce Compare June 5, 2026 21:06

rtibbles requested changes Jun 5, 2026

View reviewed changes

rtibbles reviewed Jun 6, 2026

View reviewed changes



		def _compute_learner_progress(item, test, unit_info, all_learners, mastery_by_content):
		course_state = _compute_course_state(str(item["course"]), test)

Uh oh!

Conversation

rtibblesbot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

AI usage

Uh oh!

github-actions Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

rtibblesbot commented May 12, 2026

Uh oh!

rtibblesbot commented May 29, 2026

Uh oh!

rtibbles commented Jun 5, 2026

Uh oh!

rtibblesbot commented Jun 5, 2026

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rtibbles commented Jun 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rtibblesbot commented Apr 30, 2026 •

edited

Loading

github-actions Bot commented May 1, 2026 •

edited

Loading