Merge pull request #10258 from mkllnk/report-job

Render reports in a separate process (feature-toggled)

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

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

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

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

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

app/views/admin/reports/_table.html.haml

@@ -1,5 +1,3 @@
-- report ||= @report
-
 .report__table-container
   %table.report__table
     %thead

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

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
 

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?

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')

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

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)

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