diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 79c5727eb1..9724d70967 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -692,8 +692,10 @@ Rails/ApplicationJob: - 'app/jobs/heartbeat_job.rb' - 'app/jobs/order_cycle_closing_job.rb' - 'app/jobs/order_cycle_notification_job.rb' + - 'app/jobs/report_job.rb' - 'app/jobs/subscription_confirm_job.rb' - 'app/jobs/subscription_placement_job.rb' + - 'spec/services/job_processor_spec.rb' # Offense count: 1 # This cop supports unsafe autocorrection (--autocorrect-all). diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index af09cfc77c..fe926a3441 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/app/controllers/admin/reports_controller.rb @@ -19,22 +19,22 @@ module Admin end def show - @report = report_class.new(spree_current_user, params, request) + @report = report_class.new(spree_current_user, params, render: render_data?) if report_format.present? export_report else - render_report + show_report end end private def export_report - send_data @report.render_as(report_format), filename: report_filename + send_data render_report_as(report_format), filename: report_filename end - def render_report + def show_report assign_view_data render "show" end @@ -43,13 +43,29 @@ module Admin @report_type = report_type @report_subtypes = report_subtypes @report_subtype = report_subtype - @report_title = if report_subtype - report_subtype_title - else - I18n.t(:name, scope: [:admin, :reports, @report_type]) - end + @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 + end end end diff --git a/app/controllers/concerns/reports_actions.rb b/app/controllers/concerns/reports_actions.rb index 4f6a8994c4..7fa6615733 100644 --- a/app/controllers/concerns/reports_actions.rb +++ b/app/controllers/concerns/reports_actions.rb @@ -39,6 +39,14 @@ module ReportsActions params[:report_subtype] || report_subtypes_codes.first end + def report_title + if report_subtype + report_subtype_title + else + I18n.t(:name, scope: [:admin, :reports, report_type]) + end + end + def report_subtype_title report_subtypes.select { |_name, key| key.to_sym == report_subtype.to_sym }.first[0] end diff --git a/app/jobs/report_job.rb b/app/jobs/report_job.rb new file mode 100644 index 0000000000..7143b3637c --- /dev/null +++ b/app/jobs/report_job.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +# Renders a report and saves it to a temporary file. +class ReportJob < ActiveJob::Base + def perform(report_class, user, params, format) + report = report_class.new(user, params, render: true) + result = report.render_as(format) + write(result) + end + + def done? + @done ||= File.file?(filename) + end + + def result + @result ||= read_result + end + + private + + def write(result) + File.write(filename, result) + end + + def read_result + File.read(filename) + ensure + File.unlink(filename) + end + + def filename + Rails.root.join("tmp/report-#{job_id}") + end +end diff --git a/app/services/job_processor.rb b/app/services/job_processor.rb new file mode 100644 index 0000000000..10ecb49cb1 --- /dev/null +++ b/app/services/job_processor.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +# Forks into a separate process to contain memory usage and timeout errors. +class JobProcessor + def self.perform_forked(job, *args) + # Reports should abort when puma threads are killed to avoid wasting + # resources. Nobody would be collecting the result. We still need to + # implement a way to email or download reports later. + timeout = ENV.fetch("RACK_TIMEOUT_WAIT_TIMEOUT", "30").to_i + + child = fork do + Process.setproctitle("Job worker #{job.job_id}") + Timeout.timeout(timeout) do + job.perform(*args) + end + + # Exit is not a good idea within a Rails process but Rubocop doesn't know + # that we are in a forked process. + exit # rubocop:disable Rails/Exit + end + + # Wait for all forked child processes to exit + Process.waitall + ensure + # If this Puma thread is interrupted then we need to detach the child + # process to avoid it becoming a zombie. + Process.detach(child) + end +end diff --git a/app/views/admin/reports/_table.html.haml b/app/views/admin/reports/_table.html.haml index 091d70e632..fce89c0ebd 100644 --- a/app/views/admin/reports/_table.html.haml +++ b/app/views/admin/reports/_table.html.haml @@ -1,5 +1,3 @@ -- report ||= @report - .report__table-container %table.report__table %thead diff --git a/app/views/admin/reports/show.html.haml b/app/views/admin/reports/show.html.haml index 92de53506a..3447401a19 100644 --- a/app/views/admin/reports/show.html.haml +++ b/app/views/admin/reports/show.html.haml @@ -19,7 +19,4 @@ - if request.post? %button.btn-print.icon-print{ onclick: "window.print()"}= t(:report_print) -/ We don't want to render data unless search params are supplied. -/ Compiling data can take a long time. -- if request.post? - = render "table" += @table diff --git a/lib/reporting/report_renderer.rb b/lib/reporting/report_renderer.rb index 29afa5dd39..d301c5a614 100644 --- a/lib/reporting/report_renderer.rb +++ b/lib/reporting/report_renderer.rb @@ -4,7 +4,7 @@ require 'spreadsheet_architect' module Reporting class ReportRenderer - REPORT_FORMATS = [:csv, :json, :xlsx, :pdf].freeze + REPORT_FORMATS = [:csv, :json, :html, :xlsx, :pdf].freeze def initialize(report) @report = report @@ -46,6 +46,14 @@ module Reporting public_send("to_#{target_format}") end + def to_html(layout: nil) + ApplicationController.render( + template: "admin/reports/_table", + layout: layout, + locals: { report: @report } + ) + end + def to_csv SpreadsheetArchitect.to_csv(headers: table_headers, data: table_rows) end @@ -55,11 +63,7 @@ module Reporting end def to_pdf - html = ApplicationController.render( - template: "admin/reports/_table", - layout: "pdf", - locals: { report: @report } - ) + html = to_html(layout: "pdf") WickedPdf.new.pdf_from_string(html) end diff --git a/lib/reporting/report_template.rb b/lib/reporting/report_template.rb index 9c2cc3fe00..16917347fe 100644 --- a/lib/reporting/report_template.rb +++ b/lib/reporting/report_template.rb @@ -5,7 +5,7 @@ module Reporting include ReportsHelper attr_accessor :user, :params, :ransack_params - delegate :render_as, :as_json, :to_csv, :to_xlsx, :to_pdf, :to_json, to: :renderer + delegate :render_as, :as_json, :to_html, :to_csv, :to_xlsx, :to_pdf, :to_json, to: :renderer delegate :raw_render?, :html_render?, :display_header_row?, :display_summary_row?, to: :renderer delegate :rows, :table_rows, :grouped_data, to: :rows_builder @@ -14,8 +14,8 @@ module Reporting delegate :formatted_rules, :header_option?, :summary_row_option?, to: :ruler - def initialize(user, params = {}, request = nil) - if request.nil? || request.get? + def initialize(user, params = {}, render: false) + unless render params.reverse_merge!(default_params) params[:q] ||= {} params[:q].reverse_merge!(default_params[:q]) if default_params[:q].present? diff --git a/lib/reporting/reports/enterprise_fee_summary/base.rb b/lib/reporting/reports/enterprise_fee_summary/base.rb index 99d17747a3..837c84df7d 100644 --- a/lib/reporting/reports/enterprise_fee_summary/base.rb +++ b/lib/reporting/reports/enterprise_fee_summary/base.rb @@ -6,8 +6,8 @@ module Reporting class Base < ReportTemplate attr_accessor :permissions, :parameters - def initialize(user, params = {}, request = nil) - super(user, params, request) + def initialize(user, params = {}, render: false) + super(user, params, render: render) p = params[:q] if p.present? p['start_at'] = p.delete('completed_at_gt') diff --git a/spec/jobs/report_job_spec.rb b/spec/jobs/report_job_spec.rb new file mode 100644 index 0000000000..9211ee0e44 --- /dev/null +++ b/spec/jobs/report_job_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ReportJob do + let(:report_args) { [report_class, user, params, format] } + let(:report_class) { Reporting::Reports::UsersAndEnterprises::Base } + let(:user) { enterprise.owner } + let(:enterprise) { create(:enterprise) } + let(:params) { {} } + let(:format) { :csv } + + it "generates a report" do + job = ReportJob.new + job.perform(*report_args) + expect_csv_report(job) + end + + it "enqueues a job for asynch processing" do + 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 + + expect(job.done?).to eq true + expect_csv_report(job) + end + + def expect_csv_report(job) + table = CSV.parse(job.result) + expect(table[0][1]).to eq "Relationship" + expect(table[1][1]).to eq "owns" + end +end diff --git a/spec/lib/reports/xero_invoices_report_spec.rb b/spec/lib/reports/xero_invoices_report_spec.rb index a8670a0f00..ab4a0e3b9b 100644 --- a/spec/lib/reports/xero_invoices_report_spec.rb +++ b/spec/lib/reports/xero_invoices_report_spec.rb @@ -26,8 +26,13 @@ module Reporting describe "summary rows" do let(:report) { - Base.new user, initial_invoice_number: '', invoice_date: '', due_date: '', - account_code: '' + Base.new(user, params) + } + let(:params) { + { + initial_invoice_number: '', invoice_date: '', due_date: '', + account_code: '' + } } let(:order) { double(:order) } let(:summary_rows) { report.__send__(:summary_rows_for_order, order, 1, {}) } @@ -84,7 +89,7 @@ module Reporting end describe "when an initial invoice number is given" do - subject { Base.new user, initial_invoice_number: '123' } + subject { Base.new(user, { initial_invoice_number: '123' }) } it "increments the number by the index" do expect(subject.send(:invoice_number_for, order, 456)).to eq(579) diff --git a/spec/services/job_processor_spec.rb b/spec/services/job_processor_spec.rb new file mode 100644 index 0000000000..041c80d5d4 --- /dev/null +++ b/spec/services/job_processor_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# We need to configure MiniRacer to allow forking. +# Otherwise this spec hangs on CI. +# https://github.com/rubyjs/mini_racer#fork-safety +require "mini_racer" +MiniRacer::Platform.set_flags!(:single_threaded) + +require 'spec_helper' + +class TestJob < ActiveJob::Base + def initialize + @file = Tempfile.new("test-job-result") + super + end + + def perform(message) + @file.write(message) + end + + def result + @file.rewind + @file.read + end +end + +describe JobProcessor do + describe ".perform_forked" do + let(:job) { TestJob.new } + + it "executes a job" do + JobProcessor.perform_forked(job, "hello") + + expect(job.result).to eq "hello" + end + end +end