[MDL-37396] Enrollment Sub-Menu Removes 26th person on list when someone from the first 25 list are added - Moodle Tracker

Details

Type: Bug
Status: Closed
Priority: Major
Resolution: Fixed
Affects Version/s: 2.3.3
Fix Version/s: 2.3.5, 2.4.2
Component/s: Course, Enrolments
Labels:

Testing Instructions:
Hide

Ensure that the site has > 50 users

Create a Course

Make sure that you have manual enrolments set on the course

Go to Course Admin>Users>Enrolled Users

Click the Enrol Users button to bring up the sub menu

Scroll down and show the Next 25

Make Note of who is #26

Close the sub menu

Open the Enrol Users sub menu again

Select user in numbers 1 - 5 (for simplicity sake) and enrol them

Scroll down in the menu and click Next 25

Make sure user #26 is there and the list didn't skip any user.
Show
Ensure that the site has > 50 users Create a Course Make sure that you have manual enrolments set on the course Go to Course Admin>Users>Enrolled Users Click the Enrol Users button to bring up the sub menu Scroll down and show the Next 25 Make Note of who is #26 Close the sub menu Open the Enrol Users sub menu again Select user in numbers 1 - 5 (for simplicity sake) and enrol them Scroll down in the menu and click Next 25 Make sure user #26 is there and the list didn't skip any user.
Affected Branches:
MOODLE_23_STABLE
Fixed Branches:
MOODLE_23_STABLE, MOODLE_24_STABLE
Pull from Repository:
git://github.com/rwijaya/moodle.git
Pull Master Branch:
~~MDL-37396~~
Pull Master Diff URL:
https://github.com/rwijaya/moodle/compare/master...MDL-37396

Description

When you go to the enrol users submenu, after you enrol someone within the first 25 on the list and then go to Next 25, the person who was #26 on the list is missing and the others are pushed up (in that #26 is now really #27 from before.)

Replication steps:

{Ensure that the site you are testing this in has > 50 users}

Create a Course
Make sure that you have manual enrolments set on the course
Go to Course Admin>Users>Enrolled Users
Click the Enrol Users button to bring up the sub menu
Scroll down and show the Next 25
Make Note of who is #26 and #51
Close the sub menu
Open the Enrol Users sub menu again
Select user in numbers 3, 26 and 28 and enrol them
Scroll down in the menu and click Next 25

Actual result: The #26 person who was noted earlier is not there any more.

Expected result: User #26 and #51 should be there.

Gliffy Diagrams

Attachments

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Michael de Raadt added a comment - 09/Jan/13 1:31 AM

Thanks for reporting that.

Feel free to work with us on the issue.

Show

Michael de Raadt added a comment - 09/Jan/13 1:31 AM Thanks for reporting that. Feel free to work with us on the issue.

Hide

Permalink

Rajesh Taneja added a comment - 12/Feb/13 7:35 AM

Patch looks good Rossie,

Please add phpdocs before pushing it for integration.

FYI:
This can be achieved by modifying page and perpage in JS, depending on enrolcount. But seems your approach is much cleaner.

Show

Rajesh Taneja added a comment - 12/Feb/13 7:35 AM Patch looks good Rossie, Please add phpdocs before pushing it for integration. FYI: This can be achieved by modifying page and perpage in JS, depending on enrolcount. But seems your approach is much cleaner.

Hide

Permalink

Rajesh Taneja added a comment - 12/Feb/13 8:06 AM

Note: quickenrolment.js search call 25 next values (hard-coded), so it's safe to set perpage=25.

Show

Rajesh Taneja added a comment - 12/Feb/13 8:06 AM Note: quickenrolment.js search call 25 next values (hard-coded), so it's safe to set perpage=25.

Hide

Permalink

Rossiani Wijaya added a comment - 12/Feb/13 9:21 AM

Thanks Raj for reviewing.

Updated the patch to your suggestion.

Submitting for integration review.

Show

Rossiani Wijaya added a comment - 12/Feb/13 9:21 AM Thanks Raj for reviewing. Updated the patch to your suggestion. Submitting for integration review.

Hide

Permalink

Damyon Wiese added a comment - 14/Feb/13 7:49 AM

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.

Thanks!

Show

Damyon Wiese added a comment - 14/Feb/13 7:49 AM 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. Thanks!

Hide

Permalink

Rossiani Wijaya added a comment - 18/Feb/13 2:27 AM

Patch rebased.

Show

Rossiani Wijaya added a comment - 18/Feb/13 2:27 AM Patch rebased.

Hide

Permalink

Dan Poltawski added a comment - 18/Feb/13 4:21 AM

Hi Rosie,

Wow, this is a tricky issue - seems like you've come up with a neat solution.

Got two comments:

Please can we pass the $perpage value as a param. I take the point that its hardcoded in the JS file, but now you are hardcoding it in two places rather than one. It looks like it'll be very easy to just pass this as a hardcoded param from the js file which will be neater. (You can even make it default to 25 in the optional_param)

Can you add testing instructions (and can you test to ensure this works for) more than one page of paging (i.e. user number 51 after use 3 and 26,28 have been enrolled etc)

thanks

Show

Dan Poltawski added a comment - 18/Feb/13 4:21 AM Hi Rosie, Wow, this is a tricky issue - seems like you've come up with a neat solution. Got two comments: Please can we pass the $perpage value as a param. I take the point that its hardcoded in the JS file, but now you are hardcoding it in two places rather than one. It looks like it'll be very easy to just pass this as a hardcoded param from the js file which will be neater. (You can even make it default to 25 in the optional_param) Can you add testing instructions (and can you test to ensure this works for) more than one page of paging (i.e. user number 51 after use 3 and 26,28 have been enrolled etc) thanks

Hide

Permalink

Rossiani Wijaya added a comment - 18/Feb/13 5:44 AM

Hi Dan,

Thanks for the comments.

I made the changes according to your suggestion and also created perpage variable with 25 as the default value within the JS file.

Sending this back for integration review.

Show

Rossiani Wijaya added a comment - 18/Feb/13 5:44 AM Hi Dan, Thanks for the comments. I made the changes according to your suggestion and also created perpage variable with 25 as the default value within the JS file. Sending this back for integration review.

Hide

Permalink

Dan Poltawski added a comment - 18/Feb/13 6:28 AM

Integrated to master, 24 and 23.

Show

Dan Poltawski added a comment - 18/Feb/13 6:28 AM Integrated to master, 24 and 23.

Hide

Permalink

Andrew Davis added a comment - 20/Feb/13 3:40 AM

Works as described. Passing.

Show

Andrew Davis added a comment - 20/Feb/13 3:40 AM Works as described. Passing.

Hide

Permalink

Damyon Wiese added a comment - 22/Feb/13 3:59 AM

Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

Regards, Damyon

Show

Damyon Wiese added a comment - 22/Feb/13 3:59 AM Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon

People

Assignee:

Rossiani Wijaya

Reporter:

Kevin McCarthy

Peer reviewer:

Rajesh Taneja

Integrator:

Dan Poltawski

Tester:

Andrew Davis

Participants:

Andrew Davis, Damyon Wiese, Dan Poltawski, Kevin McCarthy, Michael de Raadt, Rajesh Taneja, Rossiani Wijaya

Votes:

2 Vote for this issue

Watchers:

5 Start watching this issue

Dates

Created:

07/Jan/13 3:10 PM

Updated:

30/Sep/13 1:15 PM

Resolved:

22/Feb/13 3:59 AM

Fix Release Date:

11/Mar/13