Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Files API, Portfolio
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      Currently the download plugin, boxnet plugin, and base library are all using old file api functions.

      the plugins are marked by @todos and the function names in the base libarary are prefixed by temp_

        Gliffy Diagrams

          Activity

          Hide
          mjollnir Penny Leach added a comment -

          after a conversation with petr, it was decided that portfolio data should be put in portfolio_tempdata table and the id of that record would be used for the temp area for the files api ($itemid)

          Currently this table is opt-in for callers that want to access a lot of data across requests, but now I will switch to storign everything in it rather than the session.

          There needs also to be a cronjob to clean up records in this table from crashed requests (and associated files)

          Show
          mjollnir Penny Leach added a comment - after a conversation with petr, it was decided that portfolio data should be put in portfolio_tempdata table and the id of that record would be used for the temp area for the files api ($itemid) Currently this table is opt-in for callers that want to access a lot of data across requests, but now I will switch to storign everything in it rather than the session. There needs also to be a cronjob to clean up records in this table from crashed requests (and associated files)
          Hide
          mjollnir Penny Leach added a comment -

          problem with supporting multiple exports per session:

          15:29 < penny> gah, I just remembered one reason that two exports it the same session are not supported
          15:29 < penny> are there any repo guys here ?
          15:30 < nicolasconnault__> sorry, no
          15:30 < nicolasconnault__> better catch them early morning
          15:30 < nicolasconnault__> like fish
          15:30 < penny> stupid box.net
          15:30 < penny> it needs to redirect back to a url that I don't think can be set dynamically
          15:30 < penny> thus, you must store stuff in session
          15:31 < penny> to resume export after returning from the redirect to box.net

          Show
          mjollnir Penny Leach added a comment - problem with supporting multiple exports per session: 15:29 < penny> gah, I just remembered one reason that two exports it the same session are not supported 15:29 < penny> are there any repo guys here ? 15:30 < nicolasconnault__> sorry, no 15:30 < nicolasconnault__> better catch them early morning 15:30 < nicolasconnault__> like fish 15:30 < penny> stupid box.net 15:30 < penny> it needs to redirect back to a url that I don't think can be set dynamically 15:30 < penny> thus, you must store stuff in session 15:31 < penny> to resume export after returning from the redirect to box.net
          Show
          mjollnir Penny Leach added a comment - see: http://enabled.box.net/docs/rest#authentication
          Hide
          mjollnir Penny Leach added a comment -

          The more I think about this the more I am convinced I was right in the first place to not support multiple exports per session. We cannot assume that portfolio plugins are going to support multiple exports per session, therefore I don't think Moodle should.

          Show
          mjollnir Penny Leach added a comment - The more I think about this the more I am convinced I was right in the first place to not support multiple exports per session. We cannot assume that portfolio plugins are going to support multiple exports per session, therefore I don't think Moodle should.
          Hide
          mjollnir Penny Leach added a comment -

          Just committed a patch for this.

          I'm now storing the id of the export (matching portfolio_tempdata.id) in the session only.

          Petr if you can think of a clever work around for the problem I outlined above this is pretty easy to change later, but at the moment I'm stuck thinking we're limited to one per session.

          Show
          mjollnir Penny Leach added a comment - Just committed a patch for this. I'm now storing the id of the export (matching portfolio_tempdata.id) in the session only. Petr if you can think of a clever work around for the problem I outlined above this is pretty easy to change later, but at the moment I'm stuck thinking we're limited to one per session.
          Hide
          mjollnir Penny Leach added a comment -

          adding eloy.

          Show
          mjollnir Penny Leach added a comment - adding eloy.
          Hide
          skodak Petr Skoda added a comment - - edited

          I do not agree, session is not suitable for passing of information from one page to another, especially if used from several different contexts. The problem that some plugins do not allow concurrent communication is not solved by sessions, what if somebody opens two different browsers and works with them at the same time? I think it should be better to find out what is going by looking into portfolio tables instead of session.

          I really think you should use id from some table and pass it through all forms and build everything around this it. You might still use the session (or some serialised table field) to store the the auxiliary data, but the uniquie id is imho the key to make it all work. (ex. :$SESSION->prtfolioexportdata[$exportrequestid], I still do not like the serialisation of full blown objects either in events or session, I like arrays more there ) You said you want to store the unique id in session, I think you should instead use the unique id to "find" you information in session.

          Show
          skodak Petr Skoda added a comment - - edited I do not agree, session is not suitable for passing of information from one page to another, especially if used from several different contexts. The problem that some plugins do not allow concurrent communication is not solved by sessions, what if somebody opens two different browsers and works with them at the same time? I think it should be better to find out what is going by looking into portfolio tables instead of session. I really think you should use id from some table and pass it through all forms and build everything around this it. You might still use the session (or some serialised table field) to store the the auxiliary data, but the uniquie id is imho the key to make it all work. (ex. :$SESSION->prtfolioexportdata [$exportrequestid] , I still do not like the serialisation of full blown objects either in events or session, I like arrays more there ) You said you want to store the unique id in session, I think you should instead use the unique id to "find" you information in session.
          Hide
          mjollnir Penny Leach added a comment -

          I'm NOT storing data in session, just the portfolio request id, which is the id of the data.

          The only thing I changed my mind about was not supporting multiple exports during the same session, for the reasons outlined above. The only thing that is now stored in the session is the portfolioexport id.

          Show
          mjollnir Penny Leach added a comment - I'm NOT storing data in session, just the portfolio request id, which is the id of the data. The only thing I changed my mind about was not supporting multiple exports during the same session, for the reasons outlined above. The only thing that is now stored in the session is the portfolioexport id.
          Hide
          mjollnir Penny Leach added a comment -

          Just to be 100% explicit - storing the id in the session rather than passing it around in forms or as a request parameter - because that is not possible to do with all plugins. (what you call 'lazy')

          Show
          mjollnir Penny Leach added a comment - Just to be 100% explicit - storing the id in the session rather than passing it around in forms or as a request parameter - because that is not possible to do with all plugins. (what you call 'lazy')
          Hide
          skodak Petr Skoda added a comment -

          "because that is not possible to do with all plugins" - are you sure? As long as it is PHP or any other code stored on Moodle server it should be imho possible

          The difference above in session use is:
          1/ store data in $SESSION->mydata
          2/ store data in $SESSION->mydata[$uniqueid]

          The 1/ does not allow you to work with two different pages at the same time; 2/ keeps separate state for different pages depending on the unique id, in case of 2/ you can also store the data in database

          Show
          skodak Petr Skoda added a comment - "because that is not possible to do with all plugins" - are you sure? As long as it is PHP or any other code stored on Moodle server it should be imho possible The difference above in session use is: 1/ store data in $SESSION->mydata 2/ store data in $SESSION->mydata [$uniqueid] The 1/ does not allow you to work with two different pages at the same time; 2/ keeps separate state for different pages depending on the unique id, in case of 2/ you can also store the data in database
          Hide
          mjollnir Penny Leach added a comment - - edited

          Petr, I'm not sure you understand the problem correctly.

          Some plugins take you away from moodle entirely and then redirect you back to a url that is preconfigured per installation and does not include any dynamic content (eg portfoliotransferid) - in that case, when you land back at portfolio/add.php there is no way to determine which portfolio transfer you are returning to. Unless you are only supporting one.

          Eg box.net redirects you to wwwroot/portfolio/add.php?returncontrol=1 or something - that is configured at box.net and is not able to be configured per redirect-request. Eg no way to include the id of the current transfer it relates to.

          Show
          mjollnir Penny Leach added a comment - - edited Petr, I'm not sure you understand the problem correctly. Some plugins take you away from moodle entirely and then redirect you back to a url that is preconfigured per installation and does not include any dynamic content (eg portfoliotransferid) - in that case, when you land back at portfolio/add.php there is no way to determine which portfolio transfer you are returning to. Unless you are only supporting one . Eg box.net redirects you to wwwroot/portfolio/add.php?returncontrol=1 or something - that is configured at box.net and is not able to be configured per redirect-request. Eg no way to include the id of the current transfer it relates to.
          Hide
          skodak Petr Skoda added a comment -

          hmm, they do not allow you to pass some extra information
          ok, some would support only one active transfer - does it mean we must limit all of them?

          Just started to discover box.net, if I understand it correctly box.net may be configured to send back more information, right? Am I reading the right docs section?
          http://developers.box.net/OpenBoxBoxParameters
          http://developers.box.net/OpenBoxActions#CallbackParameters

          Show
          skodak Petr Skoda added a comment - hmm, they do not allow you to pass some extra information ok, some would support only one active transfer - does it mean we must limit all of them? Just started to discover box.net, if I understand it correctly box.net may be configured to send back more information, right? Am I reading the right docs section? http://developers.box.net/OpenBoxBoxParameters http://developers.box.net/OpenBoxActions#CallbackParameters
          Hide
          mjollnir Penny Leach added a comment -

          I was reading: http://enabled.box.net/docs/rest#authentication which is how to authenticate users (which is the exact step that is causing this problem). Try going to enabled.box.net and setting up an account (read http://docs.moodle.org/en/admin/manageportfolio/boxnet for instructions) - I cannot see how to make this include dynamic data to the return url.

          As far as:

          > ok, some would support only one active transfer - does it mean we must limit all of them?

          I honestly don't think that the use case for exporting multiple times at the same time is large enough to justify worrying about it this much.

          Perhaps we can elicit some response from Martin about this tomorrow.

          Show
          mjollnir Penny Leach added a comment - I was reading: http://enabled.box.net/docs/rest#authentication which is how to authenticate users (which is the exact step that is causing this problem). Try going to enabled.box.net and setting up an account (read http://docs.moodle.org/en/admin/manageportfolio/boxnet for instructions) - I cannot see how to make this include dynamic data to the return url. As far as: > ok, some would support only one active transfer - does it mean we must limit all of them? I honestly don't think that the use case for exporting multiple times at the same time is large enough to justify worrying about it this much. Perhaps we can elicit some response from Martin about this tomorrow.
          Hide
          mjollnir Penny Leach added a comment -

          Everything has been converted to use files api, except for the following outstanding issues:

          1. forum module (the rest of it doesn't seem to have been converted so I will wait)
          2. data module (needs to change ods/excel libs) - will start today
          3. download portfolio plugin - needs to move the files out of the temporary area into the user files area so they can download it - needs input from Petr.

          Show
          mjollnir Penny Leach added a comment - Everything has been converted to use files api, except for the following outstanding issues: 1. forum module (the rest of it doesn't seem to have been converted so I will wait) 2. data module (needs to change ods/excel libs) - will start today 3. download portfolio plugin - needs to move the files out of the temporary area into the user files area so they can download it - needs input from Petr.
          Hide
          mjollnir Penny Leach added a comment -

          todo still:

          • data module: subclasses of data_field_file need to be extracted and added separately
          Show
          mjollnir Penny Leach added a comment - todo still: data module: subclasses of data_field_file need to be extracted and added separately
          Hide
          mjollnir Penny Leach added a comment -

          last data module changes have been done. the ods/xls stuff isn't done but i'm making another bug for that.

          Show
          mjollnir Penny Leach added a comment - last data module changes have been done. the ods/xls stuff isn't done but i'm making another bug for that.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                24/Nov/10