Details

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

      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_

        Activity

        Hide
        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
        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
        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
        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
        Penny Leach added a comment - see: http://enabled.box.net/docs/rest#authentication
        Hide
        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
        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
        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
        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
        Penny Leach added a comment -

        adding eloy.

        Show
        Penny Leach added a comment - adding eloy.
        Hide
        Petr Škoda 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
        Petr Škoda 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
        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
        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
        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
        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
        Petr Škoda 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
        Petr Škoda 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
        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
        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
        Petr Škoda 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
        Petr Škoda 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
        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
        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
        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
        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
        Penny Leach added a comment -

        todo still:

        • data module: subclasses of data_field_file need to be extracted and added separately
        Show
        Penny Leach added a comment - todo still: data module: subclasses of data_field_file need to be extracted and added separately
        Hide
        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
        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: