Moodle
  1. Moodle
  2. MDL-26185

message_output_email uses undefined $userto

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.0.3
    • Component/s: Messages
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15901

      Description

      The code in message/output/email/message_output_email.php uses some weird logic because
      if ( !empty($usertoemailaddress)) {
      $userto->email = $usertoemailaddress;
      }
      does not seem to do anything which would make all the code above useless including the static caching...

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Adding Andrew as watcher.

          Hi Andrew,

          This issue occurs from code changes in MDL-23101. Since you have the experience with the code, I think you are the best person to review it.

          When you have a chance, could you take a look patch?
          https://github.com/rwijaya/moodle/compare/master...MDL-26185_m20

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Adding Andrew as watcher. Hi Andrew, This issue occurs from code changes in MDL-23101 . Since you have the experience with the code, I think you are the best person to review it. When you have a chance, could you take a look patch? https://github.com/rwijaya/moodle/compare/master...MDL-26185_m20 Thanks Rosie
          Hide
          Andrew Davis added a comment -

          You need to fix up your diff URL. It should be https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26185_m20

          MOODLE_20_STABLE not master

          Show
          Andrew Davis added a comment - You need to fix up your diff URL. It should be https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26185_m20 MOODLE_20_STABLE not master
          Hide
          Rossiani Wijaya added a comment -

          Thanks for the reminder. I just push MOODLE_20_STABLE recently and forgot to post the new diff url.

          Latest diff patch is available at: https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26185_m20

          Show
          Rossiani Wijaya added a comment - Thanks for the reminder. I just push MOODLE_20_STABLE recently and forgot to post the new diff url. Latest diff patch is available at: https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26185_m20
          Hide
          Andrew Davis added a comment - - edited

          Be aware that get_user_preferences() is now doing more caching so we can probably get rid of the email address cache in this function ie $useremailaddresses. That will hopefully simplify the logic.

          I dont think you should be passing on object into get_user_preferences() as $default.

          Show
          Andrew Davis added a comment - - edited Be aware that get_user_preferences() is now doing more caching so we can probably get rid of the email address cache in this function ie $useremailaddresses. That will hopefully simplify the logic. I dont think you should be passing on object into get_user_preferences() as $default.
          Hide
          Rossiani Wijaya added a comment -

          Thanks Andrew for the feedback.

          Create new patch to removing email address cache and set default value to email string. get_user_preference() is use to determine userto email address.

          diff url: https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26185_m20

          Show
          Rossiani Wijaya added a comment - Thanks Andrew for the feedback. Create new patch to removing email address cache and set default value to email string. get_user_preference() is use to determine userto email address. diff url: https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26185_m20
          Hide
          Andrew Davis added a comment -

          +1

          Show
          Andrew Davis added a comment - +1
          Hide
          Rossiani Wijaya added a comment -

          Submitted to pull request.

          Show
          Rossiani Wijaya added a comment - Submitted to pull request.
          Hide
          Andrew Davis added a comment -

          Reopening as this is generating an error. I commented in PULL-574 and said:

          Something is wrong. Im getting the following error when a user is sent an email notification of a personal message.

          Error: lib/moodlelib.php email_to_user(): User is null or has no email.

          The user receiving the message has an email address in their user profile but hasnt specified an email address in their messaging preferences. If there is no email address specified in the messaging preferences it should fall back to using the one in the user's user profile.

          Sorry Rossie, we've done something wrong

          Show
          Andrew Davis added a comment - Reopening as this is generating an error. I commented in PULL-574 and said: Something is wrong. Im getting the following error when a user is sent an email notification of a personal message. Error: lib/moodlelib.php email_to_user(): User is null or has no email. The user receiving the message has an email address in their user profile but hasnt specified an email address in their messaging preferences. If there is no email address specified in the messaging preferences it should fall back to using the one in the user's user profile. Sorry Rossie, we've done something wrong
          Hide
          Andrew Davis added a comment -

          I suspect that once this issue is done MDL-27026 can be closed.

          Show
          Andrew Davis added a comment - I suspect that once this issue is done MDL-27026 can be closed.
          Hide
          Andrew Davis added a comment -

          sprint 8 has ended so bringing this along to sprint 9.

          Show
          Andrew Davis added a comment - sprint 8 has ended so bringing this along to sprint 9.
          Hide
          Andrew Davis added a comment -

          The following scenarios need to be checked:

          1) the user has an email address in their user profile but the email box in their messaging preferences is empty. Email notifications go to the email address in their user profile.

          2) the user has an email address in their user profile but has a different email address in their messaging preferences. Email notifications go to the email address in their messaging preferences.

          Show
          Andrew Davis added a comment - The following scenarios need to be checked: 1) the user has an email address in their user profile but the email box in their messaging preferences is empty. Email notifications go to the email address in their user profile. 2) the user has an email address in their user profile but has a different email address in their messaging preferences. Email notifications go to the email address in their messaging preferences.
          Hide
          Sam Hemelryk added a comment -

          Andrew, does this look about right to fix this problem to you?
          It appears to fix the problem for me:

          diff --git a/message/output/email/message_output_email.php b/message/output/email/message_output_email.php
          index 0445fbe..5ebd672 100644
          --- a/message/output/email/message_output_email.php
          +++ b/message/output/email/message_output_email.php
          @@ -47,7 +47,10 @@ class message_output_email extends message_output {
                   }
           
                   //check user preference for where user wants email sent
          -        $eventdata->userto->email = get_user_preferences('message_processor_email_email', $eventdata->userto->email, $eventdata->userto->id);
          +        $preferedemail = get_user_preferences('message_processor_email_email', null, $eventdata->userto->id);
          +        if (!empty($preferedemail)) {
          +            $eventdata->userto->email = $preferedemail;
          +        }
           
                   $result = email_to_user($eventdata->userto, $eventdata->userfrom,
                       $eventdata->subject, $eventdata->fullmessage, $eventdata->fullmessagehtml);
          

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Andrew, does this look about right to fix this problem to you? It appears to fix the problem for me: diff --git a/message/output/email/message_output_email.php b/message/output/email/message_output_email.php index 0445fbe..5ebd672 100644 --- a/message/output/email/message_output_email.php +++ b/message/output/email/message_output_email.php @@ -47,7 +47,10 @@ class message_output_email extends message_output { } //check user preference for where user wants email sent - $eventdata->userto->email = get_user_preferences('message_processor_email_email', $eventdata->userto->email, $eventdata->userto->id); + $preferedemail = get_user_preferences('message_processor_email_email', null , $eventdata->userto->id); + if (!empty($preferedemail)) { + $eventdata->userto->email = $preferedemail; + } $result = email_to_user($eventdata->userto, $eventdata->userfrom, $eventdata->subject, $eventdata->fullmessage, $eventdata->fullmessagehtml); Cheers Sam
          Hide
          Andrew Davis added a comment -

          That looks right. mind you, I can clearly see why the old version wasn't working correctly unless get_user_preferences() doesnt work quite how I thought.

          Show
          Andrew Davis added a comment - That looks right. mind you, I can clearly see why the old version wasn't working correctly unless get_user_preferences() doesnt work quite how I thought.
          Hide
          Andrew Davis added a comment -

          Assigned this to me as there are issues with the pull request and I'm not sure when Rossie is in next.

          Show
          Andrew Davis added a comment - Assigned this to me as there are issues with the pull request and I'm not sure when Rossie is in next.
          Hide
          Andrew Davis added a comment -

          2.0 version
          repo: git://github.com/andyjdavis/moodle.git
          branch: MDL-26185_m20
          diff: https://github.com/andyjdavis/moodle/compare/MOODLE_20_STABLE...MDL-26185_m20

          2.1 version
          repo: git://github.com/andyjdavis/moodle.git
          branch: MDL-26185_master
          diff: https://github.com/andyjdavis/moodle/compare/master...MDL-26185_master

          Show
          Andrew Davis added a comment - 2.0 version repo: git://github.com/andyjdavis/moodle.git branch: MDL-26185 _m20 diff: https://github.com/andyjdavis/moodle/compare/MOODLE_20_STABLE...MDL-26185_m20 2.1 version repo: git://github.com/andyjdavis/moodle.git branch: MDL-26185 _master diff: https://github.com/andyjdavis/moodle/compare/master...MDL-26185_master
          Hide
          Andrew Davis added a comment - - edited

          PULL-603
          PULL-604

          Show
          Andrew Davis added a comment - - edited PULL-603 PULL-604
          Hide
          Eloy Lafuente (stronk7) added a comment -
          Show
          Eloy Lafuente (stronk7) added a comment - Side note from tester: Passed, but please take a look to http://tracker.moodle.org/browse/PULL-603?focusedCommentId=109201&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-109201 for needed followups. TIA and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: