From eb4c46616283e0a5f09f134004a3975b7632e6fb Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 9 Jun 2016 08:53:46 +0800 Subject: [PATCH] MDL-54872 assign: Order blindly marked assignments more blindly --- mod/assign/gradingtable.php | 12 +- mod/assign/locallib.php | 40 +++++- mod/assign/tests/base_test.php | 4 +- .../tests/locallib_participants_test.php | 136 ++++++++++++++++++ mod/assign/tests/locallib_test.php | 21 +-- 5 files changed, 198 insertions(+), 15 deletions(-) create mode 100644 mod/assign/tests/locallib_participants_test.php diff --git a/mod/assign/gradingtable.php b/mod/assign/gradingtable.php index 85b02d98f90..6e5f271debf 100644 --- a/mod/assign/gradingtable.php +++ b/mod/assign/gradingtable.php @@ -1420,7 +1420,17 @@ class assign_grading_table extends table_sql implements renderable { */ public function get_sort_columns() { $result = parent::get_sort_columns(); - $result = array_merge($result, array('userid' => SORT_ASC)); + + $assignment = $this->assignment->get_instance(); + if (empty($assignment->blindmarking)) { + $result = array_merge($result, array('userid' => SORT_ASC)); + } else { + $result = array_merge($result, [ + 'COALESCE(s.timecreated, ' . time() . ')' => SORT_ASC, + 'COALESCE(s.id, ' . PHP_INT_MAX . ')' => SORT_ASC, + 'um.id' => SORT_ASC, + ]); + } return $result; } diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 0c13e069168..9c26a2c81f8 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -1564,6 +1564,7 @@ class assign { * @return array List of user records */ public function list_participants($currentgroup, $idsonly) { + global $DB; if (empty($currentgroup)) { $currentgroup = 0; @@ -1571,13 +1572,42 @@ class assign { $key = $this->context->id . '-' . $currentgroup . '-' . $this->show_only_active_users(); if (!isset($this->participants[$key])) { - $order = 'u.lastname, u.firstname, u.id'; - if ($this->is_blind_marking()) { - $order = 'u.id'; - } - $users = get_enrolled_users($this->context, 'mod/assign:submit', $currentgroup, 'u.*', $order, null, null, + list($esql, $params) = get_enrolled_sql($this->context, 'mod/assign:submit', $currentgroup, $this->show_only_active_users()); + $fields = 'u.*'; + $orderby = 'u.lastname, u.firstname, u.id'; + $additionaljoins = ''; + $instance = $this->get_instance(); + if (!empty($instance->blindmarking)) { + $additionaljoins .= " LEFT JOIN {assign_user_mapping} um + ON u.id = um.userid + AND um.assignment = :assignmentid1 + LEFT JOIN {assign_submission} s + ON u.id = s.userid + AND s.assignment = :assignmentid2 + AND s.latest = 1 + "; + $params['assignmentid1'] = (int) $instance->id; + $params['assignmentid2'] = (int) $instance->id; + $fields .= ', um.id as recordid '; + + // Sort by submission time first, then by um.id to sort reliably by the blind marking id. + // Note, different DBs have different ordering of NULL values. + // Therefore we coalesce the current time into the timecreated field, and the max possible integer into + // the ID field. + $orderby = "COALESCE(s.timecreated, " . time() . ") ASC, COALESCE(s.id, " . PHP_INT_MAX . ") ASC, um.id ASC"; + } + + $sql = "SELECT $fields + FROM {user} u + JOIN ($esql) je ON je.id = u.id + $additionaljoins + WHERE u.deleted = 0 + ORDER BY $orderby"; + + $users = $DB->get_records_sql($sql, $params); + $cm = $this->get_course_module(); $info = new \core_availability\info_module($cm); $users = $info->filter_user_list($users); diff --git a/mod/assign/tests/base_test.php b/mod/assign/tests/base_test.php index 5c92b96e622..ab2a0564d6a 100644 --- a/mod/assign/tests/base_test.php +++ b/mod/assign/tests/base_test.php @@ -205,7 +205,9 @@ class mod_assign_base_testcase extends advanced_testcase { */ protected function create_instance($params=array()) { $generator = $this->getDataGenerator()->get_plugin_generator('mod_assign'); - $params['course'] = $this->course->id; + if (!isset($params['course'])) { + $params['course'] = $this->course->id; + } $instance = $generator->create_instance($params); $cm = get_coursemodule_from_instance('assign', $instance->id); $context = context_module::instance($cm->id); diff --git a/mod/assign/tests/locallib_participants_test.php b/mod/assign/tests/locallib_participants_test.php new file mode 100644 index 00000000000..fd8e54f67e8 --- /dev/null +++ b/mod/assign/tests/locallib_participants_test.php @@ -0,0 +1,136 @@ +. + +/** + * Unit tests for (some of) mod/assign/locallib.php. + * + * @package mod_assign + * @category phpunit + * @copyright 1999 onwards Martin Dougiamas {@link http://moodle.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once(__DIR__ . '/../locallib.php'); + +class mod_assign_locallib_participants extends advanced_testcase { + public function test_list_participants_blind_marking() { + global $DB; + $this->resetAfterTest(true); + + $course = $this->getDataGenerator()->create_course(); + + $roles = $DB->get_records('role', null, '', 'shortname, id'); + $teacher = $this->getDataGenerator()->create_user(); + + $this->getDataGenerator()->enrol_user($teacher->id, + $course->id, + $roles['teacher']->id); + + $this->setUser($teacher); + + // Enrol two students. + $students = []; + for ($i = 0; $i < 2; $i++) { + $student = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($student->id, + $course->id, + $roles['student']->id); + $students[$student->id] = $student; + } + + $generator = $this->getDataGenerator()->get_plugin_generator('mod_assign'); + $instance = $generator->create_instance(['course' => $course->id, 'blindmarking' => 1]); + $cm = get_coursemodule_from_instance('assign', $instance->id); + $context = context_module::instance($cm->id); + $assign = new assign($context, $cm, $course); + + $participants = $assign->list_participants(null, false); + + // There should be exactly two participants and they should be the students. + $this->assertCount(2, $participants); + foreach ($participants as $participant) { + $this->assertArrayHasKey($participant->id, $students); + } + + $keys = array_keys($participants); + + // Create a grading table, and query the DB This should have the same order. + $table = new assign_grading_table($assign, 10, '', 0, false); + $table->setup(); + $table->query_db(10); + $this->assertEquals($keys, array_keys($table->rawdata)); + + // Submit a file for the second student. + $data = new stdClass(); + $data->onlinetext_editor = array('itemid'=>file_get_unused_draft_itemid(), + 'text'=>'Submission text', + 'format'=>FORMAT_MOODLE); + static::helper_add_submission($assign, $participants[$keys[1]], $data, 'onlinetext'); + + // Assign has a private cache. The easiest way to clear this is to create a new instance. + $assign = new assign($context, $cm, $course); + + $newparticipants = $assign->list_participants(null, false); + + // There should be exactly two participants and they should be the students. + $this->assertCount(2, $newparticipants); + foreach ($newparticipants as $participant) { + $this->assertArrayHasKey($participant->id, $students); + } + $newkeys = array_keys($newparticipants); + + // The user who submitted first should now be listed first. + $this->assertEquals($participants[$keys[1]]->id, $newparticipants[$newkeys[0]]->id); + $this->assertEquals($participants[$keys[0]]->id, $newparticipants[$newkeys[1]]->id); + + // Submit for the other student. + static::helper_add_submission($assign, $participants[$keys[0]], $data, 'onlinetext'); + $assign = new assign($context, $cm, $course); + $newparticipants = $assign->list_participants(null, false); + + // The users should still be listed in order of the first submission + $this->assertEquals($participants[$keys[1]]->id, $newparticipants[$newkeys[0]]->id); + $this->assertEquals($participants[$keys[0]]->id, $newparticipants[$newkeys[1]]->id); + + // The updated grading table should have the same order as the updated participant list. + $table->query_db(10); + $this->assertEquals($newkeys, array_keys($table->rawdata)); + } + + public function helper_add_submission($assign, $user, $data, $type) { + global $USER; + + $previoususer = $USER; + + $this->setUser($user); + $submission = $assign->get_user_submission($user->id, true); + $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED; + + $rc = new ReflectionClass('assign'); + $rcm = $rc->getMethod('update_submission'); + $rcm->setAccessible(true); + $rcm->invokeArgs($assign, [$submission, $user->id, true, false]); + + $plugin = $assign->get_submission_plugin_by_type($type); + $plugin->save($submission, $data); + + $this->setUser($previoususer); + } +} diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index e3c3adb5a03..b41a3549d92 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -2482,26 +2482,31 @@ Anchor link 2:Link text * Test if the view blind details capability works */ public function test_can_view_blind_details() { - global $PAGE, $DB; - $teacherrole = $DB->get_record('role', array('shortname' => 'teacher')); - $managerrole = $DB->get_record('role', array('shortname' => 'manager')); + global $DB; + // Note: The shared setUp leads to terrible tests. Please don't use it. + $roles = $DB->get_records('role', null, '', 'shortname, id'); + $course = $this->getDataGenerator()->create_course([]); $student = $this->students[0];// Get a student user. + $this->getDataGenerator()->enrol_user($student->id, + $course->id, + $roles['student']->id); + // Create a teacher. Shouldn't be able to view blind marking ID. $teacher = $this->getDataGenerator()->create_user(); $this->getDataGenerator()->enrol_user($teacher->id, - $this->course->id, - $teacherrole->id); + $course->id, + $roles['teacher']->id); // Create a manager.. Should be able to view blind marking ID. $manager = $this->getDataGenerator()->create_user(); $this->getDataGenerator()->enrol_user($manager->id, - $this->course->id, - $managerrole->id); + $course->id, + $roles['manager']->id); // Generate blind marking assignment. - $assign = $this->create_instance(array('blindmarking' => 1)); + $assign = $this->create_instance(array('course' => $course->id, 'blindmarking' => 1)); $this->assertEquals(true, $assign->is_blind_marking()); // Test student names are hidden to teacher.