From 13633e8bea3cf7b8720f9d8698f255a8d0f0ace2 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Tue, 10 Dec 2019 16:13:13 +0000 Subject: [PATCH 1/4] Use arel in order permissions visible orders and editable orders so that we dont have queries with gigantic IN clauses The | operators here were converting the relations to long lists of IDs, in our current particular issue, an IN clause with 100k order_ids --- app/services/permissions/order.rb | 46 +++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/app/services/permissions/order.rb b/app/services/permissions/order.rb index b4c1e71a2a..9aeab61f94 100644 --- a/app/services/permissions/order.rb +++ b/app/services/permissions/order.rb @@ -7,17 +7,25 @@ module Permissions # Find orders that the user can see def visible_orders - Spree::Order.where(id: - managed_orders.select(:id) | - coordinated_orders.select(:id) | - produced_orders.select("spree_orders.id")) + Spree::Order. + with_line_items_variants_and_products_outer. + where( + Spree::Order.arel_table. + # Grouping keeps the 2 where clauses from produced_orders_where_values inside parentheses + # This way it makes the OR work between the 3 types of orders: + # produced, managed and coordinated + grouping(produced_orders_where_values). + or(managed_orders_where_values). + or(coordinated_orders_where_values) + ) end # Any orders that the user can edit def editable_orders - Spree::Order.where(id: - managed_orders.select(:id) | - coordinated_orders.select(:id) ) + Spree::Order.where( + managed_orders_where_values. + or(coordinated_orders_where_values) + ) end def visible_line_items @@ -28,27 +36,35 @@ module Permissions # Any line items that I can edit def editable_line_items - Spree::LineItem.where(order_id: editable_orders) + Spree::LineItem.where(order_id: editable_orders.select(:id)) end private # Any orders placed through any hub that I manage - def managed_orders - Spree::Order.where(distributor_id: @permissions.managed_enterprises.select("enterprises.id")) + def managed_orders_where_values + Spree::Order. + where(distributor_id: @permissions.managed_enterprises.select("enterprises.id")). + where_values. + reduce(:and) end # Any order that is placed through an order cycle one of my managed enterprises coordinates - def coordinated_orders - Spree::Order.where(order_cycle_id: @permissions.coordinated_order_cycles.select(:id)) + def coordinated_orders_where_values + Spree::Order. + where(order_cycle_id: @permissions.coordinated_order_cycles.select(:id)). + where_values. + reduce(:and) end - def produced_orders + def produced_orders_where_values Spree::Order.with_line_items_variants_and_products_outer. where( distributor_id: granted_distributor_ids, spree_products: { supplier_id: enterprises_with_associated_orders } - ) + ). + where_values. + reduce(:and) end def enterprises_with_associated_orders @@ -69,7 +85,7 @@ module Permissions # Any from visible orders, where the product is produced by one of my managed producers def produced_line_items - Spree::LineItem.where(order_id: visible_orders.select(:id)). + Spree::LineItem.where(order_id: visible_orders.select("DISTINCT spree_orders.id")). joins(:product). where(spree_products: { From f63c7cf54f92bf08c4b53bf93c9ded950ec5f618 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 11 Dec 2019 10:34:28 +0000 Subject: [PATCH 2/4] Extract visible_orders_where_values to a private method --- app/services/permissions/order.rb | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/app/services/permissions/order.rb b/app/services/permissions/order.rb index 9aeab61f94..4dcc1f6ffd 100644 --- a/app/services/permissions/order.rb +++ b/app/services/permissions/order.rb @@ -9,15 +9,7 @@ module Permissions def visible_orders Spree::Order. with_line_items_variants_and_products_outer. - where( - Spree::Order.arel_table. - # Grouping keeps the 2 where clauses from produced_orders_where_values inside parentheses - # This way it makes the OR work between the 3 types of orders: - # produced, managed and coordinated - grouping(produced_orders_where_values). - or(managed_orders_where_values). - or(coordinated_orders_where_values) - ) + where(visible_orders_where_values) end # Any orders that the user can edit @@ -41,6 +33,16 @@ module Permissions private + def visible_orders_where_values + # Grouping keeps the 2 where clauses from produced_orders_where_values inside parentheses + # This way it makes the OR work between the 3 types of orders: + # produced, managed and coordinated + Spree::Order.arel_table. + grouping(produced_orders_where_values). + or(managed_orders_where_values). + or(coordinated_orders_where_values) + end + # Any orders placed through any hub that I manage def managed_orders_where_values Spree::Order. From 55eea21bb03f1cc9c499a6535f0c4b5ac4f0a213 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 11 Dec 2019 12:05:36 +0000 Subject: [PATCH 3/4] Adapt order_and_distributor_report to the new editable_orders query --- lib/open_food_network/order_and_distributor_report.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/open_food_network/order_and_distributor_report.rb b/lib/open_food_network/order_and_distributor_report.rb index 529cf7a6f9..52205f5fcd 100644 --- a/lib/open_food_network/order_and_distributor_report.rb +++ b/lib/open_food_network/order_and_distributor_report.rb @@ -44,7 +44,7 @@ module OpenFoodNetwork # If empty array is passed in, the where clause will return all line_items, which is bad orders_with_hidden_details = - @permissions.editable_orders.empty? ? orders : orders.where('id NOT IN (?)', @permissions.editable_orders) + @permissions.editable_orders.empty? ? orders : orders.where('spree_orders.id NOT IN (?)', @permissions.editable_orders) orders.select{ |order| orders_with_hidden_details.include? order }.each do |order| # TODO We should really be hiding customer code here too, but until we From cbec4956206c71c8499ecb2795a76306433206d1 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 11 Dec 2019 12:16:47 +0000 Subject: [PATCH 4/4] Fix some rubocop issues in order_and_distributor_report --- .rubocop_manual_todo.yml | 1 - .../order_and_distributor_report.rb | 27 ++++++++++++++----- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 61e2fda2a4..634091cc73 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -114,7 +114,6 @@ Metrics/LineLength: - lib/open_food_network/enterprise_issue_validator.rb - lib/open_food_network/group_buy_report.rb - lib/open_food_network/lettuce_share_report.rb - - lib/open_food_network/order_and_distributor_report.rb - lib/open_food_network/order_cycle_form_applicator.rb - lib/open_food_network/order_cycle_management_report.rb - lib/open_food_network/payments_report.rb diff --git a/lib/open_food_network/order_and_distributor_report.rb b/lib/open_food_network/order_and_distributor_report.rb index 52205f5fcd..b32ab803cc 100644 --- a/lib/open_food_network/order_and_distributor_report.rb +++ b/lib/open_food_network/order_and_distributor_report.rb @@ -42,15 +42,17 @@ module OpenFoodNetwork orders = search.result - # If empty array is passed in, the where clause will return all line_items, which is bad - orders_with_hidden_details = - @permissions.editable_orders.empty? ? orders : orders.where('spree_orders.id NOT IN (?)', @permissions.editable_orders) - - orders.select{ |order| orders_with_hidden_details.include? order }.each do |order| + orders.select{ |order| orders_with_hidden_details(orders).include? order }.each do |order| # TODO We should really be hiding customer code here too, but until we # have an actual association between order and customer, it's a bit tricky - order.bill_address.andand.assign_attributes(firstname: I18n.t('admin.reports.hidden'), lastname: "", phone: "", address1: "", address2: "", city: "", zipcode: "", state: nil) - order.ship_address.andand.assign_attributes(firstname: I18n.t('admin.reports.hidden'), lastname: "", phone: "", address1: "", address2: "", city: "", zipcode: "", state: nil) + order.bill_address.andand. + assign_attributes(firstname: I18n.t('admin.reports.hidden'), + lastname: "", phone: "", address1: "", address2: "", + city: "", zipcode: "", state: nil) + order.ship_address.andand. + assign_attributes(firstname: I18n.t('admin.reports.hidden'), + lastname: "", phone: "", address1: "", address2: "", + city: "", zipcode: "", state: nil) order.assign_attributes(email: I18n.t('admin.reports.hidden')) end @@ -59,6 +61,17 @@ module OpenFoodNetwork private + def orders_with_hidden_details(orders) + # If empty array is passed in, the where clause will return all line_items, which is bad + if @permissions.editable_orders.empty? + orders + else + orders. + where('spree_orders.id NOT IN (?)', + @permissions.editable_orders) + end + end + def line_item_details(orders) order_and_distributor_details = []