Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.3
    • Component/s: Repositories
    • Labels:
    • Testing Instructions:
      Hide

      1. Ask Dongsheng's for EQUELLA demo server details
      2. Go to user private files, use EQUELLA repository to fetch a file
      3. Go to tinymce editor(forum post), pick an image from EQUELLA repository

      NOTE, the EQUELLA page is a bit big in filepicker, but EQUELLA team is working one a slim page for moodle, it doesn't require any changes in moodle when new theme integrated.

      Show
      1. Ask Dongsheng's for EQUELLA demo server details 2. Go to user private files, use EQUELLA repository to fetch a file 3. Go to tinymce editor(forum post), pick an image from EQUELLA repository NOTE, the EQUELLA page is a bit big in filepicker, but EQUELLA team is working one a slim page for moodle, it doesn't require any changes in moodle when new theme integrated.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      equella
    • Rank:
      38833

      Description

      Adding Equella repository to moodle core

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

          How ready is this?

          Show
          Martin Dougiamas added a comment - How ready is this?
          Hide
          Dongsheng Cai added a comment -

          The plugin is ready, Nick is working on the slim EQUELLA theme, it will fit the file picker better, currently it's a bit fat.

          Show
          Dongsheng Cai added a comment - The plugin is ready, Nick is working on the slim EQUELLA theme, it will fit the file picker better, currently it's a bit fat.
          Hide
          Martin Dougiamas added a comment -

          After a quick look at the code with Dongsheng I'm pretty confident that there is no problem integrating this.

          If there are problems with it then it would be confined to Equella users, and Equella would need to fix it.

          One smallish thing I'd like to see is more info displayed on the dialog directly after you select a file (in the file picker).

          ie Modified time, Created time, Author, License.

          This seems to require:

          1) Some work from Equella to return more info
          2) A little work in the filepicker to accept those.

          but my +1 to land this in 2.3 right away and work on those later.

          Show
          Martin Dougiamas added a comment - After a quick look at the code with Dongsheng I'm pretty confident that there is no problem integrating this. If there are problems with it then it would be confined to Equella users, and Equella would need to fix it. One smallish thing I'd like to see is more info displayed on the dialog directly after you select a file (in the file picker). ie Modified time, Created time, Author, License. This seems to require: 1) Some work from Equella to return more info 2) A little work in the filepicker to accept those. but my +1 to land this in 2.3 right away and work on those later.
          Hide
          Dongsheng Cai added a comment -

          Just added some TODO comments, so we remember to add more callback parameters later.

          Show
          Dongsheng Cai added a comment - Just added some TODO comments, so we remember to add more callback parameters later.
          Hide
          Dan Poltawski added a comment -

          Hi Dongsheng,

          Things questions that immediately strike me looking at this (not looked too deeply yet, I might be going up the wrong avenue):

          1. What does the cookiejar in the curl request do? I can't see anyything which actually sets/stores a cookie so i'm intruiged by it?
          2. Should the reference handling stuff (base64 encoding etc) be using file_storage::pack_reference and unpack_reference?
          3. I'm not sure about .'s in langfiles
          4. The get_all_editing_roles things I think need to come from accesslib functions rather than manually querying the tables in the plugins (I realise there might not be accesslib methods to do this right now, but I'd classify the internals of accesslib as too internal for the plugin itself to query)
          Show
          Dan Poltawski added a comment - Hi Dongsheng, Things questions that immediately strike me looking at this (not looked too deeply yet, I might be going up the wrong avenue): What does the cookiejar in the curl request do? I can't see anyything which actually sets/stores a cookie so i'm intruiged by it? Should the reference handling stuff (base64 encoding etc) be using file_storage::pack_reference and unpack_reference? I'm not sure about .'s in langfiles The get_all_editing_roles things I think need to come from accesslib functions rather than manually querying the tables in the plugins (I realise there might not be accesslib methods to do this right now, but I'd classify the internals of accesslib as too internal for the plugin itself to query)
          Hide
          Dongsheng Cai added a comment -

          Dan

          1. EQUELLA set JSESSION in cookie when serving the file, without it get_file() method won't work
          2. Yes, could do, I added pack_reference and unpack_reference to create moodle internal references, plugins doesn't have to use them
          3. Removed dots
          4. I am not a big fan of get_all_editing_roles too, but I think is better to make this plugin separated from moodle, we don't want to add new method just for a plugin...And EQUELLA plugin might change later to use better SSO options for different roles.
          Show
          Dongsheng Cai added a comment - Dan EQUELLA set JSESSION in cookie when serving the file, without it get_file() method won't work Yes, could do, I added pack_reference and unpack_reference to create moodle internal references, plugins doesn't have to use them Removed dots I am not a big fan of get_all_editing_roles too, but I think is better to make this plugin separated from moodle, we don't want to add new method just for a plugin...And EQUELLA plugin might change later to use better SSO options for different roles.
          Hide
          Dan Poltawski added a comment -
          1. Ah, curl won't keep the cookie, I see.
          2. Ah so this thing has nothing to do with those references?

          thanks

          Show
          Dan Poltawski added a comment - Ah, curl won't keep the cookie, I see. Ah so this thing has nothing to do with those references? thanks
          Hide
          Dongsheng Cai added a comment -

          yes, curl don't remember the cookie, so we have to store cookies in a file, the EQUELLA plugin's reference is just a simple url we used base64 encoding just for safe, it doesn't need to use the filestorage class to do that.

          Show
          Dongsheng Cai added a comment - yes, curl don't remember the cookie, so we have to store cookies in a file, the EQUELLA plugin's reference is just a simple url we used base64 encoding just for safe, it doesn't need to use the filestorage class to do that.
          Hide
          Matteo Scaramuccia added a comment -

          Hi,
          I'm pretty sure you already know it : cURL can read and store cookies and provide them on each call, see CURLOPT_COOKIEFILE and CURLOPT_COOKIEJAR in curl_setopt, keeping care of CURLOPT_COOKIESESSION too.

          Show
          Matteo Scaramuccia added a comment - Hi, I'm pretty sure you already know it : cURL can read and store cookies and provide them on each call, see CURLOPT_COOKIEFILE and CURLOPT_COOKIEJAR in curl_setopt , keeping care of CURLOPT_COOKIESESSION too.
          Hide
          Dan Poltawski added a comment -

          Hi Matteo, yes that is what is already in use.

          Show
          Dan Poltawski added a comment - Hi Matteo, yes that is what is already in use.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment - - edited

          Hi,

          I've done a few tweaks on my branch (rebased on current master)

          repo: git://github.com/danpoltawski/moodle.git
          branch: equella

          1. Various E_STRICT fixes: https://github.com/danpoltawski/moodle/commit/130b3f8c4f2bbf82865aca6ef6e8f3bc3d4b8932
          2. Remove unused global definitions: https://github.com/danpoltawski/moodle/commit/3e3910a58dcd839eb1b206fa56461b0c25ea1c7f
          3. Remove custom 'context tree' handling - user_has_capability searches parent contexts by default, so it doesn't need to check in the code category/site etc https://github.com/danpoltawski/moodle/commit/32f7f8586262a185a44c0095ad1b86b7fcff5a79
          4. Switch get_all_editing_roles to use the core function get_roles_with_capability https://github.com/danpoltawski/moodle/commit/7406ee9f57ed0fd7624d471e8acd05b871d75f34
          5. Make private functions private, and match function moodle coding style https://github.com/danpoltawski/moodle/commit/e38919c07003b4388fbf025942e43734546b784a
          Show
          Dan Poltawski added a comment - - edited Hi, I've done a few tweaks on my branch (rebased on current master) repo: git://github.com/danpoltawski/moodle.git branch: equella Various E_STRICT fixes: https://github.com/danpoltawski/moodle/commit/130b3f8c4f2bbf82865aca6ef6e8f3bc3d4b8932 Remove unused global definitions: https://github.com/danpoltawski/moodle/commit/3e3910a58dcd839eb1b206fa56461b0c25ea1c7f Remove custom 'context tree' handling - user_has_capability searches parent contexts by default, so it doesn't need to check in the code category/site etc https://github.com/danpoltawski/moodle/commit/32f7f8586262a185a44c0095ad1b86b7fcff5a79 Switch get_all_editing_roles to use the core function get_roles_with_capability https://github.com/danpoltawski/moodle/commit/7406ee9f57ed0fd7624d471e8acd05b871d75f34 Make private functions private, and match function moodle coding style https://github.com/danpoltawski/moodle/commit/e38919c07003b4388fbf025942e43734546b784a
          Hide
          Martin Dougiamas added a comment -

          Nick, we are working from Dan's branch now, please push changes there when you have them. See above.

          Show
          Martin Dougiamas added a comment - Nick, we are working from Dan's branch now, please push changes there when you have them. See above.
          Hide
          Martin Dougiamas added a comment -
          Show
          Martin Dougiamas added a comment - Added links to docs page http://docs.moodle.org/23/en/EQUELLA_repository
          Hide
          Dan Poltawski added a comment -

          I'm taking this out of the 'waiting for integration review' state, beacuse we are not there yet.

          Show
          Dan Poltawski added a comment - I'm taking this out of the 'waiting for integration review' state, beacuse we are not there yet.
          Hide
          Martin Dougiamas added a comment -

          I'm doing a last review of this now.

          Show
          Martin Dougiamas added a comment - I'm doing a last review of this now.
          Hide
          Martin Dougiamas added a comment -

          I think it's OK to land it. It's at least as good as the other repository plugins now

          Show
          Martin Dougiamas added a comment - I think it's OK to land it. It's at least as good as the other repository plugins now
          Hide
          Dan Poltawski added a comment -

          Making myself the assignee and removing from integrator since i've been doing a bit of the last push.

          Show
          Dan Poltawski added a comment - Making myself the assignee and removing from integrator since i've been doing a bit of the last push.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side note while seeing it in action. If there is no user logged (real or guest), it fails badly with:

          Notice: Undefined property: stdClass::$username in/repository/equella/lib.php on line 282
          

          Sure we'll need to have some workaround there for situations where $USER is not set.

          More tomorrow...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Side note while seeing it in action. If there is no user logged (real or guest), it fails badly with: Notice: Undefined property: stdClass::$username in/repository/equella/lib.php on line 282 Sure we'll need to have some workaround there for situations where $USER is not set. More tomorrow...ciao
          Hide
          Dan Poltawski added a comment -

          Replication steps from Eloy:
          1) logged as admin, go to one course->intro and pick one image from equella.
          2) go to site page and the course description shows the image ok.
          3) logout: image out.
          4) clean all browser caches
          5) image broken with the error above (still logged out).

          Show
          Dan Poltawski added a comment - Replication steps from Eloy: 1) logged as admin, go to one course->intro and pick one image from equella. 2) go to site page and the course description shows the image ok. 3) logout: image out. 4) clean all browser caches 5) image broken with the error above (still logged out).
          Hide
          Dan Poltawski added a comment -

          Have pushed a fix for the issue reported by Eloy.

          We could clean this up more by using moodle_url for url parsing I think.

          Show
          Dan Poltawski added a comment - Have pushed a fix for the issue reported by Eloy. We could clean this up more by using moodle_url for url parsing I think.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sent upstream, I've added two commits, fixing some phpdocs, missing MOODLE_INTERNAL and making the plugin core one.

          Show
          Eloy Lafuente (stronk7) added a comment - Sent upstream, I've added two commits, fixing some phpdocs, missing MOODLE_INTERNAL and making the plugin core one.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm passing this because I was able to access to the requella repo both from the private files UI and the editor (at course and activity level).

          3 points, for your consideration:

          1) I got 2-3 JSON errors in the private-files UI (although the images ended properly referenced).

          2) Also, I've noticed that it's possible to make references to equella files in private-files area when picking images, for example to forum intro. Is that correct? To reference a reference?

          3) When picking files from the "server files" repository, it's possible to pick "equella files" from places where you have picked them before (for example course->intro). Here there is not option to select "copy or link", but I guess it's always link. Also it's really slow to navigate on that repository if there are too many "external" files involved.

          So passing, plz, take actions about the 3 points above...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm passing this because I was able to access to the requella repo both from the private files UI and the editor (at course and activity level). 3 points, for your consideration: 1) I got 2-3 JSON errors in the private-files UI (although the images ended properly referenced). 2) Also, I've noticed that it's possible to make references to equella files in private-files area when picking images, for example to forum intro. Is that correct? To reference a reference? 3) When picking files from the "server files" repository, it's possible to pick "equella files" from places where you have picked them before (for example course->intro). Here there is not option to select "copy or link", but I guess it's always link. Also it's really slow to navigate on that repository if there are too many "external" files involved. So passing, plz, take actions about the 3 points above...ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao
          Hide
          Dan Poltawski added a comment -

          Regarding 2/ and 3/ I think they are addressed in MDL-33446, hopefully Marina can confirm.

          Show
          Dan Poltawski added a comment - Regarding 2/ and 3/ I think they are addressed in MDL-33446 , hopefully Marina can confirm.
          Hide
          Marina Glancy added a comment -

          Answering to Eloy points:
          1. I got some errors too, need to check why it happened
          2. should not be allowed. Issue MDL-33452 - in progress
          3. the issue MDL-33420 (picking from server files by reference) is not integrated yet, so they are always picked as copies now (therefore there are no options). But the current code just creates a copy of reference. After MDL-33452 lands, the file will be either copied to moodle or copy of reference will be created (pointing to the same source).

          Re: slow navigation with many external references. I'm pretty sure there are some poorly indexed queries. Hopefully David was fixing DB.

          Show
          Marina Glancy added a comment - Answering to Eloy points: 1. I got some errors too, need to check why it happened 2. should not be allowed. Issue MDL-33452 - in progress 3. the issue MDL-33420 (picking from server files by reference) is not integrated yet, so they are always picked as copies now (therefore there are no options). But the current code just creates a copy of reference. After MDL-33452 lands, the file will be either copied to moodle or copy of reference will be created (pointing to the same source). Re: slow navigation with many external references. I'm pretty sure there are some poorly indexed queries. Hopefully David was fixing DB.
          Hide
          Martin Dougiamas added a comment -


          Re: slowness. I really hope there is not something like filesize queries forcing re-downloads.

          Show
          Martin Dougiamas added a comment - Re: slowness. I really hope there is not something like filesize queries forcing re-downloads.
          Hide
          Helen Foster added a comment -

          Thanks to Martin for documenting this new feature:

          Removing the docs_required label from this issue.

          Show
          Helen Foster added a comment - Thanks to Martin for documenting this new feature: http://docs.moodle.org/23/en/EQUELLA_repository http://docs.moodle.org/23/en/Capabilities/repository/equella:view Removing the docs_required label from this issue.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: