Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.3
    • Fix Version/s: STABLE backlog
    • Component/s: Repositories
    • Labels:
    • Affected Branches:
      MOODLE_23_STABLE
    • Rank:
      41706

      Description

      I reviewed recently equella plugin and would like to add some comments.
      First, there are some changes in repositories API and some other things that I noticed

      Mostly they are not very big so I create one issue, feel free to split it or convert to META.

      1. repository_equella::get_reference_details() is missing and we can not display the human-readable "Original" in Filemanager

      2. please look at MDL-32474, there are new fields filepicker expects from repositories. In your case they are passed to M.core_filepicker.select_file()
      There is also a function repository::prepare_listing() that converts dates to userdate, file size to readable size, etc.

      3 (Moved to subtask MDL-33798)

      4. Filemanager fails with an error often (I have to discover it a bit more)

      5. When filepicker's select file window appears but you clicks 'cancel', an empty content is displayed.
      Actually now when I looked closer I don't like the idea of calling parent.M.core_filepicker.select_file(). It is unsafe and may be prohibited in some browsers. I need to come back to this issue later myself, maybe will have to change API.

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Assiging to Nick from Equella

          Show
          Dan Poltawski added a comment - Assiging to Nick from Equella
          Hide
          Marina Glancy added a comment -

          Re 4: I turned on logging of JS responses and this is the error

          Fatal error: Maximum execution time of 30 seconds exceeded in /var/www/repositories/master/moodle/lib/filelib.php on line 3034
          
          Warning: (null)(): 89 is not a valid cURL handle resource in Unknown on line 0
          

          I guess this is because we try to find out the size of the file and download it to moodle for this

          Show
          Marina Glancy added a comment - Re 4: I turned on logging of JS responses and this is the error Fatal error: Maximum execution time of 30 seconds exceeded in / var /www/repositories/master/moodle/lib/filelib.php on line 3034 Warning: ( null )(): 89 is not a valid cURL handle resource in Unknown on line 0 I guess this is because we try to find out the size of the file and download it to moodle for this
          Hide
          Nick Read added a comment -

          The file size is available to be passed directly into parent.M.core_filepicker.select_file() if it supported it, avoiding the need to download the file.

          If this can't be done, then the curl request could be changed to a HEAD request that would still return the correct file size.

          What other reasons is the file being downloaded for? There are definitely cases where it will take more than 30 seconds to download resources from EQUELLA when the resource is very large.

          Show
          Nick Read added a comment - The file size is available to be passed directly into parent.M.core_filepicker.select_file() if it supported it, avoiding the need to download the file. If this can't be done, then the curl request could be changed to a HEAD request that would still return the correct file size. What other reasons is the file being downloaded for? There are definitely cases where it will take more than 30 seconds to download resources from EQUELLA when the resource is very large.
          Hide
          Marina Glancy added a comment -

          After upload Filemanager refreshes the list of files and tries to get information about files sizes. For references it is done through file syncronization, see repository::sync_external_file().

          It calls the function repository_equella::get_file_by_reference() to get the file information (size and contenthash).

          get_file_by_reference() does not have to download file. It can return only filesize and contenthash. I improved phpdocs for parent function in MDL-33550

          If there is a way in Equella to get this file information without actually downloading the file, I would strongly recommend to change this function

          Show
          Marina Glancy added a comment - After upload Filemanager refreshes the list of files and tries to get information about files sizes. For references it is done through file syncronization, see repository::sync_external_file(). It calls the function repository_equella::get_file_by_reference() to get the file information (size and contenthash). get_file_by_reference() does not have to download file. It can return only filesize and contenthash. I improved phpdocs for parent function in MDL-33550 If there is a way in Equella to get this file information without actually downloading the file, I would strongly recommend to change this function
          Hide
          Dan Poltawski added a comment -

          removing 7 as its covered in MDL-33724

          Show
          Dan Poltawski added a comment - removing 7 as its covered in MDL-33724
          Hide
          Dan Poltawski added a comment -

          ps. just noticed there was no 6

          Show
          Dan Poltawski added a comment - ps. just noticed there was no 6
          Hide
          Martin Dougiamas added a comment -

          repository_equella::get_reference_details() needs to read the data from the files->source field and show that. (it will work as soon as MDL-33513 lands)

          Show
          Martin Dougiamas added a comment - repository_equella::get_reference_details() needs to read the data from the files->source field and show that. (it will work as soon as MDL-33513 lands)
          Hide
          Nick Read added a comment -

          Dongsheng now has repository_equella::get_reference_details() ready in MDL-33777

          Show
          Nick Read added a comment - Dongsheng now has repository_equella::get_reference_details() ready in MDL-33777
          Hide
          Marina Glancy added a comment -

          This is very major:

          In my installation the function repository_equella::get_file_by_reference() invoked to update file size / contenthash actually tries to download a file from Equella and fails because of 30s script timeout which results with not loading filemanager at all. I can not access my private files any more

          There should be at least CURL max waiting time specified or (preferably) not download file at all but just request the size/contenthash information

          Show
          Marina Glancy added a comment - This is very major: In my installation the function repository_equella::get_file_by_reference() invoked to update file size / contenthash actually tries to download a file from Equella and fails because of 30s script timeout which results with not loading filemanager at all. I can not access my private files any more There should be at least CURL max waiting time specified or (preferably) not download file at all but just request the size/contenthash information
          Hide
          Marina Glancy added a comment -

          following the situation described in previous comment I created MDL-34290

          Show
          Marina Glancy added a comment - following the situation described in previous comment I created MDL-34290
          Hide
          Adam Olley added a comment -

          "Actually now when I looked closer I don't like the idea of calling parent.M.core_filepicker.select_file(). It is unsafe and may be prohibited in some browsers. I need to come back to this issue later myself, maybe will have to change API."

          The call actually fails in IE8 with "'parent.M.core_filepicker' is null or not an object"

          Show
          Adam Olley added a comment - "Actually now when I looked closer I don't like the idea of calling parent.M.core_filepicker.select_file(). It is unsafe and may be prohibited in some browsers. I need to come back to this issue later myself, maybe will have to change API." The call actually fails in IE8 with "'parent.M.core_filepicker' is null or not an object"
          Hide
          Kien Vu added a comment -

          Hi Adam,

          How are you going with the IE8 and "'parent.M.core_filepicker' is null or not an object" issue?

          We are developing an Alfresco Moodle integration and having the same issue in IE8.

          Cheers,
          Kien

          Show
          Kien Vu added a comment - Hi Adam, How are you going with the IE8 and "'parent.M.core_filepicker' is null or not an object" issue? We are developing an Alfresco Moodle integration and having the same issue in IE8. Cheers, Kien
          Hide
          Kirill Astashov added a comment -

          Hi Kien Vu,

          We have no ideas at this stage, this is just how IE8 works and is should be one of its cross-site scripting prevention traps being triggered. To get rid of the error, I would recommend using another way to get information from remote servers (callback proxy scripts, etc).

          Show
          Kirill Astashov added a comment - Hi Kien Vu, We have no ideas at this stage, this is just how IE8 works and is should be one of its cross-site scripting prevention traps being triggered. To get rid of the error, I would recommend using another way to get information from remote servers (callback proxy scripts, etc).

            People

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

              Dates

              • Created:
                Updated: