diff --git a/admin/tool/recyclebin/lib.php b/admin/tool/recyclebin/lib.php index ee2eb748633..d4e748bd1b6 100644 --- a/admin/tool/recyclebin/lib.php +++ b/admin/tool/recyclebin/lib.php @@ -152,6 +152,17 @@ function tool_recyclebin_pre_course_module_delete($cm) { } } +/** + * Hook called to check whether async course module deletion should be performed or not. + * + * @return true if background deletion is required (is the recyclebin is enabled), false otherwise. + */ +function tool_recyclebin_course_module_background_deletion_recommended() { + if (\tool_recyclebin\course_bin::is_enabled()) { + return true; + } +} + /** * Hook called before we delete a course. * diff --git a/admin/tool/recyclebin/tests/behat/backup_user_data.feature b/admin/tool/recyclebin/tests/behat/backup_user_data.feature index 500a2730b2f..c0bc961a89e 100644 --- a/admin/tool/recyclebin/tests/behat/backup_user_data.feature +++ b/admin/tool/recyclebin/tests/behat/backup_user_data.feature @@ -58,6 +58,7 @@ Feature: Backup user data And I follow "Course 1" And I turn editing mode on And I delete "Quiz 1" activity + And I run all adhoc tasks And I navigate to "Recycle bin" node in "Course administration" And I should see "Quiz 1" And I click on "Restore" "link" in the "region-main" "region" diff --git a/admin/tool/recyclebin/tests/behat/basic_functionality.feature b/admin/tool/recyclebin/tests/behat/basic_functionality.feature index ded5e99f349..b10af91f9e4 100644 --- a/admin/tool/recyclebin/tests/behat/basic_functionality.feature +++ b/admin/tool/recyclebin/tests/behat/basic_functionality.feature @@ -69,6 +69,7 @@ Feature: Basic recycle bin functionality | Assignment name | Test assign | | Description | Test | And I delete "Test assign" activity + And I run all adhoc tasks And I navigate to "Recycle bin" node in "Course administration" When I click on "Delete" "link" Then I should see "Are you sure you want to delete the selected item from the recycle bin?" @@ -92,6 +93,7 @@ Feature: Basic recycle bin functionality | Description | Test 2 | And I delete "Test assign 1" activity And I delete "Test assign 2" activity + And I run all adhoc tasks And I navigate to "Recycle bin" node in "Course administration" And I should see "Test assign 1" And I should see "Test assign 2" diff --git a/admin/tool/recyclebin/tests/course_bin_test.php b/admin/tool/recyclebin/tests/course_bin_test.php index 1f83de31317..9a4e0c37f3a 100644 --- a/admin/tool/recyclebin/tests/course_bin_test.php +++ b/admin/tool/recyclebin/tests/course_bin_test.php @@ -71,6 +71,9 @@ class tool_recyclebin_course_bin_tests extends advanced_testcase { // Delete the course module. course_delete_module($this->quiz->cmid); + // Now, run the course module deletion adhoc task. + phpunit_util::run_all_adhoc_tasks(); + // Check the course module is now in the recycle bin. $this->assertEquals(1, $DB->count_records('tool_recyclebin_course')); @@ -112,6 +115,9 @@ class tool_recyclebin_course_bin_tests extends advanced_testcase { // Delete the course module. course_delete_module($this->quiz->cmid); + // Now, run the course module deletion adhoc task. + phpunit_util::run_all_adhoc_tasks(); + // Try purging. $recyclebin = new \tool_recyclebin\course_bin($this->course->id); foreach ($recyclebin->get_items() as $item) { @@ -134,6 +140,9 @@ class tool_recyclebin_course_bin_tests extends advanced_testcase { // Delete the quiz. course_delete_module($this->quiz->cmid); + // Now, run the course module deletion adhoc task. + phpunit_util::run_all_adhoc_tasks(); + // Set deleted date to the distant past. $recyclebin = new \tool_recyclebin\course_bin($this->course->id); foreach ($recyclebin->get_items() as $item) { @@ -147,6 +156,9 @@ class tool_recyclebin_course_bin_tests extends advanced_testcase { course_delete_module($book->cmid); + // Now, run the course module deletion adhoc task. + phpunit_util::run_all_adhoc_tasks(); + // Should have 2 items now. $this->assertEquals(2, count($recyclebin->get_items())); diff --git a/availability/condition/completion/classes/frontend.php b/availability/condition/completion/classes/frontend.php index 02dad76870d..8b425d73c24 100644 --- a/availability/condition/completion/classes/frontend.php +++ b/availability/condition/completion/classes/frontend.php @@ -64,7 +64,7 @@ class frontend extends \core_availability\frontend { foreach ($modinfo->cms as $id => $othercm) { // Add each course-module if it has completion turned on and is not // the one currently being edited. - if ($othercm->completion && (empty($cm) || $cm->id != $id)) { + if ($othercm->completion && (empty($cm) || $cm->id != $id) && !$othercm->deletioninprogress) { $cms[] = (object)array('id' => $id, 'name' => format_string($othercm->name, true, array('context' => $context))); } diff --git a/availability/condition/grade/classes/frontend.php b/availability/condition/grade/classes/frontend.php index 4a906c7d4a5..aee9f9f7739 100644 --- a/availability/condition/grade/classes/frontend.php +++ b/availability/condition/grade/classes/frontend.php @@ -42,6 +42,7 @@ class frontend extends \core_availability\frontend { \section_info $section = null) { global $DB, $CFG; require_once($CFG->libdir . '/gradelib.php'); + require_once($CFG->dirroot . '/course/lib.php'); // Get grades as basic associative array. $gradeoptions = array(); @@ -49,6 +50,10 @@ class frontend extends \core_availability\frontend { // For some reason the fetch_all things return null if none. $items = $items ? $items : array(); foreach ($items as $id => $item) { + // Don't include the grade item if it's linked with a module that is being deleted. + if (course_module_instance_pending_deletion($item->courseid, $item->itemmodule, $item->iteminstance)) { + continue; + } // Do not include grades for current item. if ($cm && $cm->instance == $item->iteminstance && $cm->modname == $item->itemmodule diff --git a/backup/util/dbops/backup_plan_dbops.class.php b/backup/util/dbops/backup_plan_dbops.class.php index 530763752be..615f802fc6c 100644 --- a/backup/util/dbops/backup_plan_dbops.class.php +++ b/backup/util/dbops/backup_plan_dbops.class.php @@ -87,7 +87,8 @@ abstract class backup_plan_dbops extends backup_dbops { FROM {course_modules} cm JOIN {modules} m ON m.id = cm.module WHERE cm.course = ? - AND cm.section = ?", array($courseid, $sectionid)); + AND cm.section = ? + AND cm.deletioninprogress <> 1", array($courseid, $sectionid)); foreach (explode(',', $sequence) as $moduleid) { if (isset($modules[$moduleid])) { $module = array('id' => $modules[$moduleid]->id, 'modname' => $modules[$moduleid]->modname); diff --git a/backup/util/loggers/base_logger.class.php b/backup/util/loggers/base_logger.class.php index 32b0c06c939..26bb05cec4a 100644 --- a/backup/util/loggers/base_logger.class.php +++ b/backup/util/loggers/base_logger.class.php @@ -114,7 +114,8 @@ abstract class base_logger implements checksumable { public final function process($message, $level, $options = null) { $result = true; - if ($this->level != backup::LOG_NONE && $this->level >= $level) { // Perform action conditionally + if ($this->level != backup::LOG_NONE && $this->level >= $level + && !(defined('BEHAT_TEST') && BEHAT_TEST)) { // Perform action conditionally. $result = $this->action($message, $level, $options); } if ($result === false) { // Something was wrong, stop the chain diff --git a/blocks/recent_activity/tests/behat/structural_changes.feature b/blocks/recent_activity/tests/behat/structural_changes.feature index d66978a4be6..5914281d57b 100644 --- a/blocks/recent_activity/tests/behat/structural_changes.feature +++ b/blocks/recent_activity/tests/behat/structural_changes.feature @@ -194,6 +194,7 @@ Feature: View structural changes in recent activity block And I follow "Course 1" And I turn editing mode on And I delete "ForumUpdated" activity + And I run all adhoc tasks And I log out And I wait "1" seconds # Students 1 and 2 see that forum was deleted diff --git a/course/classes/task/course_delete_modules.php b/course/classes/task/course_delete_modules.php new file mode 100644 index 00000000000..3770537451a --- /dev/null +++ b/course/classes/task/course_delete_modules.php @@ -0,0 +1,62 @@ +. + +/** + * Adhoc task handling course module deletion. + * + * @package core_course + * @copyright 2016 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace core_course\task; + +defined('MOODLE_INTERNAL') || die(); +/** + * Class handling course module deletion. + * + * This task supports an array of course module object as custom_data, and calls course_delete_module() in synchronous deletion + * mode for each of them. + * This will: + * 1. call any 'mod_xxx_pre_course_module_deleted' functions (e.g. Recycle bin) + * 2. delete the module + * 3. fire the deletion event + * + * @package core_course + * @copyright 2016 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class course_delete_modules extends \core\task\adhoc_task { + + /** + * Run the deletion task. + * + * @throws \coding_exception if the module could not be removed. + */ + public function execute() { + global $CFG; + require_once($CFG->dirroot. '/course/lib.php'); + + $cms = $this->get_custom_data()->cms; + foreach ($cms as $cm) { + try { + course_delete_module($cm->id); + } catch (\Exception $e) { + throw new \coding_exception("The course module {$cm->id} could not be deleted. $e->getTraceAsString()"); + } + } + } +} diff --git a/course/editsection.php b/course/editsection.php index 14b74932488..231935f31be 100644 --- a/course/editsection.php +++ b/course/editsection.php @@ -50,7 +50,7 @@ if ($deletesection) { if (course_can_delete_section($course, $sectioninfo)) { $confirm = optional_param('confirm', false, PARAM_BOOL) && confirm_sesskey(); if ($confirm) { - course_delete_section($course, $sectioninfo, true); + course_delete_section($course, $sectioninfo, true, true); $courseurl = course_get_url($course, 0, array('sr' => $sectionreturn)); redirect($courseurl); } else { diff --git a/course/lib.php b/course/lib.php index 553632602e9..353845d3c99 100644 --- a/course/lib.php +++ b/course/lib.php @@ -430,6 +430,7 @@ function get_array_of_activities($courseid) { $mod[$seq]->completionexpected = $rawmods[$seq]->completionexpected; $mod[$seq]->showdescription = $rawmods[$seq]->showdescription; $mod[$seq]->availability = $rawmods[$seq]->availability; + $mod[$seq]->deletioninprogress = $rawmods[$seq]->deletioninprogress; $modname = $mod[$seq]->mod; $functionname = $modname."_get_coursemodule_info"; @@ -504,7 +505,7 @@ function get_array_of_activities($courseid) { foreach (array('idnumber', 'groupmode', 'groupingid', 'indent', 'completion', 'extra', 'extraclasses', 'iconurl', 'onclick', 'content', 'icon', 'iconcomponent', 'customdata', 'availability', 'completionview', - 'completionexpected', 'score', 'showdescription') as $property) { + 'completionexpected', 'score', 'showdescription', 'deletioninprogress') as $property) { if (property_exists($mod[$seq], $property) && empty($mod[$seq]->{$property})) { unset($mod[$seq]->{$property}); @@ -1072,9 +1073,25 @@ function set_coursemodule_name($id, $name) { * event to the DB. * * @param int $cmid the course module id + * @param bool $async whether or not to try to delete the module using an adhoc task. Async also depends on a plugin hook. + * @throws moodle_exception * @since Moodle 2.5 */ -function course_delete_module($cmid) { +function course_delete_module($cmid, $async = false) { + // Check the 'course_module_background_deletion_recommended' hook first. + // Only use asynchronous deletion if at least one plugin returns true and if async deletion has been requested. + // Both are checked because plugins should not be allowed to dictate the deletion behaviour, only support/decline it. + // It's up to plugins to handle things like whether or not they are enabled. + if ($async && $pluginsfunction = get_plugins_with_function('course_module_background_deletion_recommended')) { + foreach ($pluginsfunction as $plugintype => $plugins) { + foreach ($plugins as $pluginfunction) { + if ($pluginfunction()) { + return course_module_flag_for_async_deletion($cmid); + } + } + } + } + global $CFG, $DB; require_once($CFG->libdir.'/gradelib.php'); @@ -1192,6 +1209,104 @@ function course_delete_module($cmid) { rebuild_course_cache($cm->course, true); } +/** + * Schedule a course module for deletion in the background using an adhoc task. + * + * This method should not be called directly. Instead, please use course_delete_module($cmid, true), to denote async deletion. + * The real deletion of the module is handled by the task, which calls 'course_delete_module($cmid)'. + * + * @param int $cmid the course module id. + * @return bool whether the module was successfully scheduled for deletion. + * @throws \moodle_exception + */ +function course_module_flag_for_async_deletion($cmid) { + global $CFG, $DB; + require_once($CFG->libdir.'/gradelib.php'); + require_once($CFG->libdir.'/questionlib.php'); + require_once($CFG->dirroot.'/blog/lib.php'); + require_once($CFG->dirroot.'/calendar/lib.php'); + + // Get the course module. + if (!$cm = $DB->get_record('course_modules', array('id' => $cmid))) { + return true; + } + + // We need to be reasonably certain the deletion is going to succeed before we background the process. + // Make the necessary delete_instance checks, etc. before proceeding further. Throw exceptions if required. + + // Get the course module name. + $modulename = $DB->get_field('modules', 'name', array('id' => $cm->module), MUST_EXIST); + + // Get the file location of the delete_instance function for this module. + $modlib = "$CFG->dirroot/mod/$modulename/lib.php"; + + // Include the file required to call the delete_instance function for this module. + if (file_exists($modlib)) { + require_once($modlib); + } else { + throw new \moodle_exception('cannotdeletemodulemissinglib', '', '', null, + "Cannot delete this module as the file mod/$modulename/lib.php is missing."); + } + + $deleteinstancefunction = $modulename . '_delete_instance'; + + // Ensure the delete_instance function exists for this module. + if (!function_exists($deleteinstancefunction)) { + throw new \moodle_exception('cannotdeletemodulemissingfunc', '', '', null, + "Cannot delete this module as the function {$modulename}_delete_instance is missing in mod/$modulename/lib.php."); + } + + // We are going to defer the deletion as we can't be sure how long the module's pre_delete code will run for. + $cm->deletioninprogress = '1'; + $DB->update_record('course_modules', $cm); + + // Create an adhoc task for the deletion of the course module. The task takes an array of course modules for removal. + $removaltask = new \core_course\task\course_delete_modules(); + $removaltask->set_custom_data(array('cms' => array($cm))); + + // Queue the task for the next run. + \core\task\manager::queue_adhoc_task($removaltask); + + // Reset the course cache to hide the module. + rebuild_course_cache($cm->course, true); +} + +/** + * Checks whether the given course has any course modules scheduled for adhoc deletion. + * + * @param int $courseid the id of the course. + * @return bool true if the course contains any modules pending deletion, false otherwise. + */ +function course_modules_pending_deletion($courseid) { + if (empty($courseid)) { + return false; + } + $modinfo = get_fast_modinfo($courseid); + foreach ($modinfo->get_cms() as $module) { + if ($module->deletioninprogress == '1') { + return true; + } + } + return false; +} + +/** + * Checks whether the course module, as defined by modulename and instanceid, is scheduled for deletion within the given course. + * + * @param int $courseid the course id. + * @param string $modulename the module name. E.g. 'assign', 'book', etc. + * @param int $instanceid the module instance id. + * @return bool true if the course module is pending deletion, false otherwise. + */ +function course_module_instance_pending_deletion($courseid, $modulename, $instanceid) { + if (empty($courseid) || empty($modulename) || empty($instanceid)) { + return false; + } + $modinfo = get_fast_modinfo($courseid); + $instances = $modinfo->get_instances_of($modulename); + return isset($instances[$instanceid]) && $instances[$instanceid]->deletioninprogress; +} + function delete_mod_from_section($modid, $sectionid) { global $DB; @@ -1285,9 +1400,10 @@ function move_section_to($course, $section, $destination, $ignorenumsections = f * @param int|stdClass $course * @param int|stdClass|section_info $section * @param bool $forcedeleteifnotempty if set to false section will not be deleted if it has modules in it. + * @param bool $async whether or not to try to delete the section using an adhoc task. Async also depends on a plugin hook. * @return bool whether section was deleted */ -function course_delete_section($course, $section, $forcedeleteifnotempty = true) { +function course_delete_section($course, $section, $forcedeleteifnotempty = true, $async = false) { global $DB; // Prepare variables. @@ -1298,6 +1414,21 @@ function course_delete_section($course, $section, $forcedeleteifnotempty = true) // No section exists, can't proceed. return false; } + + // Check the 'course_module_background_deletion_recommended' hook first. + // Only use asynchronous deletion if at least one plugin returns true and if async deletion has been requested. + // Both are checked because plugins should not be allowed to dictate the deletion behaviour, only support/decline it. + // It's up to plugins to handle things like whether or not they are enabled. + if ($async && $pluginsfunction = get_plugins_with_function('course_module_background_deletion_recommended')) { + foreach ($pluginsfunction as $plugintype => $plugins) { + foreach ($plugins as $pluginfunction) { + if ($pluginfunction()) { + return course_delete_section_async($section, $forcedeleteifnotempty); + } + } + } + } + $format = course_get_format($course); $sectionname = $format->get_section_name($section); @@ -1308,22 +1439,102 @@ function course_delete_section($course, $section, $forcedeleteifnotempty = true) if ($result) { $context = context_course::instance($courseid); $event = \core\event\course_section_deleted::create( - array( - 'objectid' => $section->id, - 'courseid' => $courseid, - 'context' => $context, - 'other' => array( - 'sectionnum' => $section->section, - 'sectionname' => $sectionname, - ) + array( + 'objectid' => $section->id, + 'courseid' => $courseid, + 'context' => $context, + 'other' => array( + 'sectionnum' => $section->section, + 'sectionname' => $sectionname, ) - ); + ) + ); $event->add_record_snapshot('course_sections', $section); $event->trigger(); } return $result; } +/** + * Course section deletion, using an adhoc task for deletion of the modules it contains. + * 1. Schedule all modules within the section for adhoc removal. + * 2. Move all modules to course section 0. + * 3. Delete the resulting empty section. + * + * @param \stdClass $section the section to schedule for deletion. + * @param bool $forcedeleteifnotempty whether to force section deletion if it contains modules. + * @return bool true if the section was scheduled for deletion, false otherwise. + */ +function course_delete_section_async($section, $forcedeleteifnotempty = true) { + global $DB; + + // Objects only, and only valid ones. + if (!is_object($section) || empty($section->id)) { + return false; + } + + // Does the object currently exist in the DB for removal (check for stale objects). + $section = $DB->get_record('course_sections', array('id' => $section->id)); + if (!$section || !$section->section) { + // No section exists, or the section is 0. Can't proceed. + return false; + } + + // Check whether the section can be removed. + if (!$forcedeleteifnotempty && (!empty($section->sequence) || !empty($section->summary))) { + return false; + } + + $format = course_get_format($section->course); + $sectionname = $format->get_section_name($section); + + // Flag those modules having no existing deletion flag. Some modules may have been scheduled for deletion manually, and we don't + // want to create additional adhoc deletion tasks for these. Moving them to section 0 will suffice. + $affectedmods = $DB->get_records_select('course_modules', 'course = ? AND section = ? AND deletioninprogress <> ?', + [$section->course, $section->id, 1], '', 'id'); + $DB->set_field('course_modules', 'deletioninprogress', '1', ['course' => $section->course, 'section' => $section->id]); + + // Move all modules to section 0. + $modules = $DB->get_records('course_modules', ['section' => $section->id], ''); + $sectionzero = $DB->get_record('course_sections', ['course' => $section->course, 'section' => '0']); + foreach ($modules as $mod) { + moveto_module($mod, $sectionzero); + } + + // Create and queue an adhoc task for the deletion of the modules. + $removaltask = new \core_course\task\course_delete_modules(); + $data = array( + 'cms' => $affectedmods + ); + $removaltask->set_custom_data($data); + \core\task\manager::queue_adhoc_task($removaltask); + + // Delete the now empty section, passing in only the section number, which forces the function to fetch a new object. + // The refresh is needed because the section->sequence is now stale. + $result = $format->delete_section($section->section, $forcedeleteifnotempty); + + // Trigger an event for course section deletion. + if ($result) { + $context = \context_course::instance($section->course); + $event = \core\event\course_section_deleted::create( + array( + 'objectid' => $section->id, + 'courseid' => $section->course, + 'context' => $context, + 'other' => array( + 'sectionnum' => $section->section, + 'sectionname' => $sectionname, + ) + ) + ); + $event->add_record_snapshot('course_sections', $section); + $event->trigger(); + } + rebuild_course_cache($section->course, true); + + return $result; +} + /** * Updates the course section * diff --git a/course/rest.php b/course/rest.php index 31064e105d6..a63f6cef128 100644 --- a/course/rest.php +++ b/course/rest.php @@ -168,7 +168,7 @@ switch($requestmethod) { switch ($class) { case 'resource': require_capability('moodle/course:manageactivities', $modcontext); - course_delete_module($cm->id); + course_delete_module($cm->id, true); break; } break; diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index f8bea359554..fd39dff0544 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -3379,4 +3379,288 @@ class core_course_courselib_testcase extends advanced_testcase { $this->assertFalse($updates->introfiles->updated); $this->assertFalse($updates->outcomes->updated); } + + public function test_async_module_deletion_hook_implemented() { + // Async module deletion depends on the 'true' being returned by at least one plugin implementing the hook, + // 'course_module_adhoc_deletion_recommended'. In core, is implemented by the course recyclebin, which will only return + // true if the recyclebin plugin is enabled. To make sure async deletion occurs, this test force-enables the recyclebin. + global $DB, $USER; + $this->resetAfterTest(true); + $this->setAdminUser(); + + // Ensure recyclebin is enabled. + set_config('coursebinenable', true, 'tool_recyclebin'); + + // Create course, module and context. + $course = $this->getDataGenerator()->create_course(['numsections' => 5]); + $module = $this->getDataGenerator()->create_module('assign', ['course' => $course->id]); + $modcontext = context_module::instance($module->cmid); + + // Verify context exists. + $this->assertInstanceOf('context_module', $modcontext); + + // Check events generated on the course_delete_module call. + $sink = $this->redirectEvents(); + + // Try to delete the module using the async flag. + course_delete_module($module->cmid, true); // Try to delete the module asynchronously. + + // Verify that no event has been generated yet. + $events = $sink->get_events(); + $event = array_pop($events); + $sink->close(); + $this->assertEmpty($event); + + // Grab the record, in it's final state before hard deletion, for comparison with the event snapshot. + // We need to do this because the 'deletioninprogress' flag has changed from '0' to '1'. + $cm = $DB->get_record('course_modules', ['id' => $module->cmid], '*', MUST_EXIST); + + // Verify the course_module is marked as 'deletioninprogress'. + $this->assertNotEquals($cm, false); + $this->assertEquals($cm->deletioninprogress, '1'); + + // Verify the context has not yet been removed. + $this->assertEquals($modcontext, context_module::instance($module->cmid, IGNORE_MISSING)); + + // Set up a sink to catch the 'course_module_deleted' event. + $sink = $this->redirectEvents(); + + // Now, run the adhoc task which performs the hard deletion. + phpunit_util::run_all_adhoc_tasks(); + + // Fetch and validate the event data. + $events = $sink->get_events(); + $event = array_pop($events); + $sink->close(); + $this->assertInstanceOf('\core\event\course_module_deleted', $event); + $this->assertEquals($module->cmid, $event->objectid); + $this->assertEquals($USER->id, $event->userid); + $this->assertEquals('course_modules', $event->objecttable); + $this->assertEquals(null, $event->get_url()); + $this->assertEquals($cm, $event->get_record_snapshot('course_modules', $module->cmid)); + + // Verify the context has been removed. + $this->assertFalse(context_module::instance($module->cmid, IGNORE_MISSING)); + + // Verify the course_module record has been deleted. + $cmcount = $DB->count_records('course_modules', ['id' => $module->cmid]); + $this->assertEmpty($cmcount); + } + + public function test_async_module_deletion_hook_not_implemented() { + // Only proceed if we are sure that no plugin is going to advocate async removal of a module. I.e. no plugin returns + // 'true' from the 'course_module_adhoc_deletion_recommended' hook. + // In the case of core, only recyclebin implements this hook, and it will only return true if enabled, so disable it. + global $DB, $USER; + $this->resetAfterTest(true); + $this->setAdminUser(); + set_config('coursebinenable', false, 'tool_recyclebin'); + + // Non-core plugins might implement the 'course_module_adhoc_deletion_recommended' hook and spoil this test. + // If at least one plugin still returns true, then skip this test. + if ($pluginsfunction = get_plugins_with_function('course_module_background_deletion_recommended')) { + foreach ($pluginsfunction as $plugintype => $plugins) { + foreach ($plugins as $pluginfunction) { + if ($pluginfunction()) { + $this->markTestSkipped(); + } + } + } + } + + // Create course, module and context. + $course = $this->getDataGenerator()->create_course(['numsections' => 5]); + $module = $this->getDataGenerator()->create_module('assign', ['course' => $course->id]); + $modcontext = context_module::instance($module->cmid); + $cm = $DB->get_record('course_modules', ['id' => $module->cmid], '*', MUST_EXIST); + + // Verify context exists. + $this->assertInstanceOf('context_module', $modcontext); + + // Check events generated on the course_delete_module call. + $sink = $this->redirectEvents(); + + // Try to delete the module using the async flag. + course_delete_module($module->cmid, true); // Try to delete the module asynchronously. + + // Fetch and validate the event data. + $events = $sink->get_events(); + $event = array_pop($events); + $sink->close(); + $this->assertInstanceOf('\core\event\course_module_deleted', $event); + $this->assertEquals($module->cmid, $event->objectid); + $this->assertEquals($USER->id, $event->userid); + $this->assertEquals('course_modules', $event->objecttable); + $this->assertEquals(null, $event->get_url()); + $this->assertEquals($cm, $event->get_record_snapshot('course_modules', $module->cmid)); + + // Verify the context has been removed. + $this->assertFalse(context_module::instance($module->cmid, IGNORE_MISSING)); + + // Verify the course_module record has been deleted. + $cmcount = $DB->count_records('course_modules', ['id' => $module->cmid]); + $this->assertEmpty($cmcount); + } + + public function test_async_section_deletion_hook_implemented() { + // Async section deletion (provided section contains modules), depends on the 'true' being returned by at least one plugin + // implementing the 'course_module_adhoc_deletion_recommended' hook. In core, is implemented by the course recyclebin, + // which will only return true if the plugin is enabled. To make sure async deletion occurs, this test enables recyclebin. + global $DB, $USER; + $this->resetAfterTest(true); + $this->setAdminUser(); + + // Ensure recyclebin is enabled. + set_config('coursebinenable', true, 'tool_recyclebin'); + + // Create course, module and context. + $generator = $this->getDataGenerator(); + $course = $generator->create_course(['numsections' => 4, 'format' => 'topics'], ['createsections' => true]); + $assign0 = $generator->create_module('assign', ['course' => $course, 'section' => 2]); + $assign1 = $generator->create_module('assign', ['course' => $course, 'section' => 2]); + $assign2 = $generator->create_module('assign', ['course' => $course, 'section' => 2]); + $assign3 = $generator->create_module('assign', ['course' => $course, 'section' => 0]); + + // Delete empty section. No difference from normal, synchronous behaviour. + $this->assertTrue(course_delete_section($course, 4, false, true)); + $this->assertEquals(3, course_get_format($course)->get_course()->numsections); + + // Delete a module in section 2 (using async). Need to verify this doesn't generate two tasks when we delete + // the section in the next step. + course_delete_module($assign2->cmid, true); + + // Confirm that the module is pending deletion in its current section. + $section = $DB->get_record('course_sections', ['course' => $course->id, 'section' => '2']); // For event comparison. + $this->assertEquals(true, $DB->record_exists('course_modules', ['id' => $assign2->cmid, 'deletioninprogress' => 1, + 'section' => $section->id])); + + // Now, delete section 2. + $this->assertFalse(course_delete_section($course, 2, false, true)); // Non-empty section, no forcedelete, so no change. + + $sink = $this->redirectEvents(); // To capture the event. + $this->assertTrue(course_delete_section($course, 2, true, true)); + + // Now, confirm that: + // a) the section's modules have been flagged for deletion and moved to section 0 and; + // b) the section has been deleted and; + // c) course_section_deleted event has been fired. The course_module_deleted events will only fire once they have been + // removed from section 0 via the adhoc task. + + // Modules should have been flagged for deletion and moved to section 0. + $sectionid = $DB->get_field('course_sections', 'id', ['course' => $course->id, 'section' => 0]); + $this->assertEquals(3, $DB->count_records('course_modules', ['section' => $sectionid, 'deletioninprogress' => 1])); + + // Confirm the section has been deleted. + $this->assertEquals(2, course_get_format($course)->get_course()->numsections); + + // Check event fired. + $events = $sink->get_events(); + $event = array_pop($events); + $sink->close(); + $this->assertInstanceOf('\core\event\course_section_deleted', $event); + $this->assertEquals($section->id, $event->objectid); + $this->assertEquals($USER->id, $event->userid); + $this->assertEquals('course_sections', $event->objecttable); + $this->assertEquals(null, $event->get_url()); + $this->assertEquals($section, $event->get_record_snapshot('course_sections', $section->id)); + + // Now, run the adhoc task to delete the modules from section 0. + $sink = $this->redirectEvents(); // To capture the events. + phpunit_util::run_all_adhoc_tasks(); + + // Confirm the modules have been deleted. + list($insql, $assignids) = $DB->get_in_or_equal([$assign0->cmid, $assign1->cmid, $assign2->cmid]); + $cmcount = $DB->count_records_select('course_modules', 'id ' . $insql, $assignids); + $this->assertEmpty($cmcount); + + // Confirm other modules in section 0 still remain. + $this->assertEquals(1, $DB->count_records('course_modules', ['id' => $assign3->cmid])); + + // Confirm that events were generated for all 3 of the modules. + $events = $sink->get_events(); + $sink->close(); + $count = 0; + while (!empty($events)) { + $event = array_pop($events); + if (in_array($event->objectid, [$assign0->cmid, $assign1->cmid, $assign2->cmid])) { + $count++; + } + } + $this->assertEquals(3, $count); + } + + public function test_async_section_deletion_hook_not_implemented() { + // If no plugins advocate async removal, then normal synchronous removal will take place. + // Only proceed if we are sure that no plugin is going to advocate async removal of a module. I.e. no plugin returns + // 'true' from the 'course_module_adhoc_deletion_recommended' hook. + // In the case of core, only recyclebin implements this hook, and it will only return true if enabled, so disable it. + global $DB, $USER; + $this->resetAfterTest(true); + $this->setAdminUser(); + set_config('coursebinenable', false, 'tool_recyclebin'); + + // Non-core plugins might implement the 'course_module_adhoc_deletion_recommended' hook and spoil this test. + // If at least one plugin still returns true, then skip this test. + if ($pluginsfunction = get_plugins_with_function('course_module_background_deletion_recommended')) { + foreach ($pluginsfunction as $plugintype => $plugins) { + foreach ($plugins as $pluginfunction) { + if ($pluginfunction()) { + $this->markTestSkipped(); + } + } + } + } + + // Create course, module and context. + $generator = $this->getDataGenerator(); + $course = $generator->create_course(['numsections' => 4, 'format' => 'topics'], ['createsections' => true]); + $assign0 = $generator->create_module('assign', ['course' => $course, 'section' => 2]); + $assign1 = $generator->create_module('assign', ['course' => $course, 'section' => 2]); + + // Delete empty section. No difference from normal, synchronous behaviour. + $this->assertTrue(course_delete_section($course, 4, false, true)); + $this->assertEquals(3, course_get_format($course)->get_course()->numsections); + + // Delete section in the middle (2). + $section = $DB->get_record('course_sections', ['course' => $course->id, 'section' => '2']); // For event comparison. + $this->assertFalse(course_delete_section($course, 2, false, true)); // Non-empty section, no forcedelete, so no change. + + $sink = $this->redirectEvents(); // To capture the event. + $this->assertTrue(course_delete_section($course, 2, true, true)); + + // Now, confirm that: + // a) The section's modules have deleted and; + // b) the section has been deleted and; + // c) course_section_deleted event has been fired and; + // d) course_module_deleted events have both been fired. + + // Confirm modules have been deleted. + list($insql, $assignids) = $DB->get_in_or_equal([$assign0->cmid, $assign1->cmid]); + $cmcount = $DB->count_records_select('course_modules', 'id ' . $insql, $assignids); + $this->assertEmpty($cmcount); + + // Confirm the section has been deleted. + $this->assertEquals(2, course_get_format($course)->get_course()->numsections); + + // Confirm the course_section_deleted event has been generated. + $events = $sink->get_events(); + $event = array_pop($events); + $sink->close(); + $this->assertInstanceOf('\core\event\course_section_deleted', $event); + $this->assertEquals($section->id, $event->objectid); + $this->assertEquals($USER->id, $event->userid); + $this->assertEquals('course_sections', $event->objecttable); + $this->assertEquals(null, $event->get_url()); + $this->assertEquals($section, $event->get_record_snapshot('course_sections', $section->id)); + + // Confirm that the course_module_deleted events have both been generated. + $count = 0; + while (!empty($events)) { + $event = array_pop($events); + if (in_array($event->objectid, [$assign0->cmid, $assign1->cmid])) { + $count++; + } + } + $this->assertEquals(2, $count); + } } diff --git a/grade/lib.php b/grade/lib.php index 9dacc0b7123..7be114e25e2 100644 --- a/grade/lib.php +++ b/grade/lib.php @@ -970,6 +970,13 @@ function print_grade_page_head($courseid, $active_type, $active_plugin=null, $user = null) { global $CFG, $OUTPUT, $PAGE; + // 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)) { + \core\notification::add(get_string('gradesmoduledeletionpendingwarning', 'grades'), + \core\output\notification::NOTIFY_WARNING); + } + if ($active_type === 'preferences') { // In Moodle 2.8 report preferences were moved under 'settings'. Allow backward compatibility for 3rd party grade reports. $active_type = 'settings'; diff --git a/lang/en/grades.php b/lang/en/grades.php index 41f91c3649f..0dfbe07defa 100644 --- a/lang/en/grades.php +++ b/lang/en/grades.php @@ -326,6 +326,8 @@ $string['grades'] = 'Grades'; $string['gradesforuser'] = 'Grades for {$a->user}'; $string['singleview'] = 'Single view for {$a}'; $string['gradesonly'] = 'Change to grades only'; +$string['gradesmoduledeletionpendingwarning'] = 'Warning: Activity deletion in progress! Some grades are about to be removed.'; +$string['gradesmoduledeletionprefix'] = '[Deletion in progress]'; $string['gradessettings'] = 'Grade settings'; $string['gradetype'] = 'Grade type'; $string['gradetype_help'] = 'There are 4 grade types: diff --git a/lib/completionlib.php b/lib/completionlib.php index cba3ed9707a..80c854242db 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -1063,7 +1063,7 @@ class completion_info { $modinfo = get_fast_modinfo($this->course); $result = array(); foreach ($modinfo->get_cms() as $cm) { - if ($cm->completion != COMPLETION_TRACKING_NONE) { + if ($cm->completion != COMPLETION_TRACKING_NONE && !$cm->deletioninprogress) { $result[$cm->id] = $cm; } } diff --git a/lib/db/install.xml b/lib/db/install.xml index af9e2dcd8b1..39bbd58abc4 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -300,6 +300,7 @@ + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 821c37e65bf..9d68867c165 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2392,5 +2392,19 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2016110500.00); } + if ($oldversion < 2016110600.00) { + // Define a field 'deletioninprogress' in the 'course_modules' table, to background deletion tasks. + $table = new xmldb_table('course_modules'); + $field = new xmldb_field('deletioninprogress', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '0', 'availability'); + + // Conditionally launch add field 'deletioninprogress'. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2016110600.00); + } + return true; } diff --git a/lib/filestorage/file_storage.php b/lib/filestorage/file_storage.php index c3abedda152..c709c5f0dc2 100644 --- a/lib/filestorage/file_storage.php +++ b/lib/filestorage/file_storage.php @@ -2215,7 +2215,12 @@ class file_storage { mkdir($trashpath, $this->dirpermissions, true); } rename($contentfile, $trashfile); - chmod($trashfile, $this->filepermissions); // fix permissions if needed + + // Fix permissions, only if needed. + $currentperms = octdec(substr(decoct(fileperms($trashfile)), -4)); + if ((int)$this->filepermissions !== $currentperms) { + chmod($trashfile, $this->filepermissions); + } } /** diff --git a/lib/grade/grade_item.php b/lib/grade/grade_item.php index 79db6985143..881e7a6eeb7 100644 --- a/lib/grade/grade_item.php +++ b/lib/grade/grade_item.php @@ -524,6 +524,14 @@ class grade_item extends grade_object { * @return bool Locked state */ public function is_locked($userid=NULL) { + global $CFG; + + // Override for any grade items belonging to activities which are in the process of being deleted. + require_once($CFG->dirroot . '/course/lib.php'); + if (course_module_instance_pending_deletion($this->courseid, $this->itemmodule, $this->iteminstance)) { + return true; + } + if (!empty($this->locked)) { return true; } @@ -1393,7 +1401,12 @@ class grade_item extends grade_object { public function get_name($fulltotal=false) { if (strval($this->itemname) !== '') { // MDL-10557 - return format_string($this->itemname); + + // Make it obvious to users if the course module to which this grade item relates, is currently being removed. + $deletionpending = course_module_instance_pending_deletion($this->courseid, $this->itemmodule, $this->iteminstance); + $deletionnotice = get_string('gradesmoduledeletionprefix', 'grades'); + + return $deletionpending ? format_string($deletionnotice . ' ' . $this->itemname) : format_string($this->itemname); } else if ($this->is_course_item()) { return get_string('coursetotal', 'grades'); diff --git a/lib/modinfolib.php b/lib/modinfolib.php index d8d3c1ec484..a3664589392 100644 --- a/lib/modinfolib.php +++ b/lib/modinfolib.php @@ -554,8 +554,7 @@ class course_modinfo { // Get section data $sections = $DB->get_records('course_sections', array('course' => $course->id), 'section', - 'section, id, course, name, summary, summaryformat, sequence, visible, ' . - 'availability'); + 'section, id, course, name, summary, summaryformat, sequence, visible, availability'); $compressedsections = array(); $formatoptionsdef = course_get_format($course)->section_format_options(); @@ -753,6 +752,7 @@ class course_modinfo { * @property-read mixed $customdata Optional custom data stored in modinfo cache for this activity, or null if none * @property-read string $afterlink Extra HTML code to display after link - calculated on request * @property-read string $afterediticons Extra HTML code to display after editing icons (e.g. more icons) - calculated on request + * @property-read bool $deletioninprogress True if this course module is scheduled for deletion, false otherwise. */ class cm_info implements IteratorAggregate { /** @@ -1038,6 +1038,11 @@ class cm_info implements IteratorAggregate { */ private $afterediticons; + /** + * @var bool representing the deletion state of the module. True if the mod is scheduled for deletion. + */ + private $deletioninprogress; + /** * List of class read-only properties and their getter methods. * Used by magic functions __get(), __isset(), __empty() @@ -1089,6 +1094,7 @@ class cm_info implements IteratorAggregate { 'uservisible' => 'get_user_visible', 'visible' => false, 'visibleold' => false, + 'deletioninprogress' => false ); /** @@ -1505,7 +1511,7 @@ class cm_info implements IteratorAggregate { static $cmfields = array('id', 'course', 'module', 'instance', 'section', 'idnumber', 'added', 'score', 'indent', 'visible', 'visibleold', 'groupmode', 'groupingid', 'completion', 'completiongradeitemnumber', 'completionview', 'completionexpected', - 'showdescription', 'availability'); + 'showdescription', 'availability', 'deletioninprogress'); foreach ($cmfields as $key) { $cmrecord->$key = $this->$key; } @@ -1700,6 +1706,7 @@ class cm_info implements IteratorAggregate { $this->added = isset($mod->added) ? $mod->added : 0; $this->score = isset($mod->score) ? $mod->score : 0; $this->visibleold = isset($mod->visibleold) ? $mod->visibleold : 0; + $this->deletioninprogress = isset($mod->deletioninprogress) ? $mod->deletioninprogress : 0; // Note: it saves effort and database space to always include the // availability and completion fields, even if availability or completion @@ -1861,6 +1868,12 @@ class cm_info implements IteratorAggregate { } $this->uservisible = true; + // If the module is being deleted, set the uservisible state to false and return. + if ($this->deletioninprogress) { + $this->uservisible = false; + return null; + } + // If the user cannot access the activity set the uservisible flag to false. // Additional checks are required to determine whether the activity is entirely hidden or just greyed out. if ((!$this->visible or !$this->get_available()) and @@ -2465,7 +2478,7 @@ class section_info implements IteratorAggregate { 'summary' => '', 'summaryformat' => '1', // FORMAT_HTML, but must be a string 'visible' => '1', - 'availability' => null, + 'availability' => null ); /** diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php index 1cc57e6f5f3..a3d2b34cef9 100644 --- a/lib/phpunit/classes/util.php +++ b/lib/phpunit/classes/util.php @@ -822,4 +822,21 @@ class phpunit_util extends testing_util { return 'en_AU.UTF-8'; } } + + /** + * Executes all adhoc tasks in the queue. Useful for testing asynchronous behaviour. + * + * @return void + */ + public static function run_all_adhoc_tasks() { + $now = time(); + while (($task = \core\task\manager::get_next_adhoc_task($now)) !== null) { + try { + $task->execute(); + \core\task\manager::adhoc_task_complete($task); + } catch (Exception $e) { + \core\task\manager::adhoc_task_failed($task); + } + } + } } diff --git a/lib/tests/behat/behat_general.php b/lib/tests/behat/behat_general.php index edc4cca71a1..a6be6f4a3b8 100644 --- a/lib/tests/behat/behat_general.php +++ b/lib/tests/behat/behat_general.php @@ -1001,6 +1001,43 @@ class behat_general extends behat_base { } } + /** + * Runs all ad-hoc tasks in the queue. + * + * This is faster and more reliable than running cron (running cron won't + * work more than once in the same test, for instance). However it is + * a little less 'realistic'. + * + * While the task is running, we suppress mtrace output because it makes + * the Behat result look ugly. + * + * @Given /^I run all adhoc tasks$/ + * @throws DriverException + */ + public function i_run_all_adhoc_tasks() { + // Do setup for cron task. + cron_setup_user(); + + // Run tasks. Locking is handled by get_next_adhoc_task. + $now = time(); + ob_start(); // Discard task output as not appropriate for Behat output! + while (($task = \core\task\manager::get_next_adhoc_task($now)) !== null) { + + try { + $task->execute(); + + // Mark task complete. + \core\task\manager::adhoc_task_complete($task); + } catch (Exception $e) { + // Mark task failed and throw exception. + \core\task\manager::adhoc_task_failed($task); + ob_end_clean(); + throw new DriverException('An adhoc task failed', 0, $e); + } + } + ob_end_clean(); + } + /** * Checks that an element and selector type exists in another element and selector type on the current page. * diff --git a/version.php b/version.php index 75929538db6..16626a61057 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2016110500.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2016110600.00; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes.