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

contentbank/classes/content.php

@@ -237,6 +237,42 @@ abstract class content {
         return $this->content->configdata;
     }
 
+    /**
+     * Import a file as a valid content.
+     *
+     * By default, all content has a public file area to interact with the content bank
+     * repository. This method should be overridden by contentypes which does not simply
+     * upload to the public file area.
+     *
+     * If any, the method will return the final stored_file. This way it can be invoked
+     * 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.
+     */
+    public function import_file(stored_file $file): ?stored_file {
+        $originalfile = $this->get_file();
+        if ($originalfile) {
+            $originalfile->replace_file_with($file);
+            return $originalfile;
+        } else {
+            $itemid = $this->get_id();
+            $fs = get_file_storage();
+            $filerecord = [
+                'contextid' => $this->get_contextid(),
+                'component' => 'contentbank',
+                'filearea' => 'public',
+                'itemid' => $this->get_id(),
+                'filepath' => '/',
+                'filename' => $file->get_filename(),
+                'timecreated' => time(),
+            ];
+            return $fs->create_file_from_storedfile($filerecord, $file);
+        }
+    }
+
     /**
      * Returns the $file related to this content.
      *

contentbank/classes/contentbank.php

@@ -224,6 +224,8 @@ 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
@@ -243,7 +245,7 @@ class contentbank {
         $record->name = $filename;
         $record->usercreated = $userid;
         $contentype = new $classname($context);
-        $content = $contentype->create_content($record);
+        $content = $contentype->upload_content($file, $record);
         $event = \core\event\contentbank_content_uploaded::create_from_record($content->get_content());
         $event->trigger();
         return $content;

contentbank/classes/contenttype.php

@@ -27,6 +27,8 @@ namespace core_contentbank;
 use core\event\contentbank_content_created;
 use core\event\contentbank_content_deleted;
 use core\event\contentbank_content_viewed;
+use stored_file;
+use file_exception;
 use moodle_url;
 
 /**
@@ -62,10 +64,11 @@ abstract class contenttype {
     /**
      * Fills content_bank table with appropiate information.
      *
+     * @throws dml_exception A DML specific exception is thrown for any creation error.
      * @param \stdClass $record An optional content record compatible object (default null)
      * @return content  Object with content bank information.
      */
-    public function create_content(\stdClass $record = null): ?content {
+    public function create_content(\stdClass $record = null): content {
         global $USER, $DB;
 
         $entry = new \stdClass();
@@ -79,7 +82,6 @@ abstract class contenttype {
         $entry->configdata = $record->configdata ?? '';
         $entry->instanceid = $record->instanceid ?? 0;
         $entry->id = $DB->insert_record('contentbank_content', $entry);
-        if ($entry->id) {
         $classname = '\\'.$entry->contenttype.'\\content';
         $content = new $classname($entry);
         // Trigger an event for creating the content.
@@ -87,7 +89,30 @@ abstract class contenttype {
         $event->trigger();
         return $content;
     }
-        return null;
+
+    /**
+     * Create a new content from 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 \stdClass|null $record an optional content record
+     * @return content  Object with content bank information.
+     */
+    public function upload_content(stored_file $file, \stdClass $record = null): content {
+        if (empty($record)) {
+            $record = new \stdClass();
+            $record->name = $file->get_filename();
+        }
+        $content = $this->create_content($record);
+        try {
+            $content->import_file($file);
+        } catch (file_exception $e) {
+            $this->delete_content($content);
+            throw $e;
+        }
+
+        return $content;
     }
 
     /**

contentbank/tests/content_test.php

@@ -189,4 +189,88 @@ class core_contenttype_content_testcase extends \advanced_testcase {
         $this->assertEquals($newcontext->id, $content->get_contextid());
         $this->assertEquals($newcontext->id, $file->get_contextid());
     }
+
+    /**
+     * Tests for 'import_file' behaviour when replacing a file.
+     *
+     * @covers ::import_file
+     */
+    public function test_import_file_replace(): 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);
+
+        $originalfile = $content->get_file();
+
+        // Create a dummy file.
+        $filerecord = array(
+            'contextid' => $context->id,
+            'component' => 'contentbank',
+            'filearea' => 'draft',
+            'itemid' => $content->get_id(),
+            'filepath' => '/',
+            'filename' => 'example.txt'
+        );
+        $fs = get_file_storage();
+        $file = $fs->create_file_from_string($filerecord, 'Dummy content ');
+
+        $importedfile = $content->import_file($file);
+
+        $this->assertEquals($originalfile->get_filename(), $importedfile->get_filename());
+        $this->assertEquals($originalfile->get_filearea(), $importedfile->get_filearea());
+        $this->assertEquals($originalfile->get_filepath(), $importedfile->get_filepath());
+        $this->assertEquals($originalfile->get_mimetype(), $importedfile->get_mimetype());
+
+        $this->assertEquals($file->get_userid(), $importedfile->get_userid());
+        $this->assertEquals($file->get_contenthash(), $importedfile->get_contenthash());
+    }
+
+    /**
+     * Tests for 'import_file' behaviour when uploading a new file.
+     *
+     * @covers ::import_file
+     */
+    public function test_import_file_upload(): 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);
+
+        // Create a dummy file.
+        $filerecord = array(
+            'contextid' => $context->id,
+            'component' => 'contentbank',
+            'filearea' => 'draft',
+            'itemid' => $content->get_id(),
+            'filepath' => '/',
+            'filename' => 'example.txt'
+        );
+        $fs = get_file_storage();
+        $file = $fs->create_file_from_string($filerecord, 'Dummy content ');
+
+        $importedfile = $content->import_file($file);
+
+        $this->assertEquals($file->get_filename(), $importedfile->get_filename());
+        $this->assertEquals($file->get_userid(), $importedfile->get_userid());
+        $this->assertEquals($file->get_mimetype(), $importedfile->get_mimetype());
+        $this->assertEquals($file->get_contenthash(), $importedfile->get_contenthash());
+        $this->assertEquals('public', $importedfile->get_filearea());
+        $this->assertEquals('/', $importedfile->get_filepath());
+
+        $contentfile = $content->get_file($file);
+        $this->assertEquals($importedfile->get_id(), $contentfile->get_id());
+    }
 }

