Merge branch 'MDL-69270-master' of git://github.com/ferranrecio/moodle

contentbank/classes/content.php

@@ -28,6 +28,7 @@
 use stored_file;
 use stdClass;
 use coding_exception;
+use context;
 use moodle_url;
 use core\event\contentbank_content_updated;
 
@@ -85,6 +86,17 @@ public function get_content_type(): string {
         return $this->content->contenttype;
     }
 
+    /**
+     * Return the contenttype instance of this content.
+     *
+     * @return contenttype The content type instance
+     */
+    public function get_content_type_instance(): contenttype {
+        $context = context::instance_by_id($this->content->contextid);
+        $contenttypeclass = "\\{$this->content->contenttype}\\contenttype";
+        return new $contenttypeclass($context);
+    }
+
     /**
      * Returns $this->content->timemodified.
      *

contentbank/classes/contentbank.php

@@ -334,4 +334,18 @@ public function get_contenttypes_with_capability_feature(string $feature, \conte
 
         return $contenttypes;
     }
+
+    /**
+     * Return a content class form a content id.
+     *
+     * @throws coding_exception if the ID is not valid or some class does no exists
+     * @param int $id the content id
+     * @return content the content class instance
+     */
+    public function get_content_from_id(int $id): content {
+        global $DB;
+        $record = $DB->get_record('contentbank_content', ['id' => $id], '*', MUST_EXIST);
+        $contentclass = "\\$record->contenttype\\content";
+        return new $contentclass($record);
+    }
 }

contentbank/classes/contenttype.php

@@ -121,6 +121,21 @@ public function upload_content(stored_file $file, \stdClass $record = null): con
         return $content;
     }
 
