From 36f588f8fb325ed2609f3e361ef98f66da678a33 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 1 Aug 2019 18:14:29 +1000 Subject: [PATCH 1/3] MDL-56789 core: Added unit test for course_modules_pending_deletion --- course/tests/courselib_test.php | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index 6eb4a3696d7..59debc09520 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -5092,4 +5092,39 @@ class core_course_courselib_testcase extends advanced_testcase { $this->assertCount(3, $result); $this->assertArrayNotHasKey($courses[0]->id, $result); } + + /** + * Data provider for test_course_modules_pending_deletion. + * + * @return array An array of arrays contain test data + */ + public function provider_course_modules_pending_deletion() { + return [ + ['forum', true], + ['assign', true], + ]; + } + + /** + * Tests the function course_modules_pending_deletion. + * + * @param string $module The module we want to test with + * @param bool $expected The expected result + * @dataProvider provider_course_modules_pending_deletion + */ + public function test_course_modules_pending_deletion(string $module, bool $expected) { + $this->resetAfterTest(); + + // Ensure recyclebin is enabled. + set_config('coursebinenable', true, 'tool_recyclebin'); + + // Create course and modules. + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + + $moduleinstance = $generator->create_module($module, array('course' => $course->id)); + + course_delete_module($moduleinstance->cmid, true); // Try to delete the instance asynchronously. + $this->assertEquals($expected, course_modules_pending_deletion($course->id)); + } } From ae0e8015c5bb00de7bb99bd768e0bab1c11eb0d7 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Sat, 22 Jun 2019 01:56:37 +1000 Subject: [PATCH 2/3] MDL-56789 core: Recycle bin warn only if a grade item is being deleted --- course/lib.php | 24 ++++++++++++++++++++++-- course/tests/courselib_test.php | 11 +++++++---- grade/lib.php | 2 +- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/course/lib.php b/course/lib.php index 2294e71c862..aa67ccb0083 100644 --- a/course/lib.php +++ b/course/lib.php @@ -1289,16 +1289,36 @@ function course_module_flag_for_async_deletion($cmid) { * Checks whether the given course has any course modules scheduled for adhoc deletion. * * @param int $courseid the id of the course. + * @param bool $onlygradable whether to check only gradable modules or all modules. * @return bool true if the course contains any modules pending deletion, false otherwise. */ -function course_modules_pending_deletion($courseid) { +function course_modules_pending_deletion($courseid, bool $onlygradable = false) : bool { if (empty($courseid)) { return false; } + + if ($onlygradable) { + // Fetch modules with grade items. + if (!$coursegradeitems = grade_item::fetch_all(['itemtype' => 'mod', 'courseid' => $courseid])) { + // Return early when there is none. + return false; + } + } + $modinfo = get_fast_modinfo($courseid); foreach ($modinfo->get_cms() as $module) { if ($module->deletioninprogress == '1') { - return true; + if ($onlygradable) { + // Check if the module being deleted is in the list of course modules with grade items. + foreach ($coursegradeitems as $coursegradeitem) { + if ($coursegradeitem->itemmodule == $module->modname && $coursegradeitem->iteminstance == $module->instance) { + // The module being deleted is within the gradable modules. + return true; + } + } + } else { + return true; + } } } return false; diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index 59debc09520..fd6907107e0 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -5100,8 +5100,10 @@ class core_course_courselib_testcase extends advanced_testcase { */ public function provider_course_modules_pending_deletion() { return [ - ['forum', true], - ['assign', true], + ['forum', false, true], + ['assign', false, true], + ['forum', true, false], + ['assign', true, true], ]; } @@ -5109,10 +5111,11 @@ class core_course_courselib_testcase extends advanced_testcase { * Tests the function course_modules_pending_deletion. * * @param string $module The module we want to test with + * @param bool $gradable The value to pass to the gradable argument of the course_modules_pending_deletion function * @param bool $expected The expected result * @dataProvider provider_course_modules_pending_deletion */ - public function test_course_modules_pending_deletion(string $module, bool $expected) { + public function test_course_modules_pending_deletion(string $module, bool $gradable, bool $expected) { $this->resetAfterTest(); // Ensure recyclebin is enabled. @@ -5125,6 +5128,6 @@ class core_course_courselib_testcase extends advanced_testcase { $moduleinstance = $generator->create_module($module, array('course' => $course->id)); course_delete_module($moduleinstance->cmid, true); // Try to delete the instance asynchronously. - $this->assertEquals($expected, course_modules_pending_deletion($course->id)); + $this->assertEquals($expected, course_modules_pending_deletion($course->id, $gradable)); } } diff --git a/grade/lib.php b/grade/lib.php index 7a976615b5b..67857cb02fa 100644 --- a/grade/lib.php +++ b/grade/lib.php @@ -986,7 +986,7 @@ function print_grade_page_head($courseid, $active_type, $active_plugin=null, // Put a warning on all gradebook pages if the course has modules currently scheduled for background deletion. require_once($CFG->dirroot . '/course/lib.php'); - if (course_modules_pending_deletion($courseid)) { + if (course_modules_pending_deletion($courseid, true)) { \core\notification::add(get_string('gradesmoduledeletionpendingwarning', 'grades'), \core\output\notification::NOTIFY_WARNING); } From 0157d682d5a0b749066e34cdaf71f5d600942c20 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Wed, 7 Aug 2019 15:36:09 +1000 Subject: [PATCH 3/3] MDL-56789 core: Improve unit tests --- course/tests/courselib_test.php | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index fd6907107e0..1e9cddc3a06 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -5100,22 +5100,27 @@ class core_course_courselib_testcase extends advanced_testcase { */ public function provider_course_modules_pending_deletion() { return [ - ['forum', false, true], - ['assign', false, true], - ['forum', true, false], - ['assign', true, true], + 'Non-gradable activity, check all' => [['forum'], 0, false, true], + 'Gradable activity, check all' => [['assign'], 0, false, true], + 'Non-gradable activity, check gradables' => [['forum'], 0, true, false], + 'Gradable activity, check gradables' => [['assign'], 0, true, true], + 'Non-gradable within multiple, check all' => [['quiz', 'forum', 'assign'], 1, false, true], + 'Non-gradable within multiple, check gradables' => [['quiz', 'forum', 'assign'], 1, true, false], + 'Gradable within multiple, check all' => [['quiz', 'forum', 'assign'], 2, false, true], + 'Gradable within multiple, check gradables' => [['quiz', 'forum', 'assign'], 2, true, true], ]; } /** * Tests the function course_modules_pending_deletion. * - * @param string $module The module we want to test with + * @param string[] $modules A complete list aff all available modules before deletion + * @param int $indextodelete The index of the module in the $modules array that we want to test with * @param bool $gradable The value to pass to the gradable argument of the course_modules_pending_deletion function * @param bool $expected The expected result * @dataProvider provider_course_modules_pending_deletion */ - public function test_course_modules_pending_deletion(string $module, bool $gradable, bool $expected) { + public function test_course_modules_pending_deletion(array $modules, int $indextodelete, bool $gradable, bool $expected) { $this->resetAfterTest(); // Ensure recyclebin is enabled. @@ -5125,9 +5130,12 @@ class core_course_courselib_testcase extends advanced_testcase { $generator = $this->getDataGenerator(); $course = $generator->create_course(); - $moduleinstance = $generator->create_module($module, array('course' => $course->id)); + $moduleinstances = []; + foreach ($modules as $module) { + $moduleinstances[] = $generator->create_module($module, array('course' => $course->id)); + } - course_delete_module($moduleinstance->cmid, true); // Try to delete the instance asynchronously. + course_delete_module($moduleinstances[$indextodelete]->cmid, true); // Try to delete the instance asynchronously. $this->assertEquals($expected, course_modules_pending_deletion($course->id, $gradable)); } }