diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 71812eb60a..098b6b6c90 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -25,7 +25,6 @@ Metrics/LineLength: - app/controllers/admin/accounts_and_billing_settings_controller.rb - app/controllers/admin/bulk_line_items_controller.rb - app/controllers/admin/business_model_configuration_controller.rb - - app/controllers/admin/cache_settings_controller.rb - app/controllers/admin/contents_controller.rb - app/controllers/admin/customers_controller.rb - app/controllers/admin/enterprise_fees_controller.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 9e9a44cb6e..9887e33ab0 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -151,7 +151,6 @@ Layout/EmptyLines: - 'app/models/exchange.rb' - 'app/models/exchange_fee.rb' - 'app/models/inventory_item.rb' - - 'app/models/order_cycle.rb' - 'app/models/producer_property.rb' - 'app/models/product_distribution.rb' - 'app/models/spree/calculator_decorator.rb' @@ -310,7 +309,6 @@ Layout/EmptyLinesAroundBlockBody: Layout/EmptyLinesAroundClassBody: Exclude: - 'app/controllers/admin/account_controller.rb' - - 'app/controllers/admin/cache_settings_controller.rb' - 'app/controllers/admin/enterprise_fees_controller.rb' - 'app/controllers/admin/inventory_items_controller.rb' - 'app/controllers/admin/tag_rules_controller.rb' @@ -763,7 +761,6 @@ Layout/SpaceInsideBlockBraces: # SupportedStylesForEmptyBraces: space, no_space Layout/SpaceInsideHashLiteralBraces: Exclude: - - 'app/controllers/admin/cache_settings_controller.rb' - 'app/controllers/admin/enterprise_relationships_controller.rb' - 'app/controllers/admin/enterprise_roles_controller.rb' - 'app/controllers/api/statuses_controller.rb' @@ -1601,7 +1598,6 @@ Style/ClassAndModuleChildren: - 'app/controllers/admin/account_controller.rb' - 'app/controllers/admin/accounts_and_billing_settings_controller.rb' - 'app/controllers/admin/business_model_configuration_controller.rb' - - 'app/controllers/admin/cache_settings_controller.rb' - 'app/controllers/spree/store_controller_decorator.rb' - 'app/helpers/angular_form_helper.rb' - 'app/models/calculator/flat_percent_per_item.rb' diff --git a/app/controllers/admin/cache_settings_controller.rb b/app/controllers/admin/cache_settings_controller.rb index 1074f13e9f..e922031f68 100644 --- a/app/controllers/admin/cache_settings_controller.rb +++ b/app/controllers/admin/cache_settings_controller.rb @@ -1,18 +1,27 @@ require 'open_food_network/products_cache_integrity_checker' -class Admin::CacheSettingsController < Spree::Admin::BaseController - def edit - @results = Exchange.cachable.map do |exchange| - checker = OpenFoodNetwork::ProductsCacheIntegrityChecker.new(exchange.receiver, exchange.order_cycle) +module Admin + class CacheSettingsController < Spree::Admin::BaseController + def edit + @results = Exchange.cachable.map do |exchange| + checker = OpenFoodNetwork::ProductsCacheIntegrityChecker + .new(exchange.receiver, exchange.order_cycle) - {distributor: exchange.receiver, order_cycle: exchange.order_cycle, status: checker.ok?, diff: checker.diff} + { + distributor: exchange.receiver, + order_cycle: exchange.order_cycle, + status: checker.ok?, + diff: checker.diff + } + end end - end - def update - Spree::Config.set(params[:preferences]) - respond_to do |format| - format.html { redirect_to main_app.edit_admin_cache_settings_path } + def update + Spree::Config.set(params[:preferences]) + + respond_to do |format| + format.html { redirect_to main_app.edit_admin_cache_settings_path } + end end end end diff --git a/app/jobs/refresh_products_cache_job.rb b/app/jobs/refresh_products_cache_job.rb index 0c66925be8..2662237dc1 100644 --- a/app/jobs/refresh_products_cache_job.rb +++ b/app/jobs/refresh_products_cache_job.rb @@ -2,12 +2,15 @@ require 'open_food_network/products_renderer' RefreshProductsCacheJob = Struct.new(:distributor_id, :order_cycle_id) do def perform - Rails.cache.write "products-json-#{distributor_id}-#{order_cycle_id}", products_json + Rails.cache.write(key, products_json) end - private + def key + "products-json-#{distributor_id}-#{order_cycle_id}" + end + def products_json distributor = Enterprise.find distributor_id order_cycle = OrderCycle.find order_cycle_id diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 2790b23b67..e83507ea8a 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -43,7 +43,6 @@ class OrderCycle < ActiveRecord::Base joins(:exchanges).merge(Exchange.outgoing).merge(Exchange.to_enterprise(distributor)) } - scope :managed_by, lambda { |user| if user.has_spree_role?('admin') scoped @@ -167,17 +166,6 @@ class OrderCycle < ActiveRecord::Base variants_distributed_by(distributor).map(&:product).uniq end - # If a product without variants is added to an order cycle, and then some variants are added - # to that product, but not the order cycle, then the master variant should not available for customers - # to purchase. - # This method filters out such products so that the customer cannot purchase them. - def valid_products_distributed_by(distributor) - variants = variants_distributed_by(distributor) - products = variants.map(&:product).uniq - product_ids = products.reject{ |p| product_has_only_obsolete_master_in_distribution?(p, variants) }.map(&:id) - Spree::Product.where(id: product_ids) - end - def products self.variants.map(&:product).uniq end @@ -257,17 +245,6 @@ class OrderCycle < ActiveRecord::Base private - # If a product without variants is added to an order cycle, and then some variants are added - # to that product, but not the order cycle, then the master variant should not available for customers - # to purchase. - # This method is used by #valid_products_distributed_by to filter out such products so that - # the customer cannot purchase them. - def product_has_only_obsolete_master_in_distribution?(product, distributed_variants) - product.has_variants? && - distributed_variants.include?(product.master) && - (product.variants & distributed_variants).empty? - end - def orders_close_at_after_orders_open_at? return if orders_open_at.blank? || orders_close_at.blank? return if orders_close_at > orders_open_at diff --git a/app/services/order_cycle_distributed_products.rb b/app/services/order_cycle_distributed_products.rb new file mode 100644 index 0000000000..c07ddd849f --- /dev/null +++ b/app/services/order_cycle_distributed_products.rb @@ -0,0 +1,41 @@ +# Finds valid products distributed by a particular distributor in an order cycle +# +# If a product without variants is added to an order cycle, and then some +# variants are added to that product, but not the order cycle, then the master +# variant should not available for customers to purchase. This class filters +# out such products so that the customer cannot purchase them. +class OrderCycleDistributedProducts + def initialize(order_cycle, distributor) + @order_cycle = order_cycle + @distributor = distributor + end + + # Returns an ActiveRecord relation without invalid products. Check + # #valid_products_distributed_by for details + # + # @return [ActiveRecord::Relation] + def relation + variants = order_cycle.variants_distributed_by(distributor) + products = variants.map(&:product).uniq + + valid_products = products.reject do |product| + product_has_only_obsolete_master_in_distribution?(product, variants) + end + product_ids = valid_products.map(&:id) + + Spree::Product.where(id: product_ids) + end + + private + + attr_reader :order_cycle, :distributor + + # If a product without variants is added to an order cycle, and then some variants are added + # to that product, but not the order cycle, then the master variant should not available for + # customers to purchase. + def product_has_only_obsolete_master_in_distribution?(product, distributed_variants) + product.has_variants? && + distributed_variants.include?(product.master) && + (product.variants & distributed_variants).empty? + end +end diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb index 6dd0d5f5ed..ef9afb3b1e 100644 --- a/lib/open_food_network/products_renderer.rb +++ b/lib/open_food_network/products_renderer.rb @@ -32,15 +32,16 @@ module OpenFoodNetwork private def load_products - if @order_cycle - scoper = ScopeProductToHub.new(@distributor) + return unless @order_cycle + scoper = ScopeProductToHub.new(@distributor) - @order_cycle. - valid_products_distributed_by(@distributor). - order(taxon_order). - each { |p| scoper.scope(p) }. - select { |p| !p.deleted? && p.has_stock_for_distribution?(@order_cycle, @distributor) } - end + OrderCycleDistributedProducts.new(@order_cycle, @distributor). + relation. + order(taxon_order). + each { |product| scoper.scope(product) }. + select do |product| + !product.deleted? && product.has_stock_for_distribution?(@order_cycle, @distributor) + end end def taxon_order diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 5f46a40985..7627118fac 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -251,60 +251,6 @@ describe OrderCycle do end end - describe "finding valid products distributed by a particular distributor" do - it "returns valid products but not invalid products" do - p_valid = create(:product) - p_invalid = create(:product) - v_valid = p_valid.variants.first - v_invalid = p_invalid.variants.first - - d = create(:distributor_enterprise) - oc = create(:simple_order_cycle, distributors: [d], variants: [v_valid, p_invalid.master]) - - oc.valid_products_distributed_by(d).should == [p_valid] - end - - describe "checking if a product has only an obsolete master variant in a distributution" do - it "returns true when so" do - master = double(:master) - unassociated_variant = double(:variant) - product = double(:product, :has_variants? => true, :master => master, :variants => []) - distributed_variants = [master, unassociated_variant] - - oc = OrderCycle.new - oc.send(:product_has_only_obsolete_master_in_distribution?, product, distributed_variants).should be true - end - - it "returns false when the product doesn't have variants" do - master = double(:master) - product = double(:product, :has_variants? => false, :master => master, :variants => []) - distributed_variants = [master] - - oc = OrderCycle.new - oc.send(:product_has_only_obsolete_master_in_distribution?, product, distributed_variants).should be false - end - - it "returns false when the master isn't distributed" do - master = double(:master) - product = double(:product, :has_variants? => true, :master => master, :variants => []) - distributed_variants = [] - - oc = OrderCycle.new - oc.send(:product_has_only_obsolete_master_in_distribution?, product, distributed_variants).should be false - end - - it "returns false when the product has other variants distributed" do - master = double(:master) - variant = double(:variant) - product = double(:product, :has_variants? => true, :master => master, :variants => [variant]) - distributed_variants = [master, variant] - - oc = OrderCycle.new - oc.send(:product_has_only_obsolete_master_in_distribution?, product, distributed_variants).should be false - end - end - end - describe "exchanges" do before(:each) do @oc = create(:simple_order_cycle) diff --git a/spec/services/order_cycle_distributed_products_spec.rb b/spec/services/order_cycle_distributed_products_spec.rb new file mode 100644 index 0000000000..9670c179dd --- /dev/null +++ b/spec/services/order_cycle_distributed_products_spec.rb @@ -0,0 +1,93 @@ +require 'spec_helper' + +describe OrderCycleDistributedProducts do + let(:order_cycle) { OrderCycle.new } + let(:distributor) { instance_double(Enterprise) } + + it 'returns valid products but not invalid products' do + valid_product = create(:product) + invalid_product = create(:product) + valid_variant = valid_product.variants.first + + distributor = create(:distributor_enterprise) + order_cycle = create( + :simple_order_cycle, + distributors: [distributor], + variants: [valid_variant, invalid_product.master] + ) + + distributed_valid_products = described_class.new(order_cycle, distributor) + + expect(distributed_valid_products.relation).to eq([valid_product]) + end + + context 'when the product has only an obsolete master variant in a distribution' do + let(:master) { create(:variant, product: product) } + let(:product) { create(:product, variants: [build(:variant)]) } + let(:unassociated_variant) { create(:variant) } + let(:distributed_variants) { [product.master, unassociated_variant] } + + before do + allow(order_cycle) + .to receive(:variants_distributed_by).with(distributor) { distributed_variants } + end + + it 'does not return the obsolete product' do + distributed_valid_products = described_class.new(order_cycle, distributor) + expect(distributed_valid_products.relation).to eq([unassociated_variant.product]) + end + end + + context "when the product doesn't have variants" do + let(:master) { build(:variant) } + let(:product) { create(:product, master: master) } + let(:distributed_variants) { [master] } + + before do + allow(product).to receive(:has_variants?) { false } + allow(order_cycle) + .to receive(:variants_distributed_by).with(distributor) { distributed_variants } + end + + it 'returns the product' do + distributed_valid_products = described_class.new(order_cycle, distributor) + expect(distributed_valid_products.relation).to eq([product]) + end + end + + context "when the master isn't distributed" do + let(:master) { build(:variant) } + let(:variant) { build(:variant) } + let(:product) { create(:product, master: master, variants: [variant]) } + let(:distributed_variants) { [variant] } + + before do + allow(product).to receive(:has_variants?) { true } + allow(order_cycle) + .to receive(:variants_distributed_by).with(distributor) { distributed_variants } + end + + it 'returns the product' do + distributed_valid_products = described_class.new(order_cycle, distributor) + expect(distributed_valid_products.relation).to eq([product]) + end + end + + context 'when the product has the master and other variants distributed' do + let(:master) { build(:variant) } + let(:variant) { build(:variant) } + let(:product) { create(:product, master: master, variants: [variant]) } + let(:distributed_variants) { [master, variant] } + + before do + allow(product).to receive(:has_variants?) { true } + allow(order_cycle) + .to receive(:variants_distributed_by).with(distributor) { distributed_variants } + end + + it 'returns the product' do + distributed_valid_products = described_class.new(order_cycle, distributor) + expect(distributed_valid_products.relation).to eq([product]) + end + end +end