SAK-51239 Samigo adjust late acceptance logic when extended time is enabled and retractDate is empty

[hornersa]

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

If the Late Acceptance Date for an exception is blank (null), this means that submissions will be accepted indefinitely for that student or group of students. Thus, for this case, acceptLateSubmission should return true instead of false. This proposed change resolves the cases described in the jira's test plan.

Summary by CodeRabbit

• Bug Fixes
  • Fixed late-submission handling for assessments with extended-time delivery: late submissions are now accepted whenever the assessment has not been retracted. This resolves cases where allowed late submissions were incorrectly blocked due to an overly strict check, ensuring extended-time users can submit when appropriate.

[coderabbitai]

Walkthrough

Updated DeliveryBean.isAcceptLateSubmission() so that when extended-time delivery is active, late submissions are accepted if the assessment is not retracted; previous behavior for non-null retractDate remains preserved.

Changes

Cohort / File(s)	Change Summary
Extended time late-submission check samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java	Modified isAcceptLateSubmission() control flow: under extended-time delivery the acceptance is determined solely by isRetracted(false) (accepted when not retracted) without requiring retractDate to be non-null; existing behavior when retractDate is non-null is retained.

Suggested reviewers

• ottenhoff
• jesusmmp

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 main change: adjusting late acceptance logic in Samigo when extended time is enabled and retractDate is empty.

✨ 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 c7f1dea and fc2db8f.

📒 Files selected for processing (1)

• samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 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.

⏰ 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/ui/bean/delivery/DeliveryBean.java (1)

2289-2298: LGTM! Logic correctly implements the PR objective.

The implementation properly handles the case where retractDate is null (blank) by accepting submissions indefinitely. The logic acceptLateSubmission = !isRetracted(false) correctly evaluates to true when:

1. retractDate is null (accept indefinitely), OR
2. retractDate is non-null but has not yet passed

Since isRetracted() returns retractDate != null && retractDate.before(currentDate), negating it gives the desired behavior via De Morgan's law.

The comment on Line 2294 now accurately reflects the logic after the previous wording fix. This approach is simpler and more correct than using && as discussed in past reviews, since && would incorrectly reject submissions when retractDate is non-null but not yet expired.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[ern]

minor but this ternary is not needed

Suggested change

	acceptLateSubmission = (extendedTimeDeliveryService.getRetractDate() != null) ? !isRetracted(false) : true;
	acceptLateSubmission = extendedTimeDeliveryService.getRetractDate() == null && !isRetracted(false);

[hornersa]

@ern - I'm not sure I concur with the '&&', as I would expect '||' there instead.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java (1)

2295-2295: The || logic is correct, but the null check is redundant.

The current implementation correctly handles all scenarios. However, the explicit null check extendedTimeDeliveryService.getRetractDate() == null is redundant because isRetracted(false) already returns false when retractDate is null (line 2303 checks retractDate != null).

As discussed in the past review comments, using && as suggested would incorrectly reject submissions when a retractDate is set but not yet reached. Your use of || is the correct choice.

For slightly cleaner code, you could simplify to:

-		  acceptLateSubmission = extendedTimeDeliveryService.getRetractDate() == null || !isRetracted(false);
+		  acceptLateSubmission = !isRetracted(false);

This works because isRetracted() returns false when retractDate is null, effectively handling both the null case and the "not yet retracted" case. However, the current explicit null check does make the intent clearer, so this simplification is entirely optional.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2853ee1 and 72f2946.

📒 Files selected for processing (1)

• samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 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.

⏰ 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: sakai-deploy
• GitHub Check: maven-build
• GitHub Check: maven-build

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72f2946 and c7f1dea.

📒 Files selected for processing (1)

• samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 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.

⏰ 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: sakai-deploy
• GitHub Check: maven-build
• GitHub Check: maven-build

🔇 Additional comments (1)

samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/delivery/DeliveryBean.java (1)

2293-2296: Logic correctly implements the PR objective.

The simplified logic !isRetracted(false) correctly handles both cases:

• When retractDate is null: returns true (accept indefinitely)
• When retractDate is not null: returns true if not yet passed, false if passed

This is cleaner than the previous ternary and matches the PR goal of accepting late submissions indefinitely when the extended time Late Acceptance Date is blank.