diff --git a/contentbank/classes/content.php b/contentbank/classes/content.php index c9dad8859d17e..b8a9d40281ae1 100644 --- a/contentbank/classes/content.php +++ b/contentbank/classes/content.php @@ -237,6 +237,42 @@ public function get_configdata() { 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. * diff --git a/contentbank/classes/contentbank.php b/contentbank/classes/contentbank.php index 326e3049d8adc..307b94938dbde 100644 --- a/contentbank/classes/contentbank.php +++ b/contentbank/classes/contentbank.php @@ -224,6 +224,8 @@ public function search_contents(?string $search = null, ?int $contextid = 0, ?ar /** * 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 @@ public function create_content_from_file(\context $context, int $userid, stored_ $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; diff --git a/contentbank/classes/contenttype.php b/contentbank/classes/contenttype.php index 6b6a140429e68..cd1fb41850345 100644 --- a/contentbank/classes/contenttype.php +++ b/contentbank/classes/contenttype.php @@ -27,6 +27,8 @@ 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 @@ public function __construct(\context $context = null) { /** * 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,15 +82,37 @@ public function create_content(\stdClass $record = null): ?content { $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. - $event = contentbank_content_created::create_from_record($content->get_content()); - $event->trigger(); - return $content; + $classname = '\\'.$entry->contenttype.'\\content'; + $content = new $classname($entry); + // Trigger an event for creating the content. + $event = contentbank_content_created::create_from_record($content->get_content()); + $event->trigger(); + return $content; + } + + /** + * 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(); } - return null; + $content = $this->create_content($record); + try { + $content->import_file($file); + } catch (file_exception $e) { + $this->delete_content($content); + throw $e; + } + + return $content; } /** diff --git a/contentbank/tests/content_test.php b/contentbank/tests/content_test.php index 45c70c8b45f57..b0bfede821811 100644 --- a/contentbank/tests/content_test.php +++ b/contentbank/tests/content_test.php @@ -189,4 +189,88 @@ public function test_set_contextid() { $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()); + } } diff --git a/contentbank/tests/contentbank_test.php b/contentbank/tests/contentbank_test.php index d347a72c27610..cd22e80a0e069 100644 --- a/contentbank/tests/contentbank_test.php +++ b/contentbank/tests/contentbank_test.php @@ -112,14 +112,14 @@ public function test_get_extension_supporter_for_admins(array $supporters, strin $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 @@ public function test_get_extension_supporter_for_teachers(array $supporters, str $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 @@ public function test_get_extension_supporter_for_teachers(array $supporters, str // 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]); } /** diff --git a/contentbank/tests/contenttype_test.php b/contentbank/tests/contenttype_test.php index f3bc67b3ca051..9c80d8fbfe74a 100644 --- a/contentbank/tests/contenttype_test.php +++ b/contentbank/tests/contenttype_test.php @@ -27,6 +27,8 @@ 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 @@ public function test_create_content() { $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(). */ diff --git a/contentbank/tests/fixtures/testable_content.php b/contentbank/tests/fixtures/testable_content.php index d379dea337404..3d3ed4a9f5adb 100644 --- a/contentbank/tests/fixtures/testable_content.php +++ b/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 @@ * @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); + } } diff --git a/contentbank/upload.php b/contentbank/upload.php index c4626f0b69d69..00cc40cbf07a7 100644 --- a/contentbank/upload.php +++ b/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 @@ $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 (!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(); diff --git a/lang/en/contentbank.php b/lang/en/contentbank.php index 7c63e1e3b815b..bac647a536b5e 100644 --- a/lang/en/contentbank.php +++ b/lang/en/contentbank.php @@ -39,6 +39,7 @@ $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 \'{$a->name}\' and all associated files? This action cannot be undone.'; $string['displaydetails'] = 'Display content bank with file details';