Moodle
  1. Moodle
  2. MDL-35664

Filepicker does not handle the current path

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.3
    • Component/s: Filepicker
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      1. Enable Wikimedia and Box.net repositories
      2. Enable another repository which supports pagination and subfolders (Evernote can, with a little config, or use dummyrepo2.zip)

      Test steps

      1. Navigate to your private files and open the file picker
      2. Search for 'cat' in Wikimedia and make sure the next results are fetched as you scroll
      1. Navigate to your Box.net (you need subfolders in there)
      2. Browse your files and directories and make sure the repository behave correctly
      1. Try the 3rd repository (do not use tree view)
      2. Navigate to some directory (do not stay on the default repository folder)
      3. Make sure hitting the refresh button, refreshes the correct path, and does not redirect you to the default folder
      4. Make sure the next results are fetched while you scroll

      Known issues:

      • the search cannot be refreshed, and the next page results do not pass the initial query MDL-35897
      • switching to tree view while in a subfolder of a dynload off repository (box.net) is not well supported
      Show
      Test pre-requisites Enable Wikimedia and Box.net repositories Enable another repository which supports pagination and subfolders (Evernote can, with a little config, or use dummyrepo2.zip) Test steps Navigate to your private files and open the file picker Search for 'cat' in Wikimedia and make sure the next results are fetched as you scroll Navigate to your Box.net (you need subfolders in there) Browse your files and directories and make sure the repository behave correctly Try the 3rd repository (do not use tree view) Navigate to some directory (do not stay on the default repository folder) Make sure hitting the refresh button, refreshes the correct path, and does not redirect you to the default folder Make sure the next results are fetched while you scroll Known issues: the search cannot be refreshed, and the next page results do not pass the initial query MDL-35897 switching to tree view while in a subfolder of a dynload off repository (box.net) is not well supported
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35664-master
    • Rank:
      44401

      Description

      When viewing (not in tree view) a repository (Dropbox for example) and browsing into some subdirectories, clicking the refresh button will reload the root of the repository.

      From what I have seen, the file picker is not storing the current path anywhere.
      Also, when requesting the next page, the path sent back to repository_ajax is undefined.
      And later on the following test will fail because the obj.path is the breadcrumb and scope.path is undefined:

      // repository/filepicker.js:1036
                          if (scope.active_repo.hasmorepages && obj.list && obj.page &&
                                  obj.repo_id == scope.active_repo.id &&
                                  obj.page == scope.active_repo.page+1 && obj.path == scope.path) {
                              scope.parse_repository_options(obj, true);
                              scope.view_files(obj.list)
                          }
      

      (I am not sure how this can be handled in the tree view.)

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          I am assigning this to me and Marina as peer reviewer as I am providing a patch.

          I noticed that the file picker did not store the information about the path, and was living only with the information returned from the ajax requests. Therefore, I have added a variable req_path which returns the path that was requested by the user. I also updated the method that fetches the next results as the arguments passed were incorrect.

          Show
          Frédéric Massart added a comment - I am assigning this to me and Marina as peer reviewer as I am providing a patch. I noticed that the file picker did not store the information about the path, and was living only with the information returned from the ajax requests. Therefore, I have added a variable req_path which returns the path that was requested by the user. I also updated the method that fetches the next results as the arguments passed were incorrect.
          Hide
          Marina Glancy added a comment -

          We had a long discussion of that with Fred. The problem seems to be much deeper, there are several points here:

          1. Refresh button does not refresh current folder for repository with subfolders and jumps to the home folder

          2. mode dynload=false is actually not used in core repositories so we did not so far hit all the problems with it. We need to create a dummy repository with subfolders and dynload=false, that has "files" with different extensions and check the following:

          • Navigation and refresh button works correctly in all view modes
          • When using with filter (i.e. only image files for tinymce) everything is filtered recursively - it seems that repository::prepare_listing() will break here

          3. Pagination AND subfolders together won't really work in tree view but we can try it in icon/list views

          Show
          Marina Glancy added a comment - We had a long discussion of that with Fred. The problem seems to be much deeper, there are several points here: 1. Refresh button does not refresh current folder for repository with subfolders and jumps to the home folder 2. mode dynload=false is actually not used in core repositories so we did not so far hit all the problems with it. We need to create a dummy repository with subfolders and dynload=false, that has "files" with different extensions and check the following: Navigation and refresh button works correctly in all view modes When using with filter (i.e. only image files for tinymce) everything is filtered recursively - it seems that repository::prepare_listing() will break here 3. Pagination AND subfolders together won't really work in tree view but we can try it in icon/list views
          Hide
          Frédéric Massart added a comment -

          2. Box.net uses dynload off, it might help testing.
          3. The function requesting the next results should send the path we are currently on to avoid having to use $SESSION in the plugins.

          Show
          Frédéric Massart added a comment - 2. Box.net uses dynload off, it might help testing. 3. The function requesting the next results should send the path we are currently on to avoid having to use $SESSION in the plugins.
          Hide
          Marina Glancy added a comment - - edited

          I attach the repository_dummyrepo that has dynload=false and subfolders. Works terribly

          Show
          Marina Glancy added a comment - - edited I attach the repository_dummyrepo that has dynload=false and subfolders. Works terribly
          Hide
          Marina Glancy added a comment -

          even worse. I tried to add

          $list = array_filter($list, array($this, 'filter'));
          

          at line 58 or repository_dummy and it seems that repository::filter does not work recursively too

          Show
          Marina Glancy added a comment - even worse. I tried to add $list = array_filter($list, array($ this , 'filter')); at line 58 or repository_dummy and it seems that repository::filter does not work recursively too
          Hide
          Frédéric Massart added a comment -

          All right, this issue is much more complicated than it looks. The problem is that the non-dynamic loading (dynload=off) is missing functionalities.

          The main problem being the breadcrumb, which while dynload on, is sent back after every directory listing, the dynload off does not. I thought I'd create a new parameter "breadcrumb" to send back as part of each level of the whole tree structure returned by the repository, but this is not really helping. The problem being that when we click on the breadcrumb, we will send a request to the repository and that's not what we want.

          Leaving the breadcrumb problem on the side is a solution, but that's not fixing the problem. I personally think that the dynload off should be an option as some repositories have the ability to return the whole structure (or part of it) in one query, but without the user's ability to go back to (at least) the parent directory it is really user friendly (although that's the case in 2.2). I also think that a repository should be able to jump from one option to the other.

          Another problem would be to consider that, when calling get_listing() with an empty $path, we are browsing the root of the repository. I think this would be a wrong assumption as the repository could handle an empty $path as some default $path. That's what I did in some repository I worked on, if the $path was empty, then the $path is considered as "somewhere/in/the/repository".

          Those were my food for thoughts, I'll have another look at this issue again soon, but I think most of this issue would be an improvement instead of a bug and should maybe be addressed in another issue.

          Show
          Frédéric Massart added a comment - All right, this issue is much more complicated than it looks. The problem is that the non-dynamic loading (dynload=off) is missing functionalities. The main problem being the breadcrumb, which while dynload on , is sent back after every directory listing, the dynload off does not. I thought I'd create a new parameter "breadcrumb" to send back as part of each level of the whole tree structure returned by the repository, but this is not really helping. The problem being that when we click on the breadcrumb, we will send a request to the repository and that's not what we want. Leaving the breadcrumb problem on the side is a solution, but that's not fixing the problem. I personally think that the dynload off should be an option as some repositories have the ability to return the whole structure (or part of it) in one query, but without the user's ability to go back to (at least) the parent directory it is really user friendly (although that's the case in 2.2). I also think that a repository should be able to jump from one option to the other. Another problem would be to consider that, when calling get_listing() with an empty $path, we are browsing the root of the repository. I think this would be a wrong assumption as the repository could handle an empty $path as some default $path. That's what I did in some repository I worked on, if the $path was empty, then the $path is considered as "somewhere/in/the/repository". Those were my food for thoughts, I'll have another look at this issue again soon, but I think most of this issue would be an improvement instead of a bug and should maybe be addressed in another issue.
          Hide
          Marina Glancy added a comment -

          I personally think we should not support dynload=off mode at all. Just assume that it is always on and write about it in dev docs. It's too much work and not much use really.

          At the same time repository may use caching to store last external request result.

          Also when user clicks 'Refresh' button in filepicker the repository should receive some additional parameter that cache needs to be rebuilt

          Show
          Marina Glancy added a comment - I personally think we should not support dynload=off mode at all. Just assume that it is always on and write about it in dev docs. It's too much work and not much use really. At the same time repository may use caching to store last external request result. Also when user clicks 'Refresh' button in filepicker the repository should receive some additional parameter that cache needs to be rebuilt
          Hide
          Frédéric Massart added a comment -

          Pushing this for peer review again.

          Show
          Frédéric Massart added a comment - Pushing this for peer review again.
          Hide
          Marina Glancy added a comment -

          Fred, looks good to me

          Show
          Marina Glancy added a comment - Fred, looks good to me
          Hide
          Frédéric Massart added a comment -

          Thanks Marina. Pushing for integration.

          Show
          Frédéric Massart added a comment - Thanks Marina. Pushing for integration.
          Hide
          Sam Hemelryk added a comment -

          Thanks Fred, a truly acceptable change. Has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Fred, a truly acceptable change. Has been integrated now
          Hide
          Mark Nelson added a comment -

          Test passed, all looking good. However, using box.net the directory path never changes and remains as root, and refreshing results in being taken back to the root folder. I was told by Fred that this is a separate issue.

          Show
          Mark Nelson added a comment - Test passed, all looking good. However, using box.net the directory path never changes and remains as root, and refreshing results in being taken back to the root folder. I was told by Fred that this is a separate issue.
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!
          Hide
          Dan Poltawski added a comment -

          FYI this issue caused a regression in MDL-36696 with an additional comma causing IE to break (when in IE7 mode)

          Show
          Dan Poltawski added a comment - FYI this issue caused a regression in MDL-36696 with an additional comma causing IE to break (when in IE7 mode)

            People

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

              Dates

              • Created:
                Updated:
                Resolved: