From 127b54ffbe4ad69472d66e73cc4099e72969c694 Mon Sep 17 00:00:00 2001 From: Zig Tan Date: Fri, 29 Jun 2018 11:21:45 +0800 Subject: [PATCH 1/3] MDL-61870 mod_assign: Fix/clean up imported group override duedates - Prevent group override duedate events from being imported when groups are excluded - Clean up any existing group override duedate events when editing assignment in upgradelib.php - Updated lib.php unit tests --- mod/assign/db/upgrade.php | 10 +++++ mod/assign/lib.php | 2 +- mod/assign/tests/lib_test.php | 31 ++++++++++++--- mod/assign/upgradelib.php | 75 +++++++++++++++++++++++++++++++++++ mod/assign/version.php | 2 +- 5 files changed, 112 insertions(+), 8 deletions(-) diff --git a/mod/assign/db/upgrade.php b/mod/assign/db/upgrade.php index 9908d9982b7..cfb745342e5 100644 --- a/mod/assign/db/upgrade.php +++ b/mod/assign/db/upgrade.php @@ -171,5 +171,15 @@ function xmldb_assign_upgrade($oldversion) { // Automatically generated Moodle v3.5.0 release upgrade line. // Put any upgrade step following this. + if ($oldversion < 2018061100) { + require_once($CFG->dirroot.'/mod/assign/upgradelib.php'); + + // Clean up duplicate event records that may have been generated from MDL-61870. + delete_assignment_duplicate_group_events(); + + // Main savepoint reached. + upgrade_mod_savepoint(true, 2018061100, 'assign'); + } + return true; } diff --git a/mod/assign/lib.php b/mod/assign/lib.php index 5eeccf327e4..e1872d30938 100644 --- a/mod/assign/lib.php +++ b/mod/assign/lib.php @@ -258,7 +258,7 @@ function assign_update_events($assign, $override = null) { // Only load events for this override. if (isset($override->userid)) { $conds['userid'] = $override->userid; - } else { + } else if (isset($override->groupid)) { $conds['groupid'] = $override->groupid; } } diff --git a/mod/assign/tests/lib_test.php b/mod/assign/tests/lib_test.php index 9c758c1086a..1d47c3675da 100644 --- a/mod/assign/tests/lib_test.php +++ b/mod/assign/tests/lib_test.php @@ -362,14 +362,23 @@ class mod_assign_lib_testcase extends advanced_testcase { ]); $instance = $assign->get_instance(); - $eventparams = ['modulename' => 'assign', 'instance' => $instance->id]; + $eventparams = [ + 'modulename' => 'assign', + 'instance' => $instance->id, + 'eventtype' => ASSIGN_EVENT_TYPE_DUE, + 'groupid' => 0 + ]; // Make sure the calendar event for assignment 1 matches the initial due date. $eventtime = $DB->get_field('event', 'timestart', $eventparams, MUST_EXIST); $this->assertEquals($eventtime, $duedate); // Manually update assignment 1's due date. - $DB->update_record('assign', (object) ['id' => $instance->id, 'duedate' => $newduedate]); + $DB->update_record('assign', (object) [ + 'id' => $instance->id, + 'duedate' => $newduedate, + 'course' => $course->id + ]); // Then refresh the assignment events of assignment 1's course. $this->assertTrue(assign_refresh_events($course->id)); @@ -380,15 +389,25 @@ class mod_assign_lib_testcase extends advanced_testcase { // Create a second course and assignment. $othercourse = $this->getDataGenerator()->create_course();; - $otherassign = $this->create_instance($othercourse, ['duedate' => $duedate, 'course' => $othercourse->id]); + $otherassign = $this->create_instance($othercourse, [ + 'duedate' => $duedate, + ]); $otherinstance = $otherassign->get_instance(); // Manually update assignment 1 and 2's due dates. $newduedate += DAYSECS; - $DB->update_record('assign', (object)['id' => $instance->id, 'duedate' => $newduedate]); - $DB->update_record('assign', (object)['id' => $otherinstance->id, 'duedate' => $newduedate]); + $DB->update_record('assign', (object)[ + 'id' => $instance->id, + 'duedate' => $newduedate, + 'course' => $course->id + ]); + $DB->update_record('assign', (object)[ + 'id' => $otherinstance->id, + 'duedate' => $newduedate, + 'course' => $othercourse->id + ]); - // Refresh events of all courses. + // Refresh events of all courses and check the calendar events matches the new date. $this->assertTrue(assign_refresh_events()); // Check the due date calendar event for assignment 1. diff --git a/mod/assign/upgradelib.php b/mod/assign/upgradelib.php index 10ab600920a..d0bbc45a93a 100644 --- a/mod/assign/upgradelib.php +++ b/mod/assign/upgradelib.php @@ -438,3 +438,78 @@ function get_assignments_with_rescaled_null_grades() { return $assignments; } + +/** + * Determined if the assignment has any duplicate group events generated from + * restoring Course backup without groups, and deletes any records found. + * + * Bug fix data clean up for MDL-61870. + */ +function delete_assignment_duplicate_group_events() { + global $DB; + + // Get all Course's assign course modules to check for any duplicate group events to remove. + list($coursesinsql, $coursesinparams) = $DB->get_in_or_equal(array_keys(get_courses()), SQL_PARAMS_NAMED); + + $query = "SELECT cm.id AS id, + cm.course AS courseid, + cm.instance AS instanceid + FROM {course_modules} cm + JOIN {modules} m ON m.id = cm.module + WHERE m.name = :modulename + AND (cm.course $coursesinsql)"; + $params = ['modulename' => 'assign']; + $params += $coursesinparams; + + foreach ($DB->get_records_sql($query, $params) as $cm) { + $selectgroupevents = "courseid = :courseid + AND modulename = :modulename + AND eventtype = :eventtype + AND instance = :instance + AND groupid <> :groupid + AND priority <> :priority"; + $paramsgroupevents = [ + 'courseid' => $cm->courseid, + 'modulename' => 'assign', + 'eventtype' => 'due', + 'instance' => $cm->instanceid, + 'groupid' => 0, + 'priority' => 0 + ]; + + // Retrieve all the Course's assign events associated with group overrides, + // which will be use to look for duplicate records that need to be deleted. + foreach ($DB->get_records_select('event', $selectgroupevents, $paramsgroupevents) as $groupevent) { + // Delete any duplicates that match the details of the current groupevent but the id does not + // match the current groupevent id and course id, groupid is 0, and priority is NULL. + $selectduplicates = "id != :eventid + AND courseid != :courseid + AND groupid = 0 + AND userid = :userid + AND repeatid = :repeatid + AND modulename = :modulename + AND type = :type + AND eventtype = :eventtype + AND timestart = :timestart + AND timeduration = :timeduration + AND timesort = :timesort + AND sequence = :sequence + AND priority IS NULL"; + $paramsduplicates = [ + 'eventid' => $groupevent->id, + 'courseid' => $groupevent->courseid, + 'groupid' => 0, + 'userid' => $groupevent->userid, + 'repeatid' => $groupevent->repeatid, + 'modulename' => $groupevent->modulename, + 'type' => $groupevent->type, + 'eventtype' => $groupevent->eventtype, + 'timestart' => $groupevent->timestart, + 'timeduration' => $groupevent->timeduration, + 'timesort' => $groupevent->timesort, + 'sequence' => $groupevent->sequence + ]; + $DB->delete_records_select('event', $selectduplicates, $paramsduplicates); + } + } +} diff --git a/mod/assign/version.php b/mod/assign/version.php index 0fe2c5731b4..45fe8fba70c 100644 --- a/mod/assign/version.php +++ b/mod/assign/version.php @@ -25,6 +25,6 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'mod_assign'; // Full name of the plugin (used for diagnostics). -$plugin->version = 2018051400; // The current module version (Date: YYYYMMDDXX). +$plugin->version = 2018061100; // The current module version (Date: YYYYMMDDXX). $plugin->requires = 2018050800; // Requires this Moodle version. $plugin->cron = 60; From 805417c33d047e8cad5544e6ae6115c2cd5109a0 Mon Sep 17 00:00:00 2001 From: Zig Tan Date: Fri, 29 Jun 2018 17:07:35 +0800 Subject: [PATCH 2/3] MDL-61870 mod_assign: Fix/clean up imported group override duedates Applying patch supplied from Damyon Wiese to address the root-cause of this issue in the backup/restore logic. --- .../moodle2/restore_assign_stepslib.php | 6 ++ mod/assign/db/upgrade.php | 10 --- mod/assign/lib.php | 3 + mod/assign/upgradelib.php | 75 ------------------- mod/assign/version.php | 2 +- 5 files changed, 10 insertions(+), 86 deletions(-) diff --git a/mod/assign/backup/moodle2/restore_assign_stepslib.php b/mod/assign/backup/moodle2/restore_assign_stepslib.php index 0987c53e531..d4ba0a56a18 100644 --- a/mod/assign/backup/moodle2/restore_assign_stepslib.php +++ b/mod/assign/backup/moodle2/restore_assign_stepslib.php @@ -387,6 +387,12 @@ class restore_assign_activity_structure_step extends restore_activity_structure_ return; } + // Skip group overrides if we are not restoring groupinfo. + $groupinfo = $this->get_setting_value('groups'); + if (!$groupinfo && !is_null($data->groupid)) { + return; + } + $data->assignid = $this->get_new_parentid('assign'); if (!is_null($data->userid)) { diff --git a/mod/assign/db/upgrade.php b/mod/assign/db/upgrade.php index cfb745342e5..9908d9982b7 100644 --- a/mod/assign/db/upgrade.php +++ b/mod/assign/db/upgrade.php @@ -171,15 +171,5 @@ function xmldb_assign_upgrade($oldversion) { // Automatically generated Moodle v3.5.0 release upgrade line. // Put any upgrade step following this. - if ($oldversion < 2018061100) { - require_once($CFG->dirroot.'/mod/assign/upgradelib.php'); - - // Clean up duplicate event records that may have been generated from MDL-61870. - delete_assignment_duplicate_group_events(); - - // Main savepoint reached. - upgrade_mod_savepoint(true, 2018061100, 'assign'); - } - return true; } diff --git a/mod/assign/lib.php b/mod/assign/lib.php index e1872d30938..36336d6247e 100644 --- a/mod/assign/lib.php +++ b/mod/assign/lib.php @@ -260,6 +260,9 @@ function assign_update_events($assign, $override = null) { $conds['userid'] = $override->userid; } else if (isset($override->groupid)) { $conds['groupid'] = $override->groupid; + } else { + // This is not a valid override, it may have been left from a bad import or restore. + $conds['groupid'] = $conds['userid'] = 0; } } $oldevents = $DB->get_records('event', $conds, 'id ASC'); diff --git a/mod/assign/upgradelib.php b/mod/assign/upgradelib.php index d0bbc45a93a..10ab600920a 100644 --- a/mod/assign/upgradelib.php +++ b/mod/assign/upgradelib.php @@ -438,78 +438,3 @@ function get_assignments_with_rescaled_null_grades() { return $assignments; } - -/** - * Determined if the assignment has any duplicate group events generated from - * restoring Course backup without groups, and deletes any records found. - * - * Bug fix data clean up for MDL-61870. - */ -function delete_assignment_duplicate_group_events() { - global $DB; - - // Get all Course's assign course modules to check for any duplicate group events to remove. - list($coursesinsql, $coursesinparams) = $DB->get_in_or_equal(array_keys(get_courses()), SQL_PARAMS_NAMED); - - $query = "SELECT cm.id AS id, - cm.course AS courseid, - cm.instance AS instanceid - FROM {course_modules} cm - JOIN {modules} m ON m.id = cm.module - WHERE m.name = :modulename - AND (cm.course $coursesinsql)"; - $params = ['modulename' => 'assign']; - $params += $coursesinparams; - - foreach ($DB->get_records_sql($query, $params) as $cm) { - $selectgroupevents = "courseid = :courseid - AND modulename = :modulename - AND eventtype = :eventtype - AND instance = :instance - AND groupid <> :groupid - AND priority <> :priority"; - $paramsgroupevents = [ - 'courseid' => $cm->courseid, - 'modulename' => 'assign', - 'eventtype' => 'due', - 'instance' => $cm->instanceid, - 'groupid' => 0, - 'priority' => 0 - ]; - - // Retrieve all the Course's assign events associated with group overrides, - // which will be use to look for duplicate records that need to be deleted. - foreach ($DB->get_records_select('event', $selectgroupevents, $paramsgroupevents) as $groupevent) { - // Delete any duplicates that match the details of the current groupevent but the id does not - // match the current groupevent id and course id, groupid is 0, and priority is NULL. - $selectduplicates = "id != :eventid - AND courseid != :courseid - AND groupid = 0 - AND userid = :userid - AND repeatid = :repeatid - AND modulename = :modulename - AND type = :type - AND eventtype = :eventtype - AND timestart = :timestart - AND timeduration = :timeduration - AND timesort = :timesort - AND sequence = :sequence - AND priority IS NULL"; - $paramsduplicates = [ - 'eventid' => $groupevent->id, - 'courseid' => $groupevent->courseid, - 'groupid' => 0, - 'userid' => $groupevent->userid, - 'repeatid' => $groupevent->repeatid, - 'modulename' => $groupevent->modulename, - 'type' => $groupevent->type, - 'eventtype' => $groupevent->eventtype, - 'timestart' => $groupevent->timestart, - 'timeduration' => $groupevent->timeduration, - 'timesort' => $groupevent->timesort, - 'sequence' => $groupevent->sequence - ]; - $DB->delete_records_select('event', $selectduplicates, $paramsduplicates); - } - } -} diff --git a/mod/assign/version.php b/mod/assign/version.php index 45fe8fba70c..0fe2c5731b4 100644 --- a/mod/assign/version.php +++ b/mod/assign/version.php @@ -25,6 +25,6 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'mod_assign'; // Full name of the plugin (used for diagnostics). -$plugin->version = 2018061100; // The current module version (Date: YYYYMMDDXX). +$plugin->version = 2018051400; // The current module version (Date: YYYYMMDDXX). $plugin->requires = 2018050800; // Requires this Moodle version. $plugin->cron = 60; From f86bd7ece031d5ec4f05354de8d49fa34ab33a4e Mon Sep 17 00:00:00 2001 From: Damyon Wiese Date: Mon, 2 Jul 2018 10:31:47 +0800 Subject: [PATCH 3/3] MDL-61870 mod_assign: Conditionally backup groups When group info is not backed up, do not backup assignment submissions or overrides that related to a specific group. We are already correctly not restoring them, but it is more robust not to include them in the backup file at all. --- .../backup/moodle2/backup_assign_stepslib.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/mod/assign/backup/moodle2/backup_assign_stepslib.php b/mod/assign/backup/moodle2/backup_assign_stepslib.php index 4facb4fda31..0f018ca9212 100644 --- a/mod/assign/backup/moodle2/backup_assign_stepslib.php +++ b/mod/assign/backup/moodle2/backup_assign_stepslib.php @@ -61,6 +61,7 @@ class backup_assign_activity_structure_step extends backup_activity_structure_st // To know if we are including userinfo. $userinfo = $this->get_setting_value('userinfo'); + $groupinfo = $this->get_setting_value('groups'); // Define each element separated. $assign = new backup_nested_element('assign', array('id'), @@ -159,8 +160,12 @@ class backup_assign_activity_structure_step extends backup_activity_structure_st $userflag->set_source_table('assign_user_flags', array('assignment' => backup::VAR_PARENTID)); - $submission->set_source_table('assign_submission', - array('assignment' => backup::VAR_PARENTID)); + $submissionparams = array('assignment' => backup::VAR_PARENTID); + if (!$groupinfo) { + // Without group info, skip group submissions. + $submissionparams['groupid'] = backup_helper::is_sqlparam(0); + } + $submission->set_source_table('assign_submission', $submissionparams); $grade->set_source_table('assign_grades', array('assignment' => backup::VAR_PARENTID)); @@ -172,6 +177,10 @@ class backup_assign_activity_structure_step extends backup_activity_structure_st $overrideparams['userid'] = backup_helper::is_sqlparam(null); // Without userinfo, skip user overrides. } + if (!$groupinfo) { + // Without group info, skip group overrides. + $overrideparams['groupid'] = backup_helper::is_sqlparam(0); + } $override->set_source_table('assign_overrides', $overrideparams); // Define id annotations.