From fb445c76c8c1cd047f6cd62e237ae02cbbbb0663 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Wed, 15 Apr 2020 09:14:27 +0200 Subject: [PATCH 1/3] MDL-67797 contentbank: add search_contents method --- contentbank/classes/content.php | 2 +- contentbank/classes/contentbank.php | 50 +++++++++ contentbank/tests/contentbank_test.php | 139 +++++++++++++++++++++++++ 3 files changed, 190 insertions(+), 1 deletion(-) diff --git a/contentbank/classes/content.php b/contentbank/classes/content.php index 28cb965079714..a77b7a08e844f 100644 --- a/contentbank/classes/content.php +++ b/contentbank/classes/content.php @@ -211,7 +211,7 @@ public function get_file_url(): string { * * @return bool True if content could be accessed. False otherwise. */ - public function can_view(): bool { + public function is_view_allowed(): bool { // There's no capability at content level to check, // but plugins can overwrite this method in case they want to check something related to content properties. return true; diff --git a/contentbank/classes/contentbank.php b/contentbank/classes/contentbank.php index 2e89d42d96c03..c5ab6c13bb85e 100644 --- a/contentbank/classes/contentbank.php +++ b/contentbank/classes/contentbank.php @@ -155,4 +155,54 @@ public function get_extension_supporter(string $extension, \context $context = n } return null; } + + /** + * Find the contents with %$search% in the contextid defined. + * If contextid and search are empty, all contents are returned. + * In all the cases, only the contents for the enabled contentbank-type plugins are returned. + * + * @param string|null $search Optional string to search (for now it will search only into the name). + * @param int $contextid Optional contextid to search. + * @return array The contents for the enabled contentbank-type plugins having $search as name and placed in $contextid. + */ + public function search_contents(?string $search = null, ?int $contextid = 0): array { + global $DB; + + $contents = []; + + // Get only contents for enabled content-type plugins. + $contenttypes = array_map(function($contenttypename) { + return "contenttype_$contenttypename"; + }, $this->get_enabled_content_types()); + if (empty($contenttypes)) { + // Early return if there are no content-type plugins enabled. + return $contents; + } + + list($sqlcontenttypes, $params) = $DB->get_in_or_equal($contenttypes, SQL_PARAMS_NAMED); + $sql = " contenttype $sqlcontenttypes "; + + // Filter contents on this context (if defined). + if (!empty($contextid)) { + $params['contextid'] = $contextid; + $sql .= ' AND contextid = :contextid '; + } + + // Search for contents having this string (if defined). + if (!empty($search)) { + $sql .= ' AND ' . $DB->sql_like('name', ':name', false, false); + $params['name'] = '%' . $DB->sql_like_escape($search) . '%'; + } + + $records = $DB->get_records_select('contentbank_content', $sql, $params); + foreach ($records as $record) { + $contentclass = "\\$record->contenttype\\content"; + $content = new $contentclass($record); + if ($content->is_view_allowed()) { + $contents[] = $content; + } + } + + return $contents; + } } diff --git a/contentbank/tests/contentbank_test.php b/contentbank/tests/contentbank_test.php index 199b73a4cf4d7..824f7a51a6be3 100644 --- a/contentbank/tests/contentbank_test.php +++ b/contentbank/tests/contentbank_test.php @@ -175,4 +175,143 @@ public function test_get_extension_supporter(array $supporters, string $extensio $supporter = $cb->get_extension_supporter($extension, $systemcontext); $this->assertEquals($expected, $supporter); } + + /** + * Test the behaviour of search_contents(). + * + * @dataProvider search_contents_provider + * @param string $search String to search. + * @param int $contextid Contextid to search. + * @param int $expectedresult Expected result. + * @param array $contexts List of contexts where to create content. + */ + public function test_search_contents(?string $search, int $contextid, int $expectedresult, array $contexts = []): void { + global $DB; + + $this->resetAfterTest(); + + // Create users. + $managerroleid = $DB->get_field('role', 'id', ['shortname' => 'manager']); + $manager = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->role_assign($managerroleid, $manager->id); + + // Add some content to the content bank. + $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank'); + foreach ($contexts as $context) { + $records = $generator->generate_contentbank_data('contenttype_h5p', 3, + $manager->id, $context, false); + } + + // Search for some content. + $cb = new \core_contentbank\contentbank(); + $contents = $cb->search_contents($search, $contextid); + + $this->assertCount($expectedresult, $contents); + if (!empty($contents) && !empty($search)) { + foreach ($contents as $content) { + $this->assertContains($search, $content->get_name()); + } + } + } + + /** + * Data provider for test_search_contents(). + * + * @return array + */ + public function search_contents_provider(): array { + // Create a category and a course. + $systemcontext = \context_system::instance(); + $coursecat = $this->getDataGenerator()->create_category(); + $course = $this->getDataGenerator()->create_course(); + $coursecatcontext = \context_coursecat::instance($coursecat->id); + $coursecontext = \context_course::instance($course->id); + + return [ + 'Search all content in all contexts' => [ + null, + 0, + 9, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in all contexts for existing string in all contents' => [ + 'content', + 0, + 9, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in all contexts for unexisting string in all contents' => [ + 'chocolate', + 0, + 0, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in all contexts for existing string in some contents' => [ + '1', + 0, + 3, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in all contexts for existing string in some contents (create only 1 context)' => [ + '1', + 0, + 1, + [$systemcontext] + ], + 'Search in system context for existing string in all contents' => [ + 'content', + $systemcontext->id, + 3, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in category context for unexisting string in all contents' => [ + 'chocolate', + $coursecatcontext->id, + 0, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in course context for existing string in some contents' => [ + '1', + $coursecontext->id, + 1, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in system context' => [ + null, + $systemcontext->id, + 3, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in course context with existing content' => [ + null, + $coursecontext->id, + 3, + [$systemcontext, $coursecatcontext, $coursecontext] + ], + 'Search in course context without existing content' => [ + null, + $coursecontext->id, + 0, + [$systemcontext, $coursecatcontext] + ], + 'Search in an empty contentbank' => [ + null, + 0, + 0, + [] + ], + 'Search in a context in an empty contentbank' => [ + null, + $systemcontext->id, + 0, + [] + ], + 'Search for a string in an empty contentbank' => [ + 'content', + 0, + 0, + [] + ], + ]; + } } From 1f8dfe79ce32e912dd9f077e9ad403ae8ace4b25 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Wed, 15 Apr 2020 09:15:15 +0200 Subject: [PATCH 2/3] MDL-67797 contentbank: use the search_contents method --- contentbank/index.php | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/contentbank/index.php b/contentbank/index.php index 3a3e9c31a00b5..bbf9786aee51c 100644 --- a/contentbank/index.php +++ b/contentbank/index.php @@ -27,6 +27,7 @@ require_login(); $contextid = optional_param('contextid', \context_system::instance()->id, PARAM_INT); +$search = optional_param('search', '', PARAM_CLEAN); $context = context::instance_by_id($contextid, MUST_EXIST); require_capability('moodle/contentbank:access', $context); @@ -46,27 +47,13 @@ $PAGE->set_pagetype('contenbank'); // Get all contents managed by active plugins to render. -$foldercontents = array(); -$contents = $DB->get_records('contentbank_content', ['contextid' => $contextid]); -foreach ($contents as $content) { - $plugin = core_plugin_manager::instance()->get_plugin_info($content->contenttype); - if (!$plugin || !$plugin->is_enabled()) { - continue; - } - $contentclass = "\\$content->contenttype\\content"; - if (class_exists($contentclass)) { - $contentmanager = new $contentclass($content); - if ($contentmanager->can_view()) { - $foldercontents[] = $contentmanager; - } - } -} +$cb = new \core_contentbank\contentbank(); +$foldercontents = $cb->search_contents($search, $contextid); // Get the toolbar ready. $toolbar = array (); if (has_capability('moodle/contentbank:upload', $context)) { // Don' show upload button if there's no plugin to support any file extension. - $cb = new \core_contentbank\contentbank(); $accepted = $cb->get_supported_extensions_as_string($context); if (!empty($accepted)) { $importurl = new moodle_url('/contentbank/upload.php', ['contextid' => $contextid]); From 1bb3663c196f30916b1d88f11d686b244c5a8795 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Thu, 23 Apr 2020 17:16:37 +0200 Subject: [PATCH 3/3] MDL-67797 contentbank: review class_exists uses --- contentbank/classes/contentbank.php | 37 +++++++++++++---------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/contentbank/classes/contentbank.php b/contentbank/classes/contentbank.php index c5ab6c13bb85e..0a21353a97488 100644 --- a/contentbank/classes/contentbank.php +++ b/contentbank/classes/contentbank.php @@ -15,7 +15,7 @@ // along with Moodle. If not, see . /** - * Content bank manager class + * Content bank class * * @package core_contentbank * @copyright 2020 Amaia Anabitarte @@ -25,7 +25,7 @@ namespace core_contentbank; /** - * Content bank manager class + * Content bank class * * @package core_contentbank * @copyright 2020 Amaia Anabitarte @@ -44,8 +44,9 @@ private function get_enabled_content_types(): array { $enabledtypes = \core\plugininfo\contenttype::get_enabled_plugins(); $types = []; foreach ($enabledtypes as $name) { - $classname = "\\contenttype_$name\\contenttype"; - if (class_exists($classname)) { + $contenttypeclassname = "\\contenttype_$name\\contenttype"; + $contentclassname = "\\contenttype_$name\\content"; + if (class_exists($contenttypeclassname) && class_exists($contentclassname)) { $types[] = $name; } } @@ -65,16 +66,14 @@ public function load_all_supported_extensions(): array { $supportedextensions = []; foreach ($this->get_enabled_content_types() as $type) { $classname = "\\contenttype_$type\\contenttype"; - if (class_exists($classname)) { - $manager = new $classname; - if ($manager->is_feature_supported($manager::CAN_UPLOAD)) { - $extensions = $manager->get_manageable_extensions(); - foreach ($extensions as $extension) { - if (array_key_exists($extension, $supportedextensions)) { - $supportedextensions[$extension][] = $type; - } else { - $supportedextensions[$extension] = [$type]; - } + $contenttype = new $classname; + if ($contenttype->is_feature_supported($contenttype::CAN_UPLOAD)) { + $extensions = $contenttype->get_manageable_extensions(); + foreach ($extensions as $extension) { + if (array_key_exists($extension, $supportedextensions)) { + $supportedextensions[$extension][] = $type; + } else { + $supportedextensions[$extension] = [$type]; } } } @@ -100,12 +99,10 @@ public function load_context_supported_extensions(\context $context = null): arr foreach ($supportedextensions as $extension => $types) { foreach ($types as $type) { $classname = "\\contenttype_$type\\contenttype"; - if (class_exists($classname)) { - $manager = new $classname($context); - if ($manager->can_upload()) { - $contextextensions[$extension] = $type; - break; - } + $contenttype = new $classname($context); + if ($contenttype->can_upload()) { + $contextextensions[$extension] = $type; + break; } } }