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

I think it's more readable now

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

Sam

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

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

Eloy

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

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

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.

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

This has been integrated, thanks!

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

Please could we have some testing instructions for this issue.

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.

Upstream-ized! Thanks!