closup / process-xbrl

3 stars 1 forks source link

Create sessions for the web app #7

Closed kwheelan closed 1 month ago

kwheelan commented 3 months ago

Goal

Right now, the web app writes and reads files from a static location. This will not work if multiple users access the site at the same time. We will need to implement sessions using Flask.

Tasks

lucakato commented 2 months ago

@kwheelan Hi, I think I have sessions working? at least for uploading input files. One question I have is being able to upload multiple files. I don't think any change I made on this branch is causing the issue because I tested it on the spinning wheel branch too, but I am not able to upload an Excel file and Google Doc file at the same time. Isn't this supposed to be possible? I've uploaded a video of what I mean: https://github.com/closup/process-xbrl/assets/55814205/508be244-a725-4a4c-bf74-6e7a3b2e8b85

Or is it just the way I'm selecting the files when the Finder window opens up? Do they have to do it in one session after clicking the button?

kwheelan commented 2 months ago

We should definitely fix this, but after the demo. I created #37 to document this issue.

For now, could you prioritize working on #28? I made a new branch 28 from this one so your changes to the sessions should be there as well. I'll merge both sets of changes into main from #28.

kwheelan commented 2 months ago

(And I'll leave this issue open until we implement a way to delete the folders after a user session ends.)

kwheelan commented 1 month ago

@lucakato Could you also make sure that the download button links to the correct output.html now that we have sessions? This will require editing line 11 of app/templates/site/upload.html to make the download link to the output file in the current session folder

lucakato commented 1 month ago

@kwheelan did you make this change when working on #28 ? or is this still something I can work on. thanks.

@lucakato Could you also make sure that the download button links to the correct output.html now that we have sessions? This will require editing line 11 of app/templates/site/upload.html to make the download link to the output file in the current session folder

kwheelan commented 1 month ago

@lucakato No -- not fixed yet. You can work on this

lucakato commented 1 month ago

@kwheelan Hi, could you remind me where converted_xbrl.html gets created? I noticed that now I don't see that html file being produced. this is for downloading the XBRL output.

my current idea is modifying the download location by making a download.js but I think I'd also need to pull in the current session data. Anyway, in the meantime I will move onto deleting the session folder.

kwheelan commented 1 month ago

@lucakato

This line defines the download:

https://github.com/closup/process-xbrl/blob/1b0813f51e63872b998e04a6fe0997632d9a20bc/app/templates/site/upload.html#L11

The download="converted_xbrl.html" means that when you download the file, it will be called converted_xbrl.html, but the first attribute (href) actually provides the document to be downloaded. Right now, it's set to static/output/output.html, but we'll want to change that to the session-specific file (which I think you've saved in static/sessions_data/<session-id>/output/output.html).

You can definitely do this with javascript if you prefer, but it might be easier to do it with jinja2 formatting. If you go with the jinja2 route, you'll want to pass the session id to the render_template() function here:

https://github.com/closup/process-xbrl/blob/1b0813f51e63872b998e04a6fe0997632d9a20bc/app/routes.py#L95

and then adjust the href in the download anchor in upload.html

Let me know if you have any questions

lucakato commented 1 month ago

@kwheelan I may have gotten it to work pls check my latest push on this branch for deleting sessions and try testing it.

It does work if you visit a different website, or try closing tab on Safari, or if you try to hold down Cmd+Q to quit Chrome.

However, for Chrome if I just click on the top-left window the red button or just cmd+w to close the tab, the prompt 'Are you sure you want to leave this site?' triggered by event.preventDefault() in delete_session.js does not appear and this results in sessions not being deleted.

It works for now for most cases but basically fails for some browser specific cases.

Also one bug that would need a fix is on Safari if you cmd+w and then choose 'stay on site' it still ends up deleting the session. Then if you cmd+w again to leave the site it gives a 404 because now the session folder doesn't exist but it tries to delete it.

kwheelan commented 1 month ago

@lucakato Thanks for working on this. For me, when I close the tab on Chrome, it deletes the folder as expected. If I cmd-W, it sometimes shows me the prompt on Chrome (about 2 out of the 4 times I tried), but deletes the folder even if I say "stay on site". For Safari, it just closes the window without a prompt (and deletes the folder). I'm not sure why the prompt isn't consistent.

Could you spend a bit more time trying to sort out these edge cases? We can chat about it/ do more testing post-demo.

lucakato commented 1 month ago

@kwheelan Yes, I've been looking into it. We may have to do it a different way though, couldn't find a well-supported way via beforeunload which is what I used.

lucakato commented 1 month ago

@kwheelan I may consider trying a totally different method that uses sessions and not detection of when a user closes tab/browser which seems unreliable.

The idea would be setting a session expiry and deleting all files associated with that session once a certain amount of time passes. Still need to research whether I would be using permanent or non-permanent sessions / also here.

Based on:

kwheelan commented 1 month ago

@lucakato That seems like a good approach! Thanks for doing the research on this

lucakato commented 1 month ago

@kwheelan I just pushed some changes. please check if it works for you too. I used a timeout feature added in helper_functions.py, set it to 60s for now. Ended up not using the flask-session.

After an upload is complete it will update the session timestamp, If you try to visit the home page (by going back to host URL) again, it will check if session has expired (compared to the timestamp of most recent successful upload) and delete all subfolders under session_data. I'm also thinking maybe it's worth adding a 'Return to Home' kind of button.

kwheelan commented 1 month ago

@lucakato Thanks for working on this. It seems like a perfect fix and works well for me. I'll work on merging this and the images changes in the dev branch (I'm going to hold off on changing the main branch until after the demo). I just added an issue to add a "Return to Home button" here: #52

If you have additional time, could you start looking through the ixbrl conversion logic on main (mainly in app/utils/xbrl_processing.py and in all the classes inside app/models/). This way you can come into the transition calls this week with any questions about how/why things work in a certain way.

kwheelan commented 1 month ago

You can also work on #37 if you have time (but Marc also asked for a home button, so #52 is higher priority)

lucakato commented 1 month ago

@kwheelan thanks sounds good, I'll have a look. I set it timeout to 60s but please change it to anything that seems more appropriate like 10 minutes if necessary. I'll do #52 first.

kwheelan commented 1 month ago

@lucakato I noticed that at least in Chrome, the download link and the view link both work even after the sessions folder is deleted (they must be automatically cached?), so the 60 seconds seems to work fine. It might be worth checking the same on other browsers, but 60 seconds seems like a good fix for now.

kwheelan commented 1 month ago

Merged in #51