Moodle
  1. Moodle
  2. MDL-30126

Our rest client should be updated fo json support

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3
    • Fix Version/s: 2.2.2
    • Component/s: Web Services
    • Labels:
    • Rank:
      24782

      Issue Links

        Activity

        Jérôme Mouneyrac created issue -
        Jérôme Mouneyrac made changes -
        Field Original Value New Value
        Pull Master Diff URL https://github.com/mouneyrac/moodle/compare/master...MDL-30126
        Pull Master Branch MDL-30126
        Fix Version/s DEV backlog [ 10464 ]
        Pull from Repository git://github.com/mouneyrac/moodle.git
        Jérôme Mouneyrac made changes -
        Status Open [ 1 ] Waiting for peer review [ 10012 ]
        Peer reviewer nebgor
        Michael de Raadt made changes -
        Labels triaged
        Aparup Banerjee made changes -
        Original Estimate 0 minutes [ 0 ]
        Remaining Estimate 0 minutes [ 0 ]
        Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
        Hide
        Aparup Banerjee added a comment -

        hi, this looks fine to me.
        I have noted however that we're back to building urls manually when we have the moodle_url to build urls with. since you're in the area, Jerome, you might want to consider cleaning that up.

        Show
        Aparup Banerjee added a comment - hi, this looks fine to me. I have noted however that we're back to building urls manually when we have the moodle_url to build urls with. since you're in the area, Jerome, you might want to consider cleaning that up.
        Aparup Banerjee made changes -
        Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
        Jérôme Mouneyrac made changes -
        Link This issue has a non-specific relationship to MDL-31253 [ MDL-31253 ]
        Jérôme Mouneyrac made changes -
        Link This issue blocks MDL-30210 [ MDL-30210 ]
        Hide
        Jérôme Mouneyrac added a comment -

        Hi Apu, thanks for the peer-review. What do you mean by clean up the aera? In the meantime I submit this issue for integration. If something else need to be done we can still open a new issue. I would need this issue integrated to make the web service unit test work with the REST server

        Show
        Jérôme Mouneyrac added a comment - Hi Apu, thanks for the peer-review. What do you mean by clean up the aera? In the meantime I submit this issue for integration. If something else need to be done we can still open a new issue. I would need this issue integrated to make the web service unit test work with the REST server
        Jérôme Mouneyrac made changes -
        Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
        Jérôme Mouneyrac made changes -
        Testing Instructions Go to MDL-30210, cherry-pick the Github patch. In webservice/simpletest/testwebservice.php enable the REST protocol and at least one test function. Run the web service unit test in Moodle admin. It should work. Previously we could not test any function with the REST protocol.
        Hide
        Aparup Banerjee added a comment -

        ah i meant in the constructor, $serverurl is a moodle_url so

        $result = download_file_content($this->serverurl. '?wstoken='.$this->token. $formatparam . '&wsfunction='. $functionname, null, $params);
        

        could be

         if ($this->format == 'json') {
          $this->serverurl->param('moodlewsrestformat','json');
         }
         $this->serverurl->param('wstoken',$this->token);
         $this->serverurl->param('wsfunction',$functionname); //you could also use params().
        
         $result = download_file_content($this->serverurl, null, $params);
        
        Show
        Aparup Banerjee added a comment - ah i meant in the constructor, $serverurl is a moodle_url so $result = download_file_content($ this ->serverurl. '?wstoken='.$ this ->token. $formatparam . '&wsfunction='. $functionname, null , $params); could be if ($ this ->format == 'json') { $ this ->serverurl->param('moodlewsrestformat','json'); } $ this ->serverurl->param('wstoken',$ this ->token); $ this ->serverurl->param('wsfunction',$functionname); //you could also use params(). $result = download_file_content($ this ->serverurl, null , $params);
        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
        Eloy Lafuente (stronk7) made changes -
        Currently in integration Yes [ 10041 ]
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Ho, I'm integrating this but agree with Aparup that the urls should be constructed using our moodle_url class. Please consider filling one issue about that.

        Show
        Eloy Lafuente (stronk7) added a comment - Ho, I'm integrating this but agree with Aparup that the urls should be constructed using our moodle_url class. Please consider filling one issue about that.
        Eloy Lafuente (stronk7) made changes -
        Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
        Integrator stronk7
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Big Q!

        This seems to be one improvement for master only, but is blocking currently to MDL-30210 (where both the 22 and master patches are the same).

        So... should this also be backported to 22 ? Plz, illustrate us, TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Big Q! This seems to be one improvement for master only, but is blocking currently to MDL-30210 (where both the 22 and master patches are the same). So... should this also be backported to 22 ? Plz, illustrate us, TIA and ciao
        Hide
        Jérôme Mouneyrac added a comment -

        Ah yes it should be in 2.2. I'll backport it and apply the suggested fix during my next ws week

        Show
        Jérôme Mouneyrac added a comment - Ah yes it should be in 2.2. I'll backport it and apply the suggested fix during my next ws week
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Sorry but... does that mean that we must halt integration of both this and MDL-30210 till the next ws week ?

        Or that we should apply both this and MDL-30210 only to master?

        Or apply this only to master and MDL-30210 to all the branches (knowing in will fail-testing under 22_STABLE)?

        Please clarify, I'm a bit lost right now (surely a good moment to go sleep a bit, hehe). TIA!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Sorry but... does that mean that we must halt integration of both this and MDL-30210 till the next ws week ? Or that we should apply both this and MDL-30210 only to master? Or apply this only to master and MDL-30210 to all the branches (knowing in will fail-testing under 22_STABLE)? Please clarify, I'm a bit lost right now (surely a good moment to go sleep a bit, hehe). TIA! Ciao
        Hide
        Jérôme Mouneyrac added a comment -

        Ah sorry Eloy Send me back both of them to development, I'll send them back to you next week. It's easier for you I guess.

        Show
        Jérôme Mouneyrac added a comment - Ah sorry Eloy Send me back both of them to development, I'll send them back to you next week. It's easier for you I guess.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        (sending back to integration, as agreed, thanks!)

        Show
        Eloy Lafuente (stronk7) added a comment - (sending back to integration, as agreed, thanks!)
        Eloy Lafuente (stronk7) made changes -
        Status Integration review in progress [ 10004 ] Reopened [ 4 ]
        Eloy Lafuente (stronk7) made changes -
        Currently in integration Yes [ 10041 ]
        Jérôme Mouneyrac made changes -
        Hide
        Jérôme Mouneyrac added a comment -

        Fixed resubmitted.

        Show
        Jérôme Mouneyrac added a comment - Fixed resubmitted.
        Jérôme Mouneyrac made changes -
        Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
        Hide
        Jérôme Mouneyrac added a comment -

        The rest server doesn't support JSON in 2.1. Anyway only if a developer set the REST protocol to true in the web service unit test file, then the tests will fail. When settings it up a message indacte this issue and the dev will understand that REST unit test doesn't work on 2.1.

        Show
        Jérôme Mouneyrac added a comment - The rest server doesn't support JSON in 2.1. Anyway only if a developer set the REST protocol to true in the web service unit test file, then the tests will fail. When settings it up a message indacte this issue and the dev will understand that REST unit test doesn't work on 2.1.
        Eloy Lafuente (stronk7) made changes -
        Currently in integration Yes [ 10041 ]
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Oki, integrating this for 22 and master... thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Oki, integrating this for 22 and master... thanks!
        Eloy Lafuente (stronk7) made changes -
        Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks (22 and master).

        Now I'm going to integrate and test MDL-30210 that will cause this to pass (or no).

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks (22 and master). Now I'm going to integrate and test MDL-30210 that will cause this to pass (or no). Ciao
        Eloy Lafuente (stronk7) made changes -
        Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
        Affects Version/s 2.3 [ 10657 ]
        Fix Version/s 2.2.2 [ 11552 ]
        Fix Version/s DEV backlog [ 10464 ]
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Uhm,

        I've got some problems running MDL-30210 (all protocols and functions enabled).

        Then I've edited webservice/simpletest/testwebservice.php, enabling only REST and 3 functions (moodle_course_get_courses, moodle_user_get_users_by_id, core_enrol_get_enrolled_users).

        And running unit-tests under /webservice I get:

        1/1 test cases complete: 53 passes, 0 fails and 0 exceptions.

        Is that enough to consider the test passed?

        Show
        Eloy Lafuente (stronk7) added a comment - Uhm, I've got some problems running MDL-30210 (all protocols and functions enabled). Then I've edited webservice/simpletest/testwebservice.php, enabling only REST and 3 functions (moodle_course_get_courses, moodle_user_get_users_by_id, core_enrol_get_enrolled_users). And running unit-tests under /webservice I get: 1/1 test cases complete: 53 passes, 0 fails and 0 exceptions. Is that enough to consider the test passed?
        Eloy Lafuente (stronk7) made changes -
        Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
        Tester stronk7
        Hide
        Jérôme Mouneyrac added a comment - - edited

        Sorry Eloy for making you test the unit test. I changed the testing instruction to make the issue not depend of the clunky unit test

        Show
        Jérôme Mouneyrac added a comment - - edited Sorry Eloy for making you test the unit test. I changed the testing instruction to make the issue not depend of the clunky unit test
        Jérôme Mouneyrac made changes -
        Testing Instructions Go to MDL-30210, cherry-pick the Github patch. In webservice/simpletest/testwebservice.php enable the REST protocol and at least one test function. Run the web service unit test in Moodle admin. It should work. Previously we could not test any function with the REST protocol. Create a Moodle script and use this Moodle REST-JSON client. You can take example of the REST-JSON demo client (https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST) to see how to use it.
        Jérôme Mouneyrac made changes -
        Testing Instructions Create a Moodle script and use this Moodle REST-JSON client. You can take example of the REST-JSON demo client (https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST) to see how to use it. Create a Moodle script and use the Moodle REST-JSON client: /webservice/rest/lib.php:webservice_rest_client. You can take example of the REST-JSON demo client (https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST) to see how to use it.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Nice, thanks, that will help to close this... trying...

        Show
        Eloy Lafuente (stronk7) added a comment - Nice, thanks, that will help to close this... trying...
        Hide
        Jérôme Mouneyrac added a comment -

        If it's too long to test you can send it to someone else for testing, or close it. I tested it multiple times with multiple functions.

        Show
        Jérôme Mouneyrac added a comment - If it's too long to test you can send it to someone else for testing, or close it. I tested it multiple times with multiple functions.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Passed, I've tested json rest with both:

        • admin/webservice/testclient.php: Hacking it a bit to specify json format, and 3-4 random functions (there are a bunch missing, surely because of they missing its "form" definition or so). All them returned proper json.
        • sample-ws-clients/PHP-REST: By configuring it and execute some "core" functions, it also worked ok, both with formats being xml and json.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Passed, I've tested json rest with both: admin/webservice/testclient.php: Hacking it a bit to specify json format, and 3-4 random functions (there are a bunch missing, surely because of they missing its "form" definition or so). All them returned proper json. sample-ws-clients/PHP-REST: By configuring it and execute some "core" functions, it also worked ok, both with formats being xml and json. Ciao
        Eloy Lafuente (stronk7) made changes -
        Status Testing in progress [ 10011 ] Tested [ 10006 ]
        Hide
        Eloy Lafuente (stronk7) added a comment -

        It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

        Closing as fixed, heading to zzzZZZzzz, niao

        Show
        Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao
        Eloy Lafuente (stronk7) made changes -
        Status Tested [ 10006 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Currently in integration Yes [ 10041 ]
        Eloy Lafuente (stronk7) made changes -
        Integration date 17/Feb/12

          People

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

            Dates

            • Created:
              Updated:
              Resolved: