diff --git a/contentbank/classes/content.php b/contentbank/classes/content.php index f1e84b78e35..583735ba815 100644 --- a/contentbank/classes/content.php +++ b/contentbank/classes/content.php @@ -296,7 +296,6 @@ abstract class content { * as parent::import_file in case any plugin want to store the file in the public area * and also parse it. * - * @throws file_exception If file operations fail * @param stored_file $file File to store in the content file area. * @return stored_file|null the stored content file or null if the file is discarted. */ @@ -306,7 +305,6 @@ abstract class content { $originalfile->replace_file_with($file); return $originalfile; } else { - $itemid = $this->get_id(); $fs = get_file_storage(); $filerecord = [ 'contextid' => $this->get_contextid(), diff --git a/contentbank/classes/contentbank.php b/contentbank/classes/contentbank.php index 05ca94d0b56..43976a6263c 100644 --- a/contentbank/classes/contentbank.php +++ b/contentbank/classes/contentbank.php @@ -258,8 +258,6 @@ class contentbank { /** * Create content from a file information. * - * @throws file_exception If file operations fail - * @throws dml_exception if the content creation fails * @param \context $context Context where to upload the file and content. * @param int $userid Id of the user uploading the file. * @param stored_file $file The file to get information from diff --git a/contentbank/classes/contenttype.php b/contentbank/classes/contenttype.php index 3295f24c5dd..491eea8a1b3 100644 --- a/contentbank/classes/contenttype.php +++ b/contentbank/classes/contenttype.php @@ -28,7 +28,6 @@ use core\event\contentbank_content_created; use core\event\contentbank_content_deleted; use core\event\contentbank_content_viewed; use stored_file; -use Exception; use moodle_url; /** @@ -120,9 +119,9 @@ abstract class contenttype { $content = $this->create_content($record); try { $content->import_file($file); - } catch (Exception $e) { + } catch (\moodle_exception $e) { $this->delete_content($content); - throw $e; + throw new \moodle_exception($e->errorcode); } return $content; diff --git a/contentbank/classes/form/upload_files.php b/contentbank/classes/form/upload_files.php index bbd729c89ce..282b165a01b 100644 --- a/contentbank/classes/form/upload_files.php +++ b/contentbank/classes/form/upload_files.php @@ -16,6 +16,8 @@ namespace core_contentbank\form; +use core\output\notification; + /** * Upload files to content bank form * @@ -161,20 +163,20 @@ class upload_files extends \core_form\dynamic_form { } $params = ['id' => $content->get_id(), 'contextid' => $this->get_context_for_dynamic_submission()->id]; $url = new \moodle_url('/contentbank/view.php', $params); - } catch (\Exception $e) { + } catch (\moodle_exception $e) { // Redirect to the right page (depending on if content is new or existing) and display an error. if ($this->get_data()->id) { $content = $cb->get_content_from_id($this->get_data()->id); $params = [ 'id' => $content->get_id(), 'contextid' => $this->get_context_for_dynamic_submission()->id, - 'errormsg' => 'notvalidpackage', + 'errormsg' => $e->errorcode, ]; $url = new \moodle_url('/contentbank/view.php', $params); } else { $url = new \moodle_url('/contentbank/index.php', [ 'contextid' => $this->get_context_for_dynamic_submission()->id, - 'errormsg' => 'notvalidpackage'], + 'errormsg' => $e->errorcode], ); } } diff --git a/contentbank/contenttype/h5p/classes/content.php b/contentbank/contenttype/h5p/classes/content.php index e573a7029ed..e26e6cc21af 100644 --- a/contentbank/contenttype/h5p/classes/content.php +++ b/contentbank/contenttype/h5p/classes/content.php @@ -24,6 +24,9 @@ namespace contenttype_h5p; +use core\notification; +use core_h5p\factory; + /** * H5P Content manager class * @@ -80,7 +83,7 @@ class content extends \core_contentbank\content { * Before importing the file, this method will check if the file is a valid H5P package. If it's not valid, it will thrown * an exception. * - * @throws \file_exception If file operations fail + * @throws \moodle_exception If file operations fail * @param \stored_file $file File to store in the content file area. * @return \stored_file|null the stored content file or null if the file is discarted. */ @@ -90,7 +93,15 @@ class content extends \core_contentbank\content { $onlyupdatelibs = !\core_h5p\helper::can_update_library($file); if (!\core_h5p\api::is_valid_package($file, $onlyupdatelibs)) { - throw new \file_exception('invalidpackage'); + $factory = new factory(); + $errors = $factory->get_framework()->getMessages('error'); + foreach ($errors as $error) { + notification::error($error->message); + } + if (empty($errors) || count($errors) > 1) { + throw new \moodle_exception('notvalidpackage', 'h5p'); + } + throw new \moodle_exception($errors[0]->code, 'h5p'); } return parent::import_file($file); } diff --git a/contentbank/contenttype/h5p/tests/behat/admin_upload_content.feature b/contentbank/contenttype/h5p/tests/behat/admin_upload_content.feature index fcc31a0e0da..8ea945320f0 100644 --- a/contentbank/contenttype/h5p/tests/behat/admin_upload_content.feature +++ b/contentbank/contenttype/h5p/tests/behat/admin_upload_content.feature @@ -97,3 +97,33 @@ Feature: H5P file upload to content bank for admins And I switch to the main frame And I navigate to "H5P > Manage H5P content types" in site administration And I should see "Fill in the Blanks" + + Scenario: Uploading invalid packages throws error + Given I follow "Dashboard" + And I follow "Manage private files..." + And I upload "h5p/tests/fixtures/no-json-file.h5p" file to "Files" filemanager + And I click on "Save changes" "button" + And I follow "Manage private files..." + And I upload "h5p/tests/fixtures/unzippable.h5p" file to "Files" filemanager + And I click on "Save changes" "button" + And I expand "Site pages" node + And I click on "Content bank" "link" + When I click on "Upload" "link" + And I click on "Choose a file..." "button" + And I click on "Private files" "link" in the ".fp-repo-area" "css_element" + And I click on "no-json-file.h5p" "link" + And I click on "Select this file" "button" + And I click on "Save changes" "button" + And I wait until the page is ready + Then I should see "A valid main h5p.json file is missing" + And I should see "Only files with the following extensions are allowed" + And I should not see "Sorry, this file is not valid" + And I click on "Upload" "link" + And I click on "Choose a file..." "button" + And I click on "Private files" "link" in the ".fp-repo-area" "css_element" + And I click on "unzippable.h5p" "link" + And I click on "Select this file" "button" + And I click on "Save changes" "button" + And I wait until the page is ready + And I should see "It is not possible to unzip it" + And I should not see "Sorry, this file is not valid" diff --git a/contentbank/contenttype/h5p/tests/behat/teacher_replace_content.feature b/contentbank/contenttype/h5p/tests/behat/teacher_replace_content.feature index ba699f65470..8f61b66c101 100644 --- a/contentbank/contenttype/h5p/tests/behat/teacher_replace_content.feature +++ b/contentbank/contenttype/h5p/tests/behat/teacher_replace_content.feature @@ -80,3 +80,30 @@ Feature: Replace H5P file from an existing content requires special capabilities When I click on "teachercontent" "link" And I click on "More" "button" Then I should not see "Replace with file" + + Scenario: Replacing with invalid packages throws error + Given I log in as "admin" + And I navigate to "H5P > Manage H5P content types" in site administration + And I upload "h5p/tests/fixtures/no-json-file.h5p" file to "H5P content type" filemanager + And I upload "h5p/tests/fixtures/unzippable.h5p" file to "H5P content type" filemanager + And I log out + And I log in as "teacher1" + And I am on "Course 1" course homepage + And I expand "Site pages" node + And I click on "Content bank" "link" + And I click on "teachercontent" "link" + When I click on "More" "button" + And I click on "Replace with file" "link" + And I upload "h5p/tests/fixtures/no-json-file.h5p" file to "Upload content" filemanager + And I click on "Save changes" "button" + And I wait until the page is ready + Then I should see "A valid main h5p.json file is missing" + And I should see "Only files with the following extensions are allowed" + And I should not see "Sorry, this file is not valid" + And I click on "More" "button" + And I click on "Replace with file" "link" + And I upload "h5p/tests/fixtures/unzippable.h5p" file to "Upload content" filemanager + And I click on "Save changes" "button" + And I wait until the page is ready + And I should see "It is not possible to unzip it" + And I should not see "Sorry, this file is not valid" diff --git a/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature b/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature index cfdaf88f12c..c527ecd78a9 100644 --- a/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature +++ b/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature @@ -118,7 +118,7 @@ Feature: H5P file upload to content bank for non admins And I click on "filltheblanks.h5p" "link" And I click on "Select this file" "button" And I click on "Save changes" "button" - Then I should see "Sorry, this file is not valid." + Then I should see "Missing required library" And I should not see "filltheblanks.h5p" And I log out And I log in as "admin" diff --git a/contentbank/index.php b/contentbank/index.php index 8dfe2907b1f..a2ff8174e3e 100644 --- a/contentbank/index.php +++ b/contentbank/index.php @@ -37,8 +37,11 @@ if (!$cb->is_context_allowed($context)) { require_capability('moodle/contentbank:access', $context); -$statusmsg = optional_param('statusmsg', '', PARAM_ALPHANUMEXT); -$errormsg = optional_param('errormsg', '', PARAM_ALPHANUMEXT); +// If notifications had been sent we don't pay attention to message parameter. +if (empty($SESSION->notifications)) { + $statusmsg = optional_param('statusmsg', '', PARAM_ALPHANUMEXT); + $errormsg = optional_param('errormsg', '', PARAM_ALPHANUMEXT); +} $title = get_string('contentbank'); \core_contentbank\helper::get_page_ready($context, $title); @@ -118,10 +121,10 @@ echo $OUTPUT->heading($title, 2); echo $OUTPUT->box_start('generalbox'); // If needed, display notifications. -if ($errormsg !== '' && get_string_manager()->string_exists($errormsg, 'core_contentbank')) { +if (!empty($errormsg) && get_string_manager()->string_exists($errormsg, 'core_contentbank')) { $errormsg = get_string($errormsg, 'core_contentbank'); echo $OUTPUT->notification($errormsg); -} else if ($statusmsg !== '' && get_string_manager()->string_exists($statusmsg, 'core_contentbank')) { +} else if (!empty($statusmsg) && get_string_manager()->string_exists($statusmsg, 'core_contentbank')) { $statusmsg = get_string($statusmsg, 'core_contentbank'); echo $OUTPUT->notification($statusmsg, 'notifysuccess'); } diff --git a/contentbank/view.php b/contentbank/view.php index 2b9e360352d..dafea573db9 100644 --- a/contentbank/view.php +++ b/contentbank/view.php @@ -37,8 +37,11 @@ $record = $DB->get_record('contentbank_content', ['id' => $id], '*', MUST_EXIST) $context = context::instance_by_id($record->contextid, MUST_EXIST); require_capability('moodle/contentbank:access', $context); -$statusmsg = optional_param('statusmsg', '', PARAM_ALPHANUMEXT); -$errormsg = optional_param('errormsg', '', PARAM_ALPHANUMEXT); +// If notifications had been sent we don't pay attention to message parameter. +if (empty($SESSION->notifications)) { + $statusmsg = optional_param('statusmsg', '', PARAM_ALPHANUMEXT); + $errormsg = optional_param('errormsg', '', PARAM_ALPHANUMEXT); +} $returnurl = new \moodle_url('/contentbank/index.php', ['contextid' => $context->id]); $plugin = core_plugin_manager::instance()->get_plugin_info($record->contenttype); @@ -81,10 +84,10 @@ $PAGE->set_secondary_active_tab('contentbank'); echo $OUTPUT->header(); // If needed, display notifications. -if ($errormsg !== '' && get_string_manager()->string_exists($errormsg, 'core_contentbank')) { +if (!empty($errormsg) && get_string_manager()->string_exists($errormsg, 'core_contentbank')) { $errormsg = get_string($errormsg, 'core_contentbank'); echo $OUTPUT->notification($errormsg); -} else if ($statusmsg !== '' && get_string_manager()->string_exists($statusmsg, 'core_contentbank')) { +} else if (!empty($statusmsg) && get_string_manager()->string_exists($statusmsg, 'core_contentbank')) { if ($statusmsg == 'contentvisibilitychanged') { switch ($content->get_visibility()) { case content::VISIBILITY_PUBLIC: diff --git a/h5p/tests/fixtures/no-json-file.h5p b/h5p/tests/fixtures/no-json-file.h5p new file mode 100644 index 00000000000..ef955989b15 Binary files /dev/null and b/h5p/tests/fixtures/no-json-file.h5p differ diff --git a/h5p/tests/fixtures/unzippable.h5p b/h5p/tests/fixtures/unzippable.h5p new file mode 100644 index 00000000000..3de57256c1a --- /dev/null +++ b/h5p/tests/fixtures/unzippable.h5p @@ -0,0 +1 @@ +This is not a zip package