From a33e54c08fccb1564a2978f3ec17b01140da9325 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Thu, 23 May 2024 23:00:11 +0100 Subject: [PATCH] MDL-82041 reportbuilder: switch time-sensitive code to new Clock API. Where current implementation, and more specifically tests, rely on the current time then replace that with the PSR-20 Clock from 298c13ac3b. Applicable updates to the date filter and report scheduling calculation. --- .upgradenotes/MDL-82041-2024053011364585.yml | 13 ++ reportbuilder/classes/local/filters/date.php | 10 +- .../classes/local/helpers/schedule.php | 22 +- reportbuilder/classes/task/send_schedule.php | 7 +- reportbuilder/classes/task/send_schedules.php | 5 +- reportbuilder/tests/generator/lib.php | 8 +- .../tests/local/filters/date_test.php | 46 +++-- .../tests/local/helpers/schedule_test.php | 189 +++++++++++------- 8 files changed, 191 insertions(+), 109 deletions(-) create mode 100644 .upgradenotes/MDL-82041-2024053011364585.yml diff --git a/.upgradenotes/MDL-82041-2024053011364585.yml b/.upgradenotes/MDL-82041-2024053011364585.yml new file mode 100644 index 00000000000..752c8300e98 --- /dev/null +++ b/.upgradenotes/MDL-82041-2024053011364585.yml @@ -0,0 +1,13 @@ +issueNumber: MDL-82041 +notes: + core_reportbuilder: + - message: >- + All time related code has been updated to the PSR-20 Clock interface, as + such the following methods no longer accept a `$timenow` parameter + (instead please use `\core\clock` dependency injection): + + - `core_reportbuilder_generator::create_schedule` + + - + `core_reportbuilder\local\helpers\schedule::[create_schedule|calculate_next_send_time]` + type: changed diff --git a/reportbuilder/classes/local/filters/date.php b/reportbuilder/classes/local/filters/date.php index f1f12346144..f7c03935cb2 100644 --- a/reportbuilder/classes/local/filters/date.php +++ b/reportbuilder/classes/local/filters/date.php @@ -18,10 +18,10 @@ declare(strict_types=1); namespace core_reportbuilder\local\filters; -use DateTimeImmutable; +use core\{clock, di}; +use core_reportbuilder\local\helpers\database; use lang_string; use MoodleQuickForm; -use core_reportbuilder\local\helpers\database; /** * Date report filter @@ -246,12 +246,12 @@ class date extends base { case self::DATE_PAST: $param = database::generate_param_name(); $sql = "{$fieldsql} < :{$param}"; - $params[$param] = time(); + $params[$param] = di::get(clock::class)->time(); break; case self::DATE_FUTURE: $param = database::generate_param_name(); $sql = "{$fieldsql} > :{$param}"; - $params[$param] = time(); + $params[$param] = di::get(clock::class)->time(); break; default: // Invalid or inactive filter. @@ -271,7 +271,7 @@ class date extends base { */ private static function get_relative_timeframe(int $operator, int $dateunitvalue, int $dateunit): array { // Initialise start/end time to now. - $datestart = $dateend = new DateTimeImmutable(); + $datestart = $dateend = di::get(clock::class)->now(); switch ($dateunit) { case self::DATE_UNIT_HOUR: diff --git a/reportbuilder/classes/local/helpers/schedule.php b/reportbuilder/classes/local/helpers/schedule.php index 0940afee914..86ba8d5b6cd 100644 --- a/reportbuilder/classes/local/helpers/schedule.php +++ b/reportbuilder/classes/local/helpers/schedule.php @@ -19,6 +19,7 @@ declare(strict_types=1); namespace core_reportbuilder\local\helpers; use context_user; +use core\{clock, di}; use core_user; use invalid_parameter_exception; use stdClass; @@ -43,14 +44,18 @@ class schedule { * Create report schedule, calculate when it should be next sent * * @param stdClass $data - * @param int|null $timenow Time to use as comparison against current date (defaults to current time) + * @param int|null $timenow Deprecated since Moodle 4.5 - please use {@see clock} dependency injection * @return model */ public static function create_schedule(stdClass $data, ?int $timenow = null): model { + if ($timenow !== null) { + debugging('Passing $timenow is deprecated, please use \core\clock dependency injection', DEBUG_DEVELOPER); + } + $data->name = trim($data->name); $schedule = (new model(0, $data)); - $schedule->set('timenextsend', self::calculate_next_send_time($schedule, $timenow)); + $schedule->set('timenextsend', self::calculate_next_send_time($schedule)); return $schedule->create(); } @@ -209,7 +214,7 @@ class schedule { return false; } - $timenow = time(); + $timenow = di::get(clock::class)->time(); // Ensure we've reached the initial scheduled start time. $timescheduled = $schedule->get('timescheduled'); @@ -231,13 +236,17 @@ class schedule { * returned value is after the current date * * @param model $schedule - * @param int|null $timenow Time to use as comparison against current date (defaults to current time) + * @param int|null $timenow Deprecated since Moodle 4.5 - please use {@see clock} dependency injection * @return int */ public static function calculate_next_send_time(model $schedule, ?int $timenow = null): int { global $CFG; - $timenow = $timenow ?? time(); + if ($timenow !== null) { + debugging('Passing $timenow is deprecated, please use \core\clock dependency injection', DEBUG_DEVELOPER); + } + + $timenow = di::get(clock::class)->time(); $recurrence = $schedule->get('recurrence'); $timescheduled = $schedule->get('timescheduled'); @@ -289,8 +298,7 @@ class schedule { // Ensure we don't modify anything in the original model. $scheduleclone = new model(0, $schedule->to_record()); - return self::calculate_next_send_time( - $scheduleclone->set('timescheduled', $timestamp), $timenow); + return self::calculate_next_send_time($scheduleclone->set('timescheduled', $timestamp)); } else { return $timestamp; } diff --git a/reportbuilder/classes/task/send_schedule.php b/reportbuilder/classes/task/send_schedule.php index 79eda5a9527..f021d92430d 100644 --- a/reportbuilder/classes/task/send_schedule.php +++ b/reportbuilder/classes/task/send_schedule.php @@ -18,8 +18,9 @@ declare(strict_types=1); namespace core_reportbuilder\task; -use core_user; +use core\{clock, di}; use core\task\adhoc_task; +use core_user; use core_reportbuilder\local\helpers\schedule as helper; use core_reportbuilder\local\models\schedule; use moodle_exception; @@ -140,7 +141,9 @@ class send_schedule extends adhoc_task { } // Finish, clean up (set persistent property manually to avoid updating it's user/time modified data). - $DB->set_field($schedule::TABLE, 'timelastsent', time(), ['id' => $schedule->get('id')]); + $DB->set_field($schedule::TABLE, 'timelastsent', di::get(clock::class)->time(), [ + 'id' => $schedule->get('id'), + ]); if ($scheduleattachment !== null) { $scheduleattachment->delete(); diff --git a/reportbuilder/classes/task/send_schedules.php b/reportbuilder/classes/task/send_schedules.php index a279a6f75a4..be03861c329 100644 --- a/reportbuilder/classes/task/send_schedules.php +++ b/reportbuilder/classes/task/send_schedules.php @@ -18,6 +18,7 @@ declare(strict_types=1); namespace core_reportbuilder\task; +use core\{clock, di}; use core\task\scheduled_task; use core_reportbuilder\local\helpers\schedule; use core_reportbuilder\local\models\schedule as model; @@ -46,7 +47,9 @@ class send_schedules extends scheduled_task { public function execute(): void { global $DB; - $schedules = model::get_records_select('enabled = 1 AND timenextsend <= :time', ['time' => time()]); + $schedules = model::get_records_select('enabled = 1 AND timenextsend <= :time', [ + 'time' => di::get(clock::class)->time(), + ]); $schedules = array_filter($schedules, [schedule::class, 'should_send_schedule']); // Loop over all schedules for sending, execute corresponding task to send each individually. diff --git a/reportbuilder/tests/generator/lib.php b/reportbuilder/tests/generator/lib.php index e83d6e2d1c4..9ddcf8b197e 100644 --- a/reportbuilder/tests/generator/lib.php +++ b/reportbuilder/tests/generator/lib.php @@ -16,6 +16,7 @@ declare(strict_types=1); +use core\{clock, di}; use core_reportbuilder\manager; use core_reportbuilder\local\helpers\report as helper; use core_reportbuilder\local\helpers\schedule as schedule_helper; @@ -208,12 +209,9 @@ class core_reportbuilder_generator extends component_generator_base { $record['message'] = $record['name'] . ' message'; } if (!array_key_exists('timescheduled', $record)) { - $record['timescheduled'] = usergetmidnight(time() + DAYSECS); + $record['timescheduled'] = usergetmidnight(di::get(clock::class)->time() + DAYSECS); } - // Time to use as comparison against current date (null means current time). - $timenow = $record['timenow'] ?? null; - - return schedule_helper::create_schedule((object) $record, $timenow); + return schedule_helper::create_schedule((object) $record); } } diff --git a/reportbuilder/tests/local/filters/date_test.php b/reportbuilder/tests/local/filters/date_test.php index 9a9b40480ad..572fed4ffc2 100644 --- a/reportbuilder/tests/local/filters/date_test.php +++ b/reportbuilder/tests/local/filters/date_test.php @@ -19,8 +19,9 @@ declare(strict_types=1); namespace core_reportbuilder\local\filters; use advanced_testcase; -use lang_string; +use core\clock; use core_reportbuilder\local\report\filter; +use lang_string; /** * Unit tests for date report filter @@ -31,14 +32,25 @@ use core_reportbuilder\local\report\filter; * @copyright 2021 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class date_test extends advanced_testcase { +final class date_test extends advanced_testcase { + + /** @var clock $clock */ + private readonly clock $clock; + + /** + * Mock the clock + */ + protected function setUp(): void { + parent::setUp(); + $this->clock = $this->mock_clock_with_frozen(1622502000); + } /** * Data provider for {@see test_get_sql_filter_simple} * * @return array */ - public function get_sql_filter_simple_provider(): array { + public static function get_sql_filter_simple_provider(): array { return [ [date::DATE_ANY, true], [date::DATE_NOT_EMPTY, true], @@ -120,7 +132,7 @@ class date_test extends advanced_testcase { * * @return array */ - public function get_sql_filter_current_week_provider(): array { + public static function get_sql_filter_current_week_provider(): array { return array_map(static function(int $day): array { return [$day]; }, range(0, 6)); @@ -141,7 +153,8 @@ class date_test extends advanced_testcase { set_config('calendar_startwday', $startweekday); - $user = $this->getDataGenerator()->create_user(['timecreated' => time()]); + $usertimecreated = $this->clock->time(); + $user = $this->getDataGenerator()->create_user(['timecreated' => $usertimecreated]); $filter = new filter( date::class, @@ -165,14 +178,14 @@ class date_test extends advanced_testcase { * * @return array */ - public function get_sql_filter_current_week_no_match_provider(): array { + public static function get_sql_filter_current_week_no_match_provider(): array { $data = []; - // For each day, create provider data for -/+ 8 days. + // For each day, create provider data for -/+ 7 days. foreach (range(0, 6) as $day) { $data = array_merge($data, [ - [$day, '-8 day'], - [$day, '+8 day'], + [$day, '-7 day'], + [$day, '+7 day'], ]); } @@ -194,7 +207,7 @@ class date_test extends advanced_testcase { set_config('calendar_startwday', $startweekday); - $usertimecreated = strtotime($timecreated); + $usertimecreated = strtotime($timecreated, $this->clock->time()); $user = $this->getDataGenerator()->create_user(['timecreated' => $usertimecreated]); $filter = new filter( @@ -219,7 +232,7 @@ class date_test extends advanced_testcase { * * @return array */ - public function get_sql_filter_relative_provider(): array { + public static function get_sql_filter_relative_provider(): array { return [ 'Before hour' => [date::DATE_BEFORE, 1, date::DATE_UNIT_HOUR, '-90 minute'], 'Before day' => [date::DATE_BEFORE, 1, date::DATE_UNIT_DAY, '-25 hour'], @@ -271,8 +284,8 @@ class date_test extends advanced_testcase { 'Next two months' => [date::DATE_NEXT, 2, date::DATE_UNIT_MONTH, '+7 week'], 'Next two years' => [date::DATE_NEXT, 2, date::DATE_UNIT_YEAR, '+15 month'], - 'In the past' => [date::DATE_PAST, null, null, '-3 hour'], - 'In the future' => [date::DATE_FUTURE, null, null, '+3 hour'], + 'In the past' => [date::DATE_PAST, null, null, '-1 minute'], + 'In the future' => [date::DATE_FUTURE, null, null, '+1 minute'], ]; } @@ -291,7 +304,12 @@ class date_test extends advanced_testcase { $this->resetAfterTest(); - $usertimecreated = ($timecreated !== null ? strtotime($timecreated) : time()); + // Use relative time period if present, otherwise default to current clock time. + $usertimecreated = $this->clock->time(); + if ($timecreated !== null) { + $usertimecreated = strtotime($timecreated, $usertimecreated); + } + $user = $this->getDataGenerator()->create_user(['timecreated' => $usertimecreated]); $filter = new filter( diff --git a/reportbuilder/tests/local/helpers/schedule_test.php b/reportbuilder/tests/local/helpers/schedule_test.php index e25dffaf5ac..266490f29ec 100644 --- a/reportbuilder/tests/local/helpers/schedule_test.php +++ b/reportbuilder/tests/local/helpers/schedule_test.php @@ -20,6 +20,7 @@ namespace core_reportbuilder\local\helpers; use advanced_testcase; use invalid_parameter_exception; +use core\clock; use core_cohort\reportbuilder\audience\cohortmember; use core_reportbuilder_generator; use core_reportbuilder\local\models\schedule as model; @@ -34,7 +35,18 @@ use core_user\reportbuilder\datasource\users; * @copyright 2021 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class schedule_test extends advanced_testcase { +final class schedule_test extends advanced_testcase { + + /** @var clock $clock */ + private readonly clock $clock; + + /** + * Mock the clock + */ + protected function setUp(): void { + parent::setUp(); + $this->clock = $this->mock_clock_with_frozen(1622847600); + } /** * Test create schedule @@ -47,7 +59,8 @@ class schedule_test extends advanced_testcase { $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); - $timescheduled = time() + DAYSECS; + // Create schedule for tomorrow. + $timescheduled = $this->clock->time() + DAYSECS; $schedule = schedule::create_schedule((object) [ 'name' => 'My schedule', 'reportid' => $report->get('id'), @@ -81,11 +94,12 @@ class schedule_test extends advanced_testcase { // Update some record properties. $record = $schedule->to_record(); $record->name = 'My updated schedule'; - $record->timescheduled = 1861340400; // 25/12/2028 07:00 UTC. + $record->timescheduled += (12 * HOURSECS); $schedule = schedule::update_schedule($record); $this->assertEquals($record->name, $schedule->get('name')); $this->assertEquals($record->timescheduled, $schedule->get('timescheduled')); + $this->assertEquals($record->timescheduled, $schedule->get('timenextsend')); } /** @@ -294,100 +308,125 @@ class schedule_test extends advanced_testcase { * * @return array[] */ - public function should_send_schedule_provider(): array { - $time = time(); - - // We just need large offsets for dates in the past/future. - $yesterday = $time - DAYSECS; - $tomorrow = $time + DAYSECS; - + public static function should_send_schedule_provider(): array { return [ - 'Disabled' => [[ - 'enabled' => false, - ], false], - 'Time scheduled in the past' => [[ - 'recurrence' => model::RECURRENCE_NONE, - 'timescheduled' => $yesterday, - ], true], - 'Time scheduled in the past, already sent prior to schedule' => [[ - 'recurrence' => model::RECURRENCE_NONE, - 'timescheduled' => $yesterday, - 'timelastsent' => $yesterday - HOURSECS, - ], true], - 'Time scheduled in the past, already sent on schedule' => [[ - 'recurrence' => model::RECURRENCE_NONE, - 'timescheduled' => $yesterday, - 'timelastsent' => $yesterday, - ], false], - 'Time scheduled in the future' => [[ - 'recurrence' => model::RECURRENCE_NONE, - 'timescheduled' => $tomorrow, - ], false], - 'Time scheduled in the future, already sent prior to schedule' => [[ - 'recurrence' => model::RECURRENCE_NONE, - 'timelastsent' => $yesterday, - 'timescheduled' => $tomorrow, - ], false], - 'Next send in the past' => [[ - 'recurrence' => model::RECURRENCE_DAILY, - 'timescheduled' => $yesterday, - 'timenextsend' => $yesterday, - ], true], - 'Next send in the future' => [[ - 'recurrence' => model::RECURRENCE_DAILY, - 'timescheduled' => $yesterday, - 'timenextsend' => $tomorrow, - ], false], + 'Time scheduled in the past' => [ + model::RECURRENCE_NONE, '-1 hour', null, null, true, + ], + 'Time scheduled in the past, already sent prior to schedule' => [ + model::RECURRENCE_NONE, '-1 hour', '-2 hour', null, true, + ], + 'Time scheduled in the past, already sent on schedule' => [ + model::RECURRENCE_NONE, '-1 hour', '-1 hour', null, false, + ], + 'Time scheduled in the future' => [ + model::RECURRENCE_NONE, '+1 hour', null, null, false, + ], + 'Time scheduled in the future, already sent prior to schedule' => [ + model::RECURRENCE_NONE, '+1 hour', '-1 hour', null, false, + ], + 'Next send in the past' => [ + model::RECURRENCE_DAILY, '-1 hour', null, '-1 hour', true, + ], + 'Next send in the future' => [ + model::RECURRENCE_DAILY, '-1 hour', null, '+1 hour', false, + ], ]; } /** * Test for whether a schedule should be sent * - * @param array $properties + * @param int $recurrence + * @param string $timescheduled Relative time suitable for passing to {@see strtotime} + * @param string|null $timelastsent Relative time suitable for passing to {@see strtotime}, or null to ignore + * @param string|null $timenextsend Relative time suitable for passing to {@see strtotime}, or null to ignore * @param bool $expected * * @dataProvider should_send_schedule_provider */ - public function test_should_send_schedule(array $properties, bool $expected): void { + public function test_should_send_schedule( + int $recurrence, + string $timescheduled, + ?string $timelastsent, + ?string $timenextsend, + bool $expected, + ): void { $this->resetAfterTest(); /** @var core_reportbuilder_generator $generator */ $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); - $schedule = $generator->create_schedule(['reportid' => $report->get('id'), 'name' => 'My schedule'] + $properties); + // Use relative time period if present, otherwise default to zero (never sent). + $scheduletimelastsent = 0; + if ($timelastsent !== null) { + $scheduletimelastsent = strtotime($timelastsent, $this->clock->time()); + } + + $schedule = $generator->create_schedule([ + 'reportid' => $report->get('id'), + 'name' => 'My schedule', + 'recurrence' => $recurrence, + 'timescheduled' => strtotime($timescheduled, $this->clock->time()), + 'timelastsent' => $scheduletimelastsent, + ]); // If "Time next send" is specified, then override calculated value. - if (array_key_exists('timenextsend', $properties)) { - $schedule->set('timenextsend', $properties['timenextsend']); + if ($timenextsend !== null) { + $schedule->set('timenextsend', strtotime($timenextsend, $this->clock->time())); } $this->assertEquals($expected, schedule::should_send_schedule($schedule)); } + /** + * Test for whether a schedule should be sent that has been disabled + */ + public function test_should_send_schedule_disabled(): void { + $this->resetAfterTest(); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $schedule = $generator->create_schedule([ + 'reportid' => $report->get('id'), + 'name' => 'My schedule', + 'enabled' => 0, + ]); + + $this->assertFalse(schedule::should_send_schedule($schedule)); + } + /** * Data provider for {@see test_calculate_next_send_time} * * @return array[] */ - public function calculate_next_send_time_provider(): array { - $timescheduled = 1635865200; // Tue Nov 02 2021 15:00:00 GMT+0000. - $timenow = 1639846800; // Sat Dec 18 2021 17:00:00 GMT+0000. - + public static function calculate_next_send_time_provider(): array { + // Times are based on the current clock time (Fri Jun 04 2021 23:00:00 UTC). return [ - 'No recurrence' => [model::RECURRENCE_NONE, $timescheduled, $timenow, $timescheduled], - 'Recurrence, time scheduled in future' => [model::RECURRENCE_DAILY, $timenow + DAYSECS, $timenow, $timenow + DAYSECS], - // Sun Dec 19 2021 15:00:00 GMT+0000. - 'Daily recurrence' => [model::RECURRENCE_DAILY, $timescheduled, $timenow, 1639926000], - // Mon Dec 20 2021 15:00:00 GMT+0000. - 'Weekday recurrence' => [model::RECURRENCE_WEEKDAYS, $timescheduled, $timenow, 1640012400], - // Tue Dec 21 2021 15:00:00 GMT+0000. - 'Weekly recurrence' => [model::RECURRENCE_WEEKLY, $timescheduled, $timenow, 1640098800], - // Sun Jan 02 2022 15:00:00 GMT+0000. - 'Monthy recurrence' => [model::RECURRENCE_MONTHLY, $timescheduled, $timenow, 1641135600], - // Wed Nov 02 2022 15:00:00 GMT+0000. - 'Annual recurrence' => [model::RECURRENCE_ANNUALLY, $timescheduled, $timenow, 1667401200], + 'No recurrence' => [ + model::RECURRENCE_NONE, '2021-06-03 12:00', '2021-06-03 12:00', + ], + 'Recurrence, time scheduled in future' => [ + model::RECURRENCE_DAILY, '2021-06-05 12:00', '2021-06-05 12:00', + ], + 'Daily recurrence' => [ + model::RECURRENCE_DAILY, '2021-06-02 12:00', '2021-06-05 12:00', + ], + 'Weekday recurrence' => [ + model::RECURRENCE_WEEKDAYS, '2021-06-02 12:00', '2021-06-07 12:00', + ], + 'Weekly recurrence' => [ + model::RECURRENCE_WEEKLY, '2021-05-18 12:00', '2021-06-08 12:00', + ], + 'Monthy recurrence' => [ + model::RECURRENCE_MONTHLY, '2021-03-19 12:00', '2021-06-19 12:00', + ], + 'Annual recurrence' => [ + model::RECURRENCE_ANNUALLY, '2019-05-01 12:00', '2022-05-01 12:00', + ], ]; } @@ -395,27 +434,27 @@ class schedule_test extends advanced_testcase { * Test for calculating next schedule send time * * @param int $recurrence - * @param int $timescheduled - * @param int $timenow - * @param int $expected + * @param string $timescheduled Absolute time suitable for passing to {@see strtotime} + * @param string $expected Absolute time suitable for passing to {@see strtotime} * * @dataProvider calculate_next_send_time_provider */ - public function test_calculate_next_send_time(int $recurrence, int $timescheduled, int $timenow, int $expected): void { + public function test_calculate_next_send_time(int $recurrence, string $timescheduled, string $expected): void { $this->resetAfterTest(); /** @var core_reportbuilder_generator $generator */ $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); - $schedule = $generator->create_schedule([ + // Create model manually, as the generator automatically calculates next send itself. + $schedule = new model(0, (object) [ 'reportid' => $report->get('id'), 'name' => 'My schedule', 'recurrence' => $recurrence, - 'timescheduled' => $timescheduled, - 'timenow' => $timenow, + 'timescheduled' => strtotime("{$timescheduled} UTC"), ]); - $this->assertEquals($expected, schedule::calculate_next_send_time($schedule, $timenow)); + $scheduleexpected = strtotime("{$expected} UTC"); + $this->assertEquals($scheduleexpected, schedule::calculate_next_send_time($schedule)); } }