From 5d828bd7ae5ee5513f219d8f7e669aaacea93f7d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 25 Jan 2019 14:19:48 +1100 Subject: [PATCH 1/6] Update soft-delete of products Spree changed their way of soft-deleting products, variants and some other models. `#destroy` is now soft-deleting and replaces `#delete`. This commit considers only products. Variants will follow in another commit. The other models can be ignored, because we don't call `delete` on them. --- .../spree/api/products_controller_decorator.rb | 2 +- app/models/spree/product_decorator.rb | 8 +++----- .../products_and_inventory_report_spec.rb | 4 ++-- spec/lib/open_food_network/products_cache_spec.rb | 4 ++-- spec/models/spree/product_spec.rb | 14 +++++++++----- spec/requests/shop_spec.rb | 2 +- 6 files changed, 18 insertions(+), 16 deletions(-) diff --git a/app/controllers/spree/api/products_controller_decorator.rb b/app/controllers/spree/api/products_controller_decorator.rb index 2101ca6f61..a58b4e47b2 100644 --- a/app/controllers/spree/api/products_controller_decorator.rb +++ b/app/controllers/spree/api/products_controller_decorator.rb @@ -33,7 +33,7 @@ Spree::Api::ProductsController.class_eval do authorize! :delete, Spree::Product @product = find_product(params[:product_id]) authorize! :delete, @product - @product.delete + @product.destroy respond_with(@product, :status => 204) end diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index aba22b5abb..8b6914d5bf 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -202,20 +202,18 @@ Spree::Product.class_eval do end end - def delete_with_delete_from_order_cycles + def destroy_with_delete_from_order_cycles transaction do OpenFoodNetwork::ProductsCache.product_deleted(self) do - # Touch supplier and distributors as we would on #destroy - self.supplier.touch touch_distributors ExchangeVariant.where('exchange_variants.variant_id IN (?)', self.variants_including_master.with_deleted).destroy_all - delete_without_delete_from_order_cycles + destroy_without_delete_from_order_cycles end end end - alias_method_chain :delete, :delete_from_order_cycles + alias_method_chain :destroy, :delete_from_order_cycles def refresh_products_cache diff --git a/spec/lib/open_food_network/products_and_inventory_report_spec.rb b/spec/lib/open_food_network/products_and_inventory_report_spec.rb index 9903b2ebed..1235c9e001 100644 --- a/spec/lib/open_food_network/products_and_inventory_report_spec.rb +++ b/spec/lib/open_food_network/products_and_inventory_report_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' module OpenFoodNetwork - xdescribe ProductsAndInventoryReport do + describe ProductsAndInventoryReport do context "As a site admin" do let(:user) do user = create(:user) @@ -105,7 +105,7 @@ module OpenFoodNetwork it "should filter deleted products" do product1 = create(:simple_product, supplier: supplier) product2 = create(:simple_product, supplier: supplier) - product2.delete + product2.destroy subject.filter(Spree::Variant.scoped).should match_array [product1.master, product1.variants.first] end describe "based on report type" do diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index a075d9ec6d..84c06ff2d4 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -98,7 +98,7 @@ module OpenFoodNetwork it "refreshes the cache based on exchanges the variant was in before destruction" do expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc) - product.delete + product.destroy end it "performs the cache refresh after the product has been removed from the order cycle" do @@ -106,7 +106,7 @@ module OpenFoodNetwork expect(product.reload.deleted_at).not_to be_nil end - product.delete + product.destroy end end diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 6f04f97a0c..2d4243eae2 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -172,7 +172,7 @@ module Spree it "refreshes the products cache on delete" do expect(OpenFoodNetwork::ProductsCache).to receive(:product_deleted).with(product) - product.delete + product.destroy end # On destroy, all distributed variants are refreshed by a Variant around_destroy @@ -185,11 +185,15 @@ module Spree let!(:oc) { create(:simple_order_cycle, distributors: [distributor], variants: [product.variants.first]) } it "touches the supplier" do - expect { product.delete }.to change { supplier.reload.updated_at } + expect { product.destroy }.to change { supplier.reload.updated_at } end it "touches all distributors" do - expect { product.delete }.to change { distributor.reload.updated_at } + 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 end @@ -701,13 +705,13 @@ module Spree it "removes the master variant from all order cycles" do e.variants << p.master - p.delete + p.destroy e.variants(true).should be_empty end it "removes all other variants from order cycles" do e.variants << v - p.delete + p.destroy e.variants(true).should be_empty end end diff --git a/spec/requests/shop_spec.rb b/spec/requests/shop_spec.rb index 1b15c08a7f..359549fa41 100644 --- a/spec/requests/shop_spec.rb +++ b/spec/requests/shop_spec.rb @@ -24,7 +24,7 @@ describe "Shop API", type: :request do set_order order v61.update_attribute(:count_on_hand, 1) - p6.delete + p6.destroy v71.update_attribute(:count_on_hand, 1) v41.update_attribute(:count_on_hand, 1) v42.update_attribute(:count_on_hand, 0) From 8f49f0006a24e4adb4512769c6732c0cacd500d4 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 25 Jan 2019 14:51:09 +1100 Subject: [PATCH 2/6] Update products controller decorator with Spree Spree changed the controller to deal with the updated soft-delete. This commit brings these changes to our decorator. --- .../spree/admin/products_controller_decorator.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/controllers/spree/admin/products_controller_decorator.rb b/app/controllers/spree/admin/products_controller_decorator.rb index c9d0f7c36d..c91a199b4a 100644 --- a/app/controllers/spree/admin/products_controller_decorator.rb +++ b/app/controllers/spree/admin/products_controller_decorator.rb @@ -78,8 +78,13 @@ Spree::Admin::ProductsController.class_eval do params[:q][:deleted_at_null] ||= "1" params[:q][:s] ||= "name asc" - - @search = Spree::Product.ransack(params[:q]) # this line is modified - hit Spree::Product instead of super, avoiding cancan error for fetching records with block permissions via accessible_by + # The next line is modified. + # Hit Spree::Product instead of super, avoiding cancan error for fetching + # records with block permissions via accessible_by. + @collection = Spree::Product + @collection = @collection.with_deleted if params[:q].delete(:deleted_at_null).blank? + # @search needs to be defined as this is passed to search_form_for + @search = @collection.ransack(params[:q]) @collection = @search.result. managed_by(spree_current_user). # this line is added to the original spree code!!!!! group_by_products_id. From cdb49f88b0b7bb4bd509dba6a098c4dbf207a95b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 25 Jan 2019 15:57:52 +1100 Subject: [PATCH 3/6] Move Variant deletion into its own service This keeps the override of Spree's model leaner. More importantly, it prepares us for using `destroy` instead of `delete`. In the past, `Product#delete` soft-deleted the product, but didn't delete the variants. When we use `Product#destroy` to soft-delete the product, it will also call destroy on the variants. If the model doesn't allow the deletion of the last variant, it will fail. So when a product is deleted we want to allow the deletion of all variants. But the user should not be allowed to delete the last variant. That's why I'm moving the check to the controller level. Related commits: - https://github.com/openfoodfoundation/openfoodnetwork/commit/e6c7acdff356d02233d5f993cb93c2d1af824548 - https://github.com/openfoodfoundation/openfoodnetwork/commit/2b47c9145a9b18f346dd186ef4254dd062eb0648 - https://github.com/openfoodfoundation/openfoodnetwork/commit/b9f19d5777ed5034297dd3725c17bccffd39cf5d#diff-412c5af2ec1ba9f6643f6df5a673c1d4R105 --- .../admin/variants_controller_decorator.rb | 7 +++++-- .../spree/api/variants_controller_decorator.rb | 2 +- app/models/spree/variant_decorator.rb | 13 ++++--------- app/services/variant_deleter.rb | 17 +++++++++++++++++ .../spree/api/variants_controller_spec.rb | 10 ++++++++++ spec/models/spree/product_spec.rb | 9 --------- spec/models/spree/variant_spec.rb | 17 ----------------- 7 files changed, 37 insertions(+), 38 deletions(-) create mode 100644 app/services/variant_deleter.rb diff --git a/app/controllers/spree/admin/variants_controller_decorator.rb b/app/controllers/spree/admin/variants_controller_decorator.rb index 999ca0b22c..cabc7e5eec 100644 --- a/app/controllers/spree/admin/variants_controller_decorator.rb +++ b/app/controllers/spree/admin/variants_controller_decorator.rb @@ -10,8 +10,11 @@ Spree::Admin::VariantsController.class_eval do def destroy @variant = Spree::Variant.find(params[:id]) - @variant.delete # This line changed, as well as removal of following conditional - flash[:success] = I18n.t('notice_messages.variant_deleted') + if VariantDeleter.new.delete(@variant) # This line changed + flash[:success] = Spree.t('notice_messages.variant_deleted') + else + flash[:success] = Spree.t('notice_messages.variant_not_deleted') + end respond_with(@variant) do |format| format.html { redirect_to admin_product_variants_url(params[:product_id]) } diff --git a/app/controllers/spree/api/variants_controller_decorator.rb b/app/controllers/spree/api/variants_controller_decorator.rb index 3bc8b1c511..0c5a1dd2a6 100644 --- a/app/controllers/spree/api/variants_controller_decorator.rb +++ b/app/controllers/spree/api/variants_controller_decorator.rb @@ -3,7 +3,7 @@ Spree::Api::VariantsController.class_eval do @variant = scope.find(params[:variant_id]) authorize! :delete, @variant - @variant.delete + VariantDeleter.new.delete(@variant) respond_with @variant, status: 204 end end diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index de8f151c2c..3c2076820c 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -106,15 +106,10 @@ Spree::Variant.class_eval do end def delete - if product.variants == [self] # Only variant left on product - errors.add :product, I18n.t(:spree_variant_product_error) - false - else - transaction do - self.update_column(:deleted_at, Time.zone.now) - ExchangeVariant.where(variant_id: self).destroy_all - self - end + transaction do + self.update_column(:deleted_at, Time.zone.now) + ExchangeVariant.where(variant_id: self).destroy_all + self end end diff --git a/app/services/variant_deleter.rb b/app/services/variant_deleter.rb new file mode 100644 index 0000000000..e76bb8d91a --- /dev/null +++ b/app/services/variant_deleter.rb @@ -0,0 +1,17 @@ +# Checks the validity of a soft-delete call. +class VariantDeleter + def delete(variant) + if only_variant_on_product?(variant) + variant.errors.add :product, I18n.t(:spree_variant_product_error) + return false + end + + variant.delete + end + + private + + def only_variant_on_product?(variant) + variant.product.variants == [variant] + end +end diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 698a3e7926..af50d783e3 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -69,6 +69,16 @@ module Spree lambda { variant.reload }.should_not raise_error variant.deleted_at.should_not be_nil end + + it "doesn't delete the only variant of the product" do + product = create(:product) + variant = product.variants.first + + spree_delete :soft_delete, {variant_id: variant.to_param, product_id: product.to_param, format: :json} + + expect(variant.reload).to_not be_deleted + expect(assigns(:variant).errors[:product]).to include "must have at least one variant" + end end end end diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 2d4243eae2..a060e34a09 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -55,15 +55,6 @@ module Spree end end - - it "does not allow the last variant to be deleted" do - product = create(:simple_product) - expect(product.variants(:reload).length).to eq 1 - v = product.variants.last - v.delete - expect(v.errors[:product]).to include "must have at least one variant" - end - context "when the product has variants" do let(:product) do product = create(:simple_product) diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index d48d16d636..5894b4e12d 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -535,22 +535,5 @@ module Spree v.destroy e.reload.variant_ids.should be_empty end - - context "as the last variant of a product" do - let!(:extra_variant) { create(:variant) } - let!(:product) { extra_variant.product } - let!(:first_variant) { product.variants.first } - - before { product.reload } - - it "cannot be deleted" do - expect(product.variants.length).to eq 2 - expect(extra_variant.delete).to eq extra_variant - expect(product.variants(:reload).length).to eq 1 - expect(first_variant.delete).to be false - expect(product.variants(:reload).length).to eq 1 - expect(first_variant.errors[:product]).to include "must have at least one variant" - end - end end end From b4d1e48693227cc22241d3f1fa7360626cce0dec Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 25 Jan 2019 17:17:52 +1100 Subject: [PATCH 4/6] Update soft-delete of variants Spree changed their way of soft-deleting variants from calling `delete` to calling `destroy`. We don't need our own implementation any more. --- app/models/spree/variant_decorator.rb | 8 -------- app/services/variant_deleter.rb | 2 +- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 3c2076820c..5f67825537 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -105,14 +105,6 @@ Spree::Variant.class_eval do OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle).fees_by_type_for self end - def delete - transaction do - self.update_column(:deleted_at, Time.zone.now) - ExchangeVariant.where(variant_id: self).destroy_all - self - end - end - def refresh_products_cache if is_master? product.refresh_products_cache diff --git a/app/services/variant_deleter.rb b/app/services/variant_deleter.rb index e76bb8d91a..f499f0c24d 100644 --- a/app/services/variant_deleter.rb +++ b/app/services/variant_deleter.rb @@ -6,7 +6,7 @@ class VariantDeleter return false end - variant.delete + variant.destroy end private From 99f0be2f1cced65bc2124d02303a8e3e0870bb0d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 25 Jan 2019 17:46:05 +1100 Subject: [PATCH 5/6] Remove unnecessary scope Variant.not_deleted Spree made that scope default so that we don't need to define or call it. There might be cases in which we were showing deleted variants and now we are not, but I have not idea how to find them. Related Spree commit: - https://github.com/openfoodfoundation/spree/commit/cd3add960ef04f5881b508a07a503b3f7cade883 --- .../spree/api/products_controller_decorator.rb | 4 ++-- app/models/order_cycle.rb | 4 +--- app/models/product_import/entry_processor.rb | 1 - app/models/spree/variant_decorator.rb | 1 - .../api/admin/for_order_cycle/enterprise_serializer.rb | 4 ++-- .../products_and_inventory_report_base.rb | 6 +----- spec/models/spree/variant_spec.rb | 10 ---------- 7 files changed, 6 insertions(+), 24 deletions(-) diff --git a/app/controllers/spree/api/products_controller_decorator.rb b/app/controllers/spree/api/products_controller_decorator.rb index a58b4e47b2..3fb8a0806c 100644 --- a/app/controllers/spree/api/products_controller_decorator.rb +++ b/app/controllers/spree/api/products_controller_decorator.rb @@ -56,8 +56,8 @@ Spree::Api::ProductsController.class_eval do def product_scope if current_api_user.has_spree_role?("admin") || current_api_user.enterprises.present? # This line modified scope = Spree::Product - unless params[:show_deleted] - scope = scope.not_deleted + if params[:show_deleted] + scope = scope.with_deleted end else scope = Spree::Product.active diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index ac9b7d8cb8..0fc159fb8d 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -146,7 +146,6 @@ class OrderCycle < ActiveRecord::Base Spree::Variant. joins(:exchanges). merge(Exchange.in_order_cycle(self)). - not_deleted. select('DISTINCT spree_variants.*'). to_a # http://stackoverflow.com/q/15110166 end @@ -163,9 +162,8 @@ class OrderCycle < ActiveRecord::Base def variants_distributed_by(distributor) return Spree::Variant.where("1=0") unless distributor.present? Spree::Variant. - not_deleted. - merge(distributor.inventory_variants). joins(:exchanges). + merge(distributor.inventory_variants). merge(Exchange.in_order_cycle(self)). merge(Exchange.outgoing). merge(Exchange.to_enterprise(distributor)) diff --git a/app/models/product_import/entry_processor.rb b/app/models/product_import/entry_processor.rb index bd9ef94d45..4e9b52cfad 100644 --- a/app/models/product_import/entry_processor.rb +++ b/app/models/product_import/entry_processor.rb @@ -49,7 +49,6 @@ module ProductImport VariantOverride.for_hubs([enterprise_id]).count else Spree::Variant. - not_deleted. not_master. joins(:product). where('spree_products.supplier_id IN (?)', enterprise_id). diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 5f67825537..30f695f70b 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -35,7 +35,6 @@ Spree::Variant.class_eval do scope :with_order_cycles_inner, joins(exchanges: :order_cycle) - scope :not_deleted, where(deleted_at: nil) scope :not_master, where(is_master: false) scope :in_order_cycle, lambda { |order_cycle| with_order_cycles_inner. diff --git a/app/serializers/api/admin/for_order_cycle/enterprise_serializer.rb b/app/serializers/api/admin/for_order_cycle/enterprise_serializer.rb index 8e4dee5f5d..0eb9b642ef 100644 --- a/app/serializers/api/admin/for_order_cycle/enterprise_serializer.rb +++ b/app/serializers/api/admin/for_order_cycle/enterprise_serializer.rb @@ -31,9 +31,9 @@ class Api::Admin::ForOrderCycle::EnterpriseSerializer < ActiveModel::Serializer def products return @products unless @products.nil? @products = if order_cycle.prefers_product_selection_from_coordinator_inventory_only? - object.supplied_products.not_deleted.visible_for(order_cycle.coordinator) + object.supplied_products.visible_for(order_cycle.coordinator) else - object.supplied_products.not_deleted + object.supplied_products end end diff --git a/lib/open_food_network/products_and_inventory_report_base.rb b/lib/open_food_network/products_and_inventory_report_base.rb index 15110047d9..04a587d215 100644 --- a/lib/open_food_network/products_and_inventory_report_base.rb +++ b/lib/open_food_network/products_and_inventory_report_base.rb @@ -31,11 +31,7 @@ module OpenFoodNetwork end def filter(variants) - filter_on_hand filter_to_distributor filter_to_order_cycle filter_to_supplier filter_not_deleted variants - end - - def filter_not_deleted(variants) - variants.not_deleted + filter_on_hand filter_to_distributor filter_to_order_cycle filter_to_supplier variants end # Using the `in_stock?` method allows overrides by distributors. diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 5894b4e12d..7be1becbbd 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -13,16 +13,6 @@ module Spree end describe "scopes" do - it "finds non-deleted variants" do - v_not_deleted = create(:variant) - v_deleted = create(:variant) - v_deleted.deleted_at = Time.zone.now - v_deleted.save - - Spree::Variant.not_deleted.should include v_not_deleted - Spree::Variant.not_deleted.should_not include v_deleted - end - describe "finding variants in a distributor" do let!(:d1) { create(:distributor_enterprise) } let!(:d2) { create(:distributor_enterprise) } From 8374c7dea40b7229306943f7f8d8206d3193f682 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 19 Feb 2019 16:48:34 +1100 Subject: [PATCH 6/6] Fix serializer spec for supplied products In a previous commit we removed the now obsolete scope Product.not_deleted. Calling Enterprise#supplied_products without any scope does not issue a new SQL query but returns the products that were present when the enterprise was loaded. The fixed spec creates more products after loading the enterprise. So the enterprise needs to be reloaded or the new products are not visible. --- .../admin/for_order_cycle/enterprise_serializer_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/serializers/admin/for_order_cycle/enterprise_serializer_spec.rb b/spec/serializers/admin/for_order_cycle/enterprise_serializer_spec.rb index 025a04f23c..03e580c6ed 100644 --- a/spec/serializers/admin/for_order_cycle/enterprise_serializer_spec.rb +++ b/spec/serializers/admin/for_order_cycle/enterprise_serializer_spec.rb @@ -1,3 +1,5 @@ +require "spec_helper" + describe Api::Admin::ForOrderCycle::EnterpriseSerializer do let(:coordinator) { create(:distributor_enterprise) } let(:order_cycle) { double(:order_cycle, coordinator: coordinator) } @@ -14,7 +16,7 @@ describe Api::Admin::ForOrderCycle::EnterpriseSerializer do let(:serialized_enterprise) do Api::Admin::ForOrderCycle::EnterpriseSerializer.new( - enterprise, + enterprise.reload, # load the products that were created after the enterprise order_cycle: order_cycle, spree_current_user: enterprise.owner ).to_json