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

mkllnk · 2023-04-05T05:50:32Z

What? Why?

Main part of Make previously generated reports available for download #10280
Replaces Make previously generated reports available for download #10629

Reports generated in the background are now stored with Active Storage and can be downloaded for up to one month. The download link is shown when the request times out. But there's no indication yet about the report being done or not. The UX is terrible. A websocket event would be nice to show the link only when the file is ready.

Review notes

Skip the first three commits which are the same as in #10644. I can rebase this PR after the other PR got merged but you can review already.

What should we test?

With feature background_reports:

Visit reports.
Request a report that takes too long.
A message should be displayed that it's taking longer and there should be a download link.
Following the download link will result in Not Found if the report needs more time.
The link will download the generated report once it's done.
There's no indication yet about it being done or not. The UX is terrible.

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

Dependencies

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

Documentation updates

jibees · 2023-04-12T08:45:38Z

Seems like #10644 is merged, I think we could rebase onto master this PR.

jibees

I just have one small comment.
Thanks that is clear!

jibees · 2023-04-12T08:52:41Z

app/controllers/admin/reports_controller.rb

@@ -57,7 +57,9 @@ def render_data?

    def render_report_as(format)
      if OpenFoodNetwork::FeatureToggle.enabled?(:background_reports, spree_current_user)
-        blob = ReportBlob.create_for_upload_later!
+        filename = report_filename
+        filename = "#{filename}html" if report_format.blank?


I'm wondering if this needs a .?

Suggested change

filename = "#{filename}html" if report_format.blank?

filename = "#{filename}.html" if report_format.blank?

Ah, good question. I can see it doesn't, because report_filename includes report_format like so:

"#{report_type || action_name}_#{file_timestamp}.#{report_format}"

So when blank, it leaves a trailing dot. I wonder if this problem should be solved inside ReportsActions.
Hmm.. yes there is already an assumption made on report_format.present?, which passes in :html as the format in this method.

So, I suggest a change to ReportsActions::report_filename to accept the format as a parameter:

Suggested change

filename = "#{filename}html" if report_format.blank?

filename = report_filename(format)

I'll have a look. I made this quick fix after finding that a lot more changes were needed to set the format to :html by default but your suggestion seems simpler.

dacook

Great, very clever use of your ActiveStorage knowledge. I learnt a thing or two!

I have some questions and a suggestion for improvement.

app/jobs/report_job.rb

dacook · 2023-04-13T01:15:56Z

app/jobs/report_job.rb

+    end
+  end
+
+  def perform(report_class, user, params, format, blob)
    report = report_class.new(user, params, render: true)
    result = report.render_as(format)


It would be cool if one day we could pass a stream to the report, so that it can write line-by-line rather than save the entire contents in a large variable...

Me too! Imagine we could read in batches with find_each and process the report in a stream. That would save a lot of memory. But we need the whole lot for grouping and sorting at the moment.

As an aside, I discovered yesterday what looks like an alternative report method, building an SQL query here: https://github.com/openfoodfoundation/openfoodnetwork/blob/master/lib/reporting/reports/enterprise_fee_summary/scope.rb

Ah, right. And I remember Matt introducing a new gem which makes custom SQL queries a lot more convenient. That's probably the way to go.

app/controllers/admin/reports_controller.rb

dacook · 2023-04-13T01:47:53Z

app/controllers/admin/reports_controller.rb

@@ -57,7 +57,9 @@ def render_data?

    def render_report_as(format)
      if OpenFoodNetwork::FeatureToggle.enabled?(:background_reports, spree_current_user)
-        blob = ReportBlob.create_for_upload_later!
+        filename = report_filename
+        filename = "#{filename}html" if report_format.blank?


Ah, good question. I can see it doesn't, because report_filename includes report_format like so:

"#{report_type || action_name}_#{file_timestamp}.#{report_format}"

So when blank, it leaves a trailing dot. I wonder if this problem should be solved inside ReportsActions.
Hmm.. yes there is already an assumption made on report_format.present?, which passes in :html as the format in this method.

So, I suggest a change to ReportsActions::report_filename to accept the format as a parameter:

Suggested change

filename = "#{filename}html" if report_format.blank?

filename = report_filename(format)

app/controllers/admin/reports_controller.rb

The production code calls `perform_later` and it's better to do the same in specs. Handy test helper allow us to control the execution. Credit to https://github.com/cyrillefr.

This will enable us to offer download links and clean them up automatically.

This makes the code heaps simpler.

For future downloads outside the ReportsController.

