From ddf368a213e8fc60172113b77e6ddfa9ea579616 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 26 Feb 2014 14:00:51 +0000 Subject: [PATCH] MDL-44141 Completion: System updates data during restore The completion system has code which is supposed to prevent it updating completion status while grades are being restored. This code was a nasty hack and had not worked for some time. As a result completion dates were restored incorrectly. This commit implements a 'proper' way to find out if a restore is currently running, replacing the previous hack. --- .../controller/restore_controller.class.php | 24 ++++++- backup/controller/tests/controller_test.php | 65 +++++++++++++++++++ lib/grade/grade_grade.php | 17 +++-- 3 files changed, 96 insertions(+), 10 deletions(-) diff --git a/backup/controller/restore_controller.class.php b/backup/controller/restore_controller.class.php index 822992ce4f7..39926492b45 100644 --- a/backup/controller/restore_controller.class.php +++ b/backup/controller/restore_controller.class.php @@ -57,6 +57,9 @@ class restore_controller extends base_controller { protected $checksum; // Cache @checksumable results for lighter @is_checksum_correct() uses + /** @var int Number of restore_controllers that are currently executing */ + protected static $executing = 0; + /** * Constructor. * @@ -325,7 +328,26 @@ class restore_controller extends base_controller { $this->log('notifying plan about excluded activities by type', backup::LOG_DEBUG); $this->plan->set_excluding_activities(); } - return $this->plan->execute(); + self::$executing++; + try { + $this->plan->execute(); + } catch (Exception $e) { + self::$executing--; + throw $e; + } + self::$executing--; + } + + /** + * Checks whether restore is currently executing. Certain parts of code that + * is called during restore, but not directly part of the restore system, may + * need to behave differently during restore (e.g. do not bother resetting a + * cache because we know it will be reset at end of operation). + * + * @return bool True if any restore is currently executing + */ + public static function is_executing() { + return self::$executing > 0; } /** diff --git a/backup/controller/tests/controller_test.php b/backup/controller/tests/controller_test.php index a4851459882..d5142f15376 100644 --- a/backup/controller/tests/controller_test.php +++ b/backup/controller/tests/controller_test.php @@ -26,6 +26,7 @@ defined('MOODLE_INTERNAL') || die(); // Include all the needed stuff global $CFG; require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); +require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); /* * controller tests (all) @@ -101,6 +102,70 @@ class core_backup_controller_testcase extends advanced_testcase { backup::INTERACTIVE_NO, backup::MODE_IMPORT, $this->userid); $this->assertEquals($bc->get_include_files(), 0); } + + /** + * Tests the restore_controller. + */ + public function test_restore_controller_is_executing() { + global $CFG; + + // Make a backup. + check_dir_exists($CFG->tempdir . '/backup'); + $bc = new backup_controller(backup::TYPE_1ACTIVITY, $this->moduleid, backup::FORMAT_MOODLE, + backup::INTERACTIVE_NO, backup::MODE_IMPORT, $this->userid); + $backupid = $bc->get_backupid(); + $bc->execute_plan(); + $bc->destroy(); + + // The progress class will get called during restore, so we can use that + // to check the executing flag is true. + $progress = new core_backup_progress_restore_is_executing(); + + // Set up restore. + $rc = new restore_controller($backupid, $this->courseid, + backup::INTERACTIVE_NO, backup::MODE_SAMESITE, $this->userid, + backup::TARGET_EXISTING_ADDING); + $this->assertTrue($rc->execute_precheck()); + + // Check restore is NOT executing. + $this->assertFalse(restore_controller::is_executing()); + + // Execute restore. + $rc->set_progress($progress); + $rc->execute_plan(); + + // Check restore is NOT executing afterward either. + $this->assertFalse(restore_controller::is_executing()); + $rc->destroy(); + + // During restore, check that executing was true. + $this->assertTrue(count($progress->executing) > 0); + $alltrue = true; + foreach ($progress->executing as $executing) { + if (!$executing) { + $alltrue = false; + break; + } + } + $this->assertTrue($alltrue); + } +} + + +/** + * Progress class that records the result of restore_controller::is_executing calls. + * + * @package core_backup + * @copyright 2014 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class core_backup_progress_restore_is_executing extends \core\progress\base { + /** @var array Array of results from calling function */ + public $executing = array(); + + public function update_progress() { + $this->executing[] = restore_controller::is_executing(); + } } diff --git a/lib/grade/grade_grade.php b/lib/grade/grade_grade.php index fc89d1f71d5..9813c5620fa 100644 --- a/lib/grade/grade_grade.php +++ b/lib/grade/grade_grade.php @@ -774,14 +774,6 @@ class grade_grade extends grade_object { function notify_changed($deleted) { global $CFG; - // Ignore during restore - // TODO There should be a proper way to determine when we are in restore - // so that this hack looking for a $restore global is not needed. - global $restore; - if (!empty($restore->backup_unique_code)) { - return; - } - // Inform conditionlib since it may cache the grades for conditional availability of modules or sections. if (!empty($CFG->enableavailability)) { require_once($CFG->libdir.'/conditionlib.php'); @@ -791,11 +783,18 @@ class grade_grade extends grade_object { require_once($CFG->libdir.'/completionlib.php'); // Bail out immediately if completion is not enabled for site (saves loading - // grade item below) + // grade item & requiring the restore stuff). if (!completion_info::is_enabled_for_site()) { return; } + // Ignore during restore, as completion data will be updated anyway and + // doing it now will result in incorrect dates (it will say they got the + // grade completion now, instead of the correct time). + if (class_exists('restore_controller', false) && restore_controller::is_executing()) { + return; + } + // Load information about grade item $this->load_grade_item();