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

      Gliffy Diagrams

        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: