From 898ab08baba377d6809f88a87f754e03cf65e9b5 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 27 Jun 2024 11:23:05 +1000 Subject: [PATCH] Add specs for invalid records It turns out that the duplicator still raises an exception in some cases. Now I think I see why the the controller was catching the exceptions. At least now we know which exceptions to catch. --- app/models/spree/variant.rb | 2 +- .../lib/spree/core/product_duplicator_spec.rb | 198 ++++++++++-------- .../system/admin/products_v3/products_spec.rb | 11 +- 3 files changed, 119 insertions(+), 92 deletions(-) diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 8d88d608a2..32f04ac651 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -65,7 +65,7 @@ module Spree validate :check_currency validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true validates :tax_category, presence: true, - if: proc { Spree::Config[:products_require_tax_category] } + if: proc { Spree::Config.products_require_tax_category } validates :unit_value, presence: true, if: ->(variant) { %w(weight volume).include?(variant.product&.variant_unit) diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 227523f1d0..71173561d8 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -3,96 +3,124 @@ require 'spec_helper' RSpec.describe Spree::Core::ProductDuplicator do - let(:product) do - double 'Product', - name: "foo", - product_properties: [property], - variants: [variant], - image:, - variant_unit: 'item' + describe "unit" do + let(:product) do + double 'Product', + name: "foo", + product_properties: [property], + variants: [variant], + image:, + variant_unit: 'item' + end + + let(:new_product) do + double 'New Product', + save: true + end + + let(:property) do + double 'Property' + end + + let(:new_property) do + double 'New Property' + end + + let(:variant) do + double 'Variant 1', + sku: "67890", + price: 19.50, + currency: "AUD", + images: [image_variant] + end + + let(:new_variant) do + double 'New Variant 1', + sku: "67890" + end + + let(:image) do + double 'Image', + attachment: double('Attachment') + end + + let(:new_image) do + double 'New Image' + end + + let(:image_variant) do + double 'Image Variant', + attachment: double('Attachment') + end + + let(:new_image_variant) do + double 'New Image Variant', + attachment: double('Attachment') + end + + before do + expect(product).to receive(:dup).and_return(new_product) + expect(variant).to receive(:dup).and_return(new_variant) + expect(image).to receive(:dup).and_return(new_image) + expect(image_variant).to receive(:dup).and_return(new_image_variant) + expect(property).to receive(:dup).and_return(new_property) + end + + it "can duplicate a product" do + duplicator = Spree::Core::ProductDuplicator.new(product) + 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]) + expect(new_product).to receive(:created_at=).with(nil) + expect(new_product).to receive(:price=).with(0) + expect(new_product).to receive(:unit_value=).with(nil) + expect(new_product).to receive(:updated_at=).with(nil) + expect(new_product).to receive(:deleted_at=).with(nil) + expect(new_product).to receive(:variants=).with([new_variant]) + expect(new_product).to receive(:image=).with(new_image) + + expect(new_variant).to receive(:sku=).with("") + expect(new_variant).to receive(:deleted_at=).with(nil) + expect(new_variant).to receive(:images=).with([new_image_variant]) + expect(new_variant).to receive(:price=).with(variant.price) + expect(new_variant).to receive(:currency=).with(variant.currency) + + expect(image).to receive(:attachment_blob) + expect(new_image).to receive_message_chain(:attachment, :attach) + + expect(image_variant).to receive(:attachment_blob) + expect(new_image_variant).to receive_message_chain(:attachment, :attach) + + expect(new_property).to receive(:created_at=).with(nil) + expect(new_property).to receive(:updated_at=).with(nil) + + duplicator.duplicate + end end - let(:new_product) do - double 'New Product', - save: true - end + describe "errors" do + context "with invalid product" do + let(:product) { + # name is a required field + create(:product).tap{ |p| p.update_columns(variant_unit: nil) } + } + subject { Spree::Core::ProductDuplicator.new(product).duplicate } - let(:property) do - double 'Property' - end + it "returns an invalid record" + end - let(:new_property) do - double 'New Property' - end + context "invalid variant" do + let(:variant) { + # tax_category is required when products_require_tax_category + create(:variant).tap{ |v| v.update_columns(tax_category_id: nil) } + } + subject { Spree::Core::ProductDuplicator.new(variant.product).duplicate } - let(:variant) do - double 'Variant 1', - sku: "67890", - price: 19.50, - currency: "AUD", - images: [image_variant] - end + before { allow(Spree::Config).to receive(:products_require_tax_category).and_return(true) } - let(:new_variant) do - double 'New Variant 1', - sku: "67890" - end - - let(:image) do - double 'Image', - attachment: double('Attachment') - end - - let(:new_image) do - double 'New Image' - end - - let(:image_variant) do - double 'Image Variant', - attachment: double('Attachment') - end - - let(:new_image_variant) do - double 'New Image Variant', - attachment: double('Attachment') - end - - before do - expect(product).to receive(:dup).and_return(new_product) - expect(variant).to receive(:dup).and_return(new_variant) - expect(image).to receive(:dup).and_return(new_image) - expect(image_variant).to receive(:dup).and_return(new_image_variant) - expect(property).to receive(:dup).and_return(new_property) - end - - it "can duplicate a product" do - duplicator = Spree::Core::ProductDuplicator.new(product) - 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]) - expect(new_product).to receive(:created_at=).with(nil) - expect(new_product).to receive(:price=).with(0) - expect(new_product).to receive(:unit_value=).with(nil) - expect(new_product).to receive(:updated_at=).with(nil) - expect(new_product).to receive(:deleted_at=).with(nil) - expect(new_product).to receive(:variants=).with([new_variant]) - expect(new_product).to receive(:image=).with(new_image) - - expect(new_variant).to receive(:sku=).with("") - expect(new_variant).to receive(:deleted_at=).with(nil) - expect(new_variant).to receive(:images=).with([new_image_variant]) - expect(new_variant).to receive(:price=).with(variant.price) - expect(new_variant).to receive(:currency=).with(variant.currency) - - expect(image).to receive(:attachment_blob) - expect(new_image).to receive_message_chain(:attachment, :attach) - - expect(image_variant).to receive(:attachment_blob) - expect(new_image_variant).to receive_message_chain(:attachment, :attach) - - expect(new_property).to receive(:created_at=).with(nil) - expect(new_property).to receive(:updated_at=).with(nil) - - duplicator.duplicate + it "raises generic ActiveRecordError" do + expect{ subject }.to raise_error(ActiveRecord::ActiveRecordError) + end + end end end diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index c43cd6e7a2..c1fbc7dfcb 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -1210,18 +1210,17 @@ RSpec.describe 'As an enterprise user, I can manage my products', feature: :admi end end - it "fails to clone the product on page when clicked on the cloned option" do - # Mock the +save+ method to return fail. That's the only expected fail case - allow_any_instance_of(Spree::Product).to receive(:save).and_return(false) + it "shows error message when cloning invalid record" do + # Existing product is invalid: + product_a.update_columns(variant_unit: nil) click_product_clone "Apples" expect(page).to have_content "Unable to clone the product" + 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).not_to match /COPY OF Apples/ + expect(all_input_values).not_to match /COPY OF Apples/ end end end