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

          Attachments

            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