Mikko Rantanen added a comment - 11/Dec/13 8:53 AM - edited And here is a patch that adds the missing break as well as another one to duplicated code.

CiBoT added a comment - 04/Feb/14 4:57 PM 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

Frédéric Massart added a comment - 06/Feb/14 7:53 AM 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

CiBoT added a comment - 11/Feb/14 8:57 PM 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

Frédéric Massart added a comment - 12/Feb/14 10:16 AM 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

Amanpreet Singh added a comment - 12/Feb/14 10:54 AM Thanks for pointing out. I have updated patches. Sorry for the inconvenience caused.

CiBoT added a comment - 12/Feb/14 11:26 AM 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

Frédéric Massart added a comment - 13/Feb/14 4:35 AM 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

Frédéric Massart added a comment - 13/Feb/14 5:30 AM 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

CiBoT added a comment - 13/Feb/14 5:57 AM 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

Petr Skoda added a comment - 13/Feb/14 6:32 AM Thanks, looks ok to me. The externallib tests need a lot more work generally, thanks for this improvements. Submitting for integration.

Eloy Lafuente (stronk7) added a comment - 13/Feb/14 4:47 PM 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

CiBoT added a comment - 16/Feb/14 9:51 PM Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

Eloy Lafuente (stronk7) added a comment - 18/Feb/14 11:51 PM Integrated (25, 26 & master), thanks!

Eloy Lafuente (stronk7) added a comment - 20/Feb/14 8:03 PM 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!