Merge pull request #3581 from coopdevs/cache-stylistic-improvements

Cache stylistic improvements

.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

.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'

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

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

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

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<Spree::Product>]
+  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

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

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)

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