SAK-52173 SCORM Import from site does not retain GB setting #14257
Conversation
WalkthroughThe PR extends ScormEntityProducer to copy gradebook settings when copying SCORM packages between contexts. It adds gradebook-related imports and a new copyGradebookSettings method that iterates through SCO items in package manifests and creates external assessments in the destination using GradingService. Spring configuration is updated to inject GradingService via lookup-method into relevant beans. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/ScormEntityProducer.java(4 hunks)scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/spring-scorm-services.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T04:03:17.598Z
Learnt from: kunaljaykam
Repo: sakaiproject/sakai PR: 13974
File: syllabus/syllabus-impl/src/webapp/WEB-INF/components.xml:80-90
Timestamp: 2025-09-16T04:03:17.598Z
Learning: In Sakai's search infrastructure, content producers like SyllabusContentProducer don't require explicit destroy-method configuration in Spring XML. The search framework handles cleanup automatically, and adding destroy-method when no destroy() method exists would cause configuration errors.
Applied to files:
scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/spring-scorm-services.xml
⏰ 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 (3)
scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/ScormEntityProducer.java (2)
99-101:gradingService()lookup-method stub is appropriate but note direct instantiationDefining a
protected GradingService gradingService()that returnsnullis the usual pattern for Springlookup-methodinjection and matches the XML wiring. Just be aware that any unit tests or callers that constructScormEntityProduceroutside of Spring will seegradingService()asnullunless they subclass/override it.
183-187: Placement ofcopyGradebookSettingsin the copy flow looks correctInvoking
copyGradebookSettings(fromContext, toContext, source, copy)aftercontentPackageDao.save(copy)andupdateCollectionDisplayNameensures the copied package has an assigned id and title before gradebook items are created. This ordering makes sense and doesn’t change existing copy behavior except for the new gradebook side effect.scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/spring-scorm-services.xml (1)
99-111: Lookup-method wiring forgradingServiceinScormEntityProduceris correctly configuredThe
lookup-method name="gradingService"at line 102 matches theprotected GradingService gradingService()method stub (line 99–101 of ScormEntityProducer.java) and aligns with the existing lookup-method pattern used for ScormApplicationServiceImpl at line 14. The bean idorg.sakaiproject.grading.api.GradingServiceis properly registered. No issues detected.
| private void copyGradebookSettings(String fromContext, String toContext, ContentPackage source, ContentPackage copy) { | ||
| GradingService gs = gradingService(); | ||
| if (gs == null) { | ||
| return; | ||
| } | ||
|
|
||
| ContentPackageManifest manifest = contentPackageManifestDao.load(copy.getManifestId()); | ||
| if (manifest == null || manifest.getLaunchData() == null) { | ||
| return; | ||
| } | ||
|
|
||
| for (LaunchData launchData : manifest.getLaunchData()) { | ||
| if (!"sco".equalsIgnoreCase(launchData.getSCORMType())) { | ||
| continue; | ||
| } | ||
|
|
||
| String itemIdentifier = launchData.getItemIdentifier(); | ||
| String sourceExternalId = source.getContentPackageId() + ":" + itemIdentifier; | ||
| String destExternalId = copy.getContentPackageId() + ":" + itemIdentifier; | ||
|
|
||
| // Only copy if source had gradebook sync enabled | ||
| if (!gs.isExternalAssignmentDefined(fromContext, sourceExternalId)) { | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| String title = StringUtils.defaultIfBlank(copy.getTitle(), launchData.getItemTitle()); | ||
| gs.addExternalAssessment(toContext, toContext, destExternalId, null, title, | ||
| 100.0, copy.getDueOn(), ScormConstants.SCORM_DFLT_TOOL_NAME, null, false, null, null); | ||
| } catch (Exception e) { | ||
| log.debug("Could not copy gradebook item for SCO {}: {}", itemIdentifier, e.getMessage()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Harden gradebook copy against null manifest ids and verify addExternalAssessment signature
Two points here:
- Null manifest id handling
copy.getManifestId() can be null (e.g., when the source had no manifest), but you call contentPackageManifestDao.load(copy.getManifestId()) without a guard. Elsewhere (copyManifest) you explicitly avoid calling load() with a null id, which suggests load(null) may throw. Recommend guarding before the DAO call:
- ContentPackageManifest manifest = contentPackageManifestDao.load(copy.getManifestId());
+ Serializable manifestId = copy.getManifestId();
+ if (manifestId == null) {
+ return;
+ }
+
+ ContentPackageManifest manifest = contentPackageManifestDao.load(manifestId);This keeps gradebook copying best-effort and avoids unexpected runtime failures when a package lacks a manifest.
- Confirm
GradingService.addExternalAssessmentargument ordering and values
The call
gs.addExternalAssessment(toContext, toContext, destExternalId, null, title,
100.0, copy.getDueOn(), ScormConstants.SCORM_DFLT_TOOL_NAME, null, false, null, null);assumes a specific 12-parameter signature and passes toContext twice, with ScormConstants.SCORM_DFLT_TOOL_NAME later in the list. Without the API in view, it’s hard to confirm that the second toContext and the position of SCORM_DFLT_TOOL_NAME align with the expected (gradebookUid, externalAppName, externalId, ...) ordering.
Please verify this against your GradingService interface and, if available, mirror the argument pattern used by other tools that create SCORM or external assessments, so points, due date, category, and release flags all line up as intended.
Also consider whether 100.0 should match the original external assignment’s points rather than a hardcoded value.
To validate both the signature and typical usage patterns in your codebase, you can run:
🏁 Script executed:
#!/bin/bash
# Inspect all usages of addExternalAssessment to confirm argument order and typical values
rg -n 'addExternalAssessment\(' -C3
# Optionally, inspect the GradingService interface to confirm the exact method signature
fd 'GradingService.java' | xargs -I{} sed -n '1,260p' "{}" | sed -n '/addExternalAssessment/,/);/p'Length of output: 28601
Null manifest id handling needs defensive guard; addExternalAssessment signature is correct
The GradingService.addExternalAssessment call is correctly formed. Verification confirms the 12-parameter signature at gradebookng/api/src/main/java/org/sakaiproject/grading/api/GradingService.java matches your usage, and passing toContext for both gradebookUid and siteId is consistent with other tools in the codebase (assignments, other SCORM usages).
However, the null manifest id issue remains valid:
Guard copy.getManifestId() before the DAO call (lines 363-364):
- ContentPackageManifest manifest = contentPackageManifestDao.load(copy.getManifestId());
+ Serializable manifestId = copy.getManifestId();
+ if (manifestId == null) {
+ return;
+ }
+
+ ContentPackageManifest manifest = contentPackageManifestDao.load(manifestId);This avoids potential DAO errors when a source package has no manifest, keeping gradebook copying best-effort and defensive.
Regarding the hardcoded 100.0 points: consider whether this should inherit from the source package's original external assignment rather than using a fixed value, to preserve grading consistency across site copies.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void copyGradebookSettings(String fromContext, String toContext, ContentPackage source, ContentPackage copy) { | |
| GradingService gs = gradingService(); | |
| if (gs == null) { | |
| return; | |
| } | |
| ContentPackageManifest manifest = contentPackageManifestDao.load(copy.getManifestId()); | |
| if (manifest == null || manifest.getLaunchData() == null) { | |
| return; | |
| } | |
| for (LaunchData launchData : manifest.getLaunchData()) { | |
| if (!"sco".equalsIgnoreCase(launchData.getSCORMType())) { | |
| continue; | |
| } | |
| String itemIdentifier = launchData.getItemIdentifier(); | |
| String sourceExternalId = source.getContentPackageId() + ":" + itemIdentifier; | |
| String destExternalId = copy.getContentPackageId() + ":" + itemIdentifier; | |
| // Only copy if source had gradebook sync enabled | |
| if (!gs.isExternalAssignmentDefined(fromContext, sourceExternalId)) { | |
| continue; | |
| } | |
| try { | |
| String title = StringUtils.defaultIfBlank(copy.getTitle(), launchData.getItemTitle()); | |
| gs.addExternalAssessment(toContext, toContext, destExternalId, null, title, | |
| 100.0, copy.getDueOn(), ScormConstants.SCORM_DFLT_TOOL_NAME, null, false, null, null); | |
| } catch (Exception e) { | |
| log.debug("Could not copy gradebook item for SCO {}: {}", itemIdentifier, e.getMessage()); | |
| } | |
| } | |
| } | |
| private void copyGradebookSettings(String fromContext, String toContext, ContentPackage source, ContentPackage copy) { | |
| GradingService gs = gradingService(); | |
| if (gs == null) { | |
| return; | |
| } | |
| Serializable manifestId = copy.getManifestId(); | |
| if (manifestId == null) { | |
| return; | |
| } | |
| ContentPackageManifest manifest = contentPackageManifestDao.load(manifestId); | |
| if (manifest == null || manifest.getLaunchData() == null) { | |
| return; | |
| } | |
| for (LaunchData launchData : manifest.getLaunchData()) { | |
| if (!"sco".equalsIgnoreCase(launchData.getSCORMType())) { | |
| continue; | |
| } | |
| String itemIdentifier = launchData.getItemIdentifier(); | |
| String sourceExternalId = source.getContentPackageId() + ":" + itemIdentifier; | |
| String destExternalId = copy.getContentPackageId() + ":" + itemIdentifier; | |
| // Only copy if source had gradebook sync enabled | |
| if (!gs.isExternalAssignmentDefined(fromContext, sourceExternalId)) { | |
| continue; | |
| } | |
| try { | |
| String title = StringUtils.defaultIfBlank(copy.getTitle(), launchData.getItemTitle()); | |
| gs.addExternalAssessment(toContext, toContext, destExternalId, null, title, | |
| 100.0, copy.getDueOn(), ScormConstants.SCORM_DFLT_TOOL_NAME, null, false, null, null); | |
| } catch (Exception e) { | |
| log.debug("Could not copy gradebook item for SCO {}: {}", itemIdentifier, e.getMessage()); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
scormplayer/scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/ScormEntityProducer.java
around lines 358 to 391, add a defensive null-check for copy.getManifestId()
before calling contentPackageManifestDao.load(...); currently the code calls
load(copy.getManifestId()) without guarding against a null manifest id which can
cause DAO errors — if copy.getManifestId() is null, skip gradebook copying
(return or continue) to keep the process best-effort; additionally consider
replacing the hardcoded 100.0 points by retrieving the points value from the
source package’s existing external assessment (if available) and use that when
calling GradingService.addExternalAssessment.
1 participant
Summary by CodeRabbit