Merge pull request #12782 from mkllnk/reports

Add fallback report loading in case websockets fail

Gemfile

@@ -104,6 +104,7 @@ gem 'sidekiq-scheduler'
 gem "cable_ready"
 gem "stimulus_reflex"
 
+gem "turbo_power"
 gem "turbo-rails"
 
 gem 'combine_pdf'

Gemfile.lock

@@ -785,6 +785,8 @@ GEM
       actionpack (>= 6.0.0)
       activejob (>= 6.0.0)
       railties (>= 6.0.0)
+    turbo_power (0.6.2)
+      turbo-rails (>= 1.3.0)
     tzinfo (2.0.6)
       concurrent-ruby (~> 1.0)
     unicode-display_width (2.5.0)
@@ -973,6 +975,7 @@ DEPENDENCIES
   stripe
   timecop
   turbo-rails
+  turbo_power
   valid_email2
   validates_lengths_from_database
   vcr

app/controllers/admin/reports_controller.rb

@@ -6,7 +6,7 @@ class ReportsController < Spree::Admin::BaseController
     include ReportsActions
     helper ReportsHelper
 
-    before_action :authorize_report, only: [:show]
+    before_action :authorize_report, only: [:show, :create]
 
     # Define model class for Can? permissions
     def model_class
@@ -20,14 +20,17 @@ def index
     end
 
     def show
-      @report = report_class.new(spree_current_user, params, render: render_data?)
-      @rendering_options = rendering_options # also stores user preferences
+      @report = report_class.new(spree_current_user, params, render: false)
+      @rendering_options = rendering_options
 
-      if render_data?
-        render_in_background
-      else
-        show_report
-      end
+      show_report
+    end
+
+    def create
+      @report = report_class.new(spree_current_user, params, render: true)
+      update_rendering_options
+
+      render_in_background
     end
 
     private
@@ -54,31 +57,15 @@ def load_selected_variant
       @variant_serialized = Api::Admin::VariantSerializer.new(variant)
     end
 
-    def render_data?
-      request.post?
-    end
-
     def render_in_background
-      cable_ready[ScopedChannel.for_id(params[:uuid])]
-        .inner_html(
-          selector: "#report-go",
-          html: helpers.button(t(:go), "report__submit-btn", "submit", disabled: true)
-        ).inner_html(
-          selector: "#report-table",
-          html: render_to_string(partial: "admin/reports/loading")
-        ).scroll_into_view(
-          selector: "#report-table",
-          block: "start"
-        ).broadcast
+      @blob = ReportBlob.create_for_upload_later!(report_filename)
 
       ReportJob.perform_later(
         report_class:, user: spree_current_user, params:,
         format: report_format,
-        filename: report_filename,
+        blob: @blob,
         channel: ScopedChannel.for_id(params[:uuid]),
       )
-
-      head :no_content
     end
   end
 end

app/controllers/concerns/reports_actions.rb

@@ -88,14 +88,10 @@ def rendering_options
         display_header_row: false
       }
     end
-    update_rendering_options
-    @rendering_options
   end
 
   def update_rendering_options
