Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-43337

limitfrom, limitnumber addition to get_enrolled_users/get_enrolled_users_with_capability missed a break

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5.4, 2.6.1, 2.7
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: Web Services
    • Labels:

      Description

      'limitfrom' and 'limitnumber' options were added in commit 3ed74dd1 in enrol/externallib.php line 116. This added two cases to switch but only one break; As a result 'userfields' case now misses a break and if userfields option is used in addition to limitfrom the limitfrom will get value 0 every time.

        Gliffy Diagrams

          Activity

          Hide
          mikran Mikko Rantanen added a comment - - edited

          And here is a patch that adds the missing break as well as another one to duplicated code.

          Show
          mikran Mikko Rantanen added a comment - - edited And here is a patch that adds the missing break as well as another one to duplicated code.
          Hide
          cibot CiBoT added a comment -

          Results for MDL-43337

          • Remote repository: git://github.com/apsdehal/moodle.git
          Show
          cibot CiBoT added a comment - Results for MDL-43337 Remote repository: git://github.com/apsdehal/moodle.git Remote branch MDL-43337 _24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1090 Warning: The MDL-43337 _24 branch at git://github.com/apsdehal/moodle.git has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1090/artifact/work/smurf.html Remote branch MDL-43337 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1091 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1091/artifact/work/smurf.html Remote branch MDL-43337 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1092 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1092/artifact/work/smurf.html
          Hide
          fred Frédéric Massart added a comment -

          Hi Amanpreet,

          thanks for submitting branches for this issue. You code is good however it is missing some testing instructions that can prove that your patch is working. What I would suggest is to add some unit tests covering it, then your testing instructions should just be to run the unit tests.

          Here is a sample that should work:

          diff --git a/enrol/tests/externallib_test.php b/enrol/tests/externallib_test.php
          index 073448e..a3bbf0a 100644
          --- a/enrol/tests/externallib_test.php
          +++ b/enrol/tests/externallib_test.php
          @@ -43,6 +43,8 @@ class core_enrol_externallib_testcase extends externallib_advanced_testcase {
                   $course = self::getDataGenerator()->create_course();
                   $user1 = self::getDataGenerator()->create_user();
                   $user2 = self::getDataGenerator()->create_user();
          +        $user3 = self::getDataGenerator()->create_user();
          +        $this->setUser($user3);
           
                   // Set the required capabilities by the external function.
                   $context = context_course::instance($course->id);
          @@ -52,7 +54,7 @@ class core_enrol_externallib_testcase extends externallib_advanced_testcase {
                   // Enrol the users in the course.
                   $this->getDataGenerator()->enrol_user($user1->id, $course->id, $roleid, 'manual');
                   $this->getDataGenerator()->enrol_user($user2->id, $course->id, $roleid, 'manual');
          -        $this->getDataGenerator()->enrol_user($USER->id, $course->id, $roleid, 'manual');
          +        $this->getDataGenerator()->enrol_user($user3->id, $course->id, $roleid, 'manual');
           
                   // Call the external function.
                   $enrolledusers = core_enrol_external::get_enrolled_users($course->id);
          @@ -60,8 +62,25 @@ class core_enrol_externallib_testcase extends externallib_advanced_testcase {
                   // We need to execute the return values cleaning process to simulate the web service server.
                   $enrolledusers = external_api::clean_returnvalue(core_enrol_external::get_enrolled_users_returns(), $enrolledusers);
           
          -        // Check we retrieve the good total number of enrolled users.
          +        // Check the result set.
                   $this->assertEquals(3, count($enrolledusers));
          +        $this->assertArrayHasKey('email', $enrolledusers[0]);
          +
          +        // Call the function with some parameters set.
          +        $enrolledusers = core_enrol_external::get_enrolled_users($course->id, array(
          +            array('name' => 'limitfrom', 'value' => 2),
          +            array('name' => 'limitnumber', 'value' => 1),
          +            array('name' => 'userfields', 'value' => 'id')
          +        ));
          +
          +        // We need to execute the return values cleaning process to simulate the web service server.
          +        $enrolledusers = external_api::clean_returnvalue(core_enrol_external::get_enrolled_users_returns(), $enrolledusers);
          +
          +        // Check the result set, we should only get the 3rd result, which is $user3.
          +        $this->assertCount(1, $enrolledusers);
          +        $this->assertEquals($user3->id, $enrolledusers[0]['id']);
          +        $this->assertArrayHasKey('id', $enrolledusers[0]);
          +        $this->assertArrayNotHasKey('email', $enrolledusers[0]);
           
                   // Call without required capability.
                   $this->unassignUserCapability('moodle/course:viewparticipants', $context->id, $roleid);
          

          Could you also provide a branch for 2.5? The 2.4 branch will not be used as we dropped support for it.

          Many thanks!
          Fred

          Show
          fred Frédéric Massart added a comment - Hi Amanpreet, thanks for submitting branches for this issue. You code is good however it is missing some testing instructions that can prove that your patch is working. What I would suggest is to add some unit tests covering it, then your testing instructions should just be to run the unit tests. Here is a sample that should work: diff --git a/enrol/tests/externallib_test.php b/enrol/tests/externallib_test.php index 073448e..a3bbf0a 100644 --- a/enrol/tests/externallib_test.php +++ b/enrol/tests/externallib_test.php @@ -43,6 +43,8 @@ class core_enrol_externallib_testcase extends externallib_advanced_testcase { $course = self::getDataGenerator()->create_course(); $user1 = self::getDataGenerator()->create_user(); $user2 = self::getDataGenerator()->create_user(); + $user3 = self::getDataGenerator()->create_user(); + $this->setUser($user3); // Set the required capabilities by the external function. $context = context_course::instance($course->id); @@ -52,7 +54,7 @@ class core_enrol_externallib_testcase extends externallib_advanced_testcase { // Enrol the users in the course. $this->getDataGenerator()->enrol_user($user1->id, $course->id, $roleid, 'manual'); $this->getDataGenerator()->enrol_user($user2->id, $course->id, $roleid, 'manual'); - $this->getDataGenerator()->enrol_user($USER->id, $course->id, $roleid, 'manual'); + $this->getDataGenerator()->enrol_user($user3->id, $course->id, $roleid, 'manual'); // Call the external function. $enrolledusers = core_enrol_external::get_enrolled_users($course->id); @@ -60,8 +62,25 @@ class core_enrol_externallib_testcase extends externallib_advanced_testcase { // We need to execute the return values cleaning process to simulate the web service server. $enrolledusers = external_api::clean_returnvalue(core_enrol_external::get_enrolled_users_returns(), $enrolledusers); - // Check we retrieve the good total number of enrolled users. + // Check the result set. $this->assertEquals(3, count($enrolledusers)); + $this->assertArrayHasKey('email', $enrolledusers[0]); + + // Call the function with some parameters set. + $enrolledusers = core_enrol_external::get_enrolled_users($course->id, array( + array('name' => 'limitfrom', 'value' => 2), + array('name' => 'limitnumber', 'value' => 1), + array('name' => 'userfields', 'value' => 'id') + )); + + // We need to execute the return values cleaning process to simulate the web service server. + $enrolledusers = external_api::clean_returnvalue(core_enrol_external::get_enrolled_users_returns(), $enrolledusers); + + // Check the result set, we should only get the 3rd result, which is $user3. + $this->assertCount(1, $enrolledusers); + $this->assertEquals($user3->id, $enrolledusers[0]['id']); + $this->assertArrayHasKey('id', $enrolledusers[0]); + $this->assertArrayNotHasKey('email', $enrolledusers[0]); // Call without required capability. $this->unassignUserCapability('moodle/course:viewparticipants', $context->id, $roleid); Could you also provide a branch for 2.5? The 2.4 branch will not be used as we dropped support for it. Many thanks! Fred
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Results for MDL-43337 Remote repository: git://github.com/apsdehal/moodle.git Remote branch MDL-43337 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1289 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1289/artifact/work/smurf.html Remote branch MDL-43337 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1290 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1290/artifact/work/smurf.html Remote branch MDL-43337 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1291 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1291/artifact/work/smurf.html
          Hide
          fred Frédéric Massart added a comment -

          Hi Amanpreet,

          have you tested the code you sent? I think there would be a fatal error because it's missing a semi colon. Also your comment is missing a white space after // and a period.

          Cheers,
          Fred

          Show
          fred Frédéric Massart added a comment - Hi Amanpreet, have you tested the code you sent? I think there would be a fatal error because it's missing a semi colon. Also your comment is missing a white space after // and a period. Cheers, Fred
          Hide
          apsdehal Amanpreet Singh added a comment -

          Thanks for pointing out.
          I have updated patches.
          Sorry for the inconvenience caused.

          Show
          apsdehal Amanpreet Singh added a comment - Thanks for pointing out. I have updated patches. Sorry for the inconvenience caused.
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Results for MDL-43337 Remote repository: git://github.com/apsdehal/moodle.git Remote branch MDL-43337 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1313 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1313/artifact/work/smurf.html Remote branch MDL-43337 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1314 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1314/artifact/work/smurf.html Remote branch MDL-43337 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1315 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1315/artifact/work/smurf.html
          Hide
          fred Frédéric Massart added a comment -

          Hi Amanpreet,

          I'm afraid there are still some issues with the patch as the Unit Test failed. I guess you do not have PHP Unit set up on your computer, I will fix the patch for you and push for integration. Thanks for your support.

          There was 1 failure:
           
          1) core_enrol_externallib_testcase::test_get_enrolled_users
          Failed asserting that 3 matches expected '5'.
           
          /home/fred/www/repositories/sm/moodle/enrol/tests/externallib_test.php:82
          /home/fred/www/repositories/sm/moodle/lib/phpunit/classes/advanced_testcase.php:80
           
          To re-run:
           vendor/bin/phpunit core_enrol_externallib_testcase enrol/tests/externallib_test.php
          

          Show
          fred Frédéric Massart added a comment - Hi Amanpreet, I'm afraid there are still some issues with the patch as the Unit Test failed. I guess you do not have PHP Unit set up on your computer, I will fix the patch for you and push for integration. Thanks for your support. There was 1 failure:   1) core_enrol_externallib_testcase::test_get_enrolled_users Failed asserting that 3 matches expected '5'.   /home/fred/www/repositories/sm/moodle/enrol/tests/externallib_test.php:82 /home/fred/www/repositories/sm/moodle/lib/phpunit/classes/advanced_testcase.php:80   To re-run: vendor/bin/phpunit core_enrol_externallib_testcase enrol/tests/externallib_test.php
          Hide
          fred Frédéric Massart added a comment -

          Thanks for your help on this Amanpreet. I took the liberty of re-assigning this issue to me in order to push it further. The Unit Tests were actually failing because the bug was not fixed. Indeed, I had missed that you fixed get_enrolled_users_with_capability, but the Unit Tests I had given were for get_enrolled_users.

          I have kept your commit so the credit to you remains, I also added some Unit Tests to prove that your patch works. On top of that I fixed the other method missing a break and added unit tests for those too.

          Considering the number of lines I added to the initial patch I think it's better to send this for peer review rather than integration directly.

          Many thanks!
          Fred

          Show
          fred Frédéric Massart added a comment - Thanks for your help on this Amanpreet. I took the liberty of re-assigning this issue to me in order to push it further. The Unit Tests were actually failing because the bug was not fixed. Indeed, I had missed that you fixed get_enrolled_users_with_capability , but the Unit Tests I had given were for get_enrolled_users . I have kept your commit so the credit to you remains, I also added some Unit Tests to prove that your patch works. On top of that I fixed the other method missing a break and added unit tests for those too. Considering the number of lines I added to the initial patch I think it's better to send this for peer review rather than integration directly. Many thanks! Fred
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Results for MDL-43337 Remote repository: git://github.com/FMCorz/moodle.git Remote branch MDL-43337 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1332 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1332/artifact/work/smurf.html Remote branch MDL-43337 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1333 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1333/artifact/work/smurf.html Remote branch MDL-43337 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1334 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1334/artifact/work/smurf.html
          Hide
          skodak Petr Skoda added a comment -

          Thanks, looks ok to me. The externallib tests need a lot more work generally, thanks for this improvements. Submitting for integration.

          Show
          skodak Petr Skoda added a comment - Thanks, looks ok to me. The externallib tests need a lot more work generally, thanks for this improvements. Submitting for integration.
          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
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated (25, 26 & master), thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (25, 26 & master), thanks!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I claim to be a simple individual
          liable to err like any other fellow mortal.
          I own, however, that I have humility enough
          to confess my errors and to retrace my steps.

          Mahatma Gandhi

          Your awesome code has met upstream, closing, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I claim to be a simple individual liable to err like any other fellow mortal. I own, however, that I have humility enough to confess my errors and to retrace my steps. Mahatma Gandhi Your awesome code has met upstream, closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Mar/14