From f4695ec722b80e86abca08aceba8f30f9167aab2 Mon Sep 17 00:00:00 2001 From: dinhloc Date: Tue, 9 Jan 2018 09:38:54 +0700 Subject: [PATCH] MDL-61033 quiz: Editing a quiz while a preview is open Editing a quiz while a preview is open in another browser tab can leads to a cryptic error that is hard to recover from --- mod/quiz/attempt.php | 3 +- mod/quiz/attemptlib.php | 9 +++-- mod/quiz/autosave.ajax.php | 3 +- mod/quiz/comment.php | 3 +- mod/quiz/lang/en/quiz.php | 3 ++ mod/quiz/locallib.php | 34 +++++++++++++++- mod/quiz/processattempt.php | 3 +- mod/quiz/renderer.php | 4 +- mod/quiz/review.php | 3 +- mod/quiz/reviewquestion.php | 3 +- mod/quiz/summary.php | 3 +- mod/quiz/tests/attempt_test.php | 62 ++++++++++++++--------------- mod/quiz/tests/attempts_test.php | 67 ++++++++++++++++++++++++++++++++ 13 files changed, 156 insertions(+), 44 deletions(-) diff --git a/mod/quiz/attempt.php b/mod/quiz/attempt.php index 96b8e1c0950..f1c26ab3e81 100644 --- a/mod/quiz/attempt.php +++ b/mod/quiz/attempt.php @@ -39,8 +39,9 @@ if ($id = optional_param('id', 0, PARAM_INT)) { // Get submitted parameters. $attemptid = required_param('attempt', PARAM_INT); $page = optional_param('page', 0, PARAM_INT); +$cmid = optional_param('cmid', null, PARAM_INT); -$attemptobj = quiz_attempt::create($attemptid); +$attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); $page = $attemptobj->force_page_number_into_range($page); $PAGE->set_url($attemptobj->attempt_url(null, $page)); diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index 8658d505830..4ed0d760ef3 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -340,6 +340,7 @@ class quiz { if ($page) { $url .= '&page=' . $page; } + $url .= '&cmid=' . $this->get_cmid(); return $url; } @@ -359,7 +360,7 @@ class quiz { * @return string the URL of the review of that attempt. */ public function review_url($attemptid) { - return new moodle_url('/mod/quiz/review.php', array('attempt' => $attemptid)); + return new moodle_url('/mod/quiz/review.php', array('attempt' => $attemptid, 'cmid' => $this->get_cmid())); } /** @@ -367,7 +368,7 @@ class quiz { * @return string the URL of the review of that attempt. */ public function summary_url($attemptid) { - return new moodle_url('/mod/quiz/summary.php', array('attempt' => $attemptid)); + return new moodle_url('/mod/quiz/summary.php', array('attempt' => $attemptid, 'cmid' => $this->get_cmid())); } // Bits of content ========================================================= @@ -1400,7 +1401,7 @@ class quiz_attempt { * @return string the URL of this quiz's summary page. */ public function summary_url() { - return new moodle_url('/mod/quiz/summary.php', array('attempt' => $this->attempt->id)); + return new moodle_url('/mod/quiz/summary.php', array('attempt' => $this->attempt->id, 'cmid' => $this->get_cmid())); } /** @@ -2104,7 +2105,7 @@ class quiz_attempt { } else { $url = new moodle_url('/mod/quiz/' . $script . '.php' . $fragment, - array('attempt' => $this->attempt->id)); + array('attempt' => $this->attempt->id, 'cmid' => $this->get_cmid())); if ($page == 0 && $showall != $defaultshowall) { $url->param('showall', (int) $showall); } else if ($page > 0) { diff --git a/mod/quiz/autosave.ajax.php b/mod/quiz/autosave.ajax.php index 412bb3dc713..ebf34994ff8 100644 --- a/mod/quiz/autosave.ajax.php +++ b/mod/quiz/autosave.ajax.php @@ -35,9 +35,10 @@ require_sesskey(); // Get submitted parameters. $attemptid = required_param('attempt', PARAM_INT); $thispage = optional_param('thispage', 0, PARAM_INT); +$cmid = optional_param('cmid', null, PARAM_INT); $transaction = $DB->start_delegated_transaction(); -$attemptobj = quiz_attempt::create($attemptid); +$attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); // Check login. require_login($attemptobj->get_course(), false, $attemptobj->get_cm()); diff --git a/mod/quiz/comment.php b/mod/quiz/comment.php index cfcebb2d66e..9ff4acc6e75 100644 --- a/mod/quiz/comment.php +++ b/mod/quiz/comment.php @@ -28,10 +28,11 @@ require_once('locallib.php'); $attemptid = required_param('attempt', PARAM_INT); $slot = required_param('slot', PARAM_INT); // The question number in the attempt. +$cmid = optional_param('cmid', null, PARAM_INT); $PAGE->set_url('/mod/quiz/comment.php', array('attempt' => $attemptid, 'slot' => $slot)); -$attemptobj = quiz_attempt::create($attemptid); +$attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); $student = $DB->get_record('user', array('id' => $attemptobj->get_userid())); // Can only grade finished attempts. diff --git a/mod/quiz/lang/en/quiz.php b/mod/quiz/lang/en/quiz.php index 6f88c803e08..65b19d5ff1d 100644 --- a/mod/quiz/lang/en/quiz.php +++ b/mod/quiz/lang/en/quiz.php @@ -90,6 +90,9 @@ $string['attemptclosed'] = 'Attempt has not closed yet'; $string['attemptduration'] = 'Time taken'; $string['attemptedon'] = 'Attempted on'; $string['attempterror'] = 'You are not allowed to attempt this quiz at this time because: {$a}'; +$string['attempterrorinvalid'] = 'Invalid quiz attempt ID'; +$string['attempterrorcontentchange'] = 'This quiz attempt no longer exists. (When a quiz is edited in-progress previews are automatically deleted.)'; +$string['attempterrorcontentchangeforuser'] = 'This quiz attempt no longer exists.'; $string['attemptfirst'] = 'First attempt'; $string['attemptincomplete'] = 'That attempt (by {$a}) is not yet completed.'; $string['attemptlast'] = 'Last attempt'; diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index 9350ec95985..3c70f2da2a9 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -2516,4 +2516,36 @@ function quiz_extract_random_question_tag_ids($tagsjson, $matchbyid = true) { // Only work with tags that exist. return array_filter(array_column($tags, 'id')); -} \ No newline at end of file +} + +/** + * Get quiz attempt and handling error. + * + * @param int $attemptid the id of the current attempt. + * @param int|null $cmid the course_module id for this quiz. + * @return quiz_attempt $attemptobj all the data about the quiz attempt. + * @throws moodle_exception + */ +function quiz_create_attempt_handling_errors($attemptid, $cmid = null) { + try { + $attempobj = quiz_attempt::create($attemptid); + } catch (moodle_exception $e) { + if (!empty($cmid)) { + list($course, $cm) = get_course_and_cm_from_cmid($cmid, 'quiz'); + $continuelink = new moodle_url('/mod/quiz/view.php', array('id' => $cmid)); + $context = context_module::instance($cm->id); + if (has_capability('mod/quiz:preview', $context)) { + throw new moodle_exception('attempterrorcontentchange', 'quiz', $continuelink); + } else { + throw new moodle_exception('attempterrorcontentchangeforuser', 'quiz', $continuelink); + } + } else { + throw new moodle_exception('attempterrorinvalid', 'quiz'); + } + } + if (!empty($cmid) && $attempobj->get_cmid() != $cmid) { + throw new moodle_exception('invalidcoursemodule'); + } else { + return $attempobj; + } +} diff --git a/mod/quiz/processattempt.php b/mod/quiz/processattempt.php index ee5c3e51d26..50dba84d6ee 100644 --- a/mod/quiz/processattempt.php +++ b/mod/quiz/processattempt.php @@ -44,8 +44,9 @@ $next = optional_param('next', false, PARAM_BOOL); $finishattempt = optional_param('finishattempt', false, PARAM_BOOL); $timeup = optional_param('timeup', 0, PARAM_BOOL); // True if form was submitted by timer. $scrollpos = optional_param('scrollpos', '', PARAM_RAW); +$cmid = optional_param('cmid', null, PARAM_INT); -$attemptobj = quiz_attempt::create($attemptid); +$attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); // Set $nexturl now. if ($next) { diff --git a/mod/quiz/renderer.php b/mod/quiz/renderer.php index a2343393746..e9fc84c6f1d 100644 --- a/mod/quiz/renderer.php +++ b/mod/quiz/renderer.php @@ -479,7 +479,8 @@ class mod_quiz_renderer extends plugin_renderer_base { // Start the form. $output .= html_writer::start_tag('form', - array('action' => $attemptobj->processattempt_url(), 'method' => 'post', + array('action' => new moodle_url($attemptobj->processattempt_url(), + array('cmid' => $attemptobj->get_cmid())), 'method' => 'post', 'enctype' => 'multipart/form-data', 'accept-charset' => 'utf-8', 'id' => 'responseform')); $output .= html_writer::start_tag('div'); @@ -731,6 +732,7 @@ class mod_quiz_renderer extends plugin_renderer_base { 'finishattempt' => 1, 'timeup' => 0, 'slots' => '', + 'cmid' => $attemptobj->get_cmid(), 'sesskey' => sesskey(), ); diff --git a/mod/quiz/review.php b/mod/quiz/review.php index 1fa481c06de..28e94b0b14f 100644 --- a/mod/quiz/review.php +++ b/mod/quiz/review.php @@ -33,6 +33,7 @@ require_once($CFG->dirroot . '/mod/quiz/report/reportlib.php'); $attemptid = required_param('attempt', PARAM_INT); $page = optional_param('page', 0, PARAM_INT); $showall = optional_param('showall', null, PARAM_BOOL); +$cmid = optional_param('cmid', null, PARAM_INT); $url = new moodle_url('/mod/quiz/review.php', array('attempt'=>$attemptid)); if ($page !== 0) { @@ -42,7 +43,7 @@ if ($page !== 0) { } $PAGE->set_url($url); -$attemptobj = quiz_attempt::create($attemptid); +$attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); $page = $attemptobj->force_page_number_into_range($page); // Now we can validate the params better, re-genrate the page URL. diff --git a/mod/quiz/reviewquestion.php b/mod/quiz/reviewquestion.php index 0ee6970aa04..91313890428 100644 --- a/mod/quiz/reviewquestion.php +++ b/mod/quiz/reviewquestion.php @@ -30,6 +30,7 @@ require_once('locallib.php'); $attemptid = required_param('attempt', PARAM_INT); $slot = required_param('slot', PARAM_INT); $seq = optional_param('step', null, PARAM_INT); +$cmid = optional_param('cmid', null, PARAM_INT); $baseurl = new moodle_url('/mod/quiz/reviewquestion.php', array('attempt' => $attemptid, 'slot' => $slot)); @@ -39,7 +40,7 @@ if (!is_null($seq)) { } $PAGE->set_url($currenturl); -$attemptobj = quiz_attempt::create($attemptid); +$attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); // Check login. require_login($attemptobj->get_course(), false, $attemptobj->get_cm()); diff --git a/mod/quiz/summary.php b/mod/quiz/summary.php index 0860a3f00d7..1d930456cf1 100644 --- a/mod/quiz/summary.php +++ b/mod/quiz/summary.php @@ -27,10 +27,11 @@ require_once(__DIR__ . '/../../config.php'); require_once($CFG->dirroot . '/mod/quiz/locallib.php'); $attemptid = required_param('attempt', PARAM_INT); // The attempt to summarise. +$cmid = optional_param('cmid', null, PARAM_INT); $PAGE->set_url('/mod/quiz/summary.php', array('attempt' => $attemptid)); -$attemptobj = quiz_attempt::create($attemptid); +$attemptobj = quiz_create_attempt_handling_errors($attemptid, $cmid); // Check login. require_login($attemptobj->get_course(), false, $attemptobj->get_cm()); diff --git a/mod/quiz/tests/attempt_test.php b/mod/quiz/tests/attempt_test.php index e4ffd371cd6..0b31121d992 100644 --- a/mod/quiz/tests/attempt_test.php +++ b/mod/quiz/tests/attempt_test.php @@ -105,19 +105,19 @@ class mod_quiz_attempt_testcase extends basic_testcase { // Attempt pages. $this->assertEquals(new moodle_url( - '/mod/quiz/attempt.php?attempt=123'), + '/mod/quiz/attempt.php?attempt=123&cmid=0'), $attempt->attempt_url()); $this->assertEquals(new moodle_url( - '/mod/quiz/attempt.php?attempt=123&page=2'), + '/mod/quiz/attempt.php?attempt=123&page=2&cmid=0'), $attempt->attempt_url(null, 2)); $this->assertEquals(new moodle_url( - '/mod/quiz/attempt.php?attempt=123&page=1#'), + '/mod/quiz/attempt.php?attempt=123&page=1&cmid=0#'), $attempt->attempt_url(3)); $this->assertEquals(new moodle_url( - '/mod/quiz/attempt.php?attempt=123&page=1#q4'), + '/mod/quiz/attempt.php?attempt=123&page=1&cmid=0#q4'), $attempt->attempt_url(4)); $this->assertEquals(new moodle_url( @@ -134,52 +134,52 @@ class mod_quiz_attempt_testcase extends basic_testcase { // Summary page. $this->assertEquals(new moodle_url( - '/mod/quiz/summary.php?attempt=123'), + '/mod/quiz/summary.php?attempt=123&cmid=0'), $attempt->summary_url()); // Review page. $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123'), + '/mod/quiz/review.php?attempt=123&cmid=0'), $attempt->review_url()); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&page=2'), + '/mod/quiz/review.php?attempt=123&page=2&cmid=0'), $attempt->review_url(null, 2)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&page=1'), + '/mod/quiz/review.php?attempt=123&page=1&cmid=0'), $attempt->review_url(3, -1, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&page=1#q4'), + '/mod/quiz/review.php?attempt=123&page=1&cmid=0#q4'), $attempt->review_url(4, -1, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123'), + '/mod/quiz/review.php?attempt=123&cmid=0'), $attempt->review_url(null, 2, true)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123'), + '/mod/quiz/review.php?attempt=123&cmid=0'), $attempt->review_url(1, -1, true)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&page=2'), + '/mod/quiz/review.php?attempt=123&page=2&cmid=0'), $attempt->review_url(null, 2, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&showall=0'), + '/mod/quiz/review.php?attempt=123&showall=0&cmid=0'), $attempt->review_url(null, 0, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&showall=0'), + '/mod/quiz/review.php?attempt=123&showall=0&cmid=0'), $attempt->review_url(1, -1, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&page=1'), + '/mod/quiz/review.php?attempt=123&page=1&cmid=0'), $attempt->review_url(3, -1, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&page=2'), + '/mod/quiz/review.php?attempt=123&page=2&cmid=0'), $attempt->review_url(null, 2)); $this->assertEquals(new moodle_url( @@ -203,7 +203,7 @@ class mod_quiz_attempt_testcase extends basic_testcase { $attempt->review_url(1, -1, true, 0)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&page=2'), + '/mod/quiz/review.php?attempt=123&page=2&cmid=0'), $attempt->review_url(null, 2, false, 0)); $this->assertEquals(new moodle_url( @@ -215,7 +215,7 @@ class mod_quiz_attempt_testcase extends basic_testcase { $attempt->review_url(1, -1, false, 0)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=123&page=1#'), + '/mod/quiz/review.php?attempt=123&page=1&cmid=0#'), $attempt->review_url(3, -1, false, 0)); // Review with more than 50 questions in the quiz. @@ -225,47 +225,47 @@ class mod_quiz_attempt_testcase extends basic_testcase { '41,42,43,44,45,46,47,48,49,50,0,51,52,53,54,55,56,57,58,59,60,0'); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124'), + '/mod/quiz/review.php?attempt=124&cmid=0'), $attempt->review_url()); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=2'), + '/mod/quiz/review.php?attempt=124&page=2&cmid=0'), $attempt->review_url(null, 2)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=1'), + '/mod/quiz/review.php?attempt=124&page=1&cmid=0'), $attempt->review_url(11, -1, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=1#q12'), + '/mod/quiz/review.php?attempt=124&page=1&cmid=0#q12'), $attempt->review_url(12, -1, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&showall=1'), + '/mod/quiz/review.php?attempt=124&showall=1&cmid=0'), $attempt->review_url(null, 2, true)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&showall=1'), + '/mod/quiz/review.php?attempt=124&showall=1&cmid=0'), $attempt->review_url(1, -1, true)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=2'), + '/mod/quiz/review.php?attempt=124&page=2&cmid=0'), $attempt->review_url(null, 2, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124'), + '/mod/quiz/review.php?attempt=124&cmid=0'), $attempt->review_url(null, 0, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=1'), + '/mod/quiz/review.php?attempt=124&page=1&cmid=0'), $attempt->review_url(11, -1, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=1#q12'), + '/mod/quiz/review.php?attempt=124&page=1&cmid=0#q12'), $attempt->review_url(12, -1, false)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=2'), + '/mod/quiz/review.php?attempt=124&page=2&cmid=0'), $attempt->review_url(null, 2)); $this->assertEquals(new moodle_url( @@ -289,7 +289,7 @@ class mod_quiz_attempt_testcase extends basic_testcase { $attempt->review_url(1, -1, true, 0)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=2'), + '/mod/quiz/review.php?attempt=124&page=2&cmid=0'), $attempt->review_url(null, 2, false, 0)); $this->assertEquals(new moodle_url( @@ -301,7 +301,7 @@ class mod_quiz_attempt_testcase extends basic_testcase { $attempt->review_url(1, -1, false, 0)); $this->assertEquals(new moodle_url( - '/mod/quiz/review.php?attempt=124&page=1#'), + '/mod/quiz/review.php?attempt=124&page=1&cmid=0#'), $attempt->review_url(11, -1, false, 0)); } } diff --git a/mod/quiz/tests/attempts_test.php b/mod/quiz/tests/attempts_test.php index af8a4e6fbc8..00a638d8ef4 100644 --- a/mod/quiz/tests/attempts_test.php +++ b/mod/quiz/tests/attempts_test.php @@ -387,4 +387,71 @@ class mod_quiz_attempt_overdue_testcase extends advanced_testcase { groups_delete_group_members($course->id); $this->assertEquals(1200, $DB->get_field('quiz_attempts', 'timecheckstate', array('id'=>$attemptid))); } + + /** + * Test the functions quiz_create_attempt_handling_errors + */ + public function test_quiz_create_attempt_handling_errors() { + $this->resetAfterTest(true); + $this->setAdminUser(); + + // Make a quiz. + $course = $this->getDataGenerator()->create_course(); + $user1 = $this->getDataGenerator()->create_user(); + $student = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($student->id, $course->id, 'student'); + $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $quiz = $quizgenerator->create_instance(array('course' => $course->id, 'questionsperpage' => 0, 'grade' => 100.0, + 'sumgrades' => 2)); + // Create questions. + $cat = $questiongenerator->create_question_category(); + $saq = $questiongenerator->create_question('shortanswer', null, array('category' => $cat->id)); + $numq = $questiongenerator->create_question('numerical', null, array('category' => $cat->id)); + // Add them to the quiz. + quiz_add_quiz_question($saq->id, $quiz); + quiz_add_quiz_question($numq->id, $quiz); + $quizobj = quiz::create($quiz->id, $user1->id); + $quba = question_engine::make_questions_usage_by_activity('mod_quiz', $quizobj->get_context()); + $quba->set_preferred_behaviour($quizobj->get_quiz()->preferredbehaviour); + $timenow = time(); + // Create an attempt. + $attempt = quiz_create_attempt($quizobj, 1, false, $timenow, false, $user1->id); + quiz_start_new_attempt($quizobj, $quba, $attempt, 1, $timenow); + quiz_attempt_save_started($quizobj, $quba, $attempt); + $result = quiz_create_attempt_handling_errors($attempt->id, $quiz->cmid); + $this->assertEquals($result->get_attemptid(), $attempt->id); + try { + $result = quiz_create_attempt_handling_errors($attempt->id, 9999); + $this->fail('Exception expected due to invalid course module id.'); + } catch (moodle_exception $e) { + $this->assertEquals('invalidcoursemodule', $e->errorcode); + } + try { + quiz_create_attempt_handling_errors(9999, $result->get_cmid()); + $this->fail('Exception expected due to quiz content change.'); + } catch (moodle_exception $e) { + $this->assertEquals('attempterrorcontentchange', $e->errorcode); + } + try { + quiz_create_attempt_handling_errors(9999); + $this->fail('Exception expected due to invalid quiz attempt id.'); + } catch (moodle_exception $e) { + $this->assertEquals('attempterrorinvalid', $e->errorcode); + } + // Set up as normal user without permission to view preview. + $this->setUser($student->id); + try { + quiz_create_attempt_handling_errors(9999, $result->get_cmid()); + $this->fail('Exception expected due to quiz content change for user without permission.'); + } catch (moodle_exception $e) { + $this->assertEquals('attempterrorcontentchangeforuser', $e->errorcode); + } + try { + quiz_create_attempt_handling_errors($attempt->id, 9999); + $this->fail('Exception expected due to invalid course module id for user without permission.'); + } catch (moodle_exception $e) { + $this->assertEquals('invalidcoursemodule', $e->errorcode); + } + } }