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

          Issue Links

            Activity

            Hide
            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
            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
            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
            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
            Andrew Nicols added a comment -

            Rebased on latest weekly.

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

            +1, submitting for integration, thanks

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

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

            Show
            Andrew Nicols added a comment - Adding backport branches at Damyon's request in MDL-41258
            Hide
            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
            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
            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
            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
            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
            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
            Adrian Greeve added a comment -

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

            Show
            Adrian Greeve added a comment - From Rossie's comments it looks like this needs to be looked at a little further.
            Hide
            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 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
            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
            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 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 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
            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
            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
            Petr Skoda added a comment -

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

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

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

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