Details

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

      Gliffy Diagrams

        Attachments

          Activity

          Hide
          jerome Jérôme Mouneyrac added a comment -

          Rebased and whitespaces removed. Cheers.

          Show
          jerome Jérôme Mouneyrac added a comment - Rebased and whitespaces removed. Cheers.
          Hide
          jerome 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
          jerome 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
          jerome Jérôme Mouneyrac added a comment -

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

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

          ok I fixed it, re-ready for peer review

          Show
          jerome Jérôme Mouneyrac added a comment - ok I fixed it, re-ready for peer review
          Hide
          andyjdavis 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
          andyjdavis 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
          jerome 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
          jerome 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
          jerome Jérôme Mouneyrac added a comment -

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

          Show
          jerome Jérôme Mouneyrac added a comment - code updated to master, no whitespaces, version bumped, submitting for integration. Cheers.
          Hide
          jerome 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
          jerome 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
          jerome Jérôme Mouneyrac added a comment -

          ok changed committed, all good for integration review.

          Show
          jerome Jérôme Mouneyrac added a comment - ok changed committed, all good for integration review.
          Hide
          samhemelryk 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
          samhemelryk 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
          samhemelryk 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
          samhemelryk 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
          samhemelryk 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
          samhemelryk 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
          samhemelryk 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
          samhemelryk 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
          jerome Jérôme Mouneyrac added a comment -

          fixing it

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

          Thanks Jerome, please comment once it is ready.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Jerome, please comment once it is ready.
          Hide
          jerome 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
          jerome 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
          samhemelryk Sam Hemelryk added a comment -

          Thanks Jerome, this has been integrated now.

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

          tested by tests

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

          This is now upstream, yay! Many thanks!

          Show
          stronk7 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:
                Fix Release Date:
                1/Jul/11