From ff6bcb113fb77d2a95ffb48177b1fdf4ce6d6c12 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 12 Oct 2023 17:21:18 +1100
Subject: [PATCH 1/6] Create report file where the content is generated

In the past, we needed the report blob to know when the report has been
finished and uploaded. But not we use cable_ready to notify when the
report is done and we don't need the blob in the controller.
---
 app/controllers/admin/reports_controller.rb |  5 ++---
 app/jobs/report_job.rb                      |  3 ++-
 spec/jobs/report_job_spec.rb                | 22 ++++++++++-----------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index 9aff9f597f..ac6f1b73b4 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -80,11 +80,10 @@ module Admin
           block: "start"
         ).broadcast
 
-      blob = ReportBlob.create_for_upload_later!(report_filename)
-
       ReportJob.perform_later(
         report_class:, user: spree_current_user, params:,
-        format:, blob:, channel: ScopedChannel.for_id(params[:uuid]),
+        format:, filename: report_filename,
+        channel: ScopedChannel.for_id(params[:uuid]),
       )
 
       head :no_content
diff --git a/app/jobs/report_job.rb b/app/jobs/report_job.rb
index 9d1e081433..f2268f794d 100644
--- a/app/jobs/report_job.rb
+++ b/app/jobs/report_job.rb
@@ -9,11 +9,12 @@ class ReportJob < ApplicationJob
 
   NOTIFICATION_TIME = 5.seconds
 
-  def perform(report_class:, user:, params:, format:, blob:, channel: nil)
+  def perform(report_class:, user:, params:, format:, filename:, channel: nil)
     start_time = Time.zone.now
 
     report = report_class.new(user, params, render: true)
     result = report.render_as(format)
+    blob = ReportBlob.create_for_upload_later!(filename)
     blob.store(result)
 
     execution_time = Time.zone.now - start_time
diff --git a/spec/jobs/report_job_spec.rb b/spec/jobs/report_job_spec.rb
index c99f5fc38a..b37b19540a 100644
--- a/spec/jobs/report_job_spec.rb
+++ b/spec/jobs/report_job_spec.rb
@@ -4,15 +4,14 @@ require 'spec_helper'
 
 describe ReportJob do
   let(:report_args) {
-    { report_class:, user:, params:, format:,
-      blob: }
+    { report_class:, user:, params:, format:, filename: }
   }
   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!("report.csv") }
+  let(:filename) { "report.csv" }
 
   it "generates a report" do
     job = perform_enqueued_jobs(only: ReportJob) do
@@ -22,12 +21,14 @@ describe ReportJob do
   end
 
   it "enqueues a job for async processing" do
-    job = ReportJob.perform_later(**report_args)
-    expect(blob.content_stored?).to eq false
+    expect {
+      ReportJob.perform_later(**report_args)
+    }.to_not change { ActiveStorage::Blob.count }
 
-    perform_enqueued_jobs(only: ReportJob)
+    expect {
+      perform_enqueued_jobs(only: ReportJob)
+    }.to change { ActiveStorage::Blob.count }
 
-    expect(blob.content_stored?).to eq true
     expect_csv_report
   end
 
@@ -44,10 +45,9 @@ describe ReportJob do
       ReportJob.perform_later(**report_args)
       perform_enqueued_jobs(only: ReportJob)
     }.to enqueue_mail(ReportMailer, :report_ready).with(
-      params: {
+      params: hash_including(
         to: user.email,
-        blob:,
-      },
+      ),
       args: [],
     )
   end
@@ -76,7 +76,7 @@ describe ReportJob do
   end
 
   def expect_csv_report
-    blob.reload
+    blob = ReportBlob.last
     expect(blob.filename.to_s).to eq "report.csv"
     expect(blob.content_type).to eq "text/csv"
 

From 20af19c912d302711d47f421e7e89d7909a3e41e Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 12 Oct 2023 17:37:10 +1100
Subject: [PATCH 2/6] Simplify report file storage

---
 app/jobs/report_job.rb             |  3 +--
 app/models/report_blob.rb          | 25 +++++--------------------
 spec/mailers/report_mailer_spec.rb |  2 +-
 spec/models/report_blob_spec.rb    |  3 +--
 4 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/app/jobs/report_job.rb b/app/jobs/report_job.rb
index f2268f794d..c72cb98b2e 100644
--- a/app/jobs/report_job.rb
+++ b/app/jobs/report_job.rb
@@ -14,8 +14,7 @@ class ReportJob < ApplicationJob
 
     report = report_class.new(user, params, render: true)
     result = report.render_as(format)