+    /**
+     * Replace a content using an uploaded file.
+     *
+     * @throws file_exception If file operations fail
+     * @throws dml_exception if the content creation fails
+     * @param stored_file $file the uploaded file
+     * @param content $content the original content record
+     * @return content Object with the updated content bank information.
+     */
+    public function replace_content(stored_file $file, content $content): content {
+        $content->import_file($file);
+        $content->update_content();
+        return $content;
+    }
+
     /**
      * Delete this content from the content_bank.
      * This method can be overwritten by the plugins if they need to delete specific information.

contentbank/contenttype/h5p/tests/behat/admin_replace_content.feature

@@ -0,0 +1,30 @@
+@core @core_contentbank @contenttype_h5p @_file_upload @_switch_iframe @javascript
+Feature: Replace H5P file from an existing content
+  In order to replace an H5P content from the content bank
+  As an admin
+  I need to be able to replace the content with a new .h5p file
+
+  Background:
+    Given the following "contentbank content" exist:
+      | contextlevel | reference | contenttype     | user  | contentname       | filepath                              |
+      | System       |           | contenttype_h5p | admin | filltheblanks.h5p | /h5p/tests/fixtures/filltheblanks.h5p |
+    And I log in as "admin"
+    And I press "Customise this page"
+    And I add the "Navigation" block if not present
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+
+  Scenario: Admins can replace the original .h5p file with a new one
+    Given I click on "filltheblanks.h5p" "link"
+    And I switch to "h5p-player" class iframe
+    And I switch to "h5p-iframe" class iframe
+    And I should see "Of which countries"
+    And I switch to the main frame
+    When I open the action menu in "region-main-settings-menu" "region"
+    And I choose "Replace with file" in the open action menu
+    And I upload "h5p/tests/fixtures/ipsums.h5p" file to "Upload content" filemanager
+    And I click on "Save changes" "button"
+    Then I switch to "h5p-player" class iframe
+    And I switch to "h5p-iframe" class iframe
+    And I should see "Lorum ipsum"
+    And I switch to the main frame

contentbank/contenttype/h5p/tests/behat/teacher_replace_content.feature

@@ -0,0 +1,67 @@
+@core @core_contentbank @contenttype_h5p @_file_upload @_switch_iframe @javascript
+Feature: Replace H5P file from an existing content requires special capabilities
+  In order replace an H5P content from the content bank
+  As a teacher
+  I need to be able to replace the content only if certain capabilities are allowed
+
+  Background:
+    Given the following "users" exist:
+      | username | firstname | lastname | email                |
+      | teacher1 | Teacher   | 1        | teacher1@example.com |
+    And the following "categories" exist:
+      | name  | category | idnumber |
+      | Cat 1 | 0        | CAT1     |
+    And the following "courses" exist:
+      | fullname | shortname | category |
+      | Course 1 | C1        | CAT1     |
+    And the following "course enrolments" exist:
+      | user     | course | role           |
+      | teacher1 | C1     | editingteacher |
+    And the following "contentbank content" exist:
+      | contextlevel | reference | contenttype     | user     | contentname       | filepath                              |
+      | Course       | C1        | contenttype_h5p | admin    | admincontent      | /h5p/tests/fixtures/ipsums.h5p        |
+      | Course       | C1        | contenttype_h5p | teacher1 | teachercontent    | /h5p/tests/fixtures/filltheblanks.h5p |
+    And I log in as "teacher1"
+    And I am on "Course 1" course homepage with editing mode on
+    And I add the "Navigation" block if not present
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    # Force the content deploy
+    And I click on "admincontent" "link"
+    And I click on "Content bank" "link"
+
+  Scenario: Teacher can replace its own H5P files
+    Given I click on "teachercontent" "link"
+    When I open the action menu in "region-main-settings-menu" "region"
+    And I choose "Replace with file" in the open action menu
+    And I upload "h5p/tests/fixtures/ipsums.h5p" file to "Upload content" filemanager
+    And I click on "Save changes" "button"
+    Then I switch to "h5p-player" class iframe
+    And I switch to "h5p-iframe" class iframe
+    And I should see "Lorum ipsum"
+    And I switch to the main frame
+
+  Scenario: Teacher cannot replace another user's H5P files
+    When I click on "admincontent" "link"
+    Then "region-main-settings-menu" "region" should not exist
+
+  Scenario: Teacher cannot replace a content without having upload capability
+    Given the following "permission overrides" exist:
+      | capability                | permission | role           | contextlevel | reference |
+      | moodle/contentbank:upload | Prevent    | editingteacher | Course       | C1        |
+    When I click on "teachercontent" "link"
+    Then "region-main-settings-menu" "region" should not exist
+
+  Scenario: Teacher cannot replace a content without having the H5P upload capability
+    Given the following "permission overrides" exist:
+      | capability             | permission | role           | contextlevel | reference |
+      | contenttype/h5p:upload | Prevent    | editingteacher | Course       | C1        |
+    When I click on "teachercontent" "link"
+    Then "region-main-settings-menu" "region" should not exist
+
+  Scenario: Teacher cannot replace a content without having the manage own content capability
+    Given the following "permission overrides" exist:
+      | capability                          | permission | role           | contextlevel | reference |
+      | moodle/contentbank:manageowncontent | Prevent    | editingteacher | Course       | C1        |
+    When I click on "teachercontent" "link"
+    Then "region-main-settings-menu" "region" should not exist

contentbank/files_form.php

@@ -44,6 +44,11 @@ public function definition() {
         $mform->addElement('hidden', 'contextid', $this->_customdata['contextid']);
         $mform->setType('contextid', PARAM_INT);
 
+        if (!empty($this->_customdata['id'])) {
+            $mform->addElement('hidden', 'id', $this->_customdata['id']);
+            $mform->setType('id', PARAM_INT);
+        }
+
         $options = $this->_customdata['options'];
         $mform->addElement('filepicker', 'file', get_string('file', 'core_contentbank'), null, $options);
         $mform->addHelpButton('file', 'file', 'core_contentbank');

contentbank/tests/content_test.php

@@ -275,4 +275,27 @@ public function test_import_file_upload(): void {
         $contentfile = $content->get_file($file);
         $this->assertEquals($importedfile->get_id(), $contentfile->get_id());
     }
+
+    /**
+     * Tests for 'get_content_type_instance'
+     *
+     * @covers ::get_content_type_instance
+     */
+    public function test_get_content_type_instance(): void {
+        global $USER;
+        $this->resetAfterTest();
+        $this->setAdminUser();
+        $context = context_system::instance();
+
+        $type = new contenttype($context);
+        $record = (object)[
+            'name' => 'content name',
+            'usercreated' => $USER->id,
+        ];
+        $content = $type->create_content($record);
+
+        $contenttype = $content->get_content_type_instance();
+
+        $this->assertInstanceOf(get_class($type), $contenttype);
+    }
 }

contentbank/tests/contentbank_test.php

@@ -31,6 +31,7 @@
 use context_course;
 use context_coursecat;
 use context_system;
+use Exception;
 
 global $CFG;
 require_once($CFG->dirroot . '/contentbank/tests/fixtures/testable_contenttype.php');
@@ -603,4 +604,31 @@ public function test_get_contenttypes_with_capability_feature(array $contenttype
         $actual = $cb->get_contenttypes_with_capability_feature('test2', null, $enabled);
         $this->assertEquals($contenttypescanfeature, array_values($actual));
     }
+
+    /**
+     * Test the behaviour of get_content_from_id()
+     *
+     * @covers  ::get_content_from_id
+     */
+    public function test_get_content_from_id() {
+
+        $this->resetAfterTest();
+        $cb = new \core_contentbank\contentbank();
+
+        // Create a category and two courses.
+        $systemcontext = context_system::instance();
+
+        // Add some content to the content bank.
+        $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
+        $contents = $generator->generate_contentbank_data(null, 3, 0, $systemcontext);
+        $content = reset($contents);
+
+        // Get the content instance form id.
+        $newinstance = $cb->get_content_from_id($content->get_id());
+        $this->assertEquals($content->get_id(), $newinstance->get_id());
+
+        // Now produce and exception with an innexistent id.
+        $this->expectException(Exception::class);
+        $cb->get_content_from_id(0);
+    }
 }

contentbank/tests/contenttype_test.php

@@ -290,6 +290,84 @@ public function test_upload_content_exception(): void {
         $this->assertEquals(1, $DB->count_records('files', ['contenthash' => $dummyfile->get_contenthash()]));
     }
 
