From a776415d553063cbe2f4279fd2411033c8da59e2 Mon Sep 17 00:00:00 2001 From: Cameron Ball Date: Thu, 13 Apr 2017 12:44:27 +0800 Subject: [PATCH] MDL-58578 core_calendar: Properly respect priorities on overview Prior to this patch, if a user was in two groups, and an override existed for both groups in an assignment the override visually lower on the override list would be displayed on the overview, whereas the one visually higher would be displayed in the assignment grading table. --- .../raw_event_retrieval_strategy.php | 2 +- calendar/lib.php | 4 +- calendar/tests/lib_test.php | 4 +- calendar/tests/local_api_test.php | 183 ++++++++++++++++++ .../raw_event_retrieval_strategy_test.php | 2 +- lib/db/upgrade.php | 23 +++ version.php | 2 +- 7 files changed, 213 insertions(+), 7 deletions(-) diff --git a/calendar/classes/local/event/strategies/raw_event_retrieval_strategy.php b/calendar/classes/local/event/strategies/raw_event_retrieval_strategy.php index 7d1836149e0..252d37412fe 100644 --- a/calendar/classes/local/event/strategies/raw_event_retrieval_strategy.php +++ b/calendar/classes/local/event/strategies/raw_event_retrieval_strategy.php @@ -228,7 +228,7 @@ class raw_event_retrieval_strategy implements raw_event_retrieval_strategy_inter $subquery = "SELECT ev.modulename, ev.instance, ev.eventtype, - MAX(ev.priority) as priority + MIN(ev.priority) as priority FROM {event} ev $subquerywhere GROUP BY ev.modulename, ev.instance, ev.eventtype"; diff --git a/calendar/lib.php b/calendar/lib.php index 94233f1c41d..5df2607af51 100644 --- a/calendar/lib.php +++ b/calendar/lib.php @@ -123,7 +123,7 @@ define('CALENDAR_SUBSCRIPTION_REMOVE', 2); /** * CALENDAR_EVENT_USER_OVERRIDE_PRIORITY - Constant for the user override priority. */ -define('CALENDAR_EVENT_USER_OVERRIDE_PRIORITY', 9999999); +define('CALENDAR_EVENT_USER_OVERRIDE_PRIORITY', 0); /** * CALENDAR_EVENT_TYPE_STANDARD - Standard events. @@ -1233,7 +1233,7 @@ function calendar_get_events($tstart, $tend, $users, $groups, $courses, $withdur $subquery = "SELECT ev.modulename, ev.instance, ev.eventtype, - MAX(ev.priority) as priority + MIN(ev.priority) as priority FROM {event} ev $subquerywhere GROUP BY ev.modulename, ev.instance, ev.eventtype"; diff --git a/calendar/tests/lib_test.php b/calendar/tests/lib_test.php index 750bb9012d8..837692e7c73 100644 --- a/calendar/tests/lib_test.php +++ b/calendar/tests/lib_test.php @@ -223,7 +223,7 @@ class core_calendar_lib_testcase extends advanced_testcase { $events = calendar_get_events($timestart, $timeend, $group12student->id, $groups, $course->id); $this->assertCount(1, $events); $event = reset($events); - $this->assertEquals('Assignment 1 due date - Group B override', $event->name); + $this->assertEquals('Assignment 1 due date - Group A override', $event->name); // Get events for user that belongs to group A and has no user override events. $this->setUser($group1student); $events = calendar_get_events($timestart, $timeend, $group1student->id, $groups, $course->id); @@ -524,7 +524,7 @@ class core_calendar_lib_testcase extends advanced_testcase { $events = calendar_get_legacy_events($timestart, $timeend, $group12student->id, $groups, $course->id); $this->assertCount(1, $events); $event = reset($events); - $this->assertEquals('Assignment 1 due date - Group B override', $event->name); + $this->assertEquals('Assignment 1 due date - Group A override', $event->name); // Get events for user that belongs to group A and has no user override events. $this->setUser($group1student); diff --git a/calendar/tests/local_api_test.php b/calendar/tests/local_api_test.php index b206454f12d..5e4b16a05c7 100644 --- a/calendar/tests/local_api_test.php +++ b/calendar/tests/local_api_test.php @@ -675,4 +675,187 @@ class core_calendar_local_api_testcase extends advanced_testcase { $event = reset($events); $this->assertEquals('assign', $event->modulename); } + + /** + * Test for \core_calendar\local\api::get_legacy_events() when there are user and group overrides. + */ + public function test_get_legacy_events_with_overrides() { + $generator = $this->getDataGenerator(); + + $course = $generator->create_course(); + + $plugingenerator = $this->getDataGenerator()->get_plugin_generator('mod_assign'); + if (!isset($params['course'])) { + $params['course'] = $course->id; + } + + $instance = $plugingenerator->create_instance($params); + + // Create users. + $useroverridestudent = $generator->create_user(); + $group1student = $generator->create_user(); + $group2student = $generator->create_user(); + $group12student = $generator->create_user(); + $nogroupstudent = $generator->create_user(); + + // Enrol users. + $generator->enrol_user($useroverridestudent->id, $course->id, 'student'); + $generator->enrol_user($group1student->id, $course->id, 'student'); + $generator->enrol_user($group2student->id, $course->id, 'student'); + $generator->enrol_user($group12student->id, $course->id, 'student'); + $generator->enrol_user($nogroupstudent->id, $course->id, 'student'); + + // Create groups. + $group1 = $generator->create_group(['courseid' => $course->id]); + $group2 = $generator->create_group(['courseid' => $course->id]); + + // Add members to groups. + $generator->create_group_member(['groupid' => $group1->id, 'userid' => $group1student->id]); + $generator->create_group_member(['groupid' => $group2->id, 'userid' => $group2student->id]); + $generator->create_group_member(['groupid' => $group1->id, 'userid' => $group12student->id]); + $generator->create_group_member(['groupid' => $group2->id, 'userid' => $group12student->id]); + $now = time(); + + // Events with the same module name, instance and event type. + $events = [ + [ + 'name' => 'Assignment 1 due date', + 'description' => '', + 'format' => 0, + 'courseid' => $course->id, + 'groupid' => 0, + 'userid' => 2, + 'modulename' => 'assign', + 'instance' => $instance->id, + 'eventtype' => 'due', + 'timestart' => $now, + 'timeduration' => 0, + 'visible' => 1 + ], [ + 'name' => 'Assignment 1 due date - User override', + 'description' => '', + 'format' => 1, + 'courseid' => 0, + 'groupid' => 0, + 'userid' => $useroverridestudent->id, + 'modulename' => 'assign', + 'instance' => $instance->id, + 'eventtype' => 'due', + 'timestart' => $now + 86400, + 'timeduration' => 0, + 'visible' => 1, + 'priority' => CALENDAR_EVENT_USER_OVERRIDE_PRIORITY + ], [ + 'name' => 'Assignment 1 due date - Group A override', + 'description' => '', + 'format' => 1, + 'courseid' => $course->id, + 'groupid' => $group1->id, + 'userid' => 2, + 'modulename' => 'assign', + 'instance' => $instance->id, + 'eventtype' => 'due', + 'timestart' => $now + (2 * 86400), + 'timeduration' => 0, + 'visible' => 1, + 'priority' => 1, + ], [ + 'name' => 'Assignment 1 due date - Group B override', + 'description' => '', + 'format' => 1, + 'courseid' => $course->id, + 'groupid' => $group2->id, + 'userid' => 2, + 'modulename' => 'assign', + 'instance' => $instance->id, + 'eventtype' => 'due', + 'timestart' => $now + (3 * 86400), + 'timeduration' => 0, + 'visible' => 1, + 'priority' => 2, + ], + ]; + + foreach ($events as $event) { + calendar_event::create($event, false); + } + + $timestart = $now - 100; + $timeend = $now + (3 * 86400); + $groups = [$group1->id, $group2->id]; + + // Get user override events. + $this->setUser($useroverridestudent); + $events = calendar_get_legacy_events($timestart, $timeend, $useroverridestudent->id, $groups, $course->id); + $this->assertCount(1, $events); + $event = reset($events); + $this->assertEquals('Assignment 1 due date - User override', $event->name); + + // Get event for user with override but with the timestart and timeend parameters only covering the original event. + $events = calendar_get_legacy_events($timestart, $now, $useroverridestudent->id, $groups, $course->id); + $this->assertCount(0, $events); + + // Get events for user that does not belong to any group and has no user override events. + $this->setUser($nogroupstudent); + $events = calendar_get_legacy_events($timestart, $timeend, $nogroupstudent->id, $groups, $course->id); + $this->assertCount(1, $events); + $event = reset($events); + $this->assertEquals('Assignment 1 due date', $event->name); + + // Get events for user that belongs to groups A and B and has no user override events. + $this->setUser($group12student); + $events = calendar_get_legacy_events($timestart, $timeend, $group12student->id, $groups, $course->id); + $this->assertCount(1, $events); + $event = reset($events); + $this->assertEquals('Assignment 1 due date - Group A override', $event->name); + + // Get events for user that belongs to group A and has no user override events. + $this->setUser($group1student); + $events = calendar_get_legacy_events($timestart, $timeend, $group1student->id, $groups, $course->id); + $this->assertCount(1, $events); + $event = reset($events); + $this->assertEquals('Assignment 1 due date - Group A override', $event->name); + + // Add repeating events. + $repeatingevents = [ + [ + 'name' => 'Repeating site event', + 'description' => '', + 'format' => 1, + 'courseid' => SITEID, + 'groupid' => 0, + 'userid' => 2, + 'repeatid' => $event->id, + 'modulename' => '0', + 'instance' => 0, + 'eventtype' => 'site', + 'timestart' => $now + 86400, + 'timeduration' => 0, + 'visible' => 1, + ], + [ + 'name' => 'Repeating site event', + 'description' => '', + 'format' => 1, + 'courseid' => SITEID, + 'groupid' => 0, + 'userid' => 2, + 'repeatid' => $event->id, + 'modulename' => '0', + 'instance' => 0, + 'eventtype' => 'site', + 'timestart' => $now + (2 * 86400), + 'timeduration' => 0, + 'visible' => 1, + ], + ]; + + foreach ($repeatingevents as $event) { + calendar_event::create($event, false); + } + + // Make sure repeating events are not filtered out. + $events = calendar_get_legacy_events($timestart, $timeend, true, true, true); + $this->assertCount(3, $events); + } } diff --git a/calendar/tests/raw_event_retrieval_strategy_test.php b/calendar/tests/raw_event_retrieval_strategy_test.php index cbf8d706d40..82d77948ad9 100644 --- a/calendar/tests/raw_event_retrieval_strategy_test.php +++ b/calendar/tests/raw_event_retrieval_strategy_test.php @@ -226,7 +226,7 @@ class core_calendar_raw_event_retrieval_strategy_testcase extends advanced_testc $events = $retrievalstrategy->get_raw_events([$group12student->id], $groups, [$course->id]); $this->assertCount(1, $events); $event = reset($events); - $this->assertEquals('Assignment 1 due date - Group B override', $event->name); + $this->assertEquals('Assignment 1 due date - Group A override', $event->name); // Get events for user that belongs to group A and has no user override events. $this->setUser($group1student); diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 007594f52ea..3b9f6d8c992 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2575,6 +2575,10 @@ function xmldb_main_upgrade($oldversion) { $record->nextruntime = $nextruntime; $DB->insert_record('task_adhoc', $record); + // This same task is queued again in a later step, but if we already queue it here + // then there is no need to queue it again. We use this flag in the second step. + $refresheventsadhocadded = true; + // Main savepoint reached. upgrade_main_savepoint(true, 2017030700.00); } @@ -2834,5 +2838,24 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2017041801.00); } + if ($oldversion < 2017042600.01) { + // If the previous step didn't execute and queue the task. + if (!isset($refresheventsadhocadded)) { + // Create adhoc task for upgrading of existing calendar events. + $record = new \stdClass(); + $record->classname = "\\core\\task\\refresh_mod_calendar_events_task"; + $record->component = 'core'; + + // Next run time based from nextruntime computation in \core\task\manager::queue_adhoc_task(). + $nextruntime = time() - 1; + $record->nextruntime = $nextruntime; + $DB->insert_record('task_adhoc', $record); + + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2017042600.01); + } + return true; } diff --git a/version.php b/version.php index 92ad9dcb552..81dd163899e 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2017042600.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2017042600.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes.