Skip to content
Browse files

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.

Conflicts:

	mod/quiz/report/attemptsreport.php
  • Loading branch information...
1 parent eabff1a commit fcfef4dd181b88b7eaf7e19078a4ee071abf884d @timhunt timhunt committed
View
176 mod/quiz/report/attemptsreport.php
@@ -121,7 +121,7 @@ protected function load_relevant_students($cm, $course = null) {
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) {
@@ -138,91 +138,6 @@ protected function validate_common_options(&$attemptsmode, &$pagesize, $course,
}
/**
- * 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,";
- }
-
- $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,
- 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.
* @param array $columns the list of columns. Added to.
@@ -394,17 +309,22 @@ protected function delete_selected_attempts($quiz, $cm, $attemptids, $allowed) {
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;
@@ -603,6 +523,86 @@ protected function get_required_latest_state_fields($slot, $alias) {
}
/**
+ * 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,";
+ }
+
+ $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,
+ 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.
*
View
143 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();
-
- //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->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);
- }
+ if (!$this->rawdata) {
+ return;
+ }
- 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);
- }
+ $this->strtimeformat = str_replace(',', '', get_string('strftimedatetime'));
+ parent::build_table();
+
+ // End of adding the data from attempts. Now add averages at bottom.
+ $this->add_separator();
+
+ 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;
}
@@ -277,30 +292,6 @@ public function query_db($pagesize, $useinitialsbar = true) {
}
/**
- * 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.
*/
View
7 mod/quiz/report/overview/report.php
@@ -127,8 +127,8 @@ public function display($quiz, $cm, $course) {
$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,
@@ -217,8 +217,7 @@ public function display($quiz, $cm, $course) {
"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);
View
6 mod/quiz/report/responses/report.php
@@ -144,7 +144,8 @@ public function display($quiz, $cm, $course) {
$displaycourseshortname = format_string($COURSE->shortname, true, 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,
@@ -195,8 +196,7 @@ public function display($quiz, $cm, $course) {
}
}
- 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);
View
9 mod/quiz/report/responses/responses_table.php
@@ -35,11 +35,12 @@
*/
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() {

0 comments on commit fcfef4d

Please sign in to comment.
Something went wrong with that request. Please try again.