-    blob = ReportBlob.create_for_upload_later!(filename)
-    blob.store(result)
+    blob = ReportBlob.create!(filename, result)
 
     execution_time = Time.zone.now - start_time
 
diff --git a/app/models/report_blob.rb b/app/models/report_blob.rb
index fb382fba31..a57bb31538 100644
--- a/app/models/report_blob.rb
+++ b/app/models/report_blob.rb
@@ -5,34 +5,19 @@ class ReportBlob < ActiveStorage::Blob
   # AWS S3 limits URL expiry to one week.
   LIFETIME = 1.week
 
-  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!(
+  def self.create!(filename, content)
+    create_and_upload!(
+      io: StringIO.new(content),
       filename:,
-      byte_size: 0,
-      checksum: "0",
       content_type: content_type(filename),
-    ).tap do |blob|
-      ActiveStorage::PurgeJob.set(wait: LIFETIME).perform_later(blob)
-    end
+      identify: false,
+    )
   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 content_stored?
-    @content_stored ||= reload.checksum != "0"
-  end
-
   def result
     @result ||= download.force_encoding(Encoding::UTF_8)
   end
diff --git a/spec/mailers/report_mailer_spec.rb b/spec/mailers/report_mailer_spec.rb
index 443617afd3..54a63845ea 100644
--- a/spec/mailers/report_mailer_spec.rb
+++ b/spec/mailers/report_mailer_spec.rb
@@ -10,7 +10,7 @@ describe ReportMailer do
         blob:,
       ).report_ready
     }
-    let(:blob) { ReportBlob.create_for_upload_later!("customers.csv") }
+    let(:blob) { ReportBlob.create!("customers.csv", "report content") }
 
     it "notifies about a report" do
       expect(email.subject).to eq "Report ready"
diff --git a/spec/models/report_blob_spec.rb b/spec/models/report_blob_spec.rb
index d16acc8fe0..1238ae1564 100644
--- a/spec/models/report_blob_spec.rb
+++ b/spec/models/report_blob_spec.rb
@@ -4,11 +4,10 @@ require 'spec_helper'
 
 describe ReportBlob, type: :model do
   it "preserves UTF-8 content" do
-    blob = ReportBlob.create_for_upload_later!("customers.html")
     content = "This works. ✓"
 
     expect do
-      blob.store(content)
+      blob = ReportBlob.create!("customers.html", content)
       content = blob.result
     end.to_not change { content.encoding }.from(Encoding::UTF_8)
   end

From eaff1ed9210f5b625a4eaeb0a86d2b85409ab409 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 12 Oct 2023 17:43:47 +1100
Subject: [PATCH 3/6] Store report files on the local disk

---
 app/models/report_blob.rb | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/app/models/report_blob.rb b/app/models/report_blob.rb
index a57bb31538..050bdbc5e8 100644
--- a/app/models/report_blob.rb
+++ b/app/models/report_blob.rb
@@ -11,6 +11,7 @@ class ReportBlob < ActiveStorage::Blob
       filename:,
       content_type: content_type(filename),
       identify: false,
+      service_name:,
     )
   end
 
@@ -18,6 +19,14 @@ class ReportBlob < ActiveStorage::Blob
     MIME::Types.of(filename).first&.to_s || "application/octet-stream"
   end
 
+  def self.service_name
+    if Rails.env.test?
+      :test
+    else
+      :local
+    end
+  end
+
   def result
     @result ||= download.force_encoding(Encoding::UTF_8)
   end

From a110ee0982b058cf9f7a9b5ee24a3c193a069b4f Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 13 Oct 2023 08:57:28 +1100
Subject: [PATCH 4/6] Simplify Active Storage config for test env

---
 app/models/report_blob.rb   | 10 +---------
 config/environments/test.rb |  2 --
 config/storage.yml          |  6 +-----
 3 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/app/models/report_blob.rb b/app/models/report_blob.rb
index 050bdbc5e8..b9d3927b68 100644
--- a/app/models/report_blob.rb
+++ b/app/models/report_blob.rb
@@ -11,7 +11,7 @@ class ReportBlob < ActiveStorage::Blob
       filename:,
       content_type: content_type(filename),
       identify: false,
-      service_name:,
+      service_name: :local,
     )
   end
 
@@ -19,14 +19,6 @@ class ReportBlob < ActiveStorage::Blob
     MIME::Types.of(filename).first&.to_s || "application/octet-stream"
   end
 