-    return unless request.post?
-
-    @rendering_options.update(
+    rendering_options.update(
       options: {
         fields_to_show: params[:fields_to_show],
         display_summary_row: params[:display_summary_row].present?,

app/jobs/report_job.rb

@@ -9,12 +9,12 @@ class ReportJob < ApplicationJob
 
   NOTIFICATION_TIME = 5.seconds
 
-  def perform(report_class:, user:, params:, format:, filename:, channel: nil)
+  def perform(report_class:, user:, params:, format:, blob:, channel: nil)
     start_time = Time.zone.now
 
     report = report_class.new(user, params, render: true)
     result = report.render_as(format)
-    blob = ReportBlob.create!(filename, result)
+    blob.store(result)
 
     execution_time = Time.zone.now - start_time
 

app/models/report_blob.rb

@@ -5,7 +5,7 @@ class ReportBlob < ActiveStorage::Blob
   # AWS S3 limits URL expiry to one week.
   LIFETIME = 1.week
 
-  def self.create!(filename, content)
+  def self.create_locally!(filename, content)
     create_and_upload!(
       io: StringIO.new(content),
       filename:,
@@ -15,11 +15,34 @@ def self.create!(filename, content)
     )
   end
 
+  def self.create_for_upload_later!(filename)
+    # ActiveStorage discourages modifying a blob later but we need a blob
+    # before we know anything about the report file. It enables us to use the
+    # same blob in the controller to read the result.
+    create_before_direct_upload!(
+      filename:,
+      byte_size: 0,
+      checksum: "0",
+      content_type: content_type(filename),
+      service_name: :local,
+    ).tap do |blob|
+      ActiveStorage::PurgeJob.set(wait: LIFETIME).perform_later(blob)
+    end
+  end
+
   def self.content_type(filename)
     MIME::Types.of(filename).first&.to_s || "application/octet-stream"
   end
 
+  def store(content)
+    io = StringIO.new(content)
+    upload(io, identify: false)
+    save!
+  end
+
   def result
+    return if checksum == "0"
+
     @result ||= download.force_encoding(Encoding::UTF_8)
   end
 

app/models/spree/ability.rb

@@ -244,7 +244,7 @@ def add_product_management_abilities(user)
 
       # Reports page
       can [:admin, :index, :show], ::Admin::ReportsController
-      can [:admin, :show, :customers, :orders_and_distributors, :group_buys, :payments,
+      can [:admin, :show, :create, :customers, :orders_and_distributors, :group_buys, :payments,
            :orders_and_fulfillment, :products_and_inventory, :order_cycle_management,
            :packing, :enterprise_fee_summary, :bulk_coop], :report
     end
@@ -324,7 +324,7 @@ def add_order_management_abilities(user)
       end
 
       # Reports page
-      can [:admin, :index, :show], ::Admin::ReportsController
+      can [:admin, :index, :show, :create], ::Admin::ReportsController
       can [:admin, :customers, :group_buys, :sales_tax, :payments,
            :orders_and_distributors, :orders_and_fulfillment, :products_and_inventory,
            :order_cycle_management, :xero_invoices, :enterprise_fee_summary, :bulk_coop], :report

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

@@ -0,0 +1,47 @@
+.download.hidden
+  = link_to t("admin.reports.download.button"), file_url, target: "_blank", class: "button icon icon-file"
+
+:javascript
+  (function () {
+    const tryDownload = function() {
+      const link = document.querySelector(".download a");
+
+      // If the report was already rendered via web sockets:
+      if (link == null) return;
+
+      fetch(link.href).then((response) => {
+        if (response.ok) {
+          response.blob().then((blob) => blob.text()).then((text) => {
+            const loading = document.querySelector(".loading");
+
+            if (loading == null) return;
+
+            loading.remove();
+            document.querySelector("#report-go button").disabled = false;
+
+            if (link.href.endsWith(".html")) {
+              // This replaces the hidden download button with the report:
+              link.parentElement.outerHTML = text;
+            } else {
+              // Or just show the download button when it's ready:
+              document.querySelector(".download").classList.remove("hidden")
+            }
+          });
+        } else {
+          setTimeout(tryDownload, 2000);
+        }
+      });
+    }
+
+    /*
+      A lot of reports are rendered within 250ms. Others take at least
+      2.5 seconds. There's a big gap in between. Observed on:
+      https://openfoodnetwork.org.au/admin/sidekiq/metrics/ReportJob?period=8h
+      https://openfoodnetwork.org.uk/admin/sidekiq/metrics/ReportJob?period=8h
+      https://coopcircuits.fr/admin/sidekiq/metrics/ReportJob?period=8h
+
+      But let's leave the timed response to websockets for now and just poll
+      as a backup mechanism.
+    */
+    setTimeout(tryDownload, 3000);
+  })();

app/views/admin/reports/create.turbo_stream.haml

@@ -0,0 +1,6 @@
+= turbo_stream.update "report-go" do
+  = button t(:go), "report__submit-btn", "submit", disabled: true
+= turbo_stream.update "report-table" do
+  = render "admin/reports/loading"
+  = render "admin/reports/fallback_display", file_url: @blob.expiring_service_url
+= turbo_stream.scroll_into_view("#report-table", behavior: "smooth")

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

@@ -3,7 +3,7 @@
 
 - content_for :minimal_js, true
 
-= form_for @report.search, { url: url_for, data: { remote: "true" } } do |f|
+= form_for @report.search, { url: url_for, data: { turbo: "true" } } do |f|
   = hidden_field_tag "uuid", request.uuid
 
   %fieldset.no-border-bottom.print-hidden

app/webpacker/js/turbo.js

@@ -1,5 +1,8 @@
 import "@hotwired/turbo";
 
+import TurboPower from "turbo_power";
+TurboPower.initialize(Turbo.StreamActions);
+
 document.addEventListener("turbo:frame-missing", (event) => {
   // don't replace frame contents
   event.preventDefault();

config/routes/admin.rb

@@ -135,6 +135,7 @@
     end
 
     get '/reports', to: 'reports#index', as: :reports
-    match '/reports/:report_type(/:report_subtype)', to: 'reports#show', via: [:get, :post], as: :report
+    match '/reports/:report_type(/:report_subtype)', to: 'reports#show', via: :get, as: :report
+    match '/reports/:report_type(/:report_subtype)', to: 'reports#create', via: :post
   end
 end

package.json

@@ -34,6 +34,7 @@
     "stimulus_reflex": "3.5.1",
     "tom-select": "^2.3.1",
     "trix": "^2.1.5",
+    "turbo_power": "^0.6.2",
     "webpack": "~4"
   },
   "devDependencies": {

spec/controllers/admin/reports_controller_spec.rb

@@ -310,14 +310,15 @@
     end
 
     it "triggers the delivery report" do
-      spree_post :show, {
+      spree_post :create, {
+        format: :turbo,
         q: { completed_at_lt: 1.day.ago },
         shipping_method_in: ["123"], # We just need to search for shipping methods
         report_type: :order_cycle_management,
         report_subtype: "delivery",
       }
 
-      expect(response).to have_http_status(:no_content)
+      expect(response).to have_http_status(:ok)
     end
   end
 

spec/jobs/report_job_spec.rb

@@ -6,13 +6,14 @@
   include CableReady::Broadcaster
 
   let(:report_args) {
-    { report_class:, user:, params:, format:, filename: }
+    { report_class:, user:, params:, format:, blob: }
   }
   let(:report_class) { Reporting::Reports::UsersAndEnterprises::Base }
   let(:user) { enterprise.owner }
   let(:enterprise) { create(:enterprise) }
   let(:params) { {} }
   let(:format) { :csv }
+  let(:blob) { ReportBlob.create_for_upload_later!(filename) }
   let(:filename) { "report.csv" }
 
   it "generates a report" do
@@ -25,11 +26,12 @@
   it "enqueues a job for async processing" do
     expect {
       ReportJob.perform_later(**report_args)
-    }.not_to change { ActiveStorage::Blob.count }
+    }.not_to change { blob.checksum }
 
     expect {
       perform_enqueued_jobs(only: ReportJob)
-    }.to change { ActiveStorage::Blob.count }
+      blob.reload
+    }.to change { blob.checksum }
 
     expect_csv_report
   end
@@ -44,7 +46,8 @@
 
     expect {
       perform_enqueued_jobs(only: ReportJob)
-    }.to change { ActiveStorage::Blob.count }
+      blob.reload
+    }.to change { blob.checksum }
   end
 
   it "triggers an email when the report is done" do

spec/mailers/report_mailer_spec.rb

@@ -10,7 +10,7 @@
         blob:,
       ).report_ready
     }
-    let(:blob) { ReportBlob.create!("customers.csv", "report content") }
+    let(:blob) { ReportBlob.create_locally!("customers.csv", "report content") }
 
     it "notifies about a report" do
       expect(email.subject).to eq "Report ready"

spec/models/report_blob_spec.rb

@@ -7,8 +7,16 @@
     content = "This works. ✓"
 
     expect do
-      blob = ReportBlob.create!("customers.html", content)
+      blob = ReportBlob.create_locally!("customers.html", content)
       content = blob.result
     end.not_to change { content.encoding }.from(Encoding::UTF_8)
   end
+
+  it "can be created first and filled later" do
+    blob = ReportBlob.create_for_upload_later!("customers.html")
+
+    expect { blob.store("Hello") }
+      .to change { blob.checksum }.from("0")
+      .and change { blob.result }.from(nil).to("Hello")
+  end
 end

spec/system/admin/reports/enterprise_fee_summaries_spec.rb

@@ -93,8 +93,7 @@
 
         it "generates file with data for all enterprises" do
           select "CSV"
-          click_on "Go"
-          perform_enqueued_jobs(only: ReportJob)
+          run_report
           click_on "Download Report"
           expect(downloaded_filename).to include ".csv"
           expect(downloaded_content).to have_content(distributor.name)
@@ -118,8 +117,7 @@
 
         it "generates file with data for the enterprise" do
           select "CSV"
-          click_on "Go"
-          perform_enqueued_jobs(only: ReportJob)
+          run_report
           click_on "Download Report"
 
           expect(downloaded_filename).to include ".csv"
@@ -154,8 +152,7 @@
 
         find("#report_format").click
         select "CSV"
-        click_on "Go"
-        perform_enqueued_jobs(only: ReportJob)
+        run_report
         click_on "Download Report"
 
         expect(downloaded_filename).to include ".csv"