-
-
Notifications
You must be signed in to change notification settings - Fork 998
SAK-51555 Assignment disable grading for no student submission #14297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughA Velocity template file for instructor assignment submissions was modified to introduce a Changes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
assignment/tool/src/webapp/vm/assignment/chef_assignments_instructor_list_submissions.vm (3)
454-459: Tighten status handling and double‑checkNO_SUBMISSIONsemantics
$submissionStatusis set but never used; you still call$service.getSubmissionStatus($!submissionId, true)again in the status column at Line 543. Either reuse$submissionStatusthere or drop this#setto avoid confusion and the extra service call in the row loop.- Comparing
canonicalStatus.name()to the literal"NO_SUBMISSION"in the template is brittle. If the enum constant ever changes, grading silently re‑enables. If there’s a more robust check (e.g. a dedicatedisNoSubmissionhelper or server‑side boolean), consider pushing this logic into the Java layer and exposing a simple$canGradeflag to the view instead.- Please regression‑test edge cases like non‑electronic/offline assignments and any states where instructors are expected to grade without an online submission; gating on
NO_SUBMISSIONmay unintentionally hide grade links in those scenarios, similar to how over‑restrictive render checks previously hid mailto links in Samigo (SAK‑49674 / SAK‑52058). Based on learnings, …
485-515: Centralized$canGradegating is good; consider consistency with other grading actionsUsing
$canGradein both the taggable and non‑taggable branches correctly centralizes the “can I grade this row?” decision and aligns with the PR goal of disabling grading for canonicalNO_SUBMISSIONstates.Two follow‑ups to consider:
- Other grading actions above (default grade form, upload‑all, release‑grades) are still gated only by
$disableGrading. If the requirement is “no grading at all when there is no student submission”, you may want to reuse$canGrade(or a related flag) around those bulk actions for consistency.- When
$canGradeis false but$disableGradingis false, instructors will see a normal status of “No submission” and a non‑clickable name. That’s functionally correct but a bit implicit; if you get support tickets about “missing” grade links, adding a small tooltip or help text explaining that grading is disabled until a submission exists could help.These are behavioral/UX refinements rather than blockers.
648-672: No‑submission row remains read‑only; table structure could be made more consistentThe “no submission yet” row continues to render as a simple, non‑clickable display, which matches the new intent to prevent grading when there’s no student work.
One minor structural nit: when peer assessment and/or review‑service columns are present, this row still only outputs the attachment/student/submitted/status/grade(/estimate)/released cells, so its
<td>count can differ from the header. Browsers handle this, but for cleaner markup (and better accessibility tooling) you might consider adding empty<td>s for any optional columns that are active in the header.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assignment/tool/src/webapp/vm/assignment/chef_assignments_instructor_list_submissions.vm(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
assignment/tool/src/webapp/vm/assignment/chef_assignments_instructor_list_submissions.vm
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
assignment/tool/src/webapp/vm/assignment/chef_assignments_instructor_list_submissions.vm
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
assignment/tool/src/webapp/vm/assignment/chef_assignments_instructor_list_submissions.vm
🧠 Learnings (2)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
📚 Learning: 2025-10-07T15:11:27.298Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Applied to files:
assignment/tool/src/webapp/vm/assignment/chef_assignments_instructor_list_submissions.vm
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (1)
assignment/tool/src/webapp/vm/assignment/chef_assignments_instructor_list_submissions.vm (1)
467-483: Bootstrap badge usage for group members looks goodThe updated badge markup (
class="badge bg-primary fs-6") is consistent with existing Bootstrap 5 usage and keeps the group member list visually distinct without affecting responsiveness.
|
This PR has been automatically marked as stale due to 21 days of inactivity. It will be closed in 7 days unless there is new activity. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.