From 5f7839f06bf041b2609c278b69ba11313338271f Mon Sep 17 00:00:00 2001 From: Ilya Tregubov Date: Tue, 20 Jun 2023 12:28:39 +0800 Subject: [PATCH 1/2] MDL-78522 cohort: Fix capability/visibility check. --- cohort/lib.php | 6 +++--- cohort/tests/lib_test.php | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/cohort/lib.php b/cohort/lib.php index 6d61d04eb20..48c7c87f645 100644 --- a/cohort/lib.php +++ b/cohort/lib.php @@ -384,13 +384,13 @@ function cohort_get_cohort($cohortorid, $currentcontext, $withcustomfields = fal if ($cohort && in_array($cohort->contextid, $currentcontext->get_parent_context_ids())) { if (!$cohort->visible) { - $cohort = false; - } else { $cohortcontext = context::instance_by_id($cohort->contextid); if (!has_capability('moodle/cohort:view', $cohortcontext)) { - $cohort = false; + return false; } } + } else { + return false; } if ($cohort && $withcustomfields) { diff --git a/cohort/tests/lib_test.php b/cohort/tests/lib_test.php index f1ba6dcd4dc..8bf48250d1c 100644 --- a/cohort/tests/lib_test.php +++ b/cohort/tests/lib_test.php @@ -710,7 +710,7 @@ class lib_test extends \advanced_testcase { $cohort2 = $this->getDataGenerator()->create_cohort(); // Test cohort_get_cohort. - $result = cohort_get_cohort($cohort1->id, \context_system::instance(), true); + $result = cohort_get_cohort($cohort1->id, $coursectx, true); $this->assertObjectHasAttribute('customfields', $result); $this->assertCount(1, $result->customfields); $field = reset($result->customfields); @@ -719,7 +719,7 @@ class lib_test extends \advanced_testcase { $this->assertEquals('Test value 1', $field->get_value()); // Test custom fields are not returned if not needed. - $result = cohort_get_cohort($cohort1->id, \context_system::instance()); + $result = cohort_get_cohort($cohort1->id, $coursectx); $this->assertObjectNotHasAttribute('customfields', $result); // Test cohort_get_cohorts. @@ -939,4 +939,35 @@ class lib_test extends \advanced_testcase { } } } + + /** + * Test the behaviour of cohort_get_cohort(). + * + * @covers ::cohort_get_cohort + */ + public function test_cohort_get_cohort() { + $this->resetAfterTest(); + + $cat = $this->getDataGenerator()->create_category(); + $cat1 = $this->getDataGenerator()->create_category(['parent' => $cat->id]); + $cat2 = $this->getDataGenerator()->create_category(['parent' => $cat->id]); + + $course1 = $this->getDataGenerator()->create_course(['category' => $cat1->id, 'shortname' => 'ANON1']); + $course2 = $this->getDataGenerator()->create_course(['category' => $cat2->id, 'shortname' => 'ANON2']); + + $cohort1 = $this->getDataGenerator()->create_cohort(['contextid' => \context_coursecat::instance($cat1->id)->id]); + + $result = cohort_get_cohort($cohort1->id, \context_course::instance($course2->id)); + $this->assertFalse($result); + + $result = cohort_get_cohort($cohort1->id, \context_course::instance($course2->id), true); + $this->assertFalse($result); + + $result = cohort_get_cohort($cohort1->id, \context_course::instance($course1->id)); + $this->assertEquals($cohort1->id, $result->id); + + $result = cohort_get_cohort($cohort1->id, \context_course::instance($course1->id), true); + $this->assertEquals($cohort1->id, $result->id); + } + } From a688a9369f0a4ff27b870401522b4617c9ea5561 Mon Sep 17 00:00:00 2001 From: Ilya Tregubov Date: Fri, 7 Jul 2023 15:41:51 +0800 Subject: [PATCH 2/2] MDL-78522 cohort: Enable tests that were disabled in 73839. --- .../uploadcourse/tests/behat/cohorts.feature | 22 +++++++ enrol/cohort/tests/lib_test.php | 64 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/admin/tool/uploadcourse/tests/behat/cohorts.feature b/admin/tool/uploadcourse/tests/behat/cohorts.feature index d2926360622..4edf6c8a9c0 100644 --- a/admin/tool/uploadcourse/tests/behat/cohorts.feature +++ b/admin/tool/uploadcourse/tests/behat/cohorts.feature @@ -33,6 +33,28 @@ Feature: An admin can create courses with cohort enrolments using a CSV file When I click on "Preview" "button" Then I should see "Cohort sync plugin is disabled" + @javascript + Scenario: Validation of cohorts for uploaded courses + Given I upload "admin/tool/uploadcourse/tests/fixtures/enrolment_cohort.csv" file to "File" filemanager + And I click on "Preview" "button" + And I should see "Unknown cohort (Not exist)!" + And I should see "Cohort Cohort 3 not allowed in this context." + When I click on "Upload courses" "button" + And I should see "Unknown cohort (Not exist)!" + And I should see "Cohort Cohort 3 not allowed in this context." + And I should see "Cohort Cohort 4 not allowed in this context." + And I should see "Courses created: 2" + And I should see "Courses updated: 0" + And I should see "Courses errors: 3" + And I am on the "Course 1" "enrolment methods" page + Then I should not see "Cohort sync (Cohort 3 - Student)" + And I am on the "Course 2" "enrolment methods" page + And I should not see "Cohort sync (Cohort 4 - Student)" + And I am on the "Course 3" "enrolment methods" page + And I should see "Cohort sync (Cohort 5 - Student)" + And I click on "Edit" "link" in the "Cohort 5" "table_row" + And the field "Add to group" matches value "None" + @javascript Scenario: Validation of groups for uploaded courses with cohort enrolments Given the following "groups" exist: diff --git a/enrol/cohort/tests/lib_test.php b/enrol/cohort/tests/lib_test.php index 9c16f77327e..6968f5107e2 100644 --- a/enrol/cohort/tests/lib_test.php +++ b/enrol/cohort/tests/lib_test.php @@ -210,6 +210,41 @@ class lib_test extends \advanced_testcase { $this->assertEquals($studentrole->id, $usersrole[$user4->id]->roleid); } + /** + * Test the behaviour of validate_plugin_data_context(). + * + * @covers ::validate_plugin_data_context + */ + public function test_validate_plugin_data_context() { + $this->resetAfterTest(); + + $cohortplugin = enrol_get_plugin('cohort'); + + $cat = $this->getDataGenerator()->create_category(); + $cat1 = $this->getDataGenerator()->create_category(['parent' => $cat->id]); + $cat2 = $this->getDataGenerator()->create_category(['parent' => $cat->id]); + + $course = $this->getDataGenerator()->create_course(['category' => $cat1->id, 'shortname' => 'ANON']); + + $cohort1 = $this->getDataGenerator()->create_cohort(['contextid' => \context_coursecat::instance($cat1->id)->id]); + $cohort2 = $this->getDataGenerator()->create_cohort(['contextid' => \context_coursecat::instance($cat2->id)->id]); + + $enrolmentdata = [ + 'customint1' => $cohort2->id, + 'cohortname' => $cohort2->name, + ]; + $error = $cohortplugin->validate_plugin_data_context($enrolmentdata, $course->id); + $this->assertInstanceOf('lang_string', $error); + $this->assertEquals('contextcohortnotallowed', $error->get_identifier()); + + $enrolmentdata = [ + 'customint1' => $cohort1->id, + 'cohortname' => $cohort1->name, + ]; + $error = $cohortplugin->validate_plugin_data_context($enrolmentdata, $course->id); + $this->assertNull($error); + } + /** * Test the behaviour of fill_enrol_custom_fields(). * @@ -272,9 +307,14 @@ class lib_test extends \advanced_testcase { $cat = $this->getDataGenerator()->create_category(); $cat1 = $this->getDataGenerator()->create_category(['parent' => $cat->id]); + $cat2 = $this->getDataGenerator()->create_category(['parent' => $cat->id]); $course = $this->getDataGenerator()->create_course(['category' => $cat1->id, 'shortname' => 'ANON']); + + $group1 = $this->getDataGenerator()->create_group(['courseid' => $course->id, 'name' => 'Group 1']); + $cohort1 = $this->getDataGenerator()->create_cohort(['contextid' => \context_coursecat::instance($cat1->id)->id]); + $cohort2 = $this->getDataGenerator()->create_cohort(['contextid' => \context_coursecat::instance($cat2->id)->id]); enrol::enable_plugin('cohort', false); @@ -305,10 +345,34 @@ class lib_test extends \advanced_testcase { $errors = $cohortplugin->validate_enrol_plugin_data($enrolmentdata, $course->id); $this->assertArrayHasKey('erroraddtogroupgroupname', $errors); + // Cohort is not allowed on a given category context. + $enrolmentdata['cohortname'] = $cohort2->name; + $errors = $cohortplugin->validate_enrol_plugin_data($enrolmentdata, $course->id); + $this->assertArrayHasKey('contextnotallowed', $errors); + // Group does not exist. unset($enrolmentdata['addtogroup']); $errors = $cohortplugin->validate_enrol_plugin_data($enrolmentdata, $course->id); $this->assertArrayHasKey('errorinvalidgroup', $errors); + + // Valid data when trying to create a group. + $enrolmentdata['cohortname'] = $cohort1->name; + $enrolmentdata['addtogroup'] = 1; + unset($enrolmentdata['groupname']); + $errors = $cohortplugin->validate_enrol_plugin_data($enrolmentdata, $course->id); + $this->assertEmpty($errors); + + // Valid data when trying to add to existing group. + $enrolmentdata['groupname'] = $group1->name; + unset($enrolmentdata['addtogroup']); + $errors = $cohortplugin->validate_enrol_plugin_data($enrolmentdata, $course->id); + $this->assertEmpty($errors); + + // Valid data when trying without group mode. + $enrolmentdata['addtogroup'] = 0; + unset($enrolmentdata['groupname']); + $errors = $cohortplugin->validate_enrol_plugin_data($enrolmentdata, $course->id); + $this->assertEmpty($errors); } }