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

      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

          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: