From 5f8e23c1a92f37fba3d33553bd2dc351e1eb8685 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/lib.php | 15 +++++++++++ course/tests/courselib_test.php | 48 +++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/course/edit_form.php b/course/edit_form.php index a305da4ced3..1fbdf73c8ca 100644 --- a/course/edit_form.php +++ b/course/edit_form.php @@ -331,16 +331,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/lib.php b/course/lib.php index 1796bae10da..9ed6def7484 100644 --- a/course/lib.php +++ b/course/lib.php @@ -3962,6 +3962,21 @@ function update_course($data, $editoroptions = NULL) { $data = file_postupdate_standard_editor($data, 'summary', $editoroptions, $context, 'course', 'summary', 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 f8f185fd07f..d2f825dd31b 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -97,6 +97,54 @@ class 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 eb57e9e60b4cb16c04449f6fc19c5a7295ed0aa6 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 1fbdf73c8ca..373e149baff 100644 --- a/course/edit_form.php +++ b/course/edit_form.php @@ -328,7 +328,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); @@ -340,10 +340,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 9ed6def7484..bdb14936dcb 100644 --- a/course/lib.php +++ b/course/lib.php @@ -3976,7 +3976,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 d2f825dd31b..9d167bfa060 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -101,7 +101,8 @@ class 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'; @@ -113,11 +114,10 @@ class 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'; @@ -136,7 +136,6 @@ class 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');