Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions app/controllers/api/v0/product_images_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
41 changes: 35 additions & 6 deletions app/controllers/spree/admin/images_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

# rubocop:disable Metrics/ClassLength
module Spree
module Admin
class ImagesController < ::Admin::ResourceController
Expand Down Expand Up @@ -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|
Expand Down Expand Up @@ -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
7 changes: 6 additions & 1 deletion app/models/spree/image.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming this refers to :

The attachment association's dependent behavior has also been overridden to prevent automatic file cleanup.

Could you add a comment to explain what it does and why it's needed ?

validates :attachment,
attached: true,
processable_file: true,
Expand Down
2 changes: 1 addition & 1 deletion app/views/spree/admin/images/edit.html.haml
Original file line number Diff line number Diff line change
@@ -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'
Expand Down
5 changes: 3 additions & 2 deletions app/views/spree/admin/images/edit.turbo_stream.haml
Original file line number Diff line number Diff line change
@@ -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
Expand Down
3 changes: 0 additions & 3 deletions app/views/spree/admin/images/index.html.haml
Original file line number Diff line number Diff line change
@@ -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?
Expand Down
33 changes: 33 additions & 0 deletions db/migrate/20260602222924_ensure_single_product_image.rb
Original file line number Diff line number Diff line change
@@ -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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to run after the other migration, and I am pretty sure currently it's the other way around, based on the migration's timestamp in the name. Could you fix the migration to make sure they run in the correct order ?

end
end

def down
raise ActiveRecord::IrreversibleMigration
end
end
8 changes: 8 additions & 0 deletions db/migrate/20260611223814_add_deleted_at_to_assets.rb
Original file line number Diff line number Diff line change
@@ -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
4 changes: 3 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
80 changes: 80 additions & 0 deletions spec/migrations/ensure_single_product_image_spec.rb
Original file line number Diff line number Diff line change
@@ -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") }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a helper for this:

Suggested change
let(:attachment) { Rack::Test::UploadedFile.new(Rails.root.join('app/webpacker/images/logo-white.png'), "image/png") }
include FileHelper
let(:attachment) { white_logo_file }


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
22 changes: 22 additions & 0 deletions spec/requests/admin/images_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 3 additions & 37 deletions spec/system/admin/products_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion spec/system/admin/products_v3/update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down