Skip to content

Conversation

@hornersa
Copy link
Contributor

@hornersa hornersa commented Nov 11, 2025

Jira: https://sakaiproject.atlassian.net/browse/SAK-52145

Summary by CodeRabbit

  • Bug Fixes
    • Restored handling for publishing assessments to selected groups so release-to-group behavior works correctly.
  • Documentation
    • Updated the user-facing message shown when no group is selected to clearly prompt the author to choose a group before publishing.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 11, 2025

Walkthrough

Replaces a null check with MapUtils.isEmpty for release-to-groups, restores a commented RELEASE_TO_SELECTED_GROUPS switch case, and adds the MapUtils import in a Java source file. Updates an author-facing message text for no_selected_groups_error in a properties file.

Changes

Cohort / File(s) Change Summary
AssessmentFacadeQueries update
samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/facade/AssessmentFacadeQueries.java
Added org.apache.commons.collections4.MapUtils import; replaced manual null/empty check with MapUtils.isEmpty(releaseToGroups); reinstated RELEASE_TO_SELECTED_GROUPS case label(s) with empty break to match existing switch structure.
Author message text update
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/bundle/AuthorFrontDoorMessages.properties
Updated no_selected_groups_error message value from a deletion-warning to a prompt: "This assessment was released to a group, but no group is selected. Please select a group to publish this assessment."

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: restoring behavior to display duplicated group-released assessments without automatically changing the 'Released To' setting to Entire site, addressing the Jira issue SAK-52145.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f24f4fa and 6ca6119.

📒 Files selected for processing (2)
  • samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/bundle/AuthorFrontDoorMessages.properties (1 hunks)
  • samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/facade/AssessmentFacadeQueries.java (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/facade/AssessmentFacadeQueries.java
🧰 Additional context used
🧠 Learnings (4)
📓 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-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages

Applied to files:

  • samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/bundle/AuthorFrontDoorMessages.properties
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`

Applied to files:

  • samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/bundle/AuthorFrontDoorMessages.properties
📚 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/bundle/AuthorFrontDoorMessages.properties
⏰ 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)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/bundle/AuthorFrontDoorMessages.properties (1)

46-46: Message change clarifies group selection requirement for duplicated assessments.

The error message now indicates that a group-specific assessment requires explicit group selection before publishing, rather than blaming deleted groups. This is more actionable for instructors duplicating group-constrained assessments.

Verify that Java code paths displaying this message (no_selected_groups_error) handle the group-specific duplication scenario correctly and that any localized variants of AuthorFrontDoorMessages.properties are updated consistently.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@jesusmmp jesusmmp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is well commented
// case AssessmentAccessControl.RELEASE_TO_SELECTED_GROUPS:

Duplicate assessments should be published to the Entire site

@hornersa
Copy link
Contributor Author

This line is well commented // case AssessmentAccessControl.RELEASE_TO_SELECTED_GROUPS:

Duplicate assessments should be published to the Entire site

Hello, @jesusmmp. Noting that this PR partly reverts a related prior commit, this reversion is nevertheless intentional for the following reason. The current behavior in Sakai 26/25 (without this PR) is especially problematic for course sites where two or more distinct groups represent different lecture meeting times (e.g., MWF vs Tues. & Thurs.) and thus different deadlines for assessments, assignments, etc. For such sites, assessments released to ‘Entire site’ should never (or only rarely) occur.

The cited lines where I introduce a break for AssessmentAccessControl.RELEASE_TO_SELECTED_GROUPS is necessary for the duplicate to display in the Assessments table with the warning that's cited in the jira.

ExpectedForColumnErrorMsginSakai23

While one could instead argue that duplicating a group-released assessment should copy the group attributions also, the scope of this PR is merely to restore behavior in Sakai 22/23 which I think is more expected, especially for the aforementioned scenario where 'Entire site' is problematic.

Should we tag the jira for TL/UX to review and decide? Let me know how you think we should proceed.

@jesusmmp
Copy link
Contributor

Hello @hornersa, maybe I'm missing something...

When you duplicate an assessment assign to group(s), the new copy should be published to the Entire site.

Trunk is working fine
image

On 22 is not working fine
image

Yes, you can talk with the TL/UX group. Thanks!

@ern
Copy link
Contributor

ern commented Nov 12, 2025

Also should consider the semantics of site import as well. Here are the rule of thumbs tools abide by:

  • When duplicating/importing changing assessment settings is generally not ideal
  • If a setting would cause some issue where auto publishing could/should not be done (i.e. groups missing, or empty groups) then don't publish (leave in draft) and display message
  • the only time you would auto publish is when the assessment is identical and no issue's exist

@hornersa
Copy link
Contributor Author

hornersa commented Nov 12, 2025

Hello @hornersa, maybe I'm missing something...

When you duplicate an assessment assign to group(s), the new copy should be published to the Entire site.

Trunk is working fine image

On 22 is not working fine image

Yes, you can talk with the TL/UX group. Thanks!

I think we disagree on end results, as I deem 22.x working correctly and 26.x as not. I'll try to reword the scenario I'm trying to address which is common at our institution.

An instructor is teaching 40 students. 20 in Group 1 meet only on Monday, Wednesday, and Friday. The other 20 in Group 2 meet on Tuesday and Thursday. Therefore, all their assessments in Tests & Quizzes have separate due dates, etc. but are otherwise identical. The instructor creates Quiz 1 for Group 1. To create Quiz 1 for Group 2, the simplest method would be to duplicate the Quiz 1 - Group 1 draft. In doing so, 26.x quietly changes the RELEASED TO value to 'Entire Site'. Not warned about this happening (or any need to change the group) the instructor forgets to alter the RELEASED TO and related values and just renames the assessment to "Quiz 1 - Group 2" and then publishes. Now Group 1 students are confused about which assessment to take. Some take the Group 1 and some take the Group 2. Also, the Gradebook likely needs attention so that some attention as well given this inadvertent mix-up.

All of this is to say is that the error as presented in Sakai 22/23 is beneficial for instructors to pay attention to the group settings (i.e., add the intended group) before publishing. (Arguably, the error message in English could use some better/clearer wording.)

I'll update the jira to reflect this and request a TL/UX review.

@hornersa
Copy link
Contributor Author

hornersa commented Dec 8, 2025

The TL/UX group has reviewed this jira, per Wilma's comment. I'm also incorporating a change in a new related jira Wilma mentions in the aforementioned comment.

@github-actions
Copy link

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.

@github-actions github-actions bot added the stale label Dec 30, 2025
@github-actions
Copy link

github-actions bot commented Jan 6, 2026

This PR has been automatically closed due to inactivity after being marked as stale. You may simply reopen it and continue working on it.

@github-actions github-actions bot closed this Jan 6, 2026
@hornersa hornersa reopened this Jan 6, 2026
@github-actions github-actions bot removed the stale label Jan 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants