Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      18636

      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.

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          Dan Marsden added a comment -

          rebased - thanks.

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

          Integrated, thanks!

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

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note I've added one extra commit to fix whitespace... plz, review that next time.
          Hide
          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
          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
          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
          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
          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
          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
          Rossiani Wijaya added a comment -

          Attaching sample file

          Show
          Rossiani Wijaya added a comment - Attaching sample file
          Hide
          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
          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
          Dan Marsden added a comment -

          looks fine to me - good work Rosie!

          Show
          Dan Marsden added a comment - looks fine to me - good work Rosie!
          Hide
          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
          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
          Rossiani Wijaya added a comment -

          Attaching testing screenshots.

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

          Attaching sample files.

          Show
          Rossiani Wijaya added a comment - Attaching sample files.
          Hide
          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
          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
          Rossiani Wijaya added a comment -

          Test passed.

          Show
          Rossiani Wijaya added a comment - Test passed.
          Hide
          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
          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 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 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: