Moodle
  1. Moodle
  2. MDL-41256

Better debugging information from create_course()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      New exception handling

      Run the core_course_courselib_testcase unit test.

      Exception strings

      1. Create a course with some enroled users.
      2. Create a group with a given ID.
      3. Attempt to create another group with the same ID. Verify that the error reads "This ID number is already in use."
      4. Attempt to create a course with the same idnumber as existing, make sure the error message quotes the idnumber.
      5. Attempt to create a course with the same shortname as existing, make sure the error message quotes the shortname
      Show
      New exception handling Run the core_course_courselib_testcase unit test. Exception strings Create a course with some enroled users. Create a group with a given ID. Attempt to create another group with the same ID. Verify that the error reads "This ID number is already in use." Attempt to create a course with the same idnumber as existing, make sure the error message quotes the idnumber. Attempt to create a course with the same shortname as existing, make sure the error message quotes the shortname
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41256-master
    • Rank:
      52225

      Description

      The exception thrown on a duplicate shortname or idnumber in create_course() doesn't include the actual string, which would be helpful for debugging purposes. Compare how the same issue is handled in duplicate_course() in course/externallib.php.

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          +1

          Show
          Dan Poltawski added a comment - +1
          Hide
          Charles Fulton added a comment -

          I've written a patch to add information for both the shortname and idnumber, plus cleanup of associated comments. Testing will be trickier since you can't hit that exception from the interface. We only hit this using an external course creation tool.

          Show
          Charles Fulton added a comment - I've written a patch to add information for both the shortname and idnumber, plus cleanup of associated comments. Testing will be trickier since you can't hit that exception from the interface. We only hit this using an external course creation tool.
          Hide
          Marina Glancy added a comment -

          Hi Charles, you need to correct the strings too. It's a little mess, 'shortnametaken' in moodle.php has argument and in error.php does not. moodle_exception takes the string from error.php

          $ git grep shortnametaken
          course/edit_form.php:                $errors['shortname']= get_string('shortnametaken', '', $foundcoursenamestring);
          course/externallib.php:                        throw new moodle_exception('shortnametaken');
          course/externallib.php:            throw new moodle_exception('shortnametaken', '', '', $foundcoursenamestring);
          course/lib.php:            throw new moodle_exception('shortnametaken');
          course/request_form.php:            $errors['shortname'] = get_string('shortnametaken', '', $foundcoursenamestring);
          lang/en/error.php:$string['shortnametaken'] = 'Short name is already used for another course';
          lang/en/moodle.php:$string['shortnametaken'] = 'Short name is already used for another course ({$a})';
          

          String for idnumbertaken does not have an argument at all:

          $ git grep idnumbertaken
          course/editcategory_form.php:                    $errors['idnumber']= get_string('idnumbertaken');
          course/externallib.php:                        throw new moodle_exception('idnumbertaken');
          course/lib.php:            throw new moodle_exception('idnumbertaken');
          course/moodleform_mod.php:                $errors['cmidnumber'] = get_string('idnumbertaken');
          grade/edit/tree/calculation.php:                $errors[$giid] = get_string('idnumbertaken');
          grade/edit/tree/item_form.php:                $errors['idnumber'] = get_string('idnumbertaken');
          grade/edit/tree/outcomeitem_form.php:                $errors['idnumber'] = get_string('idnumbertaken');
          group/group_form.php:                    $errors['idnumber']= get_string('idnumbertaken');
          group/group_form.php:            $errors['idnumber']= get_string('idnumbertaken');
          group/grouping_form.php:                    $errors['idnumber']= get_string('idnumbertaken');
          group/grouping_form.php:            $errors['idnumber']= get_string('idnumbertaken');
          group/lib.php:            throw new moodle_exception('idnumbertaken');
          group/lib.php:            throw new moodle_exception('idnumbertaken');
          group/lib.php:            throw new moodle_exception('idnumbertaken');
          group/lib.php:            throw new moodle_exception('idnumbertaken');
          lang/en/error.php:$string['idnumbertaken'] = 'ID number is already used for another course';
          lang/en/moodle.php:$string['idnumbertaken'] = 'This ID number is already taken';
          

          When you correct the strings make sure that all calls to moodle_exception are corrected.

          Thanks for spotting this!

          Show
          Marina Glancy added a comment - Hi Charles, you need to correct the strings too. It's a little mess, 'shortnametaken' in moodle.php has argument and in error.php does not. moodle_exception takes the string from error.php $ git grep shortnametaken course/edit_form.php: $errors['shortname']= get_string('shortnametaken', '', $foundcoursenamestring); course/externallib.php: throw new moodle_exception('shortnametaken'); course/externallib.php: throw new moodle_exception('shortnametaken', '', '', $foundcoursenamestring); course/lib.php: throw new moodle_exception('shortnametaken'); course/request_form.php: $errors['shortname'] = get_string('shortnametaken', '', $foundcoursenamestring); lang/en/error.php:$string['shortnametaken'] = ' Short name is already used for another course'; lang/en/moodle.php:$string['shortnametaken'] = ' Short name is already used for another course ({$a})'; String for idnumbertaken does not have an argument at all: $ git grep idnumbertaken course/editcategory_form.php: $errors['idnumber']= get_string('idnumbertaken'); course/externallib.php: throw new moodle_exception('idnumbertaken'); course/lib.php: throw new moodle_exception('idnumbertaken'); course/moodleform_mod.php: $errors['cmidnumber'] = get_string('idnumbertaken'); grade/edit/tree/calculation.php: $errors[$giid] = get_string('idnumbertaken'); grade/edit/tree/item_form.php: $errors['idnumber'] = get_string('idnumbertaken'); grade/edit/tree/outcomeitem_form.php: $errors['idnumber'] = get_string('idnumbertaken'); group/group_form.php: $errors['idnumber']= get_string('idnumbertaken'); group/group_form.php: $errors['idnumber']= get_string('idnumbertaken'); group/grouping_form.php: $errors['idnumber']= get_string('idnumbertaken'); group/grouping_form.php: $errors['idnumber']= get_string('idnumbertaken'); group/lib.php: throw new moodle_exception('idnumbertaken'); group/lib.php: throw new moodle_exception('idnumbertaken'); group/lib.php: throw new moodle_exception('idnumbertaken'); group/lib.php: throw new moodle_exception('idnumbertaken'); lang/en/error.php:$string['idnumbertaken'] = 'ID number is already used for another course'; lang/en/moodle.php:$string['idnumbertaken'] = 'This ID number is already taken'; When you correct the strings make sure that all calls to moodle_exception are corrected. Thanks for spotting this!
          Hide
          Dan Poltawski added a comment - - edited

          ps. You can test this by expanding test_create_course() or adding a new test case to cover the case in course/tests/courselib_test.php with assertExpectedException.

          Show
          Dan Poltawski added a comment - - edited ps. You can test this by expanding test_create_course() or adding a new test case to cover the case in course/tests/courselib_test.php with assertExpectedException.
          Hide
          Charles Fulton added a comment -

          Thanks Dan and Marina, I'll give this a fresh look at get back to you.

          Show
          Charles Fulton added a comment - Thanks Dan and Marina, I'll give this a fresh look at get back to you.
          Hide
          Charles Fulton added a comment -

          One thing I discovered while working though this issue is that idnumbertaken differs between lang/en/error.php and lang/en/moodle.php, and has a much broader use including groups and groupings. The language is course-specific in error.php only, but I think that's an issue since groups also throws an idnumbertaken exception. Based on this understanding (and I could be screwing all this up) I've introduced a new string in error.php called courseidnumbertaken, and changed idnumbertaken to match what's in moodle.php. I didn't add courseidnumbertaken to moodle.php because it doesn't seem to be needed: there's no client-side validation of the idnumber in the course form (which might be worth its own issue).

          I've updated the course libs to point at the new exception and extended test_create_course() with a pair of try...catch blocks to validate the exceptions.

          Show
          Charles Fulton added a comment - One thing I discovered while working though this issue is that idnumbertaken differs between lang/en/error.php and lang/en/moodle.php, and has a much broader use including groups and groupings. The language is course-specific in error.php only, but I think that's an issue since groups also throws an idnumbertaken exception. Based on this understanding (and I could be screwing all this up) I've introduced a new string in error.php called courseidnumbertaken, and changed idnumbertaken to match what's in moodle.php. I didn't add courseidnumbertaken to moodle.php because it doesn't seem to be needed: there's no client-side validation of the idnumber in the course form (which might be worth its own issue). I've updated the course libs to point at the new exception and extended test_create_course() with a pair of try...catch blocks to validate the exceptions.
          Hide
          Marina Glancy added a comment - - edited

          Thanks Charles, nice spotting of a string mis-usage.

          Code looks good and I +1 the new string. I would say that ID number is "used" and not "taken" in string value to be consistent with short name string (string name can still be 'taken')

          Looking at your changes and at the code I noticed that there is no check for id number duplicate during course update using web form (only in webservices). And I just managed to create two courses with the same ID number. I'll create a new issue for it because it needs fixing in all versions. Thanks again

          Show
          Marina Glancy added a comment - - edited Thanks Charles, nice spotting of a string mis-usage. Code looks good and I +1 the new string. I would say that ID number is "used" and not "taken" in string value to be consistent with short name string (string name can still be 'taken') Looking at your changes and at the code I noticed that there is no check for id number duplicate during course update using web form (only in webservices). And I just managed to create two courses with the same ID number. I'll create a new issue for it because it needs fixing in all versions. Thanks again
          Hide
          Charles Fulton added a comment -

          Thanks Marina, I've altered the string to read "in use." Is this ready for integration? I can't push it myself.

          Show
          Charles Fulton added a comment - Thanks Marina, I've altered the string to read "in use." Is this ready for integration? I can't push it myself.
          Hide
          Marina Glancy added a comment -

          Thanks Charles, I'm pushing for integration. But please add testing instructions.

          Show
          Marina Glancy added a comment - Thanks Charles, I'm pushing for integration. But please add testing instructions.
          Hide
          Charles Fulton added a comment -

          Instructions added.

          Show
          Charles Fulton added a comment - Instructions added.
          Hide
          Dan Poltawski added a comment -

          Thanks Charles, i've integrated this to master now.

          I had some tiny niggles with the unit tests:

          1. We were potentially testing an undefined variable
          2. We weren't explicitly asserting a failure if the exception wasn't experienced
          3. The expected/actual from the assertions were the wrong way round

          That is trivally trivial issues so I just did a quick commit:
          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=2793260fc093f2eee0c84a3a2209478c18578575;hp=c6569641e7c4ea98f13bc6a3b9206f4df1150499

          cheers!
          dan

          Show
          Dan Poltawski added a comment - Thanks Charles, i've integrated this to master now. I had some tiny niggles with the unit tests: We were potentially testing an undefined variable We weren't explicitly asserting a failure if the exception wasn't experienced The expected/actual from the assertions were the wrong way round That is trivally trivial issues so I just did a quick commit: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=2793260fc093f2eee0c84a3a2209478c18578575;hp=c6569641e7c4ea98f13bc6a3b9206f4df1150499 cheers! dan
          Hide
          Charles Fulton added a comment -

          Thanks Dan!

          Show
          Charles Fulton added a comment - Thanks Dan!
          Hide
          Mark Nelson added a comment -

          Test passed, assuming the "This ID number is already in use" in the testing instructions is actually supposed to be "This ID number is already taken".

          I did find another issue while testing this which I created a separate issue for, see - MDL-41588

          Thanks Charles.

          Show
          Mark Nelson added a comment - Test passed, assuming the "This ID number is already in use" in the testing instructions is actually supposed to be "This ID number is already taken". I did find another issue while testing this which I created a separate issue for, see - MDL-41588 Thanks Charles.
          Hide
          Dan Poltawski added a comment -

          Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

          A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

          Show
          Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: