SAK-52162 Scorm limit bypass when scorm session not properly closed

[kunaljaykam]

Summary by CodeRabbit

• Bug Fixes

  • Launch logic now respects configured maximum attempts, advancing the attempt number when the cap is reached and avoiding reuse of over‑limit attempts.
  • Launch eligibility precomputes the next allowed attempt and returns false for missing content packages to ensure accurate launch decisions.

• New Features

  • Player popup will refresh the opener window when closed (uses a safe pagehide check to avoid bfcache-triggered reloads).

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

[coderabbitai]

Walkthrough

Enforces attempt-number capping when preparing launches, updates canLaunch to compute the next attempt number considering contentPackage.getNumberOfTries(), and injects a DOM-ready script to refresh the opener window when the Scorm player popup closes.

Changes

Cohort / File(s)	Summary
Attempt capping logic scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/ScormLaunchServiceImpl.java	In prepareLaunch's not-exited branch, apply contentPackage.getNumberOfTries() cap: if numberOfTries is set and latestAttempt.getAttemptNumber() >= numberOfTries, increment attemptNumber but do not attach latestAttempt to the session bean; otherwise retain previous attemptNumber and attach latestAttempt.
canLaunch next-attempt computation scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/sakai/impl/SakaiStatefulService.java	canLaunch(ContentPackage) now null-checks the package, respects numberOfTries (using -1 for unlimited), fetches the latest Attempt, computes the next attempt number (reusing latest attempt number if suspended/not-exited, else incrementing), and delegates to canLaunchAttemptInternal with that attempt number.
Popup-close refresh script scormplayer/scorm-tool/src/java/org/sakaiproject/scorm/ui/player/pages/ScormPlayerPage.java	Adds an OnDomReadyHeaderItem script (after loading scorm-rest-launcher.js) that listens for pagehide (ignoring bfcache via event.persisted) and reloads window.opener if present and not closed.

Possibly related PRs

• SAK-52162 SCORM number of attempts allowed not honored #14249 — Modifies ScormLaunchServiceImpl.prepareLaunch attempt-number logic; overlaps changes to number-of-tries and latest-attempt handling.

Suggested reviewers

• ottenhoff

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 identifies the main issue being addressed: fixing a SCORM attempt limit bypass problem that occurs when sessions are not properly closed.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a4306fe and b83f295.

📒 Files selected for processing (2)

• scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/sakai/impl/SakaiStatefulService.java (1 hunks)
• scormplayer/scorm-tool/src/java/org/sakaiproject/scorm/ui/player/pages/ScormPlayerPage.java (2 hunks)

⏰ 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 (2)

scormplayer/scorm-tool/src/java/org/sakaiproject/scorm/ui/player/pages/ScormPlayerPage.java (1)

24-24: LGTM!

The import for OnDomReadyHeaderItem is correctly added to support the DOM-ready script injection.

scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/sakai/impl/SakaiStatefulService.java (1)

82-82: Method name getNewstAttempt is a pre-existing API typo, not introduced by this PR.

The method getNewstAttempt is defined in the ScormResultService interface and consistently used across the codebase (5 call sites). While it appears to be a typo for "Newest" (the underlying implementation calls attemptDao().lookupNewest()), this is a pre-existing public API naming issue, not specific to your changes. Fixing it would require coordinating updates across the interface and all call sites. Consider raising this as a separate refactoring task if desired.

[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 b83f295 and d9dc25d.

📒 Files selected for processing (2)

• scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/sakai/impl/SakaiStatefulService.java (1 hunks)
• scormplayer/scorm-tool/src/java/org/sakaiproject/scorm/ui/player/pages/ScormPlayerPage.java (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 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-push-utils.js : Handle browser-specific Web Push logic in sakai-push-utils.js

Applied to files:

• scormplayer/scorm-tool/src/java/org/sakaiproject/scorm/ui/player/pages/ScormPlayerPage.java

⏰ 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)

scormplayer/scorm-tool/src/java/org/sakaiproject/scorm/ui/player/pages/ScormPlayerPage.java (1)

24-25: Opener-refresh logic via pagehide looks correct and avoids beforeunload pitfalls.

Using pagehide with the !event.persisted check plus window.opener && !window.opener.closed is a solid improvement over beforeunload, and rendering it with OnDomReadyHeaderItem integrates cleanly into Wicket’s head management. This should refresh the parent only when the popup actually goes away (or navigates off) without hammering it on internal interactions.

Also applies to: 117-120