diff --git a/lang/en_utf8/quiz.php b/lang/en_utf8/quiz.php index cb4c3f01d41..84beb2b6500 100644 --- a/lang/en_utf8/quiz.php +++ b/lang/en_utf8/quiz.php @@ -90,6 +90,7 @@ $string['cannotloadquestion'] = 'Could not load question options'; $string['cannotopen'] = 'Cannot open export file ($a)'; $string['cannotread'] = 'Cannot read import file (or file is empty)'; $string['cannotrestore'] = 'Could not restore question sessions'; +$string['cannotreviewopen'] = 'You cannot review this attempt, it is still open.'; $string['cannotsavequestion'] = 'Cannot save question list'; $string['cannotwrite'] = 'Cannot write to export file ($a)'; $string['caseno'] = 'No, case is unimportant'; @@ -524,7 +525,6 @@ $string['reviewbefore'] = 'Allow review while quiz is open'; $string['reviewclosed'] = 'After the quiz is closed'; $string['reviewimmediately'] = 'Immediately after the attempt'; $string['reviewnever'] = 'Never allow review'; -$string['reviewnotallowed'] = 'You are not allowed to review other users\' attempts at this quiz.'; $string['reviewofattempt'] = 'Review of attempt $a'; $string['reviewofpreview'] = 'Review of preview'; $string['reviewopen'] = 'Later, while the quiz is still open'; diff --git a/lib/questionlib.php b/lib/questionlib.php index 9e7f56ad071..300dc0b261b 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -37,6 +37,10 @@ define('QUESTION_EVENTMANUALGRADE', '9'); // Grade was entered by teacher define('QUESTION_EVENTS_GRADED', QUESTION_EVENTGRADE.','. QUESTION_EVENTCLOSEANDGRADE.','. QUESTION_EVENTMANUALGRADE); +global $QUESTION_EVENTS_GRADED; +$QUESTION_EVENTS_GRADED = array(QUESTION_EVENTGRADE, QUESTION_EVENTCLOSEANDGRADE, + QUESTION_EVENTMANUALGRADE); + /**#@-*/ /**#@+ @@ -792,6 +796,12 @@ function question_preload_questions($questionids, $extrafields = '', $join = '', $question->_partiallyloaded = true; } + // Note, a possible optimisation here would be to not load the TEXT fields + // (that is, questiontext and generalfeedback) here, and instead load them in + // question_load_questions. That would add one DB query, but reduce the amount + // of data transferred most of the time. I am not going to do this optimisation + // until it is shown to be worthwhile. + return $questions; } @@ -995,6 +1005,53 @@ function get_question_states(&$questions, $cmoptions, $attempt, $lastattemptid = return $states; } +/** + * Load a particular previous state of a question. + * + * @param array $question The question to load the state for. + * @param object $cmoptions Options from the specifica activity module, e.g. $quiz. + * @param object $attempt The attempt for which the question sessions are to be loaded. + * @param integer $stateid The id of a specific state of this question. + * @return object the requested state. False on error. + */ +function question_load_specific_state($question, $cmoptions, $attempt, $stateid) { + global $DB, $QUESTION_EVENTS_GRADED; + // Load specified states for the question. + $sql = 'SELECT st.*, sess.sumpenalty, sess.manualcomment + FROM {question_states} st, {question_sessions} sess + WHERE st.id = ? + AND st.attempt = ? + AND sess.attemptid = st.attempt + AND st.question = ? + AND sess.questionid = st.question'; + $state = $DB->get_record_sql($sql, array($stateid, $attempt->id, $question->id)); + if (!$state) { + return false; + } + restore_question_state($question, $state); + + // Load the most recent graded states for the questions before the specified one. + list($eventinsql, $params) = $DB->get_in_or_equal($QUESTION_EVENTS_GRADED); + $sql = 'SELECT st.*, sess.sumpenalty, sess.manualcomment + FROM {question_states} st, {question_sessions} sess + WHERE st.seq_number <= ? + AND st.attempt = ? + AND sess.attemptid = st.attempt + AND st.question = ? + AND sess.questionid = st.question + AND st.event ' . $eventinsql . + 'ORDER BY st.seq_number DESC'; + $gradedstates = $DB->get_records_sql($sql, array_merge( + array($state->seq_number, $attempt->id, $question->id), $params), 0, 1); + if (empty($gradedstates)) { + $state->last_graded = clone($state); + } else { + $gradedstate = reset($gradedstates); + restore_question_state($question, $gradedstate); + $state->last_graded = $gradedstate; + } + return $state; +} /** * Creates the run-time fields for the states diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index b972db15222..31b03ff3119 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -419,6 +419,16 @@ class quiz_attempt extends quiz { $this->states = $newstates + $this->states; } + public function load_specific_question_state($questionid, $stateid) { + global $DB; + $state = question_load_specific_state($this->questions[$questionid], + $this->quiz, $this->attempt, $stateid); + if ($state === false) { + throw new moodle_quiz_exception($this, 'invalidstateid'); + } + $this->states[$questionid] = $state; + } + // Simple getters ====================================================================== /** @return integer the attempt id. */ public function get_attemptid() { @@ -450,6 +460,20 @@ class quiz_attempt extends quiz { return $this->attempt->preview; } + /** + * Is this a student dealing with their own attempt/teacher previewing, + * or someone with 'mod/quiz:viewreports' reviewing someone elses attempt. + * + * @return boolean whether this situation should be treated as someone looking at their own + * attempt. The distinction normally only matters when an attempt is being reviewed. + */ + public function is_own_attempt() { + global $USER; + return $this->attempt->userid == $USER->id && + (!$this->is_preview_user() || $this->attempt->preview); + + } + public function get_question_state($questionid) { $this->ensure_state_loaded($questionid); return $this->states[$questionid]; @@ -576,12 +600,9 @@ class quiz_attempt extends quiz { * @param $otherattemptid if given, link to another attempt, instead of the one we represent. * @return string the URL to review this attempt. */ - public function review_url($questionid = 0, $page = -1, $showall = false, $otherattemptid = null) { + public function review_url($questionid = 0, $page = -1, $showall = false) { global $CFG; - if (is_null($otherattemptid)) { - $otherattemptid = $this->attempt->id; - } - return $CFG->wwwroot . '/mod/quiz/review.php?attempt=' . $otherattemptid . + return $CFG->wwwroot . '/mod/quiz/review.php?attempt=' . $this->attempt->id . $this->page_and_question_fragment($questionid, $page, $showall); } @@ -619,6 +640,22 @@ class quiz_attempt extends quiz { $panel->display(); } + /// List of all this user's attempts for people who can see reports. + public function links_to_other_attempts($url) { + $search = '/\battempt=' . $this->attempt->id . '\b/'; + $attempts = quiz_get_user_attempts($this->quiz->id, $this->attempt->userid, 'all'); + $attemptlist = array(); + foreach ($attempts as $at) { + if ($at->id == $this->attempt->id) { + $attemptlist[] = '' . $at->attempt . ''; + } else { + $changedurl = preg_replace($search, 'attempt=' . $at->id, $url); + $attemptlist[] = '' . $at->attempt . ''; + } + } + return implode(', ', $attemptlist); + } + // Private methods ===================================================================== // Check that the state of a particular question is loaded, and if not throw an exception. private function ensure_state_loaded($id) { @@ -750,7 +787,7 @@ abstract class quiz_nav_panel_base { abstract protected function get_end_bits(); protected function get_question_state($question) { - $state = 'todo'; // TODO + $state = 'todo'; // TODO MDL-15653 if ($question->_page == $this->page) { $state .= ' thispage'; } diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index be9209536cd..6b845bebbd2 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -821,7 +821,9 @@ function quiz_get_reviewoptions($quiz, $attempt, $context=null) { $options->readonly = true; // Provide the links to the question review and comment script - $options->questionreviewlink = '/mod/quiz/reviewquestion.php'; + if (!empty($attempt->id)) { + $options->questionreviewlink = '/mod/quiz/reviewquestion.php?attempt=' . $attempt->id; + } // Show a link to the comment box only for closed attempts if ($attempt->timefinish && !is_null($context) && has_capability('mod/quiz:grade', $context)) { diff --git a/mod/quiz/report/overview/overview_table.php b/mod/quiz/report/overview/overview_table.php index fbd3ac6d467..ce2e69ef439 100644 --- a/mod/quiz/report/overview/overview_table.php +++ b/mod/quiz/report/overview/overview_table.php @@ -190,6 +190,14 @@ class quiz_report_overview_table extends table_sql { return '-'; } } + + /** + * @param string $colname the name of the column. + * @param object $attempt the row of data - see the SQL in display() in + * mod/quiz/report/overview/report.php to see what fields are present, + * and what they are called. + * @return string the contents of the cell. + */ function other_cols($colname, $attempt){ if (preg_match('/^qsgrade([0-9]+)$/', $colname, $matches)){ @@ -211,8 +219,8 @@ class quiz_report_overview_table extends table_sql { $grade = ''.$oldgrade.'
'. $newgrade; } - return link_to_popup_window('/mod/quiz/reviewquestion.php?state='. - $stateforqinattempt->id.'&number='.$question->number, + return link_to_popup_window('/mod/quiz/reviewquestion.php?attempt=' . + $attempt->attempt . '&question=' . $question->id, 'reviewquestion', $grade, 450, 650, get_string('reviewresponse', 'quiz'), 'none', true); } else { diff --git a/mod/quiz/review.php b/mod/quiz/review.php index b5fd3f90718..be4a2ae184a 100644 --- a/mod/quiz/review.php +++ b/mod/quiz/review.php @@ -19,16 +19,10 @@ /// Check login. require_login($attemptobj->get_courseid(), false, $attemptobj->get_cm()); - /// Create an object to manage all the other (non-roles) access rules. $accessmanager = $attemptobj->get_access_manager(time()); $options = $attemptobj->get_review_options(); -/// Work out if this is a student viewing their own attempt/teacher previewing, -/// or someone with 'mod/quiz:viewreports' reviewing someone elses attempt. - $reviewofownattempt = $attemptobj->get_userid() == $USER->id && - (!$attemptobj->is_preview_user() || $attemptobj->is_preview()); - /// Permissions checks for normal users who do not have quiz:viewreports capability. if (!$attemptobj->has_capability('mod/quiz:viewreports')) { /// Can't review during the attempt - send them back to the attempt page. @@ -36,8 +30,8 @@ redirect($attemptobj->attempt_url(0, $page)); } /// Can't review other users' attempts. - if (!$reviewofownattempt) { - quiz_error($quiz, 'reviewnotallowed'); + if (!$attemptobj->is_own_attempt()) { + quiz_error($quiz, 'notyourattempt'); } /// Can't review unless Students may review -> Responses option is turned on. if (!$options->responses) { @@ -46,10 +40,6 @@ } } -/// Log this review. - add_to_log($attemptobj->get_courseid(), 'quiz', 'review', 'review.php?attempt=' . - $attemptobj->get_attemptid(), $attemptobj->get_quizid(), $attemptobj->get_cmid()); - /// load the questions and states needed by this page. if ($showall) { $questionids = $attemptobj->get_question_ids(); @@ -59,8 +49,12 @@ $attemptobj->load_questions($questionids); $attemptobj->load_question_states($questionids); +/// Log this review. + add_to_log($attemptobj->get_courseid(), 'quiz', 'review', 'review.php?attempt=' . + $attemptobj->get_attemptid(), $attemptobj->get_quizid(), $attemptobj->get_cmid()); + /// Work out appropriate title. - if ($attemptobj->is_preview_user() && $reviewofownattempt) { + if ($attemptobj->is_preview_user() && $attemptobj->is_own_attempt()) { $strreviewtitle = get_string('reviewofpreview', 'quiz'); } else { $strreviewtitle = get_string('reviewofattempt', 'quiz', $attemptobj->get_attempt_number()); @@ -78,7 +72,7 @@ /// Print tabs if they should be there. if ($attemptobj->is_preview_user()) { - if ($reviewofownattempt) { + if ($attemptobj->is_own_attempt()) { $currenttab = 'preview'; } else { $currenttab = 'reports'; @@ -88,8 +82,8 @@ } /// Print heading. - print_heading(format_string($quiz->name)); - if ($attemptobj->is_preview_user() && $reviewofownattempt) { + print_heading(format_string($attemptobj->get_quiz_name())); + if ($attemptobj->is_preview_user() && $attemptobj->is_own_attempt()) { $attemptobj->print_restart_preview_button(); } print_heading($strreviewtitle); @@ -139,20 +133,12 @@ $CFG->wwwroot . '/user/view.php?id=' . $student->id . '&course=' . $attemptobj->get_courseid() . '">' . fullname($student, true) . ''; } - if (has_capability('mod/quiz:viewreports', $context) && - count($attempts = $DB->get_records_select('quiz_attempts', "quiz = ? AND userid = ?", array($quiz->id, $attempt->userid), 'attempt ASC')) > 1) { - /// List of all this user's attempts for people who can see reports. - $attemptlist = array(); - foreach ($attempts as $at) { - if ($at->id == $attempt->id) { - $attemptlist[] = '' . $at->attempt . ''; - } else { - $attemptlist[] = '' . $at->attempt . ''; - } + if ($attemptobj->has_capability('mod/quiz:viewreports')) { + $attemptlist = $attemptobj->links_to_other_attempts($attemptobj->review_url(0, $page, $showall)); + if ($attemptlist) { + $rows[] = '' . get_string('attempts', 'quiz') . + '' . $attemptlist . ''; } - $rows[] = '' . get_string('attempts', 'quiz') . - '' . implode(', ', $attemptlist) . ''; } /// Timing information. diff --git a/mod/quiz/reviewquestion.php b/mod/quiz/reviewquestion.php index 30f4d8455a6..d7b395ed996 100644 --- a/mod/quiz/reviewquestion.php +++ b/mod/quiz/reviewquestion.php @@ -1,8 +1,9 @@ get_courseid(), false, $attemptobj->get_cm()); + +/// Create an object to manage all the other (non-roles) access rules. + $accessmanager = $attemptobj->get_access_manager(time()); + $options = $attemptobj->get_review_options(); + +/// Permissions checks for normal users who do not have quiz:viewreports capability. + if (!$attemptobj->has_capability('mod/quiz:viewreports')) { + /// Can't review during the attempt - send them back to the attempt page. + if (!$attemptobj->is_finished()) { + notify(get_string('cannotreviewopen', 'quiz')); + close_window_button(); + } + /// Can't review other users' attempts. + if (!$attemptobj->is_own_attempt()) { + notify(get_string('notyourattempt', 'quiz')); + close_window_button(); + } + /// Can't review unless Students may review -> Responses option is turned on. + if (!$options->responses) { + notify($accessmanager->cannot_review_message($options)); + close_window_button(); + } + } + +/// Load the questions and states. + $questionids = array($questionid); + $attemptobj->load_questions($questionids); + $attemptobj->load_question_states($questionids); + +/// If it was asked for, load another state, instead of the latest. if ($stateid) { - if (! $state = $DB->get_record('question_states', array('id' => $stateid))) { - print_error('invalidstateid', 'quiz'); - } - if (! $attempt = $DB->get_record('quiz_attempts', array('uniqueid' => $state->attempt))) { - print_error('invalidattemptid', 'quiz'); - } - } elseif ($attemptid) { - if (! $attempt = $DB->get_record('quiz_attempts', array('id' => $attemptid))) { - print_error('invalidattemptid', 'quiz'); - } - if (! $neweststateid = $DB->get_field('question_sessions', 'newest', array('attemptid' => $attempt->uniqueid, 'questionid' => $questionid))) { - // newest_state not set, probably because this is an old attempt from the old quiz module code - if (! $state = $DB->get_record('question_states', array('question' => $questionid, 'attempt' => $attempt->uniqueid))) { - print_error('invalidquestionid', 'quiz'); - } - } else { - if (! $state = $DB->get_record('question_states', array('id' => $neweststateid))) { - print_error('invalidstateid', 'quiz'); - } - } - } else { - print_error('missingparameter'); - } - if (! $question = $DB->get_record('question', array('id' => $state->question))) { - print_error('questionmissing', 'quiz'); - } - if (! $quiz = $DB->get_record('quiz', array('id' => $attempt->quiz))) { - print_error('invalidcoursemodule'); - } - if (! $course = $DB->get_record('course', array('id' => $quiz->course))) { - print_error('coursemisconf'); - } - if (! $cm = get_coursemodule_from_instance('quiz', $quiz->id, $course->id)) { - print_error('invalidcoursemodule'); + $attemptobj->load_specific_question_state($questionid, $stateid); } - require_login($course->id, false, $cm); - $context = get_context_instance(CONTEXT_MODULE, $cm->id); - - if (!has_capability('mod/quiz:viewreports', $context)) { - if (!$attempt->timefinish) { - redirect('attempt.php?attempt='.$attempt->id); - } - // If not even responses are to be shown in review then we - // don't allow any review - if (!($quiz->review & QUIZ_REVIEW_RESPONSES)) { - print_error("noreview", "quiz"); - } - if ((time() - $attempt->timefinish) > 120) { // always allow review right after attempt - if ((!$quiz->timeclose or time() < $quiz->timeclose) and !($quiz->review & QUIZ_REVIEW_OPEN)) { - print_error("noreviewuntil", "quiz", '', userdate($quiz->timeclose)); - } - if ($quiz->timeclose and time() >= $quiz->timeclose and !($quiz->review & QUIZ_REVIEW_CLOSED)) { - print_error("noreview", "quiz"); - } - } - if ($attempt->userid != $USER->id) { - print_error('notyourattempt', 'quiz'); - } - } - - //add_to_log($course->id, 'quiz', 'review', "review.php?id=$cm->id&attempt=$attempt->id", "$quiz->id", "$cm->id"); +/// Log this review. + add_to_log($attemptobj->get_courseid(), 'quiz', 'review', 'reviewquestion.php?attempt=' . + $attemptobj->get_attemptid() . '&question=' . $questionid . + ($stateid ? '&state=' . $stateid : ''), + $attemptobj->get_quizid(), $attemptobj->get_cmid()); /// Print the page header - - $strquizzes = get_string('modulenameplural', 'quiz'); - print_header(); echo ''; // for overlib -/// Print heading - print_heading(format_string($question->name)); - - $question->maxgrade = $DB->get_field('quiz_question_instances', 'grade', array('quiz' => $quiz->id, 'question' => $question->id)); - // Some of the questions code is optimised to work with several questions - // at once so it wants the question to be in an array. - $key = $question->id; - $questions[$key] = &$question; - // Add additional questiontype specific information to the question objects. - if (!get_question_options($questions)) { - print_error('cannotloadtypeinfo', 'quiz'); - } - - $session = $DB->get_record('question_sessions', array('attemptid' => $attempt->uniqueid, 'questionid' => $question->id)); - $state->sumpenalty = $session->sumpenalty; - $state->manualcomment = $session->manualcomment; - restore_question_state($question, $state); - $state->last_graded = $state; - - $options = quiz_get_reviewoptions($quiz, $attempt, $context); - /// Print infobox - $table->align = array("right", "left"); - if ($attempt->userid <> $USER->id) { + $rows = array(); + +/// User picture and name. + if ($attemptobj->get_userid() <> $USER->id) { // Print user picture and name - $student = $DB->get_record('user', array('id' => $attempt->userid)); - $picture = print_user_picture($student, $course->id, $student->picture, false, true); - $table->data[] = array($picture, fullname($student, true)); + $student = $DB->get_record('user', array('id' => $attemptobj->get_userid())); + $picture = print_user_picture($student, $attemptobj->get_courseid(), $student->picture, false, true); + $rows[] = '' . $picture . '' . + fullname($student, true) . ''; } - // print quiz name - $table->data[] = array(get_string('modulename', 'quiz').':', format_string($quiz->name)); - if (has_capability('mod/quiz:viewreports', $context) and - count($attempts = $DB->get_records_select('quiz_attempts', "quiz = ? AND userid =?", array($quiz->id, $attempt->userid), 'attempt ASC')) > 1) { - // print list of attempts - $attemptlist = ''; - foreach ($attempts as $at) { - $attemptlist .= ($at->id == $attempt->id) - ? ''.$at->attempt.', ' - : ''.$at->attempt.', '; + +/// Quiz name. + $rows[] = '' . get_string('modulename', 'quiz') . + '' . format_string($attemptobj->get_quiz_name()) . ''; + +/// Question name. + $rows[] = '' . get_string('question', 'quiz') . + '' . format_string( + $attemptobj->get_question($questionid)->name) . ''; + +/// Other attempts at the quiz. + if ($attemptobj->has_capability('mod/quiz:viewreports')) { + $attemptlist = $attemptobj->links_to_other_attempts( + 'reviewquestion.php?attempt=' . $attemptobj->get_attemptid() . + '&question=' . $questionid); + if ($attemptlist) { + $rows[] = '' . get_string('attempts', 'quiz') . + '' . $attemptlist . ''; } - $table->data[] = array(get_string('attempts', 'quiz').':', trim($attemptlist, ' ,')); - } - if ($state->timestamp) { - // print time stamp - $table->data[] = array(get_string("completedon", "quiz").':', userdate($state->timestamp)); - } - // Print info box unless it is empty - if ($table->data) { - print_table($table); } - print_question($question, $state, $number, $quiz, $options); +/// Timestamp of this action. + $timestamp = $attemptobj->get_question_state($questionid)->timestamp; + if ($timestamp) { + $rows[] = '' . get_string('completedon', 'quiz') . + '' . userdate($timestamp) . ''; + } +/// Now output the summary table, if there are any rows to be shown. + if (!empty($rows)) { + echo '', "\n"; + echo implode("\n", $rows); + echo "\n
\n"; + } + +/// Print the question in the requested state. + $attemptobj->print_question($questionid); + +/// Finish the page print_footer(); - ?> diff --git a/question/type/questiontype.php b/question/type/questiontype.php index fa08a6f1071..27c1b97c24a 100644 --- a/question/type/questiontype.php +++ b/question/type/questiontype.php @@ -923,8 +923,10 @@ class default_questiontype { $link = ''.$st->seq_number.''; } else { if(isset($options->questionreviewlink)) { - $link = link_to_popup_window ($options->questionreviewlink.'?state='.$st->id.'&number='.$number, - 'reviewquestion', $st->seq_number, 450, 650, $strreviewquestion, 'none', true); + $link = link_to_popup_window($options->questionreviewlink . + '&question=' . $question->id . '&state=' . $st->id, + 'reviewquestion', $st->seq_number, 450, 650, $strreviewquestion, + 'none', true); } else { $link = $st->seq_number; }