Moodle

Filepicker does not handle the current path

Details

  • Type: Bug Bug
  • Status: Closed 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
  • Pull Master Diff URL:

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

caused a regression

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-36696 filepicker.js has superfluous comma which prevents the filepicker loading in IE

  • Critical - Crashes server, loss of data, severe memory leak
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
has been marked as being related by

Improvement - An enhancement to an existing Moodle feature. MDL-35894 File picker refresh button should notify the repository

  • Minor - Minor loss of function where workaround is possible
  • Open - The issue is open and ready for the assignee to start work on it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Marina Glancy added a comment -

Fred, looks good to me

Show
Marina Glancy added a comment - Fred, looks good to me
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: