Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make previously generated reports available for download #10280

Closed
2 of 3 tasks
Tracked by #10615
mkllnk opened this issue Jan 17, 2023 · 11 comments · Fixed by #10732
Closed
2 of 3 tasks
Tracked by #10615

Make previously generated reports available for download #10280

mkllnk opened this issue Jan 17, 2023 · 11 comments · Fixed by #10732
Assignees

Comments

@mkllnk
Copy link
Member

mkllnk commented Jan 17, 2023

What we should change and why (this is tech debt)

When a report takes a long time, the user may want to leave the desk and download the report later. It would be nice to tell the user that it's taking longer and provide a download link. To allow multiple downloads, we should delay report file deletion to later. We will need a cleanup task which deletes old reports for us.

Implementation

I think that we should leverage Active Storage for storing, downloading and deleting files.

  • Replace the temporary file in ReportJob with ActiveStorage::Blob (example ActiveStorage::Blob.create_and_upload!). The job will need to know the filename for download by the user.
  • Implement a 10 second timeout in Admin::ReportsController#show and display a nice message with the link. A URL can be generated with Rails.application.routes.url_helpers.url_for(blob). We need to check the expiry of the URL and may need to extend it.
  • Create an ActiveStorage::PurgeJob for each report blob: ActiveStorage::PurgeJob.set(wait: 1.week).perform_later(blob).

We may want a controller action at some point which checks if the report job completed and the file is available, or if it expired already to display nicer messages to the user. The Active Storage controller will just show an error message when the report is not there.

Context

Follow up from:

Next steps:

Impact and timeline

We are waiting for #10258 to be thoroughly tested in production. Once all instances are running stable with that, we can remove the feature toggle and start work on this issue.

@cyrillefr
Copy link
Contributor

Hello @mkllnk,
Is this feature for grab by the community ?
If so, is the testing feedback ok to allow this issue to be taken ?

@mkllnk
Copy link
Member Author

mkllnk commented Feb 22, 2023

Yes, you can grab this feature. It's a nice one to play with Active Storage. I'm looking forward to seeing your solution.

@cyrillefr
Copy link
Contributor

Hello @mkllnk
I have started working on it.
However, I am a bit stuck.

My idea was to display a list of downloadable reports somewhere on the page (do not know exactly, may be a 3rd column). Since reports are made within a Job, I thought that I could use CableReady/Stimulus to add a report since it is finished generating, by refreshing just the part we want.

On the web page,
there is a select to chose how you intend to do it: Screen/PDF/Spreadsheet.
For now, it is handled the classic way like that: classic controller,
if no choice(no params passed on POST) → classic screen/html rendering.
If param(choice) then: job -> download.

I could have done (in haml view): call to StimulusJS on the button:

  • no param → Form submission via JS
  • params: Stimulus → StimulusReflex → Job → page update
    ie : by seeing parameters or not, I know if screen vs PDF/Spreadsheet

BUT, you still have the possibility to generate PDF/Spreasheets the classic way. Which means I cannot make a simple choice in JS(Stimulus) since the parameters are server side.

So, if Jobs are handled only by Job, I can use my technique.
Or, may be, I did a bad choice/assumption with a refresh since the job has finished. I made the assumption it would be great to see a new link since the report is created.
Or may should I add a thumb/page somewhere.

Which direction should I take ? ( a bit of a long text, I hope I am clear enough :) I have no problem to explicit some bit of it if needed )

@mkllnk
Copy link
Member Author

mkllnk commented Feb 28, 2023

Hi! Thank you for working on this. And I had similar ideas when first thinking about this functionality. Having that overview of currently stored reports would be great but we currently don't have an association between user and generated report file.

So while it would be great to show the downloadable reports, I would prefer to implement quicker solutions first. This issue describes the case where you see a download link after waiting for ten seconds. And all this will be easier with Active Storage. Would you be willing to start there and maybe submit a PR for Active Storage?

@cyrillefr
Copy link
Contributor

Hello @mkllnk,

Thanks for the clarification. I am getting back at it.

@mkllnk
Copy link
Member Author

mkllnk commented Mar 28, 2023

Hi @cyrillefr, I'm getting to this now as well. Let me know if you have any WIP code already.

@cyrillefr
Copy link
Contributor

Hello @mkllnk , sorry for the delay.
Yes, I am almost done and have code.
I can surely make a PR today.

@mkllnk
Copy link
Member Author

mkllnk commented Apr 20, 2023

I think, I found the solution to add custom behaviour when the report is still being uploaded or has expired:
https://discuss.rubyonrails.org/t/active-storage-download-link-problem/80160/13

@cyrillefr
Copy link
Contributor

Very nice. I did not think one could that easily intercept ActiveStorage requests.

@mkllnk
Copy link
Member Author

mkllnk commented Apr 24, 2023

My latest PR sends links via email:

I faced a problem with the ActiveStorage redirect links though. Even though I supplied the expires_in option, they were always expiring after 5 minutes. So I'm using direct service links now because the expiration worked. With direct service links, you can't intercept the request because it goes directly to AWS S3. But then I'm only emailing the link when the data is actually there and let the link live for 30 days. That should minimise the need for a custom error message.

@cyrillefr
Copy link
Contributor

Mail is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment