From 39ed46b158b87fd9cd55a095fedafeb0e6b75103 Mon Sep 17 00:00:00 2001 From: Aaron Barnes Date: Mon, 10 Nov 2014 16:01:45 +1300 Subject: [PATCH] MDL-40097 completion: Make restoring grade criteria more robust --- backup/moodle2/backup_stepslib.php | 32 ++++++++-- backup/moodle2/restore_stepslib.php | 60 ++++++++++++------- .../criteria/completion_criteria_role.php | 3 + lang/en/completion.php | 1 + 4 files changed, 69 insertions(+), 27 deletions(-) diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php index 294f5b3c6fc..c4e6f1ce9d1 100644 --- a/backup/moodle2/backup_stepslib.php +++ b/backup/moodle2/backup_stepslib.php @@ -2368,7 +2368,8 @@ class backup_course_completion_structure_step extends backup_structure_step { $cc = new backup_nested_element('course_completion'); $criteria = new backup_nested_element('course_completion_criteria', array('id'), array( - 'course','criteriatype', 'module', 'moduleinstance', 'courseinstanceshortname', 'enrolperiod', 'timeend', 'gradepass', 'role' + 'course', 'criteriatype', 'module', 'moduleinstance', 'courseinstanceshortname', 'enrolperiod', + 'timeend', 'gradepass', 'role', 'roleshortname' )); $criteriacompletions = new backup_nested_element('course_completion_crit_completions'); @@ -2391,11 +2392,30 @@ class backup_course_completion_structure_step extends backup_structure_step { $cc->add_child($coursecompletions); $cc->add_child($aggregatemethod); - // We need to get the courseinstances shortname rather than an ID for restore - $criteria->set_source_sql("SELECT ccc.*, c.shortname AS courseinstanceshortname - FROM {course_completion_criteria} ccc - LEFT JOIN {course} c ON c.id = ccc.courseinstance - WHERE ccc.course = ?", array(backup::VAR_COURSEID)); + /* + We need some extra data for the restore + - courseinstances shortname rather than an ID + - roleshortname in case restoring on a different site + / + $criteria->set_source_sql( + " + SELECT + ccc.*, + c.shortname AS courseinstanceshortname, + r.shortname AS roleshortname + FROM + {course_completion_criteria} ccc + LEFT JOIN + {course} c + ON c.id = ccc.courseinstance + LEFT JOIN + {role} r + ON r.id = ccc.role + WHERE + ccc.course = ? + ", + array(backup::VAR_COURSEID) + ); $aggregatemethod->set_source_table('course_completion_aggr_methd', array('course' => backup::VAR_COURSEID)); diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index ef735117acf..f9ac8c69c85 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -2504,18 +2504,37 @@ class restore_course_completion_structure_step extends restore_structure_step { $data->timeend = $this->apply_date_offset($data->timeend); // Map the role from the criteria - if (!empty($data->role)) { - $data->role = $this->get_mappingid('role', $data->role); - } + if (isset($data->role) && $data->role != '') { - $skipcriteria = false; + // If same site use the same role id, otherwise try calculate it. + if (!$this->task->is_samesite()) { + // Newer backups should include roleshortname, which makes this much easier. + if (!empty($data->roleshortname)) { + $roleinstanceid = $DB->get_field('role', 'id', array('shortname' => $data->roleshortname)); + if (!$roleinstanceid) { + debugging('Could not match the role shortname in course_completion_criteria, so skipping'); + return; + } + $data->role = $roleinstanceid; + } else { + $data->role = $this->get_mappingid('role', $data->role); + } + } + + // Check we have an id, otherwise it causes all sorts of bugs. + if (!$data->role) { + debugging('Could not match role in course_completion_criteria, so skipping'); + return; + } + } // If the completion criteria is for a module we need to map the module instance // to the new module id. if (!empty($data->moduleinstance) && !empty($data->module)) { $data->moduleinstance = $this->get_mappingid('course_module', $data->moduleinstance); if (empty($data->moduleinstance)) { - $skipcriteria = true; + debugging('Could not match the module instance in course_completion_criteria, so skipping'); + return; } } else { $data->module = null; @@ -2526,28 +2545,27 @@ class restore_course_completion_structure_step extends restore_structure_step { if (!empty($data->courseinstanceshortname)) { $courseinstanceid = $DB->get_field('course', 'id', array('shortname'=>$data->courseinstanceshortname)); if (!$courseinstanceid) { - $skipcriteria = true; + debugging('Could not match the course instance in course_completion_criteria, so skipping'); + return; } } else { $courseinstanceid = null; } $data->courseinstance = $courseinstanceid; - if (!$skipcriteria) { - $params = array( - 'course' => $data->course, - 'criteriatype' => $data->criteriatype, - 'enrolperiod' => $data->enrolperiod, - 'courseinstance' => $data->courseinstance, - 'module' => $data->module, - 'moduleinstance' => $data->moduleinstance, - 'timeend' => $data->timeend, - 'gradepass' => $data->gradepass, - 'role' => $data->role - ); - $newid = $DB->insert_record('course_completion_criteria', $params); - $this->set_mapping('course_completion_criteria', $data->id, $newid); - } + $params = array( + 'course' => $data->course, + 'criteriatype' => $data->criteriatype, + 'enrolperiod' => $data->enrolperiod, + 'courseinstance' => $data->courseinstance, + 'module' => $data->module, + 'moduleinstance' => $data->moduleinstance, + 'timeend' => $data->timeend, + 'gradepass' => $data->gradepass, + 'role' => $data->role + ); + $newid = $DB->insert_record('course_completion_criteria', $params); + $this->set_mapping('course_completion_criteria', $data->id, $newid); } /** diff --git a/completion/criteria/completion_criteria_role.php b/completion/criteria/completion_criteria_role.php index cb8c57c00b1..2413fb4215f 100644 --- a/completion/criteria/completion_criteria_role.php +++ b/completion/criteria/completion_criteria_role.php @@ -123,6 +123,9 @@ class completion_criteria_role extends completion_criteria { public function get_title() { global $DB; $role = $DB->get_record('role', array('id' => $this->role)); + if (!$role) { + return '['.get_string('roleidnotfound', 'completion', $this->role).']'; + } return role_get_name($role, context_course::instance($this->course)); } diff --git a/lang/en/completion.php b/lang/en/completion.php index 2573482a470..fb7e490af31 100644 --- a/lang/en/completion.php +++ b/lang/en/completion.php @@ -165,6 +165,7 @@ $string['restoringcompletiondata'] = 'Writing completion data'; $string['roleaggregation'] = 'Condition requires'; $string['roleaggregation_all'] = 'ALL selected roles to mark when the condition is met'; $string['roleaggregation_any'] = 'ANY selected roles to mark when the condition is met'; +$string['roleidnotfound'] = 'Role ID {$a} not found'; $string['saved'] = 'Saved'; $string['seedetails'] = 'See details'; $string['self'] = 'Self';