diff --git a/CHANGELOG.md b/CHANGELOG.md index 7faa826..ab59a57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - Fix average score grading, which treated a score of 0 as "no score" and excluded it from the average. - Fix the "Edit on eXeLearning and return to course" button, which failed to send the package back to Moodle. - Fix a `[moodle-exe-bridge] Missing __MOODLE_EXE_CONFIG__` console error logged on every page load. +- Allow uploading SCORM packages without an eXeLearning source, and prevent loss of editable content when saving from the embedded editor. --- diff --git a/classes/exescorm_package.php b/classes/exescorm_package.php index 1604f84..39434c8 100644 --- a/classes/exescorm_package.php +++ b/classes/exescorm_package.php @@ -56,6 +56,32 @@ public static function is_valid_package_file(\stored_file $file) { return false; } + /** + * Whether a package carries the eXeLearning source the embedded editor needs. + * + * The editor re-opens the stored package on the next edit, so a package saved + * through editor/save.php must contain a root `content.xml` (or a legacy + * `contentvN.xml`). An uploaded package does not: it is played, never edited, + * and the "Edit in eXeLearning" button is not offered for it. + * + * @param array $filelist Entries as returned by \stored_file::list_files(). + * @return bool True when a root eXeLearning source file is present. + */ + public static function has_editable_source($filelist) { + if (!is_array($filelist)) { + return false; + } + foreach ($filelist as $info) { + if (!empty($info->is_directory)) { + continue; + } + if (preg_match('/^content(v\d+)?\.xml$/', $info->pathname)) { + return true; + } + } + return false; + } + public static function validate_file_list($filelist) { $errors = []; diff --git a/db/upgrade.php b/db/upgrade.php index e75e958..ef7b514 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -83,5 +83,23 @@ function xmldb_exescorm_upgrade($oldversion) { upgrade_mod_savepoint(true, 2026021201, 'exescorm'); } + // The mandatory-files rule no longer requires an eXeLearning content.xml by + // default: exescorm_validate_package() already enforces SCORM validity (a root + // imsmanifest.xml or an AICC .cst), so the rule only rejected plain SCORM + // packages -- including one eXeLearning produces when the author turns the + // "Editable export" property off (exelearning/exelearning#2415). Such a + // package plays here and simply is not editable. + // + // Only clear the stored value when it is still the old default, byte for byte. + // A site that customised the list keeps its own policy. + if ($oldversion < 2026091400) { + $oldmandatory = '/^content(v\d+)?\.xml$/'; + if (get_config('exescorm', 'mandatoryfileslist') === $oldmandatory) { + set_config('mandatoryfileslist', '', 'exescorm'); + } + + upgrade_mod_savepoint(true, 2026091400, 'exescorm'); + } + return true; } diff --git a/editor/bridge.js b/editor/bridge.js index ac6dc40..56cb0a6 100644 --- a/editor/bridge.js +++ b/editor/bridge.js @@ -278,6 +278,15 @@ var exportResponse = await postToEditor('REQUEST_EXPORT', { format: 'scorm12', filename: 'package.zip', + // This activity stores the SCORM package AS the project: the + // save below is the only copy, and importPackageFromMoodle() + // re-opens it on the next edit. eXeLearning otherwise omits + // the re-editable content.xml when the author turns the + // "Editable export" project property off; editor/save.php + // refuses such a package rather than strand the activity + // with no source to edit. + // See https://github.com/exelearning/exelearning/issues/2415. + options: {forceEditableSource: true}, }, null, 120000); var bytes = exportResponse.bytes; diff --git a/editor/index.php b/editor/index.php index c15eed7..f47f32a 100644 --- a/editor/index.php +++ b/editor/index.php @@ -81,6 +81,24 @@ function exescorm_editor_error_page(string $message): void { require_capability('moodle/course:manageactivities', $context); require_sesskey(); +// The editor imports the stored package and saves it back, so it can only open +// a package that carries an eXeLearning source. A plain SCORM package is a valid +// activity here -- it is played, not edited -- and the view page offers no "Edit +// in eXeLearning" button for one; refuse the direct URL too, with a reason, +// instead of letting the editor load and fail at import time. An activity with +// no package yet is fine: there is nothing to import. +// See https://github.com/exelearning/exelearning/issues/2415. +$fs = get_file_storage(); +$packagefile = $exescorm->reference + ? $fs->get_file($context->id, 'mod_exescorm', 'package', 0, '/', $exescorm->reference) + : false; +if ($packagefile) { + $filelist = $packagefile->list_files(get_file_packer('application/zip')); + if (is_array($filelist) && !\mod_exescorm\exescorm_package::has_editable_source($filelist)) { + exescorm_editor_error_page(get_string('nosourcetoedit', 'mod_exescorm')); + } +} + // Build the package URL for the editor to import. $packageurl = exescorm_get_package_url($exescorm, $context); diff --git a/editor/save.php b/editor/save.php index 390a8cd..c08556e 100644 --- a/editor/save.php +++ b/editor/save.php @@ -17,8 +17,10 @@ /** * AJAX endpoint for saving SCORM packages from the embedded eXeLearning editor. * - * Receives an uploaded SCORM ZIP file, saves it to the package filearea, - * and calls exescorm_parse() to extract content and parse the manifest. + * Receives an uploaded SCORM ZIP file, checks it is a valid SCORM package that + * carries the eXeLearning source the editor needs to re-open it, saves it to the + * package filearea and calls exescorm_parse() to extract content and parse the + * manifest. * * @package mod_exescorm * @copyright 2025 eXeLearning @@ -67,15 +69,14 @@ } $fs = get_file_storage(); - $exescorm->timemodified = time(); - - // Overwrite current package. - $fs->delete_area_files($context->id, 'mod_exescorm', 'package'); + // Stage the upload in the temppackage area, as set_ode.php does, so the + // package the activity already has is untouched if this one is refused. + $fs->delete_area_files($context->id, 'mod_exescorm', 'temppackage'); $fileinfo = [ 'contextid' => $context->id, 'component' => 'mod_exescorm', - 'filearea' => 'package', + 'filearea' => 'temppackage', 'itemid' => 0, 'filepath' => '/', 'filename' => $filename, @@ -84,9 +85,37 @@ 'author' => fullname($USER), 'license' => 'unknown', ]; - $fs->create_file_from_pathname($fileinfo, $uploadedfile['tmp_name']); + $tmpfile = $fs->create_file_from_pathname($fileinfo, $uploadedfile['tmp_name']); + + // The editor re-opens this exact package on the next edit (bridge.js, + // importPackageFromMoodle), so besides being a valid SCORM package it must + // carry the eXeLearning source. mod_form.php skips validation for the + // embedded type, so this is the only gate on that path. The editor exports + // with forceEditableSource so the source check never fires; if it ever + // does, refusing the save is what keeps the activity editable. + // See https://github.com/exelearning/exelearning/issues/2415. + $errors = exescorm_validate_package($tmpfile); + if (empty($errors)) { + $filelist = $tmpfile->list_files(get_file_packer('application/zip')); + if (!\mod_exescorm\exescorm_package::has_editable_source($filelist)) { + $errors['packagefile'] = get_string('nosourcetosave', 'mod_exescorm'); + } + } + if (!empty($errors)) { + $tmpfile->delete(); + http_response_code(400); + echo json_encode(['success' => false, 'error' => reset($errors)]); + exit; + } + + // Overwrite current package. + $fs->delete_area_files($context->id, 'mod_exescorm', 'package'); + $fileinfo['filearea'] = 'package'; + $fs->create_file_from_storedfile($fileinfo, $tmpfile); + $fs->delete_area_files($context->id, 'mod_exescorm', 'temppackage'); // Keep package name in SCORM reference and trigger re-parse. + $exescorm->timemodified = time(); $exescorm->reference = $filename; $DB->update_record('exescorm', $exescorm); exescorm_parse($exescorm, true); diff --git a/lang/ca/exescorm.php b/lang/ca/exescorm.php index 11a7782..7ea8954 100644 --- a/lang/ca/exescorm.php +++ b/lang/ca/exescorm.php @@ -524,6 +524,8 @@ $string['info'] = 'Info'; $string['displayactivityname'] = 'Mostra el nom de l\'activitat'; $string['displayactivityname_help'] = 'Si cal mostrar o no el nom de l\'activitat al damunt del reproductor eXeLearning'; +$string['nosourcetoedit'] = 'El paquet d\'aquesta activitat no conté un projecte d\'eXeLearning, per la qual cosa no es pot obrir a l\'editor integrat. Continua funcionant com a paquet SCORM.'; +$string['nosourcetosave'] = 'El paquet exportat no conté un projecte d\'eXeLearning, per la qual cosa no s\'ha desat. L\'activitat conserva el paquet anterior.'; $string['elpxneedsconversion'] = 'Aquesta activitat conté actualment un projecte .elpx. Obriu-lo a l\'editor eXeLearning integrat i deseu-lo per convertir-lo en un paquet SCORM 1.2 reproduïble i rastreable.'; $string['packageempty'] = 'Aquesta activitat encara no conté contingut reproduïble. Obriu-la a l\'editor d\'eXeLearning per afegir contingut.'; diff --git a/lang/en/exescorm.php b/lang/en/exescorm.php index eecea8e..0aab495 100644 --- a/lang/en/exescorm.php +++ b/lang/en/exescorm.php @@ -518,6 +518,8 @@ $string['info'] = 'Info'; $string['displayactivityname'] = 'Display activity name'; $string['displayactivityname_help'] = 'Whether or not to display the activity name above the eXeLearning player.'; +$string['nosourcetoedit'] = 'This activity\'s package does not contain an eXeLearning source, so it cannot be opened in the embedded editor. It still works as a SCORM package.'; +$string['nosourcetosave'] = 'The exported package does not contain an eXeLearning source, so it was not saved. The activity keeps its previous package.'; $string['elpxneedsconversion'] = 'This activity currently contains an .elpx project. Open it in the embedded eXeLearning editor and save to convert it to a SCORM 1.2 package for playback and tracking.'; $string['packageempty'] = 'This activity does not contain any playable content yet. Open it in the eXeLearning editor to add content.'; diff --git a/lang/es/exescorm.php b/lang/es/exescorm.php index 6be597a..7f831b4 100644 --- a/lang/es/exescorm.php +++ b/lang/es/exescorm.php @@ -517,6 +517,8 @@ $string['info'] = 'Info'; $string['displayactivityname'] = 'Mostrar el nombre de la actividad'; $string['displayactivityname_help'] = 'Mostrar o no mostrar el nombre de la actividad sobre el visor de eXeLearning.'; +$string['nosourcetoedit'] = 'El paquete de esta actividad no contiene un proyecto de eXeLearning, por lo que no se puede abrir en el editor integrado. Sigue funcionando como paquete SCORM.'; +$string['nosourcetosave'] = 'El paquete exportado no contiene un proyecto de eXeLearning, por lo que no se ha guardado. La actividad conserva su paquete anterior.'; $string['elpxneedsconversion'] = 'Esta actividad contiene actualmente un proyecto .elpx. Abrelo en el editor eXeLearning integrado y guardalo para convertirlo a un paquete SCORM 1.2 reproducible y trazable.'; $string['packageempty'] = 'Esta actividad aún no contiene contenido reproducible. Ábrela en el editor de eXeLearning para añadir contenido.'; diff --git a/lang/eu/exescorm.php b/lang/eu/exescorm.php index c08ac1a..bb41554 100644 --- a/lang/eu/exescorm.php +++ b/lang/eu/exescorm.php @@ -517,6 +517,8 @@ $string['info'] = 'Informazioa'; $string['displayactivityname'] = 'Erakutsi jardueraren izena'; $string['displayactivityname_help'] = 'Erakutsi edo ez erakutsi jardueraren izena eXeLearning-en bisorean.'; +$string['nosourcetoedit'] = 'Jarduera honen paketeak ez du eXeLearning proiekturik, beraz ezin da editore txertatuan ireki. SCORM pakete gisa funtzionatzen jarraitzen du.'; +$string['nosourcetosave'] = 'Esportatutako paketeak ez du eXeLearning proiekturik, beraz ez da gorde. Jarduerak aurreko paketea mantentzen du.'; $string['elpxneedsconversion'] = 'Jarduerak gaur egun .elpx proiektu bat dauka. Ireki ezazu eXeLearning editore txertatuan eta gorde ezazu SCORM 1.2 pakete erreproduziga eta arakagarri batean bihurtzeko.'; $string['packageempty'] = 'Jarduera honek oraindik ez du eduki erreproduzigarririk. Ireki eXeLearning editorean edukia gehitzeko.'; diff --git a/lang/gl/exescorm.php b/lang/gl/exescorm.php index b397edb..e098a28 100644 --- a/lang/gl/exescorm.php +++ b/lang/gl/exescorm.php @@ -517,6 +517,8 @@ $string['info'] = 'Info'; $string['displayactivityname'] = 'Amosar o nome da actividade'; $string['displayactivityname_help'] = 'Amosar ou non o nome da actividade sobre o visor de eXeLearning.'; +$string['nosourcetoedit'] = 'O paquete desta actividade non contén un proxecto de eXeLearning, polo que non se pode abrir no editor integrado. Segue funcionando como paquete SCORM.'; +$string['nosourcetosave'] = 'O paquete exportado non contén un proxecto de eXeLearning, polo que non se gardou. A actividade conserva o seu paquete anterior.'; $string['elpxneedsconversion'] = 'Esta actividade contén actualmente un proxecto .elpx. Ábreo no editor eXeLearning integrado e gárdao para convertelo nun paquete SCORM 1.2 reproducible e rastrexable.'; $string['packageempty'] = 'Esta actividade aínda non contén contido reproducible. Ábrea no editor de eXeLearning para engadir contido.'; diff --git a/set_ode.php b/set_ode.php index 3eba4a6..e924162 100644 --- a/set_ode.php +++ b/set_ode.php @@ -142,6 +142,20 @@ echo json_encode($resultmsg); exit(1); } +// This is the callback of the external eXeOnline flow: exescorm_redirector sends +// the author to exeonlinebaseuri, eXeLearning posts the package back here, and +// get_ode.php hands this exact package back to it on the next edit, so it must +// carry the eXeLearning source. Uploaded packages are only played and are +// deliberately not held to this -- see settings.php mandatoryfileslist. Refuse +// a source-less save rather than strand the activity with nothing to edit. +// editor/save.php runs the same check for the embedded editor. +$packer = get_file_packer('application/zip'); +if (!\mod_exescorm\exescorm_package::has_editable_source($tmpfile->list_files($packer))) { + $tmpfile->delete(); + $resultmsg['description'] = 'KO. Package has no eXeLearning source to edit.'; + echo json_encode($resultmsg); + exit(1); +} // Drop any previous package on the same itemid before moving the validated file in. // We can't keep both because mod_exescorm stores its package at itemid 0 and Moodle // rejects two files sharing the same {itemid, filepath, filename} triple. Deferring this diff --git a/settings.php b/settings.php index 9d8975a..8b7ac02 100644 --- a/settings.php +++ b/settings.php @@ -162,9 +162,18 @@ function toggleConnectionSettings() { get_string('exescorm:sendtemplate', 'mod_exescorm'), get_string('exescorm:sendtemplate_desc', 'mod_exescorm'), 0)); // The eXescorm package validation rules. - $mandatoryfilesre = implode("\n", [ - '/^content(v\d+)?\.xml$/', - ]); + // + // No file is mandatory by default. SCORM validity is enforced separately by + // exescorm_validate_package(), which requires a root imsmanifest.xml (or an + // AICC .cst), so this list is purely a site policy knob: add + // '/^content(v\d+)?\.xml$/' to accept eXeLearning-authored packages only. + // It used to default to that, which rejected every plain SCORM package -- + // including one eXeLearning itself produces when the author turns the + // "Editable export" property off, see + // https://github.com/exelearning/exelearning/issues/2415. Such a package + // plays fine here; it simply cannot be edited, and the "Edit in eXeLearning" + // button is not offered for uploaded packages anyway. + $mandatoryfilesre = ''; $forbiddenfilesre = implode("\n", [ '/.*\.php$/', ]); diff --git a/tests/validatepackage_test.php b/tests/validatepackage_test.php index 3bd1924..d4f4c57 100644 --- a/tests/validatepackage_test.php +++ b/tests/validatepackage_test.php @@ -58,7 +58,7 @@ public function test_validate_package() { $this->resetAfterTest(true); - $filename = "validexescorm.zip"; + $filename = "validscorm.zip"; $file = $this->create_stored_file_from_path($CFG->dirroot.'/mod/exescorm/tests/packages/'.$filename, \file_archive::OPEN); $errors = exescorm_validate_package($file); $this->assertEmpty($errors); @@ -76,7 +76,7 @@ public function test_validate_package() { $this->assertEquals(get_string('nomanifest', 'mod_exescorm'), $errors['packagefile']); } - $filename = "badexescorm.zip"; + $filename = "badscorm.zip"; $file = $this->create_stored_file_from_path($CFG->dirroot.'/mod/exescorm/tests/packages/'.$filename, \file_archive::OPEN); $errors = exescorm_validate_package($file); $this->assertArrayHasKey('packagefile', $errors); @@ -84,5 +84,107 @@ public function test_validate_package() { $this->assertEquals(get_string('badimsmanifestlocation', 'mod_exescorm'), $errors['packagefile']); } } + + /** + * A plain SCORM package, with no eXeLearning source in it, is accepted. + * + * The mandatory-files rule used to default to requiring a root content.xml, + * which rejected every third-party SCORM package and, once eXeLearning + * honoured its own "Editable export" property, eXeLearning's own output too + * (exelearning/exelearning#2415). Such a package is played, never edited, + * so it has nothing to be rejected for. + * + * @covers ::exescorm_validate_package + */ + public function test_validate_package_without_exelearning_source(): void { + global $CFG; + + $this->resetAfterTest(true); + + // The shipped fixture is a stock SCORM package: imsmanifest.xml, no content.xml. + $file = $this->create_stored_file_from_path($CFG->dirroot . '/mod/exescorm/tests/packages/validscorm.zip'); + $this->assertFalse( + exescorm_package::has_editable_source($file->list_files(get_file_packer('application/zip'))), + 'Fixture precondition: validscorm.zip must not carry an eXeLearning source.' + ); + + set_config('mandatoryfileslist', '', 'exescorm'); + $this->assertEmpty(exescorm_validate_package($file)); + } + + /** + * A site may still restrict uploads to eXeLearning-authored packages. + * + * @covers ::exescorm_validate_package + */ + public function test_validate_package_honours_a_site_mandatory_rule(): void { + global $CFG; + + $this->resetAfterTest(true); + + set_config('mandatoryfileslist', '/^content(v\d+)?\.xml$/', 'exescorm'); + + $file = $this->create_stored_file_from_path($CFG->dirroot . '/mod/exescorm/tests/packages/validscorm.zip'); + $errors = exescorm_validate_package($file); + + $this->assertArrayHasKey('packagefile', $errors); + $this->assertEquals(get_string('badexelearningpackage', 'mod_exescorm'), $errors['packagefile']); + } + + /** + * has_editable_source() finds the source the embedded editor needs. + * + * editor/save.php and set_ode.php refuse a save without one, because the + * editor re-opens the package it wrote. Only a root file counts: an editor + * that reads the archive root would not find one nested in a directory. + * + * @covers \mod_exescorm\exescorm_package::has_editable_source + */ + public function test_has_editable_source(): void { + $entry = function (string $pathname): \stdClass { + $info = new \stdClass(); + $info->pathname = $pathname; + $info->is_directory = false; + return $info; + }; + + $this->assertTrue(exescorm_package::has_editable_source([$entry('content.xml')])); + $this->assertTrue(exescorm_package::has_editable_source([$entry('contentv3.xml')])); + $this->assertTrue( + exescorm_package::has_editable_source([$entry('imsmanifest.xml'), $entry('content.xml')]) + ); + + $this->assertFalse(exescorm_package::has_editable_source([$entry('imsmanifest.xml')])); + $this->assertFalse(exescorm_package::has_editable_source([$entry('sub/content.xml')])); + $this->assertFalse(exescorm_package::has_editable_source([$entry('content.xml.bak')])); + $this->assertFalse(exescorm_package::has_editable_source([])); + $this->assertFalse(exescorm_package::has_editable_source(null)); + } + + /** + * The package the editor exports with its source passes both save-time checks. + * + * editor/save.php runs exescorm_validate_package() and has_editable_source() + * on the exported ZIP before touching the stored package. A real archive is + * used here, not synthetic entries, so the pathnames list_files() returns + * are what the root-only match is exercised against. + * + * @covers \mod_exescorm\exescorm_package::has_editable_source + * @covers ::exescorm_validate_package + */ + public function test_editor_export_with_source_passes_save_checks(): void { + $this->resetAfterTest(true); + + $packer = get_file_packer('application/zip'); + $file = $packer->archive_to_storage([ + 'imsmanifest.xml' => [''], + 'content.xml' => [''], + 'index.html' => [''], + ], \context_system::instance()->id, 'mod_exescorm', 'unittest', 0, '/', 'export.zip'); + $this->assertInstanceOf(\stored_file::class, $file); + + $this->assertEmpty(exescorm_validate_package($file)); + $this->assertTrue(exescorm_package::has_editable_source($file->list_files($packer))); + } }