Moodle
  1. Moodle
  2. MDL-28705

IMS Enterprise enrolment: Add configurable mapping between IMS course names and Moodle course names

    Details

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

      To test this issue you'll need a ims XML file with user/s, course/s and enrolment/s, there is one attachment, otherwise I think Ankit have a repository of files for this kind of uses. The interesting part is the course creation, the field used as main identifier of a course is enterprise->group->sourcedid->id

      If you use the attached ims.xml, it don't have a enterprise->group->description->full tag, you'll have to add it (just copy & paste another description child and replace the tag content) to also test this tag.

      1. Enable IMS enrolment type
        1. Go to Plugins -> Enrolments -> Manage enrol plugins
        2. Click 'Settings' on the IMS Enterprise row
        3. Add the path of the ims file and check the values 'createnewusers', 'createnewcourses' and 'createnewcategories', new you can leave the default values to the imscoursemap* settings
        4. Save changes
      2. Follow the link in the lower part of the IMS enrolment settings page 'perform an IMS Enterprise import right now' and ensure it finished without errors
      3. The course SHOULD be created with the shortname, fullname and summary values according to the specified mapping
      4. Change the imscoursemap* mapping, edit the XML file and change the enterprise->group->sourcedid->id value (to create another course) and execute again 'perform an IMS Enterprise import right now'
      5. The course SHOULD be created with the shortname, fullname and summary values according to the specified mapping
      6. Change again the imscoursemap* mapping to test all the description tag children tags (short, long and full), and check if the course creation respects that mapping
      Show
      To test this issue you'll need a ims XML file with user/s, course/s and enrolment/s, there is one attachment, otherwise I think Ankit have a repository of files for this kind of uses. The interesting part is the course creation, the field used as main identifier of a course is enterprise->group->sourcedid->id If you use the attached ims.xml, it don't have a enterprise->group->description->full tag, you'll have to add it (just copy & paste another description child and replace the tag content) to also test this tag. Enable IMS enrolment type Go to Plugins -> Enrolments -> Manage enrol plugins Click 'Settings' on the IMS Enterprise row Add the path of the ims file and check the values 'createnewusers', 'createnewcourses' and 'createnewcategories', new you can leave the default values to the imscoursemap* settings Save changes Follow the link in the lower part of the IMS enrolment settings page 'perform an IMS Enterprise import right now' and ensure it finished without errors The course SHOULD be created with the shortname, fullname and summary values according to the specified mapping Change the imscoursemap* mapping, edit the XML file and change the enterprise->group->sourcedid->id value (to create another course) and execute again 'perform an IMS Enterprise import right now' The course SHOULD be created with the shortname, fullname and summary values according to the specified mapping Change again the imscoursemap* mapping to test all the description tag children tags (short, long and full), and check if the course creation respects that mapping
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-28705_master
    • Rank:
      18365

      Description

      When using IMS Enrollment the mapping between IMS and Moodle course properties, like names, is hard coded. In our local Moodle I have altered these hard coded mappings for years to work with our IMS files. With Moodle 2.0 all of our other custom code is unnecessary except this, so I figured it was time to attempt to submit this as a feature request. Because my hard coded mapping may not be suitable to others (as the default is unsuitable to me), I have attempted to make the mapping configurable. I'm not very familiar with Moodle development. I hope that someone would look at my changes to see if I'm moving in the right direction and give me suggestions for improvement so that the code could one day be accepted into the stable tree.

      https://github.com/martinluther/moodle/compare/MOODLE_21_STABLE...namemapping

      1. ims.xml
        0.9 kB
        David Monllaó

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting this and providing a solution.

          Show
          Michael de Raadt added a comment - Thanks for suggesting this and providing a solution.
          Hide
          Michael de Raadt added a comment -

          Hi, Eugene.

          I've added you as a watcher. What do you think about this improvement?

          Show
          Michael de Raadt added a comment - Hi, Eugene. I've added you as a watcher. What do you think about this improvement?
          Hide
          David Monllaó added a comment -

          Hi,

          I've refined the provided patch; to resume the whole issue patch, there are 3 new settings to specify how to fill the course fields (shortname, fullname and summary) with the available IMS Enterprise description tags. The course summary has an option to leave it empty (is an optional field) but course shortname and fullname values should be obtained from a present IMS group description tag (sourceid, short, long or full) If the selected IMS description tags doesn't exists in the IMS file it fallsback on the sourceid and adds a log message about it.

          The default values are:

          • course shortname: description->sourceid IMS description tag
          • course fullname: description->short IMS description tag
          • course summary: empty

          I've added new language strings to explain the new settings, but as you'll know, my English skills are not as good as they could be, so I'll be glad is someone could check that I'm not saying "funny" things

          There are new settings so probably it will require a new enrol_imsenterprise version.

          Show
          David Monllaó added a comment - Hi, I've refined the provided patch; to resume the whole issue patch, there are 3 new settings to specify how to fill the course fields (shortname, fullname and summary) with the available IMS Enterprise description tags. The course summary has an option to leave it empty (is an optional field) but course shortname and fullname values should be obtained from a present IMS group description tag (sourceid, short, long or full) If the selected IMS description tags doesn't exists in the IMS file it fallsback on the sourceid and adds a log message about it. The default values are: course shortname: description->sourceid IMS description tag course fullname: description->short IMS description tag course summary: empty I've added new language strings to explain the new settings, but as you'll know, my English skills are not as good as they could be, so I'll be glad is someone could check that I'm not saying "funny" things There are new settings so probably it will require a new enrol_imsenterprise version.
          Hide
          Andrew Davis added a comment - - edited

          Hi. Your English is excellent.

          The class imsenterprise_courses's php docs need more information. http://docs.moodle.org/dev/Coding_style#Classes_3

          "// TODO Use create_course instead of insert_record and blocks_add_defualt_course_blocks"
          If there is more work to be done create an MDL and put the issue number brackets at the end of the todo comment. That way our future work is all in tracker and not hidden in the source code. Also, anyone finding the todo in the code won't have to go through the hassle of looking for an MDL.

          Finally, this issue is missing test instructions. Make sure to include everything working and one or more scenarios where it will fail due to misconfiguration.

          Show
          Andrew Davis added a comment - - edited Hi. Your English is excellent. The class imsenterprise_courses's php docs need more information. http://docs.moodle.org/dev/Coding_style#Classes_3 "// TODO Use create_course instead of insert_record and blocks_add_defualt_course_blocks" If there is more work to be done create an MDL and put the issue number brackets at the end of the todo comment. That way our future work is all in tracker and not hidden in the source code. Also, anyone finding the todo in the code won't have to go through the hassle of looking for an MDL. Finally, this issue is missing test instructions. Make sure to include everything working and one or more scenarios where it will fail due to misconfiguration.
          Hide
          David Monllaó added a comment -

          Thanks for the comments Andrew, I've added the testing instructions and the class description. About the TODO, I have removed it because is seems that there isn't any problem with the course creation. Submitting for peer review

          Show
          David Monllaó added a comment - Thanks for the comments Andrew, I've added the testing instructions and the class description. About the TODO, I have removed it because is seems that there isn't any problem with the course creation. Submitting for peer review
          Hide
          Andrew Davis added a comment -

          I think you're ok now. Go for integration.

          Show
          Andrew Davis added a comment - I think you're ok now. Go for integration.
          Hide
          David Monllaó added a comment -

          Thanks for the review and re-review Andrew

          Show
          David Monllaó added a comment - Thanks for the review and re-review Andrew
          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 -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Aparup Banerjee added a comment -

          Hi David,

          The changes here seem ok but i couldn't help wondering with the xml file 'test' resource: if we can build up some unit tests here under a /tests directory to test (at least) the functionality being added from this issue. I think this will be a step towards better reliability here.

          you can see enrol/manual/tests* for some examples there if needed. I'm reopening this since this will need some more time for building some unit tests here.

          cheers

          Show
          Aparup Banerjee added a comment - Hi David, The changes here seem ok but i couldn't help wondering with the xml file 'test' resource: if we can build up some unit tests here under a /tests directory to test (at least) the functionality being added from this issue. I think this will be a step towards better reliability here. you can see enrol/manual/tests* for some examples there if needed. I'm reopening this since this will need some more time for building some unit tests here. cheers
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          David Monllaó added a comment -

          Hi,

          I've added a test case with general courses and users tests for enrol_imsenterprise and a specific test for the feature added in this issue (test_courses_attrmapping).

          Show
          David Monllaó added a comment - Hi, I've added a test case with general courses and users tests for enrol_imsenterprise and a specific test for the feature added in this issue (test_courses_attrmapping).
          Hide
          Andrew Davis added a comment -

          [Y] Syntax
          [Y] Output
          [] Whitespace
          [Y] Language
          [NA] Databases
          [Y] Testing
          [Y] Security
          [NA] Documentation
          [Y] Git
          [Y] Sanity check

          Hi David. Sorry for the delay. This will need rebasing due to how long its sat but feel free to put this up for integration again once thats done.

          Show
          Andrew Davis added a comment - [Y] Syntax [Y] Output [] Whitespace [Y] Language [NA] Databases [Y] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check Hi David. Sorry for the delay. This will need rebasing due to how long its sat but feel free to put this up for integration again once thats done.
          Hide
          David Monllaó added a comment -

          Thanks Andrew, branch rebased against last stable master, pushing for integration

          Show
          David Monllaó added a comment - Thanks Andrew, branch rebased against last stable master, pushing for integration
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (labeling as integration_held as far as this is a candidate for master only)

          Show
          Eloy Lafuente (stronk7) added a comment - (labeling as integration_held as far as this is a candidate for master only)
          Hide
          Dan Poltawski added a comment -

          Integrated to master only. Thanks David/Aaron

          Show
          Dan Poltawski added a comment - Integrated to master only. Thanks David/Aaron
          Hide
          Dan Poltawski added a comment -

          This is failing on the CI serveR:
          Stacktrace

          enrol_imsenterprise_testcase::test_courses_attrmapping
          dml_read_exception: Error reading from database (ERROR: invalid input syntax for integer: "DEFAULT CATNAME"
          SELECT name FROM p_course_categories WHERE id = $1
          [array (
          0 => 'DEFAULT CATNAME',
          )])

          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:426
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/pgsql_native_moodle_database.php:248
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/pgsql_native_moodle_database.php:753
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1382
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1455
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1436
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1415
          /Users/Shared/Jenkins/Home/git_repositories/master/enrol/imsenterprise/tests/imsenterprise_test.php:281
          /Users/Shared/Jenkins/Home/git_repositories/master/enrol/imsenterprise/tests/imsenterprise_test.php:175
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76

          enrol_imsenterprise_testcase::test_courses_add
          dml_read_exception: Error reading from database (ERROR: invalid input syntax for integer: "DEFAULT CATNAME"
          SELECT name FROM p_course_categories WHERE id = $1
          [array (
          0 => 'DEFAULT CATNAME',
          )])

          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:426
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/pgsql_native_moodle_database.php:248
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/pgsql_native_moodle_database.php:753
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1382
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1455
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1436
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1415
          /Users/Shared/Jenkins/Home/git_repositories/master/enrol/imsenterprise/tests/imsenterprise_test.php:281
          /Users/Shared/Jenkins/Home/git_repositories/master/enrol/imsenterprise/tests/imsenterprise_test.php:150
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76

          Show
          Dan Poltawski added a comment - This is failing on the CI serveR: Stacktrace enrol_imsenterprise_testcase::test_courses_attrmapping dml_read_exception: Error reading from database (ERROR: invalid input syntax for integer: "DEFAULT CATNAME" SELECT name FROM p_course_categories WHERE id = $1 [array ( 0 => 'DEFAULT CATNAME', )]) /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:426 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/pgsql_native_moodle_database.php:248 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/pgsql_native_moodle_database.php:753 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1382 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1455 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1436 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1415 /Users/Shared/Jenkins/Home/git_repositories/master/enrol/imsenterprise/tests/imsenterprise_test.php:281 /Users/Shared/Jenkins/Home/git_repositories/master/enrol/imsenterprise/tests/imsenterprise_test.php:175 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76 enrol_imsenterprise_testcase::test_courses_add dml_read_exception: Error reading from database (ERROR: invalid input syntax for integer: "DEFAULT CATNAME" SELECT name FROM p_course_categories WHERE id = $1 [array ( 0 => 'DEFAULT CATNAME', )]) /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:426 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/pgsql_native_moodle_database.php:248 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/pgsql_native_moodle_database.php:753 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1382 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1455 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1436 /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:1415 /Users/Shared/Jenkins/Home/git_repositories/master/enrol/imsenterprise/tests/imsenterprise_test.php:281 /Users/Shared/Jenkins/Home/git_repositories/master/enrol/imsenterprise/tests/imsenterprise_test.php:150 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76
          Hide
          David Monllaó added a comment -

          Thanks Dan, I've added a commit on top of the branch:

          Show
          David Monllaó added a comment - Thanks Dan, I've added a commit on top of the branch: git://github.com/dmonllao/moodle.git 7cd5340f05a5a0f6c2e8336a4deaa40cf6fbb522 https://github.com/dmonllao/moodle/commit/7cd5340f05a5a0f6c2e8336a4deaa40cf6fbb522
          Hide
          Dan Poltawski added a comment -

          Thanks David, integrated the fix

          Show
          Dan Poltawski added a comment - Thanks David, integrated the fix
          Hide
          Mark Nelson added a comment -

          After discussing with David and discovering what mappings in Moodle corresponded with the fields in the XML file I am passing.

          Show
          Mark Nelson added a comment - After discussing with David and discovering what mappings in Moodle corresponded with the fields in the XML file I am passing.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!
          Hide
          Mary Cooch added a comment -

          (Housekeeping) I'm just doing through the docs_required labels and as I don't really understand this I wonder if anyone with more knowledge can either add something to the docs on this http://docs.moodle.org/25/en/IMS_Enterprise or confirm that it doesn't need extra documentation?

          Show
          Mary Cooch added a comment - (Housekeeping) I'm just doing through the docs_required labels and as I don't really understand this I wonder if anyone with more knowledge can either add something to the docs on this http://docs.moodle.org/25/en/IMS_Enterprise or confirm that it doesn't need extra documentation?
          Hide
          David Monllaó added a comment -
          Show
          David Monllaó added a comment - Sorry Mary, I forgot about it, I've added a info line: http://docs.moodle.org/25/en/index.php?title=IMS_Enterprise&action=historysubmit&diff=105364&oldid=104384
          Hide
          Mary Cooch added a comment -

          Thanks!

          Show
          Mary Cooch added a comment - Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: