diff --git a/app/controllers/api/v0/product_images_controller.rb b/app/controllers/api/v0/product_images_controller.rb index f2263e6ba2b..7ce6c8e78ea 100644 --- a/app/controllers/api/v0/product_images_controller.rb +++ b/app/controllers/api/v0/product_images_controller.rb @@ -9,14 +9,17 @@ def update_product_image product = Spree::Product.find(params[:product_id]) authorize! :update, product - image = product.image || Spree::Image.new( + previous_image = product.image + image = Spree::Image.new( viewable_id: product.id, viewable_type: 'Spree::Product' ) + image.attachment.attach(params[:file]) - success_status = image.persisted? ? :ok : :created + success_status = previous_image.present? ? :ok : :created - if image.update(attachment: params[:file]) + if image.save + previous_image&.destroy! 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 index 193b0135625..b6ebffb7536 100644 --- a/app/controllers/spree/admin/images_controller.rb +++ b/app/controllers/spree/admin/images_controller.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +# rubocop:disable Metrics/ClassLength module Spree module Admin class ImagesController < ::Admin::ResourceController @@ -43,15 +44,20 @@ def create format.turbo_stream { render :update } end else - respond_with_error(@object.errors) + respond_with_error((@error_target || @object).errors) end end def update @url_filters = ::ProductFilters.new.extract(request.query_parameters) - set_viewable - - if @object.update(permitted_resource_params) + update_successful = if permitted_resource_params[:attachment].present? + replace_image_without_destroy + else + set_viewable + @object.update(permitted_resource_params) + end + + if update_successful flash[:success] = flash_message_for(@object, :successfully_updated) respond_to do |format| @@ -114,10 +120,33 @@ def permitted_resource_params def respond_with_error(errors) @errors = errors.map(&:full_message) respond_to do |format| - format.html { respond_with(@object) } - format.turbo_stream { render :edit } + 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 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 index 431b1679b03..ce80aaf00e4 100644 --- a/app/models/spree/image.rb +++ b/app/models/spree/image.rb @@ -4,13 +4,18 @@ module Spree class Image < Asset ACCEPTED_CONTENT_TYPES = %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} - has_one_attached :attachment, service: image_service do |attachment| + acts_as_paranoid + + has_one_attached :attachment, service: image_service, dependent: false 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 + attachment_reflection = _reflections["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 index 40ff067b0a3..9d27bae6428 100644 --- 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: @image } += render partial: 'spree/shared/error_messages', locals: { target: @error_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 index b3ca87329b2..e73b9be32b6 100644 --- a/app/views/spree/admin/images/edit.turbo_stream.haml +++ b/app/views/spree/admin/images/edit.turbo_stream.haml @@ -1,10 +1,11 @@ = 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 defined?(@errors) && !@errors.empty? - - @errors.each do |error| + - if error_target&.errors&.any? + - error_target.errors.full_messages.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 index c7955c0255d..eb440c4f9e6 100644 --- a/app/views/spree/admin/images/index.html.haml +++ b/app/views/spree/admin/images/index.html.haml @@ -1,9 +1,6 @@ = 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_ensure_single_product_image.rb b/db/migrate/20260602222924_ensure_single_product_image.rb new file mode 100644 index 00000000000..74dbc266930 --- /dev/null +++ b/db/migrate/20260602222924_ensure_single_product_image.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class EnsureSingleProductImage < ActiveRecord::Migration[7.1] + class SpreeImage < ActiveRecord::Base + self.table_name = "spree_assets" + self.inheritance_column = :_type_disabled + end + + def up + product_ids = SpreeImage + .where(viewable_type: "Spree::Product") + .group(:viewable_id) + .having("COUNT(*) > 1") + .pluck(:viewable_id) + + product_ids.each do |product_id| + images = SpreeImage + .where( + viewable_type: "Spree::Product", + viewable_id: product_id + ) + .order(:id) + + image_to_keep = images.first + + images.where.not(id: image_to_keep.id).update_all(deleted_at: Time.current) + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20260611223814_add_deleted_at_to_assets.rb b/db/migrate/20260611223814_add_deleted_at_to_assets.rb new file mode 100644 index 00000000000..0e0657d0180 --- /dev/null +++ b/db/migrate/20260611223814_add_deleted_at_to_assets.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddDeletedAtToAssets < ActiveRecord::Migration[7.1] + def change + add_column :spree_assets, :deleted_at, :datetime + add_index :spree_assets, :deleted_at + end +end diff --git a/db/schema.rb b/db/schema.rb index e9600d14a4d..8be13a2e880 100644 --- 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_01_031443) do +ActiveRecord::Schema[7.1].define(version: 2026_06_11_223814) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "plpgsql" @@ -500,6 +500,8 @@ 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 new file mode 100644 index 00000000000..caf11aa670b --- /dev/null +++ b/spec/migrations/ensure_single_product_image_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require_relative '../../db/migrate/20260602222924_ensure_single_product_image' + +RSpec.describe EnsureSingleProductImage, type: :migration do + let(:migration) { described_class.new } + let(:attachment) { Rack::Test::UploadedFile.new(Rails.root.join('app/webpacker/images/logo-white.png'), "image/png") } + + describe '#up' do + let(:product) { create(:product) } + + it 'keeps the first image and soft deletes additional images for a product' do + first_image = Spree::Image.create(attachment:, + viewable_id: product.id, + viewable_type: 'Spree::Product') + + second_image = Spree::Image.create(attachment:, + viewable_id: product.id, + viewable_type: 'Spree::Product') + + expect do + migration.up + end.to change { + Spree::Image.where( + viewable_type: 'Spree::Product', viewable_id: product.id + ).count + }.from(2).to(1) + + remaining_ids = Spree::Image.where( + viewable_type: 'Spree::Product', + viewable_id: product.id + ).pluck(:id) + + expect(remaining_ids).to contain_exactly(first_image.id) + expect(remaining_ids).not_to include(second_image.id) + + # Verify second image is soft deleted, not hard deleted + soft_deleted_image = Spree::Image.unscoped.find(second_image.id) + expect(soft_deleted_image.deleted_at).not_to be_nil + end + + it 'does not remove an image when a product already has a single image' do + image = Spree::Image.create(attachment:, + viewable_id: product.id, + viewable_type: 'Spree::Product') + + expect do + migration.up + end.not_to change { + Spree::Image.where( + viewable_type: 'Spree::Product', viewable_id: product.id + ).count + } + + expect(Spree::Image.exists?(image.id)).to be(true) + end + + it 'does not remove assets for non-product viewables' do + variant = create(:variant) + first_variant_image = Spree::Image.create!(attachment:, + viewable_id: variant.id, + viewable_type: 'Spree::Variant') + second_variant_image = Spree::Image.create!(attachment:, + viewable_id: variant.id, + viewable_type: 'Spree::Variant') + + expect do + migration.up + end.not_to change { + Spree::Image.where( + viewable_type: 'Spree::Variant', viewable_id: variant.id + ).count + } + + # Verify both variant images are not deleted + expect(Spree::Image.unscoped.find(first_variant_image.id).deleted_at).to be_nil + expect(Spree::Image.unscoped.find(second_variant_image.id).deleted_at).to be_nil + end + end +end diff --git a/spec/requests/admin/images_spec.rb b/spec/requests/admin/images_spec.rb index 7eaedad45ea..d31a6aebce2 100644 --- a/spec/requests/admin/images_spec.rb +++ b/spec/requests/admin/images_spec.rb @@ -73,6 +73,28 @@ } 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 index 93a699892b8..9b9afbac9a4 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -514,43 +514,9 @@ describe "image page" do let!(:product) { create(:simple_product, supplier_id: supplier2.id) } - it "loading new product image page" do + it "loading image page with no image" 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 @@ -599,9 +565,9 @@ alt: "position 1", attachment: image, position: 1) visit spree.admin_product_images_path(product) - page.find('a#new_image_link').click + page.find("a.icon-edit").click attach_file('image_attachment', unsupported_image_file_path) - click_button "Create" + click_button "Update" 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 index 4ddeff1578c..7e403ad22da 100644 --- 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.image.reload.url(:large)).to match /500.jpg$/ + expect(product.reload.image.attachment.filename.to_s).to eq("500.jpg") within row_containing_name("Apples") do expect_page_to_have_image('500.jpg')