Uploaded image for project: '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 Master Branch:
      wip-mdl-38332

      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.

        Gliffy Diagrams

          Activity

          Hide
          rajeshtaneja 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
          rajeshtaneja 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
          rajeshtaneja 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
          rajeshtaneja 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
          samhemelryk 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
          samhemelryk 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
          rajeshtaneja 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
          rajeshtaneja 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
          samhemelryk Sam Hemelryk added a comment -

          Up for integration it goes

          Show
          samhemelryk Sam Hemelryk added a comment - Up for integration it goes
          Hide
          stronk7 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
          stronk7 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
          rajeshtaneja Rajesh Taneja added a comment -

          Sorry Eloy,

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

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

          Integrated (23, 24 & master), thanks!

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

          This work as expected.

          Test it for 2.3, 2.4 and master

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - This work as expected. Test it for 2.3, 2.4 and master Test passed.
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                18/Mar/13