Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Web Services
    • Labels:
      None
    • Testing Instructions:
      Hide

      Must be tested by a dev:

      Enable the unit test for this function (webservice/simpletest => enable protocol, function and token)

      Then test to:

      • send a message => should success
      • send a message to a user that block you => should fail
      • send a message when the you is set to not receive message from non contact => should fail
      • send a message to a not existing user id => should fail
      Show
      Must be tested by a dev: Enable the unit test for this function (webservice/simpletest => enable protocol, function and token) Then test to: send a message => should success send a message to a user that block you => should fail send a message when the you is set to not receive message from non contact => should fail send a message to a not existing user id => should fail
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-27566-wip
    • Rank:
      17209

      Activity

      Hide
      Jérôme Mouneyrac added a comment -

      Rebased and whitespaces removed. Cheers.

      Show
      Jérôme Mouneyrac added a comment - Rebased and whitespaces removed. Cheers.
      Hide
      Jérôme Mouneyrac added a comment -

      Doh I just noticed I forgot to check if the messaging is enabled Andrew I'll let you finish your peer-review, whatever you find it needs more work

      Show
      Jérôme Mouneyrac added a comment - Doh I just noticed I forgot to check if the messaging is enabled Andrew I'll let you finish your peer-review, whatever you find it needs more work
      Hide
      Jérôme Mouneyrac added a comment -

      I meant the issue needs more work (not your peer-review)

      Show
      Jérôme Mouneyrac added a comment - I meant the issue needs more work (not your peer-review)
      Hide
      Jérôme Mouneyrac added a comment -

      ok I fixed it, re-ready for peer review

      Show
      Jérôme Mouneyrac added a comment - ok I fixed it, re-ready for peer review
      Hide
      Andrew Davis added a comment -

      Clicking 'looks great to me' but there are some minor points you may wish to address.

      In the code checking for blocked users starting line 81 of message/externallib.php, its personal choice I suppose, but personally I find the code is far easier to read and maintain if you use named parameters instead of relying on the order that parameters are added to the $sqlparams array. If you're relying on the order its easy for someone else to accidentally mess that up when they have to alter it in the future.

      Regarding these comments, some minor English tweaks.
      //we are going to do some checkings
      //code should be matching /messages/index.php checks

      They should be:
      we are going to do some checking (or checks)
      code should match /messages/index.php checks

      Show
      Andrew Davis added a comment - Clicking 'looks great to me' but there are some minor points you may wish to address. In the code checking for blocked users starting line 81 of message/externallib.php, its personal choice I suppose, but personally I find the code is far easier to read and maintain if you use named parameters instead of relying on the order that parameters are added to the $sqlparams array. If you're relying on the order its easy for someone else to accidentally mess that up when they have to alter it in the future. Regarding these comments, some minor English tweaks. //we are going to do some checkings //code should be matching /messages/index.php checks They should be: we are going to do some checking (or checks) code should match /messages/index.php checks
      Hide
      Jérôme Mouneyrac added a comment -

      I thought that $DB->get_in_or_equal() didn't support it but it does Thanks for the english tweaks too. I'll update the code and I'll submit for review.

      Show
      Jérôme Mouneyrac added a comment - I thought that $DB->get_in_or_equal() didn't support it but it does Thanks for the english tweaks too. I'll update the code and I'll submit for review.
      Hide
      Jérôme Mouneyrac added a comment -

      code updated to master, no whitespaces, version bumped, submitting for integration. Cheers.

      Show
      Jérôme Mouneyrac added a comment - code updated to master, no whitespaces, version bumped, submitting for integration. Cheers.
      Hide
      Jérôme Mouneyrac added a comment -

      I noticed after submitting that it would have been better to put outside the if($succes):

      if (isset($message['clientmsgid'])) {
          $resultmsg['clientmsgid'] = $message['clientmsgid'];	
      }
      

      please reject if I don't comment below that I fixed it. Thanks.

      Show
      Jérôme Mouneyrac added a comment - I noticed after submitting that it would have been better to put outside the if($succes): if (isset($message['clientmsgid'])) { $resultmsg['clientmsgid'] = $message['clientmsgid']; } please reject if I don't comment below that I fixed it. Thanks.
      Hide
      Jérôme Mouneyrac added a comment -

      ok changed committed, all good for integration review.

      Show
      Jérôme Mouneyrac added a comment - ok changed committed, all good for integration review.
      Hide
      Sam Hemelryk added a comment -

      Hi Jerome,

      I've reviewed this for integration now.
      I spotted several minor things however I think they can be fixed during integration or separate bugs opened for them:

      • In several places you use 8 spaces for indenting rather than 4.
      • send_messages_parameters unused global $CFG
      • send_messages get_user_pref for blocking non contacts should be checked within the if rather than before so that it is only called if needed; A potential performance improvement.

      Once I've checked with Eloy that it is OK for things to be sent upstream this can go in.

      Cheers
      Sam

      Show
      Sam Hemelryk added a comment - Hi Jerome, I've reviewed this for integration now. I spotted several minor things however I think they can be fixed during integration or separate bugs opened for them: In several places you use 8 spaces for indenting rather than 4. send_messages_parameters unused global $CFG send_messages get_user_pref for blocking non contacts should be checked within the if rather than before so that it is only called if needed; A potential performance improvement. Once I've checked with Eloy that it is OK for things to be sent upstream this can go in. Cheers Sam
      Hide
      Sam Hemelryk added a comment -

      Ohh don't forget in the future to fill out the repository and branch fields - it is a big help

      Show
      Sam Hemelryk added a comment - Ohh don't forget in the future to fill out the repository and branch fields - it is a big help
      Hide
      Sam Hemelryk added a comment -

      Ohh don't forget in the future to fill out the repository and branch fields - it is a big help

      Show
      Sam Hemelryk added a comment - Ohh don't forget in the future to fill out the repository and branch fields - it is a big help
      Hide
      Sam Hemelryk added a comment -

      Just thought of one more thing this doesn't presently check to make sure a user has not been deleted so you can send messages to a deleted user...
      I don't know much of the history about user deletion and its limits so I'll check with Martin/Eloy about this before this is integrated.

      Show
      Sam Hemelryk added a comment - Just thought of one more thing this doesn't presently check to make sure a user has not been deleted so you can send messages to a deleted user... I don't know much of the history about user deletion and its limits so I'll check with Martin/Eloy about this before this is integrated.
      Hide
      Jérôme Mouneyrac added a comment -

      fixing it

      Show
      Jérôme Mouneyrac added a comment - fixing it
      Hide
      Sam Hemelryk added a comment -

      Thanks Jerome, please comment once it is ready.

      Show
      Sam Hemelryk added a comment - Thanks Jerome, please comment once it is ready.
      Hide
      Jérôme Mouneyrac added a comment -

      ok fixed

      • removed 8 spaces (netbeans added them during format)
      • removed old $CFG
      • move the get_user_pref into the if condition
      • only retrieve user that are not deleted (just thought we could return an error message for the deleted one, but it will be good like for the moment as time matters )

      Thanks for the review Sam

      Show
      Jérôme Mouneyrac added a comment - ok fixed removed 8 spaces (netbeans added them during format) removed old $CFG move the get_user_pref into the if condition only retrieve user that are not deleted (just thought we could return an error message for the deleted one, but it will be good like for the moment as time matters ) Thanks for the review Sam
      Hide
      Sam Hemelryk added a comment -

      Thanks Jerome, this has been integrated now.

      Show
      Sam Hemelryk added a comment - Thanks Jerome, this has been integrated now.
      Hide
      Eloy Lafuente (stronk7) added a comment -

      tested by tests

      Show
      Eloy Lafuente (stronk7) added a comment - tested by tests
      Hide
      Eloy Lafuente (stronk7) added a comment -

      This is now upstream, yay! Many thanks!

      Show
      Eloy Lafuente (stronk7) added a comment - This is now upstream, yay! Many thanks!

        People

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

          Dates

          • Created:
            Updated:
            Resolved: