Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 2.5.4, 2.6.1, 2.7
-
Component/s: Web Services
-
Testing Instructions:
-
Affected Branches:MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
-
Fixed Branches:MOODLE_25_STABLE, MOODLE_26_STABLE
-
Pull from Repository:
-
Pull Master Branch:
MDL-43337-master -
Pull Master Diff URL:
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
Attachments
Activity
- All
- Comments
- Work Log
- History
- Activity
- Links Hierarchy
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
- Remote branch
MDL-43337to be integrated into upstream master
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
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
Thanks for pointing out.
I have updated patches.
Sorry for the inconvenience caused.
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
|
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
Thanks, looks ok to me. The externallib tests need a lot more work generally, thanks for this improvements. Submitting for integration.
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 
Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
Integrated (25, 26 & master), thanks!
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!
And here is a patch that adds the missing break as well as another one to duplicated code.