From 34c9d14bb365906b61752b7ac0f9a4b92576dc0b Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Tue, 2 Dec 2014 13:40:47 +0800 Subject: [PATCH 1/2] MDL-48273 singleview: add load_user method and other group validations All credits goes to Damyon, thanks! --- .../singleview/classes/local/screen/grade.php | 14 +++--------- .../singleview/classes/local/screen/screen.php | 25 ++++++++++++++++++++++ .../singleview/classes/local/screen/user.php | 8 +++++-- grade/report/singleview/index.php | 7 ++---- grade/report/singleview/lib.php | 25 +++++++++++----------- grade/report/upgrade.txt | 3 +++ 6 files changed, 52 insertions(+), 30 deletions(-) diff --git a/grade/report/singleview/classes/local/screen/grade.php b/grade/report/singleview/classes/local/screen/grade.php index 51346d1..2905a9a 100644 --- a/grade/report/singleview/classes/local/screen/grade.php +++ b/grade/report/singleview/classes/local/screen/grade.php @@ -121,17 +121,9 @@ class grade extends tablelike implements selectable_items, filterable_items { * @param bool $selfitemisempty True if we have not selected a user. */ public function init($selfitemisempty = false) { - $roleids = explode(',', get_config('moodle', 'gradebookroles')); - - $this->items = array(); - foreach ($roleids as $roleid) { - // Keeping the first user appearance. - $this->items = $this->items + get_role_users( - $roleid, $this->context, false, '', - 'u.lastname, u.firstname', null, $this->groupid); - } - $this->totalitemcount = count_role_users($roleids, $this->context); + $this->items = $this->load_users(); + $this->totalitemcount = count($this->items); if ($selfitemisempty) { return; @@ -273,7 +265,7 @@ class grade extends tablelike implements selectable_items, filterable_items { new moodle_url('/grade/report/singleview/index.php', array( 'perpage' => $this->perpage, 'id' => $this->courseid, - 'groupid' => $this->groupid, + 'group' => $this->groupid, 'itemid' => $this->itemid, 'item' => 'grade' )) diff --git a/grade/report/singleview/classes/local/screen/screen.php b/grade/report/singleview/classes/local/screen/screen.php index 0550af3..f31c1d2 100644 --- a/grade/report/singleview/classes/local/screen/screen.php +++ b/grade/report/singleview/classes/local/screen/screen.php @@ -375,4 +375,29 @@ abstract class screen { public function supports_next_prev() { return true; } + + /** + * Load a valid list of users for this gradebook as the screen "items". + * @return array $users A list of enroled users. + */ + protected function load_users() { + global $CFG; + + // Create a graded_users_iterator because it will properly check the groups etc. + $defaultgradeshowactiveenrol = !empty($CFG->grade_report_showonlyactiveenrol); + $showonlyactiveenrol = get_user_preferences('grade_report_showonlyactiveenrol', $defaultgradeshowactiveenrol); + $showonlyactiveenrol = $showonlyactiveenrol || !has_capability('moodle/course:viewsuspendedusers', $this->context); + + require_once($CFG->dirroot.'/grade/lib.php'); + $gui = new \graded_users_iterator($this->course, null, $this->groupid); + $gui->require_active_enrolment($showonlyactiveenrol); + $gui->init(); + + // Flatten the users. + $users = array(); + while ($user = $gui->next_user()) { + $users[$user->user->id] = $user->user; + } + return $users; + } } diff --git a/grade/report/singleview/classes/local/screen/user.php b/grade/report/singleview/classes/local/screen/user.php index cf3fddd..6993237 100644 --- a/grade/report/singleview/classes/local/screen/user.php +++ b/grade/report/singleview/classes/local/screen/user.php @@ -101,7 +101,11 @@ class user extends tablelike implements selectable_items { global $DB; if (!$selfitemisempty) { - $this->item = $DB->get_record('user', array('id' => $this->itemid)); + $validusers = $this->load_users(); + if (!isset($validusers[$this->itemid])) { + print_error('invaliduserid'); + } + $this->item = $validusers[$this->itemid]; } $params = array('courseid' => $this->courseid); @@ -307,7 +311,7 @@ class user extends tablelike implements selectable_items { new moodle_url('/grade/report/singleview/index.php', array( 'perpage' => $this->perpage, 'id' => $this->courseid, - 'groupid' => $this->groupid, + 'group' => $this->groupid, 'itemid' => $this->itemid, 'item' => 'user' )) diff --git a/grade/report/singleview/index.php b/grade/report/singleview/index.php index a355b28..42e99bf 100644 --- a/grade/report/singleview/index.php +++ b/grade/report/singleview/index.php @@ -76,10 +76,7 @@ $USER->grade_last_report[$course->id] = 'singleview'; // this must be done before constructing of the grade tree. grade_regrade_final_grades($courseid); -$report = new gradereport_singleview( - $courseid, $gpr, $context, - $itemtype, $itemid, $groupid -); +$report = new gradereport_singleview($courseid, $gpr, $context, $itemtype, $itemid); $reportname = $report->screen->heading(); @@ -130,7 +127,7 @@ if (!empty($options)) { $relreport = new gradereport_singleview( $courseid, $gpr, $context, - $report->screen->item_type(), $optionitemid, $groupid + $report->screen->item_type(), $optionitemid ); $reloptions = $relreport->screen->options(); $reloptionssorting = array_keys($relreport->screen->options()); diff --git a/grade/report/singleview/lib.php b/grade/report/singleview/lib.php index 5382c6c..b40711b 100644 --- a/grade/report/singleview/lib.php +++ b/grade/report/singleview/lib.php @@ -74,18 +74,11 @@ class gradereport_singleview extends grade_report { * @param context_course $context * @param string $itemtype Should be user, select or grade * @param int $itemid The id of the user or grade item - * @param int $groupid (optional) The current groupid. + * @param string $unused Used to be group id but that was removed and this is now unused. */ - public function __construct($courseid, $gpr, $context, $itemtype, $itemid, $groupid=null) { + public function __construct($courseid, $gpr, $context, $itemtype, $itemid, $unused = null) { parent::__construct($courseid, $gpr, $context); - $screenclass = "\\gradereport_singleview\\local\\screen\\${itemtype}"; - - $this->screen = new $screenclass($courseid, $itemid, $groupid); - - // Load custom or predifined js. - $this->screen->js(); - $base = '/grade/report/singleview/index.php'; $idparams = array('id' => $courseid); @@ -93,11 +86,19 @@ class gradereport_singleview extends grade_report { $this->baseurl = new moodle_url($base, $idparams); $this->pbarurl = new moodle_url($base, $idparams + array( - 'item' => $itemtype, - 'itemid' => $itemid - )); + 'item' => $itemtype, + 'itemid' => $itemid + )); + // The setup_group method is used to validate group mode and permissions and define the currentgroup value. $this->setup_groups(); + + $screenclass = "\\gradereport_singleview\\local\\screen\\${itemtype}"; + + $this->screen = new $screenclass($courseid, $itemid, $this->currentgroup); + + // Load custom or predifined js. + $this->screen->js(); } /** diff --git a/grade/report/upgrade.txt b/grade/report/upgrade.txt index 748b210..ad6d600 100644 --- a/grade/report/upgrade.txt +++ b/grade/report/upgrade.txt @@ -1,6 +1,9 @@ This files describes API changes in /grade/report/*, information provided here is intended especially for developers. +=== 2.8.1 === +* gradereport_singleview::__construct doesn't need groupid parameter anymore, so it was renamed to $unused. + === 2.6.5, 2.7.2 === * The callback function grade_report_*_profilereport now takes one more parameter $viewasuser. This parameter -- 1.9.1 From 5f3348581ecdacfc650560a4fadf00e891e98035 Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Tue, 2 Dec 2014 13:41:15 +0800 Subject: [PATCH 2/2] MDL-48273 singleview: unit tests of load_user method --- grade/report/singleview/tests/fixtures/screen.php | 52 +++++++++++++ grade/report/singleview/tests/screen_test.php | 92 +++++++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 grade/report/singleview/tests/fixtures/screen.php create mode 100644 grade/report/singleview/tests/screen_test.php diff --git a/grade/report/singleview/tests/fixtures/screen.php b/grade/report/singleview/tests/fixtures/screen.php new file mode 100644 index 0000000..33ae6e3 --- /dev/null +++ b/grade/report/singleview/tests/fixtures/screen.php @@ -0,0 +1,52 @@ +. + +/** + * Fixtures for single view report screen class testing. + * + * @package gradereport_singleview + * @copyright 2014 onwards Simey Lameze + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +class gradereport_singleview_screen_testable extends \gradereport_singleview\local\screen\screen { + + /** + * Wrapper to make protected method accessible during testing. + * + * @return array returns array of users. + */ + public function test_load_users() { + return $this->load_users(); + } + + /** + * Return the HTML for the page. + */ + public function init($selfitemisempty = false) {} + + /** + * Get the type of items on this screen, not valid so return false. + */ + public function item_type() {} + + /** + * Return the HTML for the page. + */ + public function html() {} +} diff --git a/grade/report/singleview/tests/screen_test.php b/grade/report/singleview/tests/screen_test.php new file mode 100644 index 0000000..2476d08 --- /dev/null +++ b/grade/report/singleview/tests/screen_test.php @@ -0,0 +1,92 @@ +. + +/** + * Unit tests for gradereport_singleview screen class. + * + * @package gradereport_singleview + * @category test + * @copyright 2014 onwards Simey Lameze + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + + +require_once(__DIR__ . '/fixtures/screen.php'); + +defined('MOODLE_INTERNAL') || die(); +/** + * Tests for screen class. + * + * Class gradereport_singleview_screen_testcase. + */ +class gradereport_singleview_screen_testcase extends advanced_testcase { + + /** + * Test load_users method. + */ + public function test_load_users() { + global $DB; + + $this->setAdminUser(); + $this->resetAfterTest(true); + + $roleteacher = $DB->get_record('role', array('shortname' => 'teacher'), '*', MUST_EXIST); + + // Create a course, users and groups. + $course = $this->getDataGenerator()->create_course(); + $coursecontext = context_course::instance($course->id); + $group = $this->getDataGenerator()->create_group(array('courseid' => $course->id)); + $teacher = $this->getDataGenerator()->create_user(); + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($teacher->id, $course->id, $roleteacher->id); + $this->getDataGenerator()->enrol_user($user1->id, $course->id); + $this->getDataGenerator()->enrol_user($user2->id, $course->id); + $this->getDataGenerator()->create_group_member(array('groupid' => $group->id, 'userid' => $teacher->id)); + $this->getDataGenerator()->create_group_member(array('groupid' => $group->id, 'userid' => $user1->id)); + $this->getDataGenerator()->create_group_member(array('groupid' => $group->id, 'userid' => $user2->id)); + + $screentest = new gradereport_singleview_screen_testable($course->id, 0, $group->id); + $groupusers = $screentest->test_load_users(); + $this->assertCount(2, $groupusers); + + // Now, let's suspend the enrolment of a user. Should return only one user. + $this->getDataGenerator()->enrol_user($user2->id, $course->id, $roleteacher->id, 'manual', 0, 0, ENROL_USER_SUSPENDED); + $users = $screentest->test_load_users(); + $this->assertCount(1, $users); + + // Change the viewsuspendedusers capabilities and set the user preference to display suspended users. + assign_capability('moodle/course:viewsuspendedusers', CAP_ALLOW, $roleteacher->id, $coursecontext, true); + set_user_preference('grade_report_showonlyactiveenrol', false, $teacher); + accesslib_clear_all_caches_for_unit_testing(); + $this->setUser($teacher); + $screentest = new gradereport_singleview_screen_testable($course->id, 0, $group->id); + $users = $screentest->test_load_users(); + $this->assertCount(2, $users); + + // Change the capability again, now the user can't see the suspended enrolments. + assign_capability('moodle/course:viewsuspendedusers', CAP_PROHIBIT, $roleteacher->id, $coursecontext, true); + set_user_preference('grade_report_showonlyactiveenrol', false, $teacher); + accesslib_clear_all_caches_for_unit_testing(); + $users = $screentest->test_load_users(); + $this->assertCount(1, $users); + + // Now, activate the user enrolment again. We shall get 2 users now. + $this->getDataGenerator()->enrol_user($user2->id, $course->id, $roleteacher->id, 'manual', 0, 0, ENROL_USER_ACTIVE); + $users = $screentest->test_load_users(); + $this->assertCount(2, $users); + } +} \ No newline at end of file -- 1.9.1