From 5536a5617c1c1a0073d8ebe53e08191c5a3b9aac Mon Sep 17 00:00:00 2001 From: Francis Devine Date: Wed, 4 Sep 2013 21:21:41 +1200 Subject: [PATCH 1/2] 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); From c3bf6181aa3b2b897d55e4ca213b5152b649db4b Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Fri, 6 Sep 2013 12:40:15 +0800 Subject: [PATCH 2/2] MDL-41417 course: allow the use of duplicated idnumbers if they existed before fix --- course/edit_form.php | 6 +++--- course/lib.php | 1 - course/tests/courselib_test.php | 7 +++---- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/course/edit_form.php b/course/edit_form.php index 06de0869899..d894efb2013 100644 --- a/course/edit_form.php +++ b/course/edit_form.php @@ -329,7 +329,7 @@ class course_edit_form extends moodleform { /// perform some extra moodle validation function validation($data, $files) { - global $DB, $CFG; + global $DB; $errors = parent::validation($data, $files); @@ -341,10 +341,10 @@ class course_edit_form extends moodleform { } // Add field validation check for duplicate idnumber. - if (!empty($data['idnumber'])) { + if (!empty($data['idnumber']) && (empty($data['id']) || $this->course->idnumber != $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); + $errors['idnumber'] = get_string('courseidnumbertaken', 'error', $course->fullname); } } } diff --git a/course/lib.php b/course/lib.php index e92c5b4e804..af7854cb008 100644 --- a/course/lib.php +++ b/course/lib.php @@ -2382,7 +2382,6 @@ function update_course($data, $editoroptions = NULL) { } } - 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 c48ea4fb33f..85bcac3b22d 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -660,7 +660,8 @@ class core_course_courselib_testcase extends advanced_testcase { global $DB; $this->resetAfterTest(); - $defaultcategory = $DB->get_field_select('course_categories', "MIN(id)", "parent=0"); + + $defaultcategory = $DB->get_field_select('course_categories', 'MIN(id)', 'parent = 0'); $course = new stdClass(); $course->fullname = 'Apu loves Unit Təsts'; @@ -672,11 +673,10 @@ class core_course_courselib_testcase extends advanced_testcase { $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); + update_course($created); $course->shortname = 'test2'; $course->idnumber = '2'; @@ -695,7 +695,6 @@ class core_course_courselib_testcase extends advanced_testcase { // 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');