diff --git a/course/classes/category.php b/course/classes/category.php index 5812abdf133..a1e20c22a39 100644 --- a/course/classes/category.php +++ b/course/classes/category.php @@ -171,6 +171,24 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr debugging('Can not unset core_course_category instance properties!', DEBUG_DEVELOPER); } + /** + * Get list of plugin callback functions. + * + * @param string $name Callback function name. + * @return [callable] $pluginfunctions + */ + public function get_plugins_callback_function(string $name) : array { + $pluginfunctions = []; + if ($pluginsfunction = get_plugins_with_function($name)) { + foreach ($pluginsfunction as $plugintype => $plugins) { + foreach ($plugins as $pluginfunction) { + $pluginfunctions[] = $pluginfunction; + } + } + } + return $pluginfunctions; + } + /** * Create an iterator because magic vars can't be seen by 'foreach'. * @@ -1900,13 +1918,12 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr return false; } - $context = $this->get_context(); - if (!$this->is_uservisible() || - !has_capability('moodle/category:manage', $context)) { + if (!$this->has_manage_capability()) { return false; } // Check all child categories (not only direct children). + $context = $this->get_context(); $sql = context_helper::get_preload_record_columns_sql('ctx'); $childcategories = $DB->get_records_sql('SELECT c.id, c.visible, '. $sql. ' FROM {context} ctx '. @@ -1936,6 +1953,15 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr } } + // Check if plugins permit deletion of category content. + $pluginfunctions = $this->get_plugins_callback_function('can_course_category_delete'); + foreach ($pluginfunctions as $pluginfunction) { + // If at least one plugin does not permit deletion, stop and return false. + if (!$pluginfunction($this)) { + return false; + } + } + return true; } @@ -1961,13 +1987,9 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr $settimeout = core_php_time_limit::raise(); // Allow plugins to use this category before we completely delete it. - if ($pluginsfunction = get_plugins_with_function('pre_course_category_delete')) { - $category = $this->get_db_record(); - foreach ($pluginsfunction as $plugintype => $plugins) { - foreach ($plugins as $pluginfunction) { - $pluginfunction($category); - } - } + $pluginfunctions = $this->get_plugins_callback_function('pre_course_category_delete'); + foreach ($pluginfunctions as $pluginfunction) { + $pluginfunction($this->get_db_record()); } $deletedcourses = array(); @@ -2076,25 +2098,35 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr public function can_move_content_to($newcatid) { global $CFG; require_once($CFG->libdir . '/questionlib.php'); - $context = $this->get_context(); - if (!$this->is_uservisible() || - !has_capability('moodle/category:manage', $context)) { + + if (!$this->has_manage_capability()) { return false; } + $testcaps = array(); // If this category has courses in it, user must have 'course:create' capability in target category. if ($this->has_courses()) { $testcaps[] = 'moodle/course:create'; } // If this category has subcategories or questions, user must have 'category:manage' capability in target category. - if ($this->has_children() || question_context_has_any_questions($context)) { + if ($this->has_children() || question_context_has_any_questions($this->get_context())) { $testcaps[] = 'moodle/category:manage'; } - if (!empty($testcaps)) { - return has_all_capabilities($testcaps, context_coursecat::instance($newcatid)); + if (!empty($testcaps) && !has_all_capabilities($testcaps, context_coursecat::instance($newcatid))) { + // No sufficient capabilities to perform this task. + return false; + } + + // Check if plugins permit moving category content. + $pluginfunctions = $this->get_plugins_callback_function('can_course_category_delete_move'); + $newparentcat = self::get($newcatid, MUST_EXIST, true); + foreach ($pluginfunctions as $pluginfunction) { + // If at least one plugin does not permit move on deletion, stop and return false. + if (!$pluginfunction($this, $newparentcat)) { + return false; + } } - // There is no content but still return true. return true; } @@ -2124,6 +2156,12 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr $coursesids = $DB->get_fieldset_select('course', 'id', 'category = :category ORDER BY sortorder ASC', $params); $context = $this->get_context(); + // Allow plugins to make necessary changes before we move the category content. + $pluginfunctions = $this->get_plugins_callback_function('pre_course_category_delete_move'); + foreach ($pluginfunctions as $pluginfunction) { + $pluginfunction($this, $newparentcat); + } + if ($children) { foreach ($children as $childcat) { $childcat->change_parent_raw($newparentcat); @@ -2186,7 +2224,7 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr $event = \core\event\course_category_deleted::create(array( 'objectid' => $this->id, 'context' => $context, - 'other' => array('name' => $this->name) + 'other' => array('name' => $this->name, 'contentmovedcategoryid' => $newparentid) )); $event->set_coursecat($this); $event->trigger(); diff --git a/course/classes/deletecategory_form.php b/course/classes/deletecategory_form.php index b75f19f0fc8..ddbd43396ca 100644 --- a/course/classes/deletecategory_form.php +++ b/course/classes/deletecategory_form.php @@ -76,14 +76,23 @@ class core_course_deletecategory_form extends moodleform { // Describe the contents of this category. $contents = ''; if ($this->coursecat->has_children()) { - $contents .= '
  • ' . get_string('subcategories') . '
  • '; + $contents .= html_writer::tag('li', get_string('subcategories')); } if ($this->coursecat->has_courses()) { - $contents .= '
  • ' . get_string('courses') . '
  • '; + $contents .= html_writer::tag('li', get_string('courses')); } if (question_context_has_any_questions($categorycontext)) { - $contents .= '
  • ' . get_string('questionsinthequestionbank') . '
  • '; + $contents .= html_writer::tag('li', get_string('questionsinthequestionbank')); } + + // Check if plugins can provide more info. + $pluginfunctions = $this->coursecat->get_plugins_callback_function('get_course_category_contents'); + foreach ($pluginfunctions as $pluginfunction) { + if ($plugincontents = $pluginfunction($this->coursecat)) { + $contents .= html_writer::tag('li', $plugincontents); + } + } + if (!empty($contents)) { $mform->addElement('static', 'emptymessage', get_string('thiscategorycontains'), html_writer::tag('ul', $contents)); } else { @@ -92,7 +101,9 @@ class core_course_deletecategory_form extends moodleform { // Give the options for what to do. $mform->addElement('select', 'fulldelete', get_string('whattodo'), $options); + if (count($options) == 1) { + // Freeze selector if only one option available. $optionkeys = array_keys($options); $option = reset($optionkeys); $mform->hardFreeze('fulldelete'); @@ -111,10 +122,6 @@ class core_course_deletecategory_form extends moodleform { $mform->setType('categoryid', PARAM_ALPHANUM); $mform->addElement('hidden', 'action', 'deletecategory'); $mform->setType('action', PARAM_ALPHANUM); - $mform->addElement('hidden', 'sure'); - // This gets set by default to ensure that if the user changes it manually we can detect it. - $mform->setDefault('sure', md5(serialize($this->coursecat))); - $mform->setType('sure', PARAM_ALPHANUM); $this->add_action_buttons(true, get_string('delete')); } @@ -131,10 +138,11 @@ class core_course_deletecategory_form extends moodleform { if (empty($data['fulldelete']) && empty($data['newparent'])) { // When they have chosen the move option, they must specify a destination. $errors['newparent'] = get_string('required'); + return $errors; } - if ($data['sure'] !== md5(serialize($this->coursecat))) { - $errors['categorylabel'] = get_string('categorymodifiedcancel'); + if (!empty($data['newparent']) && !$this->coursecat->can_move_content_to($data['newparent'])) { + $errors['newparent'] = get_string('movecatcontentstoselected', 'error'); } return $errors; diff --git a/course/tests/category_hooks_test.php b/course/tests/category_hooks_test.php new file mode 100644 index 00000000000..52af5f710d7 --- /dev/null +++ b/course/tests/category_hooks_test.php @@ -0,0 +1,215 @@ +. + +/** + * Tests for class core_course_category methods invoking hooks. + * + * @package core_course + * @category test + * @copyright 2020 Ruslan Kabalin + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace tests\core_course; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/course/tests/fixtures/mock_hooks.php'); + +use PHPUnit\Framework\MockObject\MockObject; + +/** + * Functional test for class core_course_category methods invoking hooks. + */ +class core_course_category_hooks_testcase extends \advanced_testcase { + + protected function setUp() { + $this->resetAfterTest(); + $this->setAdminUser(); + } + + /** + * Provides mocked category configured for named callback function. + * + * get_plugins_callback_function will return callable prefixed with `tool_unittest_`, + * the actual callbacks are defined in mock_hooks.php fixture file. + * + * @param core_course_category $category Category to mock + * @param string $callback Callback function used in method we test. + * @return MockObject + */ + public function get_mock_category(\core_course_category $category, string $callback = '') : MockObject { + // Setup mock object for \core_course_category. + // Disable original constructor, since we can't use it directly since it is private. + $mockcategory = $this->getMockBuilder(\core_course_category::class) + ->setMethods(['get_plugins_callback_function']) + ->disableOriginalConstructor() + ->getMock(); + + // Define get_plugins_callback_function use and return value. + if (!empty($callback)) { + $mockcategory->method('get_plugins_callback_function') + ->with($this->equalTo($callback)) + ->willReturn(['tool_unittest_' . $callback]); + } + + // Modify constructor visibility and invoke mock object with real object. + // This is used to overcome private constructor. + $reflected = new \ReflectionClass(\core_course_category::class); + $constructor = $reflected->getConstructor(); + $constructor->setAccessible(true); + $constructor->invoke($mockcategory, $category->get_db_record()); + + return $mockcategory; + } + + public function test_can_course_category_delete_hook() { + $category1 = \core_course_category::create(array('name' => 'Cat1')); + $category2 = \core_course_category::create(array('name' => 'Cat2', 'parent' => $category1->id)); + $category3 = \core_course_category::create(array('name' => 'Cat3')); + + $mockcategory2 = $this->get_mock_category($category2, 'can_course_category_delete'); + + // Add course to mocked clone of category2. + $course1 = $this->getDataGenerator()->create_course(array('category' => $mockcategory2->id)); + + // Now configure fixture to return false for the callback. + mock_hooks::set_can_course_category_delete_return(false); + $this->assertFalse($mockcategory2->can_delete_full($category3->id)); + + // Now configure fixture to return true for the callback. + mock_hooks::set_can_course_category_delete_return(true); + $this->assertTrue($mockcategory2->can_delete_full($category3->id)); + + // Verify passed arguments. + $arguments = mock_hooks::get_calling_arguments(); + $this->assertCount(1, $arguments); + + // Argument 1 is the same core_course_category instance. + $argument = array_shift($arguments); + $this->assertSame($mockcategory2, $argument); + } + + public function test_can_course_category_delete_move_hook() { + $category1 = \core_course_category::create(array('name' => 'Cat1')); + $category2 = \core_course_category::create(array('name' => 'Cat2', 'parent' => $category1->id)); + $category3 = \core_course_category::create(array('name' => 'Cat3')); + + $mockcategory2 = $this->get_mock_category($category2, 'can_course_category_delete_move'); + + // Add course to mocked clone of category2. + $course1 = $this->getDataGenerator()->create_course(array('category' => $mockcategory2->id)); + + // Now configure fixture to return false for the callback. + mock_hooks::set_can_course_category_delete_move_return(false); + $this->assertFalse($mockcategory2->can_move_content_to($category3->id)); + + // Now configure fixture to return true for the callback. + mock_hooks::set_can_course_category_delete_move_return(true); + $this->assertTrue($mockcategory2->can_move_content_to($category3->id)); + + // Verify passed arguments. + $arguments = mock_hooks::get_calling_arguments(); + $this->assertCount(2, $arguments); + + // Argument 1 is the same core_course_category instance. + $argument = array_shift($arguments); + $this->assertSame($mockcategory2, $argument); + + // Argument 2 is referring to category 3. + $argument = array_shift($arguments); + $this->assertInstanceOf(\core_course_category::class, $argument); + $this->assertEquals($category3->id, $argument->id); + } + + public function test_pre_course_category_delete_hook() { + $category1 = \core_course_category::create(array('name' => 'Cat1')); + $category2 = \core_course_category::create(array('name' => 'Cat2', 'parent' => $category1->id)); + + $mockcategory2 = $this->get_mock_category($category2, 'pre_course_category_delete'); + $mockcategory2->delete_full(); + + // Verify passed arguments. + $arguments = mock_hooks::get_calling_arguments(); + $this->assertCount(1, $arguments); + + // Argument 1 is the category object. + $argument = array_shift($arguments); + $this->assertEquals($mockcategory2->get_db_record(), $argument); + } + + public function test_pre_course_category_delete_move_hook() { + $category1 = \core_course_category::create(array('name' => 'Cat1')); + $category2 = \core_course_category::create(array('name' => 'Cat2', 'parent' => $category1->id)); + $category3 = \core_course_category::create(array('name' => 'Cat3')); + + $mockcategory2 = $this->get_mock_category($category2, 'pre_course_category_delete_move'); + + // Add course to mocked clone of category2. + $course1 = $this->getDataGenerator()->create_course(array('category' => $mockcategory2->id)); + + $mockcategory2->delete_move($category3->id); + + // Verify passed arguments. + $arguments = mock_hooks::get_calling_arguments(); + $this->assertCount(2, $arguments); + + // Argument 1 is the same core_course_category instance. + $argument = array_shift($arguments); + $this->assertSame($mockcategory2, $argument); + + // Argument 2 is referring to category 3. + $argument = array_shift($arguments); + $this->assertInstanceOf(\core_course_category::class, $argument); + $this->assertEquals($category3->id, $argument->id); + } + + public function test_get_course_category_contents_hook() { + $category1 = \core_course_category::create(array('name' => 'Cat1')); + $category2 = \core_course_category::create(array('name' => 'Cat2', 'parent' => $category1->id)); + + $mockcategory2 = $this->get_mock_category($category2); + + // Define get_plugins_callback_function use in the mock, it is called twice for different callback in the form. + $mockcategory2->expects($this->exactly(2)) + ->method('get_plugins_callback_function') + ->withConsecutive( + [$this->equalTo('can_course_category_delete')], + [$this->equalTo('get_course_category_contents')] + ) + ->willReturn( + ['tool_unittest_can_course_category_delete'], + ['tool_unittest_get_course_category_contents'] + ); + + // Now configure fixture to return string for the callback. + $content = 'Bunch of test artefacts'; + mock_hooks::set_get_course_category_contents_return($content); + + $mform = new \core_course_deletecategory_form(null, $mockcategory2); + $this->expectOutputRegex("/
  • $content<\/li>/"); + $mform->display(); + + // Verify passed arguments. + $arguments = mock_hooks::get_calling_arguments(); + $this->assertCount(1, $arguments); + + // Argument 1 is the same core_course_category instance. + $argument = array_shift($arguments); + $this->assertSame($mockcategory2, $argument); + } +} diff --git a/course/tests/fixtures/mock_hooks.php b/course/tests/fixtures/mock_hooks.php new file mode 100644 index 00000000000..d5fc095ef8c --- /dev/null +++ b/course/tests/fixtures/mock_hooks.php @@ -0,0 +1,184 @@ +. + +/** + * Fixture for mocking callbacks used in \core_course_category + * + * @package core_course + * @category test + * @copyright 2020 Ruslan Kabalin + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace tests\core_course { + + /** + * Class mock_hooks + * + * @package core_course + * @category test + * @copyright 2020 Ruslan Kabalin + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + class mock_hooks { + /** @var bool $cancoursecategorydelete */ + private static $cancoursecategorydelete = true; + + /** @var bool $cancoursecategorydeletemove */ + private static $cancoursecategorydeletemove = true; + + /** @var string $getcoursecategorycontents */ + private static $getcoursecategorycontents = ''; + + /** @var array $callingarguments */ + private static $callingarguments = []; + + /** + * Set calling arguments. + * + * This is supposed to be used in the callbacks to store arguments passed to callback. + * + * @param array $callingarguments + */ + public static function set_calling_arguments($callingarguments) { + self::$callingarguments = $callingarguments; + } + + /** + * Get calling arguments. + * + * This is supposed to be used in the test to verify arguments passed to callback. + * This method also reset stored calling arguments. + * + * @return array $callingarguments + */ + public static function get_calling_arguments() { + $callingarguments = self::$callingarguments; + self::$callingarguments = []; + return $callingarguments; + } + + /** + * Get can_course_category_delete callback return. + * + * @return bool + */ + public static function get_can_course_category_delete_return() : bool { + return self::$cancoursecategorydelete; + } + + /** + * Sets can_course_category_delete callback return. + * + * @param bool $return + */ + public static function set_can_course_category_delete_return(bool $return) { + self::$cancoursecategorydelete = $return; + } + + /** + * Get can_course_category_delete_move callback return. + * + * @return bool + */ + public static function get_can_course_category_delete_move_return() : bool { + return self::$cancoursecategorydeletemove; + } + + /** + * Sets can_course_category_delete_move callback return. + * + * @param bool $return + */ + public static function set_can_course_category_delete_move_return(bool $return) { + self::$cancoursecategorydeletemove = $return; + } + + /** + * Get get_course_category_contents callback return. + * + * @return string + */ + public static function get_get_course_category_contents_return() : string { + return self::$getcoursecategorycontents; + } + + /** + * Sets get_course_category_contents callback return. + * + * @param string $return + */ + public static function set_get_course_category_contents_return(string $return) { + self::$getcoursecategorycontents = $return; + } + } +} + +namespace { + + /** + * Test pre_course_category_delete callback. + * + * @param object $category + */ + function tool_unittest_pre_course_category_delete(object $category) { + \tests\core_course\mock_hooks::set_calling_arguments(func_get_args()); + } + + /** + * Test pre_course_category_delete_move callback. + * + * @param core_course_category $category + * @param core_course_category $newcategory + */ + function tool_unittest_pre_course_category_delete_move(core_course_category $category, core_course_category $newcategory) { + \tests\core_course\mock_hooks::set_calling_arguments(func_get_args()); + } + + /** + * Test can_course_category_delete callback. + * + * @param core_course_category $category + * @return bool + */ + function tool_unittest_can_course_category_delete(core_course_category $category) { + \tests\core_course\mock_hooks::set_calling_arguments(func_get_args()); + return \tests\core_course\mock_hooks::get_can_course_category_delete_return(); + } + + /** + * Test can_course_category_delete_move callback. + * + * @param core_course_category $category + * @param core_course_category $newcategory + * @return bool + */ + function tool_unittest_can_course_category_delete_move(core_course_category $category, core_course_category $newcategory) { + \tests\core_course\mock_hooks::set_calling_arguments(func_get_args()); + return \tests\core_course\mock_hooks::get_can_course_category_delete_move_return(); + } + + /** + * Test get_course_category_contents callback. + * + * @param core_course_category $category + * @return string + */ + function tool_unittest_get_course_category_contents(core_course_category $category) { + \tests\core_course\mock_hooks::set_calling_arguments(func_get_args()); + return \tests\core_course\mock_hooks::get_get_course_category_contents_return(); + } +} \ No newline at end of file diff --git a/lang/en/error.php b/lang/en/error.php index cf78c4c5286..5a6c4637c54 100644 --- a/lang/en/error.php +++ b/lang/en/error.php @@ -405,6 +405,7 @@ $string['moduledoesnotexist'] = 'This module does not exist'; $string['moduleinstancedoesnotexist'] = 'The instance of this module does not exist'; $string['modulemissingcode'] = 'Module {$a} is missing the code needed to perform this function'; $string['movecatcontentstoroot'] = 'Moving the category content to root is not allowed. You must move the contents to an existing category!'; +$string['movecatcontentstoselected'] = 'Some of the category content can not be moved into selected category.'; $string['movecategorynotpossible'] = 'You cannot move category \'{$a}\' into the selected category.'; $string['movecategoryownparent'] = 'You cannot make category \'{$a}\' a parent of itself.'; $string['movecategoryparentconflict'] = 'You cannot make category \'{$a}\' a subcategory of one of its own subcategories.'; diff --git a/lib/classes/event/course_category_deleted.php b/lib/classes/event/course_category_deleted.php index 2e5c73a9d50..bac3029b1f9 100644 --- a/lib/classes/event/course_category_deleted.php +++ b/lib/classes/event/course_category_deleted.php @@ -33,6 +33,7 @@ defined('MOODLE_INTERNAL') || die(); * Extra information about event. * * - string name: category name. + * - string contentmovedcategoryid: (optional) category id where content was moved on deletion * } * * @package core @@ -71,7 +72,11 @@ class course_category_deleted extends base { * @return string */ public function get_description() { - return "The user with id '$this->userid' deleted the course category with id '$this->objectid'."; + $descr = "The user with id '$this->userid' deleted the course category with id '$this->objectid'."; + if (!empty($this->other['contentmovedcategoryid'])) { + $descr .= " Its content has been moved to category with id '{$this->other['contentmovedcategoryid']}'."; + } + return $descr; } /** diff --git a/lib/upgrade.txt b/lib/upgrade.txt index c6e0d0d38d8..36a0bb464d8 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -71,6 +71,20 @@ information provided here is intended especially for developers. - cron_execute_plugin_type() - cron_bc_hack_plugin_functions() Please, use the Task API instead: https://docs.moodle.org/dev/Task_API +* Introduce new hooks for plugin developers: + - _can_course_category_delete($category) + - _can_course_category_delete_move($category, $newcategory) + These hooks allow plugin developers greater control over category deletion. Plugin can return false in those + functions if category deletion or deletion with content move to the new parent category is not permitted. + Both $category and $newcategory params are instances of core_course_category class. + - _pre_course_category_delete_move($category, $newcategory) + This hook is expanding functionality of existing _pre_course_category_delete hook and allow plugin developers + to execute code prior to category deletion when its content is moved to another category. + Both $category and $newcategory params are instances of core_course_category class. + - _get_course_category_contents($category) + This hook allow plugin developers to add information that is displayed on category deletion form. Function should + return string, which will be added to the list of category contents shown on the form. $category param is an instance + of core_course_category class. === 3.8 === * Add CLI option to notify all cron tasks to stop: admin/cli/cron.php --stop