diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index b8ad5662cc..6a7fdb1664 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -81,9 +81,7 @@ Metrics/LineLength: - app/models/enterprise_fee.rb - app/models/enterprise_group.rb - app/models/enterprise_role.rb - - app/models/exchange.rb - app/models/inventory_item.rb - - app/models/order_cycle.rb - app/models/product_import/entry_processor.rb - app/models/product_import/entry_validator.rb - app/models/product_import/product_importer.rb diff --git a/app/controllers/spree/admin/base_controller_decorator.rb b/app/controllers/spree/admin/base_controller_decorator.rb index 1d5d6e476a..6bdd57f0e8 100644 --- a/app/controllers/spree/admin/base_controller_decorator.rb +++ b/app/controllers/spree/admin/base_controller_decorator.rb @@ -52,7 +52,7 @@ Spree::Admin::BaseController.class_eval do def active_distributors_not_ready_for_checkout ocs = OrderCycle.managed_by(spree_current_user).active distributors = ocs.map(&:distributors).flatten.uniq - Enterprise.where('id IN (?)', distributors).not_ready_for_checkout + Enterprise.where('enterprises.id IN (?)', distributors).not_ready_for_checkout end def active_distributors_not_ready_for_checkout_message(distributors) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index a85ffb00e7..09560060df 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -116,9 +116,9 @@ class Enterprise < ActiveRecord::Base scope :not_ready_for_checkout, lambda { # When ready_for_checkout is empty, return all rows when there are no enterprises ready for # checkout. - ready_enterprises = Enterprise.ready_for_checkout + ready_enterprises = Enterprise.ready_for_checkout.select('enterprises.id') if ready_enterprises.present? - where("id NOT IN (?)", ready_enterprises) + where("enterprises.id NOT IN (?)", ready_enterprises) else where("TRUE") end @@ -165,14 +165,14 @@ class Enterprise < ActiveRecord::Base select('DISTINCT enterprises.*') } - scope :distributing_products, lambda { |products| + scope :distributing_products, lambda { |product_ids| exchanges = joins(" INNER JOIN exchanges ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f') "). joins('INNER JOIN exchange_variants ON (exchange_variants.exchange_id = exchanges.id)'). joins('INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id)'). - where('spree_variants.product_id IN (?)', products).select('DISTINCT enterprises.id') + where('spree_variants.product_id IN (?)', product_ids).select('DISTINCT enterprises.id') where(id: exchanges) } @@ -449,7 +449,7 @@ class Enterprise < ActiveRecord::Base end def touch_distributors - Enterprise.distributing_products(supplied_products). + Enterprise.distributing_products(supplied_products.select(:id)). where('enterprises.id != ?', id). find_each(&:touch) end diff --git a/app/models/exchange.rb b/app/models/exchange.rb index 3493ebecde..c85c2f9d8d 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -26,11 +26,25 @@ class Exchange < ActiveRecord::Base scope :to_enterprise, lambda { |enterprise| where(receiver_id: enterprise) } scope :from_enterprises, lambda { |enterprises| where('exchanges.sender_id IN (?)', enterprises) } scope :to_enterprises, lambda { |enterprises| where('exchanges.receiver_id IN (?)', enterprises) } - scope :involving, lambda { |enterprises| where('exchanges.receiver_id IN (?) OR exchanges.sender_id IN (?)', enterprises, enterprises).select('DISTINCT exchanges.*') } - scope :supplying_to, lambda { |distributor| where('exchanges.incoming OR exchanges.receiver_id = ?', distributor) } - scope :with_variant, lambda { |variant| joins(:exchange_variants).where('exchange_variants.variant_id = ?', variant) } - scope :with_any_variant, lambda { |variants| joins(:exchange_variants).where('exchange_variants.variant_id IN (?)', variants).select('DISTINCT exchanges.*') } - scope :with_product, lambda { |product| joins(:exchange_variants).where('exchange_variants.variant_id IN (?)', product.variants_including_master) } + scope :involving, lambda { |enterprises| + where('exchanges.receiver_id IN (?) OR exchanges.sender_id IN (?)', enterprises, enterprises). + select('DISTINCT exchanges.*') + } + scope :supplying_to, lambda { |distributor| + where('exchanges.incoming OR exchanges.receiver_id = ?', distributor) + } + scope :with_variant, lambda { |variant| + joins(:exchange_variants).where('exchange_variants.variant_id = ?', variant) + } + scope :with_any_variant, lambda { |variant_ids| + joins(:exchange_variants). + where('exchange_variants.variant_id IN (?)', variant_ids). + select('DISTINCT exchanges.*') + } + scope :with_product, lambda { |product| + joins(:exchange_variants). + where('exchange_variants.variant_id IN (?)', product.variants_including_master) + } scope :by_enterprise_name, -> { joins('INNER JOIN enterprises AS sender ON (sender.id = exchanges.sender_id)'). joins('INNER JOIN enterprises AS receiver ON (receiver.id = exchanges.receiver_id)'). @@ -49,11 +63,12 @@ class Exchange < ActiveRecord::Base if user.has_spree_role?('admin') scoped else - joins('LEFT JOIN enterprises senders ON senders.id = exchanges.sender_id'). - joins('LEFT JOIN enterprises receivers ON receivers.id = exchanges.receiver_id'). - joins('LEFT JOIN enterprise_roles sender_roles ON sender_roles.enterprise_id = senders.id'). - joins('LEFT JOIN enterprise_roles receiver_roles ON receiver_roles.enterprise_id = receivers.id'). - where('sender_roles.user_id = ? AND receiver_roles.user_id = ?', user.id, user.id) + joins("LEFT JOIN enterprises senders ON senders.id = exchanges.sender_id"). + joins("LEFT JOIN enterprises receivers ON receivers.id = exchanges.receiver_id"). + joins("LEFT JOIN enterprise_roles sender_roles ON sender_roles.enterprise_id = senders.id"). + joins("LEFT JOIN enterprise_roles receiver_roles + ON receiver_roles.enterprise_id = receivers.id"). + where("sender_roles.user_id = ? AND receiver_roles.user_id = ?", user.id, user.id) end } @@ -76,7 +91,8 @@ class Exchange < ActiveRecord::Base end def to_h(core_only = false) - h = attributes.merge('variant_ids' => variant_ids.sort, 'enterprise_fee_ids' => enterprise_fee_ids.sort) + h = attributes.merge('variant_ids' => variant_ids.sort, + 'enterprise_fee_ids' => enterprise_fee_ids.sort) h.reject! { |k| %w(id order_cycle_id created_at updated_at).include? k } if core_only h end diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 707bdae56a..4a37a139f9 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -27,17 +27,30 @@ class OrderCycle < ActiveRecord::Base preference :product_selection_from_coordinator_inventory_only, :boolean, default: false - scope :active, lambda { where('order_cycles.orders_open_at <= ? AND order_cycles.orders_close_at >= ?', Time.zone.now, Time.zone.now) } + scope :active, lambda { + where('order_cycles.orders_open_at <= ? AND order_cycles.orders_close_at >= ?', + Time.zone.now, + Time.zone.now) + } scope :active_or_complete, lambda { where('order_cycles.orders_open_at <= ?', Time.zone.now) } - scope :inactive, lambda { where('order_cycles.orders_open_at > ? OR order_cycles.orders_close_at < ?', Time.zone.now, Time.zone.now) } + scope :inactive, lambda { + where('order_cycles.orders_open_at > ? OR order_cycles.orders_close_at < ?', + Time.zone.now, + Time.zone.now) + } scope :upcoming, lambda { where('order_cycles.orders_open_at > ?', Time.zone.now) } - scope :not_closed, lambda { where('order_cycles.orders_close_at > ? OR order_cycles.orders_close_at IS NULL', Time.zone.now) } - scope :closed, lambda { where('order_cycles.orders_close_at < ?', Time.zone.now).order("order_cycles.orders_close_at DESC") } + scope :not_closed, lambda { + where('order_cycles.orders_close_at > ? OR order_cycles.orders_close_at IS NULL', Time.zone.now) + } + scope :closed, lambda { + where('order_cycles.orders_close_at < ?', + Time.zone.now).order("order_cycles.orders_close_at DESC") + } scope :undated, -> { where('order_cycles.orders_open_at IS NULL OR orders_close_at IS NULL') } scope :dated, -> { where('orders_open_at IS NOT NULL AND orders_close_at IS NOT NULL') } scope :soonest_closing, lambda { active.order('order_cycles.orders_close_at ASC') } - # TODO This method returns all the closed orders. So maybe we can replace it with :recently_closed. + # This scope returns all the closed orders scope :most_recently_closed, lambda { closed.order('order_cycles.orders_close_at DESC') } scope :soonest_opening, lambda { upcoming.order('order_cycles.orders_open_at ASC') } @@ -52,7 +65,7 @@ class OrderCycle < ActiveRecord::Base if user.has_spree_role?('admin') scoped else - where('coordinator_id IN (?)', user.enterprises) + where('coordinator_id IN (?)', user.enterprises.map(&:id)) end } @@ -62,14 +75,17 @@ class OrderCycle < ActiveRecord::Base scoped else with_exchanging_enterprises_outer. - where('order_cycles.coordinator_id IN (?) OR enterprises.id IN (?)', user.enterprises, user.enterprises). + where('order_cycles.coordinator_id IN (?) OR enterprises.id IN (?)', + user.enterprises.map(&:id), + user.enterprises.map(&:id)). select('DISTINCT order_cycles.*') end } scope :with_exchanging_enterprises_outer, lambda { - joins('LEFT OUTER JOIN exchanges ON (exchanges.order_cycle_id = order_cycles.id)'). - joins('LEFT OUTER JOIN enterprises ON (enterprises.id = exchanges.sender_id OR enterprises.id = exchanges.receiver_id)') + joins("LEFT OUTER JOIN exchanges ON (exchanges.order_cycle_id = order_cycles.id)"). + joins("LEFT OUTER JOIN enterprises + ON (enterprises.id = exchanges.sender_id OR enterprises.id = exchanges.receiver_id)") } scope :involving_managed_distributors_of, lambda { |user| @@ -78,7 +94,9 @@ class OrderCycle < ActiveRecord::Base # Order cycles where I managed an enterprise at either end of an outgoing exchange # ie. coordinator or distributor joins(:exchanges).merge(Exchange.outgoing). - where('exchanges.receiver_id IN (?) OR exchanges.sender_id IN (?)', enterprises, enterprises). + where('exchanges.receiver_id IN (?) OR exchanges.sender_id IN (?)', + enterprises.pluck(:id), + enterprises.pluck(:id)). select('DISTINCT order_cycles.*') } @@ -88,7 +106,9 @@ class OrderCycle < ActiveRecord::Base # Order cycles where I managed an enterprise at either end of an incoming exchange # ie. coordinator or producer joins(:exchanges).merge(Exchange.incoming). - where('exchanges.receiver_id IN (?) OR exchanges.sender_id IN (?)', enterprises, enterprises). + where('exchanges.receiver_id IN (?) OR exchanges.sender_id IN (?)', + enterprises.pluck(:id), + enterprises.pluck(:id)). select('DISTINCT order_cycles.*') } @@ -113,7 +133,8 @@ class OrderCycle < ActiveRecord::Base joins(:order_cycle). merge(OrderCycle.active). group('exchanges.receiver_id'). - select('exchanges.receiver_id AS receiver_id, MIN(order_cycles.orders_close_at) AS earliest_close_at'). + select("exchanges.receiver_id AS receiver_id, + MIN(order_cycles.orders_close_at) AS earliest_close_at"). map { |ex| [ex.receiver_id, ex.earliest_close_at.to_time] } ] end @@ -123,7 +144,9 @@ class OrderCycle < ActiveRecord::Base oc.name = I18n.t("models.order_cycle.cloned_order_cycle_name", order_cycle: oc.name) oc.orders_open_at = oc.orders_close_at = nil oc.coordinator_fee_ids = coordinator_fee_ids + # rubocop:disable Metrics/LineLength oc.preferred_product_selection_from_coordinator_inventory_only = preferred_product_selection_from_coordinator_inventory_only + # rubocop:enable Metrics/LineLength oc.save! exchanges.each { |e| e.clone!(oc) } oc.reload @@ -217,7 +240,7 @@ class OrderCycle < ActiveRecord::Base end def exchanges_supplying(order) - exchanges.supplying_to(order.distributor).with_any_variant(order.variants) + exchanges.supplying_to(order.distributor).with_any_variant(order.variants.map(&:id)) end def coordinated_by?(user) @@ -229,8 +252,12 @@ class OrderCycle < ActiveRecord::Base end def items_bought_by_user(user, distributor) - # The Spree::Order.complete scope only checks for completed_at date, does not ensure state is "complete" - orders = Spree::Order.complete.where(state: "complete", user_id: user, distributor_id: distributor, order_cycle_id: self) + # The Spree::Order.complete scope only checks for completed_at date + # it does not ensure state is "complete" + orders = Spree::Order.complete.where(state: "complete", + user_id: user, + distributor_id: distributor, + order_cycle_id: self) scoper = OpenFoodNetwork::ScopeVariantToHub.new(distributor) items = Spree::LineItem.joins(:order).merge(orders) items.each { |li| scoper.scope(li.variant) } diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index 7f5f265289..2e701eb920 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -112,7 +112,7 @@ Spree::Product.class_eval do if user.has_spree_role?('admin') scoped else - where('supplier_id IN (?)', user.enterprises) + where('supplier_id IN (?)', user.enterprises.select("enterprises.id")) end } @@ -189,7 +189,9 @@ Spree::Product.class_eval do OpenFoodNetwork::ProductsCache.product_deleted(self) do touch_distributors - ExchangeVariant.where('exchange_variants.variant_id IN (?)', variants_including_master.with_deleted).destroy_all + ExchangeVariant. + where('exchange_variants.variant_id IN (?)', variants_including_master.with_deleted. + select(:id)).destroy_all destroy_without_delete_from_order_cycles end @@ -216,7 +218,7 @@ Spree::Product.class_eval do end def touch_distributors - Enterprise.distributing_products(self).each(&:touch) + Enterprise.distributing_products(id).each(&:touch) end def add_primary_taxon_to_taxons diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb index a75c27bad6..e49ee2f74f 100644 --- a/lib/open_food_network/order_cycle_permissions.rb +++ b/lib/open_food_network/order_cycle_permissions.rb @@ -55,7 +55,8 @@ module OpenFoodNetwork # TODO: Remove this when all P-OC are sorted out # Any hubs that currently have outgoing exchanges distributing variants of producers I manage variants = Spree::Variant.joins(:product).where('spree_products.supplier_id IN (?)', managed_enterprises.is_primary_producer) - active_exchanges = @order_cycle.exchanges.outgoing.with_any_variant(variants) + active_exchanges = @order_cycle. + exchanges.outgoing.with_any_variant(variants.select("spree_variants.id")) hubs_active = active_exchanges.map(&:receiver_id) # TODO: Remove this when all P-OC are sorted out @@ -248,8 +249,10 @@ module OpenFoodNetwork # active_exchanges is for backward compatability, before we restricted variants in each # outgoing exchange to those where the producer had granted P-OC to the distributor # For any of my managed producers, any outgoing exchanges with their variants - variants = Spree::Variant.joins(:product).where('spree_products.supplier_id IN (?)', producers) - active_exchanges = @order_cycle.exchanges.outgoing.with_any_variant(variants).pluck :id + variants = Spree::Variant. + joins(:product).where('spree_products.supplier_id IN (?)', producers) + active_exchanges = @order_cycle. + exchanges.outgoing.with_any_variant(variants.select("spree_variants.id")).pluck :id permitted_exchanges | active_exchanges end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index b1118cd80d..8dbf166d43 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -5,13 +5,13 @@ require 'open_food_network/products_cache_refreshment' module OpenFoodNetwork class ProductsCache def self.variant_changed(variant) - exchanges_featuring_variants(variant).each do |exchange| + exchanges_featuring_variants(variant.id).each do |exchange| refresh_cache exchange.receiver, exchange.order_cycle end end def self.variant_destroyed(variant, &block) - exchanges = exchanges_featuring_variants(variant).to_a + exchanges = exchanges_featuring_variants(variant.id).to_a block.call @@ -21,13 +21,13 @@ module OpenFoodNetwork end def self.product_changed(product) - exchanges_featuring_variants(product.variants).each do |exchange| + exchanges_featuring_variants(product.variants.map(&:id)).each do |exchange| refresh_cache exchange.receiver, exchange.order_cycle end end def self.product_deleted(product, &block) - exchanges = exchanges_featuring_variants(product.reload.variants).to_a + exchanges = exchanges_featuring_variants(product.reload.variants.map(&:id)).to_a block.call @@ -37,7 +37,7 @@ module OpenFoodNetwork end def self.variant_override_changed(variant_override) - exchanges_featuring_variants(variant_override.variant, distributor: variant_override.hub).each do |exchange| + exchanges_featuring_variants(variant_override.variant.id, distributor: variant_override.hub).each do |exchange| refresh_cache exchange.receiver, exchange.order_cycle end end @@ -52,7 +52,7 @@ module OpenFoodNetwork where(is_master: false, deleted_at: nil). where(product_id: products) - exchanges_featuring_variants(variants).each do |exchange| + exchanges_featuring_variants(variants.select(:id)).each do |exchange| refresh_cache exchange.receiver, exchange.order_cycle end end @@ -94,17 +94,18 @@ module OpenFoodNetwork end def self.inventory_item_changed(inventory_item) - exchanges_featuring_variants(inventory_item.variant, distributor: inventory_item.enterprise).each do |exchange| + exchanges_featuring_variants(inventory_item.variant.id, + distributor: inventory_item.enterprise).each do |exchange| refresh_cache exchange.receiver, exchange.order_cycle end end private - def self.exchanges_featuring_variants(variants, distributor: nil) + def self.exchanges_featuring_variants(variant_ids, distributor: nil) exchanges = Exchange. outgoing. - with_any_variant(variants). + with_any_variant(variant_ids). joins(:order_cycle). merge(OrderCycle.dated). merge(OrderCycle.not_closed) diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb index c648031e88..4e3e9d5565 100644 --- a/spec/models/enterprise_spec.rb +++ b/spec/models/enterprise_spec.rb @@ -360,13 +360,13 @@ describe Enterprise do it "returns enterprises distributing via an order cycle" do order_cycle = create(:simple_order_cycle, distributors: [distributor], variants: [product.master]) - expect(Enterprise.distributing_products(product)).to eq([distributor]) + expect(Enterprise.distributing_products(product.id)).to eq([distributor]) end it "does not return duplicate enterprises" do another_product = create(:product) order_cycle = create(:simple_order_cycle, distributors: [distributor], variants: [product.master, another_product.master]) - expect(Enterprise.distributing_products([product, another_product])).to eq([distributor]) + expect(Enterprise.distributing_products([product.id, another_product.id])).to eq([distributor]) end end diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index bd59675911..3966dbf8e9 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -240,7 +240,7 @@ describe Exchange do ex.variants << v1 ex.variants << v2 - expect(Exchange.with_any_variant([v1, v2, v3])).to eq([ex]) + expect(Exchange.with_any_variant([v1.id, v2.id, v3.id])).to eq([ex]) end it "finds exchanges with a particular product's master variant" do