From 2ce9a94d962405a1fb2f3f375068d3ad031d966e Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 12 Feb 2014 17:26:30 +0000 Subject: [PATCH] MDL-35453 quiz reports: don't order by id. We were using ORDER BY id even though there was a perfectly good attempt column to order the attempts by one user at one quiz. Also, Oracle was complaining: Debug info: ORA-01799: a column may not be outer-joined to a subquery --- mod/quiz/report/overview/report.php | 9 +-- mod/quiz/report/reportlib.php | 30 ++++------ mod/quiz/tests/reportlib_test.php | 91 ++++++++++++++++++++++++++++- 3 files changed, 106 insertions(+), 24 deletions(-) diff --git a/mod/quiz/report/overview/report.php b/mod/quiz/report/overview/report.php index ebd571cf2afe3..d60da0b879944 100644 --- a/mod/quiz/report/overview/report.php +++ b/mod/quiz/report/overview/report.php @@ -526,10 +526,11 @@ protected function count_question_attempts_needing_regrade($quiz, $groupstudents */ protected function has_regraded_questions($from, $where, $params) { global $DB; - $qubaids = new qubaid_join($from, 'uniqueid', $where, $params); - return $DB->record_exists_select('quiz_overview_regrades', - 'questionusageid ' . $qubaids->usage_id_in(), - $qubaids->usage_id_in_params()); + return $DB->record_exists_sql(" + SELECT 1 + FROM {$from} + JOIN {quiz_overview_regrades} qor ON qor.questionusageid = quiza.uniqueid + WHERE {$where}", $params); } /** diff --git a/mod/quiz/report/reportlib.php b/mod/quiz/report/reportlib.php index f75ced3471fa5..a998634148492 100644 --- a/mod/quiz/report/reportlib.php +++ b/mod/quiz/report/reportlib.php @@ -156,35 +156,27 @@ function quiz_report_qm_filter_select($quiz, $quizattemptsalias = 'quiza') { function quiz_report_grade_method_sql($grademethod, $quizattemptsalias = 'quiza') { switch ($grademethod) { case QUIZ_GRADEHIGHEST : - return "$quizattemptsalias.id = ( - SELECT MIN(qa2.id) - FROM {quiz_attempts} qa2 + return "NOT EXISTS (SELECT 1 FROM {quiz_attempts} qa2 WHERE qa2.quiz = $quizattemptsalias.quiz AND - qa2.userid = $quizattemptsalias.userid AND - COALESCE(qa2.sumgrades, 0) = ( - SELECT MAX(COALESCE(qa3.sumgrades, 0)) - FROM {quiz_attempts} qa3 - WHERE qa3.quiz = $quizattemptsalias.quiz AND - qa3.userid = $quizattemptsalias.userid - ) - )"; + qa2.userid = $quizattemptsalias.userid AND ( + COALESCE(qa2.sumgrades, 0) > COALESCE($quizattemptsalias.sumgrades, 0) OR + (COALESCE(qa2.sumgrades, 0) = COALESCE($quizattemptsalias.sumgrades, 0) AND qa2.attempt < $quizattemptsalias.attempt) + ))"; case QUIZ_GRADEAVERAGE : return ''; case QUIZ_ATTEMPTFIRST : - return "$quizattemptsalias.id = ( - SELECT MIN(qa2.id) - FROM {quiz_attempts} qa2 + return "NOT EXISTS (SELECT 1 FROM {quiz_attempts} qa2 WHERE qa2.quiz = $quizattemptsalias.quiz AND - qa2.userid = $quizattemptsalias.userid)"; + qa2.userid = $quizattemptsalias.userid AND + qa2.attempt < $quizattemptsalias.attempt)"; case QUIZ_ATTEMPTLAST : - return "$quizattemptsalias.id = ( - SELECT MAX(qa2.id) - FROM {quiz_attempts} qa2 + return "NOT EXISTS (SELECT 1 FROM {quiz_attempts} qa2 WHERE qa2.quiz = $quizattemptsalias.quiz AND - qa2.userid = $quizattemptsalias.userid)"; + qa2.userid = $quizattemptsalias.userid AND + qa2.attempt > $quizattemptsalias.attempt)"; } } diff --git a/mod/quiz/tests/reportlib_test.php b/mod/quiz/tests/reportlib_test.php index b5e2853627d1a..de4b6a66720d2 100644 --- a/mod/quiz/tests/reportlib_test.php +++ b/mod/quiz/tests/reportlib_test.php @@ -36,7 +36,7 @@ * @copyright 2008 Jamie Pratt me@jamiep.org * @license http://www.gnu.org/copyleft/gpl.html GNU Public License */ -class mod_quiz_reportlib_testcase extends basic_testcase { +class mod_quiz_reportlib_testcase extends advanced_testcase { public function test_quiz_report_index_by_keys() { $datum = array(); $object = new stdClass(); @@ -73,4 +73,93 @@ public function test_quiz_report_scale_summarks_as_percentage() { $this->assertEquals('-', quiz_report_scale_summarks_as_percentage('-', $quiz, true)); } + + public function test_quiz_report_qm_filter_select_only_one_attempt_allowed() { + $quiz = new stdClass(); + $quiz->attempts = 1; + $this->assertSame('', quiz_report_qm_filter_select($quiz)); + } + + public function test_quiz_report_qm_filter_select_average() { + $quiz = new stdClass(); + $quiz->attempts = 10; + $quiz->grademethod = QUIZ_GRADEAVERAGE; + $this->assertSame('', quiz_report_qm_filter_select($quiz)); + } + + public function test_quiz_report_qm_filter_select_first_last_best() { + global $DB; + $this->resetAfterTest(); + + $fakeattempt = new stdClass(); + $fakeattempt->userid = 123; + $fakeattempt->quiz = 456; + $fakeattempt->layout = '1,2,0,3,4,0,5'; + + // We intentionally insert these in a funny order, to test the SQL better. + // The test data is: + // id | quizid | user | attempt | sumgrades + // ---------------------------------------- + // 4 | 456 | 123 | 1 | 30 + // 2 | 456 | 123 | 2 | 50 + // 1 | 456 | 123 | 3 | 50 + // 3 | 456 | 123 | 4 | null + // 5 | 456 | 1 | 1 | 100 + // layout is only given because it has a not-null constraint. + // uniqueid values are meaningless, but that column has a unique constraint. + + $fakeattempt->attempt = 3; + $fakeattempt->sumgrades = 50; + $fakeattempt->uniqueid = 13; + $DB->insert_record('quiz_attempts', $fakeattempt); + + $fakeattempt->attempt = 2; + $fakeattempt->sumgrades = 50; + $fakeattempt->uniqueid = 26; + $DB->insert_record('quiz_attempts', $fakeattempt); + + $fakeattempt->attempt = 4; + $fakeattempt->sumgrades = null; + $fakeattempt->uniqueid = 39; + $DB->insert_record('quiz_attempts', $fakeattempt); + + $fakeattempt->attempt = 1; + $fakeattempt->sumgrades = 30; + $fakeattempt->uniqueid = 52; + $DB->insert_record('quiz_attempts', $fakeattempt); + + $fakeattempt->attempt = 1; + $fakeattempt->userid = 1; + $fakeattempt->sumgrades = 100; + $fakeattempt->uniqueid = 65; + $DB->insert_record('quiz_attempts', $fakeattempt); + + $quiz = new stdClass(); + $quiz->attempts = 10; + + $quiz->grademethod = QUIZ_ATTEMPTFIRST; + $firstattempt = $DB->get_records_sql(" + SELECT * FROM {quiz_attempts} quiza WHERE userid = ? AND quiz = ? AND " + . quiz_report_qm_filter_select($quiz), array(123, 456)); + $this->assertEquals(1, count($firstattempt)); + $firstattempt = reset($firstattempt); + $this->assertEquals(1, $firstattempt->attempt); + + $quiz->grademethod = QUIZ_ATTEMPTLAST; + $lastattempt = $DB->get_records_sql(" + SELECT * FROM {quiz_attempts} quiza WHERE userid = ? AND quiz = ? AND " + . quiz_report_qm_filter_select($quiz), array(123, 456)); + $this->assertEquals(1, count($lastattempt)); + $lastattempt = reset($lastattempt); + $this->assertEquals(4, $lastattempt->attempt); + + $quiz->attempts = 0; + $quiz->grademethod = QUIZ_GRADEHIGHEST; + $bestattempt = $DB->get_records_sql(" + SELECT * FROM {quiz_attempts} qa_alias WHERE userid = ? AND quiz = ? AND " + . quiz_report_qm_filter_select($quiz, 'qa_alias'), array(123, 456)); + $this->assertEquals(1, count($bestattempt)); + $bestattempt = reset($bestattempt); + $this->assertEquals(2, $bestattempt->attempt); + } }