From 4b16c37b9612032cfc8bed6bef438c098157aee7 Mon Sep 17 00:00:00 2001 From: Ilya Tregubov Date: Mon, 5 Feb 2024 15:08:46 +0800 Subject: [PATCH] MDL-80599 core: Refactor enrol methods Move update instance logic to its own method and cover with tests. Also update enrol plugins to work woth new methods. --- admin/tool/uploadcourse/classes/course.php | 45 ++--------- .../tool/uploadcourse/tests/behat/cohorts.feature | 2 +- enrol/cohort/lang/en/deprecated.txt | 1 + enrol/cohort/lang/en/enrol_cohort.php | 4 +- enrol/cohort/lib.php | 6 +- enrol/meta/lib.php | 6 +- enrol/tests/enrollib_test.php | 94 ++++++++++++++++++++++ lang/en/enrol.php | 1 + lib/enrollib.php | 59 +++++++++++++- lib/moodlelib.php | 9 ++- 10 files changed, 173 insertions(+), 54 deletions(-) create mode 100644 enrol/cohort/lang/en/deprecated.txt diff --git a/admin/tool/uploadcourse/classes/course.php b/admin/tool/uploadcourse/classes/course.php index 59477e3ac66..4c2564c7467 100644 --- a/admin/tool/uploadcourse/classes/course.php +++ b/admin/tool/uploadcourse/classes/course.php @@ -1129,34 +1129,7 @@ class tool_uploadcourse_course { break; } - // Now update values. - $modifiedinstance = $instance; - - // Sort out the start, end and date. - $modifiedinstance->enrolstartdate = (isset($method['startdate']) ? strtotime($method['startdate']) : 0); - $modifiedinstance->enrolenddate = (isset($method['enddate']) ? strtotime($method['enddate']) : 0); - - // Is the enrolment period set? - if (isset($method['enrolperiod']) && !empty($method['enrolperiod'])) { - if (preg_match('/^\d+$/', $method['enrolperiod'])) { - $method['enrolperiod'] = (int)$method['enrolperiod']; - } else { - // Try and convert period to seconds. - $method['enrolperiod'] = strtotime('1970-01-01 GMT + ' . $method['enrolperiod']); - } - $modifiedinstance->enrolperiod = $method['enrolperiod']; - } - if ($instance->enrolstartdate > 0 && isset($method['enrolperiod'])) { - $modifiedinstance->enrolenddate = $instance->enrolstartdate + $method['enrolperiod']; - } - if ($instance->enrolenddate > 0) { - $modifiedinstance->enrolperiod = $instance->enrolenddate - $instance->enrolstartdate; - } - if ($instance->enrolenddate < $instance->enrolstartdate) { - $modifiedinstance->enrolenddate = $instance->enrolstartdate; - } - - // Sort out the given role. + // Validate role context again since course is created. if (isset($method['role']) || isset($method['roleid'])) { if (isset($method['role'])) { $role = $method['role']; @@ -1166,22 +1139,14 @@ class tool_uploadcourse_course { $role = $DB->get_field('role', 'shortname', ['id' => $roleid], MUST_EXIST); } if (!$this->validate_role_context($course->id, $roleid)) { - $this->error('contextrolenotallowed', - new lang_string('contextrolenotallowed', 'core_role', $role)); + $this->error('contextrolenotallowed', new lang_string('contextrolenotallowed', 'core_role', $role)); break; } - - $roleids = tool_uploadcourse_helper::get_role_ids(); - if (in_array($roleid, $roleids)) { - $modifiedinstance->roleid = $roleid; - } - } - - // Sort out custom instance name. - if (isset($method['name'])) { - $modifiedinstance->name = $method['name']; } + // Now update values. + // Sort out plugin specific fields. + $modifiedinstance = $plugin->update_enrol_plugin_data($course->id, $method, $instance); $plugin->update_instance($instance, $modifiedinstance); } else { foreach ($errors as $key => $message) { diff --git a/admin/tool/uploadcourse/tests/behat/cohorts.feature b/admin/tool/uploadcourse/tests/behat/cohorts.feature index 4441ed60995..9a3676c6ea6 100644 --- a/admin/tool/uploadcourse/tests/behat/cohorts.feature +++ b/admin/tool/uploadcourse/tests/behat/cohorts.feature @@ -31,7 +31,7 @@ Feature: An admin can create courses with cohort enrolments using a CSV file | enrol_plugins_enabled | manual,guest,self | And I upload "admin/tool/uploadcourse/tests/fixtures/enrolment_cohort.csv" file to "File" filemanager When I click on "Preview" "button" - Then I should see "Cohort sync plugin is disabled" + Then I should see "Cohort sync enrol plugin is disabled" @javascript Scenario: Validation of cohorts for uploaded courses diff --git a/enrol/cohort/lang/en/deprecated.txt b/enrol/cohort/lang/en/deprecated.txt new file mode 100644 index 00000000000..b46113e9e7d --- /dev/null +++ b/enrol/cohort/lang/en/deprecated.txt @@ -0,0 +1 @@ +plugindisabled,enrol_cohort diff --git a/enrol/cohort/lang/en/enrol_cohort.php b/enrol/cohort/lang/en/enrol_cohort.php index 77bf9d601a5..a843bece15f 100644 --- a/enrol/cohort/lang/en/enrol_cohort.php +++ b/enrol/cohort/lang/en/enrol_cohort.php @@ -29,9 +29,11 @@ $string['cohort:unenrol'] = 'Unenrol suspended users'; $string['defaultgroupnametext'] = '{$a->name} cohort {$a->increment}'; $string['enrolcohortsynctask'] = 'Cohort enrolment sync task'; $string['instanceexists'] = 'Cohort is already synchronised with selected role'; -$string['plugindisabled'] = 'Cohort sync plugin is disabled'; $string['pluginname'] = 'Cohort sync'; $string['pluginname_desc'] = 'Cohort enrolment plugin synchronises cohort members with course participants.'; $string['status'] = 'Active'; $string['creategroup'] = 'Create new group'; $string['privacy:metadata:core_group'] = 'Enrol cohort plugin can create a new group or use an existing group to add all the members of the cohort.'; + +// Deprecated since Moodle 4.5. +$string['plugindisabled'] = 'Cohort sync plugin is disabled'; diff --git a/enrol/cohort/lib.php b/enrol/cohort/lib.php index b95f9206962..eba1675ea64 100644 --- a/enrol/cohort/lib.php +++ b/enrol/cohort/lib.php @@ -515,11 +515,7 @@ class enrol_cohort_plugin extends enrol_plugin { public function validate_enrol_plugin_data(array $enrolmentdata, ?int $courseid = null): array { global $DB; - $errors = []; - if (!enrol_is_enabled('cohort')) { - $errors['plugindisabled'] = - new lang_string('plugindisabled', 'enrol_cohort'); - } + $errors = parent::validate_enrol_plugin_data($enrolmentdata, $courseid); if (isset($enrolmentdata['addtogroup'])) { $addtogroup = $enrolmentdata['addtogroup']; diff --git a/enrol/meta/lib.php b/enrol/meta/lib.php index f39b62b2a6a..6c57bcc7c70 100644 --- a/enrol/meta/lib.php +++ b/enrol/meta/lib.php @@ -393,11 +393,7 @@ class enrol_meta_plugin extends enrol_plugin { public function validate_enrol_plugin_data(array $enrolmentdata, ?int $courseid = null): array { global $DB; - $errors = []; - if (!enrol_is_enabled('meta')) { - $errors['plugindisabled'] = - new lang_string('plugindisabled', 'plugin'); - } + $errors = parent::validate_enrol_plugin_data($enrolmentdata, $courseid); if (isset($enrolmentdata['addtogroup'])) { $addtogroup = $enrolmentdata['addtogroup']; diff --git a/enrol/tests/enrollib_test.php b/enrol/tests/enrollib_test.php index add5167a509..db7efa102a2 100644 --- a/enrol/tests/enrollib_test.php +++ b/enrol/tests/enrollib_test.php @@ -23,6 +23,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core\plugininfo\enrol; + defined('MOODLE_INTERNAL') || die(); @@ -1733,4 +1735,96 @@ class enrollib_test extends advanced_testcase { $this->setUser($user2); $this->assertFalse(enrol_selfenrol_available($course->id)); } + + /** + * Test the behaviour of validate_enrol_plugin_data(). + * + * @covers ::validate_enrol_plugin_data + */ + public function test_validate_enrol_plugin_data(): void { + $this->resetAfterTest(); + + // Plugin is disabled in system. + enrol::enable_plugin('manual', false); + $manualplugin = enrol_get_plugin('manual'); + + $enrolmentdata = []; + $errors = $manualplugin->validate_enrol_plugin_data($enrolmentdata); + $this->assertArrayHasKey('plugindisabled', $errors); + $this->assertArrayNotHasKey('errorunsupportedmethod', $errors); + + $categoryplugin = enrol_get_plugin('category'); + $errors = $categoryplugin->validate_enrol_plugin_data($enrolmentdata); + $this->assertArrayHasKey('errorunsupportedmethod', $errors); + } + + /** + * Test the behaviour of update_enrol_plugin_data(). + * + * @covers ::update_enrol_plugin_data + */ + public function test_update_enrol_plugin_data(): void { + global $DB; + $this->resetAfterTest(); + $manualplugin = enrol_get_plugin('manual'); + + $admin = get_admin(); + $this->setUser($admin); + + $enrolmentdata = []; + + $cat = $this->getDataGenerator()->create_category(); + $course = $this->getDataGenerator()->create_course(['category' => $cat->id, 'shortname' => 'ANON']); + $instance = $DB->get_record('enrol', ['courseid' => $course->id, 'enrol' => 'manual'], '*', MUST_EXIST); + + $teacherroleid = $DB->get_field('role', 'id', ['shortname' => 'teacher']); + $editingteacherroleid = $DB->get_field('role', 'id', ['shortname' => 'editingteacher']); + + $enrolmentdata['startdate'] = '3 Feb 2024'; + $enrolmentdata['enddate'] = '4 Feb 2024'; + $enrolmentdata['role'] = 'teacher'; + $enrolmentdata['name'] = 'testinstance'; + + $expectedinstance = $instance; + $expectedinstance->enrolstartdate = strtotime($enrolmentdata['startdate']); + $expectedinstance->enrolenddate = strtotime($enrolmentdata['enddate']); + $expectedinstance->role = $teacherroleid; + $expectedinstance->name = $enrolmentdata['name']; + $expectedinstance->enrolperiod = $expectedinstance->enrolenddate - $expectedinstance->enrolstartdate; + $modifiedinstance = $manualplugin->update_enrol_plugin_data($course->id, $enrolmentdata, $instance); + $this->assertEquals($expectedinstance, $modifiedinstance); + + $enrolmentdata['roleid'] = $editingteacherroleid; + unset($enrolmentdata['startdate']); + unset($enrolmentdata['enddate']); + unset($enrolmentdata['role']); + $enrolmentdata['enrolperiod'] = $modifiedinstance->enrolperiod++; + $expectedinstance->roleid = $editingteacherroleid; + $expectedinstance->enrolstartdate = 0; + $expectedinstance->enrolenddate = 0; + $expectedinstance->enrolperiod++; + $modifiedinstance = $manualplugin->update_enrol_plugin_data($course->id, $enrolmentdata, $instance); + $this->assertEquals($expectedinstance, $modifiedinstance); + + $enrolmentdata['startdate'] = '3 Feb 2024'; + $enrolmentdata['enrolperiod'] = 3600; + $expectedinstance->enrolstartdate = strtotime($enrolmentdata['startdate']); + $expectedinstance->enrolperiod = $enrolmentdata['enrolperiod']; + $expectedinstance->enrolenddate = $expectedinstance->enrolstartdate + $enrolmentdata['enrolperiod']; + $modifiedinstance = $manualplugin->update_enrol_plugin_data($course->id, $enrolmentdata, $instance); + $this->assertEquals($expectedinstance, $modifiedinstance); + + $enrolmentdata['enddate'] = '5 Feb 2024'; + unset($enrolmentdata['enrolperiod']); + $expectedinstance->enrolenddate = strtotime($enrolmentdata['startdate']); + $expectedinstance->enrolperiod = $expectedinstance->enrolenddate - $expectedinstance->enrolstartdate; + $modifiedinstance = $manualplugin->update_enrol_plugin_data($course->id, $enrolmentdata, $instance); + $this->assertEquals($expectedinstance, $modifiedinstance); + + $enrolmentdata['enrolperiod'] = '2hours'; + $expectedinstance->enrolperiod = 7200; + $expectedinstance->enrolenddate = $expectedinstance->enrolstartdate + $expectedinstance->enrolperiod; + $modifiedinstance = $manualplugin->update_enrol_plugin_data($course->id, $enrolmentdata, $instance); + $this->assertEquals($expectedinstance, $modifiedinstance); + } } diff --git a/lang/en/enrol.php b/lang/en/enrol.php index 7d6ad4b494c..72119f2456e 100644 --- a/lang/en/enrol.php +++ b/lang/en/enrol.php @@ -132,6 +132,7 @@ $string['periodend'] = 'until {$a}'; $string['periodnone'] = 'enrolled {$a}'; $string['periodstart'] = 'from {$a}'; $string['periodstartend'] = 'from {$a->start} until {$a->end}'; +$string['plugindisabled'] = '{$a} enrol plugin is disabled'; $string['recovergrades'] = 'Recover user\'s old grades if possible'; $string['rolefromthiscourse'] = '{$a->role} (Assigned in this course)'; $string['rolefrommetacourse'] = '{$a->role} (Inherited from parent course)'; diff --git a/lib/enrollib.php b/lib/enrollib.php index 30031a9d343..11a83a2d5d6 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -3536,7 +3536,12 @@ abstract class enrol_plugin { $errors['errorunsupportedmethod'] = new lang_string('errorunsupportedmethod', 'tool_uploadcourse', get_class($this)); - + } else { + $plugin = $this->get_name(); + if (!enrol_is_enabled($plugin)) { + $pluginname = get_string('pluginname', 'enrol_' . $plugin); + $errors['plugindisabled'] = new lang_string('plugindisabled', 'enrol', $pluginname); + } } return $errors; } @@ -3728,4 +3733,56 @@ abstract class enrol_plugin { message_send($message); } + + /** + * Updates enrol plugin instance with provided data. + * @param int $courseid Course ID. + * @param array $enrolmentdata enrolment data. + * @param stdClass $instance Instance to update. + * + * @return stdClass updated instance + */ + public function update_enrol_plugin_data(int $courseid, array $enrolmentdata, stdClass $instance): stdClass { + global $DB; + + // Sort out the start, end and date. + $instance->enrolstartdate = (isset($enrolmentdata['startdate']) ? strtotime($enrolmentdata['startdate']) : 0); + $instance->enrolenddate = (isset($enrolmentdata['enddate']) ? strtotime($enrolmentdata['enddate']) : 0); + + // Is the enrolment period set? + if (!empty($enrolmentdata['enrolperiod'])) { + if (preg_match('/^\d+$/', $enrolmentdata['enrolperiod'])) { + $enrolmentdata['enrolperiod'] = (int)$enrolmentdata['enrolperiod']; + } else { + // Try and convert period to seconds. + $enrolmentdata['enrolperiod'] = strtotime('1970-01-01 GMT + ' . $enrolmentdata['enrolperiod']); + } + $instance->enrolperiod = $enrolmentdata['enrolperiod']; + } + if ($instance->enrolstartdate > 0 && isset($enrolmentdata['enrolperiod'])) { + $instance->enrolenddate = $instance->enrolstartdate + $enrolmentdata['enrolperiod']; + } + if ($instance->enrolenddate > 0) { + $instance->enrolperiod = $instance->enrolenddate - $instance->enrolstartdate; + } + if ($instance->enrolenddate < $instance->enrolstartdate) { + $instance->enrolenddate = $instance->enrolstartdate; + } + + // Sort out the given role. + if (isset($enrolmentdata['role']) || isset($enrolmentdata['roleid'])) { + if (isset($enrolmentdata['role'])) { + $roleid = $DB->get_field('role', 'id', ['shortname' => $enrolmentdata['role']], MUST_EXIST); + } else { + $roleid = $enrolmentdata['roleid']; + } + $instance->roleid = $roleid; + } + + // Sort out custom instance name. + if (isset($enrolmentdata['name'])) { + $instance->name = $enrolmentdata['name']; + } + return $instance; + } } diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 7e0b0d95134..28edad45038 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4551,31 +4551,38 @@ function get_complete_user_data($field, $value, $mnethostid = null, $throwexcept * @param string $password the password to be checked against the password policy * @param string $errmsg the error message to display when the password doesn't comply with the policy. * @param stdClass $user the user object to perform password validation against. Defaults to null if not provided. + * @param ?array $errorsarray array of errors with string identifiers. * * @return bool true if the password is valid according to the policy. false otherwise. */ -function check_password_policy($password, &$errmsg, $user = null) { +function check_password_policy($password, &$errmsg, $user = null, ?array &$errorsarray = null) { global $CFG; if (!empty($CFG->passwordpolicy) && !isguestuser($user)) { $errmsg = ''; if (core_text::strlen($password) < $CFG->minpasswordlength) { $errmsg .= '
'. get_string('errorminpasswordlength', 'auth', $CFG->minpasswordlength) .'
'; + $errorsarray = ($errorsarray !== null) ? [...$errorsarray, 'errorminpasswordlength'] : null; } if (preg_match_all('/[[:digit:]]/u', $password, $matches) < $CFG->minpassworddigits) { $errmsg .= '
'. get_string('errorminpassworddigits', 'auth', $CFG->minpassworddigits) .'
'; + $errorsarray = ($errorsarray !== null) ? [...$errorsarray, 'errorminpassworddigits'] : null; } if (preg_match_all('/[[:lower:]]/u', $password, $matches) < $CFG->minpasswordlower) { $errmsg .= '
'. get_string('errorminpasswordlower', 'auth', $CFG->minpasswordlower) .'
'; + $errorsarray = ($errorsarray !== null) ? [...$errorsarray, 'errorminpasswordlower'] : null; } if (preg_match_all('/[[:upper:]]/u', $password, $matches) < $CFG->minpasswordupper) { $errmsg .= '
'. get_string('errorminpasswordupper', 'auth', $CFG->minpasswordupper) .'
'; + $errorsarray = ($errorsarray !== null) ? [...$errorsarray, 'errorminpasswordupper'] : null; } if (preg_match_all('/[^[:upper:][:lower:][:digit:]]/u', $password, $matches) < $CFG->minpasswordnonalphanum) { $errmsg .= '
'. get_string('errorminpasswordnonalphanum', 'auth', $CFG->minpasswordnonalphanum) .'
'; + $errorsarray = ($errorsarray !== null) ? [...$errorsarray, 'errorminpasswordnonalphanum'] : null; } if (!check_consecutive_identical_characters($password, $CFG->maxconsecutiveidentchars)) { $errmsg .= '
'. get_string('errormaxconsecutiveidentchars', 'auth', $CFG->maxconsecutiveidentchars) .'
'; + $errorsarray = ($errorsarray !== null) ? [...$errorsarray, 'errormaxconsecutiveidentchars'] : null; } // Fire any additional password policy functions from plugins. -- 2.11.4.GIT