Moodle
  1. Moodle
  2. MDL-15930

Admin: Improper default sorting not respecting $CFG->fullnamedisplay when browsing list of users

    Details

    • Testing Instructions:
      Hide

      1) Confirm language string in lang/en/moodle.php is firstname, lastname
      2) Confirm fullnamedisplay is set to use language (admin/settings.php?section=sitepolicies)
      3) Browse list of all users (admin/user.php)
      4) Confirm users sorted by firstname
      5) Set fullnamedisplay to use surname+firstname
      6) Browse list of users
      7) Ensure that users are sorted by lastname, firstname.

      Show
      1) Confirm language string in lang/en/moodle.php is firstname, lastname 2) Confirm fullnamedisplay is set to use language (admin/settings.php?section=sitepolicies) 3) Browse list of all users (admin/user.php) 4) Confirm users sorted by firstname 5) Set fullnamedisplay to use surname+firstname 6) Browse list of users 7) Ensure that users are sorted by lastname, firstname.
    • Workaround:
      Hide

      Click on the surname url (admin/user.php?sort=lastname&dir=DESC) to force a sort on the page

      Show
      Click on the surname url (admin/user.php?sort=lastname&dir=DESC) to force a sort on the page
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      wip-MDL-15930-24
    • Pull Master Branch:
      wip-MDL-15930-master
    • Rank:
      4557

      Description

      Per http://moodle.org/mod/forum/discuss.php?d=76855#p45328sammarshallou6, I checked and noticed that $CFG->fullnamedisplay is not properly implemented for sorting on the /admin/user.php page. Currently, it checks to see if the sorting is by name and if so defaults to firstname; however, if the fullnamedisplay begins with lastname it makes sense to expect that it should sort by lastname. To accomplish this, I moved up the code that checks for fullnamedisplay to before the execution of the query to get the users. That way the $sort variable is properly set prior to executing the query. Peace - Anthony

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          Here is a proposed patch to resolve this issue. Peace - Anthony

          Show
          Anthony Borrow added a comment - Here is a proposed patch to resolve this issue. Peace - Anthony
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          lqjjLKA0p6

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; lqjjLKA0p6
          Hide
          Anthony Borrow added a comment -

          Michael - I've confirmed that this is still an issue in 2.2 so I have added 2.0, 2.1 and 2.2 to the affected branches. My +1 that this remains open. I've attached a patch and explain why we are seeing the behavior we are in the description of this issue. Peace - Anthony

          Show
          Anthony Borrow added a comment - Michael - I've confirmed that this is still an issue in 2.2 so I have added 2.0, 2.1 and 2.2 to the affected branches. My +1 that this remains open. I've attached a patch and explain why we are seeing the behavior we are in the description of this issue. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          I just noticed that I had used fixed version instead of affected version. I am adding the affected versions now. Since this is just a matter of reorganizing some code, I would recommend fixing this in 1.9, 2.0, 2.1 and 2.2. Feel free to adjust the fix version as needed. Peace - Anthony

          Show
          Anthony Borrow added a comment - I just noticed that I had used fixed version instead of affected version. I am adding the affected versions now. Since this is just a matter of reorganizing some code, I would recommend fixing this in 1.9, 2.0, 2.1 and 2.2. Feel free to adjust the fix version as needed. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          Eloy/Michael - I've provided a patch for 1.9 via Github. The change for 2.x should be nearly identical. What would be the most helpful thing for me to do to help move this forward? Peace - Anthony

          Show
          Anthony Borrow added a comment - Eloy/Michael - I've provided a patch for 1.9 via Github. The change for 2.x should be nearly identical. What would be the most helpful thing for me to do to help move this forward? Peace - Anthony
          Hide
          Michael de Raadt added a comment -

          It looks like your intention was to have this reviewed, Anthony, so I'm sending the issue in that direction.

          Show
          Michael de Raadt added a comment - It looks like your intention was to have this reviewed, Anthony, so I'm sending the issue in that direction.
          Hide
          Adrian Greeve added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [Y] Security
          [-] Documentation
          [*] Git
          [Y] Sanity check

          This patch makes sense. I don't see any problems.
          The git commit is missing a component, and the patch needs serious rebasing.

          Thanks.

          Show
          Adrian Greeve added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [Y] Security [-] Documentation [*] Git [Y] Sanity check This patch makes sense. I don't see any problems. The git commit is missing a component, and the patch needs serious rebasing. Thanks.
          Hide
          Anthony Borrow added a comment -

          Adrian - I'm not sure when I'll have time to rebase and look at this. Might we consider re-assigning to someone on the stable team? Peace - Anthony

          Show
          Anthony Borrow added a comment - Adrian - I'm not sure when I'll have time to rebase and look at this. Might we consider re-assigning to someone on the stable team? Peace - Anthony
          Hide
          Adrian Greeve added a comment -

          Hi Anthony,

          I'll assign it to myself and tidy up the branches.

          Show
          Adrian Greeve added a comment - Hi Anthony, I'll assign it to myself and tidy up the branches.
          Hide
          Adrian Greeve added a comment -

          Removing the peer-reviewer so that someone else can have a look at this.

          Show
          Adrian Greeve added a comment - Removing the peer-reviewer so that someone else can have a look at this.
          Hide
          Frédéric Massart added a comment -

          [Y] Syntax
          [Y] Output
          [?] Whitespace
          [Y] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [?] Documentation
          [Y] Git
          [Y] Sanity check

          Thanks guys, the patch looks good. I just noticed a few whitespaces in the comments, as well as some periods missing at their end. Also, I don't think it is necessary to quote the MDL in the comment as this is just a fix, but that's just me.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - [Y] Syntax [Y] Output [?] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [?] Documentation [Y] Git [Y] Sanity check Thanks guys, the patch looks good. I just noticed a few whitespaces in the comments, as well as some periods missing at their end. Also, I don't think it is necessary to quote the MDL in the comment as this is just a fix, but that's just me. Cheers, Fred
          Hide
          Adrian Greeve added a comment -

          Thanks for the peer review Fred,

          I fixed up the white space and punctuation as well as removed the reference to the MDL.

          Submitting for integration.

          Show
          Adrian Greeve added a comment - Thanks for the peer review Fred, I fixed up the white space and punctuation as well as removed the reference to the MDL. Submitting for integration.
          Hide
          Dan Poltawski 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.

          TIA and ciao

          Show
          Dan Poltawski 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. TIA and ciao
          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
          Jason Fowler added a comment -

          Works fine Adrian. Just had to tweak the testing instructions, and point 7 read more like replication steps than testing instructions

          Show
          Jason Fowler added a comment - Works fine Adrian. Just had to tweak the testing instructions, and point 7 read more like replication steps than testing instructions
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note that MDL-37643 has been created about a better way to handle this, for your consideration, now that it's still warm...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Note that MDL-37643 has been created about a better way to handle this, for your consideration, now that it's still warm... Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: