Skip to content

SAK-52173 SCORM Import from site does not retain GB setting #14257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+44 −0
Draft

SAK-52173 SCORM Import from site does not retain GB setting #14257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .../scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/ScormEntityProducer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.sakaiproject.exception.PermissionException;
import org.sakaiproject.exception.ServerOverloadException;
import org.sakaiproject.exception.TypeException;
import org.sakaiproject.grading.api.GradingService;
import org.sakaiproject.scorm.api.ScormConstants;
import org.sakaiproject.scorm.dao.api.ContentPackageDao;
import org.sakaiproject.scorm.dao.api.ContentPackageManifestDao;
Expand Down Expand Up @@ -95,6 +96,10 @@ public class ScormEntityProducer implements EntityProducer, EntityTransferrer, H
private SecurityService securityService;
private SessionManager sessionManager;

protected GradingService gradingService() {
return null; // Overridden by Spring lookup-method
}

public void init() {
try {
entityManager.registerEntityProducer(this, REFERENCE_ROOT);
Expand Down Expand Up @@ -177,6 +182,9 @@ public Map<String, String> transferCopyEntities(String fromContext, String toCon

recordReferenceMapping(transversalMap, fromContext, toContext, source, copy);
updateCollectionDisplayName(newResourceId, copy.getTitle());

// Copy gradebook settings for SCOs
copyGradebookSettings(fromContext, toContext, source, copy);
} catch (Exception e) {
log.error("Failed to copy SCORM package {} from {} to {}", source.getContentPackageId(), fromContext, toContext, e);
}
Expand Down Expand Up @@ -347,6 +355,41 @@ private void updateCollectionDisplayName(String resourceId, String title) {
}
}

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());
}
}
}
Comment on lines +358 to +391
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 19, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Harden gradebook copy against null manifest ids and verify addExternalAssessment signature

Two points here:

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

  1. Confirm GradingService.addExternalAssessment argument 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.

Suggested change
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.


private <T> T runWithAdvisor(Callable<T> task) throws Exception {
SecurityAdvisor advisor = createScormContentAdvisor();
securityService.pushAdvisor(advisor);
Expand Down
1 change: 1 addition & 0 deletions ...scorm-impl/service/src/java/org/sakaiproject/scorm/service/impl/spring-scorm-services.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
<bean id="org.sakaiproject.scorm.service.impl.ScormEntityProducer"
class="org.sakaiproject.scorm.service.impl.ScormEntityProducer"
init-method="init">
<lookup-method name="gradingService" bean="org.sakaiproject.grading.api.GradingService" />
<property name="entityManager" ref="org.sakaiproject.entity.api.EntityManager" />
<property name="scormContentService" ref="org.sakaiproject.scorm.service.api.ScormContentService" />
<property name="contentPackageDao" ref="org.sakaiproject.scorm.dao.api.ContentPackageDao" />
Expand Down
Loading