Moodle

fix placeholders for file api stuff

Details

  • Type: Sub-task Sub-task
  • Status: Closed 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

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 (skodak) 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 (skodak) 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 (skodak) 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 (skodak) 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 (skodak) 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 (skodak) 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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: