Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35776

In Chrome, in Ajax UI, users are shown in userid order, not the desired order

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.5, 2.3.2, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      You need to test Chrome, to verify the bug is fixed, and other browsers, to ensure there are no regresstions.

      You need to test the user selector (e.g. add/remove group members, or assign system roles) and the enrol users / other users UI.

      You need to make sure that the users are shown in order of lastname (with any exact matches at the top, see MDL-34657), not in order of userid.

      Show
      You need to test Chrome, to verify the bug is fixed, and other browsers, to ensure there are no regresstions. You need to test the user selector (e.g. add/remove group members, or assign system roles) and the enrol users / other users UI. You need to make sure that the users are shown in order of lastname (with any exact matches at the top, see MDL-34657 ), not in order of userid.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Discovered while testing MDL-34657.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            Note that the action=getassignable Ajax calls that get the list of assignable roles are also affected by this ordering bug, however, the JS does rely on the array keys there, so it is harder to fix. Meanwhile, there are only a few roles, so the order is not critical, so I am not going to try to fix that bit.

            If anyone cares, they should file another bug about that.

            Show
            timhunt Tim Hunt added a comment - Note that the action=getassignable Ajax calls that get the list of assignable roles are also affected by this ordering bug, however, the JS does rely on the array keys there, so it is harder to fix. Meanwhile, there are only a few roles, so the order is not critical, so I am not going to try to fix that bit. If anyone cares, they should file another bug about that.
            Hide
            timhunt Tim Hunt added a comment -

            This should do it.

            I am not an expert on the enrol UI. Petr, please could you peer review that part of the code? Thanks.

            Show
            timhunt Tim Hunt added a comment - This should do it. I am not an expert on the enrol UI. Petr, please could you peer review that part of the code? Thanks.
            Hide
            timhunt Tim Hunt added a comment -

            Right, ready for peer review.

            Show
            timhunt Tim Hunt added a comment - Right, ready for peer review.
            Hide
            matteo Matteo Scaramuccia added a comment - - edited

            TNX Tim! I've learn another lesson today
            For the record, there is a big noise around this known - kind of - issue with Chrome:

            Show
            matteo Matteo Scaramuccia added a comment - - edited TNX Tim! I've learn another lesson today For the record, there is a big noise around this known - kind of - issue with Chrome: "The order in which item is bound to members of collection is implementation dependent.", §7.1.3 http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-357.pdf http://code.google.com/p/chromium/issues/detail?id=883 http://code.google.com/p/v8/issues/detail?id=164 http://wiki.ecmascript.org/doku.php?id=strawman:enumeration
            Hide
            skodak Petr Skoda added a comment -

            Hi, I am not the author of enrol UI code, but it seems ok to me. I am not sure about the changes in user/selector/search.php because that seems like BC incompatible change to me which should be done in master only.

            Show
            skodak Petr Skoda added a comment - Hi, I am not the author of enrol UI code, but it seems ok to me. I am not sure about the changes in user/selector/search.php because that seems like BC incompatible change to me which should be done in master only.
            Hide
            pholden Paul Holden added a comment -
            Show
            pholden Paul Holden added a comment - Same thing probably happens in IE too, and was also the cause of MDL-31000 , see this comment for more.. http://stackoverflow.com/questions/6139759/are-there-any-major-browsers-that-do-not-preserve-insertion-order-in-a-javascrip
            Hide
            timhunt Tim Hunt added a comment -

            Petr, thanks for looking.

            I don't understand what BC issue you are worried about Petr. user/selector/search.php and user/selector/module.js are both internal and private to the user selector. They have both been changed in a corresponding way. No external API has been changed.

            Show
            timhunt Tim Hunt added a comment - Petr, thanks for looking. I don't understand what BC issue you are worried about Petr. user/selector/search.php and user/selector/module.js are both internal and private to the user selector. They have both been changed in a corresponding way. No external API has been changed.
            Hide
            skodak Petr Skoda added a comment -

            Thanks Tim, I trust you - at least we have a volunteer to fix any unexpected regressions...

            Show
            skodak Petr Skoda added a comment - Thanks Tim, I trust you - at least we have a volunteer to fix any unexpected regressions...
            Hide
            timhunt Tim Hunt added a comment -

            Yes, due to extensive practice, I am now an expert at fixing regressions I have caused

            Am submitting this for integration.

            Show
            timhunt Tim Hunt added a comment - Yes, due to extensive practice, I am now an expert at fixing regressions I have caused Am submitting this for integration.
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks thats been integrated into 22, 23 and master.

            Show
            nebgor Aparup Banerjee added a comment - Thanks thats been integrated into 22, 23 and master.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Testing this for 2.2, 2.3 and 2.4.

            It works as expected.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - Testing this for 2.2, 2.3 and 2.4. It works as expected. Test passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            From somewhere within the clouds...

            Congrats, this has been sent upstream and is now part of Moodle (your favorite LMS platform). Many thanks for your awesome collaboration!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - From somewhere within the clouds... Congrats, this has been sent upstream and is now part of Moodle (your favorite LMS platform). Many thanks for your awesome collaboration! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Nov/12