Merge pull request #14494 from openfoodfoundation/revert-14364-task/14157-prevent-admin-from-adding-multiple-images

chahmedejaz · web-flow · commit 7c69e90aff99 · 2026-07-03T14:12:09.000+05:00
Revert "Prevent admins to be able to view carousel in shops"
diff --git a/app/controllers/api/v0/product_images_controller.rb b/app/controllers/api/v0/product_images_controller.rb
@@ -9,17 +9,14 @@ def update_product_image
         product = Spree::Product.find(params[:product_id])
         authorize! :update, product
 
-        previous_image = product.image
-        image = Spree::Image.new(
+        image = product.image || Spree::Image.new(
           viewable_id: product.id,
           viewable_type: 'Spree::Product'
         )
-        image.attachment.attach(params[:file])
 
-        success_status = previous_image.present? ? :ok : :created
+        success_status = image.persisted? ? :ok : :created
 
-        if image.save
-          previous_image&.destroy!
+        if image.update(attachment: params[:file])
           render json: image, serializer: ImageSerializer, status: success_status
         else
           error_json = { errors: image.errors.full_messages }
diff --git a/app/controllers/spree/admin/images_controller.rb b/app/controllers/spree/admin/images_controller.rb
@@ -1,6 +1,5 @@
 # frozen_string_literal: true
 
-# rubocop:disable Metrics/ClassLength
 module Spree
   module Admin
     class ImagesController < ::Admin::ResourceController
@@ -44,20 +43,15 @@ def create
             format.turbo_stream { render :update }
           end
         else
-          respond_with_error((@error_target || @object).errors)
+          respond_with_error(@object.errors)
         end
       end
 
       def update
         @url_filters = ::ProductFilters.new.extract(request.query_parameters)
-        update_successful = if permitted_resource_params[:attachment].present?
-                              replace_image_without_destroy
-                            else
-                              set_viewable
-                              @object.update(permitted_resource_params)
-                            end
-
-        if update_successful
+        set_viewable
+
+        if @object.update(permitted_resource_params)
           flash[:success] = flash_message_for(@object, :successfully_updated)
 
           respond_to do |format|
@@ -120,33 +114,10 @@ def permitted_resource_params
       def respond_with_error(errors)
         @errors = errors.map(&:full_message)
         respond_to do |format|
-          format.html {
-            render action_name == 'create' ? :new : :edit, status: :unprocessable_entity
-          }
-          format.turbo_stream { render :edit, status: :unprocessable_entity }
-        end
-      end
-
-      def replace_image_without_destroy
-        previous_image = @object
-        replacement_image = Spree::Image.new(viewable: previous_image.viewable)
-
-        replacement_image.alt = previous_image.alt
-        replacement_image.position = previous_image.position
-        replacement_image.attributes = permitted_resource_params.except(:attachment, :viewable_id)
-        replacement_image.viewable_type = 'Spree::Product'
-        replacement_image.viewable_id = params[:image][:viewable_id]
-        replacement_image.attachment.attach(permitted_resource_params[:attachment])
-
-        unless replacement_image.save
-          @error_target = replacement_image
-          return false
+          format.html { respond_with(@object) }
+          format.turbo_stream { render :edit }
         end
-
-        previous_image.destroy!
-        @object = @image = replacement_image
       end
     end
   end
 end
-# rubocop:enable Metrics/ClassLength
diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb
@@ -4,19 +4,13 @@ module Spree
   class Image < Asset
     ACCEPTED_CONTENT_TYPES = %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z}
 
-    acts_as_paranoid
-
-    has_one_attached :attachment, service: image_service, dependent: false do |attachment|
+    has_one_attached :attachment, service: image_service do |attachment|
       attachment.variant :mini, resize_to_fill: [48, 48]
       attachment.variant :small, resize_to_fill: [227, 227]
       attachment.variant :product, resize_to_limit: [240, 240]
       attachment.variant :large, resize_to_limit: [600, 600]
     end
 
-    # Prevent ActiveStorage from purging attachments when a Spree::Image is soft-deleted.
-    attachment_reflection = reflect_on_association(:attachment_attachment)
-    attachment_reflection.options[:dependent] = nil
-
     validates :attachment,
               attached: true,
               processable_file: true,
diff --git a/app/views/spree/admin/images/edit.html.haml b/app/views/spree/admin/images/edit.html.haml
@@ -1,6 +1,6 @@
 = render partial: 'spree/admin/shared/product_sub_menu'
 = render partial: 'spree/admin/shared/product_tabs', locals: { current: 'Images' }
-= render partial: 'spree/shared/error_messages', locals: { target: @error_target || @image }
+= render partial: 'spree/shared/error_messages', locals: { target: @image }
 
 - content_for :page_actions do
   %li= button_link_to t('spree.back_to_images_list'), admin_product_images_url(@product, @url_filters), icon: 'icon-arrow-left'
diff --git a/app/views/spree/admin/images/edit.turbo_stream.haml b/app/views/spree/admin/images/edit.turbo_stream.haml
@@ -1,11 +1,10 @@
 = turbo_stream.update "edit_image_modal" do
   = render ModalComponent.new id: "#modal_edit_product_image", instant: true, close_button: false, modal_class: :fit do
     %h2= t(".title")
