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

Enable configuration so that mail comes only from noreply address

    Details

    • Testing Instructions:
      Hide

      To test the patch:

      • Run the unit test test_email_to_user in moodlelib_test.php
      • Confirm that there are no failures

      To test the issue manually:

      1. Do something in Moodle which generates an email not from the noreply address (e.g. submit an assignment)
      2. Confirm that the email does not come from the noreply address
      3. In config.php, set $CFG->emailonlyfromnoreplyaddress = true
      4. Repeat the action from step 1 and confirm that the email now comes from the noreply address
      Show
      To test the patch: Run the unit test test_email_to_user in moodlelib_test.php Confirm that there are no failures To test the issue manually: Do something in Moodle which generates an email not from the noreply address (e.g. submit an assignment) Confirm that the email does not come from the noreply address In config.php, set $CFG->emailonlyfromnoreplyaddress = true Repeat the action from step 1 and confirm that the email now comes from the noreply address
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull 2.7 Branch:
      MDL-43669-m27
    • Pull Master Branch:

      Description

      It would be useful to be able to prevent Moodle sending emails from users own email addresses and instead to send everything from the server's configured noreply address.

      One scenario where this would make sense is in a large institution which has a centralised mail relay hub which controls the outward email from all of the institution's web applications. Normally a service like this will not be configured to relay emails with arbitrary "from" addresses as this is often seen as a security risk.

      In addition, it could be argued that sending an email which appears to come from another email service is "spoofing" and that institutions should not do this without express permission of the user. (For the record, I don't necessarily agree with this statement but I know people who would argue along these lines!)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            maherne Michael Aherne added a comment -

            I've attached a patch that does this. I'm not sure if the setting should be configurable in the user interface, or if a config.php setting is enough.

            Show
            maherne Michael Aherne added a comment - I've attached a patch that does this. I'm not sure if the setting should be configurable in the user interface, or if a config.php setting is enough.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43669

            • Remote repository: git://github.com/micaherne/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43669 Remote repository: git://github.com/micaherne/moodle.git Remote branch MDL-43669 -m to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/541 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/541/artifact/work/smurf.html
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for sharing that, Michael.

            Show
            salvetore Michael de Raadt added a comment - Thanks for sharing that, Michael.
            Hide
            andyjdavis Andrew Davis added a comment -

            Hi Michael (Aherne, not de Raadt), could you add some testing instructions to this issue?

            Show
            andyjdavis Andrew Davis added a comment - Hi Michael (Aherne, not de Raadt), could you add some testing instructions to this issue?
            Hide
            maherne Michael Aherne added a comment -

            Hi Andrew, I've added some manual testing instructions, although they pretty much just replicate what's in the unit test. If you need any more detail, just let me know. Cheers, Michael.

            Show
            maherne Michael Aherne added a comment - Hi Andrew, I've added some manual testing instructions, although they pretty much just replicate what's in the unit test. If you need any more detail, just let me know. Cheers, Michael.
            Hide
            poltawski Dan Poltawski added a comment -

            Sending all 'waiting for peer review' issues for integration review. The integration team are doing this to ensure any 'integratable issues' will not got missed for freeze.

            Note: We will prioritise peer reviewed issues and may not spend as much time examining non-integratable, non peer-reviewed issues.

            This is a present from the iTeam - it means that peer review is not working well enough! We really do not want to do this again! Lets improve our peer review process!

            Show
            poltawski Dan Poltawski added a comment - Sending all 'waiting for peer review' issues for integration review. The integration team are doing this to ensure any 'integratable issues' will not got missed for freeze. Note: We will prioritise peer reviewed issues and may not spend as much time examining non-integratable, non peer-reviewed issues. This is a present from the iTeam - it means that peer review is not working well enough! We really do not want to do this again! Lets improve our peer review process!
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Michael,

            Thanks for this, looks nice, but i'm going to have to reopen it because:

            1. I don't think this is quite the right place to do this, we already have the flag to the function $usetrueaddress which does the same thing? Therefore, I think you could either change the calling code to set that flag (e.g. message/output/email/message_output_email.php ), or if that is not sufficient override it depending on the $CFG value.
            2. Although this is a small change, I think it would be worthwhile starting a forum discussion about it (especially given we have a few different things in use here. There is the support email address and a few other email addresses which come into play here.
            3. I think it probably should be exposed as web interface option (although I really hate adding settings there )
            Show
            poltawski Dan Poltawski added a comment - Hi Michael, Thanks for this, looks nice, but i'm going to have to reopen it because: I don't think this is quite the right place to do this, we already have the flag to the function $usetrueaddress which does the same thing? Therefore, I think you could either change the calling code to set that flag (e.g. message/output/email/message_output_email.php ), or if that is not sufficient override it depending on the $CFG value. Although this is a small change, I think it would be worthwhile starting a forum discussion about it (especially given we have a few different things in use here. There is the support email address and a few other email addresses which come into play here. I think it probably should be exposed as web interface option (although I really hate adding settings there )
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            maherne Michael Aherne added a comment -

            Hi Dan,

            Thanks for the review. I agree with all of your points, so I'll have another look at it, and get a forum discussion started.

            For point 1, I'm not convinced that trying to do it in caller code would work, as you'd end up in a situation where third party plugins would be free to ignore the server setting. I'd be much more comfortable if it was done inside the email_to_user function itself so it was guaranteed.

            The more I think about it, I think the best implementation would probably be a setting with some options - let Moodle decide on the from address, always use the noreply address, always use the support address, but I'll see what comes out on the forum.

            Cheers

            Michael

            Show
            maherne Michael Aherne added a comment - Hi Dan, Thanks for the review. I agree with all of your points, so I'll have another look at it, and get a forum discussion started. For point 1, I'm not convinced that trying to do it in caller code would work, as you'd end up in a situation where third party plugins would be free to ignore the server setting. I'd be much more comfortable if it was done inside the email_to_user function itself so it was guaranteed. The more I think about it, I think the best implementation would probably be a setting with some options - let Moodle decide on the from address, always use the noreply address, always use the support address, but I'll see what comes out on the forum. Cheers Michael
            Hide
            poltawski Dan Poltawski added a comment -

            I'd be much more comfortable if it was done inside the email_to_user function itself so it was guaranteed.

            In the simple case, I think you could do something like:

            if (empty($CFG->emailonlyfromnoreplyaddress)) {
               $usetrueaddress = true;
            }
            

            Show
            poltawski Dan Poltawski added a comment - I'd be much more comfortable if it was done inside the email_to_user function itself so it was guaranteed. In the simple case, I think you could do something like: if (empty($CFG->emailonlyfromnoreplyaddress)) { $usetrueaddress = true; }
            Hide
            maherne Michael Aherne added a comment -

            I'm not sure if that would work for things like blind marking in assignments, where $usetrueaddress is deliberately set to false. It would maybe need to be the other way round:

            if (!empty($CFG->emailonlyfromnoreplyaddress)) {
               $usetrueaddress = false;
            }
            

            Show
            maherne Michael Aherne added a comment - I'm not sure if that would work for things like blind marking in assignments, where $usetrueaddress is deliberately set to false. It would maybe need to be the other way round: if (!empty($CFG->emailonlyfromnoreplyaddress)) { $usetrueaddress = false; }
            Hide
            poltawski Dan Poltawski added a comment -

            yep - sorry that was actually what I meant

            Show
            poltawski Dan Poltawski added a comment - yep - sorry that was actually what I meant
            Hide
            poltawski Dan Poltawski added a comment -
            Show
            poltawski Dan Poltawski added a comment - Interesting related thread https://moodle.org/mod/forum/discuss.php?d=257981
            Hide
            wsbk Blake Kidney added a comment -

            Since the issue seems to be related to the domain and not the specific email address, could there be a feature in this that allows you to use the specific email address if the email matches the domain, but if it does not, then it sends it out from the noreply address. So, for example, if the domain is mywebsite.com and email is going from personA@mywebsite.com to personB@mywebsite.com, it will preserve the original to and from. However, if it is going from personA@mywebsite.com to personA@yourwebsite.com, then it will use the the noreply address. Just a suggestion.

            Show
            wsbk Blake Kidney added a comment - Since the issue seems to be related to the domain and not the specific email address, could there be a feature in this that allows you to use the specific email address if the email matches the domain, but if it does not, then it sends it out from the noreply address. So, for example, if the domain is mywebsite.com and email is going from personA@mywebsite.com to personB@mywebsite.com, it will preserve the original to and from. However, if it is going from personA@mywebsite.com to personA@yourwebsite.com, then it will use the the noreply address. Just a suggestion.
            Hide
            maherne Michael Aherne added a comment -

            Hi Blake, thanks for the suggestion! It's an interesting idea, and would probably work for quite a lot of Moodle installations, but I'd be a bit wary of hard-coding a set of assumptions about how people have their email set up, which it effectively is. In particular, I don't think you can assume that the web domain a Moodle installation is running on is related to the email domain it's using.

            My other main concern would be matching subdomains. To take an example from my own institution:

            • Moodle runs on classes.myplace.strath.ac.uk
            • Staff email is internally hosted and is generally in the form somebody@strath.ac.uk
            • Student email is hosted by Microsoft and is in the form somebody@uni.strath.ac.uk

            If we take subdomains into account, the feature doesn't work as nobody has an email at classes.myplace.strath.ac.uk. If we don't take subdomains into account, and just match strath.ac.uk, we'll end up sending emails from uni.strath.ac.uk addresses, which could quite easily end up being blocked if Microsoft put the kind of spam controls in place that are talked about in the forum thread Dan posted.

            Having said that, I can imagine it would be a nice feature to have if you did have a Moodle setup where your web domain exactly matches your email domain, and no subdomains are used (which must surely be a common setup, particularly for smaller installations), so it's certainly worth thinking about.

            Show
            maherne Michael Aherne added a comment - Hi Blake, thanks for the suggestion! It's an interesting idea, and would probably work for quite a lot of Moodle installations, but I'd be a bit wary of hard-coding a set of assumptions about how people have their email set up, which it effectively is. In particular, I don't think you can assume that the web domain a Moodle installation is running on is related to the email domain it's using. My other main concern would be matching subdomains. To take an example from my own institution: Moodle runs on classes.myplace.strath.ac.uk Staff email is internally hosted and is generally in the form somebody@strath.ac.uk Student email is hosted by Microsoft and is in the form somebody@uni.strath.ac.uk If we take subdomains into account, the feature doesn't work as nobody has an email at classes.myplace.strath.ac.uk. If we don't take subdomains into account, and just match strath.ac.uk, we'll end up sending emails from uni.strath.ac.uk addresses, which could quite easily end up being blocked if Microsoft put the kind of spam controls in place that are talked about in the forum thread Dan posted. Having said that, I can imagine it would be a nice feature to have if you did have a Moodle setup where your web domain exactly matches your email domain, and no subdomains are used (which must surely be a common setup, particularly for smaller installations), so it's certainly worth thinking about.
            Hide
            wsbk Blake Kidney added a comment - - edited

            Thanks for the consideration Michael.

            My thought on this particular topic is that in certain cases, it is nice to be able hit "reply" from an email when noreply isn't necessary. The particular feature being introduced will impact all emails and will hinder that ability for all emails.

            How about another configuration variable that allows you to provide exceptions to the rule. This could be a comma separated list or an array. Or, you could even allow it to be a regular expression that matches against it. Administrators could then create their own rule. So, for example:

            $CFG->emailonlyfromnoreplyaddressexceptdomains = "strath.ac.uk,uni.strath.ac.uk";
            

            OR

            $CFG->emailonlyfromnoreplyaddressexcept = "/^.*?@(uni.)?strath.ac.uk$/";
            

            This is just an idea to help accommodate emails sent from domains system admins control. The feature might be more trouble than it is worth.

            Show
            wsbk Blake Kidney added a comment - - edited Thanks for the consideration Michael. My thought on this particular topic is that in certain cases, it is nice to be able hit "reply" from an email when noreply isn't necessary. The particular feature being introduced will impact all emails and will hinder that ability for all emails. How about another configuration variable that allows you to provide exceptions to the rule. This could be a comma separated list or an array. Or, you could even allow it to be a regular expression that matches against it. Administrators could then create their own rule. So, for example: $CFG->emailonlyfromnoreplyaddressexceptdomains = "strath.ac.uk,uni.strath.ac.uk"; OR $CFG->emailonlyfromnoreplyaddressexcept = "/^.*?@(uni.)?strath.ac.uk$/"; This is just an idea to help accommodate emails sent from domains system admins control. The feature might be more trouble than it is worth.
            Hide
            wsbk Blake Kidney added a comment -

            Because of the new DMARC policy on Yahoo, we implemented a solution like the one described above where all emails sent from the system came from the noreply address. This caused a further complication because professors were hitting reply in response to students, and the email wouldn't go through. I thought it would be good to mention this as the replyto address might also need to be set.

            Show
            wsbk Blake Kidney added a comment - Because of the new DMARC policy on Yahoo, we implemented a solution like the one described above where all emails sent from the system came from the noreply address. This caused a further complication because professors were hitting reply in response to students, and the email wouldn't go through. I thought it would be good to mention this as the replyto address might also need to be set.
            Hide
            rex Rex Lorenzo added a comment -

            [N] Syntax
            [Y] Whitespace
            [-] Output
            [-] Language
            [-] Databases
            [Y] Testing (instructions and automated tests)
            [Y] Security
            [Y] Documentation
            [N] Git
            [-] Third party code
            [N] Sanity check

            • Syntax - Please add a period at the end of your comment at lib/tests/moodlelib_test.php
            • Git - Please create branches for every supported version of Moodle (2.4+).
            • Sanity check - In your unit test you set $CFG and then later call unset_config(). Who not use the related set_config() when setting $CFG to be consistent?
            Show
            rex Rex Lorenzo added a comment - [N] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [Y] Security [Y] Documentation [N] Git [-] Third party code [N] Sanity check Syntax - Please add a period at the end of your comment at lib/tests/moodlelib_test.php Git - Please create branches for every supported version of Moodle (2.4+). Sanity check - In your unit test you set $CFG and then later call unset_config(). Who not use the related set_config() when setting $CFG to be consistent?
            Hide
            rex Rex Lorenzo added a comment -

            Just want to say that we just deployed this patch out to our PROD server this morning and worked in our testing. Great work!

            Show
            rex Rex Lorenzo added a comment - Just want to say that we just deployed this patch out to our PROD server this morning and worked in our testing. Great work!
            Hide
            rex Rex Lorenzo added a comment -

            Just reporting that for the week that we did not have the patch, we had a bit over 1,200 bounced email messages. Since we have deployed the patch, we haven't had a single bounce due to the DMARC policy change.

            Michael Aherne would you be able to address the issues in the peer review and Dan's comments above? I would like to see this patch get into core so we don't have to maintain this core edit for too long.

            Show
            rex Rex Lorenzo added a comment - Just reporting that for the week that we did not have the patch, we had a bit over 1,200 bounced email messages. Since we have deployed the patch, we haven't had a single bounce due to the DMARC policy change. Michael Aherne would you be able to address the issues in the peer review and Dan's comments above? I would like to see this patch get into core so we don't have to maintain this core edit for too long.
            Hide
            poltawski Dan Poltawski added a comment - - edited

            As i've seen reports of this affecting a lot of people, I am going to start the 'backport voting' process early with Integrators so that we can consider backporting this issue to the stable branches. (Note that as a 'new feature/improvement' we wouldn't normally consider doing this).

            Show
            poltawski Dan Poltawski added a comment - - edited As i've seen reports of this affecting a lot of people, I am going to start the 'backport voting' process early with Integrators so that we can consider backporting this issue to the stable branches. (Note that as a 'new feature/improvement' we wouldn't normally consider doing this).
            Hide
            skodak Petr Skoda added a comment -

            +1 from me for all branches

            Show
            skodak Petr Skoda added a comment - +1 from me for all branches
            Hide
            damyon Damyon Wiese added a comment -

            Could do with some checking that both noreplyaddress and emailonlyfromnoreplyaddress are set.

            Show
            damyon Damyon Wiese added a comment - Could do with some checking that both noreplyaddress and emailonlyfromnoreplyaddress are set.
            Hide
            maherne Michael Aherne added a comment -

            I'll try to get an updated patch for this sorted out today, taking into account the peer review and other feedback.

            Show
            maherne Michael Aherne added a comment - I'll try to get an updated patch for this sorted out today, taking into account the peer review and other feedback.
            Hide
            maherne Michael Aherne added a comment -

            I've uploaded a new version of this patch based on the feedback. I haven't done anything about checking whether $CFG->noreplyaddress is set as the existing code uses it without doing this.

            I've backported it to 2.6 and 2.5, although I don't have a 2.5 instance set up to allow me to run the unit tests, so I can't guarantee it's working.

            (Also, thanks to Rex for the initial peer review - I forgot to mention that before )

            Show
            maherne Michael Aherne added a comment - I've uploaded a new version of this patch based on the feedback. I haven't done anything about checking whether $CFG->noreplyaddress is set as the existing code uses it without doing this. I've backported it to 2.6 and 2.5, although I don't have a 2.5 instance set up to allow me to run the unit tests, so I can't guarantee it's working. (Also, thanks to Rex for the initial peer review - I forgot to mention that before )
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-43669 Remote repository: git://github.com/micaherne/moodle.git Remote branch MDL-43669 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3024 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3024/artifact/work/smurf.html Remote branch MDL-43669 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3025 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3025/artifact/work/smurf.html Remote branch MDL-43669 -m to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3026 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3026/artifact/work/smurf.html
            Hide
            maherne Michael Aherne added a comment -

            Fixed the formatting issues.

            Show
            maherne Michael Aherne added a comment - Fixed the formatting issues.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-43669 Remote repository: git://github.com/micaherne/moodle.git Remote branch MDL-43669 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3028 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3028/artifact/work/smurf.html Remote branch MDL-43669 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3029 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3029/artifact/work/smurf.html Remote branch MDL-43669 -m to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3030 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3030/artifact/work/smurf.html
            Hide
            rex Rex Lorenzo added a comment -

            Michael Aherne There are still formatting issues reported by CiBot. Once you get that resolved, you can add the label "ready-for-integration" to this ticket so that it can get looked at by the integration team.

            Show
            rex Rex Lorenzo added a comment - Michael Aherne There are still formatting issues reported by CiBot. Once you get that resolved, you can add the label "ready-for-integration" to this ticket so that it can get looked at by the integration team.
            Hide
            rex Rex Lorenzo added a comment -

            Also, a question for the integrator, should the default value for "emailonlyfromnoreplyaddress" be true? I don't believe Yahoo is going to reverse their decision and generally maybe more email providers might make the same policy decision, since it is "more secure".

            Show
            rex Rex Lorenzo added a comment - Also, a question for the integrator, should the default value for "emailonlyfromnoreplyaddress" be true? I don't believe Yahoo is going to reverse their decision and generally maybe more email providers might make the same policy decision, since it is "more secure".
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Opinion: I think that in other cases like this where:

            1) The backwards compatible value is X, and
            2) The "best" value is Y

            what we have done in the past is:

            A) For new installations, provide the "best" value (aka Y).
            B) For existing installations, provide the "BC" value (aka X).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Opinion: I think that in other cases like this where: 1) The backwards compatible value is X, and 2) The "best" value is Y what we have done in the past is: A) For new installations, provide the "best" value (aka Y). B) For existing installations, provide the "BC" value (aka X). Ciao
            Hide
            poltawski Dan Poltawski added a comment -

            FYI: The Integration team have voted on this and are universally agreed to integrate it to stable supported branches.

            Show
            poltawski Dan Poltawski added a comment - FYI: The Integration team have voted on this and are universally agreed to integrate it to stable supported branches.
            Hide
            maherne Michael Aherne added a comment -

            Hi Eloy, I like that idea but can't work out how to do it! Can you point me at some code?

            Show
            maherne Michael Aherne added a comment - Hi Eloy, I like that idea but can't work out how to do it! Can you point me at some code?
            Hide
            maherne Michael Aherne added a comment -

            I've fixed the code formatting issues that CiBot came up with, so hopefully we can get it finished!

            Show
            maherne Michael Aherne added a comment - I've fixed the code formatting issues that CiBot came up with, so hopefully we can get it finished!
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-43669 Remote repository: git://github.com/micaherne/moodle.git Remote branch MDL-43669 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3108 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3108/artifact/work/smurf.html Remote branch MDL-43669 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3109 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3109/artifact/work/smurf.html Remote branch MDL-43669 -m to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3110 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3110/artifact/work/smurf.html
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Michael,

            first of all, I think your patch looks great, good work.

            Regarding how to behave differently for new and upgrade sites, it's really simple:

            1) You define the setting default to be the best value. That automatically means that every site will get the best value.
            2) For existing sites, you add a simple upgrade step defining the setting to the old value, so they continue working like they were before the change.

            A really simple example of that way to proceed is show here:

            http://git.moodle.org/gw?p=moodle.git;a=commitdiff;h=8900213b922c577639b339618268c948052cda4f

            (the setting defaults to 0 - best value, but we keep upgraded sites with 1 - the bc value).

            For your consideration, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Michael, first of all, I think your patch looks great, good work. Regarding how to behave differently for new and upgrade sites, it's really simple: 1) You define the setting default to be the best value. That automatically means that every site will get the best value. 2) For existing sites, you add a simple upgrade step defining the setting to the old value, so they continue working like they were before the change. A really simple example of that way to proceed is show here: http://git.moodle.org/gw?p=moodle.git;a=commitdiff;h=8900213b922c577639b339618268c948052cda4f (the setting defaults to 0 - best value, but we keep upgraded sites with 1 - the bc value). For your consideration, ciao
            Hide
            maherne Michael Aherne added a comment -

            Many thanks for that, Eloy! I've updated the patches to do what you suggested. I'm not sure about how to submit a patch that changes the version number, so I've just done what seems sensible to me. Hopefully the integrator will be able to sort it out from there.

            Show
            maherne Michael Aherne added a comment - Many thanks for that, Eloy! I've updated the patches to do what you suggested. I'm not sure about how to submit a patch that changes the version number, so I've just done what seems sensible to me. Hopefully the integrator will be able to sort it out from there.
            Hide
            martinlanghoff Martín Langhoff added a comment -

            This is affecting us as well. Reading through the discussion here, a few things I am not 100% clear

            • does this patch change...
              • the envelope sender?
              • the RFC822 "from"?
              • the in-message "by"?

            Looking at moodle.org's recent forum posts, I think the answers are

            • seems to change envelope sender (I can't see it in msg headers, so I am guessing here)
            • changes the RFC822 From to "Real Name" <noreply@moodle.org>, and sets reply-to appropriately
            • the in-message 'by' line is kept

            I don't entirely like the mangling of the RFC822 "from" line, but I think that's been with us for a while, so I'm not going to complain here and now

            Show
            martinlanghoff Martín Langhoff added a comment - This is affecting us as well. Reading through the discussion here, a few things I am not 100% clear does this patch change... the envelope sender? the RFC822 "from"? the in-message "by"? Looking at moodle.org's recent forum posts, I think the answers are seems to change envelope sender (I can't see it in msg headers, so I am guessing here) changes the RFC822 From to "Real Name" <noreply@moodle.org>, and sets reply-to appropriately the in-message 'by' line is kept I don't entirely like the mangling of the RFC822 "from" line, but I think that's been with us for a while, so I'm not going to complain here and now
            Hide
            rex Rex Lorenzo added a comment - - edited

            [Y] Syntax
            [Y] Whitespace
            [Y] Output
            [Y] Language
            [-] Databases
            [Y] Testing (instructions and automated tests)
            [Y] Security
            [-] Documentation
            [N] Git
            [-] Third party code
            [N] Sanity check

            • Documentation
              • I see this ticket has the docs_required label, so should be fine once done.
            • Git
              • In your Git branch for M25, you have a bunch of changes in lib/tests/moodlelib_test.php not related to your commit.
            • Sanity check
              • Why have code in the upgrade.php to set the $CFG>emailonlyfromnoreplyaddress value to 0, which is already the default value? When Moodle does its upgrade it prompts the user to fill in any new config values. Also, I do want to reiterate that the default should be 1 for at least master. The default value should be the value that is the better option for most users right?

            <EDIT> Okay, I read Eloy's rely above. You are doing what he suggested. But for the master patch, don't set the value to 0, since for new install we want the default of 1 to be set.

            Show
            rex Rex Lorenzo added a comment - - edited [Y] Syntax [Y] Whitespace [Y] Output [Y] Language [-] Databases [Y] Testing (instructions and automated tests) [Y] Security [-] Documentation [N] Git [-] Third party code [N] Sanity check Documentation I see this ticket has the docs_required label, so should be fine once done. Git In your Git branch for M25, you have a bunch of changes in lib/tests/moodlelib_test.php not related to your commit. Sanity check Why have code in the upgrade.php to set the $CFG >emailonlyfromnoreplyaddress value to 0, which is already the default value? When Moodle does its upgrade it prompts the user to fill in any new config values. Also, I do want to reiterate that the default should be 1 for at least master. The default value should be the value that is the better option for most users right? <EDIT> Okay, I read Eloy's rely above. You are doing what he suggested. But for the master patch, don't set the value to 0, since for new install we want the default of 1 to be set.
            Hide
            maherne Michael Aherne added a comment -

            Hi Rex, thanks for the peer review again!

            I don't know how all that stuff ended up in my Moodle 2.5 branch, but I've just removed this branch as it's not required now that 2.7 is released.

            The logic for the setting is fine in master. Eloy's suggestion was that the setting would be set for new installations, but upgrades would leave the existing behaviour as it was, and this should apply to master too.

            Cheers, Michael

            Show
            maherne Michael Aherne added a comment - Hi Rex, thanks for the peer review again! I don't know how all that stuff ended up in my Moodle 2.5 branch, but I've just removed this branch as it's not required now that 2.7 is released. The logic for the setting is fine in master. Eloy's suggestion was that the setting would be set for new installations, but upgrades would leave the existing behaviour as it was, and this should apply to master too. Cheers, Michael
            Hide
            maherne Michael Aherne added a comment -

            I don't have permissions to send this for integration, so I've tried the new ready-for-integration label.

            Show
            maherne Michael Aherne added a comment - I don't have permissions to send this for integration, so I've tried the new ready-for-integration label.
            Hide
            damyon Damyon Wiese added a comment -

            Hi Michael - thanks for working on this.

            Unfortunately there are 2 things that need fixing.

            1. The unit tests are failing on all branches:

            There was 1 failure:
             
            1) core_moodlelib_testcase::test_email_to_user
            Failed asserting that two strings are identical.
            --- Expected
            +++ Actual
            @@ @@
            -username2@example.com
            +noreply@www.example.com
             
            /home/damyonw/Documents/Moodle/integration/MOODLE_26_STABLE/moodle/lib/tests/moodlelib_test.php:2561
            /home/damyonw/Documents/Moodle/integration/MOODLE_26_STABLE/moodle/lib/phpunit/classes/advanced_testcase.php:80
             
            To re-run:
             ./vendor/bin/phpunit core_moodlelib_testcase lib/tests/moodlelib_test.php
            

            2. The upgrade path needs a little more thought (because this is backported) - e.g. if someone installs 2.6.4 they will have this setting set to 1 - but then when they upgrade to 2.7.1 it will be changed to 0. This is tricky - maybe the default for 2.6 should be 1 and the default for 2.7/master should be 2 - and the upgrade step for 2.7/master should only change the setting if it is set to 2 (I have only thought about this quickly and I'm sure I have missed something).

            Please fix the unit tests and the upgrade path and resubmit for integration.

            Cheers - Damyon

            Show
            damyon Damyon Wiese added a comment - Hi Michael - thanks for working on this. Unfortunately there are 2 things that need fixing. 1. The unit tests are failing on all branches: There was 1 failure:   1) core_moodlelib_testcase::test_email_to_user Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -username2@example.com +noreply@www.example.com   /home/damyonw/Documents/Moodle/integration/MOODLE_26_STABLE/moodle/lib/tests/moodlelib_test.php:2561 /home/damyonw/Documents/Moodle/integration/MOODLE_26_STABLE/moodle/lib/phpunit/classes/advanced_testcase.php:80   To re-run: ./vendor/bin/phpunit core_moodlelib_testcase lib/tests/moodlelib_test.php 2. The upgrade path needs a little more thought (because this is backported) - e.g. if someone installs 2.6.4 they will have this setting set to 1 - but then when they upgrade to 2.7.1 it will be changed to 0. This is tricky - maybe the default for 2.6 should be 1 and the default for 2.7/master should be 2 - and the upgrade step for 2.7/master should only change the setting if it is set to 2 (I have only thought about this quickly and I'm sure I have missed something). Please fix the unit tests and the upgrade path and resubmit for integration. Cheers - Damyon
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            maherne Michael Aherne added a comment -

            Hi Damyon, thanks for the feedback! I've removed the code that messes around with the config value, and the unit tests are now working. Cheers, Michael.

            Show
            maherne Michael Aherne added a comment - Hi Damyon, thanks for the feedback! I've removed the code that messes around with the config value, and the unit tests are now working. Cheers, Michael.
            Hide
            marina Marina Glancy added a comment -

            Thanks Michael, integrated in 2.6, 2.7 and master

            Show
            marina Marina Glancy added a comment - Thanks Michael, integrated in 2.6, 2.7 and master
            Hide
            johno John Okely added a comment -

            Unit tests pass. The email comes from noreply or the relevant user's address as expected, depending on config. Tested with messages system, assignment always comes from noreply.

            Show
            johno John Okely added a comment - Unit tests pass. The email comes from noreply or the relevant user's address as expected, depending on config. Tested with messages system, assignment always comes from noreply.
            Hide
            oertel Stefan Oertel added a comment -

            Hi Marina, the integration is for 2.6 and 2.7 only? What about 2.5.6? It's affected too. A lot of mails are bounced from a increasing count of mail servers worldwide. Ciao Stefan

            Show
            oertel Stefan Oertel added a comment - Hi Marina, the integration is for 2.6 and 2.7 only? What about 2.5.6? It's affected too. A lot of mails are bounced from a increasing count of mail servers worldwide. Ciao Stefan
            Hide
            marina Marina Glancy added a comment -

            Hi Stefan, 2.5 at the moment in the "extended" support state. 2.5.6 was supposed to be the last release with general support but integrators agreed that we integrate 2.5 branches if they are provided till the end of on-sync period (3 weeks after release). In this issue the 2.5 branch was not provided

            Show
            marina Marina Glancy added a comment - Hi Stefan, 2.5 at the moment in the "extended" support state. 2.5.6 was supposed to be the last release with general support but integrators agreed that we integrate 2.5 branches if they are provided till the end of on-sync period (3 weeks after release). In this issue the 2.5 branch was not provided
            Hide
            jonfila Jon Fila added a comment - - edited

            I implemented this fix yesterday by simply adding the changes to the files noted here: https://github.com/micaherne/moodle/commit/8257e19b7ab35d79e1ca29c68b2cd3787a8621d3
            Now I'm receiving error messages when logged in as a student and trying to submit an assignment:

            Error calling message processor email
            line 226 of /lib/messagelib.php: call to debugging()
            line 4408 of /mod/assign/locallib.php: call to message_send()
            line 4437 of /mod/assign/locallib.php: call to assign::send_assignment_notification()
            line 4521 of /mod/assign/locallib.php: call to assign->send_notification()
            line 4576 of /mod/assign/locallib.php: call to assign->notify_graders()
            line 4634 of /mod/assign/locallib.php: call to assign->submit_for_grading()
            line 420 of /mod/assign/locallib.php: call to assign->process_submit_for_grading()
            line 53 of /mod/assign/view.php: call to assign->view()
            Error calling message processor email
            line 226 of /lib/messagelib.php: call to debugging()
            line 4408 of /mod/assign/locallib.php: call to message_send()
            line 4437 of /mod/assign/locallib.php: call to assign::send_assignment_notification()
            line 4489 of /mod/assign/locallib.php: call to assign->send_notification()
            line 4577 of /mod/assign/locallib.php: call to assign->notify_student_submission_receipt()
            line 4634 of /mod/assign/locallib.php: call to assign->submit_for_grading()
            line 420 of /mod/assign/locallib.php: call to assign->process_submit_for_grading()
            line 53 of /mod/assign/view.php: call to assign->view()

            Is there a way to implement the changes without upgrading the site. I can't turn the site off even for a few minutes at this time of year when so many are scrambling to complete their work.

            Show
            jonfila Jon Fila added a comment - - edited I implemented this fix yesterday by simply adding the changes to the files noted here: https://github.com/micaherne/moodle/commit/8257e19b7ab35d79e1ca29c68b2cd3787a8621d3 Now I'm receiving error messages when logged in as a student and trying to submit an assignment: Error calling message processor email line 226 of /lib/messagelib.php: call to debugging() line 4408 of /mod/assign/locallib.php: call to message_send() line 4437 of /mod/assign/locallib.php: call to assign::send_assignment_notification() line 4521 of /mod/assign/locallib.php: call to assign->send_notification() line 4576 of /mod/assign/locallib.php: call to assign->notify_graders() line 4634 of /mod/assign/locallib.php: call to assign->submit_for_grading() line 420 of /mod/assign/locallib.php: call to assign->process_submit_for_grading() line 53 of /mod/assign/view.php: call to assign->view() Error calling message processor email line 226 of /lib/messagelib.php: call to debugging() line 4408 of /mod/assign/locallib.php: call to message_send() line 4437 of /mod/assign/locallib.php: call to assign::send_assignment_notification() line 4489 of /mod/assign/locallib.php: call to assign->send_notification() line 4577 of /mod/assign/locallib.php: call to assign->notify_student_submission_receipt() line 4634 of /mod/assign/locallib.php: call to assign->submit_for_grading() line 420 of /mod/assign/locallib.php: call to assign->process_submit_for_grading() line 53 of /mod/assign/view.php: call to assign->view() Is there a way to implement the changes without upgrading the site. I can't turn the site off even for a few minutes at this time of year when so many are scrambling to complete their work.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            So, finally, this landed, making Moodle better, many thanks!

            Be patient and understanding.
            Life is too short to be
            vengeful or malicious.

            Phillips Brooks

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - So, finally, this landed, making Moodle better, many thanks! Be patient and understanding. Life is too short to be vengeful or malicious. Phillips Brooks Closing as fixed, ciao
            Hide
            johno John Okely added a comment -

            Hey Jon Fila, I think that question might be more one for the developer forums. Looking at the patch I think changing the code manually should be fine. But this depends on your original moodle version and some other factors. So yeah perhaps make a post on the forums with those details and some friendly devs should be able to help out.

            Show
            johno John Okely added a comment - Hey Jon Fila , I think that question might be more one for the developer forums . Looking at the patch I think changing the code manually should be fine. But this depends on your original moodle version and some other factors. So yeah perhaps make a post on the forums with those details and some friendly devs should be able to help out.
            Hide
            maherne Michael Aherne added a comment -

            Hi Jon, if you just want to hack your server to make all the emails come from the noreply address, rather than trying to apply the full patch you might be better just to add the line $usetrueaddress = false; at the start of the email_to_user function. I wouldn't necessarily recommend doing this, but if you're looking for a quick fix that's probably the easiest way to do it.

            Show
            maherne Michael Aherne added a comment - Hi Jon, if you just want to hack your server to make all the emails come from the noreply address, rather than trying to apply the full patch you might be better just to add the line $usetrueaddress = false; at the start of the email_to_user function. I wouldn't necessarily recommend doing this, but if you're looking for a quick fix that's probably the easiest way to do it.
            Hide
            marycooch Mary Cooch added a comment -

            Removing docs_required as I have added a note to the http://docs.moodle.org/en/Messaging_settings page of the 2.7 and 2.6 docs.

            Show
            marycooch Mary Cooch added a comment - Removing docs_required as I have added a note to the http://docs.moodle.org/en/Messaging_settings page of the 2.7 and 2.6 docs.
            Hide
            emmarichardson Emma Richardson added a comment -

            This is now partially broken again...

            So Yahoo has now updated their policy again so that they are rejecting anything with a yahoo reply address that does not come from Yahoo. This means a user with a yahoo email using the message system to message another yahoo user will not go through.

            Show
            emmarichardson Emma Richardson added a comment - This is now partially broken again... So Yahoo has now updated their policy again so that they are rejecting anything with a yahoo reply address that does not come from Yahoo. This means a user with a yahoo email using the message system to message another yahoo user will not go through.
            Hide
            marina Marina Glancy added a comment -

            Hi Emma, please create a new issue about it, since this ticket is already closed.

            Show
            marina Marina Glancy added a comment - Hi Emma, please create a new issue about it, since this ticket is already closed.
            Hide
            johno John Okely added a comment -

            Hey Emma.

            Did you enable "Always send email from the no-reply address" from email settings? (admin/settings.php?section=messagesettingemail) This was developed as a solution to the problem you described (See MDL-45030)

            Show
            johno John Okely added a comment - Hey Emma. Did you enable "Always send email from the no-reply address" from email settings? (admin/settings.php?section=messagesettingemail) This was developed as a solution to the problem you described (See MDL-45030 )
            Hide
            emmarichardson Emma Richardson added a comment -

            My mistake. Thanks John, thought I had that set that way but when I did my last rebuild, must have missed that. I will see if that fixes it.

            Show
            emmarichardson Emma Richardson added a comment - My mistake. Thanks John, thought I had that set that way but when I did my last rebuild, must have missed that. I will see if that fixes it.
            Hide
            robsong3 Robson Fernando added a comment - - edited

            John,
            When I enable "Always send email from the no-reply address" from email settings? (admin/settings.php?section=messagesettingemail)

            All emails are delivered normally.
            But after I enable the option mentioned, I get the following cron messages:

            Cron <root@domain> /usr/bin/php /var/www/html/moodle/admin/cli/cron.php > /dev/null 29 de janeiro de 2015 17:00
            From: Cron Daemon
            To: root@domain

            PHP Notice: Trying to get property of non-object in /var/www/html/moodle/lib/moodlelib.php on line 5793

            My Version: Moodle 2.6.7

            Show
            robsong3 Robson Fernando added a comment - - edited John, When I enable "Always send email from the no-reply address" from email settings? (admin/settings.php?section=messagesettingemail) All emails are delivered normally. But after I enable the option mentioned, I get the following cron messages: Cron <root@domain> /usr/bin/php /var/www/html/moodle/admin/cli/cron.php > /dev/null 29 de janeiro de 2015 17:00 From: Cron Daemon To: root@domain PHP Notice: Trying to get property of non-object in /var/www/html/moodle/lib/moodlelib.php on line 5793 My Version: Moodle 2.6.7
            Hide
            marina Marina Glancy added a comment -

            Hello Robson, since this issue is already closed, can you please create a new bug report about the php notice you encountered. Even though 2.6 is no longer supported, I'm pretty sure the same bug is still present in the currently supported versions. Will be especially useful if you can enable developer debugging mode and copy a backtrace of the error. Thanks in advance

            Show
            marina Marina Glancy added a comment - Hello Robson, since this issue is already closed, can you please create a new bug report about the php notice you encountered. Even though 2.6 is no longer supported, I'm pretty sure the same bug is still present in the currently supported versions. Will be especially useful if you can enable developer debugging mode and copy a backtrace of the error. Thanks in advance
            Hide
            robsong3 Robson Fernando added a comment -

            Thanks Marina.
            I open a new issue MDL-49142

            Show
            robsong3 Robson Fernando added a comment - Thanks Marina. I open a new issue MDL-49142

              People

              • Votes:
                21 Vote for this issue
                Watchers:
                38 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jul/14