From e92f4a120a8b7bce9dd9a450b877d4564bde6c79 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 16 Jan 2024 17:02:50 +1100
Subject: [PATCH 1/7] Refactor: re-organise spec file

No code changes, just moving it around and renaming blocks. Best viewed with white-space ignored.
---
 .../system/admin/products_v3/products_spec.rb | 453 +++++++++---------
 1 file changed, 229 insertions(+), 224 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index f62be46a48..54843e764e 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -2,7 +2,7 @@
 
 require "system_helper"
 
-describe 'As an admin, I can see the new product page', feature: :admin_style_v3 do
+describe 'As an admin, I can manage products', feature: :admin_style_v3 do
   include WebHelper
   include AuthenticationHelper
   include FileHelper
@@ -162,36 +162,6 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
     end
   end
 
-  describe "Actions columns (edit)" do
-    let!(:variant_a1) {
-      create(:variant,
-             product: product_a,
-             display_name: "Medium box",
-             sku: "APL-01",
-             price: 5.25)
-    }
-    let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
-
-    before do
-      visit admin_products_url
-    end
-
-    it "shows an actions memu with an edit link when clicking on icon for product" do
-      within row_containing_name("Apples") do
-        page.find(".vertical-ellipsis-menu").click
-        expect(page).to have_link "Edit", href: spree.edit_admin_product_path(product_a)
-      end
-    end
-
-    it "shows an actions memu with an edit link when clicking on icon for variant" do
-      within row_containing_name("Medium box") do
-        page.find(".vertical-ellipsis-menu").click
-        expect(page).to have_link "Edit",
-                                  href: spree.edit_admin_product_variant_path(product_a, variant_a1)
-      end
-    end
-  end
-
   describe "updating" do
     let!(:variant_a1) {
       create(:variant, product: product_a, display_name: "Medium box", sku: "APL-01", price: 5.25,
@@ -354,112 +324,8 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
     end
   end
 
-  describe "Cloning Feature" do
-    let!(:variant_a1) {
-      create(:variant,
-             product: product_a,
-             display_name: "Medium box",
-             sku: "APL-01",
-             price: 5.25)
-    }
-    let(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
-
-    before do
-      visit admin_products_url
-    end
-
-    describe "Actions columns (clone)" do
-      it "shows an actions menu with a clone link when clicking on icon for product" do
-        within row_containing_name("Apples") do
-          page.find(".vertical-ellipsis-menu").click
-          expect(page).to have_link "Clone", href: spree.clone_admin_product_path(product_a)
-        end
-
-        within row_containing_name("Medium box") do
-          page.find(".vertical-ellipsis-menu").click
-          expect(page).to_not have_link "Clone", href: spree.clone_admin_product_path(product_a)
-        end
-      end
-    end
-
-    describe "Cloning product" do
-      it "shows the cloned product on page when clicked on the cloned option" do
-        within "table.products" do
-          # Gather input values, because page.content doesn't include them.
-          input_content = page.find_all('input[type=text]').map(&:value).join
-
-          # Products does not include the cloned product.
-          expect(input_content).to_not match /COPY OF Apples/
-        end
-
-        within row_containing_name("Apples") do
-          page.find(".vertical-ellipsis-menu").click
-          click_link('Clone')
-        end
-
-        expect(page).to have_content "Product cloned"
-
-        within "table.products" do
-          # Gather input values, because page.content doesn't include them.
-          input_content = page.find_all('input[type=text]').map(&:value).join
-
-          # Products include the cloned product.
-          expect(input_content).to match /COPY OF Apples/
-        end
-      end
-    end
-  end
-
-  describe "Deleting Feature" do
-    let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
-    let(:delete_option_selector) { "a[data-controller='modal-link'].delete" }
-    let(:product_selector) { row_containing_name("Apples") }
-    let(:variant_selector) { row_containing_name("Medium box") }
-    let(:default_variant_selector) { "tr:has(input[aria-label=Price][value='#{product_a.price}'])" }
-
-    before do
-      visit admin_products_url
-    end
-
-    describe "Actions columns (delete)" do
-      it "shows an actions menu with a delete link when clicking on icon for product. " \
-         "doesn't show delete link for the single variant" do
-        within product_selector do
-          page.find(".vertical-ellipsis-menu").click
-          expect(page).to have_css(delete_option_selector)
-        end
-        page.find("div#content").click # to close the vertical actions menu
-
-        # to select the default variant
-        within default_variant_selector do
-          page.find(".vertical-ellipsis-menu").click
-          expect(page).to_not have_css(delete_option_selector)
-        end
-      end
-
-      it "shows an actions menu with a delete link when clicking on icon for variant" \
-         "if have multiple" do
-        create(:variant,
-               product: product_a,
-               display_name: "Medium box",
-               sku: "APL-01",
-               price: 5.25)
-
-        # to select the default variant
-        within default_variant_selector do
-          page.find(".vertical-ellipsis-menu").click
-          expect(page).to have_css(delete_option_selector)
-        end
-        page.find("div#content").click # to close the vertical actions menu
-
-        within variant_selector do
-          page.find(".vertical-ellipsis-menu").click
-          expect(page).to have_css(delete_option_selector)
-        end
-      end
-    end
-
-    describe "Delete Action" do
+  describe "actions" do
+    describe "edit" do
       let!(:variant_a1) {
         create(:variant,
                product: product_a,
@@ -467,118 +333,257 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
                sku: "APL-01",
                price: 5.25)
       }
-      let(:modal_selector) { "div[data-modal-target=modal]" }
-      let(:dismiss_button_selector) { "button[data-action='click->flash#close']" }
+      let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
 
-      context "when 'keep product/variant' is selected" do
-        it 'should not delete the product/variant' do
-          # Keep Product
-          within product_selector do
-            page.find(".vertical-ellipsis-menu").click
-            page.find(delete_option_selector).click
-          end
-          keep_button_selector = "input[type=button][value='Keep product']"
-          within modal_selector do
-            page.find(keep_button_selector).click
-          end
+      before do
+        visit admin_products_url
+      end
 
-          expect(page).to_not have_selector(modal_selector)
-          expect(page).to have_selector(product_selector)
-
-          # Keep Variant
-          within variant_selector do
-            page.find(".vertical-ellipsis-menu").click
-            page.find(delete_option_selector).click
-          end
-          keep_button_selector = "input[type=button][value='Keep variant']"
-          within modal_selector do
-            page.find(keep_button_selector).click
-          end
-
-          expect(page).to_not have_selector(modal_selector)
-          expect(page).to have_selector(variant_selector)
+      it "shows an actions memu with an edit link when clicking on icon for product" do
+        within row_containing_name("Apples") do
+          page.find(".vertical-ellipsis-menu").click
+          expect(page).to have_link "Edit", href: spree.edit_admin_product_path(product_a)
         end
       end
 
-      context "when 'delete product/variant' is selected" do
-        let(:success_flash_message_selector) { "div.flash.success" }
-        let(:error_flash_message_selector) { "div.flash.error" }
-        it 'should successfully delete the product/variant' do
-          # Delete Variant
-          within variant_selector do
+      it "shows an actions memu with an edit link when clicking on icon for variant" do
+        within row_containing_name("Medium box") do
+          page.find(".vertical-ellipsis-menu").click
+          expect(page).to have_link "Edit",
+                                    href: spree.edit_admin_product_variant_path(product_a,
+                                                                                variant_a1)
+        end
+      end
+    end
+
+    describe "clone" do
+      let!(:variant_a1) {
+        create(:variant,
+               product: product_a,
+               display_name: "Medium box",
+               sku: "APL-01",
+               price: 5.25)
+      }
+      let(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
+
+      before do
+        visit admin_products_url
+      end
+
+      describe "Actions columns (clone)" do
+        it "shows an actions menu with a clone link when clicking on icon for product" do
+          within row_containing_name("Apples") do
             page.find(".vertical-ellipsis-menu").click
-            page.find(delete_option_selector).click
+            expect(page).to have_link "Clone", href: spree.clone_admin_product_path(product_a)
           end
 
-          delete_button_selector = "input[type=button][value='Delete variant']"
-          within modal_selector do
-            page.find(delete_button_selector).click
+          within row_containing_name("Medium box") do
+            page.find(".vertical-ellipsis-menu").click
+            expect(page).to_not have_link "Clone", href: spree.clone_admin_product_path(product_a)
+          end
+        end
+      end
+
+      describe "Cloning product" do
+        it "shows the cloned product on page when clicked on the cloned option" do
+          within "table.products" do
+            # Gather input values, because page.content doesn't include them.
+            input_content = page.find_all('input[type=text]').map(&:value).join
+
+            # Products does not include the cloned product.
+            expect(input_content).to_not match /COPY OF Apples/
           end
 
-          expect(page).to_not have_selector(modal_selector)
-          # Make sure the products loading spinner is hidden
-          wait_for_class('.spinner-overlay', 'hidden')
-          expect(page).to_not have_selector(variant_selector)
-          within success_flash_message_selector do
-            expect(page).to have_content("Successfully deleted the variant")
-            page.find(dismiss_button_selector).click
+          within row_containing_name("Apples") do
+            page.find(".vertical-ellipsis-menu").click
+            click_link('Clone')
           end
 
-          # Delete product
+          expect(page).to have_content "Product cloned"
+
+          within "table.products" do
+            # Gather input values, because page.content doesn't include them.
+            input_content = page.find_all('input[type=text]').map(&:value).join
+
+            # Products include the cloned product.
+            expect(input_content).to match /COPY OF Apples/
+          end
+        end
+      end
+    end
+
+    describe "delete" do
+      let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
+      let(:delete_option_selector) { "a[data-controller='modal-link'].delete" }
+      let(:product_selector) { row_containing_name("Apples") }
+      let(:variant_selector) { row_containing_name("Medium box") }
+      let(:default_variant_selector) {
+        "tr:has(input[aria-label=Price][value='#{product_a.price}'])"
+      }
+
+      before do
+        visit admin_products_url
+      end
+
+      describe "Actions columns (delete)" do
+        it "shows an actions menu with a delete link when clicking on icon for product. " \
+           "doesn't show delete link for the single variant" do
           within product_selector do
             page.find(".vertical-ellipsis-menu").click
-            page.find(delete_option_selector).click
+            expect(page).to have_css(delete_option_selector)
           end
-          delete_button_selector = "input[type=button][value='Delete product']"
-          within modal_selector do
-            page.find(delete_button_selector).click
-          end
-          expect(page).to_not have_selector(modal_selector)
-          # Make sure the products loading spinner is hidden
-          wait_for_class('.spinner-overlay', 'hidden')
-          expect(page).to_not have_selector(product_selector)
-          within success_flash_message_selector do
-            expect(page).to have_content("Successfully deleted the product")
+          page.find("div#content").click # to close the vertical actions menu
+
+          # to select the default variant
+          within default_variant_selector do
+            page.find(".vertical-ellipsis-menu").click
+            expect(page).to_not have_css(delete_option_selector)
           end
         end
 
-        it 'should be failed to delete the product/variant' do
-          allow_any_instance_of(Spree::Product).to receive(:destroy).and_return(false)
-          allow_any_instance_of(Spree::Variant).to receive(:destroy).and_return(false)
+        it "shows an actions menu with a delete link when clicking on icon for variant" \
+           "if have multiple" do
+          create(:variant,
+                 product: product_a,
+                 display_name: "Medium box",
+                 sku: "APL-01",
+                 price: 5.25)
+
+          # to select the default variant
+          within default_variant_selector do
+            page.find(".vertical-ellipsis-menu").click
+            expect(page).to have_css(delete_option_selector)
+          end
+          page.find("div#content").click # to close the vertical actions menu
 
-          # Delete Variant
           within variant_selector do
             page.find(".vertical-ellipsis-menu").click
-            page.find(delete_option_selector).click
+            expect(page).to have_css(delete_option_selector)
+          end
+        end
+      end
+
+      describe "Delete Action" do
+        let!(:variant_a1) {
+          create(:variant,
+                 product: product_a,
+                 display_name: "Medium box",
+                 sku: "APL-01",
+                 price: 5.25)
+        }
+        let(:modal_selector) { "div[data-modal-target=modal]" }
+        let(:dismiss_button_selector) { "button[data-action='click->flash#close']" }
+
+        context "when 'keep product/variant' is selected" do
+          it 'should not delete the product/variant' do
+            # Keep Product
+            within product_selector do
+              page.find(".vertical-ellipsis-menu").click
+              page.find(delete_option_selector).click
+            end
+            keep_button_selector = "input[type=button][value='Keep product']"
+            within modal_selector do
+              page.find(keep_button_selector).click
+            end
+
+            expect(page).to_not have_selector(modal_selector)
+            expect(page).to have_selector(product_selector)
+
+            # Keep Variant
+            within variant_selector do
+              page.find(".vertical-ellipsis-menu").click
+              page.find(delete_option_selector).click
+            end
+            keep_button_selector = "input[type=button][value='Keep variant']"
+            within modal_selector do
+              page.find(keep_button_selector).click
+            end
+
+            expect(page).to_not have_selector(modal_selector)
+            expect(page).to have_selector(variant_selector)
+          end
+        end
+
+        context "when 'delete product/variant' is selected" do
+          let(:success_flash_message_selector) { "div.flash.success" }
+          let(:error_flash_message_selector) { "div.flash.error" }
+          it 'should successfully delete the product/variant' do
+            # Delete Variant
+            within variant_selector do
+              page.find(".vertical-ellipsis-menu").click
+              page.find(delete_option_selector).click
+            end
+
+            delete_button_selector = "input[type=button][value='Delete variant']"
+            within modal_selector do
+              page.find(delete_button_selector).click
+            end
+
+            expect(page).to_not have_selector(modal_selector)
+            # Make sure the products loading spinner is hidden
+            wait_for_class('.spinner-overlay', 'hidden')
+            expect(page).to_not have_selector(variant_selector)
+            within success_flash_message_selector do
+              expect(page).to have_content("Successfully deleted the variant")
+              page.find(dismiss_button_selector).click
+            end
+
+            # Delete product
+            within product_selector do
+              page.find(".vertical-ellipsis-menu").click
+              page.find(delete_option_selector).click
+            end
+            delete_button_selector = "input[type=button][value='Delete product']"
+            within modal_selector do
+              page.find(delete_button_selector).click
+            end
+            expect(page).to_not have_selector(modal_selector)
+            # Make sure the products loading spinner is hidden
+            wait_for_class('.spinner-overlay', 'hidden')
+            expect(page).to_not have_selector(product_selector)
+            within success_flash_message_selector do
+              expect(page).to have_content("Successfully deleted the product")
+            end
           end
 
-          delete_button_selector = "input[type=button][value='Delete variant']"
-          within modal_selector do
-            page.find(delete_button_selector).click
-          end
+          it 'should be failed to delete the product/variant' do
+            allow_any_instance_of(Spree::Product).to receive(:destroy).and_return(false)
+            allow_any_instance_of(Spree::Variant).to receive(:destroy).and_return(false)
 
-          expect(page).to_not have_selector(modal_selector)
-          sleep(0.5) # delay for loading spinner to complete
-          expect(page).to have_selector(variant_selector)
-          within error_flash_message_selector do
-            expect(page).to have_content("Unable to delete the variant")
-            page.find(dismiss_button_selector).click
-          end
+            # Delete Variant
+            within variant_selector do
+              page.find(".vertical-ellipsis-menu").click
+              page.find(delete_option_selector).click
+            end
 
-          # Delete product
-          within product_selector do
-            page.find(".vertical-ellipsis-menu").click
-            page.find(delete_option_selector).click
-          end
-          delete_button_selector = "input[type=button][value='Delete product']"
-          within modal_selector do
-            page.find(delete_button_selector).click
-          end
-          expect(page).to_not have_selector(modal_selector)
-          sleep(0.5) # delay for loading spinner to complete
-          expect(page).to have_selector(product_selector)
-          within error_flash_message_selector do
-            expect(page).to have_content("Unable to delete the product")
+            delete_button_selector = "input[type=button][value='Delete variant']"
+            within modal_selector do
+              page.find(delete_button_selector).click
+            end
+
+            expect(page).to_not have_selector(modal_selector)
+            sleep(0.5) # delay for loading spinner to complete
+            expect(page).to have_selector(variant_selector)
+            within error_flash_message_selector do
+              expect(page).to have_content("Unable to delete the variant")
+              page.find(dismiss_button_selector).click
+            end
+
+            # Delete product
+            within product_selector do
+              page.find(".vertical-ellipsis-menu").click
+              page.find(delete_option_selector).click
+            end
+            delete_button_selector = "input[type=button][value='Delete product']"
+            within modal_selector do
+              page.find(delete_button_selector).click
+            end
+            expect(page).to_not have_selector(modal_selector)
+            sleep(0.5) # delay for loading spinner to complete
+            expect(page).to have_selector(product_selector)
+            within error_flash_message_selector do
+              expect(page).to have_content("Unable to delete the product")
+            end
           end
         end
       end

From 25f49547951a2ae58ec75bfe5e62b1c9f886914d Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 19 Jan 2024 10:08:08 +1100
Subject: [PATCH 2/7] Combine spec

---
 spec/system/admin/products_v3/products_spec.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 54843e764e..26e55eb8d1 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -339,14 +339,12 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do
         visit admin_products_url
       end
 
-      it "shows an actions memu with an edit link when clicking on icon for product" do
+      it "shows an actions menu with an edit link for product and variant" do
         within row_containing_name("Apples") do
           page.find(".vertical-ellipsis-menu").click
           expect(page).to have_link "Edit", href: spree.edit_admin_product_path(product_a)
         end
-      end
 
-      it "shows an actions memu with an edit link when clicking on icon for variant" do
         within row_containing_name("Medium box") do
           page.find(".vertical-ellipsis-menu").click
           expect(page).to have_link "Edit",

From 07bef860b218aefca03410f734dd5839527d2644 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 16 Jan 2024 17:28:48 +1100
Subject: [PATCH 3/7] Add spec for updating image

---
 .../system/admin/products_v3/products_spec.rb | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 26e55eb8d1..3a04e6c49e 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -324,6 +324,38 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do
     end
   end
 
+  describe "edit image" do
+    context "product has an image" do
+      let!(:product) { create(:product_with_image, name: "Apples") }
+
+      before do
+        visit admin_products_url
+      end
+
+      it "updates product image" do
+        within row_containing_name("Apples") do
+          click_on "Edit"
+        end
+
+        within ".reveal-modal" do
+          expect(page).to have_content "Edit product photo"
+          expect_page_to_have_image(product.image.url(:product))
+
+          # Upload a new image file
+          attach_file 'image[attachment]', Rails.public_path.join('500.jpg'), visible: false
+        end
+
+        expect(page).to have_content "Loading"
+        expect(page).to have_content "Image has been successfully updated"
+        expect(product.image.reload.url(:product)).to match /500.jpg$/
+
+        within row_containing_name("Apples") do
+          expect_page_to_have_image('500.jpg')
+        end
+      end
+    end
+  end
+
   describe "actions" do
     describe "edit" do
       let!(:variant_a1) {
@@ -636,4 +668,8 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do
       sleep(0.1) until page.has_css?(selector, class: class_name, visible: false)
     end
   end
+
+  def expect_page_to_have_image(url)
+    expect(page).to have_selector("img[src$='#{url}']")
+  end
 end

From f72154e40c0f8b6778130ac6fb4ec82be8c0dfc5 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 16 Jan 2024 16:10:19 +1100
Subject: [PATCH 4/7] Add image create form

Re-using the edit image form, because they're basically the same.
---
 app/reflexes/products_reflex.rb               |  5 +++-
 .../admin/products_v3/_edit_image.html.haml   |  4 +--
 app/views/admin/products_v3/_table.html.haml  | 10 ++-----
 .../system/admin/products_v3/products_spec.rb | 28 +++++++++++++------
 4 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index ec6c28f9fd..e6dd59e41d 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -77,10 +77,13 @@ class ProductsReflex < ApplicationReflex
   def edit_image
     id = current_id_from_element(element)
     product = product_finder(id).find_product
+    image = product.image
+
+    image = Spree::Image.new(viewable: product) if product.image.blank?
 
     morph "#modal-component",
           render(partial: "admin/products_v3/edit_image",
-                 locals: { product:, image: product.image, return_url: url })
+                 locals: { product:, image:, return_url: url })
   end
 
   private
diff --git a/app/views/admin/products_v3/_edit_image.html.haml b/app/views/admin/products_v3/_edit_image.html.haml
index 7af268d64d..288e3d35c3 100644
--- a/app/views/admin/products_v3/_edit_image.html.haml
+++ b/app/views/admin/products_v3/_edit_image.html.haml
@@ -1,10 +1,10 @@
 = render ModalComponent.new id: "#modal_edit_product_image_#{image.id}", instant: true, close_button: false do
   %h2= t(".title")
 
-  %p= image_tag image.variant(:product)
+  %p= image_tag image.persisted? ? image.variant(:product) : Spree::Image.default_image_url(:product)
 
   -# Submit to controller, because StimulusReflex doesn't support file uploads
-  = form_for [:admin, product, image], url: admin_product_image_path(product, image),
+  = form_for [:admin, product, image],
       html: { multipart: true }, data: { controller: "form" } do |f|
     %input{ type: :hidden, name: :return_url, value: return_url}
     = f.hidden_field :viewable_id, value: product.id
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index c508972722..faf517bd3e 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -49,13 +49,9 @@
         %tbody.relaxed{ 'data-record-id': product_form.object.id }
           %tr
             %td.with-image
-              - if product.image.present?
-                %a.image-field{ href: edit_admin_product_image_path(product, product.image), data: { controller: "modal", reflex: "click->products#edit_image", "current-id": product.id} }
-                  = image_tag product.image&.url(:mini), width: 40, height: 40
-                  .button.secondary.mini= t('admin.products_page.image.edit')
-              - else
-                .image-field
-                  = image_tag Spree::Image.default_image_url(:mini), width: 40, height: 40
+              %a.image-field{ href: admin_product_images_path(product), data: { controller: "modal", reflex: "click->products#edit_image", "current-id": product.id} }
+                = image_tag product.image&.url(:mini) || Spree::Image.default_image_url(:mini), width: 40, height: 40
+                .button.secondary.mini= t('admin.products_page.image.edit')
             %td.field.align-left.header
               = product_form.hidden_field :id
               = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name')
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 3a04e6c49e..642265abea 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -325,28 +325,24 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do
   end
 
   describe "edit image" do
-    context "product has an image" do
-      let!(:product) { create(:product_with_image, name: "Apples") }
-
-      before do
+    shared_examples "updating image" do
+      it "saves product image" do
         visit admin_products_url
-      end
 
-      it "updates product image" do
         within row_containing_name("Apples") do
           click_on "Edit"
         end
 
         within ".reveal-modal" do
           expect(page).to have_content "Edit product photo"
-          expect_page_to_have_image(product.image.url(:product))
+          expect_page_to_have_image(current_img_url)
 
           # Upload a new image file
           attach_file 'image[attachment]', Rails.public_path.join('500.jpg'), visible: false
+          # It uploads automatically
         end
 
-        expect(page).to have_content "Loading"
-        expect(page).to have_content "Image has been successfully updated"
+        expect(page).to have_content /Image has been successfully (updated|created)/
         expect(product.image.reload.url(:product)).to match /500.jpg$/
 
         within row_containing_name("Apples") do
@@ -354,6 +350,20 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do
         end
       end
     end
+
+    context "with existing image" do
+      let!(:product) { create(:product_with_image, name: "Apples") }
+      let(:current_img_url) { product.image.url(:product) }
+
+      include_examples "updating image"
+    end
+
+    context "with default image" do
+      let!(:product) { create(:product, name: "Apples") }
+      let(:current_img_url) { Spree::Image.default_image_url(:product) }
+
+      include_examples "updating image"
+    end
   end
 
   describe "actions" do

From c462ac919fc97db1de5732eebc7bd818f2d6d594 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 19 Jan 2024 10:26:54 +1100
Subject: [PATCH 5/7] Use image url helper

Without it, I think the correct S3 url isn't generated.
---
 app/views/admin/products_v3/_edit_image.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/admin/products_v3/_edit_image.html.haml b/app/views/admin/products_v3/_edit_image.html.haml
index 288e3d35c3..b4f45ace5d 100644
--- a/app/views/admin/products_v3/_edit_image.html.haml
+++ b/app/views/admin/products_v3/_edit_image.html.haml
@@ -1,7 +1,7 @@
 = render ModalComponent.new id: "#modal_edit_product_image_#{image.id}", instant: true, close_button: false do
   %h2= t(".title")
 
-  %p= image_tag image.persisted? ? image.variant(:product) : Spree::Image.default_image_url(:product)
+  %p= image_tag image.persisted? ? image.url(:product) : Spree::Image.default_image_url(:product)
 
   -# Submit to controller, because StimulusReflex doesn't support file uploads
   = form_for [:admin, product, image],

From a915182bf8a31f5d51fab874bc7cc6bfe160572a Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 23 Jan 2024 15:51:11 +1100
Subject: [PATCH 6/7] Add tests for image controller

---
 spec/requests/admin/images_spec.rb | 71 ++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 spec/requests/admin/images_spec.rb

diff --git a/spec/requests/admin/images_spec.rb b/spec/requests/admin/images_spec.rb
new file mode 100644
index 0000000000..1ab0f05fcf
--- /dev/null
+++ b/spec/requests/admin/images_spec.rb
@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe "/admin/products/:product_id/images", type: :request do
+  include AuthenticationHelper
+
+  let!(:product) { create(:product) }
+
+  before do
+    login_as_admin
+  end
+
+  shared_examples "updating images" do
+    let(:params) do
+      {
+        image: {
+          attachment: fixture_file_upload("logo.png", "image/png"),
+          viewable_id: product.id,
+        }
+      }
+    end
+
+    it "creates a new image and redirects" do
+      expect {
+        subject
+        product.reload
+      }.to change{ product.image&.attachment&.filename.to_s }
+
+      expect(response.status).to eq 302
+      expect(response.location).to end_with spree.admin_product_images_path(product)
+
+      expect(product.image.url(:product)).to end_with "logo.png"
+    end
+
+    context "with wrong type of file" do
+      let(:params) do
+        {
+          image: {
+            attachment: fixture_file_upload("sample_file_120_products.csv", "text/csv"),
+            viewable_id: product.id,
+          }
+        }
+      end
+
+      it "responds with an error" do
+        expect {
+          subject
+          product.reload
+        }.to_not change{ product.image&.attachment&.filename.to_s }
+
+        pending "error status code"
+        expect(response).to be_unprocessable
+        expect(response.body).to include "Attachment has an invalid content type"
+      end
+    end
+  end
+
+  describe "POST /admin/products/:product_id/images" do
+    subject { post(spree.admin_product_images_path(product), params:) }
+
+    it_behaves_like "updating images"
+  end
+
+  describe "PATCH /admin/products/:product_id/images/:id" do
+    let!(:product) { create(:product_with_image) }
+    subject { patch(spree.admin_product_image_path(product, product.image), params:) }
+
+    it_behaves_like "updating images"
+  end
+end

From 4634917597d29813da7e864fa3570672811f77e1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 23 Jan 2024 15:50:47 +1100
Subject: [PATCH 7/7] Treat image integrity error as an input error

And thus give the user a useful message to try again.

Hmm, if this is a good idea, we should do it everywhere we upload an image. Can we build that in somehow, or at least make a shared helper that accepts a block and catches the error?

I replicated this in dev a couple of times, I think with a text file labelled as an image file. Unfortunately, I can no longer replicate in dev or with a spec.
---
 app/controllers/spree/admin/images_controller.rb | 6 ++++++
 config/locales/en.yml                            | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/app/controllers/spree/admin/images_controller.rb b/app/controllers/spree/admin/images_controller.rb
index 44b26278f8..0e0890a229 100644
--- a/app/controllers/spree/admin/images_controller.rb
+++ b/app/controllers/spree/admin/images_controller.rb
@@ -36,6 +36,9 @@ module Spree
         else
           respond_with(@object)
         end
+      rescue ActiveStorage::IntegrityError
+        @object.errors.add :attachment, :integrity_error
+        respond_with(@object)
       end
 
       def update
@@ -48,6 +51,9 @@ module Spree
         else
           respond_with(@object)
         end
+      rescue ActiveStorage::IntegrityError
+        @object.errors.add :attachment, :integrity_error
+        respond_with(@object)
       end
 
       def destroy
diff --git a/config/locales/en.yml b/config/locales/en.yml
index d9d16c50f8..2c40d8bbfb 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -99,6 +99,10 @@ en:
       models:
         enterprise_fee:
           inherit_tax_requires_per_item_calculator: "Inheriting the tax categeory requires a per-item calculator."
+        spree/image:
+          attributes:
+            attachment:
+              integrity_error: "failed to load. Please check to ensure the file is not corrupt, and try again."
         spree/user:
           attributes:
             email: