From 184f3b6814bbe874efe28d8e41cffc5c9b2fdde1 Mon Sep 17 00:00:00 2001 From: David Monllao Date: Wed, 19 Nov 2014 14:37:12 +0800 Subject: [PATCH 1/3] MDL-38128 mod_assign: Adding group selector to grading summary page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks to Pau Ferrer Ocaña and Avi Levy for their work on this issue. --- mod/assign/locallib.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 0c022876362..f8a94080300 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -4132,6 +4132,11 @@ class assign { if ($this->can_view_grades()) { $draft = ASSIGN_SUBMISSION_STATUS_DRAFT; $submitted = ASSIGN_SUBMISSION_STATUS_SUBMITTED; + + // Group selector will only be displayed if necessary. + $currenturl = new moodle_url('/mod/assign/view.php', array('id' => $this->get_course_module()->id)); + $o .= groups_print_activity_menu($this->get_course_module(), $currenturl->out(), true); + if ($instance->teamsubmission) { $summary = new assign_grading_summary($this->count_teams(), $instance->submissiondrafts, @@ -4145,7 +4150,9 @@ class assign { $instance->teamsubmission); $o .= $this->get_renderer()->render($summary); } else { - $summary = new assign_grading_summary($this->count_participants(0), + // The active group has already been updated in groups_print_activity_menu(). + $countparticipants = $this->count_participants(groups_get_activity_group($this->get_course_module())); + $summary = new assign_grading_summary($countparticipants, $instance->submissiondrafts, $this->count_submissions_with_status($draft), $this->is_any_submission_plugin_enabled(), From bc006a666c30faadfca5e4356e9c7e8fe249b11e Mon Sep 17 00:00:00 2001 From: David Monllao Date: Mon, 1 Dec 2014 16:33:50 +0800 Subject: [PATCH 2/3] MDL-38128 mod_assign: Fixing count teams and submissions with status We should restrict the groups to the activity active group. Also improving performance caching get_submission_group() results. --- mod/assign/locallib.php | 96 ++++++++++++++++++++++++++++++++++------- 1 file changed, 81 insertions(+), 15 deletions(-) diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index f8a94080300..35de806a1a1 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -139,6 +139,9 @@ class assign { /** @var array cached list of participants for this assignment. The cache key will be group, showactive and the context id */ private $participants = array(); + /** @var array cached list of user groups when team submissions are enabled. The cache key will be the user. */ + private $usersubmissiongroups = array(); + /** * Constructor for the base assign class. * @@ -1380,6 +1383,11 @@ class assign { * @return array List of user records */ public function list_participants($currentgroup, $idsonly) { + + if (empty($currentgroup)) { + $currentgroup = 0; + } + $key = $this->context->id . '-' . $currentgroup . '-' . $this->show_only_active_users(); if (!isset($this->participants[$key])) { $users = get_enrolled_users($this->context, 'mod/assign:submit', $currentgroup, 'u.*', null, null, null, @@ -1406,21 +1414,51 @@ class assign { /** * Load a count of valid teams for this assignment. * + * @param int $activitygroup Activity active group * @return int number of valid teams */ - public function count_teams() { + public function count_teams($activitygroup = 0) { - $groups = groups_get_all_groups($this->get_course()->id, - 0, - $this->get_instance()->teamsubmissiongroupingid, - 'g.id'); - $count = count($groups); + $count = 0; - // See if there are any users in the default group. - $defaultusers = $this->get_submission_group_members(0, true); - if (count($defaultusers) > 0) { - $count += 1; + $participants = $this->list_participants($activitygroup, true); + + // If a team submission grouping id is provided all good as all returned groups + // are the submission teams, but if no team submission grouping was specified + // $groups will contain all participants groups. + if ($this->get_instance()->teamsubmissiongroupingid) { + + // We restrict the users to the selected group ones. + $groups = groups_get_all_groups($this->get_course()->id, + array_keys($participants), + $this->get_instance()->teamsubmissiongroupingid, + 'DISTINCT g.id, g.name'); + + $count = count($groups); + + // When a specific group is selected we don't count the default group users. + if ($activitygroup == 0) { + + // See if there are any users in the default group. + $defaultusers = $this->get_submission_group_members(0, true); + if (count($defaultusers) > 0) { + $count += 1; + } + } + } else { + // It is faster to loop around participants if no grouping was specified. + $groups = array(); + foreach ($participants as $participant) { + if ($group = $this->get_submission_group($participant->id)) { + $groups[$group->id] = true; + } else { + $groups[0] = true; + } + } + + $count = count($groups); } + return $count; } @@ -1568,13 +1606,27 @@ class assign { $params['submissionstatus'] = $status; if ($this->get_instance()->teamsubmission) { + + $groupsstr = ''; + if ($currentgroup != 0) { + // If there is an active group we should only display the current group users groups. + $participants = $this->list_participants($currentgroup, true); + $groups = groups_get_all_groups($this->get_course()->id, + array_keys($participants), + $this->get_instance()->teamsubmissiongroupingid, + 'DISTINCT g.id, g.name'); + list($groupssql, $groupsparams) = $DB->get_in_or_equal(array_keys($groups), SQL_PARAMS_NAMED); + $groupsstr = 's.groupid ' . $groupssql . ' AND'; + $params = $params + $groupsparams; + } $sql = 'SELECT COUNT(s.groupid) FROM {assign_submission} s WHERE s.latest = 1 AND s.assignment = :assignid AND s.timemodified IS NOT NULL AND - s.userid = :groupuserid AND + s.userid = :groupuserid AND ' + . $groupsstr . ' s.status = :submissionstatus'; $params['groupuserid'] = 0; } else { @@ -1999,6 +2051,7 @@ class assign { } } } + return $members; } @@ -2221,12 +2274,23 @@ class assign { * @return mixed The group or false */ public function get_submission_group($userid) { + + if (isset($this->usersubmissiongroups[$userid])) { + return $this->usersubmissiongroups[$userid]; + } + $grouping = $this->get_instance()->teamsubmissiongroupingid; $groups = groups_get_all_groups($this->get_course()->id, $userid, $grouping); if (count($groups) != 1) { - return false; + $return = false; + } else { + $return = array_pop($groups); } - return array_pop($groups); + + // Cache the user submission group. + $this->usersubmissiongroups[$userid] = $return; + + return $return; } @@ -4137,8 +4201,10 @@ class assign { $currenturl = new moodle_url('/mod/assign/view.php', array('id' => $this->get_course_module()->id)); $o .= groups_print_activity_menu($this->get_course_module(), $currenturl->out(), true); + $activitygroup = groups_get_activity_group($this->get_course_module()); + if ($instance->teamsubmission) { - $summary = new assign_grading_summary($this->count_teams(), + $summary = new assign_grading_summary($this->count_teams($activitygroup), $instance->submissiondrafts, $this->count_submissions_with_status($draft), $this->is_any_submission_plugin_enabled(), @@ -4151,7 +4217,7 @@ class assign { $o .= $this->get_renderer()->render($summary); } else { // The active group has already been updated in groups_print_activity_menu(). - $countparticipants = $this->count_participants(groups_get_activity_group($this->get_course_module())); + $countparticipants = $this->count_participants($activitygroup); $summary = new assign_grading_summary($countparticipants, $instance->submissiondrafts, $this->count_submissions_with_status($draft), From 2dda9417ddc61e06e59580f39f18f1469bf0c575 Mon Sep 17 00:00:00 2001 From: David Monllao Date: Wed, 3 Dec 2014 14:58:03 +0800 Subject: [PATCH 3/3] MDL-38128 mod_assign: Counts and groups unit tests --- mod/assign/tests/locallib_test.php | 120 +++++++++++++++++++++-------- 1 file changed, 90 insertions(+), 30 deletions(-) diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index 94d10fdfe54..8528f82ff3b 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -633,13 +633,23 @@ class mod_assign_locallib_testcase extends mod_assign_base_testcase { public function test_count_teams() { $this->create_extra_users(); $this->setUser($this->editingteachers[0]); - $assign = $this->create_instance(array('teamsubmission'=>1)); + $assign1 = $this->create_instance(array('teamsubmission' => 1)); + $this->assertEquals(self::GROUP_COUNT + 1, $assign1->count_teams()); - $this->assertEquals(self::GROUP_COUNT + 1, $assign->count_teams()); + $grouping = $this->getDataGenerator()->create_grouping(array('courseid' => $this->course->id)); + $this->getDataGenerator()->create_grouping_group(array('groupid' => $this->groups[0]->id, 'groupingid' => $grouping->id)); + $this->getDataGenerator()->create_grouping_group(array('groupid' => $this->groups[1]->id, 'groupingid' => $grouping->id)); + + // No active group => 2 groups + the default one. + $assign2 = $this->create_instance(array('teamsubmission' => 1, 'teamsubmissiongroupingid' => $grouping->id)); + $this->assertEquals(3, $assign2->count_teams()); + + // An active group => Just the selected one. + $this->assertEquals(1, $assign2->count_teams($this->groups[0]->id)); } public function test_submit_to_default_group() { - global $DB; + global $DB, $SESSION; $this->preventResetByRollback(); $sink = $this->redirectMessages(); @@ -647,7 +657,8 @@ class mod_assign_locallib_testcase extends mod_assign_base_testcase { $this->setUser($this->editingteachers[0]); $params = array('teamsubmission' => 1, 'assignsubmission_onlinetext_enabled' => 1, - 'submissiondrafts'=>0); + 'submissiondrafts' => 0, + 'groupmode' => VISIBLEGROUPS); $assign = $this->create_instance($params); $newstudent = $this->getDataGenerator()->create_user(); @@ -667,98 +678,147 @@ class mod_assign_locallib_testcase extends mod_assign_base_testcase { $assign->save_submission($data, $notices); $this->assertEmpty($notices, 'No errors on save submission'); + // Set active groups to all groups. + $this->setUser($this->teachers[0]); + $SESSION->activegroup[$this->course->id]['aag'][0] = 0; + $this->assertEquals(1, $assign->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_SUBMITTED)); + + // Set an active group. + $anothergroup = $this->groups[0]; + $SESSION->activegroup[$this->course->id]['aag'][0] = (int)$anothergroup->id; + $this->assertEquals(0, $assign->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_SUBMITTED)); + $sink->close(); } public function test_count_submissions() { + global $SESSION; + $this->create_extra_users(); $this->setUser($this->editingteachers[0]); - $assign = $this->create_instance(array('assignsubmission_onlinetext_enabled'=>1)); + $assign1 = $this->create_instance(array('assignsubmission_onlinetext_enabled' => 1)); // Simulate a submission. $this->setUser($this->extrastudents[0]); - $submission = $assign->get_user_submission($this->extrastudents[0]->id, true); + $submission = $assign1->get_user_submission($this->extrastudents[0]->id, true); $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT; - $assign->testable_update_submission($submission, $this->extrastudents[0]->id, true, false); + $assign1->testable_update_submission($submission, $this->extrastudents[0]->id, true, false); // Leave this one as DRAFT. $data = new stdClass(); $data->onlinetext_editor = array('itemid'=>file_get_unused_draft_itemid(), 'text'=>'Submission text', 'format'=>FORMAT_MOODLE); - $plugin = $assign->get_submission_plugin_by_type('onlinetext'); + $plugin = $assign1->get_submission_plugin_by_type('onlinetext'); $plugin->save($submission, $data); // Simulate adding a grade. $this->setUser($this->teachers[0]); $data = new stdClass(); $data->grade = '50.0'; - $assign->testable_apply_grade_to_user($data, $this->extrastudents[0]->id, 0); + $assign1->testable_apply_grade_to_user($data, $this->extrastudents[0]->id, 0); // Simulate a submission. $this->setUser($this->extrastudents[1]); - $submission = $assign->get_user_submission($this->extrastudents[1]->id, true); + $submission = $assign1->get_user_submission($this->extrastudents[1]->id, true); $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED; - $assign->testable_update_submission($submission, $this->extrastudents[1]->id, true, false); + $assign1->testable_update_submission($submission, $this->extrastudents[1]->id, true, false); $data = new stdClass(); $data->onlinetext_editor = array('itemid'=>file_get_unused_draft_itemid(), 'text'=>'Submission text', 'format'=>FORMAT_MOODLE); - $plugin = $assign->get_submission_plugin_by_type('onlinetext'); + $plugin = $assign1->get_submission_plugin_by_type('onlinetext'); $plugin->save($submission, $data); // Simulate a submission. $this->setUser($this->extrastudents[2]); - $submission = $assign->get_user_submission($this->extrastudents[2]->id, true); + $submission = $assign1->get_user_submission($this->extrastudents[2]->id, true); $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED; - $assign->testable_update_submission($submission, $this->extrastudents[2]->id, true, false); + $assign1->testable_update_submission($submission, $this->extrastudents[2]->id, true, false); $data = new stdClass(); $data->onlinetext_editor = array('itemid'=>file_get_unused_draft_itemid(), 'text'=>'Submission text', 'format'=>FORMAT_MOODLE); - $plugin = $assign->get_submission_plugin_by_type('onlinetext'); + $plugin = $assign1->get_submission_plugin_by_type('onlinetext'); $plugin->save($submission, $data); // Simulate a submission. $this->setUser($this->extrastudents[3]); - $submission = $assign->get_user_submission($this->extrastudents[3]->id, true); + $submission = $assign1->get_user_submission($this->extrastudents[3]->id, true); $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED; - $assign->testable_update_submission($submission, $this->extrastudents[3]->id, true, false); + $assign1->testable_update_submission($submission, $this->extrastudents[3]->id, true, false); $data = new stdClass(); $data->onlinetext_editor = array('itemid'=>file_get_unused_draft_itemid(), 'text'=>'Submission text', 'format'=>FORMAT_MOODLE); - $plugin = $assign->get_submission_plugin_by_type('onlinetext'); + $plugin = $assign1->get_submission_plugin_by_type('onlinetext'); $plugin->save($submission, $data); // Simulate a submission for suspended user, this will never be counted. $this->setUser($this->extrastudents[3]); - $submission = $assign->get_user_submission($this->extrasuspendedstudents[0]->id, true); + $submission = $assign1->get_user_submission($this->extrasuspendedstudents[0]->id, true); $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED; - $assign->testable_update_submission($submission, $this->extrasuspendedstudents[0]->id, true, false); + $assign1->testable_update_submission($submission, $this->extrasuspendedstudents[0]->id, true, false); $data = new stdClass(); $data->onlinetext_editor = array('itemid'=>file_get_unused_draft_itemid(), 'text'=>'Submission text', 'format'=>FORMAT_MOODLE); - $plugin = $assign->get_submission_plugin_by_type('onlinetext'); + $plugin = $assign1->get_submission_plugin_by_type('onlinetext'); $plugin->save($submission, $data); // Simulate adding a grade. $this->setUser($this->teachers[0]); $data = new stdClass(); $data->grade = '50.0'; - $assign->testable_apply_grade_to_user($data, $this->extrastudents[3]->id, 0); - $assign->testable_apply_grade_to_user($data, $this->extrasuspendedstudents[0]->id, 0); + $assign1->testable_apply_grade_to_user($data, $this->extrastudents[3]->id, 0); + $assign1->testable_apply_grade_to_user($data, $this->extrasuspendedstudents[0]->id, 0); // Create a new submission with status NEW. $this->setUser($this->extrastudents[4]); - $submission = $assign->get_user_submission($this->extrastudents[4]->id, true); + $submission = $assign1->get_user_submission($this->extrastudents[4]->id, true); - $this->assertEquals(2, $assign->count_grades()); - $this->assertEquals(4, $assign->count_submissions()); - $this->assertEquals(5, $assign->count_submissions(true)); - $this->assertEquals(2, $assign->count_submissions_need_grading()); - $this->assertEquals(3, $assign->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_SUBMITTED)); - $this->assertEquals(1, $assign->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_DRAFT)); + $this->assertEquals(2, $assign1->count_grades()); + $this->assertEquals(4, $assign1->count_submissions()); + $this->assertEquals(5, $assign1->count_submissions(true)); + $this->assertEquals(2, $assign1->count_submissions_need_grading()); + $this->assertEquals(3, $assign1->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_SUBMITTED)); + $this->assertEquals(1, $assign1->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_DRAFT)); + + // Groups. + $assign2 = $this->create_instance(array( + 'assignsubmission_onlinetext_enabled' => 1, + 'groupmode' => VISIBLEGROUPS + )); + + $this->setUser($this->extrastudents[1]); + $submission = $assign2->get_user_submission($this->extrastudents[1]->id, true); + $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED; + $assign2->testable_update_submission($submission, $this->extrastudents[1]->id, true, false); + $data = new stdClass(); + $data->onlinetext_editor = array('itemid' => file_get_unused_draft_itemid(), + 'text' => 'Submission text', + 'format' => FORMAT_MOODLE); + $plugin = $assign2->get_submission_plugin_by_type('onlinetext'); + $plugin->save($submission, $data); + + $this->assertEquals(1, $assign2->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_SUBMITTED)); + + // Set active groups to all groups. + $this->setUser($this->teachers[0]); + $SESSION->activegroup[$this->course->id]['aag'][0] = 0; + $this->assertEquals(1, $assign2->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_SUBMITTED)); + + // Set the user group. + $studentgroups = groups_get_user_groups($this->course->id, $this->extrastudents[1]->id); + $this->assertEquals(1, count($studentgroups)); + $studentgroup = array_pop($studentgroups); + $SESSION->activegroup[$this->course->id]['aag'][0] = $studentgroup[0]; + $this->assertEquals(1, $assign2->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_SUBMITTED)); + + // Set another group. + $anothergroup = $this->groups[0]; + $this->assertNotEquals($anothergroup->id, $studentgroup[0]); + $SESSION->activegroup[$this->course->id]['aag'][0] = (int)$anothergroup->id; + $this->assertEquals(0, $assign2->count_submissions_with_status(ASSIGN_SUBMISSION_STATUS_SUBMITTED)); } public function test_count_submissions_for_groups() {