Add Learning Objective side panel detail view#14454
Add Learning Objective side panel detail view#14454nucleogenesis wants to merge 4 commits intolearningequality:release-v0.19.xfrom
Conversation
github-actions
bot
added
DEV: backend
Build Artifacts
Smoke test screenshot |
nucleogenesis
requested review from
rtibblesbot
and removed request for
rtibblesbot
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean addition of the Learning Objective side panel with good test coverage and a well-structured data flow through the composable layer.
CI passing. No screenshots in PR; design mockup reviewed from linked issue.
Blocking:
- Struggling learner count rendered outside i18n string — see inline comment on LearningObjectiveSidePanel.vue:62
Suggestions:
- Hard-coded hex color values for bucket backgrounds — see inline comment on LearningObjectiveSidePanel.vue:223
- Duplicated bucketing threshold logic between side panel and utility — see inline comment on LearningObjectiveSidePanel.vue:187
- Score display splits number outside i18n string — see inline comment on LearningObjectiveSidePanel.vue:97
- New i18n strings on non-default branch (
release-v0.19.xvsdevelop) — see inline comment on coursesStrings.js:423
Nitpick:
- Inline style on line 20 — see inline comment on LearningObjectiveSidePanel.vue:20
Comments on lines not in diff:
kolibri/plugins/coach/unit_report_api.py:24 — praise: Good defensive coding with the _chunked helper and _IN_CHUNK_SIZE = 900 to stay within SQLite's variable limit. The comment explaining the headroom calculation (900 + 2 = 902) is helpful for future maintenance.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| /> | ||
| <span> | ||
| <strong>{{ strugglingCount }}</strong> | ||
| {{ learnersStrugglingLabel$({ count: strugglingCount }) }} |
There was a problem hiding this comment.
blocking: The struggling learner count is rendered as a bare number outside the i18n string:
<strong>{{ strugglingCount }}</strong>
{{ learnersStrugglingLabel$({ count: strugglingCount }) }}The count parameter is only used for pluralization ("learner" vs "learners"), but the visible number itself is composed separately. This prevents translators from controlling word order — some languages place the number after the noun phrase, not before.
Move the number inside the string so translators have full control. For example, change the message to:
'{count, number} {count, plural, one {learner} other {learners}} struggling with this objective'
If the bold styling on the count is required, consider using a rich-text interpolation approach or rendering the whole string and wrapping the count portion, similar to how nOfMLearners$ keeps both numbers inside the string.
| const bucketBackgrounds = { | ||
| low: '#FFF5F4', | ||
| mid: '#FFFDF2', | ||
| high: '#F2FCF5', |
There was a problem hiding this comment.
suggestion: These three hex values (#FFF5F4, #FFFDF2, #F2FCF5) are hard-coded, which conflicts with the project convention of using $themePalette / $themeTokens for all colors (see AGENTS.md). The comment notes these are lighter than v_100 palette values, so there's no direct equivalent today.
Consider using the closest available palette tints (e.g., $themePalette.red.v_50 or v_100 equivalents) or extracting these into a shared constant alongside the scoreBucketing.js utility so the color mapping lives in one place. If custom tints are genuinely needed, this may warrant a design system addition.
|
|
||
| function getBucket(score, numQuestions) { | ||
| const ratio = numQuestions > 0 ? score / numQuestions : 0; | ||
| if (ratio > 0.8) { |
There was a problem hiding this comment.
suggestion: The getBucket function here duplicates the 50%/80% threshold logic from scoreBucketing.js:bucketScoresForObjective. If the thresholds change in one place, they could silently diverge.
Consider extracting a shared getBucket(score, numQuestions) function into scoreBucketing.js and importing it here, so the threshold constants live in exactly one place.
| <span | ||
| class="score-correct" | ||
| :style="scoreCorrectStyle(learner.bucket)" | ||
| >{{ learner.score }}</span> |
There was a problem hiding this comment.
suggestion: The score is rendered as a bare number outside the i18n string:
<span class="score-correct">{{ learner.score }}</span>
<span class="score-of-total">{{ ofTotalLabel$({ total: ... }) }}</span>There's already a correctOfTotalLabel string ('{correct, number} of {total, number}') defined in coursesStrings.js that puts both numbers inside the string. The split here is for styling (bold score), which is a legitimate need, but it means the number bypasses locale-aware formatting ({correct, number} would apply digit grouping/decimal separators).
For small scores this is unlikely to matter in practice, but it's worth noting the tradeoff. If you keep the split approach, consider at minimum using the ICU number type on the total to stay consistent.
| </span> | ||
| <span | ||
| class="summary-value" | ||
| style="font-weight: bold" |
There was a problem hiding this comment.
nitpick: style="font-weight: bold" — non-dynamic styles typically go in <style> blocks per project convention. Since font-weight isn't directional, RTLCSS isn't a concern here, but a class would be more consistent with the rest of the component.
| * @param {number} numQuestions - total questions for this LO | ||
| * @returns {{ lowCount: number, midCount: number, highCount: number }} | ||
| */ | ||
| export function bucketScoresForObjective(scores, loId, numQuestions) { |
There was a problem hiding this comment.
praise: Clean, well-documented utility with clear JSDoc and threshold documentation. The separation of bucketScoresForObjective (single LO) and bucketAllObjectives (batch) is a nice API design, and the test coverage with boundary cases (50%, 80%, 0 questions) is thorough.
| * @param {import('vue').Ref<string>} courseSessionId | ||
| * @param {import('vue').Ref<string>} unitContentnodeId | ||
| */ | ||
| export default function useUnitReport(courseSessionId, unitContentnodeId, store) { |
There was a problem hiding this comment.
praise: Clean composable design — the optional store parameter for testability (avoiding getCurrentInstance in tests) is a good pattern. The computed properties layer nicely over the raw API data.
kolibri/core/courses/viewsets.py
Outdated
| ) | ||
| ) | ||
| .order_by("-b_lft", "test_type") | ||
| .order_by("closed", "-b_lft", "test_type") |
There was a problem hiding this comment.
praise: Adding closed as the primary sort key is the right fix — an open test (closed=False sorts as 0) should always be considered the "most recent" since it's currently active, regardless of unit position.
nucleogenesis
force-pushed
the
courses/14169/learning-obj-side-panel
branch
from
fd31c9f to
0c3c29e
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
Unaddressed (re-raised below):
- Struggling learner count rendered outside i18n string (blocking) — no reply, no code change at
LearningObjectiveSidePanel.vue:62 - Duplicated bucketing logic in side panel (suggestion) — no reply, no code change at
LearningObjectiveSidePanel.vue:187. New commits extractedclassifyLearnerMasteryintoscoreBucketing.js, making the fix trivial — but the side panel still uses its own inlinegetBucket
Unaddressed (not re-raised as inline):
- Hard-coded hex colors for bucket backgrounds (suggestion) — no reply, no code change
- Score display splits number outside i18n string (suggestion) — no reply, no code change
- New i18n strings on non-default branch (suggestion) — no reply, no code change
- Inline style on line 20 (nitpick) — no reply, no code change
1/7 prior findings partially addressed (extraction done but not wired). 2 re-raised below.
New commits cleanly extract classifyLearnerMastery with MasteryThreshold/ScoreBucket constants and thorough boundary tests. Mastery pill removal and RTL text alignment are straightforward. The tab-switch re-fetch is a good UX improvement.
CI pending. No screenshots in PR (external link provided). Phase 3 skipped — side panel UI unchanged since prior review.
Blocking:
- Struggling learner count outside i18n — re-raised, see inline
Suggestions:
- Side panel
getBucketshould use extractedclassifyLearnerMastery— re-raised with updated context, see inline
Praise:
classifyLearnerMasteryextraction with boundary tests — see inline
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| /> | ||
| <span> | ||
| <strong>{{ strugglingCount }}</strong> | ||
| {{ learnersStrugglingLabel$({ count: strugglingCount }) }} |
There was a problem hiding this comment.
blocking: (re-raised — unaddressed) The struggling learner count is rendered as a bare number outside the i18n string:
<strong>{{ strugglingCount }}</strong>
{{ learnersStrugglingLabel$({ count: strugglingCount }) }}The count parameter drives pluralization only — the visible number is composed separately. This prevents translators from controlling word order (some languages place the number after the noun phrase).
The fix is to include the number inside the ICU string, e.g.:
message: '{count, number} {count, plural, one {learner} other {learners}} struggling with this objective'
Then render {{ learnersStrugglingLabel$({ count: strugglingCount }) }} without the separate <strong> tag. If the bold styling on the count is needed, use ICU markup or a wrapper approach consistent with other Kolibri i18n strings.
|
|
||
| function getBucket(score, numQuestions) { | ||
| const ratio = numQuestions > 0 ? score / numQuestions : 0; | ||
| if (ratio > 0.8) { |
There was a problem hiding this comment.
suggestion: (re-raised — unaddressed, now more actionable) The new commits extracted classifyLearnerMastery(correctCount, numQuestions) into scoreBucketing.js using MasteryThreshold and ScoreBucket constants — this does exactly what the local getBucket does. The side panel should import and use it:
import { classifyLearnerMastery } from '../../utils/scoreBucketing';
// ...
bucket: classifyLearnerMastery(score, numQuestions),This also replaces the string literals ('low', 'mid', 'high') with ScoreBucket constants throughout the component (lines 218, 222–225, 236, 241).
| bucketAllObjectives, | ||
| } from '../scoreBucketing'; | ||
|
|
||
| describe('classifyLearnerMastery', () => { |
There was a problem hiding this comment.
praise: Thorough boundary-value coverage for classifyLearnerMastery — testing exact thresholds (50%, 80%), just-above boundaries, small question counts, and the zero-questions edge case. This is the kind of test suite that catches off-by-one regressions.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
All prior blocking findings are resolved. The i18n strings are properly structured with ICU MessageFormat, classifyLearnerMastery is now shared, and the inline style was moved to CSS.
CI pending. No inline screenshots; external link provided.
Prior findings
Resolved (5/7):
- Struggling learner count outside i18n (blocking) — count now inside ICU string at line 58
getBucketduplication (suggestion) — now importsclassifyLearnerMasteryfromscoreBucketing.js- Score display splits number outside i18n (suggestion) — now uses
correctOfTotalLabel$with both numbers inside - Inline style on line 20 (nitpick) — moved to
.summary-value-boldCSS class - Hard-coded hex colors (suggestion) — extracted to
ScoreBucketTintsconstant with explanatory comment noting Figma match
Unaddressed (not re-raised):
- New i18n strings on non-default branch (suggestion) — understood as intentional for the v0.19.x release cycle
Suggestions:
- Missing
.catch()on fetch promises — see inline - Mutating API response data — see inline
Praise:
- Clean resolution of all prior review findings — see inline
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| reportData: data, | ||
| }, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
suggestion: The .then() chain has no .catch(). If fetchReport rejects (network error, 500, etc.), this produces an unhandled promise rejection and the LearningObjectivesReport component shows a perpetual spinner since unitReportInfo[unit.id] stays undefined.
A minimal fix would be to catch and log, or set the entry to an error/empty state so the component can show the empty state instead of spinning forever.
| unitContentnodeId: unit.id, | ||
| }).then(data => { | ||
| // Use the numbered title (e.g. "Unit 1: ...") instead of raw title | ||
| data.unit_title = unit.numberedTitle; |
There was a problem hiding this comment.
suggestion: data.unit_title = unit.numberedTitle mutates the API response object in place. This is generally safe since the object isn't shared, but it's a subtle side effect inside a .then() callback. Consider creating a new object with the override ({ ...data, unit_title: unit.numberedTitle }) or doing the title override when constructing unitReportInfo below.
| icon="error" | ||
| :color="$themePalette.red.v_600" | ||
| /> | ||
| <span>{{ learnersStrugglingLabel$({ count: strugglingCount }) }}</span> |
There was a problem hiding this comment.
praise: Good fix — the count is now fully inside the ICU string with {count, number} formatting and {count, plural, ...} for pluralization. Translators have full control over word order.
|
Hi @nucleogenesis, observed the following issues while manually testing:
In the following video the test is in progress and I have completed it with 1 of the 3 learners, the other two have not interacted with the test at all: have.not.started.the.test.mp4All learners have not yet completed the test and it's still in progress: all.learners.not.started.mp4
title.not.matching.mp4
1.of.1.questions.mp4
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the placeholder KRouterLink on learning objective rows with a KButton that emits select-objective. CourseSummaryPage listens for this event and renders the side panel with the selected objective data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Shows per-learner performance for a selected learning objective: - Header with LO title and numbered unit name - Summary section with completion count and pre/post test averages - Warning banner when learners are struggling (low bucket) - Learner list sorted by score ascending with color-coded row backgrounds (red/yellow/green) matching Figma designs - Score display with bold, colored correct count Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move struggling learner count inside the i18n string so translators control word order - Import classifyLearnerMastery and ScoreBucket from shared utils instead of duplicating threshold logic locally - Extract bucket background tints into ScoreBucketTints constant in courseConstants.js instead of hard-coding hex values - Use correctOfTotalLabel$ for the full score string instead of splitting into separate elements - Move inline font-weight: bold to a CSS class Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nucleogenesis
force-pushed
the
courses/14169/learning-obj-side-panel
branch
from
048c8a1 to
85c73bb
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
No new commits since the prior review. All previously blocking findings remain resolved.
Prior findings
Resolved (5/7 from review 1, carried through reviews 2-3):
- Struggling learner count outside i18n (blocking) — count inside ICU string
- Duplicated bucketing logic (suggestion) — uses shared
classifyLearnerMastery - Score display splits number outside i18n (suggestion) — uses
correctOfTotalLabel$ - Inline style on line 20 (nitpick) — moved to
.summary-value-boldCSS class - Hard-coded hex colors (suggestion) — extracted to
ScoreBucketTintsconstant
Not applicable (2/2 from review 3):
- Missing
.catch()on fetch promises —.catch()was already present atCourseSummaryPage.vue:428when this finding was raised; no code change needed - Mutating API response data —
CourseSummaryPage.vue:424already uses spread to create a new object; nested data is only read, never written
Unaddressed (not re-raised):
- New i18n strings on non-default branch — understood as intentional for v0.19.x release cycle
CI pending. No inline screenshots in PR body.
Design conformance: Compared implementation against Figma mockup from #14169. The mockup specifies colored left borders on learner rows, but the implementation uses full-row background tints (ScoreBucketTints). See inline suggestion.
QA feedback: @pcenov's manual testing comment raises 4 issues (wrong title on click, icons not centered, in-progress test behavior, limited test data). These are tracked in the PR comments and not duplicated here.
Suggestions:
- Learner row styling diverges from Figma spec — see inline
Praise:
- Clean shared utility architecture — see inline
Comments on lines not in diff:
kolibri/plugins/coach/frontend/utils/scoreBucketing.js:11 — praise: Good extraction of classifyLearnerMastery with MasteryThreshold constants into a shared utility. The boundary tests (from the earlier commit) cover the exact threshold edges, and both the side panel and the report table now use the same classification logic — no risk of drift.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| v-for="learner in sortedLearners" | ||
| :key="learner.id" | ||
| class="learner-row" | ||
| :style="learnerRowStyle(learner.bucket)" |
There was a problem hiding this comment.
suggestion: The Figma mockup for #14169 shows a colored left border on each learner row (thin vertical stripe: red for low, green for high), while this implementation uses a full-row background tint via ScoreBucketTints. The visual effect is quite different — the mockup's left-border approach is more subtle and avoids the readability concern of colored backgrounds on text rows.
If this was an intentional design decision (discussed with design or in a follow-up), feel free to dismiss. Otherwise, switching to border-left: 3px solid <color> with a white background would match the Figma more closely.
3 participants
Summary
NOTE: This PR is based on #14440 (nucleogenesis:courses/14167/learning-obj-reports-tab) - do not do code review on the commits from that PR @rtibblesbot
Closes #14169
LearningObjectiveSidePanelcomponent that opens when a coach clicks an LO row in the Objectives report tabSidePanelModal+SidePanelLayoutinfrastructure; no new routes neededFull screenshots
Blocked by
This PR is based on top of #14440 (Learning Objectives report tab) and cannot be merged until that PR lands first.
Changes
New files:
LearningObjectiveSidePanel.vue— side panel component with summary stats, warning banner, and per-learner score listLearningObjectiveSidePanel.spec.js— 12 tests covering rendering, sorting, warnings, close behavior, and edge casesModified files:
LearningObjectivesReport.vue— replace placeholderKRouterLinkwithKButtonthat emitsselect-objectiveeventCourseSummaryPage.vue— listen forselect-objective, manageselectedObjectivestate, render side panelcoursesStrings.js— add i18n strings for side panel labels and scoresLearningObjectivesReport.spec.js— add test for click-to-emit behaviorTest plan
pnpm test-jest— 16 tests pass (12 side panel + 4 report)🤖 Generated with Claude Code