From 8984f666a1f464f27591aa8037016bc680910403 Mon Sep 17 00:00:00 2001 From: Francis Devine Date: Wed, 4 Sep 2013 21:21:41 +1200 Subject: [PATCH] Duplicate id and shortname checks for update_course, better form validation for edit_form, unit tests that verify duplicate id's and shortnames are not allowed, redundant tests in externallib.php removed --- course/edit_form.php | 25 ++++++++++++--------- course/externallib.php | 10 ++------- course/lib.php | 15 +++++++++++++ course/tests/courselib_test.php | 47 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 18 deletions(-) diff --git a/course/edit_form.php b/course/edit_form.php index 06f7435..3b40e46 100644 --- a/course/edit_form.php +++ b/course/edit_form.php @@ -332,19 +332,24 @@ class course_edit_form extends moodleform { global $DB, $CFG; $errors = parent::validation($data, $files); - if ($foundcourses = $DB->get_records('course', array('shortname'=>$data['shortname']))) { - if (!empty($data['id'])) { - unset($foundcourses[$data['id']]); - } - if (!empty($foundcourses)) { - foreach ($foundcourses as $foundcourse) { - $foundcoursenames[] = $foundcourse->fullname; - } - $foundcoursenamestring = implode(',', $foundcoursenames); - $errors['shortname']= get_string('shortnametaken', '', $foundcoursenamestring); + //better field validation check for duplicate shortname + if ($course = $DB->get_record('course', array('shortname'=>$data['shortname']))) { + if($course->id != $data['id']) { + $errors['shortname'] = get_string('shortnametaken', '', $course->fullname); } } + //check we don't have a duplicate idnumber + + if (!empty($data['idnumber'])) { + if ($course = $DB->get_record('course', array('idnumber'=>$data['idnumber']))) { + if ($course->id != $data['id']) { + $errors['idnumber']= get_string('courseidnumbertaken', 'error', $course->fullname); + } + } + } + + $errors = array_merge($errors, enrol_course_edit_validation($data, $this->context)); $courseformat = course_get_format((object)array('format' => $data['format'])); diff --git a/course/externallib.php b/course/externallib.php index 26742db..8bf16e0 100644 --- a/course/externallib.php +++ b/course/externallib.php @@ -717,20 +717,14 @@ class core_course_external extends external_api { require_capability('moodle/course:changefullname', $context); } - // Check if the shortname already exist and user have capability. + // Check if the user can change shortname if (array_key_exists('shortname', $course) && ($oldcourse->shortname != $course['shortname'])) { require_capability('moodle/course:changeshortname', $context); - if ($DB->record_exists('course', array('shortname' => $course['shortname']))) { - throw new moodle_exception('shortnametaken', '', '', $course['shortname']); - } } - // Check if the id number already exist and user have capability. + // Check if the user can change the idnumber if (array_key_exists('idnumber', $course) && ($oldcourse->idnumber != $course['idnumber'])) { require_capability('moodle/course:changeidnumber', $context); - if ($DB->record_exists('course', array('idnumber' => $course['idnumber']))) { - throw new moodle_exception('courseidnumbertaken', '', '', $course['idnumber']); - } } // Check if user can change summary. diff --git a/course/lib.php b/course/lib.php index 5cfa240..15884ac 100644 --- a/course/lib.php +++ b/course/lib.php @@ -2367,6 +2367,21 @@ function update_course($data, $editoroptions = NULL) { if ($overviewfilesoptions = course_overviewfiles_options($data->id)) { $data = file_postupdate_standard_filemanager($data, 'overviewfiles', $overviewfilesoptions, $context, 'course', 'overviewfiles', 0); } + + //check we don't have a duplicate shortname + if(!empty($data->shortname) and $oldcourse->shortname != $data->shortname) { + if ($DB->record_exists('course', array('shortname' => $data->shortname))) { + throw new moodle_exception('shortnametaken', '', '', $data->shortname); + } + } + + //check we don't have a duplicate idnumber + if (!empty($data->idnumber) and $oldcourse->idnumber != $data->idnumber) { + if ($DB->record_exists('course', array('idnumber' => $data->idnumber))) { + throw new moodle_exception('courseidnumbertaken', '', '', $data->idnumber); + } + } + if (!isset($data->category) or empty($data->category)) { // prevent nulls and 0 in category field diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index eb1eab1..a0724c7 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -655,6 +655,53 @@ class core_course_courselib_testcase extends advanced_testcase { $sectionscreated = array_keys(get_fast_modinfo($course)->get_section_info_all()); $this->assertEquals(range(0, $course->numsections + 1), $sectionscreated); } + + public function test_update_course() { + global $DB; + $this->resetAfterTest(true); + $defaultcategory = $DB->get_field_select('course_categories', "MIN(id)", "parent=0"); + + $course = new stdClass(); + $course->fullname = 'Apu loves Unit Təsts'; + $course->shortname = 'test1'; + $course->idnumber = '1'; + $course->summary = 'Awesome!'; + $course->summaryformat = FORMAT_PLAIN; + $course->format = 'topics'; + $course->newsitems = 0; + $course->numsections = 5; + $course->category = $defaultcategory; + $original = (array) $course; + + $created = create_course($course); + //ensure the checks only work on idnumber/shortname that are not already ours + $created = update_course($created); + + $course->shortname = 'test2'; + $course->idnumber = '2'; + + $created2 = create_course($course); + + //test duplicate idnumber + $created2->idnumber = '1'; + try { + update_course($created2); + $this->fail('Expected exception when trying to update a course with duplicate idnumber'); + } catch (moodle_exception $e) { + $this->assertEquals(get_string('courseidnumbertaken', 'error', $created2->idnumber), $e->getMessage()); + } + + //test duplicate shortname + $created2->idnumber = '2'; + $created2->shortname = 'test1'; + + try { + update_course($created2); + $this->fail('Expected exception when trying to update a course with a duplicate shortname'); + } catch (moodle_exception $e) { + $this->assertEquals(get_string('shortnametaken', 'error', $created2->shortname), $e->getMessage()); + } + } public function test_course_add_cm_to_section() { global $DB; -- 1.7.10.4