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

message_output_email uses undefined $userto

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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...

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              rwijaya 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
              rwijaya 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
              andyjdavis 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
              andyjdavis 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
              rwijaya 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
              rwijaya 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
              andyjdavis 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
              andyjdavis 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
              rwijaya 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
              rwijaya 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
              andyjdavis Andrew Davis added a comment -

              +1

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

              Submitted to pull request.

              Show
              rwijaya Rossiani Wijaya added a comment - Submitted to pull request.
              Hide
              andyjdavis 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
              andyjdavis 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
              andyjdavis Andrew Davis added a comment -

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

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

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

              Show
              andyjdavis Andrew Davis added a comment - sprint 8 has ended so bringing this along to sprint 9.
              Hide
              andyjdavis 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
              andyjdavis 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
              samhemelryk 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
              samhemelryk 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
              andyjdavis 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
              andyjdavis 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
              andyjdavis 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
              andyjdavis 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
              andyjdavis 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
              andyjdavis 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
              andyjdavis Andrew Davis added a comment - - edited

              PULL-603
              PULL-604

              Show
              andyjdavis Andrew Davis added a comment - - edited PULL-603 PULL-604
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -
              Show
              stronk7 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:
                    Fix Release Date:
                    5/May/11