annotate attendance with historical learner references#14432
Conversation
github-actions
Bot
added
DEV: backend
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
The PR adds historical learner tracking to attendance — previously enrolled learners appear read-only, and present/absent counts include them. Good approach overall.
CI failing: frontend test does not show learners added after the session was created fails because enrolledLearnerIds is stored but never consumed by sortedLearners. See inline comment for details.
- blocking:
enrolledLearnerIdsunused — learners added after session creation still appear (useAttendanceForm.js:36) - blocking: Duplicate element IDs when both current and previously enrolled learners exist (AttendanceFormTable.vue)
- suggestion: New i18n strings on non-default branch (attendanceStrings.js)
- suggestion: Duplicate previously-enrolled row template (AttendanceFormTable.vue)
- praise: Good use of
F()annotations foruser_name/user_username— historical records survive learner removal - praise: Thorough test coverage for edge cases (no learners, all removed, negative counts)
@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
| @@ -36,6 +38,10 @@ export default function useAttendanceForm({ hasChanges, markClean, submitting, o | |||
| return [...learners].sort((a, b) => localeCompare(a.name, b.name)); | |||
There was a problem hiding this comment.
blocking: sortedLearners returns all learners from the store without filtering by enrolledLearnerIds. The setEnrolledLearnerIds function stores the IDs (line 42) but nothing reads them, so learners added to the class after a session was created still appear on the edit page. This is the root cause of the CI failure (does not show learners added after the session was created).
The fix is to filter sortedLearners when enrolledLearnerIds is set:
const sortedLearners = computed(() => {
const learners = store.getters['classSummary/learners'] || [];
const filtered = enrolledLearnerIds.value
? learners.filter(l => enrolledLearnerIds.value.has(l.id))
: learners;
return [...filtered].sort((a, b) => localeCompare(a.name, b.name));
});This preserves the current behavior for AttendanceNewPage (where enrolledLearnerIds remains null) while filtering out late additions on the edit page.
| :style="{ borderTop: `1px solid ${$themeTokens.fineLine}` }" | ||
| > | ||
| <span | ||
| :id="`learner-name-removed-${learner.id}`" |
There was a problem hiding this comment.
blocking: When both current and previously enrolled learners exist, the v-else-if block (lines 84-125) is skipped and the v-if block at line 128 renders the previously enrolled rows instead. Both blocks use the same :id="learner-name-removed-${learner.id}" template, which means duplicate element IDs will appear in the DOM if a learner somehow appears in both. More importantly, the v-else-if block is dead code when current learners exist — this is fine architecturally, but the duplicated template between lines 84-125 and 127-160 means any future change to one must be mirrored in the other.
Consider extracting the previously-enrolled row into a small helper component or a scoped slot to avoid the duplication.
There was a problem hiding this comment.
duplicate element IDs will appear in the DOM if a learner somehow appears in both
the learner cannot appear in both
| message: 'Absent', | ||
| context: 'Column header for the count of learners absent', | ||
| }, | ||
| previouslyEnrolledLabel: { |
There was a problem hiding this comment.
suggestion: This PR adds two new i18n strings (previouslyEnrolledLabel, noLearnersInClassMessage) but targets release-v0.19.x rather than the default branch (develop). New translatable strings typically need to land on the default branch so translators can pick them up in the normal workflow. Is this intentional for the release timeline?
There was a problem hiding this comment.
yes it is intentional
|
|
||
| def annotate_queryset(self, queryset): | ||
| return queryset.annotate( | ||
| user_name=F("user__full_name"), |
There was a problem hiding this comment.
praise: Good use of F() expressions to annotate user_name and user_username directly from the related FacilityUser. This ensures historical attendance records preserve the learner's name even after they're removed from the class, without adding redundant stored fields.
| // Bob joined after this session — should not appear at all | ||
| expect(screen.queryByText('Bob')).not.toBeInTheDocument(); | ||
| expect(screen.queryByText('Bob (Previously enrolled)')).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
praise: Good test coverage for the edge cases — the tests for no-learners, all-removed, previously-enrolled counts, negative count prevention, and late additions are thorough and assert the right behaviors.
| class="previously-enrolled-name" | ||
| :style="{ color: $themeTokens.annotation }" | ||
| > | ||
| {{ learner.name }} {{ previouslyEnrolledLabel$() }} |
There was a problem hiding this comment.
suggestion: The previously enrolled name is constructed by concatenating {{ learner.name }} {{ previouslyEnrolledLabel$() }} which produces e.g. Alice (Previously enrolled). In some locales, the placement of the suffix relative to the name may need to differ. Consider using a single i18n string with an interpolation parameter for the name, e.g. '{name} (Previously enrolled)', so translators have full control over the format.
marcellamaki
force-pushed
the
update-attendance-history-and-enrollement-management
branch
from
1bcd9a5 to
e05ec09
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
Resolved:
enrolledLearnerIdsunused insortedLearners(blocking) — on re-examination, this finding was incorrect:useAttendanceForm.js:39does consumeenrolledLearnerIdsvia.filter(l => enrolledLearnerIds.value === null || enrolledLearnerIds.value.has(l.id)), and the testdoes not show learners added after the session was createdverifies this behavior. Withdrawing.- String interpolation for
previouslyEnrolledLabel(suggestion, implicit) — commit 2007cca moved{name}inside the translation string, giving translators control over placement. Good fix.
Contested (not re-raised):
- Duplicate element IDs in
AttendanceFormTable.vue(blocking) — author confirmed learners cannot appear in both lists.
Acknowledged (not re-raised):
- New i18n strings on non-default branch (suggestion) — author confirmed intentional for the release timeline.
1/1 prior blocking finding withdrawn (was incorrect). 0 re-raised.
CI passing (all test checks green, build artifacts still in progress). Code is clean — the loadData flow in AttendanceEditPage correctly partitions records into current vs previously-enrolled, and the enrolledLearnerIds filter properly hides learners added after the session. The F() annotations, count arithmetic, and edge-case handling (no learners, all removed, negative counts) are all solid.
@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
| context: 'Column header for the count of learners absent', | ||
| }, | ||
| previouslyEnrolledLabel: { | ||
| message: '{name} (Previously enrolled)', |
There was a problem hiding this comment.
praise: Good improvement — moving {name} inside the translation string (rather than concatenating outside) gives translators full control over component ordering across languages. This is exactly how i18n interpolation should work.
|
|
||
| // Bob joined after this session — should not appear at all | ||
| expect(screen.queryByText('Bob')).not.toBeInTheDocument(); | ||
| expect(screen.queryByText('Bob (Previously enrolled)')).not.toBeInTheDocument(); |
There was a problem hiding this comment.
praise: This test precisely validates the enrolledLearnerIds filtering — it catches the case where a learner (Bob) is in the current class but has no attendance record for the session, confirming they're correctly excluded from the edit page.
Build Artifacts
Smoke test screenshot |
|
Implemented according to above specification. 2 learners removed - records not editable
2 learners re-enrolled - records reinstated and editable
2 new learners enrolled - hidden from past attendance record
All learners removed from class - all records available and not editable
|
…ve more accurate insight into currently and previously enrolled learners over time
marcellamaki
force-pushed
the
update-attendance-history-and-enrollement-management
branch
from
0979011 to
b4404a7
Compare
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Did further manual QA after the slight simplification tweaks, all seems good to me.
4 participants
Summary
Fixes #14413
This PR adds an annotation to attendance history, so users can be tracked over time even when their membership in a given class changes.
Reviewer guidance
I think there is still room for further improvement and different permutations, but for the near term, I think this covers more of the likely edge cases around learner unenrollment and "late additions" to the class.
Editing Attendance:
Attendance History:
New attendance session:
The "Mark attendance" form for new sessions shows only current enrollees (this is not changed)
NOTE: this is not rebased off of Samson's PR to remove the "mark attendance" button if no learners, so you can get to a blank new attendance page for no learners enrolled (but that will be resolved by his PR)
References
Here are two screencasts showing the updated UI:
Screen.Recording.2026-03-19.at.2.53.58.PM.mov
Screen.Recording.2026-03-19.at.2.54.58.PM.mov
AI usage
Written with claude, code reviewed and manually QA'd. I maybe have been a bit overly permissive with the claude comments I have retained, as i think they might be helpful in explaining some of the scenarios. I have done some comment clean up.