Uploaded image for project: '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 Master Branch:
      wip-MDL-15930-master

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            aborrow Anthony Borrow added a comment -

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

            Show
            aborrow Anthony Borrow added a comment - Here is a proposed patch to resolve this issue. Peace - Anthony
            Hide
            salvetore 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
            salvetore 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
            aborrow 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
            aborrow 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
            aborrow 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
            aborrow 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
            aborrow 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
            aborrow 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
            salvetore 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
            salvetore 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
            abgreeve 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
            abgreeve 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
            aborrow 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
            aborrow 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
            abgreeve Adrian Greeve added a comment -

            Hi Anthony,

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

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

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

            Show
            abgreeve Adrian Greeve added a comment - Removing the peer-reviewer so that someone else can have a look at this.
            Hide
            fred 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
            fred 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
            abgreeve 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
            abgreeve 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
            poltawski 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
            poltawski 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
            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
            phalacee 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
            phalacee 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  25/Dec/10