From 9deabf83442ad0898c137fbc1b1793ba86d82f8e Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 30 Jan 2023 12:31:07 +0000 Subject: [PATCH 1/5] MDL-75576 qbank_statistics: remove copy-pasted PHPdoc comments These methods are already correctly documented on the base class. --- .../classes/columns/discrimination_index.php | 16 +--------------- .../columns/discriminative_efficiency.php | 16 +--------------- .../classes/columns/facility_index.php | 16 +--------------- 3 files changed, 3 insertions(+), 45 deletions(-) diff --git a/question/bank/statistics/classes/columns/discrimination_index.php b/question/bank/statistics/classes/columns/discrimination_index.php index 8046127c821..6f565798abc 100644 --- a/question/bank/statistics/classes/columns/discrimination_index.php +++ b/question/bank/statistics/classes/columns/discrimination_index.php @@ -18,6 +18,7 @@ namespace qbank_statistics\columns; use core_question\local\bank\column_base; use qbank_statistics\helper; + /** * This columns shows a message about whether this question is OK or needs revision. * @@ -30,11 +31,6 @@ use qbank_statistics\helper; */ class discrimination_index extends column_base { - /** - * Title for this column. - * - * @return string column title - */ public function get_title(): string { return get_string('discrimination_index', 'qbank_statistics'); } @@ -43,20 +39,10 @@ class discrimination_index extends column_base { return new \help_icon('discrimination_index', 'qbank_statistics'); } - /** - * Column name. - * - * @return string column name - */ public function get_name(): string { return 'discrimination_index'; } - /** - * Output the contents of this column. - * @param object $question the row from the $question table, augmented with extra information. - * @param string $rowclasses CSS class names that should be applied to this row of output. - */ protected function display_content($question, $rowclasses) { global $PAGE; // Average discrimination index per quiz. diff --git a/question/bank/statistics/classes/columns/discriminative_efficiency.php b/question/bank/statistics/classes/columns/discriminative_efficiency.php index 3d5a8632976..05e93fdda21 100644 --- a/question/bank/statistics/classes/columns/discriminative_efficiency.php +++ b/question/bank/statistics/classes/columns/discriminative_efficiency.php @@ -18,6 +18,7 @@ namespace qbank_statistics\columns; use core_question\local\bank\column_base; use qbank_statistics\helper; + /** * This column show the average discriminative efficiency for this question. * @@ -28,11 +29,6 @@ use qbank_statistics\helper; */ class discriminative_efficiency extends column_base { - /** - * Title for this column. - * - * @return string column title - */ public function get_title(): string { return get_string('discriminative_efficiency', 'qbank_statistics'); } @@ -41,20 +37,10 @@ class discriminative_efficiency extends column_base { return new \help_icon('discriminative_efficiency', 'qbank_statistics'); } - /** - * Column name. - * - * @return string column name - */ public function get_name(): string { return 'discriminative_efficiency'; } - /** - * Output the contents of this column. - * @param object $question the row from the $question table, augmented with extra information. - * @param string $rowclasses CSS class names that should be applied to this row of output. - */ protected function display_content($question, $rowclasses) { global $PAGE; // Average discriminative efficiency per quiz. diff --git a/question/bank/statistics/classes/columns/facility_index.php b/question/bank/statistics/classes/columns/facility_index.php index d70fd5ea29b..98d050b4493 100644 --- a/question/bank/statistics/classes/columns/facility_index.php +++ b/question/bank/statistics/classes/columns/facility_index.php @@ -18,6 +18,7 @@ namespace qbank_statistics\columns; use core_question\local\bank\column_base; use qbank_statistics\helper; + /** * This column show the average facility index for this question. * @@ -28,11 +29,6 @@ use qbank_statistics\helper; */ class facility_index extends column_base { - /** - * Title for this column. - * - * @return string column title - */ public function get_title(): string { return get_string('facility_index', 'qbank_statistics'); } @@ -41,20 +37,10 @@ class facility_index extends column_base { return new \help_icon('facility_index', 'qbank_statistics'); } - /** - * Column name. - * - * @return string column name - */ public function get_name(): string { return 'facility_index'; } - /** - * Output the contents of this column. - * @param object $question the row from the $question table, augmented with extra information. - * @param string $rowclasses CSS class names that should be applied to this row of output. - */ protected function display_content($question, $rowclasses) { global $PAGE; // Average facility index per quiz. From a187d7930d4d4bf34b1cefde576a8e7b3f30eef5 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 30 Jan 2023 15:42:27 +0000 Subject: [PATCH 2/5] MDL-75576 question bank: bulk-load statistics for all questions at once There is a new method on the column class, for the column to declare any statistics it requires. The question then efficiently loads all those statistics in one go, and makes them available as the columns are rendererd, or do any subsequent processing. --- .../classes/columns/discrimination_index.php | 9 +- .../columns/discriminative_efficiency.php | 9 +- .../classes/columns/facility_index.php | 9 +- question/bank/statistics/classes/helper.php | 2 +- question/bank/upgrade.txt | 10 + question/classes/local/bank/column_base.php | 16 +- question/classes/local/bank/view.php | 85 ++- .../statistics/statistics_bulk_loader.php | 170 ++++++ .../statistics_bulk_loader_test.php | 550 ++++++++++++++++++ 9 files changed, 841 insertions(+), 19 deletions(-) create mode 100644 question/classes/local/statistics/statistics_bulk_loader.php create mode 100644 question/tests/local/statistics/statistics_bulk_loader_test.php diff --git a/question/bank/statistics/classes/columns/discrimination_index.php b/question/bank/statistics/classes/columns/discrimination_index.php index 6f565798abc..054e53851e5 100644 --- a/question/bank/statistics/classes/columns/discrimination_index.php +++ b/question/bank/statistics/classes/columns/discrimination_index.php @@ -17,7 +17,6 @@ namespace qbank_statistics\columns; use core_question\local\bank\column_base; -use qbank_statistics\helper; /** * This columns shows a message about whether this question is OK or needs revision. @@ -43,10 +42,14 @@ class discrimination_index extends column_base { return 'discrimination_index'; } + public function get_required_statistics_fields(): array { + return ['discriminationindex']; + } + protected function display_content($question, $rowclasses) { global $PAGE; - // Average discrimination index per quiz. - $discriminationindex = helper::calculate_average_question_discrimination_index($question->id); + + $discriminationindex = $this->qbank->get_aggregate_statistic($question->id, 'discriminationindex'); echo $PAGE->get_renderer('qbank_statistics')->render_discrimination_index($discriminationindex); } diff --git a/question/bank/statistics/classes/columns/discriminative_efficiency.php b/question/bank/statistics/classes/columns/discriminative_efficiency.php index 05e93fdda21..82dd400e9fd 100644 --- a/question/bank/statistics/classes/columns/discriminative_efficiency.php +++ b/question/bank/statistics/classes/columns/discriminative_efficiency.php @@ -17,7 +17,6 @@ namespace qbank_statistics\columns; use core_question\local\bank\column_base; -use qbank_statistics\helper; /** * This column show the average discriminative efficiency for this question. @@ -41,10 +40,14 @@ class discriminative_efficiency extends column_base { return 'discriminative_efficiency'; } + public function get_required_statistics_fields(): array { + return ['discriminativeefficiency']; + } + protected function display_content($question, $rowclasses) { global $PAGE; - // Average discriminative efficiency per quiz. - $discriminativeefficiency = helper::calculate_average_question_discriminative_efficiency($question->id); + + $discriminativeefficiency = $this->qbank->get_aggregate_statistic($question->id, 'discriminativeefficiency'); echo $PAGE->get_renderer('qbank_statistics')->render_discriminative_efficiency($discriminativeefficiency); } diff --git a/question/bank/statistics/classes/columns/facility_index.php b/question/bank/statistics/classes/columns/facility_index.php index 98d050b4493..cae02d81df5 100644 --- a/question/bank/statistics/classes/columns/facility_index.php +++ b/question/bank/statistics/classes/columns/facility_index.php @@ -41,15 +41,18 @@ class facility_index extends column_base { return 'facility_index'; } + public function get_required_statistics_fields(): array { + return ['facility']; + } + protected function display_content($question, $rowclasses) { global $PAGE; - // Average facility index per quiz. - $facility = helper::calculate_average_question_facility($question->id); + + $facility = $this->qbank->get_aggregate_statistic($question->id, 'facility'); echo $PAGE->get_renderer('qbank_statistics')->render_facility_index($facility); } public function get_extra_classes(): array { return ['pr-3']; } - } diff --git a/question/bank/statistics/classes/helper.php b/question/bank/statistics/classes/helper.php index e694a484e15..5832c72834c 100644 --- a/question/bank/statistics/classes/helper.php +++ b/question/bank/statistics/classes/helper.php @@ -40,7 +40,7 @@ class helper { private const NEED_FOR_REVISION_UPPER_THRESHOLD = 50; /** - * For a list of questions find all the places (defined by (component, contextid) where there are attempts. + * For a list of questions find all the places, defined by (component, contextid) where there are attempts. * * @param int[] $questionids array of question ids that we are interested in. * @return \stdClass[] list of objects with fields ->component and ->contextid. diff --git a/question/bank/upgrade.txt b/question/bank/upgrade.txt index b2a79a032d4..8709c62c6e9 100644 --- a/question/bank/upgrade.txt +++ b/question/bank/upgrade.txt @@ -1,7 +1,17 @@ This file describes core qbank plugin changes in /question/bank/*, information provided here is intended especially for developers. +=== 4.0.9 === + +* There is a new more effecient way to display statistics in the question bank, you should now override + the get_required_statistics_fields() method in your column class, and then + the values you need will be available from $this->qbank->get_aggregate_statistic(...). + If you are not in a question_bank_column class, you can directly access efficient + statistics-loading from the core_question\local\statistics\statistics_bulk_loader class. + The old method will be deprecated in 4.3. + === 4.0.5 === + * New functions qbank_usage\helper::get_question_bank_usage_sql and qbank_usage\helper::get_question_attempt_usage_sql have been implemented. When calling a query with the SQL those methods returned, you have to be sure diff --git a/question/classes/local/bank/column_base.php b/question/classes/local/bank/column_base.php index 3fa1c86dc46..3a4898ddae0 100644 --- a/question/classes/local/bank/column_base.php +++ b/question/classes/local/bank/column_base.php @@ -325,6 +325,20 @@ abstract class column_base { return []; } + /** + * If this column requires any aggregated statistics, it should declare that here. + * + * This is those statistics can be efficiently loaded in bulk. + * + * The statistics are all loaded just before load_additional_data is called on each column. + * The values are then available from $this->qbank->get_aggregate_statistic(...); + * + * @return string[] the names of the required statistics fields. E.g. ['facility']. + */ + public function get_required_statistics_fields(): array { + return []; + } + /** * If this column needs extra data (e.g. tags) then load that here. * @@ -332,7 +346,7 @@ abstract class column_base { * Probably a good idea to check that another column has not already * loaded the data you want. * - * @param \stdClass[] $questions the questions that will be displayed. + * @param \stdClass[] $questions the questions that will be displayed, indexed by question id. */ public function load_additional_data(array $questions) { } diff --git a/question/classes/local/bank/view.php b/question/classes/local/bank/view.php index bfbf766afdd..80c8a0fdf9b 100644 --- a/question/classes/local/bank/view.php +++ b/question/classes/local/bank/view.php @@ -24,14 +24,15 @@ namespace core_question\local\bank; +use core_plugin_manager; +use core_question\bank\search\condition; +use core_question\local\statistics\statistics_bulk_loader; +use qbank_columnsortorder\column_manager; +use qbank_editquestion\editquestion_helper; + defined('MOODLE_INTERNAL') || die(); require_once($CFG->dirroot . '/question/editlib.php'); -use core_plugin_manager; -use core_question\bank\search\condition; -use qbank_columnsortorder\column_manager; -use qbank_editquestion\editquestion_helper; -use qbank_managecategories\helper; /** * This class prints a view of the question bank. @@ -90,19 +91,19 @@ class view { public $course; /** - * @var \question_bank_column_base[] these are all the 'columns' that are + * @var column_base[] these are all the 'columns' that are * part of the display. Array keys are the class name. */ protected $requiredcolumns; /** - * @var \question_bank_column_base[] these are the 'columns' that are + * @var column_base[] these are the 'columns' that are * actually displayed as a column, in order. Array keys are the class name. */ protected $visiblecolumns; /** - * @var \question_bank_column_base[] these are the 'columns' that are + * @var column_base[] these are the 'columns' that are * actually displayed as an additional row (e.g. question text), in order. * Array keys are the class name. */ @@ -134,6 +135,15 @@ class view { */ protected $sqlparams; + /** + * @var ?array Stores all the average statistics that this question bank view needs. + * + * This field gets initialised in {@see display_question_list()}. It is a two dimensional + * $this->loadedstatistics[$questionid][$fieldname] = $average value of that statistics for that question. + * Column classes in qbank plugins can access these values using {@see get_aggregate_statistic()}. + */ + protected $loadedstatistics = null; + /** * @var condition[] search conditions. */ @@ -969,6 +979,11 @@ class view { } } $questionsrs->close(); + + // Bulk load any required statistics. + $this->load_required_statistics($questions); + + // Bulk load any extra data that any column requires. foreach ($this->requiredcolumns as $name => $column) { $column->load_additional_data($questions); } @@ -995,6 +1010,60 @@ class view { echo \html_writer::end_tag('form'); } + /** + * Work out the list of all the required statistics fields for this question bank view. + * + * This gathers all the required fields from all columns, so they can all be loaded at once. + * + * @return string[] the names of all the required fields for this question bank view. + */ + protected function determine_required_statistics(): array { + $requiredfields = []; + foreach ($this->requiredcolumns as $column) { + $requiredfields = array_merge($requiredfields, $column->get_required_statistics_fields()); + } + + return array_unique($requiredfields); + } + + /** + * Load the aggregate statistics that all the columns require. + * + * @param \stdClass[] $questions the questions that will be displayed indexed by question id. + */ + protected function load_required_statistics(array $questions): void { + $requiredstatistics = $this->determine_required_statistics(); + $this->loadedstatistics = statistics_bulk_loader::load_aggregate_statistics( + array_keys($questions), $requiredstatistics); + } + + /** + * Get the aggregated value of a particular statistic for a particular question. + * + * You can only get values for the questions on the current page of the question bank view, + * and only if you declared the need for this statistic in the get_required_statistics_fields() + * method of your question bank column. + * + * @param int $questionid the id of a question + * @param string $fieldname the name of a statistics field, e.g. 'facility'. + * @return float|null the average (across all users) of this statistic for this question. + * Null if the value is not available right now. + */ + public function get_aggregate_statistic(int $questionid, string $fieldname): ?float { + if (!array_key_exists($questionid, $this->loadedstatistics)) { + throw new \coding_exception('Question ' . $questionid . ' is not on the current page of ' . + 'this question bank view, so its statistics are not available.'); + } + + // Must be array_key_exists, not isset, because we care about null values. + if (!array_key_exists($fieldname, $this->loadedstatistics[$questionid])) { + throw new \coding_exception('Statistics field ' . $fieldname . ' was not requested by any ' . + 'question bank column in this view, so it is not available.'); + } + + return $this->loadedstatistics[$questionid][$fieldname]; + } + /** * Display the top pagination bar. * diff --git a/question/classes/local/statistics/statistics_bulk_loader.php b/question/classes/local/statistics/statistics_bulk_loader.php new file mode 100644 index 00000000000..60ad643a255 --- /dev/null +++ b/question/classes/local/statistics/statistics_bulk_loader.php @@ -0,0 +1,170 @@ +. + +namespace core_question\local\statistics; + +use core_question\local\bank\column_base; +use core_question\statistics\questions\all_calculated_for_qubaid_condition; +use core_component; + +/** + * Helper to efficiently load all the statistics for a set of questions. + * + * If you are implementing a question bank column, do not use this method directly. + * Instead, override the {@see column_base::get_required_statistics_fields()} method + * in your column class, and the question bank view will take care of it for you. + * + * @package core_question + * @copyright 2023 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class statistics_bulk_loader { + + /** + * Load and aggregate the requested statistics for all the places where the given questions are used. + * + * The returned array will contain a values for each questionid and field, which will be null if the value is not available. + * + * @param int[] $questionids array of question ids. + * @param string[] $requiredstatistics array of the fields required, e.g. ['facility', 'discriminationindex']. + * @return float[][] if a value is not available, it will be set to null. + */ + public static function load_aggregate_statistics(array $questionids, array $requiredstatistics): array { + $places = self::get_all_places_where_questions_were_attempted($questionids); + + // Set up blank two-dimensional arrays to store the running totals. Indexed by questionid and field name. + $zerovaluesforonequestion = array_combine($requiredstatistics, array_fill(0, count($requiredstatistics), 0)); + $counts = array_combine($questionids, array_fill(0, count($questionids), $zerovaluesforonequestion)); + $sums = array_combine($questionids, array_fill(0, count($questionids), $zerovaluesforonequestion)); + + // Load the data for each place, and add to the running totals. + foreach ($places as $place) { + $statistics = self::load_statistics_for_place($place->component, + \context::instance_by_id($place->contextid)); + if ($statistics === null) { + continue; + } + + foreach ($questionids as $questionid) { + foreach ($requiredstatistics as $item) { + $value = self::extract_item_value($statistics, $questionid, $item); + if ($value === null) { + continue; + } + + $counts[$questionid][$item] += 1; + $sums[$questionid][$item] += $value; + } + } + } + + // Compute the averages from the final totals. + $aggregates = []; + foreach ($questionids as $questionid) { + $aggregates[$questionid] = []; + foreach ($requiredstatistics as $item) { + if ($counts[$questionid][$item] > 0) { + $aggregates[$questionid][$item] = $sums[$questionid][$item] / $counts[$questionid][$item]; + } else { + $aggregates[$questionid][$item] = null; + } + + } + } + + return $aggregates; + } + + /** + * For a list of questions find all the places, defined by (component, contextid), where there are attempts. + * + * @param int[] $questionids array of question ids that we are interested in. + * @return \stdClass[] list of objects with fields ->component and ->contextid. + */ + protected static function get_all_places_where_questions_were_attempted(array $questionids): array { + global $DB; + + [$questionidcondition, $params] = $DB->get_in_or_equal($questionids); + // The MIN(qu.id) is just to ensure that the rows have a unique key. + $places = $DB->get_records_sql(" + SELECT MIN(qu.id) AS somethingunique, qu.component, qu.contextid + FROM {question_usages} qu + JOIN {question_attempts} qatt ON qatt.questionusageid = qu.id + WHERE qatt.questionid $questionidcondition + GROUP BY qu.component, qu.contextid + ORDER BY qu.contextid ASC + ", $params); + + // Strip out the unwanted ids. + $places = array_values($places); + foreach ($places as $place) { + unset($place->somethingunique); + } + + return $places; + } + + /** + * Load the question statistics for all the attempts belonging to a particular component in a particular context. + * + * @param string $component frankenstyle component name, e.g. 'mod_quiz'. + * @param \context $context the context to load the statistics for. + * @return all_calculated_for_qubaid_condition|null question statistics. + */ + protected static function load_statistics_for_place( + string $component, + \context $context + ): ?all_calculated_for_qubaid_condition { + // This check is basically if (component_exists). + if (empty(core_component::get_component_directory($component))) { + return null; + } + + if (!component_callback_exists($component, 'calculate_question_stats')) { + return null; + } + + return component_callback($component, 'calculate_question_stats', [$context]); + } + + /** + * Extract the value for one question and one type of statistic from a set of statistics. + * + * @param all_calculated_for_qubaid_condition $statistics the batch of statistics. + * @param int $questionid a question id. + * @param string $item one of the field names in all_calculated_for_qubaid_condition, e.g. 'facility'. + * @return float|null the required value. + */ + protected static function extract_item_value(all_calculated_for_qubaid_condition $statistics, + int $questionid, string $item): ?float { + + // Look in main questions. + foreach ($statistics->questionstats as $stats) { + if ($stats->questionid == $questionid && isset($stats->$item)) { + return $stats->$item; + } + } + + // If not found, look in sub questions. + foreach ($statistics->subquestionstats as $stats) { + if ($stats->questionid == $questionid && isset($stats->$item)) { + return $stats->$item; + } + } + + return null; + } +} diff --git a/question/tests/local/statistics/statistics_bulk_loader_test.php b/question/tests/local/statistics/statistics_bulk_loader_test.php new file mode 100644 index 00000000000..50f26dee858 --- /dev/null +++ b/question/tests/local/statistics/statistics_bulk_loader_test.php @@ -0,0 +1,550 @@ +. + +namespace core_question\local\statistics; + +use advanced_testcase; +use context; +use context_module; +use core_question\statistics\questions\all_calculated_for_qubaid_condition; +use core_question_generator; +use Generator; +use quiz; +use quiz_attempt; +use question_engine; +use ReflectionMethod; + +/** + * Tests for question statistics. + * + * @package core_question + * @copyright 2021 Catalyst IT Australia Pty Ltd + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core_question\local\statistics\statistics_bulk_loader + */ +class statistics_bulk_loader_test extends advanced_testcase { + + /** @var float Delta used when comparing statistics values out-of 1. */ + protected const DELTA = 0.00005; + + /** @var float Delta used when comparing statistics values out-of 100. */ + protected const PERCENT_DELTA = 0.005; + + /** + * Test quizzes that contain a specified question. + * + * @covers ::get_all_places_where_questions_were_attempted + */ + public function test_get_all_places_where_questions_were_attempted(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + + $rcm = new ReflectionMethod(statistics_bulk_loader::class, 'get_all_places_where_questions_were_attempted'); + $rcm->setAccessible(true); + + // Create a course. + $course = $this->getDataGenerator()->create_course(); + + // Create three quizzes. + $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); + $quiz1 = $quizgenerator->create_instance([ + 'course' => $course->id, + 'grade' => 100.0, 'sumgrades' => 2, + 'layout' => '1,2,0' + ]); + $quiz1context = context_module::instance($quiz1->cmid); + + $quiz2 = $quizgenerator->create_instance([ + 'course' => $course->id, + 'grade' => 100.0, 'sumgrades' => 2, + 'layout' => '1,2,0' + ]); + $quiz2context = context_module::instance($quiz2->cmid); + + $quiz3 = $quizgenerator->create_instance([ + 'course' => $course->id, + 'grade' => 100.0, 'sumgrades' => 2, + 'layout' => '1,2,0' + ]); + $quiz3context = context_module::instance($quiz3->cmid); + + // Create questions. + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $cat = $questiongenerator->create_question_category(); + $question1 = $questiongenerator->create_question('shortanswer', null, ['category' => $cat->id]); + $question2 = $questiongenerator->create_question('numerical', null, ['category' => $cat->id]); + + // Add question 1 to quiz 1 and make an attempt. + quiz_add_quiz_question($question1->id, $quiz1); + // Quiz 1 attempt. + $this->submit_quiz($quiz1, [1 => ['answer' => 'frog']]); + + // Add questions 1 and 2 to quiz 2. + quiz_add_quiz_question($question1->id, $quiz2); + quiz_add_quiz_question($question2->id, $quiz2); + $this->submit_quiz($quiz2, [1 => ['answer' => 'frog'], 2 => ['answer' => 10]]); + + // Checking quizzes that use question 1. + $q1places = $rcm->invoke(null, [$question1->id]); + $this->assertCount(2, $q1places); + $this->assertEquals((object) ['component' => 'mod_quiz', 'contextid' => $quiz1context->id], $q1places[0]); + $this->assertEquals((object) ['component' => 'mod_quiz', 'contextid' => $quiz2context->id], $q1places[1]); + + // Checking quizzes that contain question 2. + $q2places = $rcm->invoke(null, [$question2->id]); + $this->assertCount(1, $q2places); + $this->assertEquals((object) ['component' => 'mod_quiz', 'contextid' => $quiz2context->id], $q2places[0]); + + // Add a random question to quiz3. + quiz_add_random_questions($quiz3, 0, $cat->id, 1, false); + $this->submit_quiz($quiz3, [1 => ['answer' => 'willbewrong']]); + + // Quiz 3 will now be in one of these arrays. + $q1places = $rcm->invoke(null, [$question1->id]); + $q2places = $rcm->invoke(null, [$question2->id]); + if (count($q1places) == 3) { + $newplace = end($q1places); + } else { + $newplace = end($q2places); + } + $this->assertEquals((object) ['component' => 'mod_quiz', 'contextid' => $quiz3context->id], $newplace); + } + + /** + * Create 2 quizzes. + * + * @return array return 2 quizzes + */ + private function prepare_quizzes(): array { + // Create a course. + $course = $this->getDataGenerator()->create_course(); + + // Make 2 quizzes. + $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); + $layout = '1,2,0,3,4,0'; + $quiz1 = $quizgenerator->create_instance([ + 'course' => $course->id, + 'grade' => 100.0, 'sumgrades' => 2, + 'layout' => $layout + ]); + + $quiz2 = $quizgenerator->create_instance([ + 'course' => $course->id, + 'grade' => 100.0, 'sumgrades' => 2, + 'layout' => $layout + ]); + + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $cat = $questiongenerator->create_question_category(); + + $page = 1; + $questions = []; + foreach (explode(',', $layout) as $slot) { + if ($slot == 0) { + $page += 1; + continue; + } + + $question = $questiongenerator->create_question('shortanswer', null, ['category' => $cat->id]); + $questions[$slot] = $question; + quiz_add_quiz_question($question->id, $quiz1, $page); + quiz_add_quiz_question($question->id, $quiz2, $page); + } + + return [$quiz1, $quiz2, $questions]; + } + + /** + * Submit quiz answers + * + * @param object $quiz + * @param array $answers + */ + private function submit_quiz(object $quiz, array $answers): void { + // Create user. + $user = $this->getDataGenerator()->create_user(); + // Create attempt. + $quizobj = quiz::create($quiz->id, $user->id); + $quba = question_engine::make_questions_usage_by_activity('mod_quiz', $quizobj->get_context()); + $quba->set_preferred_behaviour($quizobj->get_quiz()->preferredbehaviour); + $timenow = time(); + $attempt = quiz_create_attempt($quizobj, 1, null, $timenow, false, $user->id); + quiz_start_new_attempt($quizobj, $quba, $attempt, 1, $timenow); + quiz_attempt_save_started($quizobj, $quba, $attempt); + // Submit attempt. + $attemptobj = quiz_attempt::create($attempt->id); + $attemptobj->process_submitted_actions($timenow, false, $answers); + $attemptobj->process_finish($timenow, false); + } + + /** + * Generate attempt answers. + * + * @param array $correctanswerflags array of 1 or 0 + * 1 : generate correct answer + * 0 : generate wrong answer + * + * @return array + */ + private function generate_attempt_answers(array $correctanswerflags): array { + $attempt = []; + for ($i = 1; $i <= 4; $i++) { + if (isset($correctanswerflags) && $correctanswerflags[$i - 1] == 1) { + // Correct answer. + $attempt[$i] = ['answer' => 'frog']; + } else { + $attempt[$i] = ['answer' => 'false']; + } + } + return $attempt; + } + + /** + * Generate quizzes and submit answers. + * + * @param array $quiz1attempts quiz 1 attempts + * @param array $quiz2attempts quiz 2 attempts + * + * @return array + */ + private function prepare_and_submit_quizzes(array $quiz1attempts, array $quiz2attempts): array { + list($quiz1, $quiz2, $questions) = $this->prepare_quizzes(); + // Submit attempts of quiz1. + foreach ($quiz1attempts as $attempt) { + $this->submit_quiz($quiz1, $attempt); + } + // Submit attempts of quiz2. + foreach ($quiz2attempts as $attempt) { + $this->submit_quiz($quiz2, $attempt); + } + return [$quiz1, $quiz2, $questions]; + } + + /** + * To use private helper::extract_item_value function. + * + * @param all_calculated_for_qubaid_condition $statistics the batch of statistics. + * @param int $questionid a question id. + * @param string $item one of the field names in all_calculated_for_qubaid_condition, e.g. 'facility'. + * @return float|null the required value. + */ + private function extract_item_value(all_calculated_for_qubaid_condition $statistics, + int $questionid, string $item): ?float { + $rcm = new ReflectionMethod(statistics_bulk_loader::class, 'extract_item_value'); + $rcm->setAccessible(true); + return $rcm->invoke(null, $statistics, $questionid, $item); + } + + /** + * To use private helper::load_statistics_for_place function (with mod_quiz component). + * + * @param context $context the context to load the statistics for. + * @return all_calculated_for_qubaid_condition|null question statistics. + */ + private function load_quiz_statistics_for_place(context $context): ?all_calculated_for_qubaid_condition { + $rcm = new ReflectionMethod(statistics_bulk_loader::class, 'load_statistics_for_place'); + $rcm->setAccessible(true); + return $rcm->invoke(null, 'mod_quiz', $context); + } + + /** + * Data provider for {@see test_load_question_facility()}. + * + * @return Generator + */ + public function load_question_facility_provider(): Generator { + yield 'Facility case 1' => [ + 'Quiz 1 attempts' => [ + $this->generate_attempt_answers([1, 0, 0, 0]), + ], + 'Expected quiz 1 facilities' => [1.0, 0.0, 0.0, 0.0], + 'Quiz 2 attempts' => [ + $this->generate_attempt_answers([1, 0, 0, 0]), + $this->generate_attempt_answers([1, 1, 0, 0]), + ], + 'Expected quiz 2 facilities' => [1.0, 0.5, 0.0, 0.0], + 'Expected average facilities' => [1.0, 0.25, 0.0, 0.0], + ]; + yield 'Facility case 2' => [ + 'Quiz 1 attempts' => [ + $this->generate_attempt_answers([1, 0, 0, 0]), + $this->generate_attempt_answers([1, 1, 0, 0]), + $this->generate_attempt_answers([1, 1, 1, 0]), + ], + 'Expected quiz 1 facilities' => [1.0, 0.6667, 0.3333, 0.0], + 'Quiz 2 attempts' => [ + $this->generate_attempt_answers([1, 0, 0, 0]), + $this->generate_attempt_answers([1, 1, 0, 0]), + $this->generate_attempt_answers([1, 1, 1, 0]), + $this->generate_attempt_answers([1, 1, 1, 1]), + ], + 'Expected quiz 2 facilities' => [1.0, 0.75, 0.5, 0.25], + 'Expected average facilities' => [1.0, 0.7083, 0.4167, 0.1250], + ]; + } + + /** + * Test question facility + * + * @dataProvider load_question_facility_provider + * + * @param array $quiz1attempts quiz 1 attempts + * @param array $expectedquiz1facilities expected quiz 1 facilities + * @param array $quiz2attempts quiz 2 attempts + * @param array $expectedquiz2facilities expected quiz 2 facilities + * @param array $expectedaveragefacilities expected average facilities + */ + public function test_load_question_facility( + array $quiz1attempts, + array $expectedquiz1facilities, + array $quiz2attempts, + array $expectedquiz2facilities, + array $expectedaveragefacilities) + : void { + $this->resetAfterTest(); + + list($quiz1, $quiz2, $questions) = $this->prepare_and_submit_quizzes($quiz1attempts, $quiz2attempts); + + // Quiz 1 facilities. + $stats = $this->load_quiz_statistics_for_place(context_module::instance($quiz1->cmid)); + $quiz1facility1 = $this->extract_item_value($stats, $questions[1]->id, 'facility'); + $quiz1facility2 = $this->extract_item_value($stats, $questions[2]->id, 'facility'); + $quiz1facility3 = $this->extract_item_value($stats, $questions[3]->id, 'facility'); + $quiz1facility4 = $this->extract_item_value($stats, $questions[4]->id, 'facility'); + + $this->assertEqualsWithDelta($expectedquiz1facilities[0], $quiz1facility1, self::DELTA); + $this->assertEqualsWithDelta($expectedquiz1facilities[1], $quiz1facility2, self::DELTA); + $this->assertEqualsWithDelta($expectedquiz1facilities[2], $quiz1facility3, self::DELTA); + $this->assertEqualsWithDelta($expectedquiz1facilities[3], $quiz1facility4, self::DELTA); + + // Quiz 2 facilities. + $stats = $this->load_quiz_statistics_for_place(context_module::instance($quiz2->cmid)); + $quiz2facility1 = $this->extract_item_value($stats, $questions[1]->id, 'facility'); + $quiz2facility2 = $this->extract_item_value($stats, $questions[2]->id, 'facility'); + $quiz2facility3 = $this->extract_item_value($stats, $questions[3]->id, 'facility'); + $quiz2facility4 = $this->extract_item_value($stats, $questions[4]->id, 'facility'); + + $this->assertEqualsWithDelta($expectedquiz2facilities[0], $quiz2facility1, self::DELTA); + $this->assertEqualsWithDelta($expectedquiz2facilities[1], $quiz2facility2, self::DELTA); + $this->assertEqualsWithDelta($expectedquiz2facilities[2], $quiz2facility3, self::DELTA); + $this->assertEqualsWithDelta($expectedquiz2facilities[3], $quiz2facility4, self::DELTA); + + // Average question facilities. + $stats = statistics_bulk_loader::load_aggregate_statistics( + [$questions[1]->id, $questions[2]->id, $questions[3]->id, $questions[4]->id], + ['facility'] + ); + + $this->assertEqualsWithDelta($expectedaveragefacilities[0], + $stats[$questions[1]->id]['facility'], self::DELTA); + $this->assertEqualsWithDelta($expectedaveragefacilities[1], + $stats[$questions[2]->id]['facility'], self::DELTA); + $this->assertEqualsWithDelta($expectedaveragefacilities[2], + $stats[$questions[3]->id]['facility'], self::DELTA); + $this->assertEqualsWithDelta($expectedaveragefacilities[3], + $stats[$questions[4]->id]['facility'], self::DELTA); + } + + /** + * Data provider for {@see test_load_question_discriminative_efficiency()}. + * @return Generator + */ + public function load_question_discriminative_efficiency_provider(): Generator { + yield 'Discriminative efficiency' => [ + 'Quiz 1 attempts' => [ + $this->generate_attempt_answers([1, 0, 0, 0]), + $this->generate_attempt_answers([1, 1, 0, 0]), + $this->generate_attempt_answers([1, 0, 1, 0]), + $this->generate_attempt_answers([1, 1, 1, 1]), + ], + 'Expected quiz 1 discriminative efficiency' => [null, 33.33, 33.33, 100.00], + 'Quiz 2 attempts' => [ + $this->generate_attempt_answers([1, 1, 1, 1]), + $this->generate_attempt_answers([0, 0, 0, 0]), + $this->generate_attempt_answers([1, 0, 0, 1]), + $this->generate_attempt_answers([0, 1, 1, 0]), + ], + 'Expected quiz 2 discriminative efficiency' => [50.00, 50.00, 50.00, 50.00], + 'Expected average discriminative efficiency' => [50.00, 41.67, 41.67, 75.00], + ]; + } + + /** + * Test discriminative efficiency + * + * @dataProvider load_question_discriminative_efficiency_provider + * + * @param array $quiz1attempts quiz 1 attempts + * @param array $expectedquiz1discriminativeefficiency expected quiz 1 discriminative efficiency + * @param array $quiz2attempts quiz 2 attempts + * @param array $expectedquiz2discriminativeefficiency expected quiz 2 discriminative efficiency + * @param array $expectedaveragediscriminativeefficiency expected average discriminative efficiency + */ + public function test_load_question_discriminative_efficiency( + array $quiz1attempts, + array $expectedquiz1discriminativeefficiency, + array $quiz2attempts, + array $expectedquiz2discriminativeefficiency, + array $expectedaveragediscriminativeefficiency + ): void { + $this->resetAfterTest(); + + list($quiz1, $quiz2, $questions) = $this->prepare_and_submit_quizzes($quiz1attempts, $quiz2attempts); + + // Quiz 1 discriminative efficiency. + $stats = $this->load_quiz_statistics_for_place(context_module::instance($quiz1->cmid)); + $discriminativeefficiency1 = $this->extract_item_value($stats, $questions[1]->id, 'discriminativeefficiency'); + $discriminativeefficiency2 = $this->extract_item_value($stats, $questions[2]->id, 'discriminativeefficiency'); + $discriminativeefficiency3 = $this->extract_item_value($stats, $questions[3]->id, 'discriminativeefficiency'); + $discriminativeefficiency4 = $this->extract_item_value($stats, $questions[4]->id, 'discriminativeefficiency'); + + $this->assertEqualsWithDelta($expectedquiz1discriminativeefficiency[0], + $discriminativeefficiency1, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz1discriminativeefficiency[1], + $discriminativeefficiency2, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz1discriminativeefficiency[2], + $discriminativeefficiency3, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz1discriminativeefficiency[3], + $discriminativeefficiency4, self::PERCENT_DELTA); + + // Quiz 2 discriminative efficiency. + $stats = $this->load_quiz_statistics_for_place(context_module::instance($quiz2->cmid)); + $discriminativeefficiency1 = $this->extract_item_value($stats, $questions[1]->id, 'discriminativeefficiency'); + $discriminativeefficiency2 = $this->extract_item_value($stats, $questions[2]->id, 'discriminativeefficiency'); + $discriminativeefficiency3 = $this->extract_item_value($stats, $questions[3]->id, 'discriminativeefficiency'); + $discriminativeefficiency4 = $this->extract_item_value($stats, $questions[4]->id, 'discriminativeefficiency'); + + $this->assertEqualsWithDelta($expectedquiz2discriminativeefficiency[0], + $discriminativeefficiency1, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz2discriminativeefficiency[1], + $discriminativeefficiency2, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz2discriminativeefficiency[2], + $discriminativeefficiency3, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz2discriminativeefficiency[3], + $discriminativeefficiency4, self::PERCENT_DELTA); + + // Average question discriminative efficiency. + $stats = statistics_bulk_loader::load_aggregate_statistics( + [$questions[1]->id, $questions[2]->id, $questions[3]->id, $questions[4]->id], + ['discriminativeefficiency'] + ); + + $this->assertEqualsWithDelta($expectedaveragediscriminativeefficiency[0], + $stats[$questions[1]->id]['discriminativeefficiency'], self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedaveragediscriminativeefficiency[1], + $stats[$questions[2]->id]['discriminativeefficiency'], self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedaveragediscriminativeefficiency[2], + $stats[$questions[3]->id]['discriminativeefficiency'], self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedaveragediscriminativeefficiency[3], + $stats[$questions[4]->id]['discriminativeefficiency'], self::PERCENT_DELTA); + } + + /** + * Data provider for {@see test_load_question_discrimination_index()}. + * @return Generator + */ + public function load_question_discrimination_index_provider(): Generator { + yield 'Discrimination Index' => [ + 'Quiz 1 attempts' => [ + $this->generate_attempt_answers([1, 0, 0, 0]), + $this->generate_attempt_answers([1, 1, 0, 0]), + $this->generate_attempt_answers([1, 0, 1, 0]), + $this->generate_attempt_answers([1, 1, 1, 1]), + ], + 'Expected quiz 1 Discrimination Index' => [null, 30.15, 30.15, 81.65], + 'Quiz 2 attempts' => [ + $this->generate_attempt_answers([1, 1, 1, 1]), + $this->generate_attempt_answers([0, 0, 0, 0]), + $this->generate_attempt_answers([1, 0, 0, 1]), + $this->generate_attempt_answers([0, 1, 1, 0]), + ], + 'Expected quiz 2 discrimination Index' => [44.72, 44.72, 44.72, 44.72], + 'Expected average discrimination Index' => [44.72, 37.44, 37.44, 63.19], + ]; + } + + /** + * Test discrimination index + * + * @dataProvider load_question_discrimination_index_provider + * + * @param array $quiz1attempts quiz 1 attempts + * @param array $expectedquiz1discriminationindex expected quiz 1 discrimination index + * @param array $quiz2attempts quiz 2 attempts + * @param array $expectedquiz2discriminationindex expected quiz 2 discrimination index + * @param array $expectedaveragediscriminationindex expected average discrimination index + */ + public function test_load_question_discrimination_index( + array $quiz1attempts, + array $expectedquiz1discriminationindex, + array $quiz2attempts, + array $expectedquiz2discriminationindex, + array $expectedaveragediscriminationindex + ): void { + $this->resetAfterTest(); + + list($quiz1, $quiz2, $questions) = $this->prepare_and_submit_quizzes($quiz1attempts, $quiz2attempts); + + // Quiz 1 discrimination index. + $stats = $this->load_quiz_statistics_for_place(context_module::instance($quiz1->cmid)); + $discriminationindex1 = $this->extract_item_value($stats, $questions[1]->id, 'discriminationindex'); + $discriminationindex2 = $this->extract_item_value($stats, $questions[2]->id, 'discriminationindex'); + $discriminationindex3 = $this->extract_item_value($stats, $questions[3]->id, 'discriminationindex'); + $discriminationindex4 = $this->extract_item_value($stats, $questions[4]->id, 'discriminationindex'); + + $this->assertEqualsWithDelta($expectedquiz1discriminationindex[0], + $discriminationindex1, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz1discriminationindex[1], + $discriminationindex2, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz1discriminationindex[2], + $discriminationindex3, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz1discriminationindex[3], + $discriminationindex4, self::PERCENT_DELTA); + + // Quiz 2 discrimination index. + $stats = $this->load_quiz_statistics_for_place(context_module::instance($quiz2->cmid)); + $discriminationindex1 = $this->extract_item_value($stats, $questions[1]->id, 'discriminationindex'); + $discriminationindex2 = $this->extract_item_value($stats, $questions[2]->id, 'discriminationindex'); + $discriminationindex3 = $this->extract_item_value($stats, $questions[3]->id, 'discriminationindex'); + $discriminationindex4 = $this->extract_item_value($stats, $questions[4]->id, 'discriminationindex'); + + $this->assertEqualsWithDelta($expectedquiz2discriminationindex[0], + $discriminationindex1, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz2discriminationindex[1], + $discriminationindex2, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz2discriminationindex[2], + $discriminationindex3, self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedquiz2discriminationindex[3], + $discriminationindex4, self::PERCENT_DELTA); + + // Average question discrimination index. + $stats = statistics_bulk_loader::load_aggregate_statistics( + [$questions[1]->id, $questions[2]->id, $questions[3]->id, $questions[4]->id], + ['discriminationindex'] + ); + + $this->assertEqualsWithDelta($expectedaveragediscriminationindex[0], + $stats[$questions[1]->id]['discriminationindex'], self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedaveragediscriminationindex[1], + $stats[$questions[2]->id]['discriminationindex'], self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedaveragediscriminationindex[2], + $stats[$questions[3]->id]['discriminationindex'], self::PERCENT_DELTA); + $this->assertEqualsWithDelta($expectedaveragediscriminationindex[3], + $stats[$questions[4]->id]['discriminationindex'], self::PERCENT_DELTA); + } +} From 5f2ce7c383fcd4d72379e50eb7e14477c2752294 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 30 Jan 2023 16:02:50 +0000 Subject: [PATCH 3/5] MDL-75576 quiz statistics: don't compute when a user views the qbank Previously, when users viewed the question bank, if the quiz statistics had not already been calculated, we would try to compute them there an then. This could be very, very slow, leading to session lock problems. Now, we never try to compute the statistics on the fly. Instead, we rely on the existing \quiz_statistics\task\recalculate scheduled task to do it. --- mod/quiz/lib.php | 4 ++-- mod/quiz/report/statistics/report.php | 23 +++++++++++++++---- mod/quiz/report/statistics/upgrade.txt | 7 ++++++ .../tests/behat/statistics_values.feature | 4 ++++ .../bank/statistics/tests/helper_test.php | 6 +++++ .../statistics_bulk_loader_test.php | 6 +++++ 6 files changed, 44 insertions(+), 6 deletions(-) diff --git a/mod/quiz/lib.php b/mod/quiz/lib.php index ea737cd559b..687a7100ca5 100644 --- a/mod/quiz/lib.php +++ b/mod/quiz/lib.php @@ -2482,12 +2482,12 @@ function quiz_delete_references($quizid): void { * This enables quiz statistics to be shown in statistics columns in the database. * * @param context $context return the statistics related to this context (which will be a quiz context). - * @return all_calculated_for_qubaid_condition|null The statistics for this quiz, if any, else null. + * @return all_calculated_for_qubaid_condition|null The statistics for this quiz, if available, else null. */ function mod_quiz_calculate_question_stats(context $context): ?all_calculated_for_qubaid_condition { global $CFG; require_once($CFG->dirroot . '/mod/quiz/report/statistics/report.php'); $cm = get_coursemodule_from_id('quiz', $context->instanceid); $report = new quiz_statistics_report(); - return $report->calculate_questions_stats_for_question_bank($cm->instance); + return $report->calculate_questions_stats_for_question_bank($cm->instance, false); } diff --git a/mod/quiz/report/statistics/report.php b/mod/quiz/report/statistics/report.php index 32ef27cd279..cddba4477ca 100644 --- a/mod/quiz/report/statistics/report.php +++ b/mod/quiz/report/statistics/report.php @@ -627,11 +627,15 @@ class quiz_statistics_report extends quiz_default_report { * @param \core\dml\sql_join $groupstudentsjoins Contains joins, wheres, params for students in this group. * @param array $questions full question data. * @param \core\progress\base|null $progress + * @param bool $calculateifrequired if true (the default) the stats will be calculated if not already stored. + * If false, [null, null] will be returned if the stats are not already available. * @return array with 2 elements: - $quizstats The statistics for overall attempt scores. * - $questionstats \core_question\statistics\questions\all_calculated_for_qubaid_condition + * Both may be null, if $calculateifrequired is false. */ public function get_all_stats_and_analysis( - $quiz, $whichattempts, $whichtries, \core\dml\sql_join $groupstudentsjoins, $questions, $progress = null) { + $quiz, $whichattempts, $whichtries, \core\dml\sql_join $groupstudentsjoins, + $questions, $progress = null, bool $calculateifrequired = true) { if ($progress === null) { $progress = new \core\progress\none(); @@ -645,6 +649,11 @@ class quiz_statistics_report extends quiz_default_report { $progress->start_progress('', 3); if ($quizcalc->get_last_calculated_time($qubaids) === false) { + if (!$calculateifrequired) { + $progress->progress(3); + $progress->end_progress(); + return [null, null]; + } // Recalculate now. $questionstats = $qcalc->calculate($qubaids); @@ -927,15 +936,21 @@ class quiz_statistics_report extends quiz_default_report { * Load question stats for a quiz * * @param int $quizid question usage - * @return all_calculated_for_qubaid_condition question stats + * @param bool $calculateifrequired if true (the default) the stats will be calculated if not already stored. + * If false, null will be returned if the stats are not already available. + * @return ?all_calculated_for_qubaid_condition question stats */ - public function calculate_questions_stats_for_question_bank(int $quizid): all_calculated_for_qubaid_condition { + public function calculate_questions_stats_for_question_bank( + int $quizid, + bool $calculateifrequired = true + ): ?all_calculated_for_qubaid_condition { global $DB; $quiz = $DB->get_record('quiz', ['id' => $quizid], '*', MUST_EXIST); $questions = $this->load_and_initialise_questions_for_calculations($quiz); [, $questionstats] = $this->get_all_stats_and_analysis($quiz, - $quiz->grademethod, question_attempt::ALL_TRIES, new \core\dml\sql_join(), $questions); + $quiz->grademethod, question_attempt::ALL_TRIES, new \core\dml\sql_join(), + $questions, null, $calculateifrequired); return $questionstats; } diff --git a/mod/quiz/report/statistics/upgrade.txt b/mod/quiz/report/statistics/upgrade.txt index 7926b3f81b6..66ef390739e 100644 --- a/mod/quiz/report/statistics/upgrade.txt +++ b/mod/quiz/report/statistics/upgrade.txt @@ -1,6 +1,13 @@ This files describes API changes in /mod/quiz/report/statistics/*, information provided here is intended especially for developers. +=== 4.0.9 === + +* The methods quiz_statistics_report::calculate_questions_stats_for_question_bank and get_all_stats_and_analysis + (which are really private to the quiz, and not part of any API you should be using) now have a new + optional argument $calculateifrequired. + + === 3.2 === * The function quiz_statistics_graph_get_new_colour() is deprecated in favour of the diff --git a/question/bank/statistics/tests/behat/statistics_values.feature b/question/bank/statistics/tests/behat/statistics_values.feature index a65a7f58275..9735e2be10d 100644 --- a/question/bank/statistics/tests/behat/statistics_values.feature +++ b/question/bank/statistics/tests/behat/statistics_values.feature @@ -72,6 +72,7 @@ Feature: Show statistics in question bank | slot | response | | 1 | True | | 2 | True | + And I run the scheduled task "\quiz_statistics\task\recalculate" 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" @@ -87,6 +88,7 @@ Feature: Show statistics in question bank | slot | response | | 1 | True | | 2 | True | + And I run the scheduled task "\quiz_statistics\task\recalculate" 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" @@ -102,6 +104,7 @@ Feature: Show statistics in question bank | slot | response | | 1 | True | | 2 | True | + And I run the scheduled task "\quiz_statistics\task\recalculate" 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" @@ -123,6 +126,7 @@ Feature: Show statistics in question bank | slot | response | | 1 | True | | 2 | False | + And I run the scheduled task "\quiz_statistics\task\recalculate" 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" diff --git a/question/bank/statistics/tests/helper_test.php b/question/bank/statistics/tests/helper_test.php index f80084e91d9..94e9afad6a9 100644 --- a/question/bank/statistics/tests/helper_test.php +++ b/question/bank/statistics/tests/helper_test.php @@ -225,6 +225,12 @@ class helper_test extends \advanced_testcase { foreach ($quiz2attempts as $attempt) { $this->submit_quiz($quiz2, $attempt); } + + // Calculate the statistics. + $this->expectOutputRegex('~.*Calculations completed.*~'); + $statisticstask = new \quiz_statistics\task\recalculate(); + $statisticstask->execute(); + return [$quiz1, $quiz2, $questions]; } diff --git a/question/tests/local/statistics/statistics_bulk_loader_test.php b/question/tests/local/statistics/statistics_bulk_loader_test.php index 50f26dee858..7c6ec53387d 100644 --- a/question/tests/local/statistics/statistics_bulk_loader_test.php +++ b/question/tests/local/statistics/statistics_bulk_loader_test.php @@ -232,6 +232,12 @@ class statistics_bulk_loader_test extends advanced_testcase { foreach ($quiz2attempts as $attempt) { $this->submit_quiz($quiz2, $attempt); } + + // Calculate the statistics. + $this->expectOutputRegex('~.*Calculations completed.*~'); + $statisticstask = new \quiz_statistics\task\recalculate(); + $statisticstask->execute(); + return [$quiz1, $quiz2, $questions]; } From a3ba4772a57dd33fc1a97ab3d1e7a765a020869c Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 17 Feb 2023 15:53:55 +0000 Subject: [PATCH 4/5] MDL-75576 quiz statistics: don't time-limit the use of cached values This should have been done as part of MDL-74762, but was missed. --- mod/quiz/report/statistics/classes/calculator.php | 15 +++++---------- mod/quiz/report/statistics/upgrade.txt | 5 +++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/mod/quiz/report/statistics/classes/calculator.php b/mod/quiz/report/statistics/classes/calculator.php index e6ff9567a69..8cd5620522a 100644 --- a/mod/quiz/report/statistics/classes/calculator.php +++ b/mod/quiz/report/statistics/classes/calculator.php @@ -121,21 +121,19 @@ class calculator { return $quizstats; } - /** @var integer Time after which statistics are automatically recomputed. */ + /** @var int No longer used. Previously the time after which statistics are automatically recomputed. */ const TIME_TO_CACHE = 900; // 15 minutes. /** * Load cached statistics from the database. * - * @param $qubaids \qubaid_condition + * @param \qubaid_condition $qubaids * @return calculated The statistics for overall attempt scores or false if not cached. */ public function get_cached($qubaids) { global $DB; - $timemodified = time() - self::TIME_TO_CACHE; - $fromdb = $DB->get_record_select('quiz_statistics', 'hashcode = ? AND timemodified > ?', - array($qubaids->get_hash_code(), $timemodified)); + $fromdb = $DB->get_record('quiz_statistics', ['hashcode' => $qubaids->get_hash_code()]); $stats = new calculated(); $stats->populate_from_record($fromdb); return $stats; @@ -145,14 +143,11 @@ class calculator { * Find time of non-expired statistics in the database. * * @param $qubaids \qubaid_condition - * @return integer|boolean Time of cached record that matches this qubaid_condition or false is non found. + * @return int|bool Time of cached record that matches this qubaid_condition or false is non found. */ public function get_last_calculated_time($qubaids) { global $DB; - - $timemodified = time() - self::TIME_TO_CACHE; - return $DB->get_field_select('quiz_statistics', 'timemodified', 'hashcode = ? AND timemodified > ?', - array($qubaids->get_hash_code(), $timemodified)); + return $DB->get_field('quiz_statistics', 'timemodified', ['hashcode' => $qubaids->get_hash_code()]); } /** diff --git a/mod/quiz/report/statistics/upgrade.txt b/mod/quiz/report/statistics/upgrade.txt index 66ef390739e..a85da1b8984 100644 --- a/mod/quiz/report/statistics/upgrade.txt +++ b/mod/quiz/report/statistics/upgrade.txt @@ -7,6 +7,11 @@ information provided here is intended especially for developers. (which are really private to the quiz, and not part of any API you should be using) now have a new optional argument $calculateifrequired. +* In the past, the methods \quiz_statistics\calculator::get_last_calculated_time() and calculator::get_cached() + 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. The constant calculator::TIME_TO_CACHE will be + deprecated in Moodle 4.3. + === 3.2 === From 118d3e855a3755a7c24c2927652101c853ead85b Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 14 Mar 2023 14:33:24 +0000 Subject: [PATCH 5/5] 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. --- lang/en/admin.php | 2 +- .../task/question_stats_cleanup_task.php | 22 ++++--------- lib/db/tasks.php | 3 +- lib/db/upgrade.php | 9 +++++ .../report/statistics/classes/calculated.php | 6 +++- .../report/statistics/classes/calculator.php | 17 ++++++++-- mod/quiz/report/statistics/report.php | 9 +++-- .../all_calculated_for_qubaid_condition.php | 28 +++++++++++----- .../classes/statistics/responses/analyser.php | 33 ++++++++++--------- .../analysis_for_actual_response.php | 5 +-- .../responses/analysis_for_class.php | 6 ++-- .../responses/analysis_for_question.php | 23 +++++++++++-- .../responses/analysis_for_subpart.php | 5 +-- question/upgrade.txt | 13 ++++++++ 14 files changed, 123 insertions(+), 58 deletions(-) diff --git a/lang/en/admin.php b/lang/en/admin.php index 4d6c8e7326b..4158af27a07 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -1367,7 +1367,7 @@ $string['taskplagiarismcron'] = 'Background processing for legacy cron in plagia $string['taskportfoliocron'] = 'Background processing for portfolio plugins'; $string['taskprocessing'] = 'Task processing'; $string['taskquestioncron'] = 'Background processing for cleaning up question previews'; -$string['taskquestionstatscleanupcron'] = 'Background processing for cleaning up question statistics caches'; +$string['taskquestionstatscleanupcron'] = 'Former background question statistics clean-up (no longer required)'; $string['taskrefreshsystemtokens'] = 'Refresh OAuth tokens for service accounts'; $string['taskregistrationcron'] = 'Site registration'; $string['tasksendfailedloginnotifications'] = 'Send failed login notifications'; diff --git a/lib/classes/task/question_stats_cleanup_task.php b/lib/classes/task/question_stats_cleanup_task.php index 651cc8dcdbc..055bd8fe0a5 100644 --- a/lib/classes/task/question_stats_cleanup_task.php +++ b/lib/classes/task/question_stats_cleanup_task.php @@ -17,6 +17,9 @@ /** * Task to cleanup old question statistics cache. * + * This task is no longer required. It has been kept on stable branches for backwards-compatibility, + * and will be removed completely in Moodle 4.3. + * * @package core * @copyright 2019 Simey Lameze * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later @@ -28,6 +31,9 @@ defined('MOODLE_INTERNAL') || die(); /** * A task to cleanup old question statistics cache. * + * This task is no longer required. It has been kept on stable branches for backwards-compatibility, + * and will be removed completely in Moodle 4.3. + * * @copyright 2019 Simey Lameze * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ @@ -46,20 +52,6 @@ class question_stats_cleanup_task extends scheduled_task { * 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.'); + mtrace("\nThis task is no longer required. If it is still running, please disable. Will be removed in Moodle 4.3.", ''); } } diff --git a/lib/db/tasks.php b/lib/db/tasks.php index 7a3021e92b0..60db33a1c86 100644 --- a/lib/db/tasks.php +++ b/lib/db/tasks.php @@ -228,7 +228,8 @@ $tasks = array( 'hour' => '*', 'day' => '*', 'dayofweek' => '*', - 'month' => '*' + 'month' => '*', + 'disabled' => true, ), array( 'classname' => 'core\task\registration_cron_task', diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 58f340ada4d..8fbfbdf3928 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -4575,5 +4575,14 @@ privatefiles,moodle|/user/files.php'; upgrade_main_savepoint(true, 2022041905.07); } + if ($oldversion < 2022041908.02) { + + // Remove any orphaned role assignment records (pointing to non-existing roles). + $DB->set_field('task_scheduled', 'disabled', 1, ['classname' => '\core\task\question_stats_cleanup_task']); + + // Main savepoint reached. + upgrade_main_savepoint(true, 2022041908.02); + } + return true; } diff --git a/mod/quiz/report/statistics/classes/calculated.php b/mod/quiz/report/statistics/classes/calculated.php index 223589bb814..69f82d5a348 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 8cd5620522a..c867e07c49d 100644 --- a/mod/quiz/report/statistics/classes/calculator.php +++ b/mod/quiz/report/statistics/classes/calculator.php @@ -128,12 +128,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; @@ -147,7 +152,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 cddba4477ca..92bca8953dc 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 core_question\statistics\questions\all_calculated_for_qubaid_condition; require_once($CFG->dirroot . '/mod/quiz/report/default.php'); @@ -420,7 +421,7 @@ class quiz_statistics_report extends quiz_default_report { } } - $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); @@ -748,10 +749,8 @@ class quiz_statistics_report extends quiz_default_report { 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..70aa1330b96 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,7 @@ use question_bank; */ class all_calculated_for_qubaid_condition { - /** @var int Time after which statistics are automatically recomputed. */ + /** @var int No longer used. Previously, the time after which statistics are automatically recomputed. */ const TIME_TO_CACHE = 900; // 15 minutes. /** @@ -197,9 +197,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 +251,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 +278,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..9e61c155331 100644 --- a/question/classes/statistics/responses/analyser.php +++ b/question/classes/statistics/responses/analyser.php @@ -41,7 +41,7 @@ class analyser { */ const MAX_TRY_COUNTED = 5; - /** @var int Time after which responses are automatically reanalysed. */ + /** @var int No longer used. Previously the time after which statistics are automatically recomputed. */ const TIME_TO_CACHE = 900; // 15 minutes. /** @var object full question data from db. */ @@ -52,6 +52,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 +114,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 +122,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 +137,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 +151,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 +189,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..55fe472cea1 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 ? $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 b350bd925d0..545368204ad 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 now does nothing, and will be removed in Moodle 4.3. 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. The constants TIME_TO_CACHE in those classes + will be deprecated in Moodle 4.3. + +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.0.5 ===