MDL-80087 Questions: warn if preload_all_step_users was not called

Co-authored-by: Tim Hunt <[email protected]>

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();

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']);

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,20 +99,20 @@ 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;
 
     /**
      * You should not need to call this constructor in your own code. Steps are
      * 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. */

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()]);
                 }
             }
         }

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);
         }
     }
 

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 ?.*>(.*)<\/a>/", $result, $matches);