From b11ce2cdfe0eeb2e569f4ec462c8593f539d8b7b Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 31 Mar 2026 15:59:38 +1100 Subject: [PATCH 1/4] Clone links when cloning product variants Only links within the same product are supported so far. --- lib/spree/core/product_duplicator.rb | 18 +++++++++++++- .../lib/spree/core/product_duplicator_spec.rb | 24 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb index 5000caf7cd..c36c934ea3 100644 --- a/lib/spree/core/product_duplicator.rb +++ b/lib/spree/core/product_duplicator.rb @@ -32,9 +32,12 @@ module Spree end def duplicate_variants - product.variants.map do |variant| + # Create a hash with mapping: { : , } + mapped_variants = product.variants.index_with do |variant| duplicate_variant(variant) end + duplicate_variant_links(mapped_variants) + mapped_variants.values end def duplicate_variant(variant) @@ -47,6 +50,19 @@ module Spree end end + def duplicate_variant_links(mapped_variants) + # Find any links between orig variants (links to/from another product are ignored) + variant_links = VariantLink.where(source_variant: [mapped_variants.keys], + target_variant: [mapped_variants.keys]) + # Link the new variants + variant_links.find_each do |variant_link| + VariantLink.create( + source_variant: mapped_variants[variant_link.source_variant], + target_variant: mapped_variants[variant_link.target_variant] + ) + end + end + def duplicate_image(image) new_image = image.dup new_image.attachment.attach(image.attachment_blob) diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 5f22636065..89bd582ffa 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -36,6 +36,7 @@ RSpec.describe Spree::Core::ProductDuplicator do it "can duplicate a product" do duplicator = Spree::Core::ProductDuplicator.new(product) + allow(duplicator).to receive(:duplicate_variant_links) # tested elsewhere expect(new_product).to receive(:name=).with("COPY OF foo") expect(new_product).to receive(:sku=).with("") expect(new_product).to receive(:product_properties=).with([new_property]) @@ -65,6 +66,29 @@ RSpec.describe Spree::Core::ProductDuplicator do end end + describe "duplicating" do + subject { described_class.new(product).duplicate } + context "with variant links" do + let!(:product) { create(:product) } + let!(:source_variant) { product.variants.first } + let!(:linked_variant1) { source_variant.create_linked_variant(source_variant.supplier.owner) } + let!(:linked_variant2) { source_variant.create_linked_variant(source_variant.supplier.owner) } + + it "duplicates variant links" do + expect(subject).to be_a Spree::Product + expect(subject.variants.count).to eq 3 + + # assuming they are cloned in the same order + new_source_variant = subject.variants[0] + new_linked_variant1 = subject.variants[1] + new_linked_variant2 = subject.variants[2] + expect(new_source_variant.target_variants).to eq [new_linked_variant1, new_linked_variant2] + expect(new_linked_variant1.source_variants).to eq [new_source_variant] + expect(new_linked_variant2.source_variants).to eq [new_source_variant] + end + end + end + describe "errors" do context "with invalid product" do # Name has a max length of 255 char, when cloning a product the cloned product has a name From 734af3838648aa6876289d8ad1e9de6f500b5d0c Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 1 Apr 2026 10:36:11 +1100 Subject: [PATCH 2/4] Add spec for db queries Oh there is quite a lot. --- .../lib/spree/core/product_duplicator_spec.rb | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 89bd582ffa..5623c8ab3f 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -86,6 +86,107 @@ RSpec.describe Spree::Core::ProductDuplicator do expect(new_linked_variant1.source_variants).to eq [new_source_variant] expect(new_linked_variant2.source_variants).to eq [new_source_variant] end + + it "minimises(?) database queries" do + expect { subject }.to query_database [ + "Spree::ProductProperty Load", + "Spree::Image Load", + "Spree::Variant Load", + "Spree::Image Load", + "Spree::Price Load", + "Spree::Image Load", + "Spree::Price Load", + "Spree::Image Load", + "Spree::Price Load", + "VariantLink Load", + "Spree::Variant Load", + "Spree::Variant Load", + "TRANSACTION", + "Spree::ShippingCategory Load", + "Spree::Product Load", + "Spree::Taxon Load", + "Enterprise Load", + "Spree::Variant Create", + "Spree::Price Create", + "Spree::StockItem Exists?", + "Spree::StockItem Exists?", + "Spree::StockItem Create", + "ActsAsTaggableOn::Tagging Load", + "Spree::Variant Update", + "Spree::ShippingCategory Load", + "Spree::Product Load", + "Spree::Taxon Load", + "Enterprise Load", + "Spree::Variant Create", + "Spree::Price Create", + "Spree::StockItem Exists?", + "Spree::StockItem Exists?", + "Spree::StockItem Create", + "ActsAsTaggableOn::Tagging Load", + "Spree::Variant Update", + "VariantLink Create", + "Spree::Product Update", + "Spree::Variant Exists?", + "Spree::Variant Load", + "Enterprise Load", + "Enterprise Update", + "Enterprise Update All", + "Spree::Taxon Update", + "Enterprise Update", + "Enterprise Update All", + "TRANSACTION", + "Spree::Variant Load", + "Spree::Variant Load", + "TRANSACTION", + "Spree::ShippingCategory Load", + "Spree::Product Load", + "Spree::Taxon Load", + "Enterprise Load", + "Spree::Variant Create", + "Spree::Price Create", + "Spree::StockItem Exists?", + "Spree::StockItem Exists?", + "Spree::StockItem Create", + "ActsAsTaggableOn::Tagging Load", + "Spree::Variant Update", + "VariantLink Create", + "Spree::Product Update", + "Spree::Variant Exists?", + "Spree::Variant Load", + "Enterprise Load", + "Enterprise Update", + "Enterprise Update All", + "Spree::Taxon Update", + "Enterprise Update", + "Enterprise Update All", + "TRANSACTION", + "TRANSACTION", + "Spree::Product Create", + "Spree::Variant Update", + "Spree::StockItem Load", + "Spree::StockItem Exists?", + "Spree::Product Load", + "Spree::Variant Update", + "Spree::StockItem Load", + "Spree::StockItem Exists?", + "Spree::Product Load", + "Spree::Variant Update", + "Spree::StockItem Load", + "Spree::StockItem Exists?", + "Spree::Product Load", + "Spree::Product Update", + "Spree::Variant Exists?", + "Spree::Variant Load", + "Enterprise Load", + "Enterprise Update", + "Enterprise Update All", + "Spree::Product Update", + "Enterprise Update", + "Enterprise Update All", + "Spree::Taxon Update", + "TRANSACTION" + ] + end end end From 05f0e715453114e7143d87de9b9fc6ef8aba9425 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 1 Apr 2026 10:39:18 +1100 Subject: [PATCH 3/4] Create variant links through Rails association This allows us to delay saving to the db, reducing overall queries. --- lib/spree/core/product_duplicator.rb | 8 +- .../lib/spree/core/product_duplicator_spec.rb | 91 +++++-------------- 2 files changed, 28 insertions(+), 71 deletions(-) diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb index c36c934ea3..d34b45aaeb 100644 --- a/lib/spree/core/product_duplicator.rb +++ b/lib/spree/core/product_duplicator.rb @@ -56,10 +56,10 @@ module Spree target_variant: [mapped_variants.keys]) # Link the new variants variant_links.find_each do |variant_link| - VariantLink.create( - source_variant: mapped_variants[variant_link.source_variant], - target_variant: mapped_variants[variant_link.target_variant] - ) + source_variant = mapped_variants[variant_link.source_variant] + target_variant = mapped_variants[variant_link.target_variant] + + target_variant.variant_links_as_target.new(source_variant:) end end diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 5623c8ab3f..ab952c17b5 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -101,85 +101,42 @@ RSpec.describe Spree::Core::ProductDuplicator do "VariantLink Load", "Spree::Variant Load", "Spree::Variant Load", - "TRANSACTION", - "Spree::ShippingCategory Load", - "Spree::Product Load", - "Spree::Taxon Load", - "Enterprise Load", - "Spree::Variant Create", - "Spree::Price Create", - "Spree::StockItem Exists?", - "Spree::StockItem Exists?", - "Spree::StockItem Create", - "ActsAsTaggableOn::Tagging Load", - "Spree::Variant Update", - "Spree::ShippingCategory Load", - "Spree::Product Load", - "Spree::Taxon Load", - "Enterprise Load", - "Spree::Variant Create", - "Spree::Price Create", - "Spree::StockItem Exists?", - "Spree::StockItem Exists?", - "Spree::StockItem Create", - "ActsAsTaggableOn::Tagging Load", - "Spree::Variant Update", - "VariantLink Create", - "Spree::Product Update", - "Spree::Variant Exists?", - "Spree::Variant Load", - "Enterprise Load", - "Enterprise Update", - "Enterprise Update All", - "Spree::Taxon Update", - "Enterprise Update", - "Enterprise Update All", - "TRANSACTION", "Spree::Variant Load", "Spree::Variant Load", "TRANSACTION", "Spree::ShippingCategory Load", - "Spree::Product Load", "Spree::Taxon Load", "Enterprise Load", - "Spree::Variant Create", - "Spree::Price Create", - "Spree::StockItem Exists?", - "Spree::StockItem Exists?", - "Spree::StockItem Create", - "ActsAsTaggableOn::Tagging Load", - "Spree::Variant Update", - "VariantLink Create", - "Spree::Product Update", - "Spree::Variant Exists?", - "Spree::Variant Load", + "Spree::ShippingCategory Load", + "Spree::Taxon Load", + "Enterprise Load", + "Spree::ShippingCategory Load", + "Spree::Taxon Load", "Enterprise Load", - "Enterprise Update", - "Enterprise Update All", - "Spree::Taxon Update", - "Enterprise Update", - "Enterprise Update All", - "TRANSACTION", - "TRANSACTION", "Spree::Product Create", - "Spree::Variant Update", - "Spree::StockItem Load", + "Spree::Variant Create", + "Spree::Price Create", "Spree::StockItem Exists?", - "Spree::Product Load", - "Spree::Variant Update", - "Spree::StockItem Load", "Spree::StockItem Exists?", - "Spree::Product Load", + "Spree::StockItem Create", + "ActsAsTaggableOn::Tagging Load", "Spree::Variant Update", - "Spree::StockItem Load", + "Spree::Variant Create", + "Spree::Price Create", + "VariantLink Create", "Spree::StockItem Exists?", - "Spree::Product Load", - "Spree::Product Update", - "Spree::Variant Exists?", - "Spree::Variant Load", - "Enterprise Load", - "Enterprise Update", - "Enterprise Update All", + "Spree::StockItem Exists?", + "Spree::StockItem Create", + "ActsAsTaggableOn::Tagging Load", + "Spree::Variant Update", + "Spree::Variant Create", + "Spree::Price Create", + "VariantLink Create", + "Spree::StockItem Exists?", + "Spree::StockItem Exists?", + "Spree::StockItem Create", + "ActsAsTaggableOn::Tagging Load", + "Spree::Variant Update", "Spree::Product Update", "Enterprise Update", "Enterprise Update All", From b9eef83753f27a8ec24ad8593e0a82bf0235639c Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 1 Apr 2026 16:56:53 +1100 Subject: [PATCH 4/4] Fix spec Find variants by name instead of assuming they'll be in the right order. --- .../lib/spree/core/product_duplicator_spec.rb | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index ab952c17b5..a7f5a4f7d7 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -68,23 +68,28 @@ RSpec.describe Spree::Core::ProductDuplicator do describe "duplicating" do subject { described_class.new(product).duplicate } - context "with variant links" do - let!(:product) { create(:product) } - let!(:source_variant) { product.variants.first } - let!(:linked_variant1) { source_variant.create_linked_variant(source_variant.supplier.owner) } - let!(:linked_variant2) { source_variant.create_linked_variant(source_variant.supplier.owner) } + + context "with multiple variant links" do + let(:product) { create(:product) } + + before do + src_variant = product.variants.first.tap { it.update! display_name: "SRC" } + user = src_variant.supplier.owner + src_variant.create_linked_variant(user).tap { it.update! display_name: "LNK1" } + src_variant.create_linked_variant(user).tap { it.update! display_name: "LNK2" } + end it "duplicates variant links" do expect(subject).to be_a Spree::Product expect(subject.variants.count).to eq 3 - # assuming they are cloned in the same order - new_source_variant = subject.variants[0] - new_linked_variant1 = subject.variants[1] - new_linked_variant2 = subject.variants[2] - expect(new_source_variant.target_variants).to eq [new_linked_variant1, new_linked_variant2] - expect(new_linked_variant1.source_variants).to eq [new_source_variant] - expect(new_linked_variant2.source_variants).to eq [new_source_variant] + new_src_variant = subject.variants.find { it.display_name == "SRC" } + new_lnk_variant1 = subject.variants.find { it.display_name == "LNK1" } + new_lnk_variant2 = subject.variants.find { it.display_name == "LNK2" } + + expect(new_src_variant.target_variants).to eq [new_lnk_variant1, new_lnk_variant2] + expect(new_lnk_variant1.source_variants).to eq [new_src_variant] + expect(new_lnk_variant2.source_variants).to eq [new_src_variant] end it "minimises(?) database queries" do