Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3, 2.4.4, 2.5, 2.5.1
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      PREREQUSITE: your moodle configured to send email.

      1. Download the attached IMS enterprise enrolment file and put it in your moodledata directory
      2. Go to Home / Site administration / Plugins / Enrolments
      3. Enable the IMS enterprise enrolment plugin
      4. In the settings, provide the path to the file you uploaded in File location
      5. Tick Notify admin by email
      6. Save settings
      7. Go to the link 'perform an IMS Enterprise import right now'
      8. VERIFY: the IMS enrolment works ok
      9. VERIFY: no debugging is experienced
      10. VERIFY: you receive a message about the enrollment by email
      11. Go to the link 'perform an IMS Enterprise import right now'
      12. VERIFY: the IMS enrollment is skipped because the file hasn't changed
      13. VERIFY: no debugging is experienced
      14. Rename the ims enrollment file
      15. Go to the link 'perform an IMS Enterprise import right now'
      16. VERIFY: the IMS enrollment is skipped because the file isn't there
      17. VERIFY: no debugging is experienced
      18. VERIFY: no message about the enrolment is received.
      Show
      PREREQUSITE: your moodle configured to send email. Download the attached IMS enterprise enrolment file and put it in your moodledata directory Go to Home / Site administration / Plugins / Enrolments Enable the IMS enterprise enrolment plugin In the settings, provide the path to the file you uploaded in File location Tick Notify admin by email Save settings Go to the link 'perform an IMS Enterprise import right now' VERIFY: the IMS enrolment works ok VERIFY: no debugging is experienced VERIFY: you receive a message about the enrollment by email Go to the link 'perform an IMS Enterprise import right now' VERIFY: the IMS enrollment is skipped because the file hasn't changed VERIFY: no debugging is experienced Rename the ims enrollment file Go to the link 'perform an IMS Enterprise import right now' VERIFY: the IMS enrollment is skipped because the file isn't there VERIFY: no debugging is experienced VERIFY: no message about the enrolment is received.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-39653-master
    • Story Points (Obsolete):
      2
    • Sprint:
      BACKEND Sprint 5

      Description

      Moodle 2.4.3

      Line 220 of /enrol/imsenterprise/lib.php

      This IF block should NOT run if the file does not exists or if the file is not new. When the file does not exist or is not new, an error is thrown at line 220 because $timeelapsed is not set. This causes the entire Moodle cron job to fail.

      Suggest either moving that block within the IF block above, or changing to:

      if (!empty($mailadmins) && file_exists($filename) && $fileisnew) {

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Petr Skoda added a comment -

            Thanks for the report.

            Show
            Petr Skoda added a comment - Thanks for the report.
            Hide
            Matthew Orlando added a comment -

            Another option is to add this line rather than change the above line (put it inside the IF block):

            $timeelapsed = isset($timeelapsed) ? $timeelapsed : 0;

            Show
            Matthew Orlando added a comment - Another option is to add this line rather than change the above line (put it inside the IF block): $timeelapsed = isset($timeelapsed) ? $timeelapsed : 0;
            Hide
            Dan Poltawski added a comment -

            Thanks Matthew. I think that your first solution matches the intention of the code best and so i've put this up for peer review.

            There are more elegant solutions to this, but I think this fixes the problem for now.

            Show
            Dan Poltawski added a comment - Thanks Matthew. I think that your first solution matches the intention of the code best and so i've put this up for peer review. There are more elegant solutions to this, but I think this fixes the problem for now.
            Hide
            Frédéric Massart added a comment -

            Hi Dan,

            This would definitely work, however for a cleaner code, I would suggest no repeating the file_exists(), but setting $fileisnew to false before the first file_exists() if statement. Then you can just if ($mailadmin && $fileisnew).

            Though, I am curious as to know whether it would be a good idea to still send an email to admin when the file doesn't exist, or is not new. Perhaps the real solution is to set $timelapsed to '0' and let the mail being sent.

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Hi Dan, This would definitely work, however for a cleaner code, I would suggest no repeating the file_exists(), but setting $fileisnew to false before the first file_exists() if statement. Then you can just if ($mailadmin && $fileisnew). Though, I am curious as to know whether it would be a good idea to still send an email to admin when the file doesn't exist, or is not new. Perhaps the real solution is to set $timelapsed to '0' and let the mail being sent. Cheers, Fred
            Hide
            Dan Poltawski added a comment -

            The thing is, if you set timelapsed to 0 then you get an even more meaningless mail and you don't get any info about the failure.

            The real solution is to fix this properly to send the right messages depending on the status. But I agree with your $fileisnew suggestion so will do that. (Actually I was thinking of setting $emailadmin to false everywhere, which seemed nicer but got messy.

            Show
            Dan Poltawski added a comment - The thing is, if you set timelapsed to 0 then you get an even more meaningless mail and you don't get any info about the failure. The real solution is to fix this properly to send the right messages depending on the status. But I agree with your $fileisnew suggestion so will do that. (Actually I was thinking of setting $emailadmin to false everywhere, which seemed nicer but got messy.
            Hide
            Marina Glancy added a comment -

            Thanks Matthew and Dan, integrated in 2.4, 2.5 and master

            Show
            Marina Glancy added a comment - Thanks Matthew and Dan, integrated in 2.4, 2.5 and master
            Hide
            Andrew Nicols added a comment -

            Passing - all tests pass on all affected versions.

            Show
            Andrew Nicols added a comment - Passing - all tests pass on all affected versions.
            Hide
            Dan Poltawski added a comment -

            You did it!

            Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

            Show
            Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile