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 Master Branch:
    • 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

        Hide
        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
        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
        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
        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
        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
        Rossiani Wijaya added a comment -

        Patch rebased.

        Show
        Rossiani Wijaya added a comment - Patch rebased.
        Hide
        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
        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
        Dan Poltawski added a comment -

        Integrated to master, 24 and 23.

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

        Works as described. Passing.

        Show
        Andrew Davis added a comment - Works as described. Passing.
        Hide
        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: