Skip to content

Commit

Permalink
MDL-60162 quiz reports: too many rows fetched on download
Browse files Browse the repository at this point in the history 
This happened if one user had multiple enrolments in a course, and was
quite inefficient.
  • Loading branch information
@timhunt
timhunt committed Oct 24, 2017
1 parent b8e10f2 commit 661e129
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 36 deletions.
12 changes: 12 additions & 0 deletions mod/quiz/attemptlib.php
View file
Expand Up @@ -1093,6 +1093,18 @@ public function get_active_slots($page = 'all') {
return $activeslots;
}

/**
* Helper method for unit tests. Get the underlying question usage object.
* @return question_usage_by_activity the usage.
*/
public function get_question_usage() {
if (!PHPUNIT_TEST) {
throw new coding_exception('get_question_usage is only for use in unit tests. ' .
'For other operations, use the quiz_attempt api, or extend it properly.');
}
return $this->quba;
}

/**
* Get the question_attempt object for a particular question in this attempt.
* @param int $slot the number used to identify this question within this attempt.
Expand Down
8 changes: 6 additions & 2 deletions mod/quiz/report/attemptsreport_table.php
View file
Expand Up @@ -515,8 +515,12 @@ protected function get_qubaids_condition() {

if ($this->is_downloading()) {
// We want usages for all attempts.
return new qubaid_join($this->sql->from, 'quiza.uniqueid',
$this->sql->where, $this->sql->params);
return new qubaid_join("(
SELECT DISTINCT quiza.uniqueid
FROM " . $this->sql->from . "
WHERE " . $this->sql->where . "
) quizasubquery", 'quizasubquery.uniqueid',
"1 = 1", $this->sql->params);
}

$qubaids = array();
Expand Down
127 changes: 93 additions & 34 deletions mod/quiz/report/overview/tests/report_test.php
View file
Expand Up @@ -39,65 +39,117 @@
*/
class quiz_overview_report_testcase extends advanced_testcase {

public function test_report_sql() {
/**
* Data provider for test_report_sql.
*
* @return array the data for the test sub-cases.
*/
public function report_sql_cases() {
return [[null], ['csv']]; // Only need to test on or off, not all download types.
}

/**
* Test how the report queries the database.
*
* @param bool $isdownloading a download type, or null.
* @dataProvider report_sql_cases
*/
public function test_report_sql($isdownloading) {
global $DB;
$this->resetAfterTest(true);

// Create a course and a quiz.
$generator = $this->getDataGenerator();
$course = $generator->create_course();
$quizgenerator = $generator->get_plugin_generator('mod_quiz');
$quiz = $quizgenerator->create_instance(array('course' => $course->id,
'grademethod' => QUIZ_GRADEHIGHEST, 'grade' => 100.0, 'sumgrades' => 10.0,
'attempts' => 10));

// Add one question.
$questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question');
$cat = $questiongenerator->create_question_category();
$q = $questiongenerator->create_question('essay', 'plain', ['category' => $cat->id]);
quiz_add_quiz_question($q->id, $quiz, 0 , 10);

// Create some students and enrol them in the course.
$student1 = $generator->create_user();
$student2 = $generator->create_user();
$student3 = $generator->create_user();
$generator->enrol_user($student1->id, $course->id);
$generator->enrol_user($student2->id, $course->id);
$generator->enrol_user($student3->id, $course->id);

$timestamp = 1234567890;
// This line is not really necessary for the test asserts below,
// but what it does is add an extra user row returned by
// get_enrolled_with_capabilities_join because of a second enrolment.
// The extra row returned used to make $table->query_db complain
// about duplicate records. So this is really a test that an extra
// student enrolment does not cause duplicate records in this query.
$generator->enrol_user($student2->id, $course->id, null, 'self');

// The test data.
$timestamp = 1234567890;
$fields = array('quiz', 'userid', 'attempt', 'sumgrades', 'state');
$attempts = array(
array($quiz->id, $student1->id, 1, 0.0, quiz_attempt::FINISHED),
array($quiz->id, $student1->id, 2, 5.0, quiz_attempt::FINISHED),
array($quiz->id, $student1->id, 3, 8.0, quiz_attempt::FINISHED),
array($quiz->id, $student1->id, 4, null, quiz_attempt::ABANDONED),
array($quiz->id, $student1->id, 5, null, quiz_attempt::IN_PROGRESS),
array($quiz->id, $student2->id, 1, null, quiz_attempt::ABANDONED),
array($quiz->id, $student2->id, 2, null, quiz_attempt::ABANDONED),
array($quiz->id, $student2->id, 3, 7.0, quiz_attempt::FINISHED),
array($quiz->id, $student2->id, 4, null, quiz_attempt::ABANDONED),
array($quiz->id, $student2->id, 5, null, quiz_attempt::ABANDONED),
array($quiz, $student1, 1, 0.0, quiz_attempt::FINISHED),
array($quiz, $student1, 2, 5.0, quiz_attempt::FINISHED),
array($quiz, $student1, 3, 8.0, quiz_attempt::FINISHED),
array($quiz, $student1, 4, null, quiz_attempt::ABANDONED),
array($quiz, $student1, 5, null, quiz_attempt::IN_PROGRESS),
array($quiz, $student2, 1, null, quiz_attempt::ABANDONED),
array($quiz, $student2, 2, null, quiz_attempt::ABANDONED),
array($quiz, $student2, 3, 7.0, quiz_attempt::FINISHED),
array($quiz, $student2, 4, null, quiz_attempt::ABANDONED),
array($quiz, $student2, 5, null, quiz_attempt::ABANDONED),
);

// Load it in to quiz attempts table.
$uniqueid = 1;
foreach ($attempts as $attempt) {
$data = array_combine($fields, $attempt);
$data['timestart'] = $timestamp + 3600 * $data['attempt'];
$data['timemodifed'] = $data['timestart'];
if ($data['state'] == quiz_attempt::FINISHED) {
$data['timefinish'] = $data['timestart'] + 600;
$data['timemodifed'] = $data['timefinish'];
foreach ($attempts as $attemptdata) {
list($quiz, $student, $attemptnumber, $sumgrades, $state) = $attemptdata;
$timestart = $timestamp + $attemptnumber * 3600;

$quizobj = quiz::create($quiz->id, $student->id);
$quba = question_engine::make_questions_usage_by_activity('mod_quiz', $quizobj->get_context());
$quba->set_preferred_behaviour($quizobj->get_quiz()->preferredbehaviour);

// Create the new attempt and initialize the question sessions.
$attempt = quiz_create_attempt($quizobj, $attemptnumber, null, $timestart, false, $student->id);

$attempt = quiz_start_new_attempt($quizobj, $quba, $attempt, $attemptnumber, $timestamp);
$attempt = quiz_attempt_save_started($quizobj, $quba, $attempt);

// Process some responses from the student.
$attemptobj = quiz_attempt::create($attempt->id);
switch ($state) {
case quiz_attempt::ABANDONED:
$attemptobj->process_abandon($timestart + 300, false);
break;

case quiz_attempt::IN_PROGRESS:
// Do nothing.
break;

case quiz_attempt::FINISHED:
// Save answer and finish attempt.
$attemptobj->process_submitted_actions($timestart + 300, false, [
1 => ['answer' => 'My essay by ' . $student->firstname, 'answerformat' => FORMAT_PLAIN]]);
$attemptobj->process_finish($timestart + 600, false);

// Manually grade it.
$quba = $attemptobj->get_question_usage();
$quba->get_question_attempt(1)->manual_grade(
'Comment', $sumgrades, FORMAT_HTML, $timestart + 1200);
question_engine::save_questions_usage_by_activity($quba);
$update = new stdClass();
$update->id = $attemptobj->get_attemptid();
$update->timemodified = $timestart + 1200;
$update->sumgrades = $quba->get_total_mark();
$DB->update_record('quiz_attempts', $update);
quiz_save_best_grade($attemptobj->get_quiz(), $student->id);
break;
}
$data['layout'] = ''; // Not used, but cannot be null.
$data['uniqueid'] = $uniqueid++;
$data['preview'] = 0;
$DB->insert_record('quiz_attempts', $data);
}

// This line is not really necessary for the test asserts below,
// but what it does is add an extra user row returned by
// get_enrolled_with_capabilities_join because of a second enrolment.
// The extra row returned used to make $table->query_db complain
// about duplicate records. So this is really a test that an extra
// student enrolment does not cause duplicate records in this query.
$generator->enrol_user($student2->id, $course->id, null, 'self');

// Actually getting the SQL to run is quite hard. Do a minimal set up of
// some objects.
$context = context_module::instance($quiz->cmid);
Expand All @@ -114,7 +166,8 @@ public function test_report_sql() {

// Now do a minimal set-up of the table class.
$table = new quiz_overview_table($quiz, $context, $qmsubselect, $reportoptions,
$empty, $studentsjoins, array(1), null);
$empty, $studentsjoins, array(1 => $q), null);
$table->download = $isdownloading; // Cannot call the is_downloading API, because it gives errors.
$table->define_columns(array('fullname'));
$table->sortable(true, 'uniqueid');
$table->define_baseurl(new moodle_url('/mod/quiz/report.php'));
Expand All @@ -126,6 +179,12 @@ public function test_report_sql() {
$table->set_count_sql("SELECT COUNT(1) FROM (SELECT $fields FROM $from WHERE $where) temp WHERE 1 = 1", $params);
$table->query_db(30, false);

// Should be 4 rows, matching count($table->rawdata) tested below.
// The count is only done if not downloading.
if (!$isdownloading) {
$this->assertEquals(4, $table->totalrows);
}

// Verify what was returned: Student 1's best and in progress attempts.
// Student 2's finshed attempt, and Student 3 with no attempt.
// The array key is {student id}#{attempt number}.
Expand Down

0 comments on commit 661e129

Please sign in to comment.