From 60dbaa0065e6b669c3a3ed1caca56602b2bed59a Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 14 Mar 2023 14:33:24 +0000 Subject: [PATCH] MDL-75576 quiz/question statistics: don't expire by time Previously, a set of calculated quiz statistics would only 'last' for 15 minutes. Then they would be considered invalid and not used. Now, computed statistics are kept indefinitely. Instead, when a new batch of values are computed for a particular set of settings, older numbers for the same settings are deleted first. Therefore, question_stats_cleanup_task is no more. --- lib/classes/task/question_stats_cleanup_task.php | 65 ---------------------- lib/db/tasks.php | 9 --- mod/quiz/report/statistics/classes/calculated.php | 6 +- mod/quiz/report/statistics/classes/calculator.php | 19 +++++-- mod/quiz/report/statistics/report.php | 9 ++- .../all_calculated_for_qubaid_condition.php | 32 ++++++++--- question/classes/statistics/responses/analyser.php | 37 +++++++----- .../responses/analysis_for_actual_response.php | 5 +- .../statistics/responses/analysis_for_class.php | 6 +- .../statistics/responses/analysis_for_question.php | 23 +++++++- .../statistics/responses/analysis_for_subpart.php | 5 +- question/upgrade.txt | 14 +++++ version.php | 2 +- 13 files changed, 115 insertions(+), 117 deletions(-) delete mode 100644 lib/classes/task/question_stats_cleanup_task.php diff --git a/lib/classes/task/question_stats_cleanup_task.php b/lib/classes/task/question_stats_cleanup_task.php deleted file mode 100644 index 651cc8dcdbc..00000000000 --- a/lib/classes/task/question_stats_cleanup_task.php +++ /dev/null @@ -1,65 +0,0 @@ -. - -/** - * Task to cleanup old question statistics cache. - * - * @package core - * @copyright 2019 Simey Lameze - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ -namespace core\task; - -defined('MOODLE_INTERNAL') || die(); - -/** - * A task to cleanup old question statistics cache. - * - * @copyright 2019 Simey Lameze - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ -class question_stats_cleanup_task extends scheduled_task { - - /** - * Get a descriptive name for this task (shown to admins). - * - * @return string - */ - public function get_name() { - return get_string('taskquestionstatscleanupcron', 'admin'); - } - - /** - * Perform the cleanup task. - */ - public function execute() { - global $DB; - - mtrace("\n Cleaning up old question statistics cache records...", ''); - - $expiretime = time() - 5 * HOURSECS; - $DB->delete_records_select('question_statistics', 'timemodified < ?', [$expiretime]); - $responseanlysisids = $DB->get_records_select_menu('question_response_analysis', - 'timemodified < ?', - [$expiretime], - 'id', - 'id, id AS id2'); - $DB->delete_records_list('question_response_analysis', 'id', $responseanlysisids); - $DB->delete_records_list('question_response_count', 'analysisid', $responseanlysisids); - - mtrace('done.'); - } -} diff --git a/lib/db/tasks.php b/lib/db/tasks.php index 311a95e6c22..ef69023e0e1 100644 --- a/lib/db/tasks.php +++ b/lib/db/tasks.php @@ -222,15 +222,6 @@ $tasks = array( 'month' => '*' ), array( - 'classname' => 'core\task\question_stats_cleanup_task', - 'blocking' => 0, - 'minute' => '*', - 'hour' => '*', - 'day' => '*', - 'dayofweek' => '*', - 'month' => '*' - ), - array( 'classname' => 'core\task\registration_cron_task', 'blocking' => 0, 'minute' => 'R', diff --git a/mod/quiz/report/statistics/classes/calculated.php b/mod/quiz/report/statistics/classes/calculated.php index efba5f4d9e6..b4fc6fe9f6e 100644 --- a/mod/quiz/report/statistics/classes/calculated.php +++ b/mod/quiz/report/statistics/classes/calculated.php @@ -225,9 +225,13 @@ class calculated { $toinsert->standarderror = null; } + // Delete older statistics before we save the new ones. + $transaction = $DB->start_delegated_transaction(); + $DB->delete_records('quiz_statistics', ['hashcode' => $qubaids->get_hash_code()]); + // Store the data. $DB->insert_record('quiz_statistics', $toinsert); - + $transaction->allow_commit(); } /** diff --git a/mod/quiz/report/statistics/classes/calculator.php b/mod/quiz/report/statistics/classes/calculator.php index bc5fe65af7d..976ed2f043c 100644 --- a/mod/quiz/report/statistics/classes/calculator.php +++ b/mod/quiz/report/statistics/classes/calculator.php @@ -122,7 +122,7 @@ class calculator { } /** - * @var int Time after which statistics are automatically recomputed. + * @var int previously, the time after which statistics are automatically recomputed. * @deprecated since Moodle 4.3. Use of pre-computed stats is no longer time-limited. * @todo MDL-78091 Final deprecation in Moodle 4.7 */ @@ -132,12 +132,17 @@ class calculator { * Load cached statistics from the database. * * @param \qubaid_condition $qubaids - * @return calculated The statistics for overall attempt scores or false if not cached. + * @return calculated|false The statistics for overall attempt scores or false if not cached. */ public function get_cached($qubaids) { global $DB; - $fromdb = $DB->get_record('quiz_statistics', ['hashcode' => $qubaids->get_hash_code()]); + $lastcalculatedtime = $this->get_last_calculated_time($qubaids); + if (!$lastcalculatedtime) { + return false; + } + $fromdb = $DB->get_record('quiz_statistics', ['hashcode' => $qubaids->get_hash_code(), + 'timemodified' => $lastcalculatedtime]); $stats = new calculated(); $stats->populate_from_record($fromdb); return $stats; @@ -151,7 +156,13 @@ class calculator { */ public function get_last_calculated_time($qubaids) { global $DB; - return $DB->get_field('quiz_statistics', 'timemodified', ['hashcode' => $qubaids->get_hash_code()]); + $lastcalculatedtime = $DB->get_field('quiz_statistics', 'COALESCE(MAX(timemodified), 0)', + ['hashcode' => $qubaids->get_hash_code()]); + if ($lastcalculatedtime) { + return $lastcalculatedtime; + } else { + return false; + } } /** diff --git a/mod/quiz/report/statistics/report.php b/mod/quiz/report/statistics/report.php index 90f06dff63f..4dba84328a5 100644 --- a/mod/quiz/report/statistics/report.php +++ b/mod/quiz/report/statistics/report.php @@ -25,6 +25,7 @@ defined('MOODLE_INTERNAL') || die(); +use core_question\statistics\responses\analyser; use mod_quiz\local\reports\report_base; use core_question\statistics\questions\all_calculated_for_qubaid_condition; @@ -420,7 +421,7 @@ class quiz_statistics_report extends report_base { } } - $responesanalyser = new \core_question\statistics\responses\analyser($question, $whichtries); + $responesanalyser = new analyser($question, $whichtries); $responseanalysis = $responesanalyser->load_cached($qubaids, $whichtries); $qtable->question_setup($reporturl, $question, $s, $responseanalysis); @@ -742,10 +743,8 @@ class quiz_statistics_report extends report_base { foreach ($questions as $question) { $progress->increment_progress(); if (question_bank::get_qtype($question->qtype, false)->can_analyse_responses() && !isset($done[$question->id])) { - $responesstats = new \core_question\statistics\responses\analyser($question, $whichtries); - if ($responesstats->get_last_analysed_time($qubaids, $whichtries) === false) { - $responesstats->calculate($qubaids, $whichtries); - } + $responesstats = new analyser($question, $whichtries); + $responesstats->calculate($qubaids, $whichtries); } $done[$question->id] = 1; } diff --git a/question/classes/statistics/questions/all_calculated_for_qubaid_condition.php b/question/classes/statistics/questions/all_calculated_for_qubaid_condition.php index 4fca96f8b79..9d993000a97 100644 --- a/question/classes/statistics/questions/all_calculated_for_qubaid_condition.php +++ b/question/classes/statistics/questions/all_calculated_for_qubaid_condition.php @@ -38,7 +38,11 @@ use question_bank; */ class all_calculated_for_qubaid_condition { - /** @var int Time after which statistics are automatically recomputed. */ + /** + * @var int previously, the time after which statistics are automatically recomputed. + * @deprecated since Moodle 4.3. Use of pre-computed stats is no longer time-limited. + * @todo MDL-78090 Final deprecation in Moodle 4.7 + */ const TIME_TO_CACHE = 900; // 15 minutes. /** @@ -197,9 +201,9 @@ class all_calculated_for_qubaid_condition { public function get_cached($qubaids) { global $DB; - $timemodified = time() - self::TIME_TO_CACHE; - $questionstatrecs = $DB->get_records_select('question_statistics', 'hashcode = ? AND timemodified > ?', - array($qubaids->get_hash_code(), $timemodified)); + $timemodified = self::get_last_calculated_time($qubaids); + $questionstatrecs = $DB->get_records('question_statistics', + ['hashcode' => $qubaids->get_hash_code(), 'timemodified' => $timemodified]); $questionids = array(); foreach ($questionstatrecs as $fromdb) { @@ -251,18 +255,26 @@ class all_calculated_for_qubaid_condition { */ public function get_last_calculated_time($qubaids) { global $DB; - - $timemodified = time() - self::TIME_TO_CACHE; - return $DB->get_field_select('question_statistics', 'timemodified', 'hashcode = ? AND timemodified > ?', - array($qubaids->get_hash_code(), $timemodified), IGNORE_MULTIPLE); + $lastcalculatedtime = $DB->get_field('question_statistics', 'COALESCE(MAX(timemodified), 0)', + ['hashcode' => $qubaids->get_hash_code()]); + if ($lastcalculatedtime) { + return $lastcalculatedtime; + } else { + return false; + } } /** - * Save stats to db. + * Save stats to db, first cleaning up any old ones. * * @param \qubaid_condition $qubaids Which question usages are we caching the stats of? */ public function cache($qubaids) { + global $DB; + + $transaction = $DB->start_delegated_transaction(); + $timemodified = time(); + foreach ($this->get_all_slots() as $slot) { $this->for_slot($slot)->cache($qubaids); } @@ -270,6 +282,8 @@ class all_calculated_for_qubaid_condition { foreach ($this->get_all_subq_ids() as $subqid) { $this->for_subq($subqid)->cache($qubaids); } + + $transaction->allow_commit(); } /** diff --git a/question/classes/statistics/responses/analyser.php b/question/classes/statistics/responses/analyser.php index 3203a0f1558..0f5bc91fdf2 100644 --- a/question/classes/statistics/responses/analyser.php +++ b/question/classes/statistics/responses/analyser.php @@ -41,7 +41,11 @@ class analyser { */ const MAX_TRY_COUNTED = 5; - /** @var int Time after which responses are automatically reanalysed. */ + /** + * @var int previously, the time after which statistics are automatically recomputed. + * @deprecated since Moodle 4.3. Use of pre-computed stats is no longer time-limited. + * @todo MDL-78090 Final deprecation in Moodle 4.7 + */ const TIME_TO_CACHE = 900; // 15 minutes. /** @var object full question data from db. */ @@ -53,6 +57,11 @@ class analyser { public $analysis; /** + * @var int used during calculations, so all results are stored with the same timestamp. + */ + protected $calculationtime; + + /** * @var array Two index array first index is unique string for each sub question part, the second string index is the 'class' * that sub-question part can be classified into. * @@ -109,7 +118,7 @@ class analyser { } /** - * Analyse all the response data for for all the specified attempts at this question. + * Analyse all the response data for all the specified attempts at this question. * * @param \qubaid_condition $qubaids which attempts to consider. * @param string $whichtries which tries to analyse. Will be one of @@ -117,6 +126,7 @@ class analyser { * @return analysis_for_question */ public function calculate($qubaids, $whichtries = \question_attempt::LAST_TRY) { + $this->calculationtime = time(); // Load data. $dm = new \question_engine_data_mapper(); $questionattempts = $dm->load_attempts_at_question($this->questiondata->id, $qubaids); @@ -131,7 +141,7 @@ class analyser { } } - $this->analysis->cache($qubaids, $whichtries, $this->questiondata->id); + $this->analysis->cache($qubaids, $whichtries, $this->questiondata->id, $this->calculationtime); return $this->analysis; } @@ -145,23 +155,23 @@ class analyser { public function load_cached($qubaids, $whichtries) { global $DB; - $timemodified = time() - self::TIME_TO_CACHE; + $timemodified = self::get_last_analysed_time($qubaids, $whichtries); // Variable name 'analyses' is the plural of 'analysis'. - $responseanalyses = $DB->get_records_select('question_response_analysis', - 'hashcode = ? AND whichtries = ? AND questionid = ? AND timemodified > ?', - array($qubaids->get_hash_code(), $whichtries, $this->questiondata->id, $timemodified)); + $responseanalyses = $DB->get_records('question_response_analysis', + ['hashcode' => $qubaids->get_hash_code(), 'whichtries' => $whichtries, + 'questionid' => $this->questiondata->id, 'timemodified' => $timemodified]); if (!$responseanalyses) { return false; } - $analysisids = array(); + $analysisids = []; foreach ($responseanalyses as $responseanalysis) { $analysisforsubpart = $this->analysis->get_analysis_for_subpart($responseanalysis->variant, $responseanalysis->subqid); $class = $analysisforsubpart->get_response_class($responseanalysis->aid); $class->add_response($responseanalysis->response, $responseanalysis->credit); $analysisids[] = $responseanalysis->id; } - list($sql, $params) = $DB->get_in_or_equal($analysisids); + [$sql, $params] = $DB->get_in_or_equal($analysisids); $counts = $DB->get_records_select('question_response_count', "analysisid {$sql}", $params); foreach ($counts as $count) { $responseanalysis = $responseanalyses[$count->analysisid]; @@ -183,11 +193,8 @@ class analyser { */ public function get_last_analysed_time($qubaids, $whichtries) { global $DB; - - $timemodified = time() - self::TIME_TO_CACHE; - return $DB->get_field_select('question_response_analysis', 'timemodified', - 'hashcode = ? AND whichtries = ? AND questionid = ? AND timemodified > ?', - array($qubaids->get_hash_code(), $whichtries, $this->questiondata->id, $timemodified), - IGNORE_MULTIPLE); + return $DB->get_field('question_response_analysis', 'MAX(timemodified)', + ['hashcode' => $qubaids->get_hash_code(), 'whichtries' => $whichtries, + 'questionid' => $this->questiondata->id]); } } diff --git a/question/classes/statistics/responses/analysis_for_actual_response.php b/question/classes/statistics/responses/analysis_for_actual_response.php index 8f4bc41c78b..fd8b2be6b3d 100644 --- a/question/classes/statistics/responses/analysis_for_actual_response.php +++ b/question/classes/statistics/responses/analysis_for_actual_response.php @@ -111,8 +111,9 @@ class analysis_for_actual_response { * @param int $variantno which variant. * @param string $subpartid which sub part is this actual response in? * @param string $responseclassid which response class is this actual response in? + * @param int|null $calculationtime time when the analysis was done. (Defaults to time()). */ - public function cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, $responseclassid) { + public function cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, $responseclassid, $calculationtime = null) { global $DB; $row = new \stdClass(); $row->hashcode = $qubaids->get_hash_code(); @@ -127,7 +128,7 @@ class analysis_for_actual_response { } $row->response = $this->response; $row->credit = $this->fraction; - $row->timemodified = time(); + $row->timemodified = $calculationtime ?? time(); $analysisid = $DB->insert_record('question_response_analysis', $row); if ($whichtries === \question_attempt::ALL_TRIES) { foreach ($this->trycount as $try => $count) { diff --git a/question/classes/statistics/responses/analysis_for_class.php b/question/classes/statistics/responses/analysis_for_class.php index efb7d7b5438..c658c3cdf80 100644 --- a/question/classes/statistics/responses/analysis_for_class.php +++ b/question/classes/statistics/responses/analysis_for_class.php @@ -103,11 +103,13 @@ class analysis_for_class { * @param int $questionid which question. * @param int $variantno which variant. * @param string $subpartid which sub part. + * @param int|null $calculationtime time when the analysis was done. (Defaults to time()). */ - public function cache($qubaids, $whichtries, $questionid, $variantno, $subpartid) { + public function cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, $calculationtime = null) { foreach ($this->get_responses() as $response) { $analysisforactualresponse = $this->get_response($response); - $analysisforactualresponse->cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, $this->responseclassid); + $analysisforactualresponse->cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, + $this->responseclassid, $calculationtime); } } diff --git a/question/classes/statistics/responses/analysis_for_question.php b/question/classes/statistics/responses/analysis_for_question.php index 3e06f3cf171..4e1fd670332 100644 --- a/question/classes/statistics/responses/analysis_for_question.php +++ b/question/classes/statistics/responses/analysis_for_question.php @@ -196,17 +196,36 @@ class analysis_for_question { } /** + * Save the analysis to the DB, first cleaning up any old ones. + * * @param \qubaid_condition $qubaids which question usages have been analysed. * @param string $whichtries which tries have been analysed? * @param int $questionid which question. + * @param int|null $calculationtime time when the analysis was done. (Defaults to time()). */ - public function cache($qubaids, $whichtries, $questionid) { + public function cache($qubaids, $whichtries, $questionid, $calculationtime = null) { + global $DB; + + $transaction = $DB->start_delegated_transaction(); + + $DB->delete_records_select('question_response_count', + 'analysisid IN ( + SELECT id + FROM {question_response_analysis} + WHERE hashcode= ? AND whichtries = ? AND questionid = ? + )', [$qubaids->get_hash_code(), $whichtries, $questionid]); + + $DB->delete_records('question_response_analysis', + ['hashcode' => $qubaids->get_hash_code(), 'whichtries' => $whichtries, 'questionid' => $questionid]); + foreach ($this->get_variant_nos() as $variantno) { foreach ($this->get_subpart_ids($variantno) as $subpartid) { $analysisforsubpart = $this->get_analysis_for_subpart($variantno, $subpartid); - $analysisforsubpart->cache($qubaids, $whichtries, $questionid, $variantno, $subpartid); + $analysisforsubpart->cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, $calculationtime); } } + + $transaction->allow_commit(); } /** diff --git a/question/classes/statistics/responses/analysis_for_subpart.php b/question/classes/statistics/responses/analysis_for_subpart.php index cbb7352485a..6ead7edb1db 100644 --- a/question/classes/statistics/responses/analysis_for_subpart.php +++ b/question/classes/statistics/responses/analysis_for_subpart.php @@ -119,11 +119,12 @@ class analysis_for_subpart { * @param int $questionid which question. * @param int $variantno which variant. * @param string $subpartid which sub part. + * @param int|null $calculationtime time when the analysis was done. (Defaults to time()). */ - public function cache($qubaids, $whichtries, $questionid, $variantno, $subpartid) { + public function cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, $calculationtime = null) { foreach ($this->get_response_class_ids() as $responseclassid) { $analysisforclass = $this->get_response_class($responseclassid); - $analysisforclass->cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, $responseclassid); + $analysisforclass->cache($qubaids, $whichtries, $questionid, $variantno, $subpartid, $calculationtime); } } diff --git a/question/upgrade.txt b/question/upgrade.txt index 3f2b533e51e..6ef594f8eda 100644 --- a/question/upgrade.txt +++ b/question/upgrade.txt @@ -10,6 +10,19 @@ This files describes API changes for code that uses the question API. $question = $questiongenerator->update_question($question, ...); Also, the $question object returned now has fields questionbankentryid, versionid, version and status. +2) question_stats_cleanup_task has been removed. It is no longer required. Instead, + older statistics are deleted whenever a new set are calculated for a particular quiz. + +3) In the past, the methods get_last_calculated_time() and get_cached() of \core_question\statistics\responses\analyser + and \core_question\statistics\questions\all_calculated_for_qubaid_condition + only returned the pre-computed statistics if they were computed less than 15 minutes ago. Now, they will + always return any computed statistics that exist. Therefore, the constants TIME_TO_CACHE in those classes + have been deprecated. + +4) The cache() methods of classes analysis_for_question, analysis_for_subpart, analysis_for_class + and analysis_for_actual_response now take an optional $calculationtime, which is used the time + stored in the database. If not given, time() is used. + === 4.2 === @@ -38,6 +51,7 @@ This files describes API changes for code that uses the question API. 3) display_move_form() in qbank_managecategories\question_category_object class is deprecated and moved the logic to the question/bank/managecategories/category.php. + === 4.1 === 1) get_bulk_action_key() in core_question\local\bank\bulk_action_base class is deprecated and renamed to get_key(). diff --git a/version.php b/version.php index a3c4f51c3a5..4f41556d29b 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2023051200.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2023051200.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '4.3dev (Build: 20230512)'; // Human-friendly version name -- 2.11.4.GIT