contentbank/tests/contentbank_test.php

@@ -112,14 +112,14 @@ class core_contentbank_testcase extends advanced_testcase {
         $this->resetAfterTest();
 
         $cb = new contentbank();
-        $expectedsupporters = [$extension => $expected];
 
         $systemcontext = context_system::instance();
 
         // All contexts allowed for admins.
         $this->setAdminUser();
         $contextsupporters = $cb->load_context_supported_extensions($systemcontext);
-        $this->assertEquals($expectedsupporters, $contextsupporters);
+        $this->assertArrayHasKey($extension, $contextsupporters);
+        $this->assertEquals($expected, $contextsupporters[$extension]);
     }
 
     /**
@@ -161,7 +161,6 @@ class core_contentbank_testcase extends advanced_testcase {
         $this->resetAfterTest();
 
         $cb = new contentbank();
-        $expectedsupporters = [$extension => $expected];
 
         $course = $this->getDataGenerator()->create_course();
         $teacher = $this->getDataGenerator()->create_and_enrol($course, 'editingteacher');
@@ -170,7 +169,8 @@ class core_contentbank_testcase extends advanced_testcase {
 
         // Teachers has permission in their context to upload supported by H5P content type.
         $contextsupporters = $cb->load_context_supported_extensions($coursecontext);
-        $this->assertEquals($expectedsupporters, $contextsupporters);
+        $this->assertArrayHasKey($extension, $contextsupporters);
+        $this->assertEquals($expected, $contextsupporters[$extension]);
     }
 
     /**

contentbank/tests/contenttype_test.php

@@ -27,6 +27,8 @@ namespace core_contentbank;
 
 use stdClass;
 use context_system;
+use context_user;
+use file_exception;
 use contenttype_testable\contenttype as contenttype;
 /**
  * Test for content bank contenttype class.
@@ -183,6 +185,111 @@ class core_contenttype_contenttype_testcase extends \advanced_testcase {
         $this->assertInstanceOf('\\contenttype_testable\\content', $content);
     }
 
+    /**
+     * Tests for behaviour of upload_content() with a file and a record.
+     *
+     * @dataProvider upload_content_provider
+     * @param bool $userecord if a predefined record has to be used.
+     *
+     * @covers ::upload_content
+     */
+    public function test_upload_content(bool $userecord): void {
+        global $USER;
+
+        $this->resetAfterTest();
+        $this->setAdminUser();
+
+        $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');
+
+        // Create content.
+        if ($userecord) {
+            $record = new stdClass();
+            $record->name = 'Test content';
+            $record->configdata = '';
+            $record->contenttype = '';
+            $checkname = $record->name;
+        } else {
+            $record = null;
+            $checkname = $dummyfile->get_filename();
+        }
+
+        $contenttype = new contenttype(context_system::instance());
+        $content = $contenttype->upload_content($dummyfile, $record);
+
+        $this->assertEquals('contenttype_testable', $content->get_content_type());
+        $this->assertEquals($checkname, $content->get_name());
+        $this->assertInstanceOf('\\contenttype_testable\\content', $content);
+
+        $file = $content->get_file();
+        $this->assertEquals($dummyfile->get_filename(), $file->get_filename());
+        $this->assertEquals($dummyfile->get_userid(), $file->get_userid());
+        $this->assertEquals($dummyfile->get_mimetype(), $file->get_mimetype());
+        $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());
+    }
+
+    /**
+     * Data provider for test_rename_content.
+     *
+     * @return  array
+     */
+    public function upload_content_provider() {
+        return [
+            'With record' => [true],
+            'Without record' => [false],
+        ];
+    }
+
+    /**
+     * Tests for behaviour of upload_content() with a file wrong file.
+     *
+     * @covers ::upload_content
+     */
+    public function test_upload_content_exception(): void {
+        global $USER, $DB;
+
+        $this->resetAfterTest();
+        $this->setAdminUser();
+
+        // The testing contenttype thows exception if filename is "error.*".
+        $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());
+        $cbcontents = $DB->count_records('contentbank_content');
+
+        // We need to capture the exception to check no content is created.
+        try {
+            $content = $contenttype->upload_content($dummyfile);
+            $this->assertTrue(false);
+        } catch (file_exception $e) {
+            $this->assertTrue(true);
+        }
+        $this->assertEquals($cbcontents, $DB->count_records('contentbank_content'));
+        $this->assertEquals(1, $DB->count_records('files', ['contenthash' => $dummyfile->get_contenthash()]));
+    }
+
     /**
      * Test the behaviour of can_delete().
      */

