Make previously generated reports available for download #10629

cyrillefr · 2023-03-28T20:59:01Z

What? Why?

Closes Make previously generated reports available for download #10280

What should we test?

Visit http://localhost:3000/admin/reports
if report is configured as background job (/admin/feature-toggle/features) then a flash message should display
a link to a report if everything ok, or a flash message displaying an error notice.

To change the validity of the url: config.active_storage.service_urls_expire_in in config/application.rb

Timeout generation is set to 10 second and can be changed in app/helpers/reports_helper.rb

Notes

Display when user wants to download report that is not valid no more due to time is not yet implemented(white screen).

I did not make use of Stimulus etc., but rather did it the "classical" way because too many options for the report(background or not, screen or pdf/xlsx).

cyrillefr · 2023-03-28T21:43:41Z

The failing spec is ok on my machine ....

mkllnk

Great work, thank you!

I think that we should put this on hold until this is merged:

Run background reports with Sidekiq, not fork #10615

We will need to change a few things afterwards. I also have some ideas for improvements. For example, this version doesn't allow reports to take longer than 10 seconds while some reports take 10 minutes. So we need a way to display the message after 10 seconds but still allow the download of the report later when it's done.

I'm happy to help with this and co-work on this pull request. Let's just wait for the first PR to be merged though.

mkllnk · 2023-03-29T02:20:38Z

config/application.rb

@@ -249,5 +249,7 @@ module ::Reporting; end
    config.active_storage.variable_content_types += ["image/svg+xml"]

    config.exceptions_app = self.routes
+
+    config.active_storage.service_urls_expire_in = 60.minutes


I guess that his applies for all ActiveStorage URLs? We should probably limit this to the reports.

You are right. I have not thought of that.

mkllnk · 2023-03-29T02:23:03Z

app/controllers/admin/reports_controller.rb

-      end
+    def blob_or_message
+      Timeout.timeout(ReportsHelper::JOB_TIMEOUT) {
+        blob = @report.report_from_job(report_format, spree_current_user, report_class, params)


It looks like this method doesn't exist yet in this commit.

mkllnk · 2023-03-29T02:26:07Z

app/jobs/report_job.rb

+    blob = ActiveStorage::Blob.create_and_upload!(io: File.open(filename), filename: filename)
+    ActiveStorage::PurgeJob
+      .set(wait: Rails.configuration.active_storage.service_urls_expire_in)
+      .perform_later(blob)
+    File.unlink(filename)


Hm, so we still store the file and then upload it. I was hoping to avoid the local storage and upload directly. It means that we need to find a different implementation of ReportJob#done?.

Yes, I was hoping so too. I wanted to get rid of the first temp save. But from what I understand of create_and_upload, you need to have first a local temp file to then create a blob.

mkllnk · 2023-03-29T02:27:13Z

spec/jobs/report_job_spec.rb

-    # This performs the job in the same process but that's good enought for
-    # testing the job code. I hope that we can rely on the job worker.
-    ActiveJob::Base.queue_adapter.perform_enqueued_jobs = true
-    job.retry_job
+    perform_enqueued_jobs(only: ReportJob)


Much better, thank you!

cyrillefr · 2023-03-29T20:28:25Z

Hello @mkllnk, I just saw #10615 have just been merged.
I also now have conflict with master.
What do you say: I rebase, update the code and re push it then we can talk about what remains to close this PR ?
I think I can have this done for this week-end.

mkllnk · 2023-04-03T07:31:14Z

I'm currently doing some small iterations to ship improvements quickly. Here's the next pull request:

[Hidden] Display a friendly message when a background report times out #10644

I just looked through your commits again and I think that the change in background processing means that it's better to start fresh. You are right that the official Active Storage API requires the file first but I found a workaround which I have stashed on my local machine at the moment. I'll see if I can show you something by tomorrow.

mkllnk · 2023-04-03T08:20:40Z

Work in progress:

master...mkllnk:openfoodnetwork:report-download

cyrillefr · 2023-04-04T13:04:13Z

Hello @mkllnk ,
I have reworked a bit on my first code.
Especially about the link that:

delivers the document
displays a message telling user doc is no longer valid(user has waited too long)

Front: a link that points to a method dl in reports controller (not any more a direct link to a blob)
In my dl method:

def dl
  blob = ActiveStorage::Blob.find_signed!(params[:signed_id])
  send_data blob.download, filename: blob.filename.to_s
  
  rescue ActiveSupport::MessageVerifier::InvalidSignature
  flash[:ko_to_download] = 'Too late ...'
  redirect_to admin_reports_url
end

In the export_report method in reports controller:

blob.update(filename: report_filename) # update blob fname here
@signed_id = blob.signed_id(purpose: :blob_id, expires_in: 5.seconds)
flash.now[:ok_to_download] = 'All went well'

To be able to find out if a stored document is still valid, I use signed_id and find_id.
As you can see, the signed_id method take an expires_in argument, so it is easy to set the validity of the document.
Also, very flexible in matter of configuration.
In the dl method, all I have to do is to use find_signed, if the signed_id is no more valid, means user has waited too long to download it.
We can use like a temp key for a blob. If no more valid, rescue, then flash message + redirect.
Front link would something like this:
.msg.success= link_to I18n.t('admin.reports.report_download_your_file_ok'), admin_report_path('dl', @signed_id)

If you think this snippets can be of any value, you can use it.
I wonder why I did not think of that before 😄

mkllnk · 2023-04-05T05:57:17Z

That's awesome research. And I wonder if we can still simplify the solution. Have a look at my PR:

[Hidden] Provide download link for reports generated in the background #10661

What I'm missing there is the expiry in the link which should be easy to add. But I'm also wondering what the default Rails controller does then. Is it just displaying something like "this is expired"? That would be good enough. We don't need to implement our own controller action and flashes if Rails does this for us.

Can you find out what Rails displays by default and if we can customise it? It would be good to maybe translate based on a user login or put our own layout around it.

Using the Rails controller also means that we can direct people straight to the CDN and we don't have to proxy the file once it's uploaded. This is especially useful once we include that link in an email sent after the report was generated.

mkllnk · 2023-04-05T05:58:56Z

Would you like to work on the expiry in the link and in the 10 second timeout within the controller? I'm now offline till Tuesday. So you can go wild without me interfering. 😉

mkllnk · 2023-04-05T06:40:04Z

I just tried adding expires_in: 10.seconds to the URL generation and when it's expired, the default Rails controller just returns 404 Not Found. That's with local storage. The message may be different with AWS S3. But I think that if we only display the link once the report is generated and keep the file for 1 month then we don't need to think about expiry.

I'm not sure if we can customise the Rails controller at all and if it's worth it.

cyrillefr · 2023-04-10T21:01:52Z

Hello @mkllnk ,

I have tried many combinations to try to figure out how to do this.

I got help from that blog post
https://blog.saeloun.com/2021/09/14/rails-7-adds-expiring-urls-to-active-storage

url_for does not allow to set expiry and also locally it does not work( I have a report_blob_path method missing error).

And contrary to what the doc says, rails_blob_path does not allow to set expiry. Actually, expiry time is the 5 mn default.

The url method allows for setting expiration time. It goes to the show method of the DiskController(locally):
https://api.rubyonrails.org/v7.0.4.2/classes/ActiveStorage/DiskController.html
Doc says one should not override/alter DiskController but rather BlobsController.

But I never could get a binding stop in a RedirectController.

DiskController redirect to a head: :not_found, hence the 404 and the blank page. But unfortunately, that is not the proper place to modify.

The url_for method is said to go through the RedirectController(cf. https://guides.rubyonrails.org/active_storage_overview.html#redirect-mode), but it crashes locally I do not know why. But since url_for is for a permanent url…
I also looked for a middleware, then I realised I have gone too far in delusion :)
All in all, I am clueless.

I think your idea to display the link once the report is done is good. But that would mean a second message to display after a first one that says report processing take a lot of time. Which implies another get/post.
My take would therefore to use some StimulusReflex to send one or two message in the report page to avoid multi submit and losing track of the blob/report(or one should pass the blob id in url or post).

mkllnk · 2023-04-11T07:20:01Z

Wow, thanks for that research.

use some StimulusReflex to send one or two message in the report page to avoid multi submit

Yes. I guess that we can use the websocket (cable) connection to push the event of the generated report to the browser. But that would be the first push event in OFN. I need to learn how do that. 😄

sigmundpetersen · 2023-05-16T07:17:36Z

@mkllnk @cyrillefr should we close this? Or is there still anything here we would like to keep?

mkllnk · 2023-05-17T02:34:20Z

Yes, this has been replaced. Thank you @cyrillefr for all your input here.

cyrillefr added 7 commits March 28, 2023 22:10

Create a configuration for url expiration

5f3329b

Refactor code for handling new flash messages in controller

f84a298

Refactor job to include Blob creation + delayed purge blob

a4e377e

Add front part for flash messages

20c6b4a

Add localization/wording

1bb5b78

Refactor code previously in controller

17db3a9

Add system specs

a339877

mkllnk requested changes Mar 29, 2023

View reviewed changes

mkllnk mentioned this pull request Apr 5, 2023

[Hidden] Provide download link for reports generated in the background #10661

Merged

mkllnk closed this May 17, 2023

cyrillefr deleted the Make-previously-generated-reports-available-for-download branch February 26, 2024 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make previously generated reports available for download #10629

Make previously generated reports available for download #10629

cyrillefr commented Mar 28, 2023

cyrillefr commented Mar 28, 2023

mkllnk left a comment

mkllnk Mar 29, 2023

cyrillefr Mar 29, 2023

mkllnk Mar 29, 2023

mkllnk Mar 29, 2023

cyrillefr Mar 29, 2023

mkllnk Mar 29, 2023

cyrillefr commented Mar 29, 2023

mkllnk commented Apr 3, 2023

mkllnk commented Apr 3, 2023

cyrillefr commented Apr 4, 2023

mkllnk commented Apr 5, 2023

mkllnk commented Apr 5, 2023

mkllnk commented Apr 5, 2023

cyrillefr commented Apr 10, 2023

mkllnk commented Apr 11, 2023

sigmundpetersen commented May 16, 2023

mkllnk commented May 17, 2023

Make previously generated reports available for download #10629

Make previously generated reports available for download #10629

Conversation

cyrillefr commented Mar 28, 2023

What? Why?

What should we test?

Notes

cyrillefr commented Mar 28, 2023

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Mar 29, 2023

Choose a reason for hiding this comment

cyrillefr Mar 29, 2023

Choose a reason for hiding this comment

mkllnk Mar 29, 2023

Choose a reason for hiding this comment

mkllnk Mar 29, 2023

Choose a reason for hiding this comment

cyrillefr Mar 29, 2023

Choose a reason for hiding this comment

mkllnk Mar 29, 2023

Choose a reason for hiding this comment

cyrillefr commented Mar 29, 2023

mkllnk commented Apr 3, 2023

mkllnk commented Apr 3, 2023

cyrillefr commented Apr 4, 2023

mkllnk commented Apr 5, 2023

mkllnk commented Apr 5, 2023

mkllnk commented Apr 5, 2023

cyrillefr commented Apr 10, 2023

mkllnk commented Apr 11, 2023

sigmundpetersen commented May 16, 2023

mkllnk commented May 17, 2023