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

      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.

        Gliffy Diagrams

          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: