Uploaded image for project: '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

          Attachments

            Issue Links

              Activity

              Hide
              dobedobedoh 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
              dobedobedoh 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
              salvetore Michael de Raadt added a comment -

              Hey! Congrats, Andrew.

              You got 31,000.

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

              It looks good, thanks Andrew.

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

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

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks guys, this has been integrated now and cherry-picked to 21, and 22.
              Hide
              samhemelryk 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
              samhemelryk 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
              samhemelryk Sam Hemelryk added a comment -

              Reopened

              Show
              samhemelryk Sam Hemelryk added a comment - Reopened
              Hide
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh Andrew Nicols added a comment -

              Have an updated fix for this

              Show
              dobedobedoh Andrew Nicols added a comment - Have an updated fix for this
              Hide
              dobedobedoh 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
              dobedobedoh 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
              poltawski 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
              poltawski 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              pgee 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
              pgee 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 Dongsheng Cai added a comment -

              Thanks Andrew.

              Your patch looks good.

              Show
              dongsheng Dongsheng Cai added a comment - Thanks Andrew. Your patch looks good.
              Hide
              stronk7 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
              stronk7 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
              stronk7 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
              stronk7 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
              rajeshtaneja Rajesh Taneja added a comment -

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

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

              Thanks Andrew,

              Works Great

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Andrew, Works Great
              Hide
              stronk7 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
              stronk7 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
              nimaipandit 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
              nimaipandit 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
              dobedobedoh 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
              dobedobedoh 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
              nimaipandit Dominick Inglese added a comment -

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

              Show
              nimaipandit 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:
                    Fix Release Date:
                    12/Mar/12