Moodle
  1. Moodle
  2. MDL-29347

core_enrol_get_enrolled_users web service should limit the number of participants

    Details

    • Testing Instructions:
      Hide

      Take the demo REST client and edit it this way:

      <?php
      /**
       * REST client for Moodle 2
       * Return JSON or XML format
       *
       * @authorr Jerome Mouneyrac
       */
       
      /// SETUP - NEED TO BE CHANGED
      $token = 'f95fe8ce5f6a4f01dc24ccdf333bba22';
      $domainname = 'http://jerome.moodle.local/~jerome/Moodle_HEAD';
       
      $functionname = 'core_enrol_get_enrolled_users';
       
      // REST RETURNED VALUES FORMAT
      $restformat = 'json'; //Also possible in Moodle 2.2 and later: 'json'
                           //Setting it to 'json' will fail all calls on earlier Moodle version
       
      //////// moodle_user_create_users ////////
       
      /// PARAMETERS - NEED TO BE CHANGED TO TEST DIFFERENT CASES
      $params = array('courseid' => 2, //CHANGE IT
                      'options' => array(
                                          array('name' => 'limitfrom', 'value' => 5), //CHANGE VALUE, REMOVE OPTION
                                          array('name' => 'limitnumber', 'value' => 4) //CHANGE VALUE, REMOVE OPTION
                                   )
                );
       
      /// REST CALL
      header('Content-Type: text/plain');
      $serverurl = $domainname . '/webservice/rest/server.php'. '?wstoken=' . $token . '&wsfunction='.$functionname;
      require_once('./curl.php');
      $curl = new curl;
      //if rest format == 'xml', then we do not add the param for backward compatibility with Moodle < 2.2
      $restformat = ($restformat == 'json')?'&moodlewsrestformat=' . $restformat:'';
      $resp = $curl->post($serverurl . $restformat, $params);
      print_r($resp);

      Without 'limitfrom' and 'limitnumber' the web service function should return all enrolled users in the course. With limitfrom and limitnumber you should be able to limit the number of returned users.

      Show
      Take the demo REST client and edit it this way: <?php /** * REST client for Moodle 2 * Return JSON or XML format * * @authorr Jerome Mouneyrac */   /// SETUP - NEED TO BE CHANGED $token = 'f95fe8ce5f6a4f01dc24ccdf333bba22'; $domainname = 'http://jerome.moodle.local/~jerome/Moodle_HEAD';   $functionname = 'core_enrol_get_enrolled_users';   // REST RETURNED VALUES FORMAT $restformat = 'json'; //Also possible in Moodle 2.2 and later: 'json' //Setting it to 'json' will fail all calls on earlier Moodle version   //////// moodle_user_create_users ////////   /// PARAMETERS - NEED TO BE CHANGED TO TEST DIFFERENT CASES $params = array('courseid' => 2, //CHANGE IT 'options' => array( array('name' => 'limitfrom', 'value' => 5), //CHANGE VALUE, REMOVE OPTION array('name' => 'limitnumber', 'value' => 4) //CHANGE VALUE, REMOVE OPTION ) );   /// REST CALL header('Content-Type: text/plain'); $serverurl = $domainname . '/webservice/rest/server.php'. '?wstoken=' . $token . '&wsfunction='.$functionname; require_once('./curl.php'); $curl = new curl; //if rest format == 'xml', then we do not add the param for backward compatibility with Moodle < 2.2 $restformat = ($restformat == 'json')?'&moodlewsrestformat=' . $restformat:''; $resp = $curl->post($serverurl . $restformat, $params); print_r($resp); Without 'limitfrom' and 'limitnumber' the web service function should return all enrolled users in the course. With limitfrom and limitnumber you should be able to limit the number of returned users.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      get_users_by_courseid web service should limit the number of participants and it should have `limitnumber` and `limitfrom` options.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Aparup Banerjee added a comment -

            This looks all good to me

            Show
            Aparup Banerjee added a comment - This looks all good to me
            Hide
            Dongsheng Cai added a comment -

            Aparup mentioned the options needs validation to number, but get_recordset_sql will do that, I don't think we need to do that twice.

            Show
            Dongsheng Cai added a comment - Aparup mentioned the options needs validation to number, but get_recordset_sql will do that, I don't think we need to do that twice.
            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
            Petr Skoda added a comment -

            hardcoded 50 default limit looks like a regression - I do not think you can limit the number of users like that

            my -1

            Show
            Petr Skoda added a comment - hardcoded 50 default limit looks like a regression - I do not think you can limit the number of users like that my -1
            Hide
            Aparup Banerjee added a comment -

            Petr, yes i did wonder about that but i thought it was fine as its a web service call (but i'm not too familiar with ws area yet). What sort of regression is possible?

            Show
            Aparup Banerjee added a comment - Petr, yes i did wonder about that but i thought it was fine as its a web service call (but i'm not too familiar with ws area yet). What sort of regression is possible?
            Hide
            Petr Skoda added a comment -

            any change of behaviour is a regression in WS, imagine somebody has 69 users in course and it works fine now, after this change unexpectedly only 50 are returned, weird - no?

            Show
            Petr Skoda added a comment - any change of behaviour is a regression in WS, imagine somebody has 69 users in course and it works fine now, after this change unexpectedly only 50 are returned, weird - no?
            Hide
            Dongsheng Cai added a comment -

            I agree with Petr, we shouldn't change the behavior of web service, so I change default value back to 0?

            Show
            Dongsheng Cai added a comment - I agree with Petr, we shouldn't change the behavior of web service, so I change default value back to 0?
            Hide
            Sam Hemelryk added a comment -

            Hi Dongsheng,

            I'm sending this back this week.

            Petr is spot on - hardcoding that is going to lead to regressions and we certainly need to take extra care in not changing the behaviour of any web services without 100% consideration and documentation.
            Leaving the default as all is fine, and I think adding the limitnumber and limitfrom options are a good idea. However please note that they aren't the only options available to that ws function.
            Within get_users_by_courseid you should add cases to the options switch for them and remove those if's you've added.
            They comment you've added to get_users_by_courseid_parameters is incorrect as well, really this should be expanded out to include all available options.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Dongsheng, I'm sending this back this week. Petr is spot on - hardcoding that is going to lead to regressions and we certainly need to take extra care in not changing the behaviour of any web services without 100% consideration and documentation. Leaving the default as all is fine, and I think adding the limitnumber and limitfrom options are a good idea. However please note that they aren't the only options available to that ws function. Within get_users_by_courseid you should add cases to the options switch for them and remove those if's you've added. They comment you've added to get_users_by_courseid_parameters is incorrect as well, really this should be expanded out to include all available options. Cheers Sam
            Hide
            Dongsheng Cai added a comment -

            I use `moodleapp` label to locate issues related to moodle app

            Show
            Dongsheng Cai added a comment - I use `moodleapp` label to locate issues related to moodle app
            Hide
            moodle.com added a comment -

            What is the status of this?

            Show
            moodle.com added a comment - What is the status of this?
            Hide
            Jérôme Mouneyrac added a comment -

            assigning this issue to myself. This fix would fix the app with Moodle.org.

            Show
            Jérôme Mouneyrac added a comment - assigning this issue to myself. This fix would fix the app with Moodle.org.
            Hide
            Jérôme Mouneyrac added a comment -

            fixing MDL-31349 at the same time.

            Show
            Jérôme Mouneyrac added a comment - fixing MDL-31349 at the same time.
            Hide
            Jérôme Mouneyrac added a comment -

            Apu can you peer review?

            • optional limitfrom/number (I added the clean_param() for educational purpose )
            • I added a description for all options (options should always be described. These infos are displayed in the generated Web service API documentation)
            • I didn't switch off the other other options when limitfrom/number is enabled. I think they can still be used.

            Cheers,
            Jerome

            Show
            Jérôme Mouneyrac added a comment - Apu can you peer review? optional limitfrom/number (I added the clean_param() for educational purpose ) I added a description for all options (options should always be described. These infos are displayed in the generated Web service API documentation) I didn't switch off the other other options when limitfrom/number is enabled. I think they can still be used. Cheers, Jerome
            Hide
            Jérôme Mouneyrac added a comment -

            Sending it to Rosie for peer review

            Show
            Jérôme Mouneyrac added a comment - Sending it to Rosie for peer review
            Hide
            Rossiani Wijaya added a comment -

            Hi Jerome,

            This looks good.

            Just tiny fixed to add a space after the come for (\'string,string,...\')"

            Show
            Rossiani Wijaya added a comment - Hi Jerome, This looks good. Just tiny fixed to add a space after the come for (\'string,string,...\')"
            Hide
            Jérôme Mouneyrac added a comment - - edited

            core_enrol_get_enrolled_users doesn't exist in 2.1, removing the 2.1 affected branch. I didn't test with 2.2 being the same code. Do your test with 2.2 I also put Donhsgeng as author as he started the fix for 2.1 (if I kept his commit, it would have taken more time to resolve the conflicts as it actually needed a port from the previous external function in 2.1 to the new changed external function in 2.2).

            Show
            Jérôme Mouneyrac added a comment - - edited core_enrol_get_enrolled_users doesn't exist in 2.1, removing the 2.1 affected branch. I didn't test with 2.2 being the same code. Do your test with 2.2 I also put Donhsgeng as author as he started the fix for 2.1 (if I kept his commit, it would have taken more time to resolve the conflicts as it actually needed a port from the previous external function in 2.1 to the new changed external function in 2.2).
            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
            Sam Hemelryk added a comment -

            Thanks guys - has been integrated now

            Show
            Sam Hemelryk added a comment - Thanks guys - has been integrated now
            Hide
            Sam Hemelryk added a comment -

            ...thefted the testing...

            Show
            Sam Hemelryk added a comment - ...thefted the testing...
            Hide
            Sam Hemelryk added a comment -

            Test passed thanks Jerome

            Show
            Sam Hemelryk added a comment - Test passed thanks Jerome
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

            icao_reverse('arreis olik rebemevon afla letoh ognat');
            

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: