MDL-75210 mod_quiz: fix sequential quiz access

* As a student I should only be able to access the next question or the current question
* The API should not show more than the current question
* Once the quiz is previewed we can see the question in any order (existing behaviour)
* Related to ticket MDL-71728

Co-authored-by: Rajneel Totaram <rjnlfj@yahoo.com>
Co-authored-by: Tim Hunt <t.j.hunt@open.ac.uk>

mod/quiz/attemptlib.php

@@ -2502,19 +2502,25 @@ public function process_attempt($timenow, $finishattempt, $timeup, $thispage) {
     }
 
     /**
-     * Check a page access to see if is an out of sequence access.
+     * Check a page read access to see if is an out of sequence access.
      *
-     * @param  int $page page number.
-     * @return boolean false is is an out of sequence access, true otherwise.
+     * If allownext is set then we also check whether access to the page
+     * after the current one should be permitted.
+     *
+     * @param int $page page number.
+     * @param bool $allownext in case of a sequential navigation, can we go to next page ?
+     * @return boolean false is an out of sequence access, true otherwise.
      * @since Moodle 3.1
      */
-    public function check_page_access($page) {
-        if ($this->get_currentpage() != $page) {
-            if ($this->get_navigation_method() == QUIZ_NAVMETHOD_SEQ && $this->get_currentpage() > $page) {
-                return false;
-            }
+    public function check_page_access(int $page, bool $allownext = true): bool {
+        if ($this->get_navigation_method() != QUIZ_NAVMETHOD_SEQ) {
+            return true;
         }
-        return true;
+        // Sequential access: allow access to the summary, current page or next page.
+        // Or if the user review his/her attempt, see MDLQA-1523.
+        return $page == -1
+            || $page == $this->get_currentpage()
+            || $allownext && ($page == $this->get_currentpage() + 1);
     }
 
     /**

mod/quiz/classes/external.php

@@ -1009,8 +1009,9 @@ private static function get_attempt_questions_data(quiz_attempt $attemptobj, $re
             if ($displayoptions->marks >= question_display_options::MARK_AND_MAX) {
                 $question['mark'] = $attemptobj->get_question_mark($slot);
             }
-
-            $questions[] = $question;
+            if ($attemptobj->check_page_access($attemptobj->get_question_page($slot), false)) {
+                $questions[] = $question;
+            }
         }
         return $questions;
     }

mod/quiz/tests/attempt_test.php

@@ -40,9 +40,10 @@ class mod_quiz_attempt_testcase extends advanced_testcase {
      * Create quiz and attempt data with layout.
      *
      * @param string $layout layout to set. Like quiz attempt.layout. E.g. '1,2,0,3,4,0,'.
+     * @param string $navmethod quiz navigation method (defaults to free)
      * @return quiz_attempt the new quiz_attempt object
      */
-    protected function create_quiz_and_attempt_with_layout($layout) {
+    protected function create_quiz_and_attempt_with_layout($layout, $navmethod = QUIZ_NAVMETHOD_FREE) {
         $this->resetAfterTest(true);
 
         // Make a user to do the quiz.
@@ -51,7 +52,7 @@ protected function create_quiz_and_attempt_with_layout($layout) {
         // Make a quiz.
         $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz');
         $quiz = $quizgenerator->create_instance(['course' => $course->id,
-            'grade' => 100.0, 'sumgrades' => 2, 'layout' => $layout]);
+            'grade' => 100.0, 'sumgrades' => 2, 'layout' => $layout, 'navmethod' => $navmethod]);
 
         $quizobj = quiz::create($quiz->id, $user->id);
 
@@ -377,4 +378,58 @@ public function test_quiz_prepare_and_start_new_attempt() {
         $step = $quba->get_question_attempt(1)->get_step(0);
         $this->assertEquals($student1->id, $step->get_user_id());
     }
+
+    /**
+     * Test check_page_access function
+     * @covers \quiz_attempt::check_page_access
+     */
+    public function test_check_page_access() {
+        $timenow = time();
+
+        // Free navigation.
+        $attempt = $this->create_quiz_and_attempt_with_layout('1,0,2,0,3,0,4,0,5,0', QUIZ_NAVMETHOD_FREE);
+
+        // Check access.
+        $this->assertTrue($attempt->check_page_access(4));
+        $this->assertTrue($attempt->check_page_access(3));
+        $this->assertTrue($attempt->check_page_access(2));
+        $this->assertTrue($attempt->check_page_access(1));
+        $this->assertTrue($attempt->check_page_access(0));
+        $this->assertTrue($attempt->check_page_access(2));
+
+        // Access page 2.
+        $attempt->set_currentpage(2);
+        $attempt = quiz_attempt::create($attempt->get_attempt()->id);
+
+        // Check access.
+        $this->assertTrue($attempt->check_page_access(0));
+        $this->assertTrue($attempt->check_page_access(1));
+        $this->assertTrue($attempt->check_page_access(2));
+        $this->assertTrue($attempt->check_page_access(3));
+        $this->assertTrue($attempt->check_page_access(4));
+
+        // Sequential navigation.
+        $attempt = $this->create_quiz_and_attempt_with_layout('1,0,2,0,3,0,4,0,5,0', QUIZ_NAVMETHOD_SEQ);
+
+        // Check access.
+        $this->assertTrue($attempt->check_page_access(0));
+        $this->assertTrue($attempt->check_page_access(1));
+        $this->assertFalse($attempt->check_page_access(2));
+        $this->assertFalse($attempt->check_page_access(3));
+        $this->assertFalse($attempt->check_page_access(4));
+
+        // Access page 1.
+        $attempt->set_currentpage(1);
+        $attempt = quiz_attempt::create($attempt->get_attempt()->id);
+        $this->assertTrue($attempt->check_page_access(1));
+
+        // Access page 2.
+        $attempt->set_currentpage(2);
+        $attempt = quiz_attempt::create($attempt->get_attempt()->id);
+        $this->assertTrue($attempt->check_page_access(2));
+
+        $this->assertTrue($attempt->check_page_access(3));
+        $this->assertFalse($attempt->check_page_access(4));
+        $this->assertFalse($attempt->check_page_access(1));
+    }
 }

mod/quiz/tests/behat/attempt_sequential.feature

@@ -0,0 +1,94 @@
+@mod @mod_quiz
+Feature: Attempt a quiz in a sequential mode
+  As a student I should not be able to see the previous questions
+
+  Background:
+    Given the following "users" exist:
+      | username | firstname | lastname | email               |
+      | student  | Student   | One      | student@example.com |
+      | teacher  | Teacher   | One      | teacher@example.com |
+    And the following "courses" exist:
+      | fullname | shortname | category |
+      | Course 1 | C1        | 0        |
+    And the following "course enrolments" exist:
+      | user    | course | role    |
+      | student | C1     | student |
+      | teacher | C1     | teacher |
+    And the following "question categories" exist:
+      | contextlevel | reference | name           |
+      | Course       | C1        | Test questions |
+    And the following "questions" exist:
+      | questioncategory | qtype     | name | questiontext    |
+      | Test questions   | truefalse | TF1  | First question  |
+      | Test questions   | truefalse | TF2  | Second question |
+      | Test questions   | truefalse | TF3  | Third question  |
+      | Test questions   | truefalse | TF4  | Fourth question |
+    And the following "activities" exist:
+      | activity | name   | intro              | course | idnumber | preferredbehaviour | navmethod  |
+      | quiz     | Quiz 1 | Quiz 1 description | C1     | quiz1    | immediatefeedback  | sequential |
+    And quiz "Quiz 1" contains the following questions:
+      | question | page | requireprevious |
+      | TF1      | 1    | 1               |
+      | TF2      | 2    | 1               |
+      | TF3      | 3    | 1               |
+      | TF4      | 4    | 1               |
+
+  @javascript
+  Scenario Outline: As a student I should not be able to navigate out of sequence if sequential navigation is on.
+    Given I am on the "Quiz 1" "mod_quiz > View" page logged in as "student"
+    And I press "Attempt quiz"
+    And I should see "First question"
+    When I am on the "Quiz 1 > student > Attempt 1 > <pagenumber>" "mod_quiz > Attempt view" page
+    And I should see "<canseequestion>"
+    Then I should not see "<cannotseequestion>"
+    Examples:
+      | pagenumber | canseequestion  | cannotseequestion |
+      | 1          | First question  | Second question   |
+      | 2          | Second question | First question    |
+      | 4          | First question  | Fourth question   |
+
+  @javascript
+  Scenario: As a student I should not be able to navigate out of sequence by opening new windows on the same quiz.
+    Given the following config values are set as admin:
+      | config         | value | plugin |
+      | autosaveperiod | 60    | quiz   |
+    And I am on the "Quiz 1" "mod_quiz > View" page logged in as "student"
+    And I press "Attempt quiz"
+    And I should see "First question"
+    And I click on "True" "radio" in the "First question" "question"
+    And I click on "Next page" "button"
+    When I am on the "Quiz 1 > student > Attempt 1 > 3" "mod_quiz > Attempt view" page
+    And I click on "True" "radio" in the "Third question" "question"
+    And I should see "Third question"
+    And I click on "Next page" "button"
+    And I am on the "Quiz 1 > student > Attempt 1 > 1" "mod_quiz > Attempt view" page
+    Then I should see "Fourth question"
+
+  @javascript
+  Scenario: As a student I should not be able to save my data by opening a given page out of sequence.
+    Given the following config values are set as admin:
+      | config         | value | plugin |
+      | autosaveperiod | 1     | quiz   |
+    When I am on the "Quiz 1" "mod_quiz > View" page logged in as "student"
+    And I press "Attempt quiz"
+    And I am on the "Quiz 1 > student > Attempt 1 > 2" "mod_quiz > Attempt view" page
+    And I should see "Second question"
+    And I click on "True" "radio" in the "Second question" "question"
+    And I wait "2" seconds
+    And I am on the "Quiz 1 > student > Attempt 1 > 1" "mod_quiz > Attempt view" page
+    Then I should see "Second question"
+
+  @javascript
+  Scenario: As a student I can review question I have finished in any order
+    Given user "student" has attempted "Quiz 1" with responses:
+      | slot | response |
+      |   1  | True     |
+      |   2  | False    |
+      |   3  | False    |
+      |   4  | False    |
+    When I am on the "Quiz 1" "mod_quiz > View" page logged in as "student"
+    And I follow "Review"
+    And I am on the "Quiz 1 > student > Attempt 1 > 3" "mod_quiz > Attempt view" page
+    And I should see "Third question"
+    And I am on the "Quiz 1 > student > Attempt 1 > 2" "mod_quiz > Attempt view" page
+    Then I should see "Second question"

mod/quiz/tests/behat/behat_mod_quiz.php

@@ -112,7 +112,21 @@ protected function resolve_page_instance_url(string $type, string $identifier):
             case 'manual grading report':
                 return new moodle_url('/mod/quiz/report.php',
                         ['id' => $this->get_cm_by_quiz_name($identifier)->id, 'mode' => 'grading']);
-
+            case 'attempt view':
+                list($quizname, $username, $attemptno, $pageno) = explode(' > ', $identifier);
+                $pageno = intval($pageno);
+                $pageno = $pageno > 0 ? $pageno - 1 : 0;
+                $attemptno = (int) trim(str_replace ('Attempt', '', $attemptno));
+                $quiz = $this->get_quiz_by_name($quizname);
+                $quizcm = get_coursemodule_from_instance('quiz', $quiz->id, $quiz->course);
+                $user = $DB->get_record('user', ['username' => $username], '*', MUST_EXIST);
+                $attempt = $DB->get_record('quiz_attempts',
+                    ['quiz' => $quiz->id, 'userid' => $user->id, 'attempt' => $attemptno], '*', MUST_EXIST);
+                return new moodle_url('/mod/quiz/attempt.php', [
+                    'attempt' => $attempt->id,
+                    'cmid' => $quizcm->id,
+                    'page' => $pageno
+                ]);
             case 'attempt review':
                 if (substr_count($identifier, ' > ') !== 2) {
                     throw new coding_exception('For "attempt review", name must be ' .

mod/quiz/tests/external/external_test.php

@@ -1625,7 +1625,7 @@ public function test_view_attempt() {
         $this->assertEventContextNotUsed($event);
         $this->assertNotEmpty($event->get_name());
 
-        // Now, force the quiz with QUIZ_NAVMETHOD_SEQ (sequencial) navigation method.
+        // Now, force the quiz with QUIZ_NAVMETHOD_SEQ (sequential) navigation method.
         $DB->set_field('quiz', 'navmethod', QUIZ_NAVMETHOD_SEQ, array('id' => $quiz->id));
         // Quiz requiring preflightdata.
         $DB->set_field('quiz', 'password', 'abcdef', array('id' => $quiz->id));
@@ -2013,4 +2013,137 @@ public function test_get_quiz_required_qtypes_random() {
 
         $this->assertEquals($expected, $result['questiontypes']);
     }
+
+    /**
+     * Test that a sequential navigation quiz is not allowing to see questions in advance except if reviewing
+     */
+    public function test_sequential_navigation_view_attempt() {
+        // Test user with full capabilities.
+        $quiz = $this->prepare_sequential_quiz();
+        $attemptobj = $this->create_quiz_attempt_object($quiz);
+        $this->setUser($this->student);
+        // Check out of sequence access for view.
+        $this->assertNotEmpty(mod_quiz_external::view_attempt($attemptobj->get_attemptid(), 0, []));
+        try {
+            mod_quiz_external::view_attempt($attemptobj->get_attemptid(), 3, []);
+            $this->fail('Exception expected due to out of sequence access.');
+        } catch (\moodle_exception $e) {
+            $this->assertStringContainsString('quiz/Out of sequence access', $e->getMessage());
+        }
+    }
+
+    /**
+     * Test that a sequential navigation quiz is not allowing to see questions in advance for a student
+     */
+    public function test_sequential_navigation_attempt_summary() {
+        // Test user with full capabilities.
+        $quiz = $this->prepare_sequential_quiz();
+        $attemptobj = $this->create_quiz_attempt_object($quiz);
+        $this->setUser($this->student);
+        // Check that we do not return other questions than the one currently viewed.
+        $result = mod_quiz_external::get_attempt_summary($attemptobj->get_attemptid());
+        $this->assertCount(1, $result['questions']);
+        $this->assertStringContainsString('Question (1)', $result['questions'][0]['html']);
+    }
+
+    /**
+     * Test that a sequential navigation quiz is not allowing to see questions in advance for student
+     */
+    public function test_sequential_navigation_get_attempt_data() {
+        // Test user with full capabilities.
+        $quiz = $this->prepare_sequential_quiz();
+        $attemptobj = $this->create_quiz_attempt_object($quiz);
+        $this->setUser($this->student);
+        // Test invalid instance id.
+        try {
+            mod_quiz_external::get_attempt_data($attemptobj->get_attemptid(), 2);
+            $this->fail('Exception expected due to out of sequence access.');
+        } catch (\moodle_exception $e) {
+            $this->assertStringContainsString('quiz/Out of sequence access', $e->getMessage());
+        }
+        // Now we moved to page 1, we should see page 2 and 1 but not 0 or 3.
+        $attemptobj->set_currentpage(1);
+        // Test invalid instance id.
+        try {
+            mod_quiz_external::get_attempt_data($attemptobj->get_attemptid(), 0);
+            $this->fail('Exception expected due to out of sequence access.');
+        } catch (\moodle_exception $e) {
+            $this->assertStringContainsString('quiz/Out of sequence access', $e->getMessage());
+        }
+
+        try {
+            mod_quiz_external::get_attempt_data($attemptobj->get_attemptid(), 3);
+            $this->fail('Exception expected due to out of sequence access.');
+        } catch (\moodle_exception $e) {
+            $this->assertStringContainsString('quiz/Out of sequence access', $e->getMessage());
+        }
+
+        // Now we can see page 1.
+        $result = mod_quiz_external::get_attempt_data($attemptobj->get_attemptid(), 1);
+        $this->assertCount(1, $result['questions']);
+        $this->assertStringContainsString('Question (2)', $result['questions'][0]['html']);
+    }
+
+    /**
+     * Prepare quiz for sequential navigation tests
+     *
+     * @return quiz
+     */
+    private function prepare_sequential_quiz(): quiz {
+        // Create a new quiz with 5 questions and one attempt started.
+        // Create a new quiz with attempts.
+        $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz');
+        $data = [
+            'course' => $this->course->id,
+            'sumgrades' => 2,
+            'preferredbehaviour' => 'deferredfeedback',
+            'navmethod' => QUIZ_NAVMETHOD_SEQ
+        ];
+        $quiz = $quizgenerator->create_instance($data);
+
+        // Now generate the questions.
+        $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question');
+        $cat = $questiongenerator->create_question_category();
+        for ($pageindex = 1; $pageindex <= 5; $pageindex++) {
+            $question = $questiongenerator->create_question('truefalse', null, [
+                'category' => $cat->id,
+                'questiontext' => ['text' => "Question ($pageindex)"]
+            ]);
+            quiz_add_quiz_question($question->id, $quiz, $pageindex);
+        }
+
+        $quizobj = quiz::create($quiz->id, $this->student->id);
+        // Set grade to pass.
+        $item = \grade_item::fetch(array('courseid' => $this->course->id, 'itemtype' => 'mod',
+            'itemmodule' => 'quiz', 'iteminstance' => $quiz->id, 'outcomeid' => null));
+        $item->gradepass = 80;
+        $item->update();
+        return $quizobj;
+    }
+
+    /**
+     * Create question attempt
+     *
+     * @param quiz $quizobj
+     * @param int|null $userid
+     * @param bool|null $ispreview
+     * @return quiz_attempt
+     * @throws \moodle_exception
+     */
+    private function create_quiz_attempt_object(quiz $quizobj, ?int $userid = null, ?bool $ispreview = false): quiz_attempt {
+        global $USER;
+        $timenow = time();
+        // Now, do one attempt.
+        $quba = \question_engine::make_questions_usage_by_activity('mod_quiz', $quizobj->get_context());
+        $quba->set_preferred_behaviour($quizobj->get_quiz()->preferredbehaviour);
+        $attemptnumber = 1;
+        if (!empty($USER->id)) {
+            $attemptnumber = count(quiz_get_user_attempts($quizobj->get_quizid(), $USER->id)) + 1;
+        }
+        $attempt = quiz_create_attempt($quizobj, $attemptnumber, false, $timenow, $ispreview, $userid ?? $this->student->id);
+        quiz_start_new_attempt($quizobj, $quba, $attempt, $attemptnumber, $timenow);
+        quiz_attempt_save_started($quizobj, $quba, $attempt);
+        $attemptobj = quiz_attempt::create($attempt->id);
+        return $attemptobj;
+    }
 }