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:
    • Rank:
      19058

      Description

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

        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 Škoda 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 Škoda 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 Škoda 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 Škoda 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: