From 448308710a1f4dbfeb35aa9310ab3ddbe526c7d9 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 19 Jun 2024 14:28:37 +1000 Subject: [PATCH] Per review, distributor are now updated via variant When a product is deleted, it will delete associated variant and in turn will touch the affected distributors --- app/models/enterprise.rb | 8 ++--- app/models/spree/product.rb | 6 ---- app/models/spree/variant.rb | 16 ++++++++-- spec/models/enterprise_spec.rb | 52 +++++++++++++++++++++---------- spec/models/spree/product_spec.rb | 7 +---- spec/models/spree/variant_spec.rb | 14 +++++++++ 6 files changed, 69 insertions(+), 34 deletions(-) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index 3d95f20353..03de14c176 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -205,14 +205,14 @@ class Enterprise < ApplicationRecord select('DISTINCT enterprises.*') } - scope :distributing_products, lambda { |product_ids| + scope :distributing_variants, lambda { |variants_ids| exchanges = joins(" INNER JOIN exchanges - ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f') + ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = false) "). joins('INNER JOIN exchange_variants ON (exchange_variants.exchange_id = exchanges.id)'). joins('INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id)'). - where(spree_variants: { product_id: product_ids }).select('DISTINCT enterprises.id') + where(spree_variants: { id: variants_ids }).select('DISTINCT enterprises.id') where(id: exchanges) } @@ -598,7 +598,7 @@ class Enterprise < ApplicationRecord # Touch distributors without them touching their distributors. # We avoid an infinite loop and don't need to touch the whole distributor tree. def touch_distributors - Enterprise.distributing_products(supplied_products.select(:id)). + Enterprise.distributing_variants(supplied_variants.select(:id)). where.not(enterprises: { id: }). update_all(updated_at: Time.zone.now) end diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index a8ae210631..fa906cca83 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -240,8 +240,6 @@ module Spree def destruction transaction do - touch_distributors - ExchangeVariant. where(exchange_variants: { variant_id: variants.with_deleted. select(:id) }).destroy_all @@ -319,10 +317,6 @@ module Spree first_variant.supplier.touch if first_variant.supplier.present? end - def touch_distributors - Enterprise.distributing_products(id).each(&:touch) - end - def validate_image return if image.blank? || !image.changed? || image.valid? diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 45a4e993a2..00340f49d7 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -252,8 +252,16 @@ module Spree end def destruction - exchange_variants.reload.destroy_all - yield + transaction do + # Even tough Enterprise will touch associated variant distributors when touched, + # the variant will be removed from the exchange by the time it's triggered, + # so it won't be able to find the deleted variant's distributors. + # This why we do it here + touch_distributors + + exchange_variants.reload.destroy_all + yield + end end def ensure_unit_value @@ -270,5 +278,9 @@ module Spree def convert_variant_weight_to_decimal self.weight = weight.to_d end + + def touch_distributors + Enterprise.distributing_variants(id).each(&:touch) + end end end diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb index 971bea4b0c..f69c6f96d2 100644 --- a/spec/models/enterprise_spec.rb +++ b/spec/models/enterprise_spec.rb @@ -412,12 +412,33 @@ RSpec.describe Enterprise do end describe "callbacks" do - it "restores permalink to original value when it is changed and invalid" do - e1 = create(:enterprise, permalink: "taken") - e2 = create(:enterprise, permalink: "not_taken") - e2.permalink = "taken" - e2.save - expect(e2.reload.permalink).to eq "not_taken" + describe "restore_permalink" do + it "restores permalink to original value when it is changed and invalid" do + e1 = create(:enterprise, permalink: "taken") + e2 = create(:enterprise, permalink: "not_taken") + e2.permalink = "taken" + e2.save + expect(e2.reload.permalink).to eq "not_taken" + end + end + + describe "touch_distributors" do + it "touches supplied variant distributors" do + enterprise = create(:enterprise) + variant = create(:variant) + enterprise.supplied_variants << variant + + updated_at = 1.hour.ago + distributor1 = create(:distributor_enterprise, updated_at:) + distributor2 = create(:distributor_enterprise, updated_at:) + + create(:simple_order_cycle, distributors: [distributor1], variants: [variant]) + create(:simple_order_cycle, distributors: [distributor2], variants: [variant]) + + expect { enterprise.touch } + .to change { distributor1.reload.updated_at } + .and change { distributor2.reload.updated_at } + end end end @@ -595,23 +616,22 @@ RSpec.describe Enterprise do end end - describe "distributing_products" do + describe "distributing_variants" do let(:distributor) { create(:distributor_enterprise) } - let(:product) { create(:product) } + let(:variant) { create(:variant) } it "returns enterprises distributing via an order cycle" do - order_cycle = create(:simple_order_cycle, distributors: [distributor], - variants: [product.variants.first]) - expect(Enterprise.distributing_products(product.id)).to eq([distributor]) + order_cycle = create(:simple_order_cycle, distributors: [distributor], variants: [variant]) + expect(Enterprise.distributing_variants(variant.id)).to eq([distributor]) end it "does not return duplicate enterprises" do - another_product = create(:product) + another_variant = create(:variant) order_cycle = create(:simple_order_cycle, distributors: [distributor], - variants: [product.variants.first, - another_product.variants.first]) - expect(Enterprise.distributing_products([product.id, - another_product.id])).to eq([distributor]) + variants: [variant, another_variant]) + expect(Enterprise.distributing_variants( + [variant.id, another_variant.id] + )).to eq([distributor]) end end diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index bc6c66ff34..385588538d 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -278,19 +278,14 @@ module Spree describe "callbacks" do let(:product) { create(:simple_product) } - describe "touching affected enterprises when the product is deleted" do + describe "destroy product" do let(:product) { create(:simple_product, supplier_id: distributor.id) } let(:distributor) { create(:distributor_enterprise) } - let(:supplier) { create(:supplier_enterprise) } let!(:oc) { create(:simple_order_cycle, distributors: [distributor], variants: [product.variants.first]) } - it "touches all distributors" do - expect { product.destroy }.to change { distributor.reload.updated_at } - end - it "removes variants from order cycles" do expect { product.destroy }.to change { ExchangeVariant.count } end diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 4788cb68a3..ef1f22fcdf 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -843,6 +843,20 @@ RSpec.describe Spree::Variant do expect { variant.destroy }.to change { supplier.reload.updated_at } end + + it "touches distributors" do + variant = create(:variant) + updated_at = 1.hour.ago + distributor1 = create(:distributor_enterprise, updated_at:) + distributor2 = create(:distributor_enterprise, updated_at:) + + create(:simple_order_cycle, distributors: [distributor1], variants: [variant]) + create(:simple_order_cycle, distributors: [distributor2], variants: [variant]) + + expect { variant.destroy } + .to change { distributor1.reload.updated_at } + .and change { distributor2.reload.updated_at } + end end describe "#ensure_unit_value" do