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

          Attachments

            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
              Hide
              mikidream miki Alliel added a comment - - edited

              We have Moodle 2.9.2 and a teacher who has Yahoo account can't send a message through email - it comes back to the admin.
              Can you tell me please if there any solution for that without enabling the "emailonlyfromnoreplyaddress" setting?
              and another questions - Is this working in Moodle 3.0+ (again without enabling the "emailonlyfromnoreplyaddress" setting)?
              Thanks

              Show
              mikidream miki Alliel added a comment - - edited We have Moodle 2.9.2 and a teacher who has Yahoo account can't send a message through email - it comes back to the admin. Can you tell me please if there any solution for that without enabling the "emailonlyfromnoreplyaddress" setting? and another questions - Is this working in Moodle 3.0+ (again without enabling the "emailonlyfromnoreplyaddress" setting)? Thanks

                People

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

                  Dates

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