Course table progress - #15260
Course table progress#15260
Conversation
github-actions
Bot
added
DEV: backend
🟡 Waiting for changesLast updated: 2026-09-14 18:35 UTC |
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15260 — the backend fix is right; the delivery of these notifications is not. Off-diff, caused by it:
Fixes #15189 -> Refs #15189: the issue's resource-completion case still
fails. batch_process_summarylogs -> parse_summarylog -> get_assignments
matches Lessons only, and kolibri_plugin.py:33 passes no
course_session_id, so get_course_lesson_dict is unreachable at sync.
UnitDetailPage.vue:13 holds the whole body behind v-if="loading", set by
useUnitDetail.js:72 in every notification-driven fetchData. Measured:
689ms blank, and the coach's expanded accordion re-collapses. 40-60x
for a class of 20 sitting a pre-test.
nit api.py:914 _IS_COACH_ASSIGNED_QUIZ now matches pre/post tests, whose
mastery_criterion has no coach_assigned key
(progress_tracking.py:374). _IS_COACH_MONITORED_QUIZ.
nit test_api.py:2263 named for batch_process_attemptlogs_for_quizzes,
calls batch_process_masterylogs_for_quizzes.
nit CoursesRootPage.spec.js:42 SET_MAX_NOTIFICATION_TIMESTAMP is not a
real mutation; maxNotificationTimestamp derives from
notifications[0].timestamp via SET_/INSERT_NOTIFICATIONS.
CI green bar Windows/py3.6. axe-core clean on the touched routes.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
|
Hi @marcellamaki - in terms of manual QA both courses and quizzes are syncing correctly and the 'Learner progress' values in the A related issue that can be filed separately (if needed at all, as it seems unlikely for a coach to use it as a live dashboard) is that at unit.lessons.progress.auto.update.mp4 |
bc7206e to
7270ef4
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15260 — 5 of 12 prior findings resolved; 5 still open, 2 acknowledged. Five new blocking findings: the new commit does not run.
Five undefined names landed across the backend, the composable and both spec files. Locally 4 of 11 PrePostTestNotificationsTestCase tests and 4 of 12 useCourseNotificationPolling tests fail with NameError/ReferenceError on the exact paths this PR exists to fix; ruff F821 and eslint no-undef flag all five, so prek and the frontend-test job are red.
Note @pcenov's green QA at 13:52 UTC predates this push and ran against bc7206e3, which contained none of these. No QA ran against this commit.
- blocking ×5 — undefined names (
course_session_cache,classId,CaptureQueriesContext/connection,handleApiError) and the deadrefreshClassCourses(true)argument; inline. - blocking ×1 — re-raised: a delayed LOD sync still writes notifications below the poll's watermark.
- suggestion ×1 — re-raised: the synthetic
quiz_idstill breaks classSummary and Coach Home.
Prior-finding status
UNADDRESSED — CoursesRootPage.vue:483 — polled refresh reuses the foreground loader and fires on unrelated notifications
UNADDRESSED — useCourseNotificationPolling.js:23 — class-scoping invariant breaks on an in-app class switch
UNADDRESSED — api.py:995 — a delayed LOD sync writes notifications the poll cannot fetch
UNADDRESSED — api.py:1002 — the synthetic quiz_id breaks three coach surfaces
UNADDRESSED — UnitDetailPage.vue:13 — notification-driven fetchData blanks the page; out of this branch's scope, not re-raised
ACKNOWLEDGED — PR body — Fixes #15189 → Refs #15189; resource-completion case scoped to a follow-up
ACKNOWLEDGED — api.py:922 — praise for the docstring, carried
RESOLVED — api.py:980 — N+1 for all coach-assigned quizzes
RESOLVED — useCourseNotificationPolling.spec.js:160 — the baseline test could not fail
RESOLVED — api.py:914 — _IS_COACH_ASSIGNED_QUIZ renamed to _IS_COACH_MONITORED_QUIZ
RESOLVED — test_api.py:2263 — test name did not match the function under test
RESOLVED — CoursesRootPage.spec.js:42 — SET_MAX_NOTIFICATION_TIMESTAMP was not a real mutation
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| ) | ||
| if course_session_id is None: | ||
| continue | ||
| quiz_started_notification(masterylog, masterylog.quiz_id, course_session_id) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: re-raising — the synthetic quiz_id still breaks the classSummary and Coach Home paths. classSummary/actions.js:30 reloads whenever !examMap[quiz_id], and a synthetic pre/post-test content_id can never be an Exam id, so every poll carrying one triggers a full /coach/api/classsummary/ fetch that can never satisfy the check; coachNotifications/getters.js:42 drops the same notifications, so Coach Home reads "No activity in your class". Gating both on notification.course_session_id being set is the smaller change, but the card routing (notificationLink() → EXAM_SUMMARY/QUIZ_LEARNER_PAGE_ROOT) has to move to course_session_id at the same time or the cards render blank.
7270ef4 to
a9f2f01
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15260 — 9 of 12 prior findings resolved; 3 still open (both remaining concerns are about how far the poll actually reaches — see inline).
Backend reads correctly now: the widened exclude() still admits masterylog_id IS NULL rows, the reverse-hash lookup is justified because course_session_id lives only in the ephemeral session-token context, and pytest kolibri/core/notifications/test/test_api.py passes locally (61).
Withdrawing my earlier suggestion that the synthetic quiz_id breaks classSummary and Coach Home: progress_tracking.py:558/709/901 already passes masterylog.summarylog.content_id as quiz_id on the same-device path, so that is pre-existing behaviour this branch makes the sync path consistent with, not something it introduces.
COMMENT rather than approve: two suggestions open, one Windows CI job still pending, and manual QA did not run — nothing in the UI was visually verified.
Prior-finding status
RESOLVED — kolibri/plugins/coach/frontend/views/courses/CoursesRootPage.vue — reuses the foreground loader, and fires on unrelated notifications
UNADDRESSED — kolibri/plugins/coach/frontend/composables/useCourseNotificationPolling.js:24 — this invariant breaks on an in-app class switch
UNADDRESSED — kolibri/core/notifications/api.py:996 — a delayed LOD sync writes notifications the poll cannot fetch
ACKNOWLEDGED — kolibri/core/notifications/api.py:1003 — the synthetic quiz_id breaks three coach surfaces (withdrawn)
RESOLVED — kolibri/core/notifications/api.py:985 — course_session_cache is never bound
RESOLVED — kolibri/plugins/coach/frontend/composables/useCourseNotificationPolling.js:29 — classId is not a parameter
RESOLVED — kolibri/plugins/coach/frontend/views/courses/CoursesRootPage.vue:489 — refreshClassCourses(true) has no effect
RESOLVED — kolibri/core/notifications/test/test_api.py:2319 — CaptureQueriesContext and connection are not imported
RESOLVED — kolibri/plugins/coach/frontend/views/courses/tests/CoursesRootPage.spec.js:193 — handleApiError is never imported or mocked
UNADDRESSED — kolibri/core/notifications/api.py:996 — re-raising: delayed LOD sync below the watermark
ACKNOWLEDGED — kolibri/core/notifications/api.py:1003 — re-raising: synthetic quiz_id (withdrawn)
RESOLVED — kolibri/core/notifications/api.py:933 — threading an explicit cache rather than decorating
RESOLVED — kolibri/core/notifications/api.py:981 — N+1 for all coach-assigned quizzes
RESOLVED — kolibri/plugins/coach/frontend/composables/tests/useCourseNotificationPolling.spec.js — cannot fail
RESOLVED — kolibri/core/notifications/api.py — the docstring earns its place
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| (!classId || n.classroom_id === classId.value) && | ||
| new Date(n.timestamp).getTime() > baselineMs, | ||
| ); | ||
| baselineMs = newMs; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: baselineMs carries over across a class switch.
It is captured once at line 13 when setup() runs and only moves forward. CoursesRootPage is reused across classes rather than remounted — that is why line 480 needs an explicit watch(() => route.params.classId, ...) — so setup() does not re-run while fetchNotificationsForClass replaces the store list wholesale.
Switching from a busy class to a quieter one leaves a stale, too-high baseline: newMs <= baselineMs returns early at line 20, and the > baselineMs predicate on line 30 filters the rest. Alone this self-heals once a live event arrives with a current wall-clock timestamp; combined with the backdating above it does not, and a backdated LOD notification stays invisible for the life of the page. The classId filter you added closes the false-positive half of my earlier finding, but not this half — re-reading maxNotificationTimestamp into baselineMs in a watch on classId would.
a9f2f01 to
c7fe91d
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15260 — 12 of 16 prior findings resolved, 3 withdrawn, 1 open and failing CI.
Frontend tests fails on the classId reset test; the test is right, the composable lacks the reset. Inline.
PR description — suggestion: Fixes #15189 auto-closes an issue whose resource half this PR defers. Drop the keyword or link a follow-up.
Manual QA did not run — no UI verification claimed.
Prior-finding status
RESOLVED — CoursesRootPage.vue — foreground loader
RESOLVED — api.py:981 — N+1 lookup
RESOLVED — useCourseNotificationPolling.spec.js — cannot fail
RESOLVED — api.py — praise: docstring
RESOLVED — api.py:985 — course_session_cache unbound
RESOLVED — useCourseNotificationPolling.js:29 — classId unthreaded
RESOLVED — CoursesRootPage.vue:489 — silent refresh inert
RESOLVED — test_api.py:2319 — missing imports
RESOLVED — CoursesRootPage.spec.js:193 — handleApiError unmocked
RESOLVED — api.py:933 — praise: explicit cache
RESOLVED — useCourseNotificationPolling.js:24 — classroom scoping
RESOLVED — test_api.py:2295 — praise: regression guard
CONTESTED — api.py:1003 — synthetic quiz_id; withdrawn, matches progress_tracking.py:558
CONTESTED — api.py:996 — delayed LOD sync; withdrawn, store polls after the watermark
CONTESTED — useCourseNotificationPolling.js:20 — backdated notifications; withdrawn, same cause
UNADDRESSED — useCourseNotificationPolling.js:32 — baselineMs survives a class switch
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| await global.flushPromises(); | ||
|
|
||
| expect(refreshClassCourses).toHaveBeenCalledTimes(1); | ||
| expect(handleApiError).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Cannot fail. handleApiError is reachable only from loadClassData's catch (CoursesRootPage.vue:467), never from the poll callback — remove the .catch(() => {}) on :489 and this still passes.
c7fe91d to
0a52867
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15260 — 16 of 17 prior findings settled; 1 open, 1 new, both inline. CI green. No manual QA.
Prior-finding status
RESOLVED — CoursesRootPage.vue — foreground loader, unrelated notifications
RESOLVED — useCourseNotificationPolling.js:31 — class-scoping invariant
RESOLVED — useCourseNotificationPolling.js:13 — baselineMs re-seeding
RESOLVED — useCourseNotificationPolling.js:36 — classId not a parameter
RESOLVED — useCourseNotificationPolling.spec.js — baseline test cannot fail
RESOLVED — CoursesRootPage.vue:489 — refreshClassCourses(true) no-op
RESOLVED — CoursesRootPage.spec.js:193 — handleApiError unmocked
RESOLVED — api.py:985 — course_session_cache unbound
RESOLVED — api.py:981 — N+1 on coach-assigned quizzes
RESOLVED — api.py:914 — constant misnamed for pre/post tests
RESOLVED — test_api.py:2319 — missing imports
RESOLVED — test_api.py:2263 — test name mismatch
RESOLVED — api.py — praise: docstring
RESOLVED — api.py:933 — praise: explicit cache argument
RESOLVED — test_api.py:2295 — praise: query-count guard
RESOLVED — test_api.py:2248 — praise: pins the None skip
ACKNOWLEDGED — api.py:996 — delayed LOD sync below the watermark
ACKNOWLEDGED — useCourseNotificationPolling.js:27 — backdated notifications
ACKNOWLEDGED — api.py:1003 — synthetic quiz_id (withdrawn: matches progress_tracking.py:558)
ACKNOWLEDGED — PR body — Fixes #15189 with the resource half deferred
ACKNOWLEDGED — UnitDetailPage.vue:13 — outside this diff
UNADDRESSED — CoursesRootPage.spec.js:206 — poll-failure test cannot fail
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| let baselineMs = initial ? new Date(initial).getTime() : 0; | ||
|
|
||
| if (classId) { | ||
| watch(classId, () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Withdrawing the re-seed I asked for. On a class switch fetchNotificationsForClass has not returned, so this reads the outgoing class's max — which baselineMs already holds (line 39, forward-only getter). No-op; the spec passes only by mutating both synchronously. Drop it, or re-seed when the notifications array is replaced.
| await global.flushPromises(); | ||
|
|
||
| expect(refreshClassCourses).toHaveBeenCalledTimes(1); | ||
| expect(handleApiError).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Cannot fail: handleApiError is reachable only from loadClassData's catch, so removing the .catch() at CoursesRootPage.vue:489 leaves this green. Reject the refresh, assert the page still renders.
| save_notifications(notifications) | ||
|
|
||
|
|
||
| _IS_COACH_MONITORED_QUIZ = Q( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Paired constants make the exclude/filter complementarity structural.
0a52867 to
b31109d
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15260 — 20 of 20 prior findings resolved; 1 new finding (a scoping/tracker point, not a code defect).
pytest kolibri/core/notifications/test/test_api.py → 61 passed locally. CI: 7 checks green, 2 still running, none failed. Manual QA did not run in this round — the UI was not visually verified.
- suggestion:
Fixes #15189auto-closes an issue whose resource-completion half is deferred and untracked (inline).
Prior-finding status
RESOLVED — kolibri/plugins/coach/frontend/views/courses/CoursesRootPage.vue — reuses the foreground loader, fires on unrelated notifications
RESOLVED — kolibri/plugins/coach/frontend/composables/useCourseNotificationPolling.js:31 — invariant breaks on an in-app class switch
RESOLVED — kolibri/core/notifications/api.py:996 — delayed LOD sync writes notifications the poll cannot fetch
RESOLVED — kolibri/core/notifications/api.py:981 — N+1 for all coach-assigned quizzes
RESOLVED — kolibri/core/notifications/api.py:1003 — synthetic quiz_id breaks three coach surfaces
RESOLVED — kolibri/plugins/coach/frontend/composables/tests/useCourseNotificationPolling.spec.js — cannot fail
RESOLVED — kolibri/core/notifications/api.py — docstring earns its place (praise)
RESOLVED — kolibri/core/notifications/api.py:985 — course_session_cache never bound
RESOLVED — kolibri/plugins/coach/frontend/composables/useCourseNotificationPolling.js:36 — classId not a parameter
RESOLVED — kolibri/plugins/coach/frontend/views/courses/CoursesRootPage.vue:489 — refreshClassCourses(true) has no effect
RESOLVED — kolibri/core/notifications/test/test_api.py:2319 — CaptureQueriesContext and connection not imported
RESOLVED — kolibri/plugins/coach/frontend/views/courses/tests/CoursesRootPage.spec.js:196 — handleApiError never imported or mocked
RESOLVED — kolibri/core/notifications/api.py:996 — re-raised: notifications below the poll's watermark
RESOLVED — kolibri/core/notifications/api.py:1003 — re-raised: synthetic quiz_id on classSummary and Coach Home
RESOLVED — kolibri/core/notifications/api.py:933 — explicit cache threaded through _resolve_prepost_test_course_session_id (praise)
RESOLVED — kolibri/plugins/coach/frontend/composables/useCourseNotificationPolling.js:27 — backdated sync notifications dropped by the timestamp-forward gate
RESOLVED — kolibri/plugins/coach/frontend/composables/useCourseNotificationPolling.js:39 — baselineMs carries over across a class switch
RESOLVED — kolibri/core/notifications/test/test_api.py:2295 — regression guard on the course-session lookup (praise)
RESOLVED — kolibri/plugins/coach/frontend/composables/useCourseNotificationPolling.js:13 — baselineMs seeded once, class switch kills polling
RESOLVED — kolibri/core/notifications/test/test_api.py:2248 — pins the course_session_id is None skip (praise)
RESOLVED — kolibri/plugins/coach/frontend/views/courses/tests/CoursesRootPage.spec.js:209 — handleApiError assertion cannot fail
RESOLVED — kolibri/plugins/coach/frontend/composables/useCourseNotificationPolling.js:16 — re-seed request withdrawn
RESOLVED — kolibri/core/notifications/api.py:914 — paired constants make exclude/filter complementarity structural (praise)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| def batch_process_attemptlogs(attemptlog_ids): | ||
| for attemptlog in AttemptLog.objects.filter(id__in=attemptlog_ids).exclude( | ||
| masterylog__mastery_criterion__contains="coach_assigned" | ||
| _IS_COACH_MONITORED_QUIZ |
There was a problem hiding this comment.
suggestion: Fixes #15189 closes an issue whose resource half stays broken and untracked.
Step 4 of #15189's repro is "complete a pre test (or post test or mark a resource as completed)". The PR body defers the resource case, which I'm taking as a deliberate scoping call — the problem is the closing keyword. batch_process_summarylogs and this parse_attemptslog path both go through get_assignments, a Lesson-only lookup whose dicts never carry a course_session_id, so every notification they produce has course_session_id = None. Both modes of useCourseNotificationPolling require it truthy, so those can never trigger a course refresh.
Either file the follow-up issue and link it here, or change to Refs #15189 so QA closes it once both halves land.
| const { refreshClassCourses, coursesAreLoading } = useCourses(); | ||
| const refreshPromise = refreshClassCourses(true); | ||
|
|
||
| expect(coursesAreLoading.value).toBe(false); |
There was a problem hiding this comment.
praise: Asserting synchronously before the await is what gives this teeth — setCoursesAreLoading(true) runs synchronously, so dropping the silent guard fails here. The post-await assertion alone would have passed either way.
|
LGTM, no new issues observed while regression testing. |
radinamatic
left a comment
radinamatic
left a comment
There was a problem hiding this comment.
Manual QA passes, good to go! 💯 
🚀
LianaHarris360
left a comment
LianaHarris360
left a comment
There was a problem hiding this comment.
Code changes make sense to me, this is good to go from my side as well.
7ad8873
into
learningequality:develop
5 participants
Summary
When a learner completes a pre-test or post-test on a LOD and it synced, the update was not visible to the coach until they manually refreshed the page. This PR updates the filter for the notifications, which didn't include pre-post tests, and also updates the courses table with a poll, to properly update without a manual refresh. There will be a follow up PR that manages notifying about lesson resource progress within a course - there is a different check we will have to do
References
Fixes #15189
Reviewer guidance
Follow guidance from Peter for original bug to confirm the fix. Note that the sync has to happen, and there may still be a few seconds delay, but both "progress reports" for pre and post tests should be updated.
Screen.Recording.2026-09-01.at.9.08.29.PM.mov
Part 2 - this fixes the course table for pre-tests and post-test statuses (both LODs and non-LODs)
Screen.Recording.2026-09-01.at.6.43.01.PM.mov
Part 3 - Confirm no regressions with "regular" coach assigned quiz progress displays to the coach.
AI usage
Co-implemented with Claude, code-reviewed and manually QA'd by me