Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Enrolments
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Best if executed in some stable branch (20, 21)

      1) Under current moodle.git

      • Import one IMS Enterprise file (see last comments in the issue to know which file is ok for our testing purposes)
      • TEST: Verify the process crashes

      2) Under current integration.git

      • Import the same IMS Enterprise file
      • TEST: Process ends without error and data is imported accordingly
      Show
      Best if executed in some stable branch (20, 21) 1) Under current moodle.git Import one IMS Enterprise file (see last comments in the issue to know which file is ok for our testing purposes) TEST: Verify the process crashes 2) Under current integration.git Import the same IMS Enterprise file TEST: Process ends without error and data is imported accordingly
    • Workaround:
      Hide

      not use <extension><cohort> element or correct the line #648 of the file /enrol/imsenterprise/lib.php like that :

      before :
      if($groupid = $DB->get_field('groups', 'id', 'name', $member->groupname, array('courseid'=>$ship->courseid))){

      after:
      if($groupid = $DB->get_field('groups', 'id', array('courseid'=>$ship->courseid, 'name'=>$member->groupname))){

      Show
      not use <extension><cohort> element or correct the line #648 of the file /enrol/imsenterprise/lib.php like that : before : if($groupid = $DB->get_field('groups', 'id', 'name', $member->groupname, array('courseid'=>$ship->courseid))){ after: if($groupid = $DB->get_field('groups', 'id', array('courseid'=>$ship->courseid, 'name'=>$member->groupname))){
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      master_MDL-28962

      Description

      Our IMS Enterprise XML input file stopped working after upgrading to Moodle 2.0. The plugin crash when importing the IMS Enterprise XML file with the following reason : PHP catchable fatal error. Our XML file uses the extension cohort element in the memberships.

      Here is the line that cause the file /enrol/imsenterprise/lib.php to crash.

      On line #648 :

      if($groupid = $DB->get_field('groups', 'id', 'name', $member->groupname, array('courseid'=>$ship->courseid))){

      I had to change to the following :

      if($groupid = $DB->get_field('groups', 'id', array('courseid'=>$ship->courseid, 'name'=>$member->groupname))){

      All other calls to this function uses 3 parameters. I guest this caused by the changes made to the API between version 1.x to 2.x because it's the same thing with Moodle 2.1.x.

      Now the import works and the cohorts (groups) too.

        Gliffy Diagrams

          Attachments

          1. ims_code1.png
            ims_code1.png
            79 kB
          2. ims_code2.png
            ims_code2.png
            78 kB
          3. ims_debug.png
            ims_debug.png
            65 kB
          4. ims_debug2.png
            ims_debug2.png
            52 kB
          5. ims_enrolled.png
            ims_enrolled.png
            72 kB
          6. imsenterprise-enrol_2.xml
            1 kB
          7. imsenterprise-enrol.xml
            3 kB

            Issue Links

              Activity

              Hide
              danmarsden Dan Marsden added a comment -

              Note to integrator - this patch will cherry pick from master onto 2.1 and 2.0 if that's easier.

              Show
              danmarsden Dan Marsden added a comment - Note to integrator - this patch will cherry pick from master onto 2.1 and 2.0 if that's easier.
              Hide
              stronk7 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
              stronk7 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
              danmarsden Dan Marsden added a comment -

              rebased - thanks.

              Show
              danmarsden Dan Marsden added a comment - rebased - thanks.
              Hide
              abarbary Adam Barbary added a comment -
              Show
              abarbary Adam Barbary added a comment - This is a duplicate of http://tracker.moodle.org/browse/MDL-26022
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated, thanks!

              Note I've added one extra commit to fix whitespace... plz, review that next time.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note I've added one extra commit to fix whitespace... plz, review that next time.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Dan, Patrick, Adam, could you point us to some example IMS Enterprise file to test with, able to reproduce the crash?

              I've found http://wiki.cetis.ac.uk/IMS_Enterprise_Example but not sure if that's ok for testing purposes, if you can confirm it or point to a valid one.. it will be really welcome.

              TIA!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Dan, Patrick, Adam, could you point us to some example IMS Enterprise file to test with, able to reproduce the crash? I've found http://wiki.cetis.ac.uk/IMS_Enterprise_Example but not sure if that's ok for testing purposes, if you can confirm it or point to a valid one.. it will be really welcome. TIA!
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Eloy,

              I don't have one but I would have thought it was an obvious one to test - the old code clearly isn't valid use of Moodle 2.0 dml syntax.

              also - if you look at the old 1.9 code:
              http://git.moodle.org/gw?p=moodle.git;a=blob;f=enrol/imsenterprise/enrol.php;h=35b4e1529b604632f317c3bc4798c7442b5846bd;hb=MOODLE_19_STABLE

              you'll see this line:
              $groupid = get_field('groups', 'id', 'name', addslashes($member->groupname), 'courseid', $ship->courseid)){

              so although this can't be tested with a file - you should be able to verify that the fix brings in line with correct conversion from 1.9 ->2.0 code

              Show
              danmarsden Dan Marsden added a comment - Hi Eloy, I don't have one but I would have thought it was an obvious one to test - the old code clearly isn't valid use of Moodle 2.0 dml syntax. also - if you look at the old 1.9 code: http://git.moodle.org/gw?p=moodle.git;a=blob;f=enrol/imsenterprise/enrol.php;h=35b4e1529b604632f317c3bc4798c7442b5846bd;hb=MOODLE_19_STABLE you'll see this line: $groupid = get_field('groups', 'id', 'name', addslashes($member->groupname), 'courseid', $ship->courseid)){ so although this can't be tested with a file - you should be able to verify that the fix brings in line with correct conversion from 1.9 ->2.0 code
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              LOL, yes, it's an obvious one for developers (looking at code, the original query was obviously borked), and the new one is perfect, hence it was integrated.

              But ideally, testing should be independent of that, and performed in a pure functional way, checking that the wrong situation (the bug) has been annihilated. It's possible the code is missing one parenthesis or whatever has escaped to the integrator eagle-eye, so functional test is a must (that or unittest covering the bug are the only 2 valid alternatives).

              Personally, when I get my testing hat, I forget completely about code. Just try to follow and reproduce.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - LOL, yes, it's an obvious one for developers (looking at code, the original query was obviously borked), and the new one is perfect, hence it was integrated. But ideally, testing should be independent of that, and performed in a pure functional way, checking that the wrong situation (the bug) has been annihilated. It's possible the code is missing one parenthesis or whatever has escaped to the integrator eagle-eye, so functional test is a must (that or unittest covering the bug are the only 2 valid alternatives). Personally, when I get my testing hat, I forget completely about code. Just try to follow and reproduce. Ciao
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Attaching sample file

              Show
              rwijaya Rossiani Wijaya added a comment - Attaching sample file
              Hide
              rwijaya Rossiani Wijaya added a comment -

              I found a zip file from http://moodle.org/mod/forum/discuss.php?d=37291 and use it to test this issue.

              In order to test this, make sure:

              1. enable IMS Enterprise file plugins (admin > enrolments > IMS Enterprise file)
              2. place the sample file into moodledata folder
              3. Modified the setting for IMS plugins for: file location
              4. enable the following options: "Create user accounts for users not yet registered in Moodle", 'Use the "sourcedid" for a person's userid if the "userid" field is not found', 'Create new (hidden) courses if not found in Moodle', and 'Create new (hidden) course categories if not found in Moodle'
              5. save changes
              6. on the bottom of setting page, select 'perform an IMS Enterprise import right now'

              The above procedures work on my local machine and resulted on creation of new users and course to the site.

              I'm not quite sure if this is the correct way to test the issue. I spoke to Eloy earlier and he mentioned to add some debug to the ims enterprise importer just before and after the line that has changed. I might have to look at this again tomorrow.

              Eloy or Dan, please feel free to comment regarding my testing procedures.

              Thanks
              Rosie

              Show
              rwijaya Rossiani Wijaya added a comment - I found a zip file from http://moodle.org/mod/forum/discuss.php?d=37291 and use it to test this issue. In order to test this, make sure: enable IMS Enterprise file plugins (admin > enrolments > IMS Enterprise file) place the sample file into moodledata folder Modified the setting for IMS plugins for: file location enable the following options: "Create user accounts for users not yet registered in Moodle", 'Use the "sourcedid" for a person's userid if the "userid" field is not found', 'Create new (hidden) courses if not found in Moodle', and 'Create new (hidden) course categories if not found in Moodle' save changes on the bottom of setting page, select 'perform an IMS Enterprise import right now' The above procedures work on my local machine and resulted on creation of new users and course to the site. I'm not quite sure if this is the correct way to test the issue. I spoke to Eloy earlier and he mentioned to add some debug to the ims enterprise importer just before and after the line that has changed. I might have to look at this again tomorrow. Eloy or Dan, please feel free to comment regarding my testing procedures. Thanks Rosie
              Hide
              danmarsden Dan Marsden added a comment -

              looks fine to me - good work Rosie!

              Show
              danmarsden Dan Marsden added a comment - looks fine to me - good work Rosie!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Ho Rossie, the debugging alternative was to be 100% sure that the fixed line was being executed while processing the example IMS file. Something like "echo here we go;". If you get it in the import process and no SQL problem happens (I'm 99.9% sure won't happen), then this can be considered passed.

              Alternatively you can also enable $DB->debug(true) and disable later and see if the SQL being executed is the expected one.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Ho Rossie, the debugging alternative was to be 100% sure that the fixed line was being executed while processing the example IMS file. Something like "echo here we go;". If you get it in the import process and no SQL problem happens (I'm 99.9% sure won't happen), then this can be considered passed. Alternatively you can also enable $DB->debug(true) and disable later and see if the SQL being executed is the expected one. Ciao
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Attaching testing screenshots.

              Show
              rwijaya Rossiani Wijaya added a comment - Attaching testing screenshots.
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Attaching sample files.

              Show
              rwijaya Rossiani Wijaya added a comment - Attaching sample files.
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Hi Eloy,

              I re-tested this issue by modifying the sample file from yesterday and adding some debug message to the code.

              The patch for this issue seems to work as it should be.

              I'm marking this issue as passed.

              Show
              rwijaya Rossiani Wijaya added a comment - Hi Eloy, I re-tested this issue by modifying the sample file from yesterday and adding some debug message to the code. The patch for this issue seems to work as it should be. I'm marking this issue as passed.
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Test passed.

              Show
              rwijaya Rossiani Wijaya added a comment - Test passed.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.
              Hide
              jens Jens Gammelgaard added a comment -

              Hello,

              Should Moodle 2.3 also be able to use IMS Enterprise to enroll users with their cohorts and groups?

              Could you maybe even share a demo IMS.xml file as an example for Moodle 2.3

              Thanks.

              Show
              jens Jens Gammelgaard added a comment - Hello, Should Moodle 2.3 also be able to use IMS Enterprise to enroll users with their cohorts and groups? Could you maybe even share a demo IMS.xml file as an example for Moodle 2.3 Thanks.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Oct/11