From a6a4034b388b0e75900ba9858111f0793f0402e0 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 19 Sep 2019 18:59:15 +0100 Subject: [PATCH 1/3] MDL-66709 backup: improve some PHPdoc type hints I hope these are right. They seem to be, and made the autocomplete better in some backup/restore code I was writing. --- backup/controller/backup_controller.class.php | 1 + backup/moodle2/backup_plugin.class.php | 14 ++++++++++++++ backup/moodle2/restore_plugin.class.php | 12 ++++++++++++ backup/moodle2/restore_stepslib.php | 1 + 4 files changed, 28 insertions(+) diff --git a/backup/controller/backup_controller.class.php b/backup/controller/backup_controller.class.php index cf5fcf96622..52793b24d68 100644 --- a/backup/controller/backup_controller.class.php +++ b/backup/controller/backup_controller.class.php @@ -55,6 +55,7 @@ class backup_controller extends base_controller { protected $status; // Current status of the controller (created, planned, configured...) + /** @var backup_plan */ protected $plan; // Backup execution plan protected $includefiles; // Whether this backup includes files or not. diff --git a/backup/moodle2/backup_plugin.class.php b/backup/moodle2/backup_plugin.class.php index 6a060abf8fb..32f6c1ddf23 100644 --- a/backup/moodle2/backup_plugin.class.php +++ b/backup/moodle2/backup_plugin.class.php @@ -34,13 +34,27 @@ defined('MOODLE_INTERNAL') || die(); */ abstract class backup_plugin { + /** @var string */ protected $plugintype; + /** @var string */ protected $pluginname; + /** @var string */ protected $connectionpoint; + /** @var backup_optigroup_element */ protected $optigroup; // Optigroup, parent of all optigroup elements + /** @var backup_structure_step */ protected $step; + /** @var backup_course_task|backup_activity_task */ protected $task; + /** + * backup_plugin constructor. + * + * @param string $plugintype + * @param string $pluginname + * @param backup_optigroup_element $optigroup + * @param backup_structure_step $step + */ public function __construct($plugintype, $pluginname, $optigroup, $step) { $this->plugintype = $plugintype; $this->pluginname = $pluginname; diff --git a/backup/moodle2/restore_plugin.class.php b/backup/moodle2/restore_plugin.class.php index b90bcdf6fdd..4b5631d9d52 100644 --- a/backup/moodle2/restore_plugin.class.php +++ b/backup/moodle2/restore_plugin.class.php @@ -34,12 +34,24 @@ defined('MOODLE_INTERNAL') || die(); */ abstract class restore_plugin { + /** @var string */ protected $plugintype; + /** @var string */ protected $pluginname; + /** @var string */ protected $connectionpoint; + /** @var restore_structure_step */ protected $step; + /** @var restore_course_task|restore_activity_task */ protected $task; + /** + * restore_plugin constructor. + * + * @param string $plugintype + * @param string $pluginname + * @param restore_structure_step $step + */ public function __construct($plugintype, $pluginname, $step) { $this->plugintype = $plugintype; $this->pluginname = $pluginname; diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 0d6518d3bfb..437c9cb3ca9 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -5734,6 +5734,7 @@ abstract class restore_questions_activity_structure_step extends restore_activit } } + /** * Restore completion defaults for each module type * From 118d62a26601a48d061f64d831e676cbb6094d4f Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 19 Sep 2019 19:01:24 +0100 Subject: [PATCH 2/3] MDL-66709 quiz: add a Behat test for backup/restore of quiz attempts --- mod/quiz/tests/behat/backup.feature | 31 +++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/mod/quiz/tests/behat/backup.feature b/mod/quiz/tests/behat/backup.feature index 99fb72fc49a..eaa8b44c744 100644 --- a/mod/quiz/tests/behat/backup.feature +++ b/mod/quiz/tests/behat/backup.feature @@ -34,6 +34,37 @@ Feature: Backup and restore of quizzes Then I should see "TF1" And I should see "TF2" + @javascript + Scenario: Backup and restore a course containing a quiz with user data. + Given the following "activities" exist: + | activity | name | intro | course | idnumber | + | quiz | Quiz 1 | For testing backup | C1 | quiz1 | + And the following "questions" exist: + | questioncategory | qtype | name | questiontext | + | Test questions | truefalse | TF1 | First question | + | Test questions | truefalse | TF2 | Second question | + And quiz "Quiz 1" contains the following questions: + | question | page | + | TF1 | 1 | + | TF2 | 2 | + And the following "users" exist: + | username | + | student | + And the following "course enrolments" exist: + | user | course | role | + | student | C1 | student | + And user "student" has attempted "Quiz 1" with responses: + | slot | response | + | 1 | True | + | 2 | False | + When I backup "Course 1" course using this options: + | Confirmation | Filename | test_backup.mbz | + And I restore "test_backup.mbz" backup into a new course using this options: + | Schema | Course name | Restored course | + Then I should see "Restored course" + And I follow "Quiz 1" + And I should see "Attempts: 1" + @javascript @_file_upload Scenario: Restore a Moodle 2.8 quiz backup When I am on "Course 1" course homepage From e05d99b94faeccbc48609385e5770df27c964812 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 19 Sep 2019 19:02:14 +0100 Subject: [PATCH 3/3] MDL-66709 backup: move question attempt data helpers to a trait This lets plugins other than activity modules backup and restore question attempt data. The old backup_questions_activity_structure_step and restore_questions_activity_structure_step base classes still exist and work exactly the same way they did before (because they use the trait) so this change is completely backwards compatible. To make this work fully, a few other things in the code had to be tweaked: * Adding restore path elements had to consider the possibility of grouped parents in more places. * I needed to add protected get_task() to the restore_plugin class. I don't think that is a problem. * In the restore trait, the process_question_... methods needed to be changed to public for some reasons to do with PHP traits that I don't fully understand. However, I don't think this change is a problem. * The way question_usage restore got the new contextid had to be changed (or it did not work in activity contexts), but the new code looks like a better way to do it anyway so that is good. --- backup/moodle2/backup_stepslib.php | 16 ++++++++-- backup/moodle2/restore_plugin.class.php | 17 +++++++++-- backup/moodle2/restore_stepslib.php | 29 ++++++++++++------- .../plan/restore_structure_step.class.php | 16 ++++++---- 4 files changed, 56 insertions(+), 22 deletions(-) diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php index 11dd4ce9c78..49daa6fb41f 100644 --- a/backup/moodle2/backup_stepslib.php +++ b/backup/moodle2/backup_stepslib.php @@ -125,10 +125,9 @@ abstract class backup_activity_structure_step extends backup_structure_step { } /** - * Abstract structure step, to be used by all the activities using core questions stuff - * (namely quiz module), supporting question plugins, states and sessions + * Helper code for use by any plugin that stores question attempt data that it needs to back up. */ -abstract class backup_questions_activity_structure_step extends backup_activity_structure_step { +trait backup_questions_attempt_data_trait { /** * Attach to $element (usually attempts) the needed backup structures @@ -200,6 +199,17 @@ abstract class backup_questions_activity_structure_step extends backup_activity_ } +/** + * Abstract structure step to help activities that store question attempt data. + * + * @copyright 2011 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +abstract class backup_questions_activity_structure_step extends backup_activity_structure_step { + use backup_questions_attempt_data_trait; +} + + /** * backup structure step in charge of calculating the categories to be * included in backup, based in the context being backuped (module/course) diff --git a/backup/moodle2/restore_plugin.class.php b/backup/moodle2/restore_plugin.class.php index 4b5631d9d52..31a94f50e9f 100644 --- a/backup/moodle2/restore_plugin.class.php +++ b/backup/moodle2/restore_plugin.class.php @@ -70,9 +70,11 @@ abstract class restore_plugin { $methodname = 'define_' . basename($this->connectionpoint->get_path()) . '_plugin_structure'; if (method_exists($this, $methodname)) { - if ($bluginpaths = $this->$methodname()) { - foreach ($bluginpaths as $path) { - $path->set_processing_object($this); + if ($pluginpaths = $this->$methodname()) { + foreach ($pluginpaths as $path) { + if ($path->get_processing_object() === null && !$this->step->grouped_parent_exists($path, $paths)) { + $path->set_processing_object($this); + } $paths[] = $path; } } @@ -270,4 +272,13 @@ abstract class restore_plugin { 'plugin_' . $this->plugintype . '_' . $this->pluginname . '_' . basename($this->connectionpoint->get_path()) . $path; } + + /** + * Get the task we are part of. + * + * @return restore_activity_task|restore_course_task the task. + */ + protected function get_task() { + return $this->task; + } } diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 437c9cb3ca9..51fbbb1a325 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -5323,10 +5323,9 @@ class restore_process_file_aliases_queue extends restore_execution_step { /** - * Abstract structure step, to be used by all the activities using core questions stuff - * (like the quiz module), to support qtype plugins, states and sessions + * Helper code for use by any plugin that stores question attempt data that it needs to back up. */ -abstract class restore_questions_activity_structure_step extends restore_activity_structure_step { +trait restore_questions_attempt_data_trait { /** @var array question_attempt->id to qtype. */ protected $qtypes = array(); /** @var array question_attempt->id to questionid. */ @@ -5378,21 +5377,21 @@ abstract class restore_questions_activity_structure_step extends restore_activit /** * Process question_usages */ - protected function process_question_usage($data) { + public function process_question_usage($data) { $this->restore_question_usage_worker($data, ''); } /** * Process question_attempts */ - protected function process_question_attempt($data) { + public function process_question_attempt($data) { $this->restore_question_attempt_worker($data, ''); } /** * Process question_attempt_steps */ - protected function process_question_attempt_step($data) { + public function process_question_attempt_step($data) { $this->restore_question_attempt_step_worker($data, ''); } @@ -5412,8 +5411,7 @@ abstract class restore_questions_activity_structure_step extends restore_activit $data = (object)$data; $oldid = $data->id; - $oldcontextid = $this->get_task()->get_old_contextid(); - $data->contextid = $this->get_mappingid('context', $this->task->get_old_contextid()); + $data->contextid = $this->task->get_contextid(); // Everything ready, insert (no mapping needed) $newitemid = $DB->insert_record('question_usages', $data); @@ -5569,6 +5567,17 @@ abstract class restore_questions_activity_structure_step extends restore_activit $this->add_related_files('question', $filearea, 'question_attempt_step'); } } +} + + +/** + * Abstract structure step to help activities that store question attempt data. + * + * @copyright 2011 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +abstract class restore_questions_activity_structure_step extends restore_activity_structure_step { + use restore_questions_attempt_data_trait; /** * Attach below $element (usually attempts) the needed restore_path_elements @@ -5612,7 +5621,7 @@ abstract class restore_questions_activity_structure_step extends restore_activit /** * Process the attempt data defined by {@link add_legacy_question_attempt_data()}. * @param object $data contains all the grouped attempt data to process. - * @param pbject $quiz data about the activity the attempts belong to. Required + * @param object $quiz data about the activity the attempts belong to. Required * fields are (basically this only works for the quiz module): * oldquestions => list of question ids in this activity - using old ids. * preferredbehaviour => the behaviour to use for questionattempts. @@ -5674,7 +5683,7 @@ abstract class restore_questions_activity_structure_step extends restore_activit $data->uniqueid = $usage->id; $upgrader->save_usage($quiz->preferredbehaviour, $data, $qas, - $this->questions_recode_layout($quiz->oldquestions)); + $this->questions_recode_layout($quiz->oldquestions)); } protected function find_question_session_and_states($data, $questionid) { diff --git a/backup/util/plan/restore_structure_step.class.php b/backup/util/plan/restore_structure_step.class.php index bffc7b75290..3fe1978deff 100644 --- a/backup/util/plan/restore_structure_step.class.php +++ b/backup/util/plan/restore_structure_step.class.php @@ -496,9 +496,9 @@ abstract class restore_structure_step extends restore_step { // Now, for each element not having one processing object, if // not child of grouped element, assign $this (the step itself) as processing element // Note method must exist or we'll get one @restore_path_element_exception - foreach($paths as $key => $pelement) { + foreach ($paths as $pelement) { if ($pelement->get_processing_object() === null && !$this->grouped_parent_exists($pelement, $paths)) { - $paths[$key]->set_processing_object($this); + $pelement->set_processing_object($this); } // Populate $elementsoldid and $elementsoldid based on available pathelements $this->elementsoldid[$pelement->get_name()] = null; @@ -510,18 +510,22 @@ abstract class restore_structure_step extends restore_step { /** * Given one pathelement, return true if grouped parent was found + * + * @param restore_path_element $pelement the element we are interested in. + * @param restore_path_element[] $elements the elements that exist. + * @return bool true if this element is inside a grouped parent. */ - protected function grouped_parent_exists($pelement, $elements) { + public function grouped_parent_exists($pelement, $elements) { foreach ($elements as $element) { if ($pelement->get_path() == $element->get_path()) { - continue; // Don't compare against itself + continue; // Don't compare against itself. } - // If element is grouped and parent of pelement, return true + // If element is grouped and parent of pelement, return true. if ($element->is_grouped() and strpos($pelement->get_path() . '/', $element->get_path()) === 0) { return true; } } - return false; // no grouped parent found + return false; // No grouped parent found. } /**