Details

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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

            Assiging to Nick from Equella

            Show
            poltawski Dan Poltawski added a comment - Assiging to Nick from Equella
            Hide
            marina 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 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
            nickread 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
            nickread 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 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 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
            poltawski Dan Poltawski added a comment -

            removing 7 as its covered in MDL-33724

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

            ps. just noticed there was no 6

            Show
            poltawski Dan Poltawski added a comment - ps. just noticed there was no 6
            Hide
            dougiamas 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
            dougiamas 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
            nickread Nick Read added a comment -

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

            Show
            nickread Nick Read added a comment - Dongsheng now has repository_equella::get_reference_details() ready in MDL-33777
            Hide
            marina 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 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 Marina Glancy added a comment -

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

            Show
            marina Marina Glancy added a comment - following the situation described in previous comment I created MDL-34290
            Hide
            aolley 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
            aolley 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
            kienvu 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
            kienvu 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
            badblock 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
            badblock 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: