Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-33777

Equella repository needs to set field-source field correctly and use it to indicate source file too

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Repositories
    • Labels:

      Description

      Following on from MDL-33513

      repository/equella/lib.php needs two new functions:

      function get_file_source_info($filepath)
      .... returns 'Equella: originalfilename.ext' which came from filepicker which may not be the current name in Moodle.

      function get_reference_details($reference, $filestatus = 0)
      .... returns the data from the associated file->source field

      (I just tried to do this quickly myself but I'm getting a bit lost in the repository woods!)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dougiamas Martin Dougiamas added a comment -

            Dongsheng can you please look at this too? It was missing from the other patch MDL-33513

            Show
            dougiamas Martin Dougiamas added a comment - Dongsheng can you please look at this too? It was missing from the other patch MDL-33513
            Hide
            dongsheng Dongsheng Cai added a comment -

            Sure

            Show
            dongsheng Dongsheng Cai added a comment - Sure
            Hide
            dougiamas Martin Dougiamas added a comment -

            Thanks DS. Nick, does this look OK to you?

            Show
            dougiamas Martin Dougiamas added a comment - Thanks DS. Nick, does this look OK to you?
            Hide
            nickread Nick Read added a comment -

            It does for now given the limited time. After 2.3 there's a couple of areas like this that should change away from doing URL grabs like this (although this one is only a HEAD which is great) and instead using the information returned from EQUELLA during the original selection. That's a bigger fix that is capable for this release though.

            Show
            nickread Nick Read added a comment - It does for now given the limited time. After 2.3 there's a couple of areas like this that should change away from doing URL grabs like this (although this one is only a HEAD which is great) and instead using the information returned from EQUELLA during the original selection. That's a bigger fix that is capable for this release though.
            Hide
            dongsheng Dongsheng Cai added a comment -

            Wait a second, I didn't look the ticket carefully yesterday, I didn't add "get_reference_details" method, I'm gonna do this now.

            Show
            dongsheng Dongsheng Cai added a comment - Wait a second, I didn't look the ticket carefully yesterday, I didn't add "get_reference_details" method, I'm gonna do this now.
            Hide
            dongsheng Dongsheng Cai added a comment -

            repository_equella::get_reference_details() method added.

            Show
            dongsheng Dongsheng Cai added a comment - repository_equella::get_reference_details() method added.
            Hide
            dougiamas Martin Dougiamas added a comment - - edited

            Dongsheng, for future reference ... why are you grabbing the data like this in get_file_source_info()?

            Is it not possible to save the original name passed back to the filepicker? That's where I got lost, trying to find that point.

            Show
            dougiamas Martin Dougiamas added a comment - - edited Dongsheng, for future reference ... why are you grabbing the data like this in get_file_source_info()? Is it not possible to save the original name passed back to the filepicker? That's where I got lost, trying to find that point.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Also, now I'm looking at the new patch ... having to call CURL every time we call either of these statements is really scary. That was the whole point of storing it in files->source field ... so we could refer to it in future (it doesn't change).

            Show
            dougiamas Martin Dougiamas added a comment - Also, now I'm looking at the new patch ... having to call CURL every time we call either of these statements is really scary. That was the whole point of storing it in files->source field ... so we could refer to it in future (it doesn't change).
            Hide
            dongsheng Dongsheng Cai added a comment -

            Martin

            It's not possible at the moment, the filename isn't sent back to repository_ajax.php, in the future, we could send original file name to repository_ajax.php when click the select button if it exists.

            Show
            dongsheng Dongsheng Cai added a comment - Martin It's not possible at the moment, the filename isn't sent back to repository_ajax.php, in the future, we could send original file name to repository_ajax.php when click the select button if it exists.
            Hide
            dongsheng Dongsheng Cai added a comment -


            Also, now I'm looking at the new patch ... having to call CURL every time we call either of these statements is really scary. That was the whole point of storing it in files->source field ... so we could refer to it in future (it doesn't change).

            Martin, that's a HEAD request, no http body will be returned. We are not able to access source filed in get_reference_details method because we don't have stored_file instance here.

            Show
            dongsheng Dongsheng Cai added a comment - Also, now I'm looking at the new patch ... having to call CURL every time we call either of these statements is really scary. That was the whole point of storing it in files->source field ... so we could refer to it in future (it doesn't change). Martin, that's a HEAD request, no http body will be returned. We are not able to access source filed in get_reference_details method because we don't have stored_file instance here.
            Hide
            marina Marina Glancy added a comment -

            do we really need to send http request to get the name of the source file?
            can't we store this information?

            for exmple, just retrieve this once and store in files.source
            and every time we need get_reference_details parse it from there

            Show
            marina Marina Glancy added a comment - do we really need to send http request to get the name of the source file? can't we store this information? for exmple, just retrieve this once and store in files.source and every time we need get_reference_details parse it from there
            Hide
            poltawski Dan Poltawski added a comment -
            1. Whitespace:
              https://github.com/dongsheng/moodle/compare/integration...dev_MDL-33777_equella_sourcefield#L0R367
            2. We already discovered that using $USER unchecked causes problems, please avoid that.
            3. This 'grab_filename_from_url' looks really ugly. This should be coming from equella as metadata not hacking around with the http request.

            I don't like this at all.

            Show
            poltawski Dan Poltawski added a comment - Whitespace: https://github.com/dongsheng/moodle/compare/integration...dev_MDL-33777_equella_sourcefield#L0R367 We already discovered that using $USER unchecked causes problems, please avoid that. This 'grab_filename_from_url' looks really ugly. This should be coming from equella as metadata not hacking around with the http request. I don't like this at all.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Marina/Dan - that's what I said too, but DS says not possible.

            Show
            dougiamas Martin Dougiamas added a comment - Marina/Dan - that's what I said too, but DS says not possible.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated and testing.

            Show
            poltawski Dan Poltawski added a comment - Integrated and testing.
            Hide
            poltawski Dan Poltawski added a comment -

            Tested during integration.

            Show
            poltawski Dan Poltawski added a comment - Tested during integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yay. just in time for Moodle 2.3 release! Many thanks!

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yay. just in time for Moodle 2.3 release! Many thanks! Closing as fixed, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12