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 #10629

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions app/controllers/admin/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module Admin
class ReportsController < Spree::Admin::BaseController
include ReportsActions
helper ReportsHelper
include ActiveStorage::SetCurrent

before_action :authorize_report, only: [:show]

Expand Down Expand Up @@ -31,11 +32,18 @@ def show
private

def export_report
send_data render_report_as(report_format), filename: report_filename
if OpenFoodNetwork::FeatureToggle.enabled?(:background_reports, spree_current_user)
assign_view_data
blob_or_message
render "show"
else
send_data @report.render_as(report_format), filename: report_filename
end
end

def show_report
assign_view_data
@table = @report.render_as(:html) if render_data?
render "show"
end

Expand All @@ -45,27 +53,22 @@ def assign_view_data
@report_subtype = report_subtype
@report_title = report_title
@rendering_options = rendering_options
@table = render_report_as(:html) if render_data?
@data = Reporting::FrontendData.new(spree_current_user)
end

def render_data?
request.post?
end

def render_report_as(format)
if OpenFoodNetwork::FeatureToggle.enabled?(:background_reports, spree_current_user)
job = ReportJob.new
JobProcessor.perform_forked(
job,
report_class, spree_current_user, params, format
)

# This result has been rendered by Rails in safe mode already.
job.result.html_safe # rubocop:disable Rails/OutputSafety
else
@report.render_as(format)
end
def blob_or_message
Timeout.timeout(ReportsHelper::JOB_TIMEOUT) {
blob = @report.report_from_job(report_format, spree_current_user, report_class, params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

flash.now[:ok_to_download] = blob
}
rescue Errno::ENOENT
flash.now[:ko_to_download] = I18n.t('admin.reports.errors.no_file_could_be_generated')
rescue StandardError
flash.now[:ko_to_download] = I18n.t('admin.reports.errors.report_generation_timed_out')
end
end
end
2 changes: 2 additions & 0 deletions app/helpers/reports_helper.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# frozen_string_literal: true

module ReportsHelper
JOB_TIMEOUT = 10

def report_order_cycle_options(order_cycles)
order_cycles.map do |oc|
orders_open_at = oc.orders_open_at&.to_s(:short) || 'NA'
Expand Down
13 changes: 6 additions & 7 deletions app/jobs/report_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ def done?
end

def result
@result ||= read_result
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)
Comment on lines +16 to +20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

blob
end

private
Expand All @@ -22,12 +27,6 @@ def write(result)
File.write(filename, result, mode: "wb")
end

def read_result
File.read(filename)
ensure
File.unlink(filename)
end

def filename
Rails.root.join("tmp/report-#{job_id}")
end
Expand Down
4 changes: 4 additions & 0 deletions app/views/spree/layouts/_admin_body.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
.flash.notice= notice
- if flash[:success]
.flash.success= flash[:success]
- if flash[:ok_to_download]
.msg.success= link_to I18n.t('admin.reports.report_download_your_file_ok'), flash[:ok_to_download].url(disposition: 'attachment')
- if flash[:ko_to_download]
.msg.error= flash[:ko_to_download]

= render partial: "shared/flashes"

Expand Down
4 changes: 3 additions & 1 deletion app/webpacker/css/admin/components/messages.scss
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
width: 100%;
z-index: 1000;

.flash {
.flash, .msg {
padding: 18px;
text-align: center;
font-size: 120%;
Expand All @@ -48,6 +48,8 @@
&:nth-child(2) { padding: 24px; }
&:nth-child(3) { padding: 20px; }
}

a, a:visited { color: $color-1; }
}

