From e5a7a18ae276f3c607a7a853409e53572a60e9b8 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Thu, 14 Sep 2023 10:13:03 +0100 Subject: [PATCH] MDL-79254 quiz_statistics: Convert recalculate task to ad-hoc Previously, quiz statistics processing happened on a scheduled task. This task looked for all quizzes with completed attempts, then determined if those quizzes had a statistics calculation that's newer than the most recent attempt, then ran the statistics calculation if needed. It was hard coded to stop processing after 1 hour. The queries involved in determining which quizzes needed processing weren't terribly efficient, and combined with the 1 hour limit this made the statistics unusable on large sites, where they are the most useful. This converts the scheduled task to an ad-hoc task, and uses an event observer for mod_quiz\event\attempt_submitted to queue a task when it is needed. This removes the need for a query to work out what needs processing, and allows the task processing to be scaled up as needed. --- .../event/observer/attempt_submitted.php | 47 +++++ .../statistics/classes/task/recalculate.php | 120 +++++-------- .../classes/tests/statistics_helper.php | 46 +++++ .../statistics/db/{tasks.php => events.php} | 24 +-- .../event/observer/attempt_submitted_test.php | 165 ++++++++++++++++++ mod/quiz/report/statistics/version.php | 2 +- .../tests/behat/behat_qbank_statistics.php | 41 +++++ .../tests/behat/statistics_values.feature | 10 +- .../bank/statistics/tests/helper_test.php | 4 +- .../tests/datalib_reporting_queries_test.php | 4 +- .../statistics_bulk_loader_test.php | 4 +- 11 files changed, 367 insertions(+), 100 deletions(-) create mode 100644 mod/quiz/report/statistics/classes/event/observer/attempt_submitted.php create mode 100644 mod/quiz/report/statistics/classes/tests/statistics_helper.php rename mod/quiz/report/statistics/db/{tasks.php => events.php} (60%) create mode 100644 mod/quiz/report/statistics/tests/event/observer/attempt_submitted_test.php create mode 100644 question/bank/statistics/tests/behat/behat_qbank_statistics.php diff --git a/mod/quiz/report/statistics/classes/event/observer/attempt_submitted.php b/mod/quiz/report/statistics/classes/event/observer/attempt_submitted.php new file mode 100644 index 0000000000000..acafe5983779b --- /dev/null +++ b/mod/quiz/report/statistics/classes/event/observer/attempt_submitted.php @@ -0,0 +1,47 @@ +. + +namespace quiz_statistics\event\observer; + +use quiz_statistics\task\recalculate; + +/** + * Event observer for \mod_quiz\event\attempt_submitted + * + * @package quiz_statistics + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class attempt_submitted { + /** + * Queue an ad-hoc task to recalculate statistics for the quiz. + * + * This will defer running the task for 1 hour, to give other attempts in progress + * a chance to submit. + * + * @param \mod_quiz\event\attempt_submitted $event + * @return void + */ + public static function process(\mod_quiz\event\attempt_submitted $event): void { + $data = $event->get_data(); + $quizid = $data['other']['quizid']; + + $task = recalculate::instance($quizid); + $task->set_next_run_time(time() + HOURSECS); + \core\task\manager::queue_adhoc_task($task, true); + } +} diff --git a/mod/quiz/report/statistics/classes/task/recalculate.php b/mod/quiz/report/statistics/classes/task/recalculate.php index 76c0ceaf827f2..8b9dce2f18a20 100644 --- a/mod/quiz/report/statistics/classes/task/recalculate.php +++ b/mod/quiz/report/statistics/classes/task/recalculate.php @@ -18,7 +18,6 @@ use core\dml\sql_join; use mod_quiz\quiz_attempt; -use mod_quiz\quiz_settings; use quiz_statistics_report; defined('MOODLE_INTERNAL') || die(); @@ -36,9 +35,25 @@ * @author Nathan Nguyen * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class recalculate extends \core\task\scheduled_task { - /** @var int the maximum length of time one instance of this task will run. */ - const TIME_LIMIT = 3600; +class recalculate extends \core\task\adhoc_task { + /** + * Create a new instance of the task. + * + * This sets the properties so that only one task will be queued at a time for a given quiz. + * + * @param int $quizid + * @return recalculate + */ + public static function instance(int $quizid): recalculate { + $task = new self(); + $task->set_component('quiz_statistics'); + $task->set_userid(get_admin()->id); + $task->set_custom_data((object)[ + 'quizid' => $quizid, + ]); + return $task; + } + public function get_name(): string { return get_string('recalculatetask', 'quiz_statistics'); @@ -46,78 +61,37 @@ public function get_name(): string { public function execute(): void { global $DB; - $stoptime = time() + self::TIME_LIMIT; $dateformat = get_string('strftimedatetimeshortaccurate', 'core_langconfig'); + $data = $this->get_custom_data(); + $quiz = $DB->get_record('quiz', ['id' => $data->quizid]); + if (!$quiz) { + mtrace('Could not find quiz with ID ' . $data->quizid . '.'); + return; + } + $course = $DB->get_record('course', ['id' => $quiz->course]); + if (!$course) { + mtrace('Could not find course with ID ' . $quiz->course . '.'); + return; + } + $attemptcount = $DB->count_records('quiz_attempts', ['quiz' => $data->quizid, 'state' => quiz_attempt::FINISHED]); + if ($attemptcount === 0) { + mtrace('Could not find any finished attempts for course with ID ' . $data->quizid . '.'); + return; + } - // TODO: MDL-75197, add quizid in quiz_statistics so that it is simpler to find quizzes for stats calculation. - // Only calculate stats for quizzes which have recently finished attempt. - $latestattempts = $DB->get_records_sql(" - SELECT q.id AS quizid, - q.name AS quizname, - q.grademethod AS quizgrademethod, - c.id AS courseid, - c.shortname AS courseshortname, - MAX(quiza.timefinish) AS mostrecentattempttime, - COUNT(1) AS numberofattempts - - FROM {quiz_attempts} quiza - JOIN {quiz} q ON q.id = quiza.quiz - JOIN {course} c ON c.id = q.course - - WHERE quiza.preview = 0 - AND quiza.state = :quizstatefinished - - GROUP BY q.id, q.name, q.grademethod, c.id, c.shortname - ORDER BY MAX(quiza.timefinish) DESC - ", ["quizstatefinished" => quiz_attempt::FINISHED]); - - $anyexception = null; - foreach ($latestattempts as $latestattempt) { - if (time() >= $stoptime) { - mtrace("This task has been running for more than " . - format_time(self::TIME_LIMIT) . " so stopping this execution."); - break; - } - - // Check if there is any existing question stats, and it has been calculated after latest quiz attempt. - $qubaids = quiz_statistics_qubaids_condition($latestattempt->quizid, - new sql_join(), $latestattempt->quizgrademethod); - $lateststatstime = $DB->get_field('quiz_statistics', 'COALESCE(MAX(timemodified), 0)', - ['hashcode' => $qubaids->get_hash_code()]); - - $quizinfo = "'$latestattempt->quizname' ($latestattempt->quizid) in course " . - "$latestattempt->courseshortname ($latestattempt->courseid) has most recent attempt finished at " . - userdate($latestattempt->mostrecentattempttime, $dateformat); - if ($lateststatstime) { - $quizinfo .= " and statistics from " . userdate($lateststatstime, $dateformat); - } - - if ($lateststatstime >= $latestattempt->mostrecentattempttime) { - mtrace(" " . $quizinfo . " so nothing to do."); - continue; - } - - // OK, so we need to calculate for this quiz. - mtrace(" " . $quizinfo . " so re-calculating statistics for $latestattempt->numberofattempts attempts, start time " . - userdate(time(), $dateformat) . " ..."); - - try { - $quizobj = quiz_settings::create($latestattempt->quizid); - $report = new quiz_statistics_report(); - $report->clear_cached_data($qubaids); - $report->calculate_questions_stats_for_question_bank($quizobj->get_quizid()); - mtrace(" Calculations completed at " . userdate(time(), $dateformat) . "."); + mtrace("Re-calculating statistics for quiz {$quiz->name} ({$quiz->id}) " . + "from course {$course->shortname} ({$course->id}) with {$attemptcount} attempts, start time " . + userdate(time(), $dateformat) . " ..."); - } catch (\Throwable $e) { - // We don't want an exception from one quiz to stop processing of other quizzes. - mtrace_exception($e); - $anyexception = $e; - } - } + $qubaids = quiz_statistics_qubaids_condition( + $quiz->id, + new sql_join(), + $quiz->grademethod + ); - if ($anyexception) { - // If there was any error, ensure the task fails. - throw $anyexception; - } + $report = new quiz_statistics_report(); + $report->clear_cached_data($qubaids); + $report->calculate_questions_stats_for_question_bank($quiz->id); + mtrace(' Calculations completed at ' . userdate(time(), $dateformat) . '.'); } } diff --git a/mod/quiz/report/statistics/classes/tests/statistics_helper.php b/mod/quiz/report/statistics/classes/tests/statistics_helper.php new file mode 100644 index 0000000000000..23b7873664b7d --- /dev/null +++ b/mod/quiz/report/statistics/classes/tests/statistics_helper.php @@ -0,0 +1,46 @@ +. +namespace quiz_statistics\tests; + +/** + * Test helper functions for statistics + * + * @package quiz_statistics + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class statistics_helper { + /** + * Run any ad-hoc recalculation tasks that have been scheduled. + * + * We need a special function to do this as the tasks are deferred by one hour, + * so we need to pass a custom $timestart argument. + * + * @return void + */ + public static function run_pending_recalculation_tasks(): void { + while ($task = \core\task\manager::get_next_adhoc_task( + time() + HOURSECS + 1, + false, + '\quiz_statistics\task\recalculate' + )) { + $task->execute(); + \core\task\manager::adhoc_task_complete($task); + } + } + +} diff --git a/mod/quiz/report/statistics/db/tasks.php b/mod/quiz/report/statistics/db/events.php similarity index 60% rename from mod/quiz/report/statistics/db/tasks.php rename to mod/quiz/report/statistics/db/events.php index 44c863318316e..9028cdfb1cb7c 100644 --- a/mod/quiz/report/statistics/db/tasks.php +++ b/mod/quiz/report/statistics/db/events.php @@ -15,25 +15,19 @@ // along with Moodle. If not, see . /** - * Legacy Cron Quiz Reports Task - * - * @package quiz_statistics - * @copyright 2017 Michael Hughes, University of Strathclyde - * @author Michael Hughes - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * Add event observers for quiz_statistics * + * @package quiz_statistics + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ defined('MOODLE_INTERNAL') || die(); -$tasks = [ +$observers = [ [ - 'classname' => 'quiz_statistics\task\recalculate', - 'blocking' => 0, - 'minute' => 'R', - 'hour' => '*/4', - 'day' => '*', - 'dayofweek' => '*', - 'month' => '*' - ] + 'eventname' => '\mod_quiz\event\attempt_submitted', + 'callback' => '\quiz_statistics\event\observer\attempt_submitted::process', + ], ]; diff --git a/mod/quiz/report/statistics/tests/event/observer/attempt_submitted_test.php b/mod/quiz/report/statistics/tests/event/observer/attempt_submitted_test.php new file mode 100644 index 0000000000000..3d119f8fea6e9 --- /dev/null +++ b/mod/quiz/report/statistics/tests/event/observer/attempt_submitted_test.php @@ -0,0 +1,165 @@ +. +namespace quiz_statistics\event\observer; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/mod/quiz/tests/quiz_question_helper_test_trait.php'); + +use core\task\manager; +use quiz_statistics\task\recalculate; +use quiz_statistics\tests\statistics_helper; + +/** + * Unit tests for attempt_submitted observer + * + * @package quiz_statistics + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \quiz_statistics\event\observer\attempt_submitted + */ +class attempt_submitted_test extends \advanced_testcase { + use \quiz_question_helper_test_trait; + + /** + * Return a user, and a quiz with 2 questions. + * + * @return array [$user, $quiz, $course] + */ + protected function create_test_data(): array { + $this->resetAfterTest(true); + $generator = $this->getDataGenerator(); + $user = $generator->create_user(); + $course = $generator->create_course(); + $quiz = $this->create_test_quiz($course); + $this->add_two_regular_questions($generator->get_plugin_generator('core_question'), $quiz); + return [$user, $quiz, $course]; + } + + /** + * Assert that a task is queued for a quiz. + * + * Check that the quizid stored in the task's custom data matches the provided quiz, + * and that the run time is in one hour from when the test is being run (within a small margin of error). + * + * @param recalculate $task + * @param \stdClass $quiz + * @return void + */ + protected function assert_task_is_queued_for_quiz(recalculate $task, \stdClass $quiz): void { + $data = $task->get_custom_data(); + $this->assertEquals($quiz->id, $data->quizid); + $this->assertEqualsWithDelta(time() + HOURSECS, $task->get_next_run_time(), 1); + } + + /** + * Attempting a quiz should queue the recalculation task for that quiz in 1 hour's time. + * + * @return void + */ + public function test_queue_task_on_submission(): void { + [$user, $quiz] = $this->create_test_data(); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertEmpty($tasks); + + $this->attempt_quiz($quiz, $user); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertCount(1, $tasks); + $task = reset($tasks); + $this->assert_task_is_queued_for_quiz($task, $quiz); + } + + /** + * Attempting a quiz multiple times should only queue one instance of the task. + * + * @return void + */ + public function test_queue_single_task_for_multiple_submissions(): void { + [$user1, $quiz] = $this->create_test_data(); + $user2 = $this->getDataGenerator()->create_user(); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertEmpty($tasks); + + $this->attempt_quiz($quiz, $user1); + $this->attempt_quiz($quiz, $user2); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertCount(1, $tasks); + $task = reset($tasks); + $this->assert_task_is_queued_for_quiz($task, $quiz); + } + + /** + * Attempting the quiz again after processing the task should queue a new task. + * + * @return void + */ + public function test_queue_new_task_after_processing(): void { + [$user1, $quiz, $course] = $this->create_test_data(); + $user2 = $this->getDataGenerator()->create_user(); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertEmpty($tasks); + + $this->attempt_quiz($quiz, $user1); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertCount(1, $tasks); + + $this->expectOutputRegex("~Re-calculating statistics for quiz {$quiz->name} \({$quiz->id}\) " . + "from course {$course->shortname} \({$course->id}\) with 1 attempts~"); + statistics_helper::run_pending_recalculation_tasks(); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertEmpty($tasks); + + $this->attempt_quiz($quiz, $user2); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertCount(1, $tasks); + + $task = reset($tasks); + $this->assert_task_is_queued_for_quiz($task, $quiz); + } + + /** + * Attempting different quizzes will queue a task for each. + * + * @return void + */ + public function test_queue_separate_tasks_for_multiple_quizzes(): void { + [$user1, $quiz1] = $this->create_test_data(); + [$user2, $quiz2] = $this->create_test_data(); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertEmpty($tasks); + + $this->attempt_quiz($quiz1, $user1); + $this->attempt_quiz($quiz2, $user2); + + $tasks = manager::get_adhoc_tasks(recalculate::class); + $this->assertCount(2, $tasks); + $task1 = array_shift($tasks); + $this->assert_task_is_queued_for_quiz($task1, $quiz1); + $task2 = array_shift($tasks); + $this->assert_task_is_queued_for_quiz($task2, $quiz2); + } +} diff --git a/mod/quiz/report/statistics/version.php b/mod/quiz/report/statistics/version.php index 0daf9a28610b3..6b369801c9f5e 100644 --- a/mod/quiz/report/statistics/version.php +++ b/mod/quiz/report/statistics/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2023042400; +$plugin->version = 2023042403; $plugin->requires = 2023041800; $plugin->component = 'quiz_statistics'; diff --git a/question/bank/statistics/tests/behat/behat_qbank_statistics.php b/question/bank/statistics/tests/behat/behat_qbank_statistics.php new file mode 100644 index 0000000000000..5c3bc9a34b5d6 --- /dev/null +++ b/question/bank/statistics/tests/behat/behat_qbank_statistics.php @@ -0,0 +1,41 @@ +. + +require_once(__DIR__ . '/../../../../../lib/behat/behat_base.php'); + +/** + * Behat steps for qbank_statistics + * + * @package qbank_statistics + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class behat_qbank_statistics extends behat_base { + + /** + * Run pending recalculation tasks. + * + * This runs the recalcuation ad-hoc tasks. We need a special step for this + * as the run time for these tasks is set to an hour in the future, so + * "I run all ad-hoc tasks" will not trigger them. + * + * @Given /^I run pending statistics recalculation tasks$/ + */ + function i_run_pending_statistics_recalculation_tasks() { + \quiz_statistics\tests\statistics_helper::run_pending_recalculation_tasks(); + } +} \ No newline at end of file diff --git a/question/bank/statistics/tests/behat/statistics_values.feature b/question/bank/statistics/tests/behat/statistics_values.feature index d14f3cd370e94..b2c0126869ce6 100644 --- a/question/bank/statistics/tests/behat/statistics_values.feature +++ b/question/bank/statistics/tests/behat/statistics_values.feature @@ -73,7 +73,7 @@ Feature: Show statistics in question bank | slot | response | | 1 | True | | 2 | True | - And I run the scheduled task "\quiz_statistics\task\recalculate" + And I run pending statistics recalculation tasks When I am on the "Course 1" "core_question > course question bank" page logged in as "admin" Then I should see "50.00%" in the "TF1" "table_row" And I should see "75.00%" in the "TF2" "table_row" @@ -89,7 +89,7 @@ Feature: Show statistics in question bank | slot | response | | 1 | True | | 2 | True | - And I run the scheduled task "\quiz_statistics\task\recalculate" + And I run pending statistics recalculation tasks When I am on the "Course 1" "core_question > course question bank" page logged in as "admin" Then I should see "50.00%" in the "TF1" "table_row" And I should see "75.00%" in the "TF2" "table_row" @@ -105,7 +105,7 @@ Feature: Show statistics in question bank | slot | response | | 1 | True | | 2 | True | - And I run the scheduled task "\quiz_statistics\task\recalculate" + And I run pending statistics recalculation tasks When I am on the "Course 1" "core_question > course question bank" page logged in as "admin" Then I should see "Likely" in the "TF1" "table_row" And I should see "Unlikely" in the "TF2" "table_row" @@ -127,7 +127,7 @@ Feature: Show statistics in question bank | slot | response | | 1 | True | | 2 | False | - And I run the scheduled task "\quiz_statistics\task\recalculate" + And I run pending statistics recalculation tasks When I am on the "Course 1" "core_question > course question bank" page logged in as "admin" Then I should see "Likely" in the "TF1" "table_row" And I should see "Very likely" in the "TF2" "table_row" @@ -207,7 +207,7 @@ Feature: Show statistics in question bank | 3 | Two | | 4 | Two | | 5 | One | - And I run the scheduled task "\quiz_statistics\task\recalculate" + And I run pending statistics recalculation tasks # Confirm the "Needs checking?" column matches the expected values based on students' answers When I am on the "Quiz 3" "mod_quiz > question bank" page logged in as "admin" Then the following should exist in the "categoryquestions" table: diff --git a/question/bank/statistics/tests/helper_test.php b/question/bank/statistics/tests/helper_test.php index 8b77b3f2cb758..92e3c2d44b5da 100644 --- a/question/bank/statistics/tests/helper_test.php +++ b/question/bank/statistics/tests/helper_test.php @@ -19,6 +19,7 @@ defined('MOODLE_INTERNAL') || die(); use core_question\statistics\questions\all_calculated_for_qubaid_condition; +use quiz_statistics\tests\statistics_helper; use mod_quiz\quiz_attempt; use mod_quiz\quiz_settings; use question_engine; @@ -235,8 +236,7 @@ private function prepare_and_submit_quizzes(array $quiz1attempts, array $quiz2at // Calculate the statistics. $this->expectOutputRegex('~.*Calculations completed.*~'); - $statisticstask = new \quiz_statistics\task\recalculate(); - $statisticstask->execute(); + statistics_helper::run_pending_recalculation_tasks(); return [$quiz1, $quiz2, $questions]; } diff --git a/question/engine/tests/datalib_reporting_queries_test.php b/question/engine/tests/datalib_reporting_queries_test.php index 6f9612999311c..2450b685ee439 100644 --- a/question/engine/tests/datalib_reporting_queries_test.php +++ b/question/engine/tests/datalib_reporting_queries_test.php @@ -16,6 +16,7 @@ namespace core_question; +use quiz_statistics\tests\statistics_helper; use mod_quiz\quiz_attempt; use mod_quiz\quiz_settings; use qubaid_list; @@ -376,7 +377,6 @@ public function test_quiz_with_description_questions_recalculate_statistics(): v // Calculate the statistics. $this->expectOutputRegex('~.*Calculations completed.*~'); - $statisticstask = new \quiz_statistics\task\recalculate(); - $statisticstask->execute(); + statistics_helper::run_pending_recalculation_tasks(); } } diff --git a/question/tests/local/statistics/statistics_bulk_loader_test.php b/question/tests/local/statistics/statistics_bulk_loader_test.php index 14c14321dba18..8fd078bc44304 100644 --- a/question/tests/local/statistics/statistics_bulk_loader_test.php +++ b/question/tests/local/statistics/statistics_bulk_loader_test.php @@ -22,6 +22,7 @@ use context; use context_module; use core_question\statistics\questions\all_calculated_for_qubaid_condition; +use quiz_statistics\tests\statistics_helper; use core_question_generator; use Generator; use mod_quiz\quiz_attempt; @@ -256,8 +257,7 @@ private function prepare_and_submit_quizzes(array $quiz1attempts, array $quiz2at // Calculate the statistics. $this->expectOutputRegex('~.*Calculations completed.*~'); - $statisticstask = new \quiz_statistics\task\recalculate(); - $statisticstask->execute(); + statistics_helper::run_pending_recalculation_tasks(); return [$quiz1, $quiz2, $questions]; }