contentbank/tests/fixtures/testable_content.php

@@ -25,6 +25,9 @@
 
 namespace contenttype_testable;
 
+use file_exception;
+use stored_file;
+
 /**
  * Testable content plugin class.
  *
@@ -33,4 +36,21 @@ namespace contenttype_testable;
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class content extends \core_contentbank\content {
+
+    /**
+     * Import a file as a valid content.
+     *
+     * This method will thow an error if the filename is "error.*"
+     *
+     * @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.
+     * @throws file_exception if the filename contains the word "error"
+     */
+    public function import_file(stored_file $file): ?stored_file {
+        $filename = $file->get_filename();
+        if (strrpos($filename, 'error') !== false) {
+            throw new file_exception('yourerrorthanks', 'contenttype_test');
+        }
+        return parent::import_file($file);
+    }
 }

contentbank/upload.php

@@ -25,6 +25,8 @@
 require('../config.php');
 require_once("$CFG->dirroot/contentbank/files_form.php");
 
+use core\output\notification;
+
 require_login();
 
 $contextid = optional_param('contextid', \context_system::instance()->id, PARAM_INT);
@@ -68,6 +70,8 @@ file_prepare_standard_filemanager($data, 'files', $options, $context, 'contentba
 
 $mform = new contentbank_files_form(null, ['contextid' => $contextid, 'data' => $data, 'options' => $options]);
 
+$error = '';
+
 if ($mform->is_cancelled()) {
     redirect($returnurl);
 } else if ($formdata = $mform->get_data()) {
@@ -79,16 +83,20 @@ if ($mform->is_cancelled()) {
     if (!empty($files)) {
         $file = reset($files);
         $content = $cb->create_content_from_file($context, $USER->id, $file);
-        file_save_draft_area_files($formdata->file, $contextid, 'contentbank', 'public', $content->get_id());
         $viewurl = new \moodle_url('/contentbank/view.php', ['id' => $content->get_id(), 'contextid' => $contextid]);
         redirect($viewurl);
+    } else {
+        $error = get_string('errornofile', 'contentbank');
     }
-    redirect($returnurl);
 }
 
 echo $OUTPUT->header();
 echo $OUTPUT->box_start('generalbox');
 
+if (!empty($error)) {
+    echo $OUTPUT->notification($error, notification::NOTIFY_ERROR);
+}
+
 $mform->display();
 
 echo $OUTPUT->box_end();

lang/en/contentbank.php

@@ -39,6 +39,7 @@ $string['eventcontentupdated'] = 'Content updated';
 $string['eventcontentuploaded'] = 'Content uploaded';
 $string['eventcontentviewed'] = 'Content viewed';
 $string['errordeletingcontentfromcategory'] = 'Error deleting content from category {$a}.';
+$string['errornofile'] = 'A compatible file is needed to create a content';
 $string['deletecontent'] = 'Delete content';
 $string['deletecontentconfirm'] = 'Are you sure you want to delete the content <em>\'{$a->name}\'</em> and all associated files? This action cannot be undone.';
 $string['displaydetails'] = 'Display content bank with file details';