From a487a3edf2f20d75dfeb26f3c8516ed72f5f25d0 Mon Sep 17 00:00:00 2001 From: David Knuplesch Date: Sat, 22 Sep 2018 22:54:20 +0200 Subject: [PATCH 1/4] MDL-60710 course: setting to show/hide duplicate coursecontact --- admin/settings/appearance.php | 3 + course/classes/list_element.php | 57 ++++--- course/externallib.php | 9 +- course/renderer.php | 9 +- course/tests/behat/behat_course.php | 78 +++++++++ course/tests/behat/course_contact.feature | 198 ++++++++++++++++++++++ course/tests/category_test.php | 164 +++++++++++++++++- course/upgrade.txt | 6 + lang/en/admin.php | 2 + 9 files changed, 498 insertions(+), 28 deletions(-) create mode 100644 course/tests/behat/course_contact.feature diff --git a/admin/settings/appearance.php b/admin/settings/appearance.php index b971e30c0e4..57a230e2fd1 100644 --- a/admin/settings/appearance.php +++ b/admin/settings/appearance.php @@ -218,6 +218,9 @@ preferences,moodle|/user/preferences.php|t/preferences', // coursecontact is the person responsible for course - usually manages enrolments, receives notification, etc. $temp = new admin_settingpage('coursecontact', new lang_string('courses')); $temp->add(new admin_setting_special_coursecontact()); + $temp->add(new admin_setting_configcheckbox('coursecontactduplicates', + new lang_string('coursecontactduplicates', 'admin'), + new lang_string('coursecontactduplicates_desc', 'admin'), 0)); $temp->add(new admin_setting_configcheckbox('courselistshortnames', new lang_string('courselistshortnames', 'admin'), new lang_string('courselistshortnames_desc', 'admin'), 0)); diff --git a/course/classes/list_element.php b/course/classes/list_element.php index cb9defc0be4..b9d30104c9b 100644 --- a/course/classes/list_element.php +++ b/course/classes/list_element.php @@ -151,37 +151,50 @@ class core_course_list_element implements IteratorAggregate { // No roles are configured to be displayed as course contacts. return array(); } + + if (!$this->has_course_contacts()) { + // No course contacts exist. + return array(); + } + if ($this->coursecontacts === null) { $this->coursecontacts = array(); + $context = context_course::instance($this->id); - if (!isset($this->record->managers)) { - // Preload course contacts from DB. - $courses = array($this->id => &$this->record); - core_course_category::preload_course_contacts($courses); - } - - // Build return array with full roles names (for this course context) and users names. $canviewfullnames = has_capability('moodle/site:viewfullnames', $context); + + $displayall = get_config('core', 'coursecontactduplicates'); + foreach ($this->record->managers as $ruser) { - if (isset($this->coursecontacts[$ruser->id])) { - // Only display a user once with the highest sortorder role. + $processed = array_key_exists($ruser->id, $this->coursecontacts); + if (!$displayall && $processed) { continue; } - $user = new stdClass(); - $user = username_load_fields_from_object($user, $ruser, null, array('id', 'username')); - $role = new stdClass(); - $role->id = $ruser->roleid; - $role->name = $ruser->rolename; - $role->shortname = $ruser->roleshortname; - $role->coursealias = $ruser->rolecoursealias; - $this->coursecontacts[$user->id] = array( - 'user' => $user, - 'role' => $role, - 'rolename' => role_get_name($role, $context, ROLENAME_ALIAS), - 'username' => fullname($user, $canviewfullnames) - ); + $role = (object)[ + 'id' => $ruser->roleid, + 'name' => $ruser->rolename, + 'shortname' => $ruser->roleshortname, + 'coursealias' => $ruser->rolecoursealias, + ]; + $role->displayname = role_get_name($role, $context, ROLENAME_ALIAS); + + if (!$processed) { + $user = username_load_fields_from_object((object)[], $ruser, null, ['id', 'username']); + $this->coursecontacts[$ruser->id] = [ + 'user' => $user, + 'username' => fullname($user, $canviewfullnames), + + // List of all roles. + 'roles' => [], + + // Primary role of this user. + 'role' => $role, + 'rolename' => $role->displayname, + ]; + } + $this->coursecontacts[$ruser->id]['roles'][$ruser->roleid] = $role; } } return $this->coursecontacts; diff --git a/course/externallib.php b/course/externallib.php index b7cabe7799f..e57011977b1 100644 --- a/course/externallib.php +++ b/course/externallib.php @@ -2263,8 +2263,13 @@ class core_course_external extends external_api { foreach ($course->get_course_contacts() as $contact) { $coursecontacts[] = array( 'id' => $contact['user']->id, - 'fullname' => $contact['username'] - ); + 'fullname' => $contact['username'], + 'roles' => array_map(function($role){ + return array('id' => $role->id, 'name' => $role->displayname); + }, $contact['role']), + 'role' => array('id' => $contact['role']->id, 'name' => $contact['role']->displayname), + 'rolename' => $contact['rolename'] + ); } // Allowed enrolment methods (maybe we can self-enrol). diff --git a/course/renderer.php b/course/renderer.php index 91a871f5f83..974414b0c6d 100644 --- a/course/renderer.php +++ b/course/renderer.php @@ -1197,10 +1197,13 @@ class core_course_renderer extends plugin_renderer_base { // Display course contacts. See core_course_list_element::get_course_contacts(). if ($course->has_course_contacts()) { $content .= html_writer::start_tag('ul', array('class' => 'teachers')); - foreach ($course->get_course_contacts() as $userid => $coursecontact) { - $name = $coursecontact['rolename'].': '. + foreach ($course->get_course_contacts() as $coursecontact) { + $rolenames = array_map(function ($role) { + return $role->displayname; + }, $coursecontact['roles']); + $name = implode(", ", $rolenames).': '. html_writer::link(new moodle_url('/user/view.php', - array('id' => $userid, 'course' => SITEID)), + array('id' => $coursecontact['user']->id, 'course' => SITEID)), $coursecontact['username']); $content .= html_writer::tag('li', $name); } diff --git a/course/tests/behat/behat_course.php b/course/tests/behat/behat_course.php index 44b93425b5b..557d94e124a 100644 --- a/course/tests/behat/behat_course.php +++ b/course/tests/behat/behat_course.php @@ -1879,4 +1879,82 @@ class behat_course extends behat_base { $xpath = "//div[contains(@class,'block')]//li[p/*[string(.)=$coursestr or string(.)=$mycoursestr]]"; $this->execute('behat_general::i_click_on_in_the', [get_string('participants'), 'link', $xpath, 'xpath_element']); } + + /** + * Check that one teacher appears before another in the course contacts. + * + * @Given /^I should see teacher "(?P(?:[^"]|\\")*)" before "(?P(?:[^"]|\\")*)" in the course contact listing$/ + * + * @param string $pteacher The first teacher to find + * @param string $fteacher The second teacher to find (should be after the first teacher) + * + * @throws ExpectationException + */ + public function i_should_see_teacher_before($pteacher, $fteacher) { + $xpath = "//ul[contains(@class,'teachers')]//li//a[text()='{$pteacher}']/ancestor::li//following::a[text()='{$fteacher}']"; + $msg = "Teacher {$pteacher} does not appear before Teacher {$fteacher}"; + if (!$this->getSession()->getDriver()->find($xpath)) { + throw new ExpectationException($msg, $this->getSession()); + } + } + + /** + * Check that one teacher oes not appears after another in the course contacts. + * + * @Given /^I should not see teacher "(?P(?:[^"]|\\")*)" after "(?P(?:[^"]|\\")*)" in the course contact listing$/ + * + * @param string $fteacher The teacher that should not be found (after the other teacher) + * @param string $pteacher The teacher after who the other should not be found (this teacher must be found!) + * + * @throws ExpectationException + */ + public function i_should_not_see_teacher_after($fteacher, $pteacher) { + $xpathliteral = behat_context_helper::escape($pteacher); + $xpath = "/descendant-or-self::*[contains(., $xpathliteral)]" . + "[count(descendant::*[contains(., $xpathliteral)]) = 0]"; + try { + $nodes = $this->find_all('xpath', $xpath); + } catch (ElementNotFoundException $e) { + throw new ExpectationException('"' . $pteacher . '" text was not found in the page', $this->getSession()); + } + $xpath = "//ul[contains(@class,'teachers')]//li//a[text()='{$pteacher}']/ancestor::li//following::a[text()='{$fteacher}']"; + $msg = "Teacher {$fteacher} appears after Teacher {$pteacher}"; + if ($this->getSession()->getDriver()->find($xpath)) { + throw new ExpectationException($msg, $this->getSession()); + } + } + + /** + * Moves up the specified role. + * + * @Given /^I move up role "(?P(?:[^"]|\\")*)" in the global role sortorder$/ + * @param String $role + */ + public function i_move_up_role($role) { + global $DB; + $roledb = $DB->get_record('role', array('shortname' => $role), 'id, sortorder', MUST_EXIST); + $query = "SELECT id, sortorder FROM {role} WHERE sortorder < ".$roledb->sortorder." ORDER BY sortorder DESC"; + $previousroles = $DB->get_records_sql($query, null, 0, 1); + foreach ($previousroles as $id => $previousrole) { + switch_roles($previousrole, $roledb); + break; + } + } + + /** + * Moves down the specified role. + * + * @Given /^I move down role "(?P(?:[^"]|\\")*)" in the global role sortorder$/ + * @param String $role + */ + public function i_move_down_role($role) { + global $DB; + $roledb = $DB->get_record('role', array('shortname' => $role), 'id, sortorder', MUST_EXIST); + $query = "SELECT id, sortorder FROM {role} WHERE sortorder > " . $roledb->sortorder . " ORDER BY sortorder ASC"; + $previousroles = $DB->get_records_sql($query, null, 0, 1); + foreach ($previousroles as $id => $previousrole) { + switch_roles($previousrole, $roledb); + break; + } + } } diff --git a/course/tests/behat/course_contact.feature b/course/tests/behat/course_contact.feature new file mode 100644 index 00000000000..e8ed7d0e607 --- /dev/null +++ b/course/tests/behat/course_contact.feature @@ -0,0 +1,198 @@ +@core @core_course +Feature: Test we can see coursecontacts. + As a student I need to see coursecontacts + As a admin I need to test we can resort coursecontacts. + As a admin I need to test we can show duplicate course contacts + Scenario: Test coursecontacts functionality + Given the following "categories" exist: + | name | category | idnumber | + | Cat 1 | 0 | CAT1 | + And the following "courses" exist: + | fullname | shortname | category | format | + | Course 1 | C1 | CAT1 | topics | + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher 1 | T | teacher1@example.com | + | teacher2 | Teacher 2 | T | teacher2@example.com | + | teacher3 | Teacher 3 | T | teacher3@example.com | + | manager1 | Manager 1 | M | manager1@example.com | + | student1 | Student 1 | S | student1@example.com | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + | teacher1 | C1 | teacher | + | teacher2 | C1 | teacher | + | teacher3 | C1 | editingteacher | + | manager1 | C1 | manager | + And I log in as "admin" + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should not see "Teacher 2" in the "#region-main" "css_element" + And I log out + And I log in as "manager1" + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should not see "Teacher 2" in the "#region-main" "css_element" + And I log out + And I log in as "teacher1" + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should not see "Teacher 2" in the "#region-main" "css_element" + And I log out + And I log in as "student1" + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should not see "Teacher 2" in the "#region-main" "css_element" + And I should see teacher "Teacher 1 T" before "Teacher 3 T" in the course contact listing + And I should not see teacher "Teacher 1 T" after "Teacher 3 T" in the course contact listing + And I log out + Scenario: Test selection and duplicates of coursecontact roles + Given the following "categories" exist: + | name | category | idnumber | + | Cat 1 | 0 | CAT1 | + And the following "courses" exist: + | fullname | shortname | category | format | + | Course 1 | C1 | CAT1 | topics | + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher 1 | T | teacher1@example.com | + | teacher2 | Teacher 2 | T | teacher2@example.com | + | teacher3 | Teacher 3 | T | teacher3@example.com | + | manager1 | Manager 1 | M | manager1@example.com | + | student1 | Student 1 | S | student1@example.com | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + | teacher1 | C1 | teacher | + | teacher2 | C1 | teacher | + | teacher3 | C1 | editingteacher | + | manager1 | C1 | manager | + And I log in as "admin" + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should not see "Teacher 2" in the "#region-main" "css_element" + And I should not see "Manager 1" in the "#region-main" "css_element" + And I am on site homepage + And I navigate to "Appearance > Courses" in site administration + And I set the following fields to these values: + | Manager | 1 | + | Non-editing teacher | 1 | + | Show duplicate course contacts | 1 | + And I press "Save changes" + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should see "Teacher 2" in the "#region-main" "css_element" + And I should see "Teacher 3" in the "#region-main" "css_element" + And I should see "Manager 1" in the "#region-main" "css_element" + And I should see teacher "Manager 1 M" before "Teacher 1 T" in the course contact listing + And I should see teacher "Teacher 1 T" before "Teacher 3 T" in the course contact listing + And I should see teacher "Teacher 3 T" before "Teacher 2 T" in the course contact listing + And I am on site homepage + And I navigate to "Appearance > Courses" in site administration + And I set the following fields to these values: + | Manager | 0 | + And I press "Save changes" + And I log out + And I log in as "admin" + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should not see "Manager 1" in the "#region-main" "css_element" + Scenario: Test selection of coursecontact roles + Given the following "categories" exist: + | name | category | idnumber | + | Cat 1 | 0 | CAT1 | + And the following "courses" exist: + | fullname | shortname | category | format | + | Course 1 | C1 | CAT1 | topics | + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher 1 | T | teacher1@example.com | + | teacher2 | Teacher 2 | T | teacher2@example.com | + | teacher3 | Teacher 3 | T | teacher3@example.com | + | manager1 | Manager 1 | M | manager1@example.com | + | manager2 | Manager 2 | M | manager1@example.com | + | student1 | Student 1 | S | student1@example.com | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + | teacher1 | C1 | teacher | + | teacher2 | C1 | teacher | + | teacher3 | C1 | editingteacher | + | manager1 | C1 | manager | + And I log in as "admin" + And I navigate to "Appearance > Courses" in site administration + And I set the following fields to these values: + | Manager | Yes | + | Non-editing teacher | Yes | + | Show duplicate course contacts | Yes | + And I press "Save changes" + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should see "Teacher 2" in the "#region-main" "css_element" + And I should see "Teacher 3" in the "#region-main" "css_element" + And I should see "Manager 1" in the "#region-main" "css_element" + And I should see teacher "Manager 1 M" before "Teacher 1 T" in the course contact listing + And I should see teacher "Teacher 1 T" before "Teacher 3 T" in the course contact listing + And I should see teacher "Teacher 3 T" before "Teacher 2 T" in the course contact listing + And I navigate to "Appearance > Courses" in site administration + And I move up role "teacher" in the global role sortorder + And I move up role "teacher" in the global role sortorder + And I move up role "teacher" in the global role sortorder + And I move up role "teacher" in the global role sortorder + And I move up role "teacher" in the global role sortorder + And I move up role "teacher" in the global role sortorder + And I move up role "teacher" in the global role sortorder + And I move up role "teacher" in the global role sortorder + And I press "Save changes" + And I am on site homepage + And I follow "Course 1" + And I log out + And I log in as "admin" + And I am on course index + And I follow "Purge all caches" + And I wait until the page is ready + And I am on course index + And I should see "Cat 1" in the "#region-main" "css_element" + And I follow "Cat 1" + And I wait until the page is ready + And I should see "Course 1" in the "#region-main" "css_element" + And I should see "Teacher 1" in the "#region-main" "css_element" + And I should see "Teacher 2" in the "#region-main" "css_element" + And I should see "Teacher 3" in the "#region-main" "css_element" + And I should not see teacher "Teacher 2 T" after "Manager 1 M" in the course contact listing + And I should not see teacher "Teacher 2 T" after "Teacher 3 T" in the course contact listing + And I should see teacher "Teacher 1 T" before "Manager 1 M" in the course contact listing + And I should not see teacher "Teacher 2 T" after "Manager 1 M" in the course contact listing + And I should not see teacher "Teacher 2 T" after "Teacher 3 T" in the course contact listing diff --git a/course/tests/category_test.php b/course/tests/category_test.php index 239744d94b9..1dcd1efc44a 100644 --- a/course/tests/category_test.php +++ b/course/tests/category_test.php @@ -570,6 +570,9 @@ class core_course_category_testcase extends advanced_testcase { public function test_course_contacts() { global $DB, $CFG; + + set_config('coursecontactduplicates', false); + $teacherrole = $DB->get_record('role', array('shortname'=>'editingteacher')); $managerrole = $DB->get_record('role', array('shortname'=>'manager')); $studentrole = $DB->get_record('role', array('shortname'=>'student')); @@ -681,8 +684,167 @@ class core_course_category_testcase extends advanced_testcase { // Suspend user 4 and make sure he is no longer in contacts of course 1 in category 4. $manual->enrol_user($enrol[4][1], $user[4], $teacherrole->id, 0, 0, ENROL_USER_SUSPENDED); + $allcourses = core_course_category::get(0)->get_courses(array( + 'recursive' => true, + 'coursecontacts' => true, + 'sort' => array('idnumber' => 1)) + ); + $contacts = $allcourses[$course[4][1]]->get_course_contacts(); + $this->assertCount(1, $contacts); + $contact = reset($contacts); + $this->assertEquals('F5 L5', $contact['username']); + + $CFG->coursecontact = $oldcoursecontact; + } + + public function test_course_contacts_with_duplicates() { + global $DB, $CFG; + + set_config('coursecontactduplicates', true); + + $displayall = get_config('core', 'coursecontactduplicates'); + $this->assertEquals(true, $displayall); + + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + $managerrole = $DB->get_record('role', array('shortname' => 'manager')); + $studentrole = $DB->get_record('role', array('shortname' => 'student')); + $oldcoursecontact = $CFG->coursecontact; + + $CFG->coursecontact = $managerrole->id. ','. $teacherrole->id; + + /* + * User is listed in course contacts for the course if he has one of the + * "course contact" roles ($CFG->coursecontact) AND is enrolled in the course. + * If the user has several roles all roles are displayed, but each role only once per user. + */ + + /* + * Test case: + * + * == Cat1 (user2 has teacher role) + * == Cat2 + * -- course21 (user2 is enrolled as manager) | [Expected] Manager: F2 L2 + * -- course22 (user2 is enrolled as student) | [Expected] Teacher: F2 L2 + * == Cat4 (user2 has manager role) + * -- course41 (user4 is enrolled as teacher, user5 is enrolled as manager) + * | [Expected] Manager: F5 L5, Teacher: F4 L4 + * -- course42 (user2 is enrolled as teacher) | [Expected] Manager: F2 L2 + * == Cat3 (user3 has manager role) + * -- course31 (user3 is enrolled as student) | [Expected] Manager: F3 L3 + * -- course32 | [Expected] + * -- course11 (user1 is enrolled as teacher) | [Expected] Teacher: F1 L1 + * -- course12 (user1 has teacher role) | [Expected] + * also user4 is enrolled as teacher but enrolment is not active + */ + $category = $course = $enrol = $user = array(); + $category[1] = core_course_category::create(array('name' => 'Cat1'))->id; + $category[2] = core_course_category::create(array('name' => 'Cat2', 'parent' => $category[1]))->id; + $category[3] = core_course_category::create(array('name' => 'Cat3', 'parent' => $category[1]))->id; + $category[4] = core_course_category::create(array('name' => 'Cat4', 'parent' => $category[2]))->id; + foreach (array(1, 2, 3, 4) as $catid) { + foreach (array(1, 2) as $courseid) { + $course[$catid][$courseid] = $this->getDataGenerator()->create_course(array( + 'idnumber' => 'id'.$catid.$courseid, + 'category' => $category[$catid]) + )->id; + $enrol[$catid][$courseid] = $DB->get_record( + 'enrol', + array('courseid' => $course[$catid][$courseid], 'enrol' => 'manual'), + '*', + MUST_EXIST + ); + } + } + foreach (array(1, 2, 3, 4, 5) as $userid) { + $user[$userid] = $this->getDataGenerator()->create_user(array( + 'firstname' => 'F'.$userid, + 'lastname' => 'L'.$userid) + )->id; + } + + $manual = enrol_get_plugin('manual'); + + // Nobody is enrolled now and course contacts are empty. + $allcourses = core_course_category::get(0)->get_courses(array( + 'recursive' => true, + 'coursecontacts' => true, + 'sort' => array('idnumber' => 1)) + ); + foreach ($allcourses as $onecourse) { + $this->assertEmpty($onecourse->get_course_contacts()); + } + + // Cat1: user2 has teacher role. + role_assign($teacherrole->id, $user[2], context_coursecat::instance($category[1])); + // Course21: user2 is enrolled as manager. + $manual->enrol_user($enrol[2][1], $user[2], $managerrole->id); + // Course22: user2 is enrolled as student. + $manual->enrol_user($enrol[2][2], $user[2], $studentrole->id); + // Cat4: user2 has manager role. + role_assign($managerrole->id, $user[2], context_coursecat::instance($category[4])); + // Course41: user4 is enrolled as teacher, user5 is enrolled as manager. + $manual->enrol_user($enrol[4][1], $user[4], $teacherrole->id); + $manual->enrol_user($enrol[4][1], $user[5], $managerrole->id); + // Course42: user2 is enrolled as teacher. + $manual->enrol_user($enrol[4][2], $user[2], $teacherrole->id); + // Cat3: user3 has manager role. + role_assign($managerrole->id, $user[3], context_coursecat::instance($category[3])); + // Course31: user3 is enrolled as student. + $manual->enrol_user($enrol[3][1], $user[3], $studentrole->id); + // Course11: user1 is enrolled as teacher and user4 is enrolled as teacher and has manager role. + $manual->enrol_user($enrol[1][1], $user[1], $teacherrole->id); + $manual->enrol_user($enrol[1][1], $user[4], $teacherrole->id); + role_assign($managerrole->id, $user[4], context_course::instance($course[1][1])); + // Course12: user1 has teacher role, but is not enrolled, as well as user4 is enrolled as teacher, but user4's enrolment is + // not active. + role_assign($teacherrole->id, $user[1], context_course::instance($course[1][2])); + $manual->enrol_user($enrol[1][2], $user[4], $teacherrole->id, 0, 0, ENROL_USER_SUSPENDED); + $allcourses = core_course_category::get(0)->get_courses( - array('recursive' => true, 'coursecontacts' => true, 'sort' => array('idnumber' => 1))); + array('recursive' => true, 'coursecontacts' => true, 'sort' => array('idnumber' => 1))); + // Simplify the list of contacts for each course (similar as renderer would do). + $contacts = array(); + foreach (array(1, 2, 3, 4) as $catid) { + foreach (array(1, 2) as $courseid) { + $tmp = array(); + foreach ($allcourses[$course[$catid][$courseid]]->get_course_contacts() as $contact) { + $rolenames = array_map(function ($role) { + return $role->displayname; + }, $contact['roles']); + $tmp[] = implode(", ", $rolenames). ': '. + $contact['username']; + } + $contacts[$catid][$courseid] = join(', ', $tmp); + } + } + + // Assert: + // Course21: user2 is enrolled as manager. [Expected] Manager: F2 L2, Teacher: F2 L2. + $this->assertSame('Manager, Teacher: F2 L2', $contacts[2][1]); + // Course22: user2 is enrolled as student. [Expected] Teacher: F2 L2. + $this->assertSame('Teacher: F2 L2', $contacts[2][2]); + // Course41: user4 is enrolled as teacher, user5 is enrolled as manager. [Expected] Manager: F5 L5, Teacher: F4 L4. + $this->assertSame('Manager: F5 L5, Teacher: F4 L4', $contacts[4][1]); + // Course42: user2 is enrolled as teacher. [Expected] Manager: F2 L2, Teacher: F2 L2. + $this->assertSame('Manager, Teacher: F2 L2', $contacts[4][2]); + // Course31: user3 is enrolled as student. [Expected] Manager: F3 L3. + $this->assertSame('Manager: F3 L3', $contacts[3][1]); + // Course32: nobody is enrolled. [Expected] (nothing). + $this->assertSame('', $contacts[3][2]); + // Course11: user1 is enrolled as teacher and user4 is enrolled as teacher and has manager role. [Expected] Manager: F4 L4, + // Teacher: F1 L1, Teacher: F4 L4. + $this->assertSame('Manager, Teacher: F4 L4, Teacher: F1 L1', $contacts[1][1]); + // Course12: user1 has teacher role, but is not enrolled, as well as user4 is enrolled as teacher, but user4's enrolment is + // not active. [Expected] (nothing). + $this->assertSame('', $contacts[1][2]); + + // Suspend user 4 and make sure he is no longer in contacts of course 1 in category 4. + $manual->enrol_user($enrol[4][1], $user[4], $teacherrole->id, 0, 0, ENROL_USER_SUSPENDED); + $allcourses = core_course_category::get(0)->get_courses(array( + 'recursive' => true, + 'coursecontacts' => true, + 'sort' => array('idnumber' => 1) + )); $contacts = $allcourses[$course[4][1]]->get_course_contacts(); $this->assertCount(1, $contacts); $contact = reset($contacts); diff --git a/course/upgrade.txt b/course/upgrade.txt index 62a9d36808e..d95217078aa 100644 --- a/course/upgrade.txt +++ b/course/upgrade.txt @@ -1,7 +1,13 @@ This files describes API changes in /course/*, information provided here is intended especially for developers. +=== 3.6 === + + * External function core_course_external::get_course_public_information now returns the roles and the primary role of course + contacts. + === 3.5 === + * There is a new capability 'moodle/course:setforcedlanguage' to control which users can force the course language; create_course and update_course functions delegate access control to the caller code; if you are calling those functions you may be interested in checking if the logged in user has 'moodle/course:setforcedlanguage' capability. diff --git a/lang/en/admin.php b/lang/en/admin.php index 1c93d92f1a8..d94b37eda09 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -382,6 +382,8 @@ $string['cookiesecure'] = 'Secure cookies only'; $string['country'] = 'Default country'; $string['coursecontact'] = 'Course contacts'; $string['coursecontact_desc'] = 'This setting allows you to control who appears on the course description. Users need to have at least one of these roles in a course to be shown on the course description for that course.'; +$string['coursecontactduplicates'] = 'Show duplicate course contacts'; +$string['coursecontactduplicates_desc'] = 'If enabled, a users will be displayed with each course contact role to which she/he is assigned to in the course contact list. If disabled, users that are course contacts will be displayed only once, even if they are assigned to multiple course contact roles (i.e. roles that are selected in the list above).'; $string['coursegraceperiodafter'] = 'Grace period for past courses'; $string['coursegraceperiodbefore'] = 'Grace period for future courses'; $string['courselistshortnames'] = 'Display extended course names'; From 63044a239179d9581e15fa56adac202b2e223d33 Mon Sep 17 00:00:00 2001 From: Alexander Bias Date: Thu, 4 Oct 2018 08:32:30 +0200 Subject: [PATCH 2/4] MDL-60710 course: Refine setting name and description --- lang/en/admin.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lang/en/admin.php b/lang/en/admin.php index d94b37eda09..e0762fca7e1 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -382,8 +382,8 @@ $string['cookiesecure'] = 'Secure cookies only'; $string['country'] = 'Default country'; $string['coursecontact'] = 'Course contacts'; $string['coursecontact_desc'] = 'This setting allows you to control who appears on the course description. Users need to have at least one of these roles in a course to be shown on the course description for that course.'; -$string['coursecontactduplicates'] = 'Show duplicate course contacts'; -$string['coursecontactduplicates_desc'] = 'If enabled, a users will be displayed with each course contact role to which she/he is assigned to in the course contact list. If disabled, users that are course contacts will be displayed only once, even if they are assigned to multiple course contact roles (i.e. roles that are selected in the list above).'; +$string['coursecontactduplicates'] = 'Display all course contact roles'; +$string['coursecontactduplicates_desc'] = 'If enabled, users with more than one of the selected course contact roles will be shown with each of their roles in the course description. Otherwise, only the role which is listed highest in \'Define roles\' in the Site administration will be shown.'; $string['coursegraceperiodafter'] = 'Grace period for past courses'; $string['coursegraceperiodbefore'] = 'Grace period for future courses'; $string['courselistshortnames'] = 'Display extended course names'; From 3ebba90a6b1d1c8fa7ac0004650291f732b8fe50 Mon Sep 17 00:00:00 2001 From: Alexander Bias Date: Mon, 8 Oct 2018 22:49:13 +0200 Subject: [PATCH 3/4] MDL-60710 course: Refine strings --- lang/en/admin.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lang/en/admin.php b/lang/en/admin.php index e0762fca7e1..6824b16aa46 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -383,7 +383,7 @@ $string['country'] = 'Default country'; $string['coursecontact'] = 'Course contacts'; $string['coursecontact_desc'] = 'This setting allows you to control who appears on the course description. Users need to have at least one of these roles in a course to be shown on the course description for that course.'; $string['coursecontactduplicates'] = 'Display all course contact roles'; -$string['coursecontactduplicates_desc'] = 'If enabled, users with more than one of the selected course contact roles will be shown with each of their roles in the course description. Otherwise, only the role which is listed highest in \'Define roles\' in the Site administration will be shown.'; +$string['coursecontactduplicates_desc'] = 'If enabled, users with more than one of the selected course contact roles will be displayed in the course description with each of their roles. Otherwise, they will be displayed with only one role (whichever is listed highest in \'Define roles\' in the Site administration).'; $string['coursegraceperiodafter'] = 'Grace period for past courses'; $string['coursegraceperiodbefore'] = 'Grace period for future courses'; $string['courselistshortnames'] = 'Display extended course names'; From da4d17313d087993ca65ed662d3274c54db7065b Mon Sep 17 00:00:00 2001 From: Alexander Bias Date: Mon, 8 Oct 2018 22:49:36 +0200 Subject: [PATCH 4/4] MDL-60710 course: Overhaul of Behat tests --- course/tests/behat/behat_course.php | 34 ---- course/tests/behat/course_contact.feature | 233 +++++++++------------- 2 files changed, 93 insertions(+), 174 deletions(-) diff --git a/course/tests/behat/behat_course.php b/course/tests/behat/behat_course.php index 557d94e124a..93b88603692 100644 --- a/course/tests/behat/behat_course.php +++ b/course/tests/behat/behat_course.php @@ -1923,38 +1923,4 @@ class behat_course extends behat_base { throw new ExpectationException($msg, $this->getSession()); } } - - /** - * Moves up the specified role. - * - * @Given /^I move up role "(?P(?:[^"]|\\")*)" in the global role sortorder$/ - * @param String $role - */ - public function i_move_up_role($role) { - global $DB; - $roledb = $DB->get_record('role', array('shortname' => $role), 'id, sortorder', MUST_EXIST); - $query = "SELECT id, sortorder FROM {role} WHERE sortorder < ".$roledb->sortorder." ORDER BY sortorder DESC"; - $previousroles = $DB->get_records_sql($query, null, 0, 1); - foreach ($previousroles as $id => $previousrole) { - switch_roles($previousrole, $roledb); - break; - } - } - - /** - * Moves down the specified role. - * - * @Given /^I move down role "(?P(?:[^"]|\\")*)" in the global role sortorder$/ - * @param String $role - */ - public function i_move_down_role($role) { - global $DB; - $roledb = $DB->get_record('role', array('shortname' => $role), 'id, sortorder', MUST_EXIST); - $query = "SELECT id, sortorder FROM {role} WHERE sortorder > " . $roledb->sortorder . " ORDER BY sortorder ASC"; - $previousroles = $DB->get_records_sql($query, null, 0, 1); - foreach ($previousroles as $id => $previousrole) { - switch_roles($previousrole, $roledb); - break; - } - } } diff --git a/course/tests/behat/course_contact.feature b/course/tests/behat/course_contact.feature index e8ed7d0e607..78e2e01d48c 100644 --- a/course/tests/behat/course_contact.feature +++ b/course/tests/behat/course_contact.feature @@ -1,22 +1,22 @@ @core @core_course -Feature: Test we can see coursecontacts. - As a student I need to see coursecontacts - As a admin I need to test we can resort coursecontacts. - As a admin I need to test we can show duplicate course contacts - Scenario: Test coursecontacts functionality +Feature: Test if displaying the course contacts works correctly: + As a user I need to see the course contacts of a course. + As an admin I need to be able to control the appearance of the course contacts. + + Background: Given the following "categories" exist: | name | category | idnumber | | Cat 1 | 0 | CAT1 | And the following "courses" exist: | fullname | shortname | category | format | | Course 1 | C1 | CAT1 | topics | - Given the following "users" exist: + And the following "users" exist: | username | firstname | lastname | email | - | teacher1 | Teacher 1 | T | teacher1@example.com | - | teacher2 | Teacher 2 | T | teacher2@example.com | - | teacher3 | Teacher 3 | T | teacher3@example.com | - | manager1 | Manager 1 | M | manager1@example.com | - | student1 | Student 1 | S | student1@example.com | + | teacher1 | Teacher | 1 | teacher1@example.com | + | teacher2 | Teacher | 2 | teacher2@example.com | + | teacher3 | Teacher | 3 | teacher3@example.com | + | manager1 | Manager | 1 | manager1@example.com | + | student1 | Student | 1 | student1@example.com | And the following "course enrolments" exist: | user | course | role | | teacher1 | C1 | editingteacher | @@ -24,175 +24,128 @@ Feature: Test we can see coursecontacts. | teacher2 | C1 | teacher | | teacher3 | C1 | editingteacher | | manager1 | C1 | manager | - And I log in as "admin" - And I am on course index + + Scenario: Test general course contacts functionality for all user roles + Given I log in as "admin" + And I navigate to "Appearance > Courses" in site administration + And I set the following fields to these values: + | Manager | 0 | + | Teacher | 1 | + | Non-editing teacher | 0 | + | Display all course contact roles | 0 | + And I press "Save changes" + + When I am on course index And I should see "Cat 1" in the "#region-main" "css_element" And I follow "Cat 1" And I wait until the page is ready And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should not see "Teacher 2" in the "#region-main" "css_element" - And I log out + Then I should see "Teacher 1" in the ".teachers" "css_element" + And I should not see "Teacher 2" in the ".teachers" "css_element" + And I should not see "Manager 1" in the ".teachers" "css_element" + + When I log out And I log in as "manager1" And I am on course index And I should see "Cat 1" in the "#region-main" "css_element" And I follow "Cat 1" And I wait until the page is ready And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should not see "Teacher 2" in the "#region-main" "css_element" - And I log out + Then I should see "Teacher 1" in the ".teachers" "css_element" + And I should not see "Teacher 2" in the ".teachers" "css_element" + And I should not see "Manager 1" in the ".teachers" "css_element" + + When I log out And I log in as "teacher1" And I am on course index And I should see "Cat 1" in the "#region-main" "css_element" And I follow "Cat 1" And I wait until the page is ready And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should not see "Teacher 2" in the "#region-main" "css_element" - And I log out + Then I should see "Teacher 1" in the ".teachers" "css_element" + And I should not see "Teacher 2" in the ".teachers" "css_element" + And I should not see "Manager 1" in the ".teachers" "css_element" + + When I log out And I log in as "student1" And I am on course index And I should see "Cat 1" in the "#region-main" "css_element" And I follow "Cat 1" And I wait until the page is ready And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should not see "Teacher 2" in the "#region-main" "css_element" - And I should see teacher "Teacher 1 T" before "Teacher 3 T" in the course contact listing - And I should not see teacher "Teacher 1 T" after "Teacher 3 T" in the course contact listing - And I log out - Scenario: Test selection and duplicates of coursecontact roles - Given the following "categories" exist: - | name | category | idnumber | - | Cat 1 | 0 | CAT1 | - And the following "courses" exist: - | fullname | shortname | category | format | - | Course 1 | C1 | CAT1 | topics | - Given the following "users" exist: - | username | firstname | lastname | email | - | teacher1 | Teacher 1 | T | teacher1@example.com | - | teacher2 | Teacher 2 | T | teacher2@example.com | - | teacher3 | Teacher 3 | T | teacher3@example.com | - | manager1 | Manager 1 | M | manager1@example.com | - | student1 | Student 1 | S | student1@example.com | - And the following "course enrolments" exist: - | user | course | role | - | teacher1 | C1 | editingteacher | - | teacher1 | C1 | teacher | - | teacher2 | C1 | teacher | - | teacher3 | C1 | editingteacher | - | manager1 | C1 | manager | - And I log in as "admin" - And I am on course index - And I should see "Cat 1" in the "#region-main" "css_element" - And I follow "Cat 1" - And I wait until the page is ready - And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should not see "Teacher 2" in the "#region-main" "css_element" - And I should not see "Manager 1" in the "#region-main" "css_element" - And I am on site homepage - And I navigate to "Appearance > Courses" in site administration - And I set the following fields to these values: - | Manager | 1 | - | Non-editing teacher | 1 | - | Show duplicate course contacts | 1 | - And I press "Save changes" - And I am on course index - And I should see "Cat 1" in the "#region-main" "css_element" - And I follow "Cat 1" - And I wait until the page is ready - And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should see "Teacher 2" in the "#region-main" "css_element" - And I should see "Teacher 3" in the "#region-main" "css_element" - And I should see "Manager 1" in the "#region-main" "css_element" - And I should see teacher "Manager 1 M" before "Teacher 1 T" in the course contact listing - And I should see teacher "Teacher 1 T" before "Teacher 3 T" in the course contact listing - And I should see teacher "Teacher 3 T" before "Teacher 2 T" in the course contact listing - And I am on site homepage + Then I should see "Teacher 1" in the ".teachers" "css_element" + And I should not see "Teacher 2" in the ".teachers" "css_element" + And I should not see "Manager 1" in the ".teachers" "css_element" + + Scenario: Test course contact roles without displaying all roles + Given I log in as "admin" And I navigate to "Appearance > Courses" in site administration And I set the following fields to these values: | Manager | 0 | + | Teacher | 1 | + | Non-editing teacher | 1 | + | Display all course contact roles | 0 | And I press "Save changes" - And I log out - And I log in as "admin" - And I am on course index + When I am on course index And I should see "Cat 1" in the "#region-main" "css_element" And I follow "Cat 1" And I wait until the page is ready And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should not see "Manager 1" in the "#region-main" "css_element" - Scenario: Test selection of coursecontact roles - Given the following "categories" exist: - | name | category | idnumber | - | Cat 1 | 0 | CAT1 | - And the following "courses" exist: - | fullname | shortname | category | format | - | Course 1 | C1 | CAT1 | topics | - Given the following "users" exist: - | username | firstname | lastname | email | - | teacher1 | Teacher 1 | T | teacher1@example.com | - | teacher2 | Teacher 2 | T | teacher2@example.com | - | teacher3 | Teacher 3 | T | teacher3@example.com | - | manager1 | Manager 1 | M | manager1@example.com | - | manager2 | Manager 2 | M | manager1@example.com | - | student1 | Student 1 | S | student1@example.com | - And the following "course enrolments" exist: - | user | course | role | - | teacher1 | C1 | editingteacher | - | teacher1 | C1 | teacher | - | teacher2 | C1 | teacher | - | teacher3 | C1 | editingteacher | - | manager1 | C1 | manager | - And I log in as "admin" + Then I should see "Teacher 1" in the ".teachers" "css_element" + And I should see "Teacher 2" in the ".teachers" "css_element" + And I should see "Teacher 3" in the ".teachers" "css_element" + And I should see "Teacher: Teacher 1" in the ".teachers" "css_element" + And I should not see "Teacher, Non-editing teacher: Teacher 1" in the ".teachers" "css_element" + And I should not see "Manager 1" in the ".teachers" "css_element" + + Scenario: Test course contact roles with displaying all roles and standard sorting + Given I log in as "admin" And I navigate to "Appearance > Courses" in site administration And I set the following fields to these values: - | Manager | Yes | - | Non-editing teacher | Yes | - | Show duplicate course contacts | Yes | + | Manager | 0 | + | Teacher | 1 | + | Non-editing teacher | 1 | + | Display all course contact roles | 1 | And I press "Save changes" - And I am on course index + When I am on course index And I should see "Cat 1" in the "#region-main" "css_element" And I follow "Cat 1" And I wait until the page is ready And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should see "Teacher 2" in the "#region-main" "css_element" - And I should see "Teacher 3" in the "#region-main" "css_element" - And I should see "Manager 1" in the "#region-main" "css_element" - And I should see teacher "Manager 1 M" before "Teacher 1 T" in the course contact listing - And I should see teacher "Teacher 1 T" before "Teacher 3 T" in the course contact listing - And I should see teacher "Teacher 3 T" before "Teacher 2 T" in the course contact listing + Then I should see "Teacher 1" in the ".teachers" "css_element" + And I should see "Teacher 2" in the ".teachers" "css_element" + And I should see "Teacher 3" in the ".teachers" "css_element" + And I should see "Teacher, Non-editing teacher: Teacher 1" in the ".teachers" "css_element" + And I should not see "Teacher: Teacher 1" in the ".teachers" "css_element" + And I should not see "Manager 1" in the ".teachers" "css_element" + And I should see teacher "Teacher 1" before "Teacher 3" in the course contact listing + And I should see teacher "Teacher 3" before "Teacher 2" in the course contact listing + And I should not see teacher "Teacher 1" after "Teacher 3" in the course contact listing + And I should not see teacher "Teacher 3" after "Teacher 2" in the course contact listing + + Scenario: Test course contact roles with displaying all roles and modified sorting + Given I log in as "admin" And I navigate to "Appearance > Courses" in site administration - And I move up role "teacher" in the global role sortorder - And I move up role "teacher" in the global role sortorder - And I move up role "teacher" in the global role sortorder - And I move up role "teacher" in the global role sortorder - And I move up role "teacher" in the global role sortorder - And I move up role "teacher" in the global role sortorder - And I move up role "teacher" in the global role sortorder - And I move up role "teacher" in the global role sortorder + And I set the following fields to these values: + | Manager | 0 | + | Teacher | 1 | + | Non-editing teacher | 1 | + | Display all course contact roles | 1 | And I press "Save changes" - And I am on site homepage - And I follow "Course 1" - And I log out - And I log in as "admin" - And I am on course index - And I follow "Purge all caches" - And I wait until the page is ready - And I am on course index + And I navigate to "Users > Permissions > Define roles" in site administration + And I click on "Move up" "link" in the "//td[text()[contains(.,'Non-editing teacher')]]/parent::tr/td[contains(@class, 'lastcol')]" "xpath_element" + When I am on course index And I should see "Cat 1" in the "#region-main" "css_element" And I follow "Cat 1" And I wait until the page is ready And I should see "Course 1" in the "#region-main" "css_element" - And I should see "Teacher 1" in the "#region-main" "css_element" - And I should see "Teacher 2" in the "#region-main" "css_element" - And I should see "Teacher 3" in the "#region-main" "css_element" - And I should not see teacher "Teacher 2 T" after "Manager 1 M" in the course contact listing - And I should not see teacher "Teacher 2 T" after "Teacher 3 T" in the course contact listing - And I should see teacher "Teacher 1 T" before "Manager 1 M" in the course contact listing - And I should not see teacher "Teacher 2 T" after "Manager 1 M" in the course contact listing - And I should not see teacher "Teacher 2 T" after "Teacher 3 T" in the course contact listing + Then I should see "Teacher 1" in the ".teachers" "css_element" + And I should see "Teacher 2" in the ".teachers" "css_element" + And I should see "Teacher 3" in the ".teachers" "css_element" + And I should see "Non-editing teacher, Teacher: Teacher 1" in the ".teachers" "css_element" + And I should not see "Non-editing teacher: Teacher 1" in the ".teachers" "css_element" + And I should not see "Manager 1" in the ".teachers" "css_element" + And I should see teacher "Teacher 1" before "Teacher 2" in the course contact listing + And I should see teacher "Teacher 2" before "Teacher 3" in the course contact listing + And I should not see teacher "Teacher 1" after "Teacher 2" in the course contact listing + And I should not see teacher "Teacher 2" after "Teacher 3" in the course contact listing