Details

    • Rank:
      24701

      Description

      Follow http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core

      This is how I see those methods implemented (Fred):

      create_contacts
          Add a contact to the 'address book'
          @param array(int User ID, ...)
          @return warnings
      
      delete_contacts
          Remove a contact to the 'address book'
          @param array(int User ID, ...)
          @return void
      
      block_contacts
          Block a contact
          @param array(int User ID, ...)
          @return warnings
      
      unblock_contacts
          Unblock a contact
          @param  array(int User ID, ...)
          @return void
      
      get_contacts
          List the contacts
          @return array(
              array(
                  online contacts => array(
                      array(
                          user ID,
                          fullname,
                          picture,
                          picture small,
                          unread messages count
                      )
                  ),
                  offline contacts => array(
                      array(
                          user ID,
                          fullname,
                          picture,
                          picture small,
                          unread messages count
                      )
                  ),
                  strangers => array(
                      array(
                          user ID,
                          fullname,
                          picture,
                          picture small,
                          unread messages count
                      )
                  )
              )
          )
      
      search_contacts
          Search for contacts among the users
          @param string name 
          @param bool only in my courses (default false)
          @return array(
                      array(
                          user ID,
                          fullname,
                          picture,
                          picture small
                      )
                  )
      

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Fred. Plenty of ws functions, yum It looks good. Some notes:

          • I learned about compact/extract thanks I'm a bit worry of the extract though, specially when it's recreating a variable with the same name that one that existed before. I think this will be confusing as you'll need to know what's inside. It's ok here for me, but I do know how external lib function work.
          • to be consistent with other ws functions you can do a validate_context on the context user:
            $context = context_user::instance($USER->id);
            self::validate_context($context);
          • you don't need the $extradetails array and doing a merge I think. You can use userdetails straight away. If the image url are empty and need to test, you can test and add it to $newcontact after the declaration. You can use VALUE_OPTIONAL in the return values it's ok, it works with all servers.
          • search_contacts got a query string. However there is no way that I can know without looking at the code. you need to be explicit on how it works in your _parameters() function. In fact I'll keep 'searchtext' as name of the param. It is much clearer. But still add it does a search like %searchtext%.
          • you forgot to declare few the $string['exceptionerrorcode']. It's important to have some nice language text to display to the client dev that just an errorcode. (query_string_cannot_be_empty)
          • obviously the message_search_users call into a foreach is not performance friendly. I'll say in a first time it's ok, if we have performance issue on this function we'll come back to it.
          • in my opinion I would not add restriction to search myself and the guest user in message_search_users(). I don't it doesn't make much sens but it used to work like that, doesn't hurt anyone, it's useful for debugging, and some people use Moodle in different way. I don't think we should hardcode "restriction".

          Cheers.

          Show
          Jérôme Mouneyrac added a comment - Thanks Fred. Plenty of ws functions, yum It looks good. Some notes: I learned about compact/extract thanks I'm a bit worry of the extract though, specially when it's recreating a variable with the same name that one that existed before. I think this will be confusing as you'll need to know what's inside. It's ok here for me, but I do know how external lib function work. to be consistent with other ws functions you can do a validate_context on the context user: $context = context_user::instance($USER->id); self::validate_context($context); you don't need the $extradetails array and doing a merge I think. You can use userdetails straight away. If the image url are empty and need to test, you can test and add it to $newcontact after the declaration. You can use VALUE_OPTIONAL in the return values it's ok, it works with all servers. search_contacts got a query string. However there is no way that I can know without looking at the code. you need to be explicit on how it works in your _parameters() function. In fact I'll keep 'searchtext' as name of the param. It is much clearer. But still add it does a search like %searchtext%. you forgot to declare few the $string ['exceptionerrorcode'] . It's important to have some nice language text to display to the client dev that just an errorcode. (query_string_cannot_be_empty) obviously the message_search_users call into a foreach is not performance friendly. I'll say in a first time it's ok, if we have performance issue on this function we'll come back to it. in my opinion I would not add restriction to search myself and the guest user in message_search_users(). I don't it doesn't make much sens but it used to work like that, doesn't hurt anyone, it's useful for debugging, and some people use Moodle in different way. I don't think we should hardcode "restriction". Cheers.
          Hide
          Frédéric Massart added a comment -

          Thanks for the review Jerome,

          • I removed the use of extract(), I think that was safe to use it here, but I understand the risks of using such a function and the less reading code.
          • I think it is a bit of an overkill to get the context of each user just to check if the IDs are correct, the rest of the code would be executed just fine if those IDs are wrong, and if they're not an, in some cases, a warning will be send.
          • Simplified the $extrafields thing by using VALUE_OPTIONAL.
          • search_contacts parameter renamed.
          • I didn't translate the exception because I wanted to check the message in the PHP Unit, to make sure I go the right exception. But as we cannot/should not use get_string() in Unit Tests, I am just checking if a moodle_exception was raised.
          • I have improved the performances of message_search_users(). It can now receive an array of course IDs.
          • I removed the restriction code and raised MDL-37164 to work on at.

          Pushing for a second review.
          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Thanks for the review Jerome, I removed the use of extract(), I think that was safe to use it here, but I understand the risks of using such a function and the less reading code. I think it is a bit of an overkill to get the context of each user just to check if the IDs are correct, the rest of the code would be executed just fine if those IDs are wrong, and if they're not an, in some cases, a warning will be send. Simplified the $extrafields thing by using VALUE_OPTIONAL. search_contacts parameter renamed. I didn't translate the exception because I wanted to check the message in the PHP Unit, to make sure I go the right exception. But as we cannot/should not use get_string() in Unit Tests, I am just checking if a moodle_exception was raised. I have improved the performances of message_search_users(). It can now receive an array of course IDs. I removed the restriction code and raised MDL-37164 to work on at. Pushing for a second review. Cheers, Fred
          Hide
          Jérôme Mouneyrac added a comment -

          Good, thanks Fred. What's left:

          • you changed the SQL in the case of SITEID when there is more than one value in the array. I'm not sure if it's still work the same as the original. Check it if you didn't (the PHPunit does not check it).
          • you end up with a OR IN OR IN OR IN ... for the same ra.contextid. Look for using get_parent_context_ids() with a single ra.contextid IN .

          Have some great holidays

          Show
          Jérôme Mouneyrac added a comment - Good, thanks Fred. What's left: you changed the SQL in the case of SITEID when there is more than one value in the array. I'm not sure if it's still work the same as the original. Check it if you didn't (the PHPunit does not check it). you end up with a OR IN OR IN OR IN ... for the same ra.contextid. Look for using get_parent_context_ids() with a single ra.contextid IN . Have some great holidays
          Hide
          Jérôme Mouneyrac added a comment -

          You can send it to integration once fixed.

          Show
          Jérôme Mouneyrac added a comment - You can send it to integration once fixed.
          Hide
          Frédéric Massart added a comment -

          Hi Jerome,

          pushing for peer review again.

          • I am now using directly get_parent_context_ids to create a cleaner SQL query.
          • And I changed the logic a bit in messaging so that if the SITEID is passed the search is made on the site level. Grouping a search including courses and the site course was pointless and this reflects more the original behaviour. I also added some checks to be sure that the original behaviour is maintained.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Jerome, pushing for peer review again. I am now using directly get_parent_context_ids to create a cleaner SQL query. And I changed the logic a bit in messaging so that if the SITEID is passed the search is made on the site level. Grouping a search including courses and the site course was pointless and this reflects more the original behaviour. I also added some checks to be sure that the original behaviour is maintained. Cheers, Fred
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Fred, my review:

          • last week, we added clean_returnvalue() calls to all PHPunit tests, so we are simulating web service server. See how to do it in: http://docs.moodle.org/dev/Web_Services_Unit_Test
          • I just checked the your two last points, I agree that it matches current logic and the usage of get_parent_context_ids seems good to me.

          Thank you.

          Show
          Jérôme Mouneyrac added a comment - Hi Fred, my review: last week, we added clean_returnvalue() calls to all PHPunit tests, so we are simulating web service server. See how to do it in: http://docs.moodle.org/dev/Web_Services_Unit_Test I just checked the your two last points, I agree that it matches current logic and the usage of get_parent_context_ids seems good to me. Thank you.
          Hide
          Frédéric Massart added a comment -

          Thanks Jerome. It was a very good idea to user clean_returnvalue as it helped me to find some return description which were not properly defined.

          I have amended my commit and am pushing for integration.

          Cheers!

          Show
          Frédéric Massart added a comment - Thanks Jerome. It was a very good idea to user clean_returnvalue as it helped me to find some return description which were not properly defined. I have amended my commit and am pushing for integration. Cheers!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          1. I'm going to put my integrators foot down on compact(), I just think that it introduces too much 'magic' which is not worth it for the sake of 3 less lines. Being absolutely clear with an assignment is nicer. A principle that Martin has expressed (somewhere, I think) is that moodle code should be clear to understand. The problem with compact() is that we don't use it much and if you don't use it much you won't realise its taking variables outside of what (I would perceive as) its scope.
          2. I'm very suspicious of the passing by reference in the for loop of get_contacts(). We've been tripped up by this before (see: https://moodle.org/local/chatlogs/index.php?conversationid=10167#c352296). Is this necessary, can you explain in a comment why you are doing it? Its taking me a while to understand it by reading throught it. Again better to be clearer than too clever.
          Show
          Dan Poltawski added a comment - Hi Fred, I'm going to put my integrators foot down on compact(), I just think that it introduces too much 'magic' which is not worth it for the sake of 3 less lines. Being absolutely clear with an assignment is nicer. A principle that Martin has expressed (somewhere, I think) is that moodle code should be clear to understand. The problem with compact() is that we don't use it much and if you don't use it much you won't realise its taking variables outside of what (I would perceive as) its scope. I'm very suspicious of the passing by reference in the for loop of get_contacts(). We've been tripped up by this before (see: https://moodle.org/local/chatlogs/index.php?conversationid=10167#c352296 ). Is this necessary, can you explain in a comment why you are doing it? Its taking me a while to understand it by reading throught it. Again better to be clearer than too clever.
          Hide
          Frédéric Massart added a comment -

          Hi Dan,

          I understand that this can be an issue, even though I'm sad because I like compact ! I have nothing to say about the references, I removed them so that it's clearer now.

          Show
          Frédéric Massart added a comment - Hi Dan, I understand that this can be an issue, even though I'm sad because I like compact ! I have nothing to say about the references, I removed them so that it's clearer now.
          Hide
          Dan Poltawski added a comment -

          Thanks fred, i've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks fred, i've integrated this now.
          Hide
          Mark Nelson added a comment -

          Hi Fred, as discussed I get an error when I run the PHPUnit test. You have told me that this is not due to your code, but that I should still fail this test.

          The error is as follows:

          1) core_message_external_testcase::test_send_instant_messages
          dml_transaction_exception: Database transaction error (This code can not be excecuted in transaction)

          /Users/mark/htdocs/mstorage/im/moodle/lib/dml/moodle_database.php:2117
          /Users/mark/htdocs/mstorage/im/moodle/lib/messagelib.php:61
          /Users/mark/htdocs/mstorage/im/moodle/message/lib.php:2057
          /Users/mark/htdocs/mstorage/im/moodle/message/externallib.php:147
          /Users/mark/htdocs/mstorage/im/moodle/message/tests/externallib_test.php:87
          /Users/mark/htdocs/mstorage/im/moodle/lib/phpunit/classes/advanced_testcase.php:76

          Show
          Mark Nelson added a comment - Hi Fred, as discussed I get an error when I run the PHPUnit test. You have told me that this is not due to your code, but that I should still fail this test. The error is as follows: 1) core_message_external_testcase::test_send_instant_messages dml_transaction_exception: Database transaction error (This code can not be excecuted in transaction) /Users/mark/htdocs/mstorage/im/moodle/lib/dml/moodle_database.php:2117 /Users/mark/htdocs/mstorage/im/moodle/lib/messagelib.php:61 /Users/mark/htdocs/mstorage/im/moodle/message/lib.php:2057 /Users/mark/htdocs/mstorage/im/moodle/message/externallib.php:147 /Users/mark/htdocs/mstorage/im/moodle/message/tests/externallib_test.php:87 /Users/mark/htdocs/mstorage/im/moodle/lib/phpunit/classes/advanced_testcase.php:76
          Hide
          Dan Poltawski added a comment -

          Hi,

          I've already reported that problem in: MDL-37086

          Show
          Dan Poltawski added a comment - Hi, I've already reported that problem in: MDL-37086
          Hide
          Dan Poltawski added a comment -

          Sending back to testing.

          Show
          Dan Poltawski added a comment - Sending back to testing.
          Hide
          Mark Nelson added a comment -

          Ok, since that was the only error that occurred and it is not specific to this issue I am passing.

          Show
          Mark Nelson added a comment - Ok, since that was the only error that occurred and it is not specific to this issue I am passing.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: