From: tjhunt Date: Tue, 23 Sep 2008 07:18:15 +0000 (+0000) Subject: MDL-16625 Detection of whether a quiz has grades was broken in HEAD. X-Git-Url: http://git.mjollnir.org/gw?a=commitdiff_plain;h=739b07112e32142de14e32ad9e67466ef6c2f03f;p=moodle.git MDL-16625 Detection of whether a quiz has grades was broken in HEAD. 1. Fix this by factoring out a quiz_has_grades function. 2. Fix the quiz unit tests. 3. Fix quiz_has_feedback to return false for ungraded quizzes. 4. Fix use of get_records to do record_exists in quiz reports. --- diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index 9225016e05..aaf94cb1d5 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -49,12 +49,15 @@ class quiz { * @param object $quiz the row from the quiz table. * @param object $cm the course_module object for this quiz. * @param object $course the row from the course table for the course we belong to. + * @param boolean $getcontext intended for testing - stops the constructor getting the context. */ - function __construct($quiz, $cm, $course) { + function __construct($quiz, $cm, $course, $getcontext = true) { $this->quiz = $quiz; $this->cm = $cm; $this->course = $course; - $this->context = get_context_instance(CONTEXT_MODULE, $cm->id); + if ($getcontext && !empty($cm->id)) { + $this->context = get_context_instance(CONTEXT_MODULE, $cm->id); + } $this->determine_layout(); } diff --git a/mod/quiz/lib.php b/mod/quiz/lib.php index 6a05bfc745..95f97aa0d1 100644 --- a/mod/quiz/lib.php +++ b/mod/quiz/lib.php @@ -213,6 +213,18 @@ function quiz_user_outline($course, $user, $mod, $quiz) { return NULL; } +/** + * Is this a graded quiz? If this method returns true, you can assume that + * $quiz->grade and $quiz->sumgrades are non-zero (for example, if you want to + * divide by them). + * + * @param object $quiz a row from the quiz table. + * @return boolean whether this is a graded quiz. + */ +function quiz_has_grades($quiz) { + return $quiz->grade != 0 && $quiz->sumgrades != 0; +} + /** * Get the best current grade for a particular user in a quiz. * @@ -239,7 +251,7 @@ function quiz_user_complete($course, $user, $mod, $quiz) { /// a given particular instance of this module, for user activity reports. if ($attempts = $DB->get_records_select('quiz_attempts', "userid=? AND quiz=?", array($user->id, $quiz->id), 'attempt ASC')) { - if ($quiz->grade && $quiz->sumgrades && $grade = quiz_get_best_grade($quiz, $user->id)) { + if (quiz_has_grades($quiz) && $grade = quiz_get_best_grade($quiz, $user->id)) { echo get_string('grade') . ': ' . $grade . '/' . $quiz->grade . '
'; } foreach ($attempts as $attempt) { diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index 50fc960cae..d632890bc1 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -420,14 +420,16 @@ function quiz_feedback_for_grade($grade, $quizid) { * @param integer $quizid the id of the quiz object. * @return boolean Whether this quiz has any non-blank feedback text. */ -function quiz_has_feedback($quizid) { +function quiz_has_feedback($quiz) { global $DB; static $cache = array(); - if (!array_key_exists($quizid, $cache)) { - $cache[$quizid] = $DB->record_exists_select('quiz_feedback', - "quizid = ? AND " . $DB->sql_isnotempty('quiz_feedback', 'feedbacktext', false, true), array($quizid)); + if (!array_key_exists($quiz->id, $cache)) { + $cache[$quiz->id] = quiz_has_grades($quiz) && + $DB->record_exists_select('quiz_feedback', "quizid = ? AND " . + $DB->sql_isnotempty('quiz_feedback', 'feedbacktext', false, true), + array($quiz->id)); } - return $cache[$quizid]; + return $cache[$quiz->id]; } /** diff --git a/mod/quiz/report/overview/report.php b/mod/quiz/report/overview/report.php index aa42d3b9e7..242c9868f0 100644 --- a/mod/quiz/report/overview/report.php +++ b/mod/quiz/report/overview/report.php @@ -23,13 +23,13 @@ class quiz_overview_report extends quiz_default_report { $this->context = get_context_instance(CONTEXT_MODULE, $cm->id); // Work out some display options - whether there is feedback, and whether scores should be shown. - $hasfeedback = quiz_has_feedback($quiz->id) && $quiz->grade > 1.e-7 && $quiz->sumgrades > 1.e-7; + $hasfeedback = quiz_has_feedback($quiz); $fakeattempt = new stdClass(); $fakeattempt->preview = false; $fakeattempt->timefinish = $quiz->timeopen; $fakeattempt->userid = 0; $reviewoptions = quiz_get_reviewoptions($quiz, $fakeattempt, $this->context); - $showgrades = ($quiz->grade !== 0) && ($quiz->sumgrades !== 0) && $reviewoptions->scores; + $showgrades = quiz_has_grades($quiz) && $reviewoptions->scores; $download = optional_param('download', '', PARAM_ALPHA); @@ -402,15 +402,14 @@ class quiz_overview_report extends quiz_default_report { if ($currentgroup && $groupstudents){ list($usql, $params) = $DB->get_in_or_equal($groupstudents); $params[] = $quiz->id; - //should be quicker than a COUNT to test if there is at least one record : - if ($DB->get_records_select('quiz_grades', "userid $usql AND quiz = ?", $params, '', '*', 0, 1)){ + if ($DB->record_exists_select('quiz_grades', "userid $usql AND quiz = ?", $params)) { $imageurl = "{$CFG->wwwroot}/mod/quiz/report/overview/overviewgraph.php?id={$quiz->id}&groupid=$currentgroup"; $graphname = get_string('overviewreportgraphgroup', 'quiz_overview', groups_get_group_name($currentgroup)); print_heading($graphname); echo '
'.$graphname.'
'; } } - if ($DB->get_records('quiz_grades', array('quiz'=> $quiz->id), '', '*', 0, 1)){ + if ($DB->record_exists('quiz_grades', array('quiz'=> $quiz->id))){ $graphname = get_string('overviewreportgraph', 'quiz_overview'); $imageurl = $CFG->wwwroot.'/mod/quiz/report/overview/overviewgraph.php?id='.$quiz->id; print_heading($graphname); diff --git a/mod/quiz/report/responses/report.php b/mod/quiz/report/responses/report.php index 918d557ddc..58116ac3e0 100644 --- a/mod/quiz/report/responses/report.php +++ b/mod/quiz/report/responses/report.php @@ -22,13 +22,13 @@ class quiz_responses_report extends quiz_default_report { $context = get_context_instance(CONTEXT_MODULE, $cm->id); // Work out some display options - whether there is feedback, and whether scores should be shown. - $hasfeedback = quiz_has_feedback($quiz->id) && $quiz->grade > 1.e-7 && $quiz->sumgrades > 1.e-7; + $hasfeedback = quiz_has_feedback($quiz); $fakeattempt = new stdClass(); $fakeattempt->preview = false; $fakeattempt->timefinish = $quiz->timeopen; $fakeattempt->userid = 0; $reviewoptions = quiz_get_reviewoptions($quiz, $fakeattempt, $context); - $showgrades = $quiz->grade && $quiz->sumgrades && $reviewoptions->scores; + $showgrades = quiz_has_grades($quiz) && $reviewoptions->scores; $download = optional_param('download', '', PARAM_ALPHA); @@ -171,8 +171,8 @@ class quiz_responses_report extends quiz_default_report { } - $showgrades = $quiz->grade && $quiz->sumgrades && $reviewoptions->scores; - $hasfeedback = quiz_has_feedback($quiz->id) && $quiz->grade > 1.e-7 && $quiz->sumgrades > 1.e-7; + $showgrades = quiz_has_grades($quiz) && $reviewoptions->scores; + $hasfeedback = quiz_has_feedback($quiz); // Construct the SQL diff --git a/mod/quiz/review.php b/mod/quiz/review.php index 17cfb5e15e..2d9a03ebd0 100644 --- a/mod/quiz/review.php +++ b/mod/quiz/review.php @@ -166,7 +166,7 @@ /// Show scores (if the user is allowed to see scores at the moment). $grade = quiz_rescale_grade($attempt->sumgrades, $quiz); if ($options->scores) { - if ($quiz->grade and $quiz->sumgrades) { + if (quiz_has_grades($quiz)) { if($overtime) { $result->sumgrades = "0"; $result->grade = "0.0"; diff --git a/mod/quiz/simpletest/testaccessrules.php b/mod/quiz/simpletest/testaccessrules.php index 8ec7647c16..72a57e55fb 100644 --- a/mod/quiz/simpletest/testaccessrules.php +++ b/mod/quiz/simpletest/testaccessrules.php @@ -18,7 +18,9 @@ class simple_rules_test extends MoodleUnitTestCase { function test_num_attempts_access_rule() { $quiz = new stdClass; $quiz->attempts = 3; - $rule = new num_attempts_access_rule($quiz, 0); + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); + $rule = new num_attempts_access_rule($quizobj, 0); $attempt = new stdClass; $this->assertEqual($rule->description(), get_string('attemptsallowedn', 'quiz', 3)); @@ -42,7 +44,9 @@ class simple_rules_test extends MoodleUnitTestCase { $attempt = new stdClass; $quiz->subnet = getremoteaddr(); - $rule = new ipaddress_access_rule($quiz, 0); + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); + $rule = new ipaddress_access_rule($quizobj, 0); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->description()); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); @@ -50,7 +54,9 @@ class simple_rules_test extends MoodleUnitTestCase { $this->assertFalse($rule->time_left($attempt, 1)); $quiz->subnet = '0.0.0.0'; - $rule = new ipaddress_access_rule($quiz, 0); + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); + $rule = new ipaddress_access_rule($quizobj, 0); $this->assertTrue($rule->prevent_access()); $this->assertFalse($rule->description()); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); @@ -61,7 +67,9 @@ class simple_rules_test extends MoodleUnitTestCase { function test_time_limit_access_rule() { $quiz = new stdClass; $quiz->timelimit = 60; - $rule = new time_limit_access_rule($quiz, 10000); + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); + $rule = new time_limit_access_rule($quizobj, 10000); $attempt = new stdClass; $this->assertEqual($rule->description(), get_string('quiztimelimit', 'quiz', format_time(3600))); @@ -82,9 +90,12 @@ class open_close_date_access_rule_test extends MoodleUnitTestCase { $quiz = new stdClass; $quiz->timeopen = 0; $quiz->timeclose = 0; + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); $attempt = new stdClass; + $attempt->preview = 0; - $rule = new open_close_date_access_rule($quiz, 10000); + $rule = new open_close_date_access_rule($quizobj, 10000); $this->assertFalse($rule->description()); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); @@ -92,7 +103,7 @@ class open_close_date_access_rule_test extends MoodleUnitTestCase { $this->assertFalse($rule->time_left($attempt, 10000)); $this->assertFalse($rule->time_left($attempt, 0)); - $rule = new open_close_date_access_rule($quiz, 0); + $rule = new open_close_date_access_rule($quizobj, 0); $this->assertFalse($rule->description()); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); @@ -104,16 +115,19 @@ class open_close_date_access_rule_test extends MoodleUnitTestCase { $quiz = new stdClass; $quiz->timeopen = 10000; $quiz->timeclose = 0; + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); $attempt = new stdClass; + $attempt->preview = 0; - $rule = new open_close_date_access_rule($quiz, 9999); + $rule = new open_close_date_access_rule($quizobj, 9999); $this->assertEqual($rule->description(), array(get_string('quiznotavailable', 'quiz', userdate(10000)))); $this->assertEqual($rule->prevent_access(), get_string('notavailable', 'quiz')); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); $this->assertFalse($rule->is_finished(0, $attempt)); $this->assertFalse($rule->time_left($attempt, 0)); - $rule = new open_close_date_access_rule($quiz, 10000); + $rule = new open_close_date_access_rule($quizobj, 10000); $this->assertEqual($rule->description(), array(get_string('quizopenedon', 'quiz', userdate(10000)))); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); @@ -125,9 +139,12 @@ class open_close_date_access_rule_test extends MoodleUnitTestCase { $quiz = new stdClass; $quiz->timeopen = 0; $quiz->timeclose = 20000; + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); $attempt = new stdClass; + $attempt->preview = 0; - $rule = new open_close_date_access_rule($quiz, 20000); + $rule = new open_close_date_access_rule($quizobj, 20000); $this->assertEqual($rule->description(), array(get_string('quizcloseson', 'quiz', userdate(20000)))); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); @@ -137,7 +154,7 @@ class open_close_date_access_rule_test extends MoodleUnitTestCase { $this->assertEqual($rule->time_left($attempt, 20000), 0); $this->assertEqual($rule->time_left($attempt, 20100), -100); - $rule = new open_close_date_access_rule($quiz, 20001); + $rule = new open_close_date_access_rule($quizobj, 20001); $this->assertEqual($rule->description(), array(get_string('quizclosed', 'quiz', userdate(20000)))); $this->assertEqual($rule->prevent_access(), get_string('notavailable', 'quiz')); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); @@ -152,29 +169,32 @@ class open_close_date_access_rule_test extends MoodleUnitTestCase { $quiz = new stdClass; $quiz->timeopen = 10000; $quiz->timeclose = 20000; + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); $attempt = new stdClass; + $attempt->preview = 0; - $rule = new open_close_date_access_rule($quiz, 9999); + $rule = new open_close_date_access_rule($quizobj, 9999); $this->assertEqual($rule->description(), array(get_string('quiznotavailable', 'quiz', userdate(10000)))); $this->assertEqual($rule->prevent_access(), get_string('notavailable', 'quiz')); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); $this->assertFalse($rule->is_finished(0, $attempt)); - $rule = new open_close_date_access_rule($quiz, 10000); + $rule = new open_close_date_access_rule($quizobj, 10000); $this->assertEqual($rule->description(), array(get_string('quizopenedon', 'quiz', userdate(10000)), get_string('quizcloseson', 'quiz', userdate(20000)))); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); $this->assertFalse($rule->is_finished(0, $attempt)); - $rule = new open_close_date_access_rule($quiz, 20000); + $rule = new open_close_date_access_rule($quizobj, 20000); $this->assertEqual($rule->description(), array(get_string('quizopenedon', 'quiz', userdate(10000)), get_string('quizcloseson', 'quiz', userdate(20000)))); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); $this->assertFalse($rule->is_finished(0, $attempt)); - $rule = new open_close_date_access_rule($quiz, 20001); + $rule = new open_close_date_access_rule($quizobj, 20001); $this->assertEqual($rule->description(), array(get_string('quizclosed', 'quiz', userdate(20000)))); $this->assertEqual($rule->prevent_access(), get_string('notavailable', 'quiz')); $this->assertFalse($rule->prevent_new_attempt(0, $attempt)); @@ -194,10 +214,12 @@ class inter_attempt_delay_access_rule_test extends MoodleUnitTestCase { $quiz->delay1 = 1000; $quiz->delay2 = 0; $quiz->timeclose = 0; + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); $attempt = new stdClass; $attempt->timefinish = 10000; - $rule = new inter_attempt_delay_access_rule($quiz, 10000); + $rule = new inter_attempt_delay_access_rule($quizobj, 10000); $this->assertFalse($rule->description()); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->is_finished(0, $attempt)); @@ -221,10 +243,12 @@ class inter_attempt_delay_access_rule_test extends MoodleUnitTestCase { $quiz->delay1 = 0; $quiz->delay2 = 1000; $quiz->timeclose = 0; + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); $attempt = new stdClass; $attempt->timefinish = 10000; - $rule = new inter_attempt_delay_access_rule($quiz, 10000); + $rule = new inter_attempt_delay_access_rule($quizobj, 10000); $this->assertFalse($rule->description()); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->is_finished(0, $attempt)); @@ -251,10 +275,12 @@ class inter_attempt_delay_access_rule_test extends MoodleUnitTestCase { $quiz->delay1 = 2000; $quiz->delay2 = 1000; $quiz->timeclose = 0; + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); $attempt = new stdClass; $attempt->timefinish = 10000; - $rule = new inter_attempt_delay_access_rule($quiz, 10000); + $rule = new inter_attempt_delay_access_rule($quizobj, 10000); $this->assertFalse($rule->description()); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->is_finished(0, $attempt)); @@ -289,10 +315,12 @@ class inter_attempt_delay_access_rule_test extends MoodleUnitTestCase { $quiz->delay1 = 2000; $quiz->delay2 = 1000; $quiz->timeclose = 15000; + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); $attempt = new stdClass; $attempt->timefinish = 13000; - $rule = new inter_attempt_delay_access_rule($quiz, 10000); + $rule = new inter_attempt_delay_access_rule($quizobj, 10000); $this->assertFalse($rule->description()); $this->assertFalse($rule->prevent_access()); $this->assertFalse($rule->is_finished(0, $attempt)); @@ -307,7 +335,7 @@ class inter_attempt_delay_access_rule_test extends MoodleUnitTestCase { $attempt->timefinish = 14001; $this->assertEqual($rule->prevent_new_attempt(2, $attempt), get_string('youcannotwait', 'quiz')); - $rule = new inter_attempt_delay_access_rule($quiz, 15000); + $rule = new inter_attempt_delay_access_rule($quizobj, 15000); $attempt->timefinish = 13000; $this->assertFalse($rule->prevent_new_attempt(1, $attempt)); $attempt->timefinish = 13001; @@ -317,7 +345,7 @@ class inter_attempt_delay_access_rule_test extends MoodleUnitTestCase { $attempt->timefinish = 14001; $this->assertEqual($rule->prevent_new_attempt(2, $attempt), get_string('youcannotwait', 'quiz')); - $rule = new inter_attempt_delay_access_rule($quiz, 15001); + $rule = new inter_attempt_delay_access_rule($quizobj, 15001); $attempt->timefinish = 13000; $this->assertFalse($rule->prevent_new_attempt(1, $attempt)); $attempt->timefinish = 13001; @@ -333,7 +361,9 @@ class password_access_rule_test extends MoodleUnitTestCase { function test_password_access_rule() { $quiz = new stdClass; $quiz->password = 'frog'; - $rule = new password_access_rule($quiz, 0); + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); + $rule = new password_access_rule($quizobj, 0); $attempt = new stdClass; $this->assertFalse($rule->prevent_access()); @@ -351,7 +381,11 @@ class password_access_rule_test extends MoodleUnitTestCase { $quiz->id = -1; // So as not to interfere with any real quizzes. $quiz->intro = 'SOME INTRO TEXT'; $quiz->password = 'frog'; - $rule = new password_access_rule($quiz, 0); + $quiz->questions = ''; + $cm = new stdClass; + $cm->id = 666; + $quizobj = new quiz($quiz, $cm, null, false); + $rule = new password_access_rule($quizobj, 0); $rule->clear_access_allowed(-1); $_POST['cancelpassword'] = false; @@ -385,7 +419,9 @@ class securewindow_access_rule_test extends MoodleUnitTestCase { function test_securewindow_access_rule() { $quiz = new stdClass; $quiz->popup = 1; - $rule = new securewindow_access_rule($quiz, 0); + $quiz->questions = ''; + $quizobj = new quiz($quiz, null, null); + $rule = new securewindow_access_rule($quizobj, 0); $attempt = new stdClass; $this->assertFalse($rule->prevent_access()); diff --git a/mod/quiz/simpletest/testlib.php b/mod/quiz/simpletest/testlib.php new file mode 100644 index 0000000000..ba9fe2cc57 --- /dev/null +++ b/mod/quiz/simpletest/testlib.php @@ -0,0 +1,30 @@ +dirroot . '/mod/quiz/lib.php'); + +class quiz_lib_test extends MoodleUnitTestCase { + function test_quiz_has_grades() { + $quiz = new stdClass; + $quiz->grade = '100.0000'; + $quiz->sumgrades = '100.0000'; + $this->assertTrue(quiz_has_grades($quiz)); + $quiz->sumgrades = '0.0000'; + $this->assertFalse(quiz_has_grades($quiz)); + $quiz->grade = '0.0000'; + $this->assertFalse(quiz_has_grades($quiz)); + $quiz->sumgrades = '100.0000'; + $this->assertFalse(quiz_has_grades($quiz)); + } +} +?> \ No newline at end of file diff --git a/mod/quiz/view.php b/mod/quiz/view.php index f68b9a9467..2d335db0b0 100644 --- a/mod/quiz/view.php +++ b/mod/quiz/view.php @@ -177,11 +177,11 @@ $attemptcolumn = $quiz->attempts != 1; - $gradecolumn = $someoptions->scores && $quiz->grade && $quiz->sumgrades; + $gradecolumn = $someoptions->scores && quiz_has_grades($quiz); $markcolumn = $gradecolumn && ($quiz->grade != $quiz->sumgrades); $overallstats = $alloptions->scores; - $feedbackcolumn = quiz_has_feedback($quiz->id) && $alloptions->overallfeedback; + $feedbackcolumn = quiz_has_feedback($quiz) && $alloptions->overallfeedback; // Prepare table header $table->class = 'generaltable quizattemptsummary'; @@ -306,7 +306,7 @@ print_heading(get_string("nomoreattempts", "quiz")); } - if ($numattempts && $quiz->sumgrades && !is_null($mygrade)) { + if ($numattempts && $gradecolumn && !is_null($mygrade)) { $resultinfo = ''; if ($overallstats) {