Moodle
  1. Moodle
  2. MDL-25476

Email change notification not sent from support user

    Details

    • Testing Instructions:
      Hide

      Testing this is easiest if you have a course that you don't mind unenrolling everyone from. You'll need your admin and a student. You'll also need an email address you can access that isnt currently used on your test site.

      Go to /admin/settings.php?section=supportcontact and enter a name and email address for your support contact.
      The email address must be different from the default.

      Test email change notification:
      Check that the site setting "emailchangeconfirmation" is ticked
      Log in as a student
      Go to /user/edit.php?id=2 2 == the student's user id
      Change the user's email address. You should get a screen saying an email has been sent to the new address.
      Check the new email address. You should get an email about the email address change
      Check that the from name and email address match the ones you entered for the support contact

      Test that the fallback to admin works:
      Go back to the support contact page and clear the name and email address.
      As a student, go back to /user/edit.php?id=2 and change the email again.
      You should get a confirmation email, this time from admin.

      Re-add the support contact name and email address.

      Test failed login attempts notification:
      Set the site setting "notifyloginthreshold" to 1
      Set the site setting "notifyloginfailures" to admin
      Log out (or open a different browser), try to log in and get a users password wrong.
      Run cron.
      You should get an email about failed logins.
      Check that the from user and email address are the support contact.

      Test email_welcome_message():
      Check that the site setting "sendcoursewelcomemessage" is ticked
      Go to /admin/settings.php?section=manageenrols and enable self enrolment if it isnt
      Go into a course and go to course administration > Users > Enrolment methods.
      Enable self enrolment if if it isnt already.
      Go to course admininstration > users > Enrolled users and unenrol everyone
      As the student go to the course. There should be a "enrol me in this course" link in course administration.
      Enrol in the course.
      You should get a welcome email from the support contact.

      Show
      Testing this is easiest if you have a course that you don't mind unenrolling everyone from. You'll need your admin and a student. You'll also need an email address you can access that isnt currently used on your test site. Go to /admin/settings.php?section=supportcontact and enter a name and email address for your support contact. The email address must be different from the default. Test email change notification: Check that the site setting "emailchangeconfirmation" is ticked Log in as a student Go to /user/edit.php?id=2 2 == the student's user id Change the user's email address. You should get a screen saying an email has been sent to the new address. Check the new email address. You should get an email about the email address change Check that the from name and email address match the ones you entered for the support contact Test that the fallback to admin works: Go back to the support contact page and clear the name and email address. As a student, go back to /user/edit.php?id=2 and change the email again. You should get a confirmation email, this time from admin. Re-add the support contact name and email address. Test failed login attempts notification: Set the site setting "notifyloginthreshold" to 1 Set the site setting "notifyloginfailures" to admin Log out (or open a different browser), try to log in and get a users password wrong. Run cron. You should get an email about failed logins. Check that the from user and email address are the support contact. Test email_welcome_message(): Check that the site setting "sendcoursewelcomemessage" is ticked Go to /admin/settings.php?section=manageenrols and enable self enrolment if it isnt Go into a course and go to course administration > Users > Enrolment methods. Enable self enrolment if if it isnt already. Go to course admininstration > users > Enrolled users and unenrol everyone As the student go to the course. There should be a "enrol me in this course" link in course administration. Enrol in the course. You should get a welcome email from the support contact.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-25476_email_from_support
    • Rank:
      6543

      Description

      Email is sent using details from admin user instead of support settings

        Issue Links

          Activity

          Hide
          Paulo Matos added a comment -

          Attached patch solves this issue. Easy to merge.

          Show
          Paulo Matos added a comment - Attached patch solves this issue. Easy to merge.
          Hide
          Chris Follin added a comment -

          Various system emails in Moodle are sent by passing the result of get_admin() to the email_to_user() function as the from user. get_admin() returns the first admin user. Understandably, some people would prefer to have system emails sent out from a more generic address like the support user settings rather than from a real admin. Although the admin's email address can be hidden and replaced with the no-reply address, the admin's name still shows up in the From field of the message.

          We took a slightly different approach to this (but still similar to Paulo's patch) in that we modified the email_to_user() function. Our patch will apply to all system emails, not just the email change notification. We also added a setting under Site Administration > Server > Email that will turn on/off the override. When the setting is on, any call to email_to_user() that passes the result of get_admin() as the from user will be detected and $from will be overridden with the Support User info found on the same Server > Email settings page. The default for the setting is No so no behavior will change unless the setting is selected by an admin.

          Patch is attached.

          Show
          Chris Follin added a comment - Various system emails in Moodle are sent by passing the result of get_admin() to the email_to_user() function as the from user. get_admin() returns the first admin user. Understandably, some people would prefer to have system emails sent out from a more generic address like the support user settings rather than from a real admin. Although the admin's email address can be hidden and replaced with the no-reply address, the admin's name still shows up in the From field of the message. We took a slightly different approach to this (but still similar to Paulo's patch) in that we modified the email_to_user() function. Our patch will apply to all system emails, not just the email change notification. We also added a setting under Site Administration > Server > Email that will turn on/off the override. When the setting is on, any call to email_to_user() that passes the result of get_admin() as the from user will be detected and $from will be overridden with the Support User info found on the same Server > Email settings page. The default for the setting is No so no behavior will change unless the setting is selected by an admin. Patch is attached.
          Hide
          Helen Foster added a comment -

          Just confirming that this issue also affects moodle.org which is running Moodle 2.2.2.

          Show
          Helen Foster added a comment - Just confirming that this issue also affects moodle.org which is running Moodle 2.2.2.
          Hide
          Andrew Davis added a comment - - edited

          I'm not particularly keen on overriding the "from user", switching it from admin to the support user. For anyone looking at the code sending the email this behaviour is likely to be confusing.

          Also, it seems like it would behave oddly in the case of the admin user sending messages. For example if the admin sends a personal message to someone who has email notifications enabled. As I read this patch that notification would look like it came from the support user rather than the admin.

          edit: Im going through the calls to email_to_user() and it looks like theres very few places where we're not explicitly supplying the support user anyway. I'll go through them and see which need to be switched over.

          Show
          Andrew Davis added a comment - - edited I'm not particularly keen on overriding the "from user", switching it from admin to the support user. For anyone looking at the code sending the email this behaviour is likely to be confusing. Also, it seems like it would behave oddly in the case of the admin user sending messages. For example if the admin sends a personal message to someone who has email notifications enabled. As I read this patch that notification would look like it came from the support user rather than the admin. edit: Im going through the calls to email_to_user() and it looks like theres very few places where we're not explicitly supplying the support user anyway. I'll go through them and see which need to be switched over.
          Hide
          Ray Lawrence added a comment -

          Agree about the potential confusion but this email address needs to be separate from the admin's account. In many sites the default admin wont handle these requests.

          Show
          Ray Lawrence added a comment - Agree about the potential confusion but this email address needs to be separate from the admin's account. In many sites the default admin wont handle these requests.
          Hide
          Andrew Davis added a comment -

          Filled out the testing instructions.

          Show
          Andrew Davis added a comment - Filled out the testing instructions.
          Hide
          Andrew Davis added a comment -

          Im adding a branch that I think resolves this. As well as the email change notification Ive also switched over the course welcome and login failure emails to come from the support contact.

          Show
          Andrew Davis added a comment - Im adding a branch that I think resolves this. As well as the email change notification Ive also switched over the course welcome and login failure emails to come from the support contact.
          Hide
          Ray Lawrence added a comment -

          Andrew, I think the course welcome emails are a separate case.

          IIRC these are sent from the first "teacher" to be added to the course. I may be a bit behind the times here.

          The support contact is likely to be an overarching role in the organisation, not necessarily someone that students enrolling in a course would ever encounter.

          Show
          Ray Lawrence added a comment - Andrew, I think the course welcome emails are a separate case. IIRC these are sent from the first "teacher" to be added to the course. I may be a bit behind the times here. The support contact is likely to be an overarching role in the organisation, not necessarily someone that students enrolling in a course would ever encounter.
          Hide
          Aparup Banerjee added a comment -

          hm, just looking at this, the patch & code seems fine but some thoughts about the patch overall :

          in messaging and emailing, there is the concept of masquerading whereby we (hopefully) do not modify the source nor destination but simply appended to this message later on with a header, according to some flag/config, such that the message is masked to seem like coming from someone else. we can also have proper log /audit trail this way.

          • perhaps we should allow for moodle to optionally allow masquerading to some roles (isn't there something almost analogous to this : login as) and the messaging system changed to allow this option.
          • not sure message details should show the original 'from' but that can be further discussed.

          anyway thats my 2cents

          Show
          Aparup Banerjee added a comment - hm, just looking at this, the patch & code seems fine but some thoughts about the patch overall : in messaging and emailing, there is the concept of masquerading whereby we (hopefully) do not modify the source nor destination but simply appended to this message later on with a header, according to some flag/config, such that the message is masked to seem like coming from someone else. we can also have proper log /audit trail this way. perhaps we should allow for moodle to optionally allow masquerading to some roles (isn't there something almost analogous to this : login as) and the messaging system changed to allow this option. not sure message details should show the original 'from' but that can be further discussed. anyway thats my 2cents
          Hide
          Andrew Davis added a comment -

          Ray, you are correct in that the welcome emails do typically come from a teacher. If, however, there is no teacher in the course the email will now come from the support contact. Previously, it fell back to the admin.

          Aparup, masquerading could have some value Im not sure its relevant here. In the case of this bug the messages are simply being sent from the wrong email address, which is likely to be attached to the wrong human being.

          Show
          Andrew Davis added a comment - Ray, you are correct in that the welcome emails do typically come from a teacher. If, however, there is no teacher in the course the email will now come from the support contact. Previously, it fell back to the admin. Aparup, masquerading could have some value Im not sure its relevant here. In the case of this bug the messages are simply being sent from the wrong email address, which is likely to be attached to the wrong human being.
          Hide
          Andrew Davis added a comment -

          Putting this up for integration.

          Show
          Andrew Davis added a comment - Putting this up for integration.
          Hide
          Tim Hunt added a comment -

          Thanks for working on this.

          I was just wondering why you had only fixed those places, and not all the other similar places where we send emails from admin, but is seems that you have got all the places that use email_to_user. We really need to fix MDL-31560 to deal with the majority of instances of this problem since most messages are now sent using message_send. It is really great that you are working on that too.

          +1 from me.

          Have you updated the developer docs, to explain the 'right' way to send system emails?

          Show
          Tim Hunt added a comment - Thanks for working on this. I was just wondering why you had only fixed those places, and not all the other similar places where we send emails from admin, but is seems that you have got all the places that use email_to_user. We really need to fix MDL-31560 to deal with the majority of instances of this problem since most messages are now sent using message_send. It is really great that you are working on that too. +1 from me. Have you updated the developer docs, to explain the 'right' way to send system emails?
          Hide
          Dan Poltawski added a comment -

          Thanks Andrew, integrated to master, 23 and 22.

          Show
          Dan Poltawski added a comment - Thanks Andrew, integrated to master, 23 and 22.
          Hide
          Adrian Greeve added a comment -

          Tested and it all works. I didn't encounter any problems.
          Thanks

          Show
          Adrian Greeve added a comment - Tested and it all works. I didn't encounter any problems. Thanks
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

            • Votes:
              20 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: