Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41196

Add phpmailer message sink to allow unit testing of email_to_user

    Details

      Description

      We have a message sink for items sent using the message framework, but not email_to_user

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              skodak Petr Skoda added a comment -

              Maybe we should fix/rework messaging instead to allow sending from those support addresses so tahat we do not need to do these direct mailing hacks.

              Show
              skodak Petr Skoda added a comment - Maybe we should fix/rework messaging instead to allow sending from those support addresses so tahat we do not need to do these direct mailing hacks.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Quite possibly, but I think that we still need this sink so that we can adequately unit test the message processors.

              Changing all existing uses of the email_to_user side of things is a much bigger job and we need to be able to unit test things in the mean time.

              Show
              dobedobedoh Andrew Nicols added a comment - Quite possibly, but I think that we still need this sink so that we can adequately unit test the message processors. Changing all existing uses of the email_to_user side of things is a much bigger job and we need to be able to unit test things in the mean time.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Rebased on latest weekly.

              Show
              dobedobedoh Andrew Nicols added a comment - Rebased on latest weekly.
              Hide
              skodak Petr Skoda added a comment -

              +1, submitting for integration, thanks

              Show
              skodak Petr Skoda added a comment - +1, submitting for integration, thanks
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Adding backport branches at Damyon's request in MDL-41258

              Show
              dobedobedoh Andrew Nicols added a comment - Adding backport branches at Damyon's request in MDL-41258
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Andrew I've integrated this now.
              I've also backported as I agree with Damyon that having this in stable branches is beneficial and safe.
              Many thanks
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Andrew I've integrated this now. I've also backported as I agree with Damyon that having this in stable branches is beneficial and safe. Many thanks Sam
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Hi Andrew,

              Just wanted to make a note for phpunit_util fatal error when running a cron job for MDL-41144.

              Fatal error: Class 'phpunit_util' not found in /integration/master/lib/phpmailer/moodle_phpmailer.php on line 138

              Thanks.

              Show
              rwijaya Rossiani Wijaya added a comment - Hi Andrew, Just wanted to make a note for phpunit_util fatal error when running a cron job for MDL-41144 . Fatal error: Class 'phpunit_util' not found in /integration/master/lib/phpmailer/moodle_phpmailer.php on line 138 Thanks.
              Hide
              skodak Petr Skoda added a comment -

              ah, right, this needs to be fixed by adding

              if (PHPUNIT_TEST AND phpunit_util::is_redirecting_messages()) {

              In any case integrators please stop backporting things like this to stable branches, this is definitely not bug and as you can see there is always a big risk of serious regressions. Also using your absurd peer review rules - I did the peer review in master only, you can not backport it to stable without peer review.

              Thanks very much Rossi!!!

              Show
              skodak Petr Skoda added a comment - ah, right, this needs to be fixed by adding if (PHPUNIT_TEST AND phpunit_util::is_redirecting_messages()) { In any case integrators please stop backporting things like this to stable branches, this is definitely not bug and as you can see there is always a big risk of serious regressions. Also using your absurd peer review rules - I did the peer review in master only, you can not backport it to stable without peer review. Thanks very much Rossi!!!
              Hide
              abgreeve Adrian Greeve added a comment -

              From Rossie's comments it looks like this needs to be looked at a little further.

              Show
              abgreeve Adrian Greeve added a comment - From Rossie's comments it looks like this needs to be looked at a little further.
              Hide
              damyon Damyon Wiese added a comment -

              Sam added patches for the cron fix suggested by Petr and I tested it on all branches (without patch - error, with patch no error). I used the test script from MDL-4908 to make my life easier. The fix has been integrated to all branches - sending back to testing.

              Show
              damyon Damyon Wiese added a comment - Sam added patches for the cron fix suggested by Petr and I tested it on all branches (without patch - error, with patch no error). I used the test script from MDL-4908 to make my life easier. The fix has been integrated to all branches - sending back to testing.
              Hide
              abgreeve Adrian Greeve added a comment -

              I ran through the test that Rossie had, as Damyon had already done MDL-4908.
              The cron ran fine and didn't display the same fatal error.
              Test passed.

              Show
              abgreeve Adrian Greeve added a comment - I ran through the test that Rossie had, as Damyon had already done MDL-4908 . The cron ran fine and didn't display the same fatal error. Test passed.
              Hide
              damyon Damyon Wiese added a comment -

              Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week.

              Hurray!

              Show
              damyon Damyon Wiese added a comment - Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week. Hurray!
              Hide
              ybozhko Yuliya Bozhko added a comment -

              Looking through the comments history, I think in moodle_phpmailer.php it should check not redirected messages, but redirected emails...

              if (PHPUNIT_TEST && phpunit_util::is_redirecting_phpmailer()) {

              Otherwise, if I redirect emails only, my message sink is always empty. Discovered while trying to write tests for MDL-41956.

              Show
              ybozhko Yuliya Bozhko added a comment - Looking through the comments history, I think in moodle_phpmailer.php it should check not redirected messages, but redirected emails... if (PHPUNIT_TEST && phpunit_util::is_redirecting_phpmailer()) { Otherwise, if I redirect emails only, my message sink is always empty. Discovered while trying to write tests for MDL-41956 .
              Hide
              skodak Petr Skoda added a comment -

              Ah, right, another example why not backport non-bugs, thanks, new issue created MDL-42054...

              Show
              skodak Petr Skoda added a comment - Ah, right, another example why not backport non-bugs, thanks, new issue created MDL-42054 ...
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This new facility needs documentation in phpunit dev docs (How to write unit tests and friends).

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This new facility needs documentation in phpunit dev docs (How to write unit tests and friends).

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    9/Sep/13