From 33d212d27447c7d59f499bbdb38d9f89df4a38a1 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 23 Jun 2023 12:12:23 +1000
Subject: [PATCH] Gracefully deal with missing S3 config

I could have split this into several commits:

* DRY direct linking to images.
* Check S3 config before direct linking.
* Just check if service is public instead of relying on name.

Developers may copy a staging or production database or experiment with
S3 storage. But when the S3 config is missing then calling `service`
raises an ArgumentError due to a missing name.

Now we only try to call `service` if the S3 config is present.
---
 app/models/application_record.rb | 10 ++++++++++
 app/models/enterprise.rb         |  3 +--
 app/models/spree/image.rb        |  3 +--
 spec/models/spree/image_spec.rb  |  9 +++++++--
 4 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/app/models/application_record.rb b/app/models/application_record.rb
index 4e726e63b6..7870e01a56 100644
--- a/app/models/application_record.rb
+++ b/app/models/application_record.rb
@@ -15,6 +15,16 @@ class ApplicationRecord < ActiveRecord::Base
     ENV["S3_BUCKET"].present? ? :amazon_public : :local
   end
 
+  # We might have a development environment without S3 but with a database
+  # dump pointing to S3 images. Accessing the service fails then.
+  def image_variant_url_for(variant)
+    if ENV["S3_BUCKET"].present? && variant.service.public?
+      variant.processed.url
+    else
+      url_for(variant)
+    end
+  end
+
   def url_for(*args)
     Rails.application.routes.url_helpers.url_for(*args)
   end
diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb
index d1c6c88911..f026207e66 100644
--- a/app/models/enterprise.rb
+++ b/app/models/enterprise.rb
@@ -459,9 +459,8 @@ class Enterprise < ApplicationRecord
 
   def image_url_for(image, name)
     return unless image.variable?
-    return image.variant(name).processed.url if image.attachment.service.name == :amazon_public
 
-    url_for(image.variant(name))
+    image_variant_url_for(image.variant(name))
   rescue ActiveStorage::Error => e
     Bugsnag.notify "Enterprise ##{id} #{image.try(:name)} error: #{e.message}"
     Rails.logger.error(e.message)
diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb
index 6215ce97b3..2ed624b595 100644
--- a/app/models/spree/image.rb
+++ b/app/models/spree/image.rb
@@ -31,9 +31,8 @@ module Spree
 
     def url(size)
       return self.class.default_image_url(size) unless attachment.attached?
-      return variant(size).processed.url if attachment.service.name == :amazon_public
 
-      url_for(variant(size))
+      image_variant_url_for(variant(size))
     rescue ActiveStorage::Error => e
       Bugsnag.notify "Product ##{viewable_id} Image ##{id} error: #{e.message}"
       Rails.logger.error(e.message)
diff --git a/spec/models/spree/image_spec.rb b/spec/models/spree/image_spec.rb
index d6ac3b5b0c..d4f710c43a 100644
--- a/spec/models/spree/image_spec.rb
+++ b/spec/models/spree/image_spec.rb
@@ -55,9 +55,14 @@ module Spree
 
       context "when using public images" do
         it "returns the direct URL for the processed image" do
+          allow(ENV).to receive(:[])
+          expect(ENV).to receive(:[]).with("S3_BUCKET").and_return("present")
+
+          variant = double(:variant)
           allow(subject).to receive_message_chain(:attachment, :attached?) { true }
-          expect(subject).to receive_message_chain(:attachment, :service, :name) { :amazon_public }
-          expect(subject).to receive_message_chain(:variant, :processed, :url) { "https://ofn-s3/123.png" }
+          expect(subject).to receive(:variant) { variant }
+          expect(variant).to receive_message_chain(:service, :public?) { true }
+          expect(variant).to receive_message_chain(:processed, :url) { "https://ofn-s3/123.png" }
 
           expect(subject.url(:small)).to eq "https://ofn-s3/123.png"
         end