-    - error_target = @error_target || @image
 
     -# Display image in the same way it appears in the shopfront popup
-    - if error_target&.errors&.any?
-      - error_target.errors.full_messages.each do |error|
+    - if defined?(@errors) && !@errors.empty?
+      - @errors.each do |error|
         %p
           = error
     - else
diff --git a/app/views/spree/admin/images/index.html.haml b/app/views/spree/admin/images/index.html.haml
@@ -1,6 +1,9 @@
 = render partial: 'spree/admin/shared/product_sub_menu'
 = render partial: 'spree/admin/shared/product_tabs', locals: { current: 'Images'}
 
+- content_for :page_actions do
+  %li= link_to_with_icon('icon-plus', t('spree.new_image'), new_admin_product_image_url(@product, @url_filters), id: 'new_image_link', class: 'button')
+
 #images
 
 - unless @product.image.present?
diff --git a/db/migrate/20260602222924_add_deleted_at_to_assets.rb b/db/migrate/20260602222924_add_deleted_at_to_assets.rb
diff --git a/db/migrate/20260611223814_ensure_single_product_image.rb b/db/migrate/20260611223814_ensure_single_product_image.rb
diff --git a/db/schema.rb b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2026_06_11_223814) do
+ActiveRecord::Schema[7.2].define(version: 2026_06_01_032848) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_stat_statements"
   enable_extension "plpgsql"
@@ -500,8 +500,6 @@
     t.string "viewable_type", limit: 50
     t.string "type", limit: 75
     t.text "alt"
-    t.datetime "deleted_at"
-    t.index ["deleted_at"], name: "index_spree_assets_on_deleted_at"
     t.index ["viewable_id"], name: "index_assets_on_viewable_id"
     t.index ["viewable_type", "type"], name: "index_assets_on_viewable_type_and_type"
   end
diff --git a/spec/migrations/ensure_single_product_image_spec.rb b/spec/migrations/ensure_single_product_image_spec.rb
diff --git a/spec/requests/admin/images_spec.rb b/spec/requests/admin/images_spec.rb
@@ -73,28 +73,6 @@
     }
 
     it_behaves_like "updating images", 302
-
-    context "when attachment is not provided" do
-      let(:params) do
-        {
-          image: {
-            viewable_id: product.id,
-            alt: "Updated alt text",
-          }
-        }
-      end
-
-      it "updates image metadata in place" do
-        expect {
-          subject
-          product.reload
-        }.not_to change { Spree::Image.count }
-
-        expect(response).to have_http_status(:found)
-        expect(response).to redirect_to(spree.admin_product_images_path(product))
-        expect(product.image.alt).to eq("Updated alt text")
-      end
-    end
   end
 
   describe "PATCH /admin/products/:product_id/images/:id with turbo" do
diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb
@@ -514,9 +514,43 @@
     describe "image page" do
       let!(:product) { create(:simple_product, enterprise_id: supplier2.id) }
 
-      it "loading image page with no image" do
+      it "loading new product image page" do
         visit spree.admin_product_images_path(product)
         expect(page).to have_selector ".no-objects-found"
+
+        page.find('a#new_image_link').click
+        expect(page).to have_selector "#image_attachment"
+      end
+
+      it "loading new product image page including url filters" do
+        visit spree.admin_product_images_path(product, filter)
+
+        page.find('a#new_image_link').click
+
+        expected_cancel_link = Regexp.new(Regexp.escape(spree.admin_product_images_path(product,
+                                                                                        filter)))
+        expect(page).to have_link('Cancel', href: expected_cancel_link)
+      end
+
+      it "upload a new product image including url filters" do
+        visit spree.admin_product_images_path(product, filter)
+
+        page.find('a#new_image_link').click
+
+        attach_file('image_attachment', image_file_path)
+        click_button "Create"
+
+        uri = URI.parse(current_url)
+        expect("#{uri.path}?#{uri.query}").to eq spree.admin_product_images_path(product, filter)
+      end
+
+      it "loading image page including url filter" do
+        visit spree.admin_product_images_path(product, filter)
+
+        expected_new_image_link = Regexp.new(Regexp.escape(spree.new_admin_product_image_path(
+                                                             product, filter
+                                                           )))
+        expect(page).to have_link('New Image', href: expected_new_image_link)
       end
 
       it "loading edit product image page including url filter" do
@@ -565,9 +599,9 @@
                             alt: "position 1", attachment: image, position: 1)
 
         visit spree.admin_product_images_path(product)
-        page.find("a.icon-edit").click
+        page.find('a#new_image_link').click
         attach_file('image_attachment', unsupported_image_file_path)
-        click_button "Update"
+        click_button "Create"
 
         expect(page).to have_text "Attachment has an invalid content type"
         expect(page).to have_text "Attachment is not identified as a valid media file"
diff --git a/spec/system/admin/products_v3/update_spec.rb b/spec/system/admin/products_v3/update_spec.rb
@@ -792,7 +792,7 @@
         end
 
         expect(page).to have_content /Image has been successfully (updated|created)/
-        expect(product.reload.image.attachment.filename.to_s).to eq("500.jpg")
+        expect(product.image.reload.url(:large)).to match /500.jpg$/
 
         within row_containing_name("Apples") do
           expect_page_to_have_image('500.jpg')
