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

        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
            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