+    /**
+     * Tests for behaviour of replace_content() using a dummy file.
+     *
+     * @covers ::replace_content
+     */
+    public function test_replace_content(): void {
+        global $USER;
+
+        $this->resetAfterTest();
+        $this->setAdminUser();
+        $context = context_system::instance();
+
+        // Add some content to the content bank.
+        $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
+        $contents = $generator->generate_contentbank_data('contenttype_testable', 3, 0, $context);
+        $content = reset($contents);
+
+        $dummy = [
+            'contextid' => context_user::instance($USER->id)->id,
+            'component' => 'user',
+            'filearea' => 'draft',
+            'itemid' => 1,
+            'filepath' => '/',
+            'filename' => 'file.h5p',
+            'userid' => $USER->id,
+        ];
+        $fs = get_file_storage();
+        $dummyfile = $fs->create_file_from_string($dummy, 'Dummy content');
+
+        $contenttype = new contenttype(context_system::instance());
+        $content = $contenttype->replace_content($dummyfile, $content);
+
+        $this->assertEquals('contenttype_testable', $content->get_content_type());
+        $this->assertInstanceOf('\\contenttype_testable\\content', $content);
+
+        $file = $content->get_file();
+        $this->assertEquals($dummyfile->get_userid(), $file->get_userid());
+        $this->assertEquals($dummyfile->get_contenthash(), $file->get_contenthash());
+        $this->assertEquals('contentbank', $file->get_component());
+        $this->assertEquals('public', $file->get_filearea());
+        $this->assertEquals('/', $file->get_filepath());
+    }
+
+    /**
+     * Tests for behaviour of replace_content() using an error file.
+     *
+     * @covers ::replace_content
+     */
+    public function test_replace_content_exception(): void {
+        global $USER;
+
+        $this->resetAfterTest();
+        $this->setAdminUser();
+        $context = context_system::instance();
+
+        // Add some content to the content bank.
+        $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
+        $contents = $generator->generate_contentbank_data('contenttype_testable', 3, 0, $context);
+        $content = reset($contents);
+
+        $dummy = [
+            'contextid' => context_user::instance($USER->id)->id,
+            'component' => 'user',
+            'filearea' => 'draft',
+            'itemid' => 1,
+            'filepath' => '/',
+            'filename' => 'error.txt',
+            'userid' => $USER->id,
+        ];
+        $fs = get_file_storage();
+        $dummyfile = $fs->create_file_from_string($dummy, 'Dummy content');
+
+        $contenttype = new contenttype(context_system::instance());
+
+        $this->expectException(Exception::class);
+        $content = $contenttype->replace_content($dummyfile, $content);
+    }
+
     /**
      * Test the behaviour of can_delete().
      */

contentbank/upload.php

@@ -34,6 +34,17 @@
 
 require_capability('moodle/contentbank:upload', $context);
 
+$cb = new \core_contentbank\contentbank();
+
+$id = optional_param('id', null, PARAM_INT);
+if ($id) {
+    $content = $cb->get_content_from_id($id);
+    $contenttype = $content->get_content_type_instance();
+    if (!$contenttype->can_manage($content) || !$contenttype->can_upload()) {
+        print_error('nopermissions', 'error', $returnurl, get_string('replacecontent', 'contentbank'));
+    }
+}
+
 $title = get_string('contentbank');
 \core_contentbank\helper::get_page_ready($context, $title, true);
 if ($PAGE->course) {
@@ -55,8 +66,12 @@
     $maxareabytes = FILE_AREA_MAX_BYTES_UNLIMITED;
 }
 
-$cb = new \core_contentbank\contentbank();
-$accepted = $cb->get_supported_extensions_as_string($context);
+if ($id) {
+    $extensions = $contenttype->get_manageable_extensions();
+    $accepted = implode(',', $extensions);
+} else {
+    $accepted = $cb->get_supported_extensions_as_string($context);
+}
 
 $data = new stdClass();
 $options = array(
@@ -68,7 +83,7 @@
 );
 file_prepare_standard_filemanager($data, 'files', $options, $context, 'contentbank', 'public', 0);
 
-$mform = new contentbank_files_form(null, ['contextid' => $contextid, 'data' => $data, 'options' => $options]);
+$mform = new contentbank_files_form(null, ['contextid' => $contextid, 'data' => $data, 'options' => $options, 'id' => $id]);
 
 $error = '';
 
@@ -82,7 +97,11 @@
     $files = $fs->get_area_files($usercontext->id, 'user', 'draft', $formdata->file, 'itemid, filepath, filename', false);
     if (!empty($files)) {
         $file = reset($files);
-        $content = $cb->create_content_from_file($context, $USER->id, $file);
+        if ($id) {
+            $content = $contenttype->replace_content($file, $content);
+        } else {
+            $content = $cb->create_content_from_file($context, $USER->id, $file);
+        }
         $viewurl = new \moodle_url('/contentbank/view.php', ['id' => $content->get_id(), 'contextid' => $contextid]);
         redirect($viewurl);
     } else {