Moodle

Enrollment Sub-Menu Removes 26th person on list when someone from the first 25 list are added

Details

  • Testing Instructions:
    Hide
    1. Ensure that the site has > 50 users
    2. Create a Course
    3. Make sure that you have manual enrolments set on the course
    4. Go to Course Admin>Users>Enrolled Users
    5. Click the Enrol Users button to bring up the sub menu
    6. Scroll down and show the Next 25
    7. Make Note of who is #26
    8. Close the sub menu
    9. Open the Enrol Users sub menu again
    10. Select user in numbers 1 - 5 (for simplicity sake) and enrol them
    11. 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:
  • Pull 2.4 Branch:
    MDL-37396_m24
  • Pull 2.4 Diff URL:
  • Pull Master Branch:
  • Pull Master Diff URL:

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}
  1. Create a Course
  2. Make sure that you have manual enrolments set on the course
  3. Go to Course Admin>Users>Enrolled Users
  4. Click the Enrol Users button to bring up the sub menu
  5. Scroll down and show the Next 25
  6. Make Note of who is #26 and #51
  7. Close the sub menu
  8. Open the Enrol Users sub menu again
  9. Select user in numbers 3, 26 and 28 and enrol them
  10. 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.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Michael de Raadt added a comment -

Thanks for reporting that.

Feel free to work with us on the issue.

Show
Michael de Raadt added a comment - Thanks for reporting that. Feel free to work with us on the issue.
Hide
Permalink
Rajesh Taneja added a comment -

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 - 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 -

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

Show
Rajesh Taneja added a comment - 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 -

Thanks Raj for reviewing.

Updated the patch to your suggestion.

Submitting for integration review.

Show
Rossiani Wijaya added a comment - Thanks Raj for reviewing. Updated the patch to your suggestion. Submitting for integration review.
Hide
Permalink
Damyon Wiese 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.

Thanks!

Show
Damyon Wiese 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. Thanks!
Hide
Permalink
Rossiani Wijaya added a comment -

Patch rebased.

Show
Rossiani Wijaya added a comment - Patch rebased.
Hide
Permalink
Dan Poltawski added a comment -

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 - 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 -

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 - 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 -

Integrated to master, 24 and 23.

Show
Dan Poltawski added a comment - Integrated to master, 24 and 23.
Hide
Permalink
Andrew Davis added a comment -

Works as described. Passing.

Show
Andrew Davis added a comment - Works as described. Passing.
Hide
Permalink
Damyon Wiese added a comment -

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 - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon

People

Vote (2)
Watch (6)

Dates

  • Created:
    Updated:
    Resolved: