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. 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/products_controller_decorator.rb b/app/controllers/spree/api/products_controller_decorator.rb index 2101ca6f61..3fb8a0806c 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 @@ -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/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/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 5f25287b2a..e6c004b7ca 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/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/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index de8f151c2c..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. @@ -105,19 +104,6 @@ Spree::Variant.class_eval do OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle).fees_by_type_for self 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 - end - end - def refresh_products_cache if is_master? product.refresh_products_cache 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/app/services/variant_deleter.rb b/app/services/variant_deleter.rb new file mode 100644 index 0000000000..f499f0c24d --- /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.destroy + end + + private + + def only_variant_on_product?(variant) + variant.product.variants == [variant] + 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/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/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..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) @@ -172,7 +163,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 +176,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 +696,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/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index d48d16d636..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) } @@ -535,22 +525,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 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) 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