Moodle
  1. Moodle
  2. MDL-27936

send_message webservice doesn't respect contacts settings

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Web Services
    • Labels:
      None
    • Testing Instructions:
      Hide

      This issue needs a developer to test.
      1. download the xmlrpc.php file from this issue, copy it to moodle dirroot
      2. modify the token and userid in source code, then run the test

      Show
      This issue needs a developer to test. 1. download the xmlrpc.php file from this issue, copy it to moodle dirroot 2. modify the token and userid in source code, then run the test
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
      git@github.com:dongsheng/moodle.git
    • Pull Master Branch:
      s11_MDL-27936_send_message_master
    • Rank:
      17580

      Description

      send_message webservice doesn't respect contacts settings

        Activity

        Hide
        Sam Hemelryk added a comment -

        Hi DS,

        Are you sure that second query to non blocked contacts is correct?
        Would it be possible to select all contacts - blocked or not and then iterate over them and build a separate array of blocked contacts?

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi DS, Are you sure that second query to non blocked contacts is correct? Would it be possible to select all contacts - blocked or not and then iterate over them and build a separate array of blocked contacts? Cheers Sam
        Hide
        Dongsheng Cai added a comment -

        I think it's more readable now

        Show
        Dongsheng Cai added a comment - I think it's more readable now
        Hide
        Sam Hemelryk added a comment -

        Hi DS,

        Looks good to me!
        Only thing I spotted that I'm not a fan of is the use of $USER->id directly within the SQL query.
        Really it needs to be a proper parameter as well.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi DS, Looks good to me! Only thing I spotted that I'm not a fan of is the use of $USER->id directly within the SQL query. Really it needs to be a proper parameter as well. Cheers Sam
        Hide
        Dongsheng Cai added a comment -

        Sam

        I fixed $USER->id parameter, and rebased.

        Show
        Dongsheng Cai added a comment - Sam I fixed $USER->id parameter, and rebased.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Some comments (some directly related to this and some not but with messaging in general:

        1) unrelated: message_post_message() that is such a horrible name for one forced "instantmessage" sender function
        2) unrelated: while reviewing this I've looked a bunch of code at message/lib.php that, wow, has surprised me a lot, like the fact of being storing ALL the messages sent in the message or message_read tables always. IMO only if some processor needs local storage (popup) that should be used, but the multiplicity effect when sending forum posts or other "multiple destination" providers can be really, really huge! (unless I'm missing something that is highly probable).
        3) related: This webservice should be called send_instantmessage() not sent_message().
        4) related: the service does a lot of checks for instantmessages (contacts, blocks...) and that's ok... but does not check other higher level things like $CFG->messaging (return exception) or what happens if the target user has all processors for instantmessage disabled. I know message_send() does that at a lower level, just guessing which one should be the returned information by this service if any of that happens.
        5) related: unittests covering this?

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Some comments (some directly related to this and some not but with messaging in general: 1) unrelated: message_post_message() that is such a horrible name for one forced "instantmessage" sender function 2) unrelated: while reviewing this I've looked a bunch of code at message/lib.php that, wow, has surprised me a lot, like the fact of being storing ALL the messages sent in the message or message_read tables always. IMO only if some processor needs local storage (popup) that should be used, but the multiplicity effect when sending forum posts or other "multiple destination" providers can be really, really huge! (unless I'm missing something that is highly probable). 3) related: This webservice should be called send_instantmessage() not sent_message(). 4) related: the service does a lot of checks for instantmessages (contacts, blocks...) and that's ok... but does not check other higher level things like $CFG->messaging (return exception) or what happens if the target user has all processors for instantmessage disabled. I know message_send() does that at a lower level, just guessing which one should be the returned information by this service if any of that happens. 5) related: unittests covering this? Ciao
        Hide
        Dongsheng Cai added a comment -

        Eloy

        Are you suggest to fix 3) 4) 5) in this issue?

        Show
        Dongsheng Cai added a comment - Eloy Are you suggest to fix 3) 4) 5) in this issue?
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Well, as far as 3) is a public name I really thing we must rename it to better one, yes.

        About 4) I really don't know how is this expected to behave. If messaging is disabled in the server or if the target user has all processors disabled... which should be the reply sent to the WS consumer? exception? meaningful error (like happens when the contact checks aren't succesful?). I mean all return situations should be clear, not exception sometimes, proper error others...

        And 5) is about the lack of testing instructions, or existing MDLQA to handle this or existence of unit tests about it. Without any of that information available, who is going to test-pass this, just that is reason to avoid integrating the issue? Ideally we should have all the WS + externallib appliances 100% covered with unit tests IMO. Surely we cannot achieve that before release (1-2 days) but should be one of the main goals for 2.1.1.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Well, as far as 3) is a public name I really thing we must rename it to better one, yes. About 4) I really don't know how is this expected to behave. If messaging is disabled in the server or if the target user has all processors disabled... which should be the reply sent to the WS consumer? exception? meaningful error (like happens when the contact checks aren't succesful?). I mean all return situations should be clear, not exception sometimes, proper error others... And 5) is about the lack of testing instructions, or existing MDLQA to handle this or existence of unit tests about it. Without any of that information available, who is going to test-pass this, just that is reason to avoid integrating the issue? Ideally we should have all the WS + externallib appliances 100% covered with unit tests IMO. Surely we cannot achieve that before release (1-2 days) but should be one of the main goals for 2.1.1. Ciao
        Hide
        Dongsheng Cai added a comment -

        Hi, Eloy

        I renamed send_messages to moodle_message_send_instant_messages, created MDL-28089 to cover unit test later.

        About 4, I am not sure what the correct behavior, I saw plenty exceptions in moodle services elsewhere, when I use xmlrpc, I can capture the exception by checking faultSting fields in xml payload. I suggest we review all webservice implementation after 2.1. Decide the proper methods to handle errors and exceptions.

        Show
        Dongsheng Cai added a comment - Hi, Eloy I renamed send_messages to moodle_message_send_instant_messages, created MDL-28089 to cover unit test later. About 4, I am not sure what the correct behavior, I saw plenty exceptions in moodle services elsewhere, when I use xmlrpc, I can capture the exception by checking faultSting fields in xml payload. I suggest we review all webservice implementation after 2.1. Decide the proper methods to handle errors and exceptions.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Oki, this will be integrated, thanks for renaming and creating MDL-28089.

        About the returned info/exceptions... perhaps it would be a good candidate for another new issue, and check how we are doing for all WS layers and also how that is implemeted by all the currently available services.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Oki, this will be integrated, thanks for renaming and creating MDL-28089 . About the returned info/exceptions... perhaps it would be a good candidate for another new issue, and check how we are doing for all WS layers and also how that is implemeted by all the currently available services. Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This has been integrated, thanks!

        I got minor conflicts but I think they were resolved ok.

        Show
        Eloy Lafuente (stronk7) added a comment - This has been integrated, thanks! I got minor conflicts but I think they were resolved ok.
        Hide
        Helen Foster added a comment -

        Please could we have some testing instructions for this issue.

        Show
        Helen Foster added a comment - Please could we have some testing instructions for this issue.
        Hide
        Glenn Ansley added a comment -

        Ok. It took a while to confirm this one because I had to install the xmlrpc extension on MAMP. Then I had to figure out how web services / messaging worked, then I had to figure out that messages aren't sent by default if the user is logged in... lots of trial and error but I can confirm the following:

        • Messages are not sent if user has blocked sender (unless the sender is admin
        • Messages are not sent if user is logged in and does not have 'send email for messages when logged in' checked.
        • Messages are send it opposite of above.

        Looks good.

        Show
        Glenn Ansley added a comment - Ok. It took a while to confirm this one because I had to install the xmlrpc extension on MAMP. Then I had to figure out how web services / messaging worked, then I had to figure out that messages aren't sent by default if the user is logged in... lots of trial and error but I can confirm the following: Messages are not sent if user has blocked sender (unless the sender is admin Messages are not sent if user is logged in and does not have 'send email for messages when logged in' checked. Messages are send it opposite of above. Looks good.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Upstream-ized! Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Upstream-ized! Thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: