From 199b34b5d06812c3a3bcb7d30eb303d14d465685 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 5 Feb 2018 17:30:21 +0000 Subject: [PATCH] MDL-61348 quiz reports: fix incorrect groups averages the averages were wrong if a user had mulitple enrolments in a course, which can happen with some enrolment plugins. --- mod/quiz/report/overview/overview_table.php | 52 ++++++++++++++----- .../report/overview/tests/report_test.php | 14 ++++- 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/mod/quiz/report/overview/overview_table.php b/mod/quiz/report/overview/overview_table.php index 6e0e5ccf793..1064975295d 100644 --- a/mod/quiz/report/overview/overview_table.php +++ b/mod/quiz/report/overview/overview_table.php @@ -92,42 +92,64 @@ class quiz_overview_table extends quiz_attempts_report_table { } } - /** - * Add an average grade over the attempts of a set of users. + * Calculate the average overall and question scores for a set of attempts at the quiz. + * * @param string $label the title ot use for this row. - * @param \core\dml\sql_join $usersjoins (joins, wheres, params) for the users to average over. + * @param \core\dml\sql_join $usersjoins to indicate a set of users. + * @return array of table cells that make up the average row. */ - protected function add_average_row($label, \core\dml\sql_join $usersjoins) { + public function compute_average_row($label, \core\dml\sql_join $usersjoins) { global $DB; list($fields, $from, $where, $params) = $this->base_sql($usersjoins); $record = $DB->get_record_sql(" - SELECT AVG(quiza.sumgrades) AS grade, COUNT(quiza.sumgrades) AS numaveraged - FROM $from - WHERE $where", $params); + SELECT AVG(quizaouter.sumgrades) AS grade, COUNT(quizaouter.sumgrades) AS numaveraged + FROM {quiz_attempts} quizaouter + JOIN ( + SELECT DISTINCT quiza.id + FROM $from + WHERE $where + ) relevant_attempt_ids ON quizaouter.id = relevant_attempt_ids.id + ", $params); $record->grade = quiz_rescale_grade($record->grade, $this->quiz, false); - if ($this->is_downloading()) { $namekey = 'lastname'; } else { $namekey = 'fullname'; } $averagerow = array( - $namekey => $label, - 'sumgrades' => $this->format_average($record), - 'feedbacktext'=> strip_tags(quiz_report_feedback_for_grade( - $record->grade, $this->quiz->id, $this->context)) + $namekey => $label, + 'sumgrades' => $this->format_average($record), + 'feedbacktext' => strip_tags(quiz_report_feedback_for_grade( + $record->grade, $this->quiz->id, $this->context)) ); if ($this->options->slotmarks) { $dm = new question_engine_data_mapper(); - $qubaids = new qubaid_join($from, 'quiza.uniqueid', $where, $params); + $qubaids = new qubaid_join("{quiz_attempts} quizaouter + JOIN ( + SELECT DISTINCT quiza.id + FROM $from + WHERE $where + ) relevant_attempt_ids ON quizaouter.id = relevant_attempt_ids.id", + 'quizaouter.uniqueid', '1 = 1', $params); $avggradebyq = $dm->load_average_marks($qubaids, array_keys($this->questions)); $averagerow += $this->format_average_grade_for_questions($avggradebyq); } + return $averagerow; + } + + /** + * Add an average grade row for a set of users. + * + * @param string $label the title ot use for this row. + * @param \core\dml\sql_join $usersjoins (joins, wheres, params) for the users to average over. + */ + protected function add_average_row($label, \core\dml\sql_join $usersjoins) { + $averagerow = $this->compute_average_row($label, $usersjoins); $this->add_data_keyed($averagerow); } @@ -163,7 +185,9 @@ class quiz_overview_table extends quiz_attempts_report_table { /** * Format an entry in an average row. - * @param object $record with fields grade and numaveraged + * @param object $record with fields grade and numaveraged. + * @param bool $question true if this is a question score, false if it is an overall score. + * @return string HTML fragment for an average score (with number of things included in the average). */ protected function format_average($record, $question = false) { if (is_null($record->grade)) { diff --git a/mod/quiz/report/overview/tests/report_test.php b/mod/quiz/report/overview/tests/report_test.php index 0438155eb2a..54606daa649 100644 --- a/mod/quiz/report/overview/tests/report_test.php +++ b/mod/quiz/report/overview/tests/report_test.php @@ -89,7 +89,6 @@ class quiz_overview_report_testcase extends advanced_testcase { // The test data. $timestamp = 1234567890; - $fields = array('quiz', 'userid', 'attempt', 'sumgrades', 'state'); $attempts = array( array($quiz, $student1, 1, 0.0, quiz_attempt::FINISHED), array($quiz, $student1, 2, 5.0, quiz_attempt::FINISHED), @@ -165,6 +164,8 @@ class quiz_overview_report_testcase extends advanced_testcase { $reportoptions->states = array(quiz_attempt::IN_PROGRESS, quiz_attempt::OVERDUE, quiz_attempt::FINISHED); // Now do a minimal set-up of the table class. + $q->slot = 1; + $q->maxmark = 10; $table = new quiz_overview_table($quiz, $context, $qmsubselect, $reportoptions, $empty, $studentsjoins, array(1 => $q), null); $table->download = $isdownloading; // Cannot call the is_downloading API, because it gives errors. @@ -196,7 +197,16 @@ class quiz_overview_report_testcase extends advanced_testcase { $this->assertArrayHasKey($student3->id . '#0', $table->rawdata); $this->assertEquals(0, $table->rawdata[$student3->id . '#0']->gradedattempt); - // Ensure that filtering by inital does not break it. + // Check the calculation of averages. + $averagerow = $table->compute_average_row('overallaverage', $studentsjoins); + $this->assertContains('75.00', $averagerow['sumgrades']); + $this->assertContains('75.00', $averagerow['qsgrade1']); + if (!$isdownloading) { + $this->assertContains('(2)', $averagerow['sumgrades']); + $this->assertContains('(2)', $averagerow['qsgrade1']); + } + + // Ensure that filtering by initial does not break it. // This involves setting a private properly of the base class, which is // only really possible using reflection :-(. $reflectionobject = new ReflectionObject($table);