From 025e0297b65e6a8bd61efad0fdf36168c613f918 Mon Sep 17 00:00:00 2001 From: Laurent David Date: Wed, 20 Jul 2022 13:00:18 -0400 Subject: [PATCH] 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 Co-authored-by: Tim Hunt --- mod/quiz/attemptlib.php | 24 ++-- mod/quiz/classes/external.php | 5 +- mod/quiz/tests/attempt_test.php | 59 +++++++- .../tests/behat/attempt_sequential.feature | 94 ++++++++++++ mod/quiz/tests/behat/behat_mod_quiz.php | 16 ++- mod/quiz/tests/external/external_test.php | 135 +++++++++++++++++- mod/quiz/upgrade.txt | 6 +- 7 files changed, 323 insertions(+), 16 deletions(-) create mode 100644 mod/quiz/tests/behat/attempt_sequential.feature diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index 260a6366748..03cc9d11940 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -2502,19 +2502,25 @@ class quiz_attempt { } /** - * 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); } /** diff --git a/mod/quiz/classes/external.php b/mod/quiz/classes/external.php index ea445bb2009..2520e91fe59 100644 --- a/mod/quiz/classes/external.php +++ b/mod/quiz/classes/external.php @@ -1009,8 +1009,9 @@ class mod_quiz_external extends external_api { 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; } diff --git a/mod/quiz/tests/attempt_test.php b/mod/quiz/tests/attempt_test.php index 44ea42e4af5..1dc56438fd6 100644 --- a/mod/quiz/tests/attempt_test.php +++ b/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 @@ class mod_quiz_attempt_testcase extends advanced_testcase { // 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 @@ class mod_quiz_attempt_testcase extends advanced_testcase { $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)); + } } diff --git a/mod/quiz/tests/behat/attempt_sequential.feature b/mod/quiz/tests/behat/attempt_sequential.feature new file mode 100644 index 00000000000..5f620c56d5b --- /dev/null +++ b/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 > " "mod_quiz > Attempt view" page + And I should see "" + Then I should not see "" + 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" diff --git a/mod/quiz/tests/behat/behat_mod_quiz.php b/mod/quiz/tests/behat/behat_mod_quiz.php index 2832a890194..a4f777619d1 100644 --- a/mod/quiz/tests/behat/behat_mod_quiz.php +++ b/mod/quiz/tests/behat/behat_mod_quiz.php @@ -112,7 +112,21 @@ class behat_mod_quiz extends behat_question_base { 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 ' . diff --git a/mod/quiz/tests/external/external_test.php b/mod/quiz/tests/external/external_test.php index 98bdb06062d..90dd1d39d47 100644 --- a/mod/quiz/tests/external/external_test.php +++ b/mod/quiz/tests/external/external_test.php @@ -1625,7 +1625,7 @@ class external_test extends externallib_advanced_testcase { $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 @@ class external_test extends externallib_advanced_testcase { $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; + } } diff --git a/mod/quiz/upgrade.txt b/mod/quiz/upgrade.txt index 17a73e8d847..24afe9e9808 100644 --- a/mod/quiz/upgrade.txt +++ b/mod/quiz/upgrade.txt @@ -1,5 +1,10 @@ This files describes API changes in the quiz code. +=== 4.0.3, 4.1 === + +* If a quiz is sequential the external functions mod_quiz_external::get_attempt_data, mod_quiz_external::get_attempt_summary will not return any data +related to out of sequence question except while reviewing the quiz (MDL-75210). + === 4.0.2, 4.1 === * No external code should be calling quiz_overview_report::regrade_attempt because it is an @@ -7,7 +12,6 @@ This files describes API changes in the quiz code. that the API changed slightly. It now returns an array listing any questions which could not be regraded. - === 4.0 === * The following API methods have a new parameter, $studentisonline, to define whether the student is currently interacting: