From 365e163946ddfe760793c1e751c859e1ec8804d2 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 13 Dec 2011 15:49:49 +0000 Subject: [PATCH] MDL-30704 Quiz grades report shows inconsistent averages. Previously, for the overall grade, we averaged the final marks for each student; while for the individual question grades, we averaged all grades. The report now works consistently on the principle that the averages should include exactly what is currenlty being shown in the report. This is more logical, and so should be easier for users to understand. If you want to see the averages that are currently shown (e.g. just the average of each student's highers grade) then the report options let you do that. --- mod/quiz/report/attemptsreport.php | 182 +++++++++--------- mod/quiz/report/overview/overview_table.php | 139 +++++++------ mod/quiz/report/overview/report.php | 7 +- mod/quiz/report/responses/report.php | 6 +- mod/quiz/report/responses/responses_table.php | 9 +- 5 files changed, 167 insertions(+), 176 deletions(-) diff --git a/mod/quiz/report/attemptsreport.php b/mod/quiz/report/attemptsreport.php index fa4f0d46016..554bd91dca1 100644 --- a/mod/quiz/report/attemptsreport.php +++ b/mod/quiz/report/attemptsreport.php @@ -121,7 +121,7 @@ abstract class quiz_attempt_report extends quiz_default_report { protected function validate_common_options(&$attemptsmode, &$pagesize, $course, $currentgroup) { if ($currentgroup) { //default for when a group is selected - if ($attemptsmode === null || $attemptsmode == QUIZ_REPORT_ATTEMPTS_ALL) { + if ($attemptsmode === null || $attemptsmode == QUIZ_REPORT_ATTEMPTS_ALL) { $attemptsmode = QUIZ_REPORT_ATTEMPTS_STUDENTS_WITH; } } else if (!$currentgroup && $course->id == SITEID) { @@ -137,94 +137,6 @@ abstract class quiz_attempt_report extends quiz_default_report { } } - /** - * Contruct all the parts of the main database query. - * @param object $quiz the quiz settings. - * @param string $qmsubselect SQL fragment from {@link quiz_report_qm_filter_select()}. - * @param bool $qmfilter whether to show all, or only the final grade attempt. - * @param int $attemptsmode which attempts to show. - * One of the QUIZ_REPORT_ATTEMPTS_... constants. - * @param array $reportstudents list if userids of users to include in the report. - * @return array with 4 elements ($fields, $from, $where, $params) that can be used to - * build the actual database query. - */ - protected function base_sql($quiz, $qmsubselect, $qmfilter, $attemptsmode, $reportstudents) { - global $DB; - - $fields = $DB->sql_concat('u.id', "'#'", 'COALESCE(quiza.attempt, 0)') . ' AS uniqueid,'; - - if ($qmsubselect) { - $fields .= "\n(CASE WHEN $qmsubselect THEN 1 ELSE 0 END) AS gradedattempt,"; - } - - $extrafields = get_extra_user_fields_sql($this->context, 'u', '', - array('id', 'idnumber', 'firstname', 'lastname', 'picture', - 'imagealt', 'institution', 'department', 'email')); - $fields .= ' - quiza.uniqueid AS usageid, - quiza.id AS attempt, - u.id AS userid, - u.idnumber, - u.firstname, - u.lastname, - u.picture, - u.imagealt, - u.institution, - u.department, - u.email' . $extrafields . ', - quiza.sumgrades, - quiza.timefinish, - quiza.timestart, - CASE WHEN quiza.timefinish = 0 THEN null - WHEN quiza.timefinish > quiza.timestart THEN quiza.timefinish - quiza.timestart - ELSE 0 END AS duration'; - // To explain that last bit, in MySQL, qa.timestart and qa.timefinish - // are unsigned. Since MySQL 5.5.5, when they introduced strict mode, - // subtracting a larger unsigned int from a smaller one gave an error. - // Therefore, we avoid doing that. timefinish can be non-zero and less - // than timestart when you have two load-balanced servers with very - // badly synchronised clocks, and a student does a really quick attempt.'; - - // This part is the same for all cases - join users and quiz_attempts tables - $from = "\n{user} u"; - $from .= "\nLEFT JOIN {quiz_attempts} quiza ON - quiza.userid = u.id AND quiza.quiz = :quizid"; - $params = array('quizid' => $quiz->id); - - if ($qmsubselect && $qmfilter) { - $from .= " AND $qmsubselect"; - } - switch ($attemptsmode) { - case QUIZ_REPORT_ATTEMPTS_ALL: - // Show all attempts, including students who are no longer in the course - $where = 'quiza.id IS NOT NULL AND quiza.preview = 0'; - break; - case QUIZ_REPORT_ATTEMPTS_STUDENTS_WITH: - // Show only students with attempts - list($usql, $uparams) = $DB->get_in_or_equal( - $reportstudents, SQL_PARAMS_NAMED, 'u'); - $params += $uparams; - $where = "u.id $usql AND quiza.preview = 0 AND quiza.id IS NOT NULL"; - break; - case QUIZ_REPORT_ATTEMPTS_STUDENTS_WITH_NO: - // Show only students without attempts - list($usql, $uparams) = $DB->get_in_or_equal( - $reportstudents, SQL_PARAMS_NAMED, 'u'); - $params += $uparams; - $where = "u.id $usql AND quiza.id IS NULL"; - break; - case QUIZ_REPORT_ATTEMPTS_ALL_STUDENTS: - // Show all students with or without attempts - list($usql, $uparams) = $DB->get_in_or_equal( - $reportstudents, SQL_PARAMS_NAMED, 'u'); - $params += $uparams; - $where = "u.id $usql AND (quiza.preview = 0 OR quiza.preview IS NULL)"; - break; - } - - return array($fields, $from, $where, $params); - } - /** * Add all the user-related columns to the $columns and $headers arrays. * @param table_sql $table the table being constructed. @@ -401,17 +313,22 @@ abstract class quiz_attempt_report_table extends table_sql { protected $quiz; protected $context; protected $qmsubselect; + protected $qmfilter; + protected $attemptsmode; protected $groupstudents; protected $students; protected $questions; protected $includecheckboxes; - public function __construct($uniqueid, $quiz, $context, $qmsubselect, $groupstudents, - $students, $questions, $includecheckboxes, $reporturl, $displayoptions) { + public function __construct($uniqueid, $quiz, $context, $qmsubselect, $qmfilter, + $attemptsmode, $groupstudents, $students, $questions, $includecheckboxes, + $reporturl, $displayoptions) { parent::__construct($uniqueid); $this->quiz = $quiz; $this->context = $context; $this->qmsubselect = $qmsubselect; + $this->qmfilter = $qmfilter; + $this->attemptsmode = $attemptsmode; $this->groupstudents = $groupstudents; $this->students = $students; $this->questions = $questions; @@ -609,6 +526,89 @@ abstract class quiz_attempt_report_table extends table_sql { return ''; } + /** + * Contruct all the parts of the main database query. + * @param array $reportstudents list if userids of users to include in the report. + * @return array with 4 elements ($fields, $from, $where, $params) that can be used to + * build the actual database query. + */ + public function base_sql($reportstudents) { + global $DB; + + $fields = $DB->sql_concat('u.id', "'#'", 'COALESCE(quiza.attempt, 0)') . ' AS uniqueid,'; + + if ($this->qmsubselect) { + $fields .= "\n(CASE WHEN $this->qmsubselect THEN 1 ELSE 0 END) AS gradedattempt,"; + } + + $extrafields = get_extra_user_fields_sql($this->context, 'u', '', + array('id', 'idnumber', 'firstname', 'lastname', 'picture', + 'imagealt', 'institution', 'department', 'email')); + $fields .= ' + quiza.uniqueid AS usageid, + quiza.id AS attempt, + u.id AS userid, + u.idnumber, + u.firstname, + u.lastname, + u.picture, + u.imagealt, + u.institution, + u.department, + u.email' . $extrafields . ', + quiza.sumgrades, + quiza.timefinish, + quiza.timestart, + CASE WHEN quiza.timefinish = 0 THEN null + WHEN quiza.timefinish > quiza.timestart THEN quiza.timefinish - quiza.timestart + ELSE 0 END AS duration'; + // To explain that last bit, in MySQL, qa.timestart and qa.timefinish + // are unsigned. Since MySQL 5.5.5, when they introduced strict mode, + // subtracting a larger unsigned int from a smaller one gave an error. + // Therefore, we avoid doing that. timefinish can be non-zero and less + // than timestart when you have two load-balanced servers with very + // badly synchronised clocks, and a student does a really quick attempt.'; + + // This part is the same for all cases - join users and quiz_attempts tables + $from = "\n{user} u"; + $from .= "\nLEFT JOIN {quiz_attempts} quiza ON + quiza.userid = u.id AND quiza.quiz = :quizid"; + $params = array('quizid' => $this->quiz->id); + + if ($this->qmsubselect && $this->qmfilter) { + $from .= " AND $this->qmsubselect"; + } + switch ($this->attemptsmode) { + case QUIZ_REPORT_ATTEMPTS_ALL: + // Show all attempts, including students who are no longer in the course + $where = 'quiza.id IS NOT NULL AND quiza.preview = 0'; + break; + case QUIZ_REPORT_ATTEMPTS_STUDENTS_WITH: + // Show only students with attempts + list($usql, $uparams) = $DB->get_in_or_equal( + $reportstudents, SQL_PARAMS_NAMED, 'u'); + $params += $uparams; + $where = "u.id $usql AND quiza.preview = 0 AND quiza.id IS NOT NULL"; + break; + case QUIZ_REPORT_ATTEMPTS_STUDENTS_WITH_NO: + // Show only students without attempts + list($usql, $uparams) = $DB->get_in_or_equal( + $reportstudents, SQL_PARAMS_NAMED, 'u'); + $params += $uparams; + $where = "u.id $usql AND quiza.id IS NULL"; + break; + case QUIZ_REPORT_ATTEMPTS_ALL_STUDENTS: + // Show all students with or without attempts + list($usql, $uparams) = $DB->get_in_or_equal( + $reportstudents, SQL_PARAMS_NAMED, 'u'); + $params += $uparams; + $where = "u.id $usql AND (quiza.preview = 0 OR quiza.preview IS NULL)"; + break; + } + + return array($fields, $from, $where, $params); + } + /** * Add the information about the latest state of the question with slot * $slot to the query. diff --git a/mod/quiz/report/overview/overview_table.php b/mod/quiz/report/overview/overview_table.php index 7efa3d8b79f..1da0ff7a8e4 100644 --- a/mod/quiz/report/overview/overview_table.php +++ b/mod/quiz/report/overview/overview_table.php @@ -37,86 +37,101 @@ class quiz_report_overview_table extends quiz_attempt_report_table { protected $regradedqs = array(); - public function __construct($quiz, $context, $qmsubselect, $groupstudents, - $students, $detailedmarks, $questions, $includecheckboxes, $reporturl, $displayoptions) { + public function __construct($quiz, $context, $qmsubselect, $qmfilter, + $attemptsmode, $groupstudents, $students, $detailedmarks, + $questions, $includecheckboxes, $reporturl, $displayoptions) { parent::__construct('mod-quiz-report-overview-report', $quiz , $context, - $qmsubselect, $groupstudents, $students, $questions, $includecheckboxes, - $reporturl, $displayoptions); + $qmsubselect, $qmfilter, $attemptsmode, $groupstudents, $students, + $questions, $includecheckboxes, $reporturl, $displayoptions); $this->detailedmarks = $detailedmarks; } public function build_table() { global $DB; - if ($this->rawdata) { - $this->strtimeformat = str_replace(',', '', get_string('strftimedatetime')); - parent::build_table(); + if (!$this->rawdata) { + return; + } - //end of adding data from attempts data to table / download - //now add averages at bottom of table : - $params = array($this->quiz->id); - $averagesql = ' - SELECT AVG(qg.grade) AS grade, COUNT(qg.grade) AS numaveraged - FROM {quiz_grades} qg - WHERE quiz = ?'; + $this->strtimeformat = str_replace(',', '', get_string('strftimedatetime')); + parent::build_table(); - $this->add_separator(); - if ($this->is_downloading()) { - $namekey = 'lastname'; - } else { - $namekey = 'fullname'; - } - if ($this->groupstudents) { - list($usql, $uparams) = $DB->get_in_or_equal($this->groupstudents); - $record = $DB->get_record_sql($averagesql . ' AND qg.userid ' . $usql, - array_merge($params, $uparams)); - $groupaveragerow = array( - $namekey => get_string('groupavg', 'grades'), - 'sumgrades' => $this->format_average($record), - 'feedbacktext'=> strip_tags(quiz_report_feedback_for_grade( - $record->grade, $this->quiz->id, $this->context))); - if ($this->detailedmarks && ($this->quiz->attempts == 1 || $this->qmsubselect)) { - $avggradebyq = $this->load_average_question_grades($this->groupstudents); - $groupaveragerow += $this->format_average_grade_for_questions($avggradebyq); - } - $this->add_data_keyed($groupaveragerow); - } + // End of adding the data from attempts. Now add averages at bottom. + $this->add_separator(); - if ($this->students) { - list($usql, $uparams) = $DB->get_in_or_equal($this->students); - $record = $DB->get_record_sql($averagesql . ' AND qg.userid ' . $usql, - array_merge($params, $uparams)); - $overallaveragerow = array( - $namekey => get_string('overallaverage', 'grades'), - 'sumgrades' => $this->format_average($record), - 'feedbacktext'=> strip_tags(quiz_report_feedback_for_grade( - $record->grade, $this->quiz->id, $this->context))); - if ($this->detailedmarks && ($this->quiz->attempts == 1 || $this->qmsubselect)) { - $avggradebyq = $this->load_average_question_grades($this->students); - $overallaveragerow += $this->format_average_grade_for_questions($avggradebyq); - } - $this->add_data_keyed($overallaveragerow); - } + if ($this->groupstudents) { + $this->add_average_row(get_string('groupavg', 'grades'), $this->groupstudents); + } + + if ($this->students) { + $this->add_average_row(get_string('overallaverage', 'grades'), $this->students); } } + /** + * Add an average grade over the attempts of a set of users. + * @param string $label the title ot use for this row. + * @param array $users the users to average over. + */ + protected function add_average_row($label, $users) { + global $DB; + + list($fields, $from, $where, $params) = $this->base_sql($users); + $record = $DB->get_record_sql(" + SELECT AVG(quiza.sumgrades) AS grade, COUNT(quiza.sumgrades) AS numaveraged + FROM $from + WHERE $where", $params); + + 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)) + ); + + if ($this->detailedmarks) { + $dm = new question_engine_data_mapper(); + $qubaids = new qubaid_join($from, 'quiza.uniqueid', $where, $params); + $avggradebyq = $dm->load_average_marks($qubaids, array_keys($this->questions)); + + $averagerow += $this->format_average_grade_for_questions($avggradebyq); + } + + $this->add_data_keyed($averagerow); + } + + /** + * Helper userd by {@link add_average_row()}. + * @param array $gradeaverages the raw grades. + * @return array the (partial) row of data. + */ protected function format_average_grade_for_questions($gradeaverages) { $row = array(); + if (!$gradeaverages) { $gradeaverages = array(); } + foreach ($this->questions as $question) { if (isset($gradeaverages[$question->slot]) && $question->maxmark > 0) { $record = $gradeaverages[$question->slot]; $record->grade = quiz_rescale_grade( $record->averagefraction * $question->maxmark, $this->quiz, false); + } else { $record = new stdClass(); $record->grade = null; $record->numaveraged = null; } + $row['qsgrade' . $question->slot] = $this->format_average($record, true); } + return $row; } @@ -276,30 +291,6 @@ class quiz_report_overview_table extends quiz_attempt_report_table { } } - /** - * Load the average grade for each question, averaged over particular users. - * @param array $userids the user ids to average over. - */ - protected function load_average_question_grades($userids) { - global $DB; - - $qmfilter = ''; - if ($this->quiz->attempts != 1) { - $qmfilter = '(' . quiz_report_qm_filter_select($this->quiz, 'quiza') . ') AND '; - } - - list($usql, $params) = $DB->get_in_or_equal($userids, SQL_PARAMS_NAMED, 'u'); - $params['quizid'] = $this->quiz->id; - $qubaids = new qubaid_join( - '{quiz_attempts} quiza', - 'quiza.uniqueid', - "quiza.userid $usql AND quiza.quiz = :quizid", - $params); - - $dm = new question_engine_data_mapper(); - return $dm->load_average_marks($qubaids, array_keys($this->questions)); - } - /** * Get all the questions in all the attempts being displayed that need regrading. * @return array A two dimensional array $questionusageid => $slot => $regradeinfo. diff --git a/mod/quiz/report/overview/report.php b/mod/quiz/report/overview/report.php index 42ef36983d9..c7e268149d4 100644 --- a/mod/quiz/report/overview/report.php +++ b/mod/quiz/report/overview/report.php @@ -129,8 +129,8 @@ class quiz_overview_report extends quiz_attempt_report { $questions = quiz_report_get_significant_questions($quiz); $table = new quiz_report_overview_table($quiz, $this->context, $qmsubselect, - $groupstudents, $students, $detailedmarks, $questions, $includecheckboxes, - $reporturl, $displayoptions); + $qmfilter, $attemptsmode, $groupstudents, $students, $detailedmarks, + $questions, $includecheckboxes, $reporturl, $displayoptions); $filename = quiz_report_download_filename(get_string('overviewfilename', 'quiz_overview'), $courseshortname, $quiz->name); $table->is_downloading($download, $filename, @@ -219,8 +219,7 @@ class quiz_overview_report extends quiz_attempt_report { "END) AS gradedattempt, "; } - list($fields, $from, $where, $params) = - $this->base_sql($quiz, $qmsubselect, $qmfilter, $attemptsmode, $allowed); + list($fields, $from, $where, $params) = $table->base_sql($allowed); $table->set_count_sql("SELECT COUNT(1) FROM $from WHERE $where", $params); diff --git a/mod/quiz/report/responses/report.php b/mod/quiz/report/responses/report.php index 8da9e642771..5282d3c7074 100644 --- a/mod/quiz/report/responses/report.php +++ b/mod/quiz/report/responses/report.php @@ -146,7 +146,8 @@ class quiz_responses_report extends quiz_attempt_report { array('context' => $displaycoursecontext)); $table = new quiz_report_responses_table($quiz, $this->context, $qmsubselect, - $groupstudents, $students, $questions, $includecheckboxes, $reporturl, $displayoptions); + $qmfilter, $attemptsmode, $groupstudents, $students, $questions, + $includecheckboxes, $reporturl, $displayoptions); $filename = quiz_report_download_filename(get_string('responsesfilename', 'quiz_responses'), $courseshortname, $quiz->name); $table->is_downloading($download, $filename, @@ -197,8 +198,7 @@ class quiz_responses_report extends quiz_attempt_report { } } - list($fields, $from, $where, $params) = - $this->base_sql($quiz, $qmsubselect, $qmfilter, $attemptsmode, $allowed); + list($fields, $from, $where, $params) = $table->base_sql($allowed); $table->set_count_sql("SELECT COUNT(1) FROM $from WHERE $where", $params); diff --git a/mod/quiz/report/responses/responses_table.php b/mod/quiz/report/responses/responses_table.php index cfb4633ddcf..cb2a48d4a8a 100644 --- a/mod/quiz/report/responses/responses_table.php +++ b/mod/quiz/report/responses/responses_table.php @@ -35,11 +35,12 @@ defined('MOODLE_INTERNAL') || die(); */ class quiz_report_responses_table extends quiz_attempt_report_table { - public function __construct($quiz, $context, $qmsubselect, $groupstudents, - $students, $questions, $includecheckboxes, $reporturl, $displayoptions) { + public function __construct($quiz, $context, $qmsubselect, $qmfilter, + $attemptsmode, $groupstudents, $students, + $questions, $includecheckboxes, $reporturl, $displayoptions) { parent::__construct('mod-quiz-report-responses-report', $quiz, $context, - $qmsubselect, $groupstudents, $students, $questions, $includecheckboxes, - $reporturl, $displayoptions); + $qmsubselect, $qmfilter, $attemptsmode, $groupstudents, $students, + $questions, $includecheckboxes, $reporturl, $displayoptions); } public function build_table() {