From e7eb02dfe343bbcc49c8e1e8a12230d5f86b62eb Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Thu, 1 Feb 2018 23:10:22 +1100 Subject: [PATCH] Reduce cognitive complexity of VariantsController#search --- .../admin/variants_controller_decorator.rb | 26 ++------ app/models/spree/variant_decorator.rb | 2 +- .../scope_variants_for_search.rb | 51 +++++++++++++++ .../scope_variants_to_search_spec.rb | 62 +++++++++++++++++++ 4 files changed, 118 insertions(+), 23 deletions(-) create mode 100644 lib/open_food_network/scope_variants_for_search.rb create mode 100644 spec/lib/open_food_network/scope_variants_to_search_spec.rb diff --git a/app/controllers/spree/admin/variants_controller_decorator.rb b/app/controllers/spree/admin/variants_controller_decorator.rb index aaa5d914e7..999ca0b22c 100644 --- a/app/controllers/spree/admin/variants_controller_decorator.rb +++ b/app/controllers/spree/admin/variants_controller_decorator.rb @@ -1,29 +1,11 @@ +require 'open_food_network/scope_variants_for_search' + Spree::Admin::VariantsController.class_eval do helper 'spree/products' def search - search_params = { :product_name_cont => params[:q], :sku_cont => params[:q] } - - @variants = Spree::Variant.where(is_master: false).ransack(search_params.merge(:m => 'or')).result - - if params[:schedule_id].present? - schedule = Schedule.find params[:schedule_id] - @variants = @variants.in_schedule(schedule) - end - - if params[:order_cycle_id].present? - order_cycle = OrderCycle.find params[:order_cycle_id] - @variants = @variants.in_order_cycle(order_cycle) - end - - if params[:distributor_id].present? - distributor = Enterprise.find params[:distributor_id] - @variants = @variants.in_distributor(distributor) - scoper = OpenFoodNetwork::ScopeVariantToHub.new(distributor) - # Perform scoping after all filtering is done. - # Filtering could be a problem on scoped variants. - @variants.each { |v| scoper.scope(v) } - end + scoper = OpenFoodNetwork::ScopeVariantsForSearch.new(params) + @variants = scoper.search end def destroy diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 9d3f56e7dd..2e543785a4 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -43,7 +43,7 @@ Spree::Variant.class_eval do } scope :in_schedule, lambda { |schedule| - joins(exchanges: { order_cycle: :schedule}). + joins(exchanges: { order_cycle: :schedules}). merge(Exchange.outgoing). where(schedules: { id: schedule}). select('DISTINCT spree_variants.*') diff --git a/lib/open_food_network/scope_variants_for_search.rb b/lib/open_food_network/scope_variants_for_search.rb new file mode 100644 index 0000000000..a613f96df7 --- /dev/null +++ b/lib/open_food_network/scope_variants_for_search.rb @@ -0,0 +1,51 @@ +# Used to return a set of variants which match the criteria provided +# A query string is required, which will be match to the name and/or SKU of a product +# Further restrictions on the schedule, order_cycle or distributor through which the +# products are available are also possible + +module OpenFoodNetwork + class ScopeVariantsForSearch + def initialize(params) + @params = params + end + + def search + @variants = query_scope + + scope_to_schedule if params[:schedule_id] + scope_to_order_cycle if params[:order_cycle_id] + scope_to_distributor if params[:distributor_id] + + @variants + end + + private + + attr_reader :params + + def search_params + { :product_name_cont => params[:q], :sku_cont => params[:q] } + end + + def query_scope + Spree::Variant.where(is_master: false).ransack(search_params.merge(:m => 'or')).result + end + + def scope_to_schedule + @variants = @variants.in_schedule(params[:schedule_id]) + end + + def scope_to_order_cycle + @variants = @variants.in_order_cycle(params[:order_cycle_id]) + end + + def scope_to_distributor + distributor = Enterprise.find params[:distributor_id] + @variants = @variants.in_distributor(distributor) + scoper = OpenFoodNetwork::ScopeVariantToHub.new(distributor) + # Perform scoping after all filtering is done. + # Filtering could be a problem on scoped variants. + @variants.each { |v| scoper.scope(v) } + end + end +end diff --git a/spec/lib/open_food_network/scope_variants_to_search_spec.rb b/spec/lib/open_food_network/scope_variants_to_search_spec.rb new file mode 100644 index 0000000000..2d8270597e --- /dev/null +++ b/spec/lib/open_food_network/scope_variants_to_search_spec.rb @@ -0,0 +1,62 @@ +require 'open_food_network/scope_variants_for_search' + +describe OpenFoodNetwork::ScopeVariantsForSearch do + let!(:p1) { create(:simple_product, name: 'Product 1') } + let!(:p2) { create(:simple_product, sku: 'Product 1a') } + let!(:p3) { create(:simple_product, name: 'Product 3') } + let!(:p4) { create(:simple_product, name: 'Product 4') } + let!(:v1) { p1.variants.first } + let!(:v2) { p2.variants.first } + let!(:v3) { p3.variants.first } + let!(:v4) { p4.variants.first } + let!(:d1) { create(:distributor_enterprise) } + let!(:d2) { create(:distributor_enterprise) } + let!(:oc1) { create(:simple_order_cycle, distributors: [d1], variants: [v1, v3]) } + let!(:oc2) { create(:simple_order_cycle, distributors: [d1], variants: [v2]) } + let!(:oc3) { create(:simple_order_cycle, distributors: [d2], variants: [v4]) } + let!(:s1) { create(:schedule, order_cycles: [oc1]) } + let!(:s2) { create(:schedule, order_cycles: [oc2]) } + + let(:scoper) { OpenFoodNetwork::ScopeVariantsForSearch.new(params) } + + describe "search" do + let(:result) { scoper.search } + + context "when a search query is provided" do + let(:params) { { q: "product 1" } } + + it "returns all products whose names or SKUs match the query" do + expect(result).to include v1, v2 + expect(result).to_not include v3, v4 + end + end + + context "when a schedule_id is specified" do + let(:params) { { q: "product", schedule_id: s1.id } } + + it "returns all products distributed through that schedule" do + lala = result + expect(lala).to include v1, v3 + expect(result).to_not include v2, v4 + end + end + + context "when an order_cycle_id is specified" do + let(:params) { { q: "product", order_cycle_id: oc2.id } } + + it "returns all products distributed through that order cycle" do + expect(result).to include v2 + expect(result).to_not include v1, v3, v4 + end + end + + context "when a distributor_id is specified" do + let(:params) { { q: "product", distributor_id: d2.id } } + + it "returns all products distributed through that distributor" do + expect(result).to include v4 + expect(result).to_not include v1, v2, v3 + end + end + end +end