.notice:not(.flash) {
Expand Down
2 changes: 1 addition & 1 deletion app/webpacker/css/admin/openfoodnetwork.scss
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ text-angular .ta-editor {
left: 275px;
}

span.error, div.error:not(.flash) {
span.error, div.error:not(.flash, .msg) {
color: $warning-red;
}

Expand Down
2 changes: 2 additions & 0 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I have not thought of that.

end
end
3 changes: 3 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1422,6 +1422,7 @@ en:
pack_by_customer: Pack By Customer
pack_by_supplier: Pack By Supplier
pack_by_product: Pack By Product
report_download_your_file_ok: Please download your report
revenues_by_hub:
name: Revenues By Hub
description: Revenues by hub
Expand Down Expand Up @@ -1457,6 +1458,8 @@ en:
no_report_type: "Please specify a report type"
report_not_found: "Report not found"
missing_ransack_params: "Please supply Ransack search params in the request"
report_generation_timed_out: "Sorry, processing of your report have taken too much time"
no_file_could_be_generated: "No file could be generated"
hidden_field: "< Hidden >"
summary_row:
total: "TOTAL"
Expand Down
9 changes: 9 additions & 0 deletions lib/reporting/report_renderer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ def render_as(target_format)
public_send("to_#{target_format}")
end

def report_from_job(format, user, report_class, params)
job = ReportJob.new
JobProcessor.perform_forked(
job,
report_class, user, params, format
)
job.result
end

def to_html(layout: nil)
ApplicationController.render(
template: "admin/reports/_table",
Expand Down
3 changes: 2 additions & 1 deletion lib/reporting/report_template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ class ReportTemplate
include ReportsHelper
attr_accessor :user, :params, :ransack_params

delegate :render_as, :as_json, :to_html, :to_csv, :to_xlsx, :to_pdf, :to_json, to: :renderer
delegate :render_as, :report_from_job, :as_json, :to_html, :to_csv, :to_xlsx, :to_pdf, :to_json,
to: :renderer
delegate :unformatted_render?, :html_render?, :display_header_row?, :display_summary_row?,
to: :renderer

Expand Down
19 changes: 14 additions & 5 deletions spec/jobs/report_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
let(:enterprise) { create(:enterprise) }
let(:params) { {} }
let(:format) { :csv }
let(:configured_job) { instance_double(ActiveJob::ConfiguredJob) }

it "generates a report" do
job = ReportJob.new
Expand All @@ -20,17 +21,25 @@
job = ReportJob.perform_later(*report_args)
expect(job.done?).to eq false

# 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)
Comment on lines -23 to +24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thank you!


expect(job.done?).to eq true
expect_csv_report(job)
end

it 'sets a purge job on blob creation' do
allow(ActiveStorage::PurgeJob).to receive(:set).and_return(configured_job)
allow(configured_job).to receive(:perform_later)
job = ReportJob.new
job.perform(*report_args)
job.result

expect(ActiveStorage::PurgeJob).to have_received(:set).with(hash_including(:wait))
end

def expect_csv_report(job)
table = CSV.parse(job.result)
blob = job.result
table = CSV.parse(blob.download)
expect(table[0][1]).to eq "Relationship"
expect(table[1][1]).to eq "owns"
end
Expand Down
41 changes: 41 additions & 0 deletions spec/system/admin/reports_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -683,4 +683,45 @@ def xero_invoice_row(sku, description, amount, quantity, tax_type, opts = {})
'', 'N']
end
end

describe 'Generating report messages' do
before do
allow(OpenFoodNetwork::FeatureToggle).to receive(:enabled?).and_return(true)
@admin = login_as_admin
visit admin_reports_path
click_link "Order Cycle Supplier Totals"
select('PDF', from: 'report_format')
end

let(:report_class) { Reporting::Reports::OrdersAndFulfillment::OrderCycleSupplierTotals }

context 'When reporting job succeeded' do
before do
allow_any_instance_of(report_class).to receive(:report_from_job).and_return(blob)
click_button "Go"
end

let(:blob) { ActiveStorage::Blob.new(filename: 'example.pdf') }

it 'displays a flash message with a link to the report' do
expect(page).to have_content 'Please download your report'
end
end

context 'When reporting job failed/took too long' do
it 'displays a flash message noticing the error' do
allow_any_instance_of(report_class).to receive(:report_from_job).and_raise(Timeout::Error)
click_button "Go"
expect(page).to have_content 'Sorry, processing of your report have taken too much time'
end
end

context 'When Errno error from generation' do
it 'displays a flash message noticing no file could be generated' do
allow_any_instance_of(report_class).to receive(:report_from_job).and_raise(Errno::ENOENT)
click_button "Go"
expect(page).to have_content 'No file could be generated'
end
end
end
end