Merge branch 'MDL-78183-master' of https://github.com/aanabit/moodle

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(),

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

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;

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],
                     );
                 }
             }

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);
     }

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"

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"

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"

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');
 }

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:

h5p/tests/fixtures/unzippable.h5p

@@ -0,0 +1 @@
+This is not a zip package