-  def self.service_name
-    if Rails.env.test?
-      :test
-    else
-      :local
-    end
-  end
-
   def result
     @result ||= download.force_encoding(Encoding::UTF_8)
   end
diff --git a/config/environments/test.rb b/config/environments/test.rb
index e51d8b19cd..e5cc306c18 100644
--- a/config/environments/test.rb
+++ b/config/environments/test.rb
@@ -56,6 +56,4 @@ Openfoodnetwork::Application.configure do
   config.active_support.deprecation = :stderr
 
   config.active_job.queue_adapter = :test
-
-  config.active_storage.service = :test
 end
diff --git a/config/storage.yml b/config/storage.yml
index 271782041f..8e0bb583a8 100644
--- a/config/storage.yml
+++ b/config/storage.yml
@@ -1,10 +1,6 @@
 local:
   service: Disk
-  root: <%= Rails.root.join("storage") %>
-
-test:
-  service: Disk
-  root: <%= Rails.root.join("tmp/storage") %>
+  root: <%= Rails.root.join(Rails.env.test? ? "tmp/storage" : "storage") %>
 
 test_amazon:
   service: S3

From 9f00817852b60cdef84db6ff886fe68af3cc7f59 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 20 Oct 2023 16:58:29 +1100
Subject: [PATCH 5/6] Use source of truth of url_options for report URLs

The ActionController options were not set in testing nor Sidekiq jobs.
The now used config is always set in config/application.rb.
---
 app/jobs/application_job.rb  |  2 +-
 spec/jobs/report_job_spec.rb | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb
index 6411c58c0b..5fd395e89f 100644
--- a/app/jobs/application_job.rb
+++ b/app/jobs/application_job.rb
@@ -12,6 +12,6 @@ class ApplicationJob < ActiveJob::Base
 
   def enable_active_storage_urls
     ActiveStorage::Current.url_options ||=
-      Rails.application.config.action_controller.default_url_options
+      Rails.application.routes.default_url_options
   end
 end
diff --git a/spec/jobs/report_job_spec.rb b/spec/jobs/report_job_spec.rb
index b37b19540a..14807c6578 100644
--- a/spec/jobs/report_job_spec.rb
+++ b/spec/jobs/report_job_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe ReportJob do
+  include CableReady::Broadcaster
+
   let(:report_args) {
     { report_class:, user:, params:, format:, filename: }
   }
@@ -32,6 +34,19 @@ describe ReportJob do
     expect_csv_report
   end
 
+  it "notifies Cable Ready when the report is done" do
+    channel = ScopedChannel.for_id("123")
+    with_channel = report_args.merge(channel:)
+
+    ReportJob.perform_later(**with_channel)
+
+    expect(cable_ready[channel]).to receive(:broadcast).and_call_original
+
+    expect {
+      perform_enqueued_jobs(only: ReportJob)
+    }.to change { ActiveStorage::Blob.count }
+  end
+
   it "triggers an email when the report is done" do
     # Setup test data which also triggers emails:
     report_args

From 3e6db7fda440e0ce18a3b5910691d0ee62a17b6e Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 24 Oct 2023 10:15:30 +1100
Subject: [PATCH 6/6] Update spec of changed report link

The link now contains the local test server instead of some fake domain.
---
 spec/system/admin/reports_spec.rb | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/spec/system/admin/reports_spec.rb b/spec/system/admin/reports_spec.rb
index 33a29028da..2547e59c2f 100644
--- a/spec/system/admin/reports_spec.rb
+++ b/spec/system/admin/reports_spec.rb
@@ -80,14 +80,13 @@ describe '
       email = ActionMailer::Base.deliveries.last
       expect(email.body).to have_link(
         "customers",
-        href: %r"^http://test\.host/rails/active_storage/disk/.*/customers_[0-9]+\.html$"
+        href: %r"^http://.*/rails/active_storage/disk/.*/customers_[0-9]+\.html$"
       )
 
       # ActiveStorage links usually expire after 5 minutes.
       # But we want a longer expiry in emailed links.
       parsed_email = Capybara::Node::Simple.new(email.body.to_s)
-      email_link_href = parsed_email.find(:link, "customers")[:href]
-      report_link = email_link_href.sub("test.host", Rails.application.default_url_options[:host])
+      report_link = parsed_email.find(:link, "customers")[:href]
       content = URI.parse(report_link).read
       expect(content).to match "<th>\nFirst Name\n</th>"