From 5536a5617c1c1a0073d8ebe53e08191c5a3b9aac Mon Sep 17 00:00:00 2001 From: Francis Devine Date: Wed, 4 Sep 2013 21:21:41 +1200 Subject: [PATCH] MDL-41417 course: prevent duplicate idnumbers being used when updating a course --- course/edit_form.php | 20 ++++++++------ course/externallib.php | 10 ++----- course/lib.php | 15 +++++++++++ course/tests/courselib_test.php | 48 +++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 16 deletions(-) diff --git a/course/edit_form.php b/course/edit_form.php index 06f74359cfc..06de0869899 100644 --- a/course/edit_form.php +++ b/course/edit_form.php @@ -332,16 +332,20 @@ 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']]); + + // Add field validation check for duplicate shortname. + if ($course = $DB->get_record('course', array('shortname' => $data['shortname']), '*', IGNORE_MULTIPLE)) { + if (empty($data['id']) || $course->id != $data['id']) { + $errors['shortname'] = get_string('shortnametaken', '', $course->fullname); } - if (!empty($foundcourses)) { - foreach ($foundcourses as $foundcourse) { - $foundcoursenames[] = $foundcourse->fullname; + } + + // Add field validation check for duplicate idnumber. + if (!empty($data['idnumber'])) { + if ($course = $DB->get_record('course', array('idnumber' => $data['idnumber']), '*', IGNORE_MULTIPLE)) { + if (empty($data['id']) || $course->id != $data['id']) { + $errors['idnumber']= get_string('courseidnumbertaken', 'error', $course->fullname); } - $foundcoursenamestring = implode(',', $foundcoursenames); - $errors['shortname']= get_string('shortnametaken', '', $foundcoursenamestring); } } diff --git a/course/externallib.php b/course/externallib.php index 26742dba53c..2d61c28cb86 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 5cfa240165e..e92c5b4e804 100644 --- a/course/lib.php +++ b/course/lib.php @@ -2368,6 +2368,21 @@ function update_course($data, $editoroptions = NULL) { $data = file_postupdate_standard_filemanager($data, 'overviewfiles', $overviewfilesoptions, $context, 'course', 'overviewfiles', 0); } + // Check we don't have a duplicate shortname. + if (!empty($data->shortname) && $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) && $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 unset($data->category); diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index 4c42b9f2a01..c48ea4fb33f 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -656,6 +656,54 @@ class core_course_courselib_testcase extends advanced_testcase { $this->assertEquals(range(0, $course->numsections + 1), $sectionscreated); } + public function test_update_course() { + global $DB; + + $this->resetAfterTest(); + $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; $this->resetAfterTest(true);