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
    • Rank:
      37404

      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.

        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: