Moodle
  1. Moodle
  2. MDL-31000

Repository Filepicker doesn't list repositories in correct order

    Details

    • Testing Instructions:
      Hide

      On a fresh installation

      • navigate to Settings -> Site administration -> Plugins -> Repositories -> Manage repositories;
      • change the order of the repositories and make a note of the new order (I moved 'Upload a file' to the top);
      • open a course;
      • Turn editing on;
      • From "Add a resource", choose "File";
      • Select "Add..."

      Expected Results

      The repositories are listed in the updated order

      Actual Results

      The order of repositories listed in the picker has not changed

      Show
      On a fresh installation navigate to Settings -> Site administration -> Plugins -> Repositories -> Manage repositories; change the order of the repositories and make a note of the new order (I moved 'Upload a file' to the top); open a course; Turn editing on; From "Add a resource", choose "File"; Select "Add..." Expected Results The repositories are listed in the updated order Actual Results The order of repositories listed in the picker has not changed
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31000-master-3

      Description

      The filepicker currently ignores the sortorder of repository plugins

      When get_instances() retrieves the list of plugins it performs an ORDER BY sortorder on the search.
      However, when the repository metadata is retrieved (repository/lib.php->initialise_filepicker()), the array is indexed by the repository ID. As a result, the JS in the filepicker orders results by ID rather than the order of the array.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Andrew Nicols added a comment -

            From what I've seen, there isn't anywhere in the filepicker which actually uses the array index for anything - filepicker.js->render() uses r[i].id rather than i for example.

            I've tested this fix on master using:

            • IE 9.0 under Windows 7
            • FireFox 8.0.1 under Windows 7
            • Chrome 16.0.912.63 under Windows 7
            • Opera 11.60 under Windows 7
            • Chrome 16.0.912.63 under Debian Squeeze
            • Iceweasel 3.5.16 under Debian Squeeze
            • Safari/7534.48.3 on iOS 5.0.1
            Show
            Andrew Nicols added a comment - From what I've seen, there isn't anywhere in the filepicker which actually uses the array index for anything - filepicker.js->render() uses r [i] .id rather than i for example. I've tested this fix on master using: IE 9.0 under Windows 7 FireFox 8.0.1 under Windows 7 Chrome 16.0.912.63 under Windows 7 Opera 11.60 under Windows 7 Chrome 16.0.912.63 under Debian Squeeze Iceweasel 3.5.16 under Debian Squeeze Safari/7534.48.3 on iOS 5.0.1
            Hide
            Michael de Raadt added a comment -

            Hey! Congrats, Andrew.

            You got 31,000.

            Show
            Michael de Raadt added a comment - Hey! Congrats, Andrew. You got 31,000.
            Hide
            Dongsheng Cai added a comment -

            It looks good, thanks Andrew.

            Show
            Dongsheng Cai added a comment - It looks good, thanks Andrew.
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            Sam Hemelryk added a comment -

            Thanks guys, this has been integrated now and cherry-picked to 21, and 22.

            Show
            Sam Hemelryk added a comment - Thanks guys, this has been integrated now and cherry-picked to 21, and 22.
            Hide
            Sam Hemelryk added a comment -

            This has failed testing and is being reverted.

            There is several areas of code in JavaScript that rely on the repositories array keys equalling the repository id.
            Sorry but we need to find where the sort order is being messed up and address it there.
            We must keep the keys equalling the repo id's.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - This has failed testing and is being reverted. There is several areas of code in JavaScript that rely on the repositories array keys equalling the repository id. Sorry but we need to find where the sort order is being messed up and address it there. We must keep the keys equalling the repo id's. Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            Reopened

            Show
            Sam Hemelryk added a comment - Reopened
            Hide
            Andrew Nicols added a comment -

            This is going to be much harder then.

            The sort order isn't being messed up, JavaScript just doesn't respect it as an order at all. It takes an array as an object, and loops through the elements of the object. Objects are unordered by definition.

            Take this example - http://dumping.andrewrn.co.uk/moodle/MDL-31000.html

            We'll have to look at converting the object of objects to an array of objects to get the sorting I think...

            Show
            Andrew Nicols added a comment - This is going to be much harder then. The sort order isn't being messed up, JavaScript just doesn't respect it as an order at all. It takes an array as an object, and loops through the elements of the object. Objects are unordered by definition. Take this example - http://dumping.andrewrn.co.uk/moodle/MDL-31000.html We'll have to look at converting the object of objects to an array of objects to get the sorting I think...
            Hide
            Andrew Nicols added a comment -

            Have an updated fix for this

            Show
            Andrew Nicols added a comment - Have an updated fix for this
            Hide
            Andrew Nicols added a comment -

            I've updated the fix to create a new array based on the repository sortorder.

            Have tested in:

            • IE9 Windows 7
            • Firefox Windows 7
            • Chrome Windows 7
            • Opera Windows 7
            • Chrome Debian
            Show
            Andrew Nicols added a comment - I've updated the fix to create a new array based on the repository sortorder. Have tested in: IE9 Windows 7 Firefox Windows 7 Chrome Windows 7 Opera Windows 7 Chrome Debian
            Hide
            Dan Poltawski added a comment -

            It seems to me like fixing this in SQL is really the best solution. Sam, other than the drag and drop stuff just integrated whre else were you finding this being used? I can't actually find where else its used.

            Show
            Dan Poltawski added a comment - It seems to me like fixing this in SQL is really the best solution. Sam, other than the drag and drop stuff just integrated whre else were you finding this being used? I can't actually find where else its used.
            Hide
            Andrew Nicols added a comment -

            Hi Dan,

            There is no issue with the SQL - the SQL does return ordered content; but it's presented to the JavaScript as an object. The object is in the correct order, but JavaScript as a language doesn't support ordering of object keys.

            In my example, I've defined the object as:

                  var repositories = {
                    3  : 'Object 3',
                    4  : 'Object 4',
                    5  : 'Object 5',
                    2  : 'Object 2',
                    1  : 'Object 1',
                    0  : 'Object 0'
                  }
            

            I've then looped through the object and printed out the results - see http://dumping.andrewrn.co.uk/moodle/MDL-31000.html for the working example.

            Because the keys are numeric, they're printed out in ascending numerical order and the order they're defined in is ignored as per the ECMA specification.

            The best solution I can think of is to add the sortorder parameter to the sub-objects and create a second array sorted by those values as in my later patch.

            Show
            Andrew Nicols added a comment - Hi Dan, There is no issue with the SQL - the SQL does return ordered content; but it's presented to the JavaScript as an object. The object is in the correct order, but JavaScript as a language doesn't support ordering of object keys. In my example, I've defined the object as: var repositories = { 3 : 'Object 3', 4 : 'Object 4', 5 : 'Object 5', 2 : 'Object 2', 1 : 'Object 1', 0 : 'Object 0' } I've then looped through the object and printed out the results - see http://dumping.andrewrn.co.uk/moodle/MDL-31000.html for the working example. Because the keys are numeric, they're printed out in ascending numerical order and the order they're defined in is ignored as per the ECMA specification. The best solution I can think of is to add the sortorder parameter to the sub-objects and create a second array sorted by those values as in my later patch.
            Hide
            Andrew Nicols added a comment -

            I've updated the commit on the last master. It should also cherry-pick cleanly to MOODLE_21_STABLE and MOODLE_22_STABLE

            Show
            Andrew Nicols added a comment - I've updated the commit on the last master. It should also cherry-pick cleanly to MOODLE_21_STABLE and MOODLE_22_STABLE
            Hide
            Andrew Nicols added a comment -

            Hi Dongsheng,

            Would you be able to peer review the latest version of this and put it up for integration review if it's acceptable?

            This cherry-picks cleanly to:

            • MOODLE_21_STABLE
            • MOODLE_22_STABLE
            • master

            Thanks,

            Andrew

            Show
            Andrew Nicols added a comment - Hi Dongsheng, Would you be able to peer review the latest version of this and put it up for integration review if it's acceptable? This cherry-picks cleanly to: MOODLE_21_STABLE MOODLE_22_STABLE master Thanks, Andrew
            Hide
            Peter Gee added a comment -

            this was annoying me a lot.
            i did this:
            changed line 105 of <moodle dir>/repository/filesystem/lib.php
            added sort($list['list']); to sort the file system list.
            works fine.

            Show
            Peter Gee added a comment - this was annoying me a lot. i did this: changed line 105 of <moodle dir>/repository/filesystem/lib.php added sort($list ['list'] ); to sort the file system list. works fine.
            Hide
            Dongsheng Cai added a comment -

            Thanks Andrew.

            Your patch looks good.

            Show
            Dongsheng Cai added a comment - Thanks Andrew. Your patch looks good.
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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 -

            As far as the ordering now happens 100% within the rendering and the original "r" continues being the same, I assume this is safe. I'd recommend to also test other JS facilities.

            Integrated (21, 22 & master), thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - As far as the ordering now happens 100% within the rendering and the original "r" continues being the same, I assume this is safe. I'd recommend to also test other JS facilities. Integrated (21, 22 & master), thanks!
            Hide
            Rajesh Taneja added a comment -

            As Sam is on leave, I am picking this for testing

            Show
            Rajesh Taneja added a comment - As Sam is on leave, I am picking this for testing
            Hide
            Rajesh Taneja added a comment -

            Thanks Andrew,

            Works Great

            Show
            Rajesh Taneja added a comment - Thanks Andrew, Works Great
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao
            Hide
            Dominick Inglese added a comment -

            I'm not sure what to do with the file in the "source" tab next to activity (above). Which file should I download? And I think I should just replace the existing file with the downloaded file? Right?

            "stash" let me sign in so I can see the source files. But I don't know what to do with them or which one to download.

            Thanks for your time,

            Show
            Dominick Inglese added a comment - I'm not sure what to do with the file in the "source" tab next to activity (above). Which file should I download? And I think I should just replace the existing file with the downloaded file? Right? "stash" let me sign in so I can see the source files. But I don't know what to do with them or which one to download. Thanks for your time,
            Hide
            Andrew Nicols added a comment -

            Hi Dominick,

            This issue was integrated into versions 2.1.5, and 2.2.2 of Moodle and is also present from 2.3.0 onwards. Please upgrade to the latest version. It's extremely unwise to try apply files from different versions of Moodle as other changes are likely to have been made in addition to those you're expecting.

            Show
            Andrew Nicols added a comment - Hi Dominick, This issue was integrated into versions 2.1.5, and 2.2.2 of Moodle and is also present from 2.3.0 onwards. Please upgrade to the latest version. It's extremely unwise to try apply files from different versions of Moodle as other changes are likely to have been made in addition to those you're expecting.
            Hide
            Dominick Inglese added a comment -

            Thank you for the quick reply Andrew. I am using 2.2 now so I will upgrade ASAP.

            Show
            Dominick Inglese added a comment - Thank you for the quick reply Andrew. I am using 2.2 now so I will upgrade ASAP.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: