Pre/Post test renderer#14389
Pre/Post test renderer#14389AlexVelezLl wants to merge 4 commits intolearningequality:release-v0.19.xfrom
Conversation
github-actions
bot
added
APP: Learn
Build Artifacts
Smoke test screenshot |
|
Hey @rtibblesbot, could you take a look, please? |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean PR that adds pre/post test rendering within the course context. The strategy pattern refactor of initContentSession is a nice improvement. CI passing.
- blocking: QUIZ strategy validate regression —
lessonIdno longer checked (test silently passes due to try/catch antipattern) - suggestion: Missing
force/immediateflags on exam submission in PrePostTestRenderer - suggestion: No intermediate progress tracking when saving answers without closing
- suggestion: PrePostTestRenderer and AnswerHistory use Options API (AGENTS.md requires Composition API for new components)
- suggestion:
postTestItemDescriptionalways readsversion_a_item_idsregardless of which version the learner gets - praise: Strategy pattern refactor, thorough COURSE_TEST test coverage, good navigation constraint logic
@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
| QUIZ: 'quiz', | ||
| NODE: 'node', | ||
| COURSE_TEST: 'course_test', | ||
| }; |
There was a problem hiding this comment.
praise: The strategy pattern refactor is well-structured — validate, buildData, and isSessionAlreadyStarted are cleanly separated per session type, making the logic much easier to follow than the previous imperative approach. The COURSE_TEST strategy slots in naturally.
|
|
||
| if (close) { | ||
| data.progress = 1; | ||
| this.submitting = true; |
There was a problem hiding this comment.
suggestion: When close is true, this doesn't pass force: true or immediate: true to updateContentSession, unlike QuizRenderer (line 340-343). The immediate flag is likely set automatically because progress = 1 triggers the completion path in updateContentSession. However, force: true is a safety net that ensures the update is sent even if no thresholds are met (e.g., if the session was somehow already at progress 1). Consider adding both flags to match QuizRenderer behavior:
if (close) {
data.progress = 1;
data.force = true;
data.immediate = true;
this.submitting = true;
}There was a problem hiding this comment.
I matched the behavior we have on ExamPage, which I think is the most similar behavior here. But I think it doesn't hurt if we add these additional flags.
There was a problem hiding this comment.
Yes, I think adding this update doesn't hurt and might save some data along the way!
| this.submitting = true; | ||
| } | ||
|
|
||
| return this.updateContentSession(data).then(() => { |
There was a problem hiding this comment.
suggestion: When close is false, QuizRenderer tracks intermediate progress (lines 347-354) so the quiz shows as "in progress" in the UI. This component sends no progress at all when saving a non-closing answer. Was this intentional for pre/post tests? If intermediate progress isn't needed, a brief comment explaining why would help future readers understand the divergence from QuizRenderer.
There was a problem hiding this comment.
It's not really needed no - we do this for the QuizRenderer for the UI so that the visual indicator to the user changes, but as we don't have that for coach assigned exams or pre/post tests in the same way, it's less of a concern.
I think, again, we can revisit this when we have one component to rule them all!
| let _uid = 0; | ||
|
|
||
| export default { | ||
| name: 'PrePostTestRenderer', |
There was a problem hiding this comment.
suggestion: AGENTS.md says "New components must use setup(). Do not use Options API (data(), computed:, methods:)." This component and AnswerHistory.vue use the Options API. The PR description explains this was an intentional copy from QuizRenderer for diff visibility, which is a reasonable trade-off for reviewability — but it might be worth migrating to Composition API before merge or as a fast follow, to avoid accumulating tech debt.
There was a problem hiding this comment.
We may do this later when we unify all these components
| return completedLabel$(); | ||
| } | ||
| const numQuestions = get( | ||
| props.unitTree, |
There was a problem hiding this comment.
suggestion: This always reads version_a_item_ids.length for the question count. If versions A and B can have different numbers of items, this could show the wrong total for learners assigned version B. If they're always the same length, a comment noting that assumption would help.
There was a problem hiding this comment.
In theory these should always have the same length, right @rtibbles?
There was a problem hiding this comment.
Yes, if they're not something has gone wrong!
| }; | ||
|
|
||
| const onResourceFinished = () => { | ||
| if (activeTest.value) { |
There was a problem hiding this comment.
praise: Good handling of the active test case in onResourceFinished — cleanly prevents the resume position from being updated when the learner submits a pre/post test, which is the right behavior since test lifecycle is coach-controlled.
| await initContentSession({ | ||
| courseTest: { unitId: 'test_unit', testType: 'pre' }, | ||
| node, | ||
| courseSessionId: 'test_course_session', |
There was a problem hiding this comment.
praise: Thorough test coverage for the COURSE_TEST strategy — validation, happy path, session deduplication, repeat behavior, and different unit/test type transitions are all covered.
AlexVelezLl
force-pushed
the
assessment-viewer
branch
from
afa67bf to
ca16b60
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Delta review of the pre/post test renderer PR.
Prior findings
| Finding | Status |
|---|---|
QUIZ strategy lessonId validation regression |
RESOLVED — lessonId added to QUIZ validate check |
Missing force/immediate flags on submission |
RESOLVED — both flags now included |
| No intermediate progress tracking | ACKNOWLEDGED — matches ExamPage behavior, deferred to maintainer |
| Options API usage | ACKNOWLEDGED — planned for later unification |
version_a_item_ids assumption |
ACKNOWLEDGED — awaiting maintainer confirmation |
All prior blocking/suggestion findings are resolved or acknowledged.
CI still in progress. One new suggestion below.
@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
kolibri/plugins/learn/frontend/composables/__tests__/useProgressTracking.spec.js
Show resolved
Hide resolved
| ); | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
praise: The strategy pattern refactor is well-executed — each strategy cleanly encapsulates its own validation, data building, and session identity logic. The COURSE_TEST strategy slots in naturally alongside QUIZ and NODE without increasing complexity in the main initContentSession flow.
AlexVelezLl
force-pushed
the
assessment-viewer
branch
from
ca16b60 to
5cae3f8
Compare
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Have not manually tested yet - just one thing stood out, I'd have to double check the specs (and/or it might be an i18n decision later) but I think just putting a separate submit string in the translator object that we can tweak later would be helpful.
| }; | ||
|
|
||
| // Each strategy encapsulates one session type's rules | ||
| const SESSION_STRATEGIES = { |
There was a problem hiding this comment.
What a splendid application of the Rule of Three!
| v-if="submitModalOpen" | ||
| :title="isSurvey ? $tr('submitSurvey') : $tr('submitExam')" | ||
| :submitText="isSurvey ? $tr('submitSurvey') : $tr('submitExam')" | ||
| :title="$tr('submitExam')" |
There was a problem hiding this comment.
We may want to add a new string for "submit test" instead of "quiz" - can put it in the dedicated translator.
There was a problem hiding this comment.
Yes, this was the same strings the specs showed, but definitely "submit test" sounds better. Is it okay just having a "submit test" string? Or should we differentiate "submit pre-test", "submit post-test"?
There was a problem hiding this comment.
Yes - it feels like for consistency with the other strings, saying "test" makes more sense than quiz - and then people can tell us later whether we need to say "pretest" and "posttest" separably.
AlexVelezLl
force-pushed
the
assessment-viewer
branch
2 times, most recently
from
614a405 to
b680aba
Compare
github-actions
bot
added
DEV: dev-ops
AlexVelezLl
force-pushed
the
assessment-viewer
branch
from
b680aba to
0657187
Compare
AlexVelezLl
force-pushed
the
assessment-viewer
branch
from
0657187 to
3db474c
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Delta review — all prior blocking/suggestion findings are resolved or acknowledged.
Prior findings
| Finding | Status |
|---|---|
QUIZ strategy lessonId validation regression |
RESOLVED — lessonId added to QUIZ validate check |
Missing force/immediate flags on submission |
RESOLVED — both flags now included |
| No intermediate progress tracking | ACKNOWLEDGED — matches ExamPage behavior, deferred to unification |
| Options API usage | ACKNOWLEDGED — planned for later unification |
version_a_item_ids assumption |
ACKNOWLEDGED — confirmed by maintainer |
CI passing. No new blocking issues.
- nitpick: Typo in variable name (see inline)
- praise: Strategy pattern refactor, thorough COURSE_TEST test coverage, good
submitTestActionstring update per maintainer feedback
@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
| return []; | ||
| } | ||
| const preposttestVersion = this.masteryCriterion.version; | ||
| const preposttestAssesments = get( |
There was a problem hiding this comment.
nitpick: Typo: preposttestAssesments → preposttestAssessments (missing an s).
| }); | ||
| } | ||
|
|
||
| const SessionStrategyType = { |
There was a problem hiding this comment.
praise: The strategy pattern refactor here is a clean improvement — each session type's validation, data-building, and deduplication logic is now co-located and independently testable. The thorough COURSE_TEST test coverage gives good confidence in the new path.
pcenov
left a comment
pcenov
left a comment
There was a problem hiding this comment.
Thanks @AlexVelezLl - I confirm that everything is implemented as specified by you in the reviewer guidance. Will be further testing this once the other pieces of the puzzle are put together! 👍
|
Flagging that we'll hold off on merge here until after today's off-cycle patch release. |
4 participants
Summary
useProgressTrackingcomposable to track pre/post test progress.initContentSessionto use a Strategy pattern to make this method more flexible, maintainable, and understandable, since the current implementation was super difficult to read (at least it was to me). Previous unit tests had all these execution paths well covered, so I have good confidence in the refactor.PrePostTestRendereras another case of the "CourseContentViewer" component, trying to make it compatible with other renderers.Grabacion.de.pantalla.2026-03-16.a.la.s.10.13.59.a.m.mov
References
Closes #14134.
Reviewer guidance
AI usage
I used Claude to write unit tests for the updated behavior of the
useProgressTrackingcomposable