SAK-51713 Samigo instructors should see all student submissions when a assessment is published to the entire site #14256
Conversation
WalkthroughThis pull request expands assessment authorization functionality by adding new realm functions for assessment permissions and calendar options across database systems, introduces a new authorization constant, and modifies the evaluation bean to handle privileged multi-group access within the assessment system. Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/evaluation/TotalScoresBean.java (1)
926-931: Consider caching the privilege check result.The privilege check is performed on every call to
getEnrollmentListForSelectedSections, which may be invoked multiple times during histogram generation, exports, and score calculations. Since site-level permissions don't change during a user session, consider caching the result as an instance variable.Example refactor:
+ private Boolean hasAllGroupsPrivilegeCache = null; + private List getEnrollmentListForSelectedSections(int calledFrom, String siteId) { List enrollments; - // Check if current user has privilege to assess all groups - if so, they should see all student submissions regardless of group restrictions - boolean hasAllGroupsPrivilege = PersistenceService.getInstance() - .getAuthzQueriesFacade() - .hasPrivilege(SamigoConstants.AUTHZ_ASSESSMENT_ALL_GROUPS, siteId); + // Check if current user has privilege to assess all groups - if so, they should see all student submissions regardless of group restrictions + if (hasAllGroupsPrivilegeCache == null) { + hasAllGroupsPrivilegeCache = PersistenceService.getInstance() + .getAuthzQueriesFacade() + .hasPrivilege(SamigoConstants.AUTHZ_ASSESSMENT_ALL_GROUPS, siteId); + } + boolean hasAllGroupsPrivilege = hasAllGroupsPrivilegeCache;Note: Ensure the cache is cleared in
resetFields()or when the assessment context changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kernel/kernel-impl/src/main/sql/mysql/sakai_realm.sql(3 hunks)kernel/kernel-impl/src/main/sql/oracle/sakai_realm.sql(3 hunks)samigo/samigo-api/src/java/org/sakaiproject/samigo/util/SamigoConstants.java(1 hunks)samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/evaluation/TotalScoresBean.java(3 hunks)
🧰 Additional context used
🧠 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:
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/evaluation/TotalScoresBean.java
🧬 Code graph analysis (1)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/evaluation/TotalScoresBean.java (1)
samigo/samigo-api/src/java/org/sakaiproject/samigo/util/SamigoConstants.java (1)
SamigoConstants(28-213)
⏰ 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: sakai-deploy
- GitHub Check: maven-build
🔇 Additional comments (6)
kernel/kernel-impl/src/main/sql/oracle/sakai_realm.sql (3)
747-747: Grant to maintain on !site.template — LGTM.Matches existing practice (e.g., 'asn.all.groups', 'calendar.all.groups'). No further changes needed here.
916-916: Code change verified and approved — LTI propagation confirmed.The LTI copy mechanism (Oracle lines 1807-1808) confirms that all Instructor functions from
!site.template.course— including the newassessment.all.groupsgrant — automatically flow into!site.template.ltiInstructor and ContentDeveloper roles. Assessment function pruning in LTI is Mentor-only (lines 1822), leaving Instructor/ContentDeveloper unaffected. No duplicate inserts detected.
313-313: New realm function 'assessment.all.groups' verified across Oracle and MySQL with proper role mappings.Cross-database parity confirmed:
- Oracle and MySQL base DDL both include the function insertion and identical role mappings (maintain on !site.template, Instructor on !site.template.course).
- SamigoConstants.java line 152 defines
AUTHZ_ASSESSMENT_ALL_GROUPSand is actively used in TotalScoresBean line 930.- No kernel-specific upgrade scripts exist (standard for Sakai core schema changes; realm adjustments for existing sites are typically manual or bundled with release).
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/evaluation/TotalScoresBean.java (2)
73-75: LGTM! Imports support the new privilege check.The added imports for
SamigoConstantsandPersistenceServiceare necessary for implementing the all-groups privilege check and are correctly placed.
946-951: LGTM! Code correctly implements the privilege scope.The conditional enrollment logic correctly implements the new privilege:
- When users have all-groups privilege and select "All Sections," they see all enrollments without group filtering via
getAvailableEnrollments(false, siteId).- When users lack the privilege, the existing group-release behavior is preserved via
getAllGroupsReleaseEnrollments(siteId).The privilege scope is intentionally limited to the "All Sections" path (lines 946-951) and does not apply to the "Released Sections/Groups" path (line 954). This is correct: when instructors explicitly select "Released Sections/Groups," they expect group filtering regardless of their privileges.
samigo/samigo-api/src/java/org/sakaiproject/samigo/util/SamigoConstants.java (1)
152-152: SQL realm function mappings verified across MySQL and Oracle.The new authorization constant
AUTHZ_ASSESSMENT_ALL_GROUPSis correctly defined inSamigoConstants.javaand the corresponding SQL migration scripts properly introduce and map this function:
kernel/kernel-impl/src/main/sql/mysql/sakai_realm.sqland Oracle equivalent insert the function and map it to both 'maintain' (for!site.templaterealm) and 'Instructor' (for!site.template.courserealm)- Consistent implementation across both database dialects
…ions when a assessment is published to the entire site
ern
changed the title
1 participant
https://sakaiproject.atlassian.net/browse/SAK-51713
sakai-reference: sakaiproject/sakai-reference#278
Summary by CodeRabbit