app/controllers/admin/reports_controller.rb

app/controllers/concerns/reports_actions.rb

dacook

Nice update!

filipefurtad0 · 2023-04-19T10:44:09Z

Hi @mkllnk ,

Before this PR:

This PR introduces the download link

Hum, somehow I could not download the report, and - for a given range - got this error:
https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/643fc2072915e90008ca984e?event_id=643fc20700bbc49df86d0000&i=sk&m=nw

This is probably is related to the data itself. But that brings about: how should the report download page handle reports which are broken, either due to the error above or some other.

I've later found a range in which no error is triggered, but still, no report download.

No sure this is ok, but I see a step forward, and this is behind a feature toggle - so I guess we'd be good to merge.

filipefurtad0 · 2023-04-19T10:46:52Z

Humpf. It's a bit nerve wrecking to wait for timeouts, only to find some data inconsistencies. I wonder if we can disable sidekiq to force the timeout and the link generation, an then re-enable to generate the download data... I'll give that a go.

filipefurtad0 · 2023-04-19T10:53:09Z

Cool - It works 🎉

It downloads the report as html - if no file type is selected:

Or as pdf:

Or as xlsx:

Or as csv:

Awesome 💪
Good to go!!!

mkllnk self-assigned this Apr 5, 2023

mkllnk mentioned this pull request Apr 5, 2023

Make previously generated reports available for download #10629

Closed

mkllnk marked this pull request as ready for review April 5, 2023 06:01

mkllnk force-pushed the report-download branch 2 times, most recently from 3ffdb81 to 3e89a11 Compare April 11, 2023 23:46

jibees approved these changes Apr 12, 2023

View reviewed changes

dacook requested changes Apr 13, 2023

View reviewed changes

mkllnk requested a review from dacook April 14, 2023 01:22

mkllnk added 8 commits April 14, 2023 11:29

Leverage test helpers for enqueued jobs

4f751c2

The production code calls `perform_later` and it's better to do the same in specs. Handy test helper allow us to control the execution. Credit to https://github.com/cyrillefr.

Fix typo

12848ac

Use ApplicationJob as it's a Rails default

71f9415

Store reports with Active Storage

ef7dfa3

This will enable us to offer download links and clean them up automatically.

Simplify ReportJob by extracting model for result

028c4f4

This makes the code heaps simpler.

Store report's filename within the blob

ebb1578

For future downloads outside the ReportsController.

Provide download link for generated reports

98e4f86

Simplify report filename generation

43cbac7

mkllnk force-pushed the report-download branch from 643686c to 43cbac7 Compare April 14, 2023 01:29

dacook reviewed Apr 14, 2023

View reviewed changes

app/controllers/admin/reports_controller.rb Show resolved Hide resolved

app/controllers/concerns/reports_actions.rb Show resolved Hide resolved

dacook approved these changes Apr 18, 2023

View reviewed changes

filipefurtad0 self-assigned this Apr 19, 2023

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Apr 19, 2023

filipefurtad0 merged commit f206b7e into openfoodfoundation:master Apr 19, 2023

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Apr 19, 2023

mkllnk deleted the report-download branch April 20, 2023 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mkllnk commented Apr 5, 2023 •

edited

Loading

jibees commented Apr 12, 2023

jibees left a comment

jibees Apr 12, 2023

dacook Apr 13, 2023

mkllnk Apr 14, 2023

dacook left a comment

dacook Apr 13, 2023

mkllnk Apr 14, 2023

dacook Apr 14, 2023

mkllnk Apr 16, 2023

dacook Apr 13, 2023

dacook left a comment

filipefurtad0 commented Apr 19, 2023 •

edited

Loading

filipefurtad0 commented Apr 19, 2023

filipefurtad0 commented Apr 19, 2023 •

edited

Loading

	filename = "#{filename}html" if report_format.blank?
	filename = "#{filename}.html" if report_format.blank?

	filename = "#{filename}html" if report_format.blank?
	filename = report_filename(format)

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

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

Conversation

mkllnk commented Apr 5, 2023 • edited Loading

What? Why?

Review notes

What should we test?

Release notes

Dependencies

Documentation updates

jibees commented Apr 12, 2023

jibees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Apr 19, 2023 • edited Loading

filipefurtad0 commented Apr 19, 2023

filipefurtad0 commented Apr 19, 2023 • edited Loading

mkllnk commented Apr 5, 2023 •

edited

Loading

filipefurtad0 commented Apr 19, 2023 •

edited

Loading

filipefurtad0 commented Apr 19, 2023 •

edited

Loading