Uploaded image for project: '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

          Attachments

            Issue Links

              Activity

              Hide
              nebgor Aparup Banerjee added a comment -

              This looks all good to me

              Show
              nebgor Aparup Banerjee added a comment - This looks all good to me
              Hide
              dongsheng 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 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
              stronk7 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
              stronk7 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
              skodak 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
              skodak 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
              nebgor 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
              nebgor 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
              skodak 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
              skodak 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 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 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
              samhemelryk 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
              samhemelryk 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 Dongsheng Cai added a comment -

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

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

              What is the status of this?

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

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

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

              fixing MDL-31349 at the same time.

              Show
              jerome Jérôme Mouneyrac added a comment - fixing MDL-31349 at the same time.
              Hide
              jerome 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
              jerome 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
              jerome Jérôme Mouneyrac added a comment -

              Sending it to Rosie for peer review

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

              Hi Jerome,

              This looks good.

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

              Show
              rwijaya Rossiani Wijaya added a comment - Hi Jerome, This looks good. Just tiny fixed to add a space after the come for (\'string,string,...\')"
              Hide
              jerome 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
              jerome 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
              stronk7 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
              stronk7 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
              samhemelryk Sam Hemelryk added a comment -

              Thanks guys - has been integrated now

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

              ...thefted the testing...

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

              Test passed thanks Jerome

              Show
              samhemelryk Sam Hemelryk added a comment - Test passed thanks Jerome
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    12/Mar/12