Hide mark attendance button when no learners are enrolled#14419
Hide mark attendance button when no learners are enrolled#14419rtibbles merged 5 commits intolearningequality:release-v0.19.xfrom
Conversation
github-actions
bot
added
DEV: dev-ops
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Code changes look correct to me - and I have just noticed that the bot implemented the attendance block tests using vue test utils, not vue test library! Will open a follow up issue in develop to rectify this once we've merged down.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Straightforward fix that gates the button behind learners.length — the core logic is correct.
CI passing. Screenshots provided and consistent with code changes.
- blocking: New translatable string
noSessionsEnrollMessageadded onrelease-v0.19.x(non-default branch), and adding strings was not specified in #14412. See inline comment. - suggestion: Missing test assertion for button visibility — see inline comment.
- praise: Clean test setup wiring the Vuex
classSummarymodule with learner data — see inline comment.
@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
| message: 'No attendance sessions yet. Enroll learners to mark attendance', | ||
| context: | ||
| 'Empty state message when no attendance sessions exist and no learners are enrolled in the class', | ||
| }, |
There was a problem hiding this comment.
blocking: This PR adds a new translatable string (noSessionsEnrollMessage) but targets release-v0.19.x, not the default branch (develop). New strings on release branches can disrupt the i18n/translation pipeline since string extraction and Crowdin syncs typically run against the default branch.
The linked issue (#14412) doesn't call for a new message — its expected behavior is: "the 'Attendance' section to be present with just the text 'No attendance sessions yet'." Consider dropping this new string and reusing the existing noSessionsMessage for both cases (matching the issue's stated expectation). If the distinct message is desired, it should land on develop first and be cherry-picked after translations are in place.
There was a problem hiding this comment.
This is fine, this feature is gated to English only, so untranslated strings will not appear.
| const { wrapper } = makeWrapper({ sessions: [], learners: [] }); | ||
| await global.flushPromises(); | ||
| expect(wrapper.text()).toContain('No attendance sessions yet'); | ||
| expect(wrapper.text()).toContain('Enroll learners to mark attendance'); |
There was a problem hiding this comment.
suggestion: The primary acceptance criterion from #14412 is that the "Mark attendance" button is hidden when no learners are enrolled, but no test asserts this. Consider adding an assertion that the KRouterLink stub is absent when learners is empty, e.g.:
expect(wrapper.findComponent({ name: 'KRouterLink' }).exists()).toBe(false);And conversely, that it's present when learners exist.
| learners.forEach(l => { | ||
| learnerMap[l.id] = l; | ||
| }); | ||
| testStore.state.classSummary.learnerMap = learnerMap; |
There was a problem hiding this comment.
praise: Good job wiring up the classSummary module and learnerMap in the test store — this ensures the component's computed(() => store.getters['classSummary/learners']) works correctly in tests without over-mocking.
Build Artifacts
Smoke test screenshot |
|
Looking good in Firefox and Chrome on Ubuntu.
0 learners, no button, 1+ learner button appears 🙂 👏🏽 |
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Bot does not have access to the full facts.
4 participants
Summary
Fixes a bug where the "Mark attendance" button was visible on the class home page even when no learners were enrolled in the class. This is inconsistent with the behavior of other blocks on the same page, which hide their action buttons when there's no data to act on.
Changes:
v-if="learners.length"check so it only renders when the class has enrolled learners"No attendance sessions yet. Enroll learners to mark attendance") instead of"No attendance sessions yet"when there are zero learnersReferences
Fixes #14412
Reviewer guidance
AI usage
I used Claude Code to implement this change, guiding it to use the existing
learnerscomputed fromclassSummary/learnersin the Vuex store and wrap the button in av-ifconditional. I reviewed the generated code, verified the logic against the issue description, and confirmed the fix works in both the empty-class and populated-class scenarios.