From 7419d5d319f09426c3730b7c10b25ba5d8e6fdf2 Mon Sep 17 00:00:00 2001 From: Philipp Imhof <52650214+PhilippImhof@users.noreply.github.com> Date: Sun, 12 Nov 2023 14:19:27 +0100 Subject: [PATCH] MDL-80087 Questions: warn if preload_all_step_users was not called Co-authored-by: Tim Hunt --- mod/quiz/classes/external.php | 4 ++++ mod/quiz/tests/external/external_test.php | 2 +- question/engine/questionattemptstep.php | 24 +++++++++++++--------- question/engine/questionusage.php | 5 ++--- question/engine/tests/helpers.php | 16 +++++++++------ question/engine/tests/walkthrough_test.php | 8 ++++++++ 6 files changed, 39 insertions(+), 20 deletions(-) diff --git a/mod/quiz/classes/external.php b/mod/quiz/classes/external.php index e0120d8ea5d45..0f860ebe25964 100644 --- a/mod/quiz/classes/external.php +++ b/mod/quiz/classes/external.php @@ -1473,6 +1473,10 @@ public static function get_attempt_review($attemptid, $page = -1) { $page = 'all'; } + // Make sure all users associated to the attempt steps are loaded. Otherwise, this will + // trigger a debugging message. + $attemptobj->preload_all_attempt_step_users(); + // Prepare the output. $result = []; $result['attempt'] = $attemptobj->get_attempt(); diff --git a/mod/quiz/tests/external/external_test.php b/mod/quiz/tests/external/external_test.php index 9824e71e160e6..ff0c6be73d559 100644 --- a/mod/quiz/tests/external/external_test.php +++ b/mod/quiz/tests/external/external_test.php @@ -1720,7 +1720,7 @@ public function test_get_attempt_review() { $this->assertEquals('gradedright', $result['questions'][0]['state']); $this->assertEquals(1, $result['questions'][0]['slot']); - $this->assertCount(1, $result['additionaldata']); + $this->assertCount(1, $result['additionaldata']); $this->assertEquals('feedback', $result['additionaldata'][0]['id']); $this->assertEquals('Feedback', $result['additionaldata'][0]['title']); $this->assertEquals('Feedback text 1', $result['additionaldata'][0]['content']); diff --git a/question/engine/questionattemptstep.php b/question/engine/questionattemptstep.php index f8bddf6850733..da1750a090e49 100644 --- a/question/engine/questionattemptstep.php +++ b/question/engine/questionattemptstep.php @@ -87,10 +87,10 @@ class question_attempt_step { */ private $fraction = null; - /** @var integer the timestamp when this step was created. */ + /** @var int the timestamp when this step was created. */ private $timecreated; - /** @var integer the id of the user resonsible for creating this step. */ + /** @var int the id of the user responsible for creating this step. */ private $userid; /** @var array name => value pairs. The submitted data. */ @@ -99,7 +99,7 @@ class question_attempt_step { /** @var array name => array of {@see stored_file}s. Caches the contents of file areas. */ private $files = array(); - /** @var stdClass User information. */ + /** @var stdClass|null User information. */ private $user = null; /** @@ -107,12 +107,12 @@ class question_attempt_step { * normally created by {@see question_attempt} methods like * {@see question_attempt::process_action()}. * @param array $data the submitted data that defines this step. - * @param int $timestamp the time to record for the action. (If not given, use now.) - * @param int $userid the user to attribute the aciton to. (If not given, use the current user.) - * @param int $existingstepid if this step is going to replace an existing step + * @param int|null $timecreated the time to record for the action. (If not given, use now.) + * @param int|null $userid the user to attribute the aciton to. (If not given, use the current user.) + * @param int|null $existingstepid if this step is going to replace an existing step * (for example, during a regrade) this is the id of the previous step we are replacing. */ - public function __construct($data = array(), $timecreated = null, $userid = null, + public function __construct($data = [], $timecreated = null, $userid = null, $existingstepid = null) { global $USER; @@ -196,9 +196,13 @@ public function add_full_user_object(stdClass $user): void { /** * Return the full user object. * - * @return stdClass Get full user object. + * @return null|stdClass Get full user object. */ - public function get_user(): stdClass { + public function get_user(): ?stdClass { + if ($this->user === null) { + debugging('Attempt to access the step user before it was initialised. ' . + 'Did you forget to call question_usage_by_activity::preload_all_step_users() or similar?', DEBUG_DEVELOPER); + } return $this->user; } @@ -208,7 +212,7 @@ public function get_user(): stdClass { * @return string full name of user. */ public function get_user_fullname(): string { - return fullname($this->user); + return fullname($this->get_user()); } /** @return int the timestamp when this step was created. */ diff --git a/question/engine/questionusage.php b/question/engine/questionusage.php index c1aa1542014c1..798e369d738b7 100644 --- a/question/engine/questionusage.php +++ b/question/engine/questionusage.php @@ -1030,9 +1030,8 @@ public function preload_all_step_users(): void { // Update user information for steps. foreach ($this->questionattempts as $qa) { foreach ($qa->get_full_step_iterator() as $step) { - $user = $users[$step->get_user_id()]; - if (isset($user)) { - $step->add_full_user_object($user); + if (isset($users[$step->get_user_id()])) { + $step->add_full_user_object($users[$step->get_user_id()]); } } } diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index 402e2c10b5bb4..f83d9697a413e 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -791,11 +791,13 @@ public function __construct($pattern, $message = '') { abstract class qbehaviour_walkthrough_test_base extends question_testcase { /** @var question_display_options */ protected $displayoptions; + /** @var question_usage_by_activity */ protected $quba; - /** @var integer */ + /** @var int The slot number of the question_attempt we are using in $quba. */ protected $slot; + /** * @var string after {@link render()} has been called, this contains the * display of the question in its current state. @@ -804,7 +806,8 @@ abstract class qbehaviour_walkthrough_test_base extends question_testcase { protected function setUp(): void { parent::setUp(); - $this->resetAfterTest(true); + $this->resetAfterTest(); + $this->setAdminUser(); $this->displayoptions = new question_display_options(); $this->quba = question_engine::make_questions_usage_by_activity('unit_test', @@ -914,6 +917,7 @@ protected function check_current_mark($mark) { * $this->currentoutput so that it can be verified. */ protected function render() { + $this->quba->preload_all_step_users(); $this->currentoutput = $this->quba->render_question($this->slot, $this->displayoptions); } @@ -1005,9 +1009,9 @@ protected function get_tag_matcher($tag, $attributes) { * @param $condition one or more Expectations. (users varargs). */ protected function check_current_output() { - $html = $this->quba->render_question($this->slot, $this->displayoptions); + $this->render(); foreach (func_get_args() as $condition) { - $this->assert($condition, $html); + $this->assert($condition, $this->currentoutput); } } @@ -1019,9 +1023,9 @@ protected function check_current_output() { * @param question_contains_select_expectation $expectations One or more expectations. */ protected function check_output_contains_selectoptions(...$expectations) { - $html = $this->quba->render_question($this->slot, $this->displayoptions); + $this->render(); foreach ($expectations as $expectation) { - $this->assert_select_options($expectation, $html); + $this->assert_select_options($expectation, $this->currentoutput); } } diff --git a/question/engine/tests/walkthrough_test.php b/question/engine/tests/walkthrough_test.php index a834b003aa412..678f7a57d337d 100644 --- a/question/engine/tests/walkthrough_test.php +++ b/question/engine/tests/walkthrough_test.php @@ -107,6 +107,7 @@ public function test_action_author_with_display_options_testcase() { $displayoptions->userinfoinhistory = question_display_options::SHOW_ALL; $this->load_quba(); + $this->quba->preload_all_step_users(); $result = $this->quba->render_question($this->slot, $displayoptions); $numsteps = $this->quba->get_question_attempt($this->slot)->get_num_steps(); @@ -121,6 +122,13 @@ public function test_action_author_with_display_options_testcase() { $this->load_quba(); $result = $this->quba->render_question($this->slot, $displayoptions); + $message = 'Attempt to access the step user before it was initialised.'; + $message .= ' Did you forget to call question_usage_by_activity::preload_all_step_users() or similar?'; + $this->assertDebuggingCalled($message, DEBUG_DEVELOPER); + $this->resetDebugging(); + $this->quba->preload_all_step_users(); + $result = $this->quba->render_question($this->slot, $displayoptions); + $this->assertDebuggingNotCalled(); // The step just show the user profile link if the step's userid is different with student id. preg_match_all("/(.*)<\/a>/", $result, $matches);