Moodle
  1. Moodle
  2. MDL-20306

idnumber in mdl_course_categories

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.5, 1.9.6, 2.0
    • Fix Version/s: 2.2
    • Component/s: Course
    • Labels:
      None
    • Testing Instructions:
      Hide

      Choose
      'Site administration'
      -> 'Courses'
      -> 'Add/edit courses'

      Select the 'Edit this category' link for any of the listed categories (e.g. Miscellaneous)

      • A Category ID number field is now available, complete with appropriate tooltip help.
      • The field now also exists in the database in the course_categories table.
      • Try both adding one idnumber and later modifying it. The idnumber is properly stored and viewed on re-edition.
      • Edit another category and try to insert one already used idnumber, message about id number already taken should be shown and prevent the offending idenumber to be saved.
      • Edit another category already having one valid idnumber and try to modify it to one already used idnumber, message about id - number already taken should be shown and prevent the offending idenumber to be saved.
      Show
      Choose 'Site administration' -> 'Courses' -> 'Add/edit courses' Select the 'Edit this category' link for any of the listed categories (e.g. Miscellaneous) A Category ID number field is now available, complete with appropriate tooltip help. The field now also exists in the database in the course_categories table. Try both adding one idnumber and later modifying it. The idnumber is properly stored and viewed on re-edition. Edit another category and try to insert one already used idnumber, message about id number already taken should be shown and prevent the offending idenumber to be saved. Edit another category already having one valid idnumber and try to modify it to one already used idnumber, message about id - number already taken should be shown and prevent the offending idenumber to be saved.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      5546

      Description

      Hi,

      Could an idnumber be added to the mdl_course_categories table? We, as a college, map our college structures to moodle as follows.

      faculty ----------------------- category level 1

      -> program------------- category level 2
      ----> subject -------- course

      We have a script system which matches subjectcode to mdl_course.idnumber. However, we have to use the name field in the program which is not at all ideal. There is a program code which we'd like to store in mdl_course_categories.idnumber.

      Could this be done, it seems a very simple change?

      Thanks.
      Gavin

        Issue Links

          Activity

          Hide
          Gavin Mccullagh added a comment -

          Is there any chance of someone taking a look at this?

          It seems like it would take very little time to add this feature?

          Show
          Gavin Mccullagh added a comment - Is there any chance of someone taking a look at this? It seems like it would take very little time to add this feature?
          Hide
          Ian Tasker added a comment -

          +1 for this.
          I only have this requirement for it in moodle 2.0.X

          The indumber field is there for external systems to use in user,course and cohort.
          This would be helpful to have if the external system managed the course structure.

          Show
          Ian Tasker added a comment - +1 for this. I only have this requirement for it in moodle 2.0.X The indumber field is there for external systems to use in user,course and cohort. This would be helpful to have if the external system managed the course structure.
          Hide
          Dan Poltawski added a comment -

          We've been working on a patch for this at LUNS - patch incoming

          Show
          Dan Poltawski added a comment - We've been working on a patch for this at LUNS - patch incoming
          Hide
          Dan Poltawski added a comment -

          Integrators: This patch can be cherry picked into 21_STABLE

          Show
          Dan Poltawski added a comment - Integrators: This patch can be cherry picked into 21_STABLE
          Hide
          Petr Škoda added a comment -

          Shouldn't there be some sort of duplicity validation? I thing all other idnumbers have it.

          Show
          Petr Škoda added a comment - Shouldn't there be some sort of duplicity validation? I thing all other idnumbers have it.
          Hide
          Dan Poltawski added a comment -

          Hi Petr,

          Good idea - updated with duplicate validation.

          Show
          Dan Poltawski added a comment - Hi Petr, Good idea - updated with duplicate validation.
          Hide
          Andrew Nicols added a comment -

          Hi Petr,
          I had considered this when I first wrote it, but decided against adding the unique constraint because there wasn't a DB constraint on the course or user tables.

          I think that it makes more sense to have the constraint and I've released a new branch which:

          • adds a database constraint ensuring an unique idnumber; and
          • adds a form validation ensuring that duplicates cannot exist on creation/update of categories in the interface.

          The new branch is MDL-20306-with_validation

          Show
          Andrew Nicols added a comment - Hi Petr, I had considered this when I first wrote it, but decided against adding the unique constraint because there wasn't a DB constraint on the course or user tables. I think that it makes more sense to have the constraint and I've released a new branch which: adds a database constraint ensuring an unique idnumber; and adds a form validation ensuring that duplicates cannot exist on creation/update of categories in the interface. The new branch is MDL-20306 -with_validation
          Hide
          Petr Škoda added a comment -

          The problem is that we can not make it unique in DB because the value is not required. In other places we do only duplicate test in forms validation, if existing duplicates are detected we do not force updates. The code has to deal with these special duplicate cases elsewhere.

          Show
          Petr Škoda added a comment - The problem is that we can not make it unique in DB because the value is not required. In other places we do only duplicate test in forms validation, if existing duplicates are detected we do not force updates. The code has to deal with these special duplicate cases elsewhere.
          Hide
          Dan Poltawski added a comment -

          Andrew has updated this again removing the DB constraint. We missed the empty case (thought the other fields didn't have a constraint because they might have duplicates and impossible to deal with).

          Hoping we can get this re-reviewed

          Show
          Dan Poltawski added a comment - Andrew has updated this again removing the DB constraint. We missed the empty case (thought the other fields didn't have a constraint because they might have duplicates and impossible to deal with). Hoping we can get this re-reviewed
          Hide
          Andrew Nicols added a comment -

          Fair point
          Dan pointed out that it's difficult to enforce old data with new constraints, but I hadn't considered the issue with empty values being considered duplicates
          New branch added with form validation, but not database constraint!

          MDL-20306-with_validation-nodbconstraint

          Show
          Andrew Nicols added a comment - Fair point Dan pointed out that it's difficult to enforce old data with new constraints, but I hadn't considered the issue with empty values being considered duplicates New branch added with form validation, but not database constraint! MDL-20306 -with_validation-nodbconstraint
          Hide
          Petr Škoda added a comment -

          I am proposing to wait with this issue until next week so that Eloy can review this change.

          my +1 but I do not want to decide this myself

          Thanks a lot for the patches!!

          Show
          Petr Škoda added a comment - I am proposing to wait with this issue until next week so that Eloy can review this change. my +1 but I do not want to decide this myself Thanks a lot for the patches!!
          Hide
          Dan Poltawski added a comment -

          thanks for the Review

          Show
          Dan Poltawski added a comment - thanks for the Review
          Hide
          Sam Hemelryk added a comment -

          Making Eloy the integrator on this issue so that he looks at it

          Show
          Sam Hemelryk added a comment - Making Eloy the integrator on this issue so that he looks at it
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week due to time constraints. Thanks for your support and patience!

          Sorry and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week due to time constraints. Thanks for your support and patience! Sorry and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, I think it's ok to integrated this but:

          1) Q: Does the validation work ok on category creation? Or only updating?
          2) Please use the XMLDB Editor for editing the install xml files and to get the correct PHP code to be put on upgrade.php (specially this last is incomplete/incorrect)
          3) I'd suggest you to wait until tomorrow/past-tomorrow to get upstream updated and then base your patch (versions and friends) on it.

          So, I'm rejecting this now aimed to get it added next week, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, I think it's ok to integrated this but: 1) Q: Does the validation work ok on category creation? Or only updating? 2) Please use the XMLDB Editor for editing the install xml files and to get the correct PHP code to be put on upgrade.php (specially this last is incomplete/incorrect) 3) I'd suggest you to wait until tomorrow/past-tomorrow to get upstream updated and then base your patch (versions and friends) on it. So, I'm rejecting this now aimed to get it added next week, thanks!
          Hide
          Andrew Nicols added a comment - - edited

          Hi Eloy,

          Thanks for the feedback.
          1) The validation does work for both creation and update
          2) Sorry I'm a CLI fan. I've done as you ask and used the editor to generate both the XML and upgrade.php code
          3) Past tomorrow now

          New branch name: MDL-20306-2
          Updated diff at: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=2a4912586a3879663c7c7e3c33dd7337a7e3e713

          Show
          Andrew Nicols added a comment - - edited Hi Eloy, Thanks for the feedback. 1) The validation does work for both creation and update 2) Sorry I'm a CLI fan. I've done as you ask and used the editor to generate both the XML and upgrade.php code 3) Past tomorrow now New branch name: MDL-20306 -2 Updated diff at: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=2a4912586a3879663c7c7e3c33dd7337a7e3e713
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho, just pre-looking to some issues (before start integrating them...):

          1) It seems that the unique constraint strikes back (install.xml). We cannot use it for NOT NULL columns at all, only for nullables. So perhaps it's better to take rid of it, as was commented above.

          2) Please amend the commit to use current versions & rebase if possible, that would make merging here easier.

          Will look to this tomorrow/past tomorrow, thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, just pre-looking to some issues (before start integrating them...): 1) It seems that the unique constraint strikes back (install.xml). We cannot use it for NOT NULL columns at all, only for nullables. So perhaps it's better to take rid of it, as was commented above. 2) Please amend the commit to use current versions & rebase if possible, that would make merging here easier. Will look to this tomorrow/past tomorrow, thanks! Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (reopening til fix for this arrives)

          Show
          Eloy Lafuente (stronk7) added a comment - (reopening til fix for this arrives)
          Show
          Andrew Nicols added a comment - New branch: MDL-20306 -5 Diff URL: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=0387db778865c569af0f02bbfabff28055ee9a9d
          Hide
          Andrew Nicols added a comment -

          I've updated the patch to remove the notnull constraint, and rebased on latest master (on Friday)

          Show
          Andrew Nicols added a comment - I've updated the patch to remove the notnull constraint, and rebased on latest master (on Friday)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note1: of course this will go to master only - no DB changes in stables, anybody disagrees?

          Note2: You edited it (install.xml) manually again, eh? hahaha. Yes I've your computer cracked :-P

          Note3: Regarding unique constraints / indexes, note they can be used if the column is defined as nullable, because null is allow to be "repeated" afaik. Anyway, I think we can live without it for now (as far as there aren't other methods to create course categories but the UI / manual one).

          I'll happy to integrate this to master of nobody has objections to Note1. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Note1: of course this will go to master only - no DB changes in stables, anybody disagrees? Note2: You edited it (install.xml) manually again, eh? hahaha. Yes I've your computer cracked :-P Note3: Regarding unique constraints / indexes, note they can be used if the column is defined as nullable, because null is allow to be "repeated" afaik. Anyway, I think we can live without it for now (as far as there aren't other methods to create course categories but the UI / manual one). I'll happy to integrate this to master of nobody has objections to Note1. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (slightly improved testing instructions)

          Show
          Eloy Lafuente (stronk7) added a comment - (slightly improved testing instructions)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oops, reopening once more... it seems the upgrade code has some minor mistakes:

          1) copy and pasted comment?
          2) incomplete xmldb_field specs (plz, plz, use the editor it will give you the correct PHP code to use too!
          3) Inconsistencies between the version in the "if" and the version in the upgrade_main_savepoint() call.

          Show
          Eloy Lafuente (stronk7) added a comment - Oops, reopening once more... it seems the upgrade code has some minor mistakes: 1) copy and pasted comment? 2) incomplete xmldb_field specs (plz, plz, use the editor it will give you the correct PHP code to use too! 3) Inconsistencies between the version in the "if" and the version in the upgrade_main_savepoint() call.
          Hide
          Andrew Nicols added a comment -

          Thanks for your patience Eloy.

          I did originally create the php upgrade code with the XMLDB editor so I don't know quite how I managed to break it there.

          To ensure that I've done it correctly, I've dropped the field, and re-created it from scratch so the install.xml file has an updated version too. I've copied the php code from the editor too and incremented last week's version number by .01.

          Hopefully I've got everything time!

          Show
          Andrew Nicols added a comment - Thanks for your patience Eloy. I did originally create the php upgrade code with the XMLDB editor so I don't know quite how I managed to break it there. To ensure that I've done it correctly, I've dropped the field, and re-created it from scratch so the install.xml file has an updated version too. I've copied the php code from the editor too and incremented last week's version number by .01. Hopefully I've got everything time!
          Hide
          Dan Poltawski added a comment -

          Integrators: we were just discussing what is the best thing to do with version numbers for you? Obviously you'll most often need to fix up the version number, but is there something you'd prefer us to do?

          Show
          Dan Poltawski added a comment - Integrators: we were just discussing what is the best thing to do with version numbers for you? Obviously you'll most often need to fix up the version number, but is there something you'd prefer us to do?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          we use to perform .xx increments usually only, not really important in master (where we can accept whole date changes) but critical for stables, where only those .xx digits are available between minor releases.

          So, I think you can try to use always .xx increments. If that leads to conflict with other issues in the same week, np, we'll sort them out on integration.

          Ciao

          PS: Thanks for the patch, hope we get it on board next week, yay!

          Show
          Eloy Lafuente (stronk7) added a comment - we use to perform .xx increments usually only, not really important in master (where we can accept whole date changes) but critical for stables, where only those .xx digits are available between minor releases. So, I think you can try to use always .xx increments. If that leads to conflict with other issues in the same week, np, we'll sort them out on integration. Ciao PS: Thanks for the patch, hope we get it on board next week, yay!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Andrew Nicols added a comment -

          Updated on latest master.
          Version numbers are now 2077700700.01

          Show
          Andrew Nicols added a comment - Updated on latest master. Version numbers are now 2077700700.01
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been integrated, thanks!

          Note I've added one extra commit, bumping the version and adding the missing call to add_field()... how the hell is your copy/paste working? Using Windows? LOL

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been integrated, thanks! Note I've added one extra commit, bumping the version and adding the missing call to add_field()... how the hell is your copy/paste working? Using Windows? LOL
          Hide
          Dan Poltawski added a comment -

          Heh, wow - we deserved to be bumped another week for that Eloy!

          Thanks for integrating!

          Show
          Dan Poltawski added a comment - Heh, wow - we deserved to be bumped another week for that Eloy! Thanks for integrating!
          Hide
          Andrew Nicols added a comment -

          Thank you Eloy! I must be losing my marbles or something. thank you for your patience

          Show
          Andrew Nicols added a comment - Thank you Eloy! I must be losing my marbles or something. thank you for your patience
          Hide
          Sam Hemelryk added a comment -

          Thanks guys - works perfectly

          Show
          Sam Hemelryk added a comment - Thanks guys - works perfectly
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work developing and testing this. It has been spread to cvs and git upstream repositories.

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work developing and testing this. It has been spread to cvs and git upstream repositories. Closing, ciao

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: