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/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/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 e6ff9567a69..c867e07c49d 100644 --- a/mod/quiz/report/statistics/classes/calculator.php +++ b/mod/quiz/report/statistics/classes/calculator.php @@ -121,21 +121,24 @@ 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 - * @return calculated The statistics for overall attempt scores or false if not cached. + * @param \qubaid_condition $qubaids + * @return calculated|false 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)); + $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; @@ -145,14 +148,17 @@ 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)); + $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 32ef27cd279..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); @@ -627,11 +628,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 +650,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); @@ -739,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; } @@ -927,15 +935,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..a85da1b8984 100644 --- a/mod/quiz/report/statistics/upgrade.txt +++ b/mod/quiz/report/statistics/upgrade.txt @@ -1,6 +1,18 @@ 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. + +* 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 === * The function quiz_statistics_graph_get_new_colour() is deprecated in favour of the diff --git a/question/bank/statistics/classes/columns/discrimination_index.php b/question/bank/statistics/classes/columns/discrimination_index.php index 8046127c821..054e53851e5 100644 --- a/question/bank/statistics/classes/columns/discrimination_index.php +++ b/question/bank/statistics/classes/columns/discrimination_index.php @@ -17,7 +17,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 +30,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,24 +38,18 @@ 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. - */ + 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 3d5a8632976..82dd400e9fd 100644 --- a/question/bank/statistics/classes/columns/discriminative_efficiency.php +++ b/question/bank/statistics/classes/columns/discriminative_efficiency.php @@ -17,7 +17,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 +28,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,24 +36,18 @@ 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. - */ + 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 d70fd5ea29b..cae02d81df5 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,29 +37,22 @@ 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. - */ + 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/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/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/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/tests/local/statistics/statistics_bulk_loader_test.php b/question/tests/local/statistics/statistics_bulk_loader_test.php new file mode 100644 index 00000000000..7c6ec53387d --- /dev/null +++ b/question/tests/local/statistics/statistics_bulk_loader_test.php @@ -0,0 +1,556 @@ +. + +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); + } + + // Calculate the statistics. + $this->expectOutputRegex('~.*Calculations completed.*~'); + $statisticstask = new \quiz_statistics\task\recalculate(); + $statisticstask->execute(); + + 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); + } +} 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 ===