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

          Attachments

            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