From d7d1ed65a5736ab768f818f106eff1bd53ae9935 Mon Sep 17 00:00:00 2001 From: Vitaly Date: Fri, 18 Sep 2020 21:45:26 +0300 Subject: [PATCH] MDL-56653 enrol_meta: a single DB query in edit_instance_validation The 'edit_instance_validation()' method checks for existing meta enrolment instances. The fix replaces DB queries in a loop for each course with a single query for all courses. Also, a new testing method 'test_edit_instance_validation_with_existing_courses()' was added to /enrol/meta/tests/plugin_test.php to test if the new implementation returns an error in case of trying to save the already linked courses in the 'customint1' field. --- enrol/meta/lib.php | 48 +++++++---- enrol/meta/tests/plugin_test.php | 142 +++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 17 deletions(-) diff --git a/enrol/meta/lib.php b/enrol/meta/lib.php index 632860a4bb9..4a725feff7c 100644 --- a/enrol/meta/lib.php +++ b/enrol/meta/lib.php @@ -328,31 +328,45 @@ class enrol_meta_plugin extends enrol_plugin { */ public function edit_instance_validation($data, $files, $instance, $context) { global $DB; + $errors = array(); $thiscourseid = $context->instanceid; - $c = false; if (!empty($data['customint1'])) { - $courses = is_array($data['customint1']) ? $data['customint1'] : [$data['customint1']]; - foreach ($courses as $courseid) { - $c = $DB->get_record('course', array('id' => $courseid), '*', MUST_EXIST); - $coursecontext = context_course::instance($c->id); + $coursesidarr = is_array($data['customint1']) ? $data['customint1'] : [$data['customint1']]; + list($coursesinsql, $coursesinparams) = $DB->get_in_or_equal($coursesidarr, SQL_PARAMS_NAMED, 'metacourseid'); + if ($coursesrecords = $DB->get_records_select('course', "id {$coursesinsql}", + $coursesinparams, '', 'id,visible')) { + // Cast NULL to 0 to avoid possible mess with the SQL. + $instanceid = $instance->id ?? 0; - $sqlexists = 'enrol = :meta AND courseid = :currentcourseid AND customint1 = :courseid AND id != :id'; - $existing = $DB->record_exists_select('enrol', $sqlexists, [ + $existssql = "enrol = :meta AND courseid = :currentcourseid AND id != :id AND customint1 {$coursesinsql}"; + $existsparams = [ 'meta' => 'meta', 'currentcourseid' => $thiscourseid, - 'courseid' => $c->id, - 'id' => $instance->id - ]); - - if (!$c->visible and !has_capability('moodle/course:viewhiddencourses', $coursecontext)) { - $errors['customint1'] = get_string('error'); - } else if (!has_capability('enrol/meta:selectaslinked', $coursecontext)) { - $errors['customint1'] = get_string('error'); - } else if ($c->id == SITEID or $c->id == $thiscourseid or $existing) { - $errors['customint1'] = get_string('error'); + 'id' => $instanceid + ]; + $existsparams += $coursesinparams; + if ($DB->record_exists_select('enrol', $existssql, $existsparams)) { + // We may leave right here as further checks do not make sense in case we have existing enrol records + // with the parameters from above. + $errors['customint1'] = get_string('invalidcourseid', 'error'); + } else { + foreach ($coursesrecords as $coursesrecord) { + $coursecontext = context_course::instance($coursesrecord->id); + if (!$coursesrecord->visible and !has_capability('moodle/course:viewhiddencourses', $coursecontext)) { + $errors['customint1'] = get_string('nopermissions', 'error', + 'moodle/course:viewhiddencourses'); + } else if (!has_capability('enrol/meta:selectaslinked', $coursecontext)) { + $errors['customint1'] = get_string('nopermissions', 'error', + 'enrol/meta:selectaslinked'); + } else if ($coursesrecord->id == SITEID or $coursesrecord->id == $thiscourseid) { + $errors['customint1'] = get_string('invalidcourseid', 'error'); + } + } } + } else { + $errors['customint1'] = get_string('invalidcourseid', 'error'); } } else { $errors['customint1'] = get_string('required'); diff --git a/enrol/meta/tests/plugin_test.php b/enrol/meta/tests/plugin_test.php index c51ba228463..8fb9b1c74f4 100644 --- a/enrol/meta/tests/plugin_test.php +++ b/enrol/meta/tests/plugin_test.php @@ -922,4 +922,146 @@ class enrol_meta_plugin_testcase extends advanced_testcase { // Meta-link enrolment has enrol actions for suspended students -- unenrol. $this->assertCount(1, $actions); } + + /** + * Test how data for instance editing is validated. + */ + public function test_edit_instance_validation() { + global $DB; + + $this->resetAfterTest(); + + $metaplugin = enrol_get_plugin('meta'); + + // A course with meta enrolment. + $course = $this->getDataGenerator()->create_course(); + $coursecontext = context_course::instance($course->id); + + // Create a meta enrolment instance. + $instance = (object)$metaplugin->get_instance_defaults(); + $instance->id = null; + $instance->courseid = $course->id; + $instance->status = ENROL_INSTANCE_ENABLED; + // Emulate the form data. + $data = [ + 'customint1' => 0, + 'customint2' => 0 + ]; + // Test when no valid 'customint1' field (meta courses links) is provided. + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + // We're going to check the string contents of the errors returned as this is the only way + // to differentiate the errors produced by the 'edit_instance_validation()' method somehow. + // The method always returns what the edit instance form expects and this is an array of form fields + // with the corresponding errors messages. + $this->assertEquals('Required', $errors['customint1']); + + // Test when 'customint1' contains an unknown course. + // Fetch the max course id from the courses table and increment it to get + // the course id which surely doesn't exist. + $maxid = $DB->get_field_sql('SELECT MAX(id) FROM {course}'); + // Use the same instance as before but set another data. + $data = [ + 'customint1' => [$maxid + 1], + 'customint2' => 0 + ]; + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + $this->assertEquals('You are trying to use an invalid course ID', $errors['customint1']); + + // Test when 'customint1' field already contains courses meta linked with the current one. + $metacourse1 = $this->getDataGenerator()->create_course(); + $metaplugin->add_instance($course, array('customint1' => $metacourse1->id)); + // Use the same instance as before but set another data. + $data = [ + 'customint1' => [$metacourse1->id], + 'customint2' => 0 + ]; + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + $this->assertEquals('You are trying to use an invalid course ID', $errors['customint1']); + + // Test when a course is set as a not visible and a user doesn't have the capability to use it here. + $metacourse2record = new stdClass(); + $metacourse2record->visible = 0; + $metacourse2 = $this->getDataGenerator()->create_course($metacourse2record); + $metacourse2context = context_course::instance($metacourse2->id); + + $user = $this->getDataGenerator()->create_user(); + $teacherrole = $DB->get_record('role', array('shortname' => 'teacher')); + role_assign($teacherrole->id, $user->id, $metacourse2context->id); + unassign_capability('moodle/course:viewhiddencourses', $teacherrole->id); + $this->setUser($user); + + // Use the same instance as before but set another data. + $data = [ + 'customint1' => [$metacourse2->id], + 'customint2' => 0 + ]; + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + $this->assertEquals('Sorry, but you do not currently have permissions to do that (moodle/course:viewhiddencourses).', + $errors['customint1']); + + // Revert some changes from the last assertion to reuse the course. + $metacourse2->visible = 1; + $DB->update_record('course', $metacourse2); + assign_capability('moodle/course:viewhiddencourses', CAP_ALLOW, + $teacherrole->id, context_course::instance($metacourse2->id)); + + // Test with no 'enrol/meta:selectaslinked' capability. + unassign_capability('enrol/meta:selectaslinked', $teacherrole->id); + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + $this->assertEquals('Sorry, but you do not currently have permissions to do that (enrol/meta:selectaslinked).', + $errors['customint1']); + + // Back to admin user to regain the capabilities quickly. + $this->setAdminUser(); + + // Test when meta course id is the site id. + $site = $DB->get_record('course', ['id' => SITEID]); + // Use the same instance as before but set another data. + $data = [ + 'customint1' => [$site->id], + 'customint2' => 0 + ]; + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + $this->assertEquals('You are trying to use an invalid course ID', $errors['customint1']); + + // Test when meta course id is id of the current course. + // Use the same instance as before but set another data. + $data = [ + 'customint1' => [$course->id], + 'customint2' => 0 + ]; + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + $this->assertEquals('You are trying to use an invalid course ID', $errors['customint1']); + + // Test with the 'customint2' field set (which is groups). + // Prepare some groups data. + $this->getDataGenerator()->create_group(array('courseid' => $course->id)); + $this->getDataGenerator()->create_group(array('courseid' => $course->id)); + $groups = []; + foreach (groups_get_all_groups($course->id) as $group) { + $groups[$group->id] = format_string($group->name, true, array('context' => $coursecontext)); + } + + // Use the same instance as before but set another data. + // Use a non-existing group id. + if (!$maxid = $DB->get_field_sql('SELECT MAX(id) FROM {groups}')) { + $maxid = 0; + } + $data = [ + 'customint1' => [$metacourse2->id], + 'customint2' => [$maxid + 1] + ]; + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + $this->assertArrayHasKey('customint2', $errors); + + // Test with valid data. + $validgroup = reset($groups); + $data = [ + 'customint1' => [$metacourse2->id], + 'customint2' => $validgroup + ]; + $errors = $metaplugin->edit_instance_validation($data, [], $instance, $coursecontext); + $this->assertArrayNotHasKey('customint1', $errors); + $this->assertArrayNotHasKey('customint2', $errors); + } }