Moodle
  1. Moodle
  2. MDL-41417

Possible to have two courses with the same ID number

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4.5, 2.5.1
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      Before patch is applied
      1. Create a course with an idnumber.
      2. Create another course with a different idnumber.
      3. Edit the second course and change the idnumber to the idnumber for the first course (there is only a check on duplicate idnumbers when creating a course, not updating).
      After patch is applied
      Test 1. Web interface
      1. Go to the settings for one of the courses you created prior to testing and do not edit anything and save, ensure it saves without an idnumber warning (even though the courses share the same idnumber).
      2. Create course with filled shortname and idnumber
      3. Attempt to create another course with the same shortname, make sure the nice descriptive error message appears
      4. Attempt to create another course with the same idnumber, make sure the nice descriptive error message appears
      5. Create another course with different shortname and empty/different idnumber
      6. Attempt to update the second course and set the same shortname as in the first course, make sure the nice descriptive error message appears
      7. Attempt to update the second course and set the same idnumber as in the first course, make sure the nice descriptive error message appears
      8. Edit some other fields and save the changes, make sure there are no errors
      9. Edit second course and try to change shortname to unused one, make sure it is updated ok
      10. Edit second course and try to change idnumber to unused one, make sure it is updated ok
      11. Revoke from teacher capability to change course shortname
      12. Login as teacher and edit any other fields in the course, make sure everything is edited ok and without warnings.
      13. Repeat for user without capability to change course idnumber
      Test 2. Webservices (2.5 and master only)
      1. Create and update course using webservices, make sure you can not create or update course with the same shortname or idnumber as another existing course (basically repeat test 1 but using webservices)
      Show
      Before patch is applied Create a course with an idnumber. Create another course with a different idnumber. Edit the second course and change the idnumber to the idnumber for the first course (there is only a check on duplicate idnumbers when creating a course, not updating). After patch is applied Test 1. Web interface Go to the settings for one of the courses you created prior to testing and do not edit anything and save, ensure it saves without an idnumber warning (even though the courses share the same idnumber). Create course with filled shortname and idnumber Attempt to create another course with the same shortname, make sure the nice descriptive error message appears Attempt to create another course with the same idnumber, make sure the nice descriptive error message appears Create another course with different shortname and empty/different idnumber Attempt to update the second course and set the same shortname as in the first course, make sure the nice descriptive error message appears Attempt to update the second course and set the same idnumber as in the first course, make sure the nice descriptive error message appears Edit some other fields and save the changes, make sure there are no errors Edit second course and try to change shortname to unused one, make sure it is updated ok Edit second course and try to change idnumber to unused one, make sure it is updated ok Revoke from teacher capability to change course shortname Login as teacher and edit any other fields in the course, make sure everything is edited ok and without warnings. Repeat for user without capability to change course idnumber Test 2. Webservices (2.5 and master only) Create and update course using webservices, make sure you can not create or update course with the same shortname or idnumber as another existing course (basically repeat test 1 but using webservices)
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
      MDL-41417_m24
    • Pull 2.5 Branch:
      MDL-41417_m25
    • Pull Master Branch:
      MDL-41417_master
    • Rank:
      52433

      Description

      As found while working on MDL-41256 there is no check for duplicate of course ID number when updating the course. It is impossible to create a course with the same ID number as existing but it is possible to create a course without an ID number and then edit it and change to existing ID number.

      Webservices add their own check but instead it should be inside update_course() function (and additional check in course_edit_form::validate() ).

      1. patch_2.txt
        7 kB
        Francis Devine
      2. patch.txt
        3 kB
        Francis Devine

        Issue Links

          Activity

          Hide
          Francis Devine added a comment -

          This patch adds duplicate id and short name checks in update_course

          it also adds duplicate id validation to the course edit form

          Show
          Francis Devine added a comment - This patch adds duplicate id and short name checks in update_course it also adds duplicate id validation to the course edit form
          Hide
          Marina Glancy added a comment -

          Thanks Francis, this patch addresses the right places.

          Whoever takes over there are a couple of things to fix/add to the patch:
          1. There is some copy-pasted text in patch in update_course() when checking shortname - it is compared against idnumber
          2. It needs unittests
          3. The idnumber check needs to be excluded from webservices that call update_course()
          4. Patch needs to be rebased over MDL-41256 (currently in integration) and use correct error strings with arguments

          Thanks again for working on it

          Show
          Marina Glancy added a comment - Thanks Francis, this patch addresses the right places. Whoever takes over there are a couple of things to fix/add to the patch: 1. There is some copy-pasted text in patch in update_course() when checking shortname - it is compared against idnumber 2. It needs unittests 3. The idnumber check needs to be excluded from webservices that call update_course() 4. Patch needs to be rebased over MDL-41256 (currently in integration) and use correct error strings with arguments Thanks again for working on it
          Hide
          Francis Devine added a comment - - edited

          Oops, my apologies, that's what you get for committing things late at night.

          I'm happy to fix that and the other things you mentioned and upload a new patch asap.
          Could you point me towards the web services that are doing manual checks themselves?
          e: Is webservices referring to the code in course/externallib.php?
          Sorry for the questions, I'm still new to moodles architecture.

          Show
          Francis Devine added a comment - - edited Oops, my apologies, that's what you get for committing things late at night. I'm happy to fix that and the other things you mentioned and upload a new patch asap. Could you point me towards the web services that are doing manual checks themselves? e: Is webservices referring to the code in course/externallib.php? Sorry for the questions, I'm still new to moodles architecture.
          Hide
          Marina Glancy added a comment - - edited

          Hi Francis, this is really great start for a moodle newbie

          If you want to bring this issue to the final state, here are couple of links/hints:

          1. In /course/externallib.php in function core_course_external::update_courses() there is an additional check for duplication of shortname and idnubmer. Now when it is part of update_course() (where it should be) it needs to be removed from there. But you still need to leave capability check there because update_course() does not perform any capabilities checks.
          2. Don't know if you are aware of unittests, if not check the page on your local installation (logged in as admin): Site Administration – > Development – > PHPUnit tests. We try to have the most of moodle functionality covered by unittests and it is obligatory for such core API functions like update_course. New test has to be added to course/tests/courselib_test.php to test the exceptions throwing, similar to one in course/tests/externallib_test.php but instead of checking warnings it needs to call $this->setExpectedException() (search the code for examples)
          3. And the last, I really recommend you to create a github.com account and setup git. See http://docs.moodle.org/dev/Git_for_developers

          I hope it's not too much

          Show
          Marina Glancy added a comment - - edited Hi Francis, this is really great start for a moodle newbie If you want to bring this issue to the final state, here are couple of links/hints: In /course/externallib.php in function core_course_external::update_courses() there is an additional check for duplication of shortname and idnubmer. Now when it is part of update_course() (where it should be) it needs to be removed from there. But you still need to leave capability check there because update_course() does not perform any capabilities checks. Don't know if you are aware of unittests, if not check the page on your local installation (logged in as admin): Site Administration – > Development – > PHPUnit tests. We try to have the most of moodle functionality covered by unittests and it is obligatory for such core API functions like update_course. New test has to be added to course/tests/courselib_test.php to test the exceptions throwing, similar to one in course/tests/externallib_test.php but instead of checking warnings it needs to call $this->setExpectedException() (search the code for examples) And the last, I really recommend you to create a github.com account and setup git. See http://docs.moodle.org/dev/Git_for_developers I hope it's not too much
          Hide
          Mark Nelson added a comment -

          Can't believe this has only been discovered now. The check only occurs when creating a course, not updating. I am increasing the urgency of this as you can pass the shortname to course/view.php, and in this case it will break.

          Show
          Mark Nelson added a comment - Can't believe this has only been discovered now. The check only occurs when creating a course, not updating. I am increasing the urgency of this as you can pass the shortname to course/view.php, and in this case it will break.
          Hide
          Francis Devine added a comment - - edited

          This is attempt number 2 at fixing this issue and the patch does a number of things

          Firstly it adds duplicate idnumber and shortname checks to update_course using the new error messages bought in by MDL-41256

          Secondly it removes the now redundant checks in externallib.php's update_course function

          Thirdly it improves the edit_form.php validation code. It now checks for duplicate idnumbers and has a better error message for users. I believe this will likely solve MDL-41588

          Finally it adds a new unit test test_update_course, this unit test checks that updating a course with a duplicate idnumber or shortname fails with an error. It also tests that you can successfully update with a duplicate idnumber and shortname where that idnumber and shortname belong to you anyway

          I've hopefully got everything this time around but any comments or criticisms are greatly appreciated.

          My github is optimumtact and my moodle fork is located here

          Here is the compare view, unfortunately it's not as useful since it contains the changes from MDL-41256 which pollute things slightly

          Edit: here is the actual commit view on github

          Show
          Francis Devine added a comment - - edited This is attempt number 2 at fixing this issue and the patch does a number of things Firstly it adds duplicate idnumber and shortname checks to update_course using the new error messages bought in by MDL-41256 Secondly it removes the now redundant checks in externallib.php's update_course function Thirdly it improves the edit_form.php validation code. It now checks for duplicate idnumbers and has a better error message for users. I believe this will likely solve MDL-41588 Finally it adds a new unit test test_update_course, this unit test checks that updating a course with a duplicate idnumber or shortname fails with an error. It also tests that you can successfully update with a duplicate idnumber and shortname where that idnumber and shortname belong to you anyway I've hopefully got everything this time around but any comments or criticisms are greatly appreciated. My github is optimumtact and my moodle fork is located here Here is the compare view, unfortunately it's not as useful since it contains the changes from MDL-41256 which pollute things slightly Edit: here is the actual commit view on github
          Hide
          Marina Glancy added a comment -

          This is amazing Francis, I don't believe you've never done it before
          You are not yet in developers group so I can't assign an issue to you, I assigned it to myself. Also I filled all necessary git fields and wrote testing instructions. You are welcome to correct if you think that something is missing. It is not necessary to ask to run unittests in testing instructions because they are run automatically on integration server.
          Now the development procedure is to ask for peer review. When it is successful we'll need to create branches for stable versions. This is definitely a bug and it needs to be backported to 2.5 and 2.4. (Previous versions are not supported except for serious security issues.)

          PS Mark, do you want to peer review?

          Show
          Marina Glancy added a comment - This is amazing Francis, I don't believe you've never done it before You are not yet in developers group so I can't assign an issue to you, I assigned it to myself. Also I filled all necessary git fields and wrote testing instructions. You are welcome to correct if you think that something is missing. It is not necessary to ask to run unittests in testing instructions because they are run automatically on integration server. Now the development procedure is to ask for peer review. When it is successful we'll need to create branches for stable versions. This is definitely a bug and it needs to be backported to 2.5 and 2.4. (Previous versions are not supported except for serious security issues.) PS Mark, do you want to peer review?
          Hide
          Marina Glancy added a comment -

          BTW Francis, I'm sure peer reviewer will find some violations of coding styles ( http://docs.moodle.org/dev/Coding_style ). Don't worry, all developers have to modify their commits several times, even those that work with Moodle for many years.

          Show
          Marina Glancy added a comment - BTW Francis, I'm sure peer reviewer will find some violations of coding styles ( http://docs.moodle.org/dev/Coding_style ). Don't worry, all developers have to modify their commits several times, even those that work with Moodle for many years.
          Hide
          Francis Devine added a comment -

          Thanks for all your help so far! The testing instructions look good to me.

          Regarding the backporting, I'll have a look at applying the changes onto 2.4 and 2.5 and checking for any errors/issues that arise.

          I'm assuming MDL-41256 won't be backported so I'll use the previously existing error messages from 2.4 and 2.5

          Please let me know if my assumption here is incorrect

          Thanks again!

          Show
          Francis Devine added a comment - Thanks for all your help so far! The testing instructions look good to me. Regarding the backporting, I'll have a look at applying the changes onto 2.4 and 2.5 and checking for any errors/issues that arise. I'm assuming MDL-41256 won't be backported so I'll use the previously existing error messages from 2.4 and 2.5 Please let me know if my assumption here is incorrect Thanks again!
          Hide
          Marina Glancy added a comment -

          You assumptions are absolutely right! My recommendation is to wait for peer review before creating stable branches. Otherwise you'll have to fix 3 branches instead of one.
          One thing that peer reviewer will ask you to do for sure: the commit message must start with the issue number, in your case MDL-41417 - It makes it easier later to do "git log --grep MDL-xxxx" and just view the commit log.
          Other things are whitespaces in the end of the lines, missing fullstop in the end of the comment, missing space in the beginning of comment, multiple consecutive empty lines, etc. We try to keep the coding style consistent but those rules were introduced just couple of years ago so there is still a lot of old code in moodle with jumping indentation, odd comments, etc.
          There is a tool https://github.com/moodlehq/moodle-local_codechecker to check the coding style and another one https://github.com/moodlehq/moodle-local_moodlecheck to check phpdocs (not applicable in your case because you did not create any new functions) if you want to run them yourself.

          It was my big pleasure to help you with this issue. Even though I realise that I would fix it myself faster than writing all those comments, it is always great to see a new developer picking up so fast. Besides I hope it is not the last issue you are fixing and this will pay back in the end If you need my comments/advice on other issues, you can always add me as a watcher.

          PS I pulled your branch, tested the web interface edit, run all phpunit tests - everything works great. Your patch also fixes MDL-41588

          Show
          Marina Glancy added a comment - You assumptions are absolutely right! My recommendation is to wait for peer review before creating stable branches. Otherwise you'll have to fix 3 branches instead of one. One thing that peer reviewer will ask you to do for sure: the commit message must start with the issue number, in your case MDL-41417 - It makes it easier later to do "git log --grep MDL-xxxx" and just view the commit log. Other things are whitespaces in the end of the lines, missing fullstop in the end of the comment, missing space in the beginning of comment, multiple consecutive empty lines, etc. We try to keep the coding style consistent but those rules were introduced just couple of years ago so there is still a lot of old code in moodle with jumping indentation, odd comments, etc. There is a tool https://github.com/moodlehq/moodle-local_codechecker to check the coding style and another one https://github.com/moodlehq/moodle-local_moodlecheck to check phpdocs (not applicable in your case because you did not create any new functions) if you want to run them yourself. It was my big pleasure to help you with this issue. Even though I realise that I would fix it myself faster than writing all those comments, it is always great to see a new developer picking up so fast. Besides I hope it is not the last issue you are fixing and this will pay back in the end If you need my comments/advice on other issues, you can always add me as a watcher. PS I pulled your branch, tested the web interface edit, run all phpunit tests - everything works great. Your patch also fixes MDL-41588
          Hide
          Mark Nelson added a comment -

          Hi Francis, I will be happy to peer review this. I am finishing up for the night so will do it tomorrow. Cheers!

          Show
          Mark Nelson added a comment - Hi Francis, I will be happy to peer review this. I am finishing up for the night so will do it tomorrow. Cheers!
          Hide
          Mark Nelson added a comment -

          Hi Francis,

          Great work!

          A few things I noticed. Very trivial, just pointing them out so the code conforms to Moodle guidelines.

          course/edit_form.php
          1. The comment "//better field validation check for duplicate shortname" should have spacing after "//", start with a capital and end with a full-stop. Also, the term "better" would be appropriate for a commit message, but not in this case as a user looking at the code will not actually know what it is 'better' than. You can simply just use "// Add field validation check for duplicate shortnames."
          2. Add spacing before and after '=>' in "('shortname'=>$data['shortname'])".
          3. Add spacing between the 'if' and the '(' in "if($course->id != $data['id'])".
          4. "//check we don't have a duplicate idnumber", again this needs to be formatted. I suggest "// Add field validation check for duplicate idnumbers." to match the comment regarding shortnames.
          5. The empty line after the above comment is not necessary.
          6. Add spacing before and after '=>' in "('idnumber'=>$data['idnumber'])".
          7. Change "$errors['idnumber']=" to "$errors['idnumber'] =" (notice the space change).
          8. Remove all whitespaces.
          course/externallib.php
          1. Add full-stops after the comments here.
          course/lib.php
          1. Add spacing after "//", start your comments with capitals and add full-stops.
          2. Add spacing between the 'if' and the '(' in "if(!empty($data->shortname)".
          3. Remove all whitespaces.
          course/tests/courselib_test.php
          1. Add spacing after "//", start your comments with capitals and add full-stops.
          2. I usually add a new line after a global declaration.
          3. The true in "$this->resetAfterTest(true);" is not necessary.
          4. Remove all whitespaces.
          General
          1. I prefer to use '&&' and '||' instead of 'and' and 'or', please see http://stackoverflow.com/questions/2803321/and-vs-as-operator. In your example it won't be an issue, just wanted to point it out fyi.
          2. I suggest creating an upgrade path for sites where there may been an issue where courses use the same idnumbers. That way you know the $DB->get_record call in your validation won't complain about there being more than one. I suggest doing something like adding (2) to the idnumber for the course that was added after the other, though there could potentially be numerous sites using the same idnumber, so keep this logic and add (3) and so on to alter them.

          Thanks for all your hard work so far, it's looking great, especially for someone new to Moodle!

          Regards,

          Mark

          Show
          Mark Nelson added a comment - Hi Francis, Great work! A few things I noticed. Very trivial, just pointing them out so the code conforms to Moodle guidelines. course/edit_form.php The comment "//better field validation check for duplicate shortname" should have spacing after "//", start with a capital and end with a full-stop. Also, the term "better" would be appropriate for a commit message, but not in this case as a user looking at the code will not actually know what it is 'better' than. You can simply just use "// Add field validation check for duplicate shortnames." Add spacing before and after '=>' in "('shortname'=>$data ['shortname'] )". Add spacing between the 'if' and the '(' in "if($course->id != $data ['id'] )". "//check we don't have a duplicate idnumber", again this needs to be formatted. I suggest "// Add field validation check for duplicate idnumbers." to match the comment regarding shortnames. The empty line after the above comment is not necessary. Add spacing before and after '=>' in "('idnumber'=>$data ['idnumber'] )". Change "$errors ['idnumber'] =" to "$errors ['idnumber'] =" (notice the space change). Remove all whitespaces. course/externallib.php Add full-stops after the comments here. course/lib.php Add spacing after "//", start your comments with capitals and add full-stops. Add spacing between the 'if' and the '(' in "if(!empty($data->shortname)". Remove all whitespaces. course/tests/courselib_test.php Add spacing after "//", start your comments with capitals and add full-stops. I usually add a new line after a global declaration. The true in "$this->resetAfterTest(true);" is not necessary. Remove all whitespaces. General I prefer to use '&&' and '||' instead of 'and' and 'or', please see http://stackoverflow.com/questions/2803321/and-vs-as-operator . In your example it won't be an issue, just wanted to point it out fyi. I suggest creating an upgrade path for sites where there may been an issue where courses use the same idnumbers. That way you know the $DB->get_record call in your validation won't complain about there being more than one. I suggest doing something like adding (2) to the idnumber for the course that was added after the other, though there could potentially be numerous sites using the same idnumber, so keep this logic and add (3) and so on to alter them. Thanks for all your hard work so far, it's looking great, especially for someone new to Moodle! Regards, Mark
          Hide
          Marina Glancy added a comment -

          BTW we just decided between integrators that whether developer wants to use "and"/"or" or "$$"/"||" it's completely up to him. The only rule is not to mix them in one expression. Even though I personally prefer $$/|| too, we can't make it a policy.

          Re upgrade script - it's a good idea but needs a separate issue and a discussion.

          Show
          Marina Glancy added a comment - BTW we just decided between integrators that whether developer wants to use "and"/"or" or "$$"/"||" it's completely up to him. The only rule is not to mix them in one expression. Even though I personally prefer $$/|| too, we can't make it a policy. Re upgrade script - it's a good idea but needs a separate issue and a discussion.
          Hide
          Mark Nelson added a comment -

          Ok, sure. I will create a separate issue for the upgrade.

          Francis, just a note, when I say "remove all whitespaces" I mean "remove all trailing whitespaces". Obviously I don't want you to remove every whitespace and break the indentation.

          Show
          Mark Nelson added a comment - Ok, sure. I will create a separate issue for the upgrade. Francis, just a note, when I say "remove all whitespaces" I mean "remove all trailing whitespaces". Obviously I don't want you to remove every whitespace and break the indentation.
          Hide
          Mark Nelson added a comment -

          I created MDL-41616 to address the upgrade. Francis, you may want to take this one when you are done here, to increase your Moodle experience (and lower our workload ).

          Show
          Mark Nelson added a comment - I created MDL-41616 to address the upgrade. Francis, you may want to take this one when you are done here, to increase your Moodle experience (and lower our workload ).
          Hide
          Francis Devine added a comment -

          The original validation checks handled multiple courses with the same shortname/idnumber

          I changed them to checks that only handled one course.
          It didn't even occur to me that there could be sites out there that might have multiple courses with the same idnumber or shortname!

          I could probably revert to the old check and add one for idnumber in a similar style.

          I'll put this on a separate branch to single version checks.

          I'll get to work on fixing the issues you highlighted.
          I wasn't aware of the && vs and differences and I agree that &&/|| are much more sensible so I'll switch to using those as well.

          Show
          Francis Devine added a comment - The original validation checks handled multiple courses with the same shortname/idnumber I changed them to checks that only handled one course. It didn't even occur to me that there could be sites out there that might have multiple courses with the same idnumber or shortname! I could probably revert to the old check and add one for idnumber in a similar style. I'll put this on a separate branch to single version checks. I'll get to work on fixing the issues you highlighted. I wasn't aware of the && vs and differences and I agree that &&/|| are much more sensible so I'll switch to using those as well.
          Hide
          Mark Nelson added a comment -

          Hi Francis, your way is fine! The old way was pretty nasty and unnecessary. I just had a think about it and think you should replace the get_record call with a record_exists call to avoid any issues.

          Show
          Mark Nelson added a comment - Hi Francis, your way is fine! The old way was pretty nasty and unnecessary. I just had a think about it and think you should replace the get_record call with a record_exists call to avoid any issues.
          Hide
          Mark Nelson added a comment - - edited

          Actually, if you use record_exists you won't receive the course name to include in the error message. Just leave the get_record call there and add IGNORE_MULTIPLE as the fourth parameter.

          Show
          Mark Nelson added a comment - - edited Actually, if you use record_exists you won't receive the course name to include in the error message. Just leave the get_record call there and add IGNORE_MULTIPLE as the fourth parameter.
          Hide
          Mark Nelson added a comment -

          I suggest updating the testing instructions so that the user has a site that contains courses that share the same shortname and idnumber. Thanks!

          Show
          Mark Nelson added a comment - I suggest updating the testing instructions so that the user has a site that contains courses that share the same shortname and idnumber. Thanks!
          Hide
          Francis Devine added a comment -

          Alright, I've gone through the code and hopefully fixed all the stylistic issues in this commit here

          The second commit here adds the IGNORE_MULTIPLE param as per Marks suggestion. I also added a check for data['id'] being empty, which occurs when a course is being created.

          Since it only took a few seconds I did up checks in the old style which you can view here but Mark said they're pretty ugly

          Hopefully I managed to fix all the things Mark mentioned

          Show
          Francis Devine added a comment - Alright, I've gone through the code and hopefully fixed all the stylistic issues in this commit here The second commit here adds the IGNORE_MULTIPLE param as per Marks suggestion. I also added a check for data ['id'] being empty, which occurs when a course is being created. Since it only took a few seconds I did up checks in the old style which you can view here but Mark said they're pretty ugly Hopefully I managed to fix all the things Mark mentioned
          Hide
          Mark Nelson added a comment -

          The code isn't ugly per say, just I find the solution unnecessarily cumbersome as there should never be more than one course using a shortname/idnumber.

          Show
          Mark Nelson added a comment - The code isn't ugly per say, just I find the solution unnecessarily cumbersome as there should never be more than one course using a shortname/idnumber.
          Hide
          Francis Devine added a comment -

          I've just realised that I've forgotten to add the bug name to the start of the commit logs again

          Apologies!

          If you want I can rebase it all into one commit and fix the name.

          Show
          Francis Devine added a comment - I've just realised that I've forgotten to add the bug name to the start of the commit logs again Apologies! If you want I can rebase it all into one commit and fix the name.
          Hide
          Mark Nelson added a comment -

          Hi Francis, I have taken your commits and squashed them into one readable one. I will also backport this once I review it again.

          When creating commits you should always start with the MDL tracker number as it allows people to easily search through the git history, users know where to find more information on the commit etc etc. I also squashed it into one commit as the added commits were very trivial (ie. removing trailing spaces).

          Show
          Mark Nelson added a comment - Hi Francis, I have taken your commits and squashed them into one readable one. I will also backport this once I review it again. When creating commits you should always start with the MDL tracker number as it allows people to easily search through the git history, users know where to find more information on the commit etc etc. I also squashed it into one commit as the added commits were very trivial (ie. removing trailing spaces).
          Hide
          Mark Nelson added a comment -

          Whoops, did that all before I read your message. Didn't want to keep pestering you to fix trivial things as the patch is good to go.

          Show
          Mark Nelson added a comment - Whoops, did that all before I read your message. Didn't want to keep pestering you to fix trivial things as the patch is good to go.
          Hide
          Mark Nelson added a comment -

          Hi Francis, I will backport this and push for integration tomorrow (if you are fine with the commit message I used for you on your behalf). Thanks a lot for your work! It is much appreciated.

          Show
          Mark Nelson added a comment - Hi Francis, I will backport this and push for integration tomorrow (if you are fine with the commit message I used for you on your behalf). Thanks a lot for your work! It is much appreciated.
          Hide
          Francis Devine added a comment - - edited

          That commit message looks good to me.
          e: I was going to squash them up myself, I was just a bit concerned about rewriting public history when other people might have pulled my branch

          Thanks for the peer review, once this gets integrated, I'll have a look at MDL-41616 and make a start to getting something workable for that.

          Marina, thanks once again for your help!

          Show
          Francis Devine added a comment - - edited That commit message looks good to me. e: I was going to squash them up myself, I was just a bit concerned about rewriting public history when other people might have pulled my branch Thanks for the peer review, once this gets integrated, I'll have a look at MDL-41616 and make a start to getting something workable for that. Marina, thanks once again for your help!
          Hide
          Marina Glancy added a comment -

          Great job guys, thanks.

          One concern about current idnumber validation:
          Imagine website that already has course idnumber duplication, teacher edits some field in course edit form (for example groupmode or something even less significant) and he can't save the changes because of failing idnumber validation. This is not nice. How about performing this check only if

          if (!empty($data['idnumber']) && $this->course->idnumber != $data['idnumber']) {
              // ... Make sure no other course uses this idnumber
          }
          

          Changes in shortname validation are fine, it must be unique in DB already.

          And another comment: I'm afraid that current validation script will have warnings if teacher does not have capability to change shortname/idnumber because you've removed !empty($data['shortname'])

          Show
          Marina Glancy added a comment - Great job guys, thanks. One concern about current idnumber validation: Imagine website that already has course idnumber duplication, teacher edits some field in course edit form (for example groupmode or something even less significant) and he can't save the changes because of failing idnumber validation. This is not nice. How about performing this check only if if (!empty($data['idnumber']) && $ this ->course->idnumber != $data['idnumber']) { // ... Make sure no other course uses this idnumber } Changes in shortname validation are fine, it must be unique in DB already. And another comment: I'm afraid that current validation script will have warnings if teacher does not have capability to change shortname/idnumber because you've removed !empty($data ['shortname'] )
          Hide
          Marina Glancy added a comment -

          BTW Francis, please make sure that you have set the "Debug messages" = "Developer" and "Display debug messages" = "Yes" in
          Site Administration > Development > Debugging

          Show
          Marina Glancy added a comment - BTW Francis, please make sure that you have set the "Debug messages" = "Developer" and "Display debug messages" = "Yes" in Site Administration > Development > Debugging
          Hide
          Mark Nelson added a comment -

          Hi Marina,

          1) Don't we want user to fix their broken sites? I think it's fine if we leave it as it is, that way people can fix their sites.
          2) I do not see where Francis has removed !empty($data['shortname']). I tested this as a teacher who did not have the permission to change the shortname and it worked as expected. The shortname can not be empty, so the check is not necessary.

          Regards,

          Mark

          Show
          Mark Nelson added a comment - Hi Marina, 1) Don't we want user to fix their broken sites? I think it's fine if we leave it as it is, that way people can fix their sites. 2) I do not see where Francis has removed !empty($data ['shortname'] ). I tested this as a teacher who did not have the permission to change the shortname and it worked as expected. The shortname can not be empty, so the check is not necessary. Regards, Mark
          Hide
          Mark Nelson added a comment -

          I spoke to Dan and he agrees with you. I will add a commit on top of this one.

          Show
          Mark Nelson added a comment - I spoke to Dan and he agrees with you. I will add a commit on top of this one.
          Hide
          Mark Nelson added a comment -

          Ok, I have backported this. Note, the update_courses webservice function was only introduced in 2.5, so is missing in the 2.4 diff.

          Show
          Mark Nelson added a comment - Ok, I have backported this. Note, the update_courses webservice function was only introduced in 2.5, so is missing in the 2.4 diff.
          Hide
          Mark Nelson added a comment -

          Hey Marina, assigning to myself as I will be fixing any issues that arise from the code in integration.

          Show
          Mark Nelson added a comment - Hey Marina, assigning to myself as I will be fixing any issues that arise from the code in integration.
          Hide
          Sam Hemelryk added a comment -

          Thanks Mark - this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Mark - this has been integrated now.
          Hide
          Sam Hemelryk added a comment -

          This has caused unit tests to fail, the string courseidnumbertaken only exists on the master branch.

          Show
          Sam Hemelryk added a comment - This has caused unit tests to fail, the string courseidnumbertaken only exists on the master branch.
          Hide
          Francis Devine added a comment -

          Mark, my apologies, I forgot to point out that the courseidnumbertaken string is part of a fix pushed in MDL-41256, when backporting you would have to use idnumbertaken (which takes no arguments, so the error throwing code will have to be updated to take this into account)

          Show
          Francis Devine added a comment - Mark, my apologies, I forgot to point out that the courseidnumbertaken string is part of a fix pushed in MDL-41256 , when backporting you would have to use idnumbertaken (which takes no arguments, so the error throwing code will have to be updated to take this into account)
          Hide
          Marina Glancy added a comment -

          Francis, you have actually pointed it out above, it is just Mark who forgot to read

          Show
          Marina Glancy added a comment - Francis, you have actually pointed it out above, it is just Mark who forgot to read
          Hide
          Sam Hemelryk added a comment -

          Yip Marina is right, you were onto it Francis.

          I've fixed it now by adding a commit for 24/25 to use the old strings.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Yip Marina is right, you were onto it Francis. I've fixed it now by adding a commit for 24/25 to use the old strings. Many thanks Sam
          Hide
          Mark Nelson added a comment -

          Sorry guys, thought I checked that. Today just doesn't seem to be my day.

          Show
          Mark Nelson added a comment - Sorry guys, thought I checked that. Today just doesn't seem to be my day.
          Hide
          David Monllaó added a comment -

          Finally! it passes, tested in 24, 25 and master

          Show
          David Monllaó added a comment - Finally! it passes, tested in 24, 25 and master
          Hide
          Damyon Wiese added a comment -

          This issue along with 77 of it's friends has been sent upstream and released to the world.

          Thankyou for your contribution.

          Show
          Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.
          Hide
          Gregor McNish added a comment -

          This "fix" presents a problem for us

          https://tracker.moodle.org/browse/MDL-14084 chose not to fix it because of how people were using it.

          From the documents http://docs.moodle.org/25/en/Course_settings#Course_ID_number
          ID number is an alphanumeric field. It has several potential uses. Generally, it is not displayed to students. However, it can be used to match this course against an external system's ID, as your course catalogue ID or can be used in the certificate module as a printed field.

          The capability moodle/course:changeidnumber controls whether a user can edit the ID number.


          We use it for a reference to our schools enrolment database; often a single enrollable unit in the database is served by multiple moodle "courses", so the same idnumber is used in multiple shells.

          Looks like we'll have to do a local patch to revert this.

          Show
          Gregor McNish added a comment - This "fix" presents a problem for us https://tracker.moodle.org/browse/MDL-14084 chose not to fix it because of how people were using it. From the documents http://docs.moodle.org/25/en/Course_settings#Course_ID_number ID number is an alphanumeric field. It has several potential uses. Generally, it is not displayed to students. However, it can be used to match this course against an external system's ID, as your course catalogue ID or can be used in the certificate module as a printed field. The capability moodle/course:changeidnumber controls whether a user can edit the ID number. We use it for a reference to our schools enrolment database; often a single enrollable unit in the database is served by multiple moodle "courses", so the same idnumber is used in multiple shells. Looks like we'll have to do a local patch to revert this.
          Hide
          Marina Glancy added a comment -

          Gregor, the idnumber of courses and/or users is also used in some plugins responsible for enrolment or bulk update. If idnumber is present it must be unique.

          I would highly not recommend you to revert this patch but instead consider using the postfix in moodle idnumbers and compare the idnumbers without postfixes and/or just first several characters.

          Show
          Marina Glancy added a comment - Gregor, the idnumber of courses and/or users is also used in some plugins responsible for enrolment or bulk update. If idnumber is present it must be unique. I would highly not recommend you to revert this patch but instead consider using the postfix in moodle idnumbers and compare the idnumbers without postfixes and/or just first several characters.
          Hide
          Gregor McNish added a comment -

          Could you point me to code which uses course idnumber? It's null by default, so how could it be used by other plugins?

          My understanding was that both student and course idnumber fields were there for system integrations, not for moodle core to use.

          The id fields are of course different, being primary keys.

          Show
          Gregor McNish added a comment - Could you point me to code which uses course idnumber? It's null by default, so how could it be used by other plugins? My understanding was that both student and course idnumber fields were there for system integrations, not for moodle core to use. The id fields are of course different, being primary keys.
          Hide
          Marina Glancy added a comment -

          ones I know:

          enrol/flatfile/lib.php
          enrol/ldap/
          admin/tool/uploadcourse/classes/course.php
          Enrolment in generator that is used in behat tests identifies courses by idnumber as well.

          I'm sure there are much more, grepping by idnumber gives lots of results. From the quick glance I can see it in grades import code and in externallib

          Show
          Marina Glancy added a comment - ones I know: enrol/flatfile/lib.php enrol/ldap/ admin/tool/uploadcourse/classes/course.php Enrolment in generator that is used in behat tests identifies courses by idnumber as well. I'm sure there are much more, grepping by idnumber gives lots of results. From the quick glance I can see it in grades import code and in externallib
          Hide
          Gregor McNish added a comment -

          Thanks for that.

          It shouldn't be too much trouble to go the postfix route. Thanks.

          Show
          Gregor McNish added a comment - Thanks for that. It shouldn't be too much trouble to go the postfix route. Thanks.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: