Moodle
  1. Moodle
  2. MDL-38332

Site admin - Browse users does not paginate properly for multiples of 30 users

    Details

    • Testing Instructions:
      Hide
      1. Set up a site so that it has exactly 30 users
      2. Go to Site admin > users > browse list of users
      3. Make sure 30 users are shown and there is NO pagination option
      4. Add an additional user
      5. Now there is a link to show 'Page 2' and all 31 users are visible
      6. delete a user (back to 30) and make sure all 30 users are visible.
      Show
      Set up a site so that it has exactly 30 users Go to Site admin > users > browse list of users Make sure 30 users are shown and there is NO pagination option Add an additional user Now there is a link to show 'Page 2' and all 31 users are visible delete a user (back to 30) and make sure all 30 users are visible.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      wip-mdl-38332-m24
    • Pull Master Branch:
      wip-mdl-38332
    • Rank:
      48206

      Description

      To reproduce...

      • Set up a site so that it has exactly 30 users
      • Go to Site admin > users > browse list of users
      • Only 29 are shown and there is NO pagination option
      • Add an additional user
      • Now there is a link to show 'Page 2' and all 31 users are visible
      • delete a user (back to 30) and the bottom one is missing again

      There is an error in the pagination code. It is showing 29 users per page rather than 30 most probably.

        Activity

        Hide
        Rajesh Taneja added a comment -

        Thanks for reporting this Howard,

        This is happening because in admin/users.php we skip guest users (while displaying) and get_users_listing() return recordset containing guest record.

        I have put this on backlog.

        In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

        Show
        Rajesh Taneja added a comment - Thanks for reporting this Howard, This is happening because in admin/users.php we skip guest users (while displaying) and get_users_listing() return recordset containing guest record. I have put this on backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
        Hide
        Rajesh Taneja added a comment -

        get_users_listing is only used in admin/user.php.
        Below this get_users() is used which excludes guest users.

        So the best solution seems to exclude guest user from the listing.

        Show
        Rajesh Taneja added a comment - get_users_listing is only used in admin/user.php. Below this get_users() is used which excludes guest users. So the best solution seems to exclude guest user from the listing.
        Hide
        Sam Hemelryk added a comment -

        Hi Raj,

        I agree that excluding the guest user from get_users_listing makes sense. Especially as get_users excludes the guest user.
        The changes to master make good sense, although I think you should note the API change in a upgrade.txt file.

        Given this is a bug it would be nice to have a solution for stables as well.
        Having looked at that function it appears you should be able to use the extraselect and extraparams args when calling get_users_listing within admin/user.php to avoid loading the guest user.
        This way you could fix it for stables as well.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hi Raj, I agree that excluding the guest user from get_users_listing makes sense. Especially as get_users excludes the guest user. The changes to master make good sense, although I think you should note the API change in a upgrade.txt file. Given this is a bug it would be nice to have a solution for stables as well. Having looked at that function it appears you should be able to use the extraselect and extraparams args when calling get_users_listing within admin/user.php to avoid loading the guest user. This way you could fix it for stables as well. Many thanks Sam
        Hide
        Rajesh Taneja added a comment -

        Thanks Sam,

        Sorry, I had 24 solution, but forgot to add.

        Will cherry-pick 24 commit on 23 after review.

        Show
        Rajesh Taneja added a comment - Thanks Sam, Sorry, I had 24 solution, but forgot to add. Will cherry-pick 24 commit on 23 after review.
        Hide
        Sam Hemelryk added a comment -

        Up for integration it goes

        Show
        Sam Hemelryk added a comment - Up for integration it goes
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Plz, amend upgrade.txt and get_users_listing() phpdocs to explain the change in master and this can land perfectly. TIA!

        Show
        Eloy Lafuente (stronk7) added a comment - Plz, amend upgrade.txt and get_users_listing() phpdocs to explain the change in master and this can land perfectly. TIA!
        Hide
        Rajesh Taneja added a comment -

        Sorry Eloy,

        Added another commit for master updating phpdoc and update.txt.

        Show
        Rajesh Taneja added a comment - Sorry Eloy, Added another commit for master updating phpdoc and update.txt.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (23, 24 & master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
        Hide
        Rossiani Wijaya added a comment -

        This work as expected.

        Test it for 2.3, 2.4 and master

        Test passed.

        Show
        Rossiani Wijaya added a comment - This work as expected. Test it for 2.3, 2.4 and master Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

        Thanks!

        PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

        Show
        Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: