Moodle
  1. Moodle
  2. MDL-37396

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:
        • Rank:
          47019

          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

              • Votes:
                2 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: