From df0458743b804f67ab0129c376c4a5abb61f264b Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Thu, 28 Nov 2019 23:37:49 +0000
Subject: [PATCH 01/13] Replace pluck with select in permissions to avoid extra
 queries

---
 lib/open_food_network/permissions.rb | 94 +++++++++++++++-------------
 1 file changed, 51 insertions(+), 43 deletions(-)

diff --git a/lib/open_food_network/permissions.rb b/lib/open_food_network/permissions.rb
index 3c47b73b9c..1fb912ca53 100644
--- a/lib/open_food_network/permissions.rb
+++ b/lib/open_food_network/permissions.rb
@@ -58,55 +58,63 @@ module OpenFoodNetwork
       permissions
     end
 
-    # Find enterprises that an admin is allowed to add to an order cycle
+    # Find orders that a user can see
     def visible_orders
-      # Any orders that I can edit
-      editable = editable_orders.pluck(:id)
+      Spree::Order.where(id:
+        managed_orders.select(:id) |
+        coordinated_orders.select(:id) |
+        produced_orders.select(:id))
+    end
 
-      produced = Spree::Order.with_line_items_variants_and_products_outer.
+    # Any orders that the user can edit
+    def editable_orders
+      Spree::Order.where(id:
+        managed_orders.select(:id) |
+        coordinated_orders.select(:id) )
+    end
+
+    # Any orders placed through any hub that I manage
+    def managed_orders
+      Spree::Order.where(distributor_id: managed_enterprises.select(:id))
+    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: coordinated_order_cycles.select(:id))
+    end
+
+    def produced_orders
+      Spree::Order.with_line_items_variants_and_products_outer.
         where(
           distributor_id: granted_distributor_ids,
           spree_products: { supplier_id: enterprises_with_associated_orders }
-        ).pluck(:id)
-
-      Spree::Order.where(id: editable | produced)
-    end
-
-    # Find enterprises that an admin is allowed to add to an order cycle
-    def editable_orders
-      # Any orders placed through any hub that I manage
-      managed = Spree::Order.where(distributor_id: managed_enterprises.pluck(:id)).pluck(:id)
-
-      # Any order that is placed through an order cycle one of my managed enterprises coordinates
-      coordinated = Spree::Order.
-        where(order_cycle_id: coordinated_order_cycles.pluck(:id)).
-        pluck(:id)
-
-      Spree::Order.where(id: managed | coordinated )
+        )
     end
 
     def visible_line_items
-      # Any line items that I can edit
-      editable = editable_line_items.pluck(:id)
-
-      # Any from visible orders, where the product is produced by one of my managed producers
-      produced = Spree::LineItem.where(order_id: visible_orders.pluck(:id)).
-        joins(:product).
-        where(spree_products: { supplier_id: managed_enterprises.is_primary_producer.pluck(:id) })
-
-      Spree::LineItem.where(id: editable | produced)
+      Spree::LineItem.where(id:
+        editable_line_items.select(:id) |
+        produced_line_items.select(:id))
     end
 
+    # Any line items that I can edit
     def editable_line_items
       Spree::LineItem.where(order_id: editable_orders)
     end
 
+    # 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)).
+        joins(:product).
+        where(spree_products: { supplier_id: managed_enterprises.is_primary_producer.select(:id) })
+    end
+
     def editable_products
       permitted_enterprise_products_ids = product_ids_supplied_by(
         related_enterprises_granting(:manage_products)
       )
       Spree::Product.where(
-        id: managed_enterprise_products.pluck(:id) | permitted_enterprise_products_ids
+        id: managed_enterprise_products.select(:id) | permitted_enterprise_products_ids
       )
     end
 
@@ -116,10 +124,14 @@ module OpenFoodNetwork
           related_enterprises_granting(:add_to_order_cycle)
       )
       Spree::Product.where(
-        id: managed_enterprise_products.pluck(:id) | permitted_enterprise_products_ids
+        id: managed_enterprise_products.select(:id) | permitted_enterprise_products_ids
       )
     end
 
+    def product_ids_supplied_by(supplier_ids)
+      Spree::Product.where(supplier_id: supplier_ids).select(:id)
+    end
+
     def managed_product_enterprises
       managed_and_related_enterprises_granting :manage_products
     end
@@ -130,13 +142,13 @@ module OpenFoodNetwork
 
     def editable_schedules
       Schedule.joins(:order_cycles).
-        where(order_cycles: { id: OrderCycle.managed_by(@user).pluck(:id) }).
+        where(order_cycles: { id: OrderCycle.managed_by(@user).select(:id) }).
         select("DISTINCT schedules.*")
     end
 
     def visible_schedules
       Schedule.joins(:order_cycles).
-        where(order_cycles: { id: OrderCycle.accessible_by(@user).pluck(:id) }).
+        where(order_cycles: { id: OrderCycle.accessible_by(@user).select(:id) }).
         select("DISTINCT schedules.*")
     end
 
@@ -157,8 +169,8 @@ module OpenFoodNetwork
     def granted_distributor_ids
       @granted_distributor_ids ||= related_enterprises_granted(
         :add_to_order_cycle,
-        by: managed_enterprises.is_primary_producer.pluck(:id)
-      ).pluck(:id)
+        by: managed_enterprises.is_primary_producer.select(:id)
+      ).select(:id)
     end
 
     def enterprises_with_associated_orders
@@ -176,7 +188,7 @@ module OpenFoodNetwork
         Enterprise.scoped
       else
         Enterprise.where(
-          id: managed_enterprises.pluck(:id) | related_enterprises_granting(permission)
+          id: managed_enterprises.select(:id) | related_enterprises_granting(permission)
         )
       end
     end
@@ -185,7 +197,7 @@ module OpenFoodNetwork
       if admin?
         Enterprise.scoped
       else
-        managed_enterprise_ids = managed_enterprises.pluck(:id)
+        managed_enterprise_ids = managed_enterprises.select(:id)
         granting_enterprise_ids = related_enterprises_granting(permission)
         granted_enterprise_ids = related_enterprises_granted(permission)
 
@@ -209,7 +221,7 @@ module OpenFoodNetwork
       parent_ids = EnterpriseRelationship.
         permitting(options[:to] || managed_enterprises.select("enterprises.id")).
         with_permission(permission).
-        pluck(:parent_id)
+        select(:parent_id)
 
       (options[:scope] || Enterprise).where(id: parent_ids).select("enterprises.id")
     end
@@ -218,7 +230,7 @@ module OpenFoodNetwork
       child_ids = EnterpriseRelationship.
         permitted_by(options[:by] || managed_enterprises.select("enterprises.id")).
         with_permission(permission).
-        pluck(:child_id)
+        select(:child_id)
 
       (options[:scope] || Enterprise).where(id: child_ids).select("enterprises.id")
     end
@@ -226,9 +238,5 @@ module OpenFoodNetwork
     def managed_enterprise_products
       Spree::Product.managed_by(@user)
     end
-
-    def product_ids_supplied_by(supplier_ids)
-      Spree::Product.where(supplier_id: supplier_ids).pluck(:id)
-    end
   end
 end

From 89056e13edc8e65abbe038d0246a6a203f68a44c Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 00:00:36 +0000
Subject: [PATCH 02/13] Extract order permissions to a separate class

---
 app/services/permissions/order_permissions.rb | 77 +++++++++++++++++++
 lib/open_food_network/packing_report.rb       |  2 +-
 lib/open_food_network/permissions.rb          | 68 ----------------
 3 files changed, 78 insertions(+), 69 deletions(-)
 create mode 100644 app/services/permissions/order_permissions.rb

diff --git a/app/services/permissions/order_permissions.rb b/app/services/permissions/order_permissions.rb
new file mode 100644
index 0000000000..9c266f40cd
--- /dev/null
+++ b/app/services/permissions/order_permissions.rb
@@ -0,0 +1,77 @@
+module Permissions
+  class Order
+    def initialize(user)
+      @user = user
+      @permissions = OpenFoodNetwork::Permissions.new(@user)
+    end
+
+    # 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(:id))
+    end
+
+    # Any orders that the user can edit
+    def editable_orders
+      Spree::Order.where(id:
+        managed_orders.select(:id) |
+        coordinated_orders.select(:id) )
+    end
+
+    def visible_line_items
+      Spree::LineItem.where(id:
+        editable_line_items.select(:id) |
+        produced_line_items.select(:id))
+    end
+
+    # Any line items that I can edit
+    def editable_line_items
+      Spree::LineItem.where(order_id: editable_orders)
+    end
+
+    private
+
+    # Any orders placed through any hub that I manage
+    def managed_orders
+      Spree::Order.where(distributor_id: @permissions.managed_enterprises.select(:id))
+    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))
+    end
+
+    def produced_orders
+      Spree::Order.with_line_items_variants_and_products_outer.
+        where(
+          distributor_id: granted_distributor_ids,
+          spree_products: { supplier_id: enterprises_with_associated_orders }
+        )
+    end
+
+    def enterprises_with_associated_orders
+      # Any orders placed through hubs that my producers have granted P-OC,
+      #   and which contain their products. This is pretty complicated but it's looking for order
+      #   where at least one of my producers has granted P-OC to the distributor
+      #   AND the order contains products of at least one of THE SAME producers
+      @permissions.related_enterprises_granting(:add_to_order_cycle, to: granted_distributor_ids).
+        merge(@permissions.managed_enterprises.is_primary_producer)
+    end
+
+    def granted_distributor_ids
+      @granted_distributor_ids ||= @permissions.related_enterprises_granted(
+        :add_to_order_cycle,
+        by: @permissions.managed_enterprises.is_primary_producer.select(:id)
+      ).select(:id)
+    end
+
+    # 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)).
+        joins(:product).
+        where(spree_products: { supplier_id: @permissions.managed_enterprises.is_primary_producer.select(:id) })
+    end
+  end
+end
diff --git a/lib/open_food_network/packing_report.rb b/lib/open_food_network/packing_report.rb
index 3f5e420626..0ecfdfce9b 100644
--- a/lib/open_food_network/packing_report.rb
+++ b/lib/open_food_network/packing_report.rb
@@ -131,7 +131,7 @@ module OpenFoodNetwork
 
     def permissions
       return @permissions unless @permissions.nil?
-      @permissions = OpenFoodNetwork::Permissions.new(@user)
+      @permissions = Permissions::Order.new(@user)
     end
 
     def is_temperature_controlled?(line_item)
diff --git a/lib/open_food_network/permissions.rb b/lib/open_food_network/permissions.rb
index 1fb912ca53..4af7bbd907 100644
--- a/lib/open_food_network/permissions.rb
+++ b/lib/open_food_network/permissions.rb
@@ -58,57 +58,6 @@ module OpenFoodNetwork
       permissions
     end
 
-    # Find orders that a user can see
-    def visible_orders
-      Spree::Order.where(id:
-        managed_orders.select(:id) |
-        coordinated_orders.select(:id) |
-        produced_orders.select(:id))
-    end
-
-    # Any orders that the user can edit
-    def editable_orders
-      Spree::Order.where(id:
-        managed_orders.select(:id) |
-        coordinated_orders.select(:id) )
-    end
-
-    # Any orders placed through any hub that I manage
-    def managed_orders
-      Spree::Order.where(distributor_id: managed_enterprises.select(:id))
-    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: coordinated_order_cycles.select(:id))
-    end
-
-    def produced_orders
-      Spree::Order.with_line_items_variants_and_products_outer.
-        where(
-          distributor_id: granted_distributor_ids,
-          spree_products: { supplier_id: enterprises_with_associated_orders }
-        )
-    end
-
-    def visible_line_items
-      Spree::LineItem.where(id:
-        editable_line_items.select(:id) |
-        produced_line_items.select(:id))
-    end
-
-    # Any line items that I can edit
-    def editable_line_items
-      Spree::LineItem.where(order_id: editable_orders)
-    end
-
-    # 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)).
-        joins(:product).
-        where(spree_products: { supplier_id: managed_enterprises.is_primary_producer.select(:id) })
-    end
-
     def editable_products
       permitted_enterprise_products_ids = product_ids_supplied_by(
         related_enterprises_granting(:manage_products)
@@ -166,23 +115,6 @@ module OpenFoodNetwork
       @user.admin?
     end
 
-    def granted_distributor_ids
-      @granted_distributor_ids ||= related_enterprises_granted(
-        :add_to_order_cycle,
-        by: managed_enterprises.is_primary_producer.select(:id)
-      ).select(:id)
-    end
-
-    def enterprises_with_associated_orders
-      # Any orders placed through hubs that my producers have granted P-OC,
-      #   and which contain their products. This is pretty complicated but it's looking for order
-      #   where at least one of my producers has granted P-OC to the distributor
-      #   AND the order contains products of at least one of THE SAME producers
-
-      related_enterprises_granting(:add_to_order_cycle, to: granted_distributor_ids).
-        merge(managed_enterprises.is_primary_producer)
-    end
-
     def managed_and_related_enterprises_granting(permission)
       if admin?
         Enterprise.scoped

From bb2e6324bddd2e666785dd54c600f2e2604a22b4 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 08:41:43 +0000
Subject: [PATCH 03/13] Rename order permissions to just order

---
 app/services/permissions/{order_permissions.rb => order.rb} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename app/services/permissions/{order_permissions.rb => order.rb} (100%)

diff --git a/app/services/permissions/order_permissions.rb b/app/services/permissions/order.rb
similarity index 100%
rename from app/services/permissions/order_permissions.rb
rename to app/services/permissions/order.rb

From 484cdd1e072f1a831c66c62427e975d7d9c8cf24 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 09:28:35 +0000
Subject: [PATCH 04/13] Make managed_and_related_enterprises public so they can
 be used by other permissions classes

---
 lib/open_food_network/permissions.rb | 60 ++++++++++++++--------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/lib/open_food_network/permissions.rb b/lib/open_food_network/permissions.rb
index 4af7bbd907..a914d116c8 100644
--- a/lib/open_food_network/permissions.rb
+++ b/lib/open_food_network/permissions.rb
@@ -109,36 +109,6 @@ module OpenFoodNetwork
       editable_subscriptions
     end
 
-    private
-
-    def admin?
-      @user.admin?
-    end
-
-    def managed_and_related_enterprises_granting(permission)
-      if admin?
-        Enterprise.scoped
-      else
-        Enterprise.where(
-          id: managed_enterprises.select(:id) | related_enterprises_granting(permission)
-        )
-      end
-    end
-
-    def managed_and_related_enterprises_with(permission)
-      if admin?
-        Enterprise.scoped
-      else
-        managed_enterprise_ids = managed_enterprises.select(:id)
-        granting_enterprise_ids = related_enterprises_granting(permission)
-        granted_enterprise_ids = related_enterprises_granted(permission)
-
-        Enterprise.where(
-          id: managed_enterprise_ids | granting_enterprise_ids | granted_enterprise_ids
-        )
-      end
-    end
-
     def managed_enterprises
       @managed_enterprises ||= Enterprise.managed_by(@user)
     end
@@ -167,6 +137,36 @@ module OpenFoodNetwork
       (options[:scope] || Enterprise).where(id: child_ids).select("enterprises.id")
     end
 
+    private
+
+    def admin?
+      @user.admin?
+    end
+
+    def managed_and_related_enterprises_granting(permission)
+      if admin?
+        Enterprise.scoped
+      else
+        Enterprise.where(
+          id: managed_enterprises.select("enterprises.id") | related_enterprises_granting(permission)
+        )
+      end
+    end
+
+    def managed_and_related_enterprises_with(permission)
+      if admin?
+        Enterprise.scoped
+      else
+        managed_enterprise_ids = managed_enterprises.select("enterprises.id")
+        granting_enterprise_ids = related_enterprises_granting(permission)
+        granted_enterprise_ids = related_enterprises_granted(permission)
+
+        Enterprise.where(
+          id: managed_enterprise_ids | granting_enterprise_ids | granted_enterprise_ids
+        )
+      end
+    end
+
     def managed_enterprise_products
       Spree::Product.managed_by(@user)
     end

From 82b274e52253717db8a51239e4ead979f8c44ae7 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 09:29:04 +0000
Subject: [PATCH 05/13] Make selector more specific to avoid sql error
 'ambiguos column'

---
 app/services/permissions/order.rb | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/services/permissions/order.rb b/app/services/permissions/order.rb
index 9c266f40cd..9b15eb739c 100644
--- a/app/services/permissions/order.rb
+++ b/app/services/permissions/order.rb
@@ -10,7 +10,7 @@ module Permissions
       Spree::Order.where(id:
         managed_orders.select(:id) |
         coordinated_orders.select(:id) |
-        produced_orders.select(:id))
+        produced_orders.select("spree_orders.id"))
     end
 
     # Any orders that the user can edit
@@ -23,7 +23,7 @@ module Permissions
     def visible_line_items
       Spree::LineItem.where(id:
         editable_line_items.select(:id) |
-        produced_line_items.select(:id))
+        produced_line_items.select("spree_line_items.id"))
     end
 
     # Any line items that I can edit
@@ -35,7 +35,7 @@ module Permissions
 
     # Any orders placed through any hub that I manage
     def managed_orders
-      Spree::Order.where(distributor_id: @permissions.managed_enterprises.select(:id))
+      Spree::Order.where(distributor_id: @permissions.managed_enterprises.select("enterprises.id"))
     end
 
     # Any order that is placed through an order cycle one of my managed enterprises coordinates
@@ -63,15 +63,15 @@ module Permissions
     def granted_distributor_ids
       @granted_distributor_ids ||= @permissions.related_enterprises_granted(
         :add_to_order_cycle,
-        by: @permissions.managed_enterprises.is_primary_producer.select(:id)
-      ).select(:id)
+        by: @permissions.managed_enterprises.is_primary_producer.select("enterprises.id")
+      ).select("enterprises.id")
     end
 
     # 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)).
         joins(:product).
-        where(spree_products: { supplier_id: @permissions.managed_enterprises.is_primary_producer.select(:id) })
+        where(spree_products: { supplier_id: @permissions.managed_enterprises.is_primary_producer.select("enterprises.id") })
     end
   end
 end

From 8d16f496f4b8419387baa1433a12fb6619947049 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 10:34:59 +0000
Subject: [PATCH 06/13] Move Permissions::Order specs to its specific spec file

---
 .../lib/open_food_network/permissions_spec.rb | 131 ----------------
 spec/services/permissions/order_spec.rb       | 142 ++++++++++++++++++
 2 files changed, 142 insertions(+), 131 deletions(-)
 create mode 100644 spec/services/permissions/order_spec.rb

diff --git a/spec/lib/open_food_network/permissions_spec.rb b/spec/lib/open_food_network/permissions_spec.rb
index 473a65fb2b..83d6de78ae 100644
--- a/spec/lib/open_food_network/permissions_spec.rb
+++ b/spec/lib/open_food_network/permissions_spec.rb
@@ -269,137 +269,6 @@ module OpenFoodNetwork
       end
     end
 
-    describe "finding orders that are visible in reports" do
-      let(:distributor) { create(:distributor_enterprise) }
-      let(:coordinator) { create(:distributor_enterprise) }
-      let(:random_enterprise) { create(:distributor_enterprise) }
-      let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator, distributors: [distributor]) }
-      let(:order) { create(:order, order_cycle: order_cycle, distributor: distributor ) }
-      let!(:line_item) { create(:line_item, order: order) }
-      let!(:producer) { create(:supplier_enterprise) }
-
-      before do
-        allow(permissions).to receive(:coordinated_order_cycles) { Enterprise.where("1=0") }
-      end
-
-      context "as the hub through which the order was placed" do
-        before do
-          allow(permissions).to receive(:managed_enterprises) { Enterprise.where(id: distributor) }
-        end
-
-        it "should let me see the order" do
-          expect(permissions.visible_orders).to include order
-        end
-      end
-
-      context "as the coordinator of the order cycle through which the order was placed" do
-        before do
-          allow(permissions).to receive(:managed_enterprises) { Enterprise.where(id: coordinator) }
-          allow(permissions).to receive(:coordinated_order_cycles) { OrderCycle.where(id: order_cycle) }
-        end
-
-        it "should let me see the order" do
-          expect(permissions.visible_orders).to include order
-        end
-      end
-
-      context "as a producer which has granted P-OC to the distributor of an order" do
-        before do
-          allow(permissions).to receive(:managed_enterprises) { Enterprise.where(id: producer) }
-          create(:enterprise_relationship, parent: producer, child: distributor, permissions_list: [:add_to_order_cycle])
-        end
-
-        context "which contains my products" do
-          before do
-            line_item.product.supplier = producer
-            line_item.product.save
-          end
-
-          it "should let me see the order" do
-            expect(permissions.visible_orders).to include order
-          end
-        end
-
-        context "which does not contain my products" do
-          it "should not let me see the order" do
-            expect(permissions.visible_orders).to_not include order
-          end
-        end
-      end
-
-      context "as an enterprise that is a distributor in the order cycle, but not the distributor of the order" do
-        before do
-          allow(permissions).to receive(:managed_enterprises) { Enterprise.where(id: random_enterprise) }
-        end
-
-        it "should not let me see the order" do
-          expect(permissions.visible_orders).to_not include order
-        end
-      end
-    end
-
-    describe "finding line items that are visible in reports" do
-      let(:distributor) { create(:distributor_enterprise) }
-      let(:coordinator) { create(:distributor_enterprise) }
-      let(:random_enterprise) { create(:distributor_enterprise) }
-      let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator, distributors: [distributor]) }
-      let(:order) { create(:order, order_cycle: order_cycle, distributor: distributor ) }
-      let!(:line_item1) { create(:line_item, order: order) }
-      let!(:line_item2) { create(:line_item, order: order) }
-      let!(:producer) { create(:supplier_enterprise) }
-
-      before do
-        allow(permissions).to receive(:coordinated_order_cycles) { Enterprise.where("1=0") }
-      end
-
-      context "as the hub through which the parent order was placed" do
-        before do
-          allow(permissions).to receive(:managed_enterprises) { Enterprise.where(id: distributor) }
-        end
-
-        it "should let me see the line_items" do
-          expect(permissions.visible_line_items).to include line_item1, line_item2
-        end
-      end
-
-      context "as the coordinator of the order cycle through which the parent order was placed" do
-        before do
-          allow(permissions).to receive(:managed_enterprises) { Enterprise.where(id: coordinator) }
-          allow(permissions).to receive(:coordinated_order_cycles) { OrderCycle.where(id: order_cycle) }
-        end
-
-        it "should let me see the line_items" do
-          expect(permissions.visible_line_items).to include line_item1, line_item2
-        end
-      end
-
-      context "as the manager producer which has granted P-OC to the distributor of the parent order" do
-        before do
-          allow(permissions).to receive(:managed_enterprises) { Enterprise.where(id: producer) }
-          create(:enterprise_relationship, parent: producer, child: distributor, permissions_list: [:add_to_order_cycle])
-
-          line_item1.product.supplier = producer
-          line_item1.product.save
-        end
-
-        it "should let me see the line_items pertaining to variants I produce" do
-          ps = permissions.visible_line_items
-          expect(ps).to include line_item1
-          expect(ps).to_not include line_item2
-        end
-      end
-
-      context "as an enterprise that is a distributor in the order cycle, but not the distributor of the parent order" do
-        before do
-          allow(permissions).to receive(:managed_enterprises) { Enterprise.where(id: random_enterprise) }
-        end
-
-        it "should not let me see the line_items" do
-          expect(permissions.visible_line_items).to_not include line_item1, line_item2
-        end
-      end
-    end
-
     describe "finding visible subscriptions" do
       let!(:so1) { create(:subscription) }
       let!(:so2) { create(:subscription) }
diff --git a/spec/services/permissions/order_spec.rb b/spec/services/permissions/order_spec.rb
new file mode 100644
index 0000000000..1ccb6648ec
--- /dev/null
+++ b/spec/services/permissions/order_spec.rb
@@ -0,0 +1,142 @@
+require 'spec_helper'
+
+module Permissions
+  describe Order do
+    let(:user) { double(:user) }
+    let(:permissions) { Permissions::Order.new(user) }
+    let!(:basic_permissions) { OpenFoodNetwork::Permissions.new(user) }
+
+    before { allow(OpenFoodNetwork::Permissions).to receive(:new) { basic_permissions } }
+
+    describe "finding orders that are visible in reports" do
+      let(:distributor) { create(:distributor_enterprise) }
+      let(:coordinator) { create(:distributor_enterprise) }
+      let(:random_enterprise) { create(:distributor_enterprise) }
+      let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator, distributors: [distributor]) }
+      let(:order) { create(:order, order_cycle: order_cycle, distributor: distributor ) }
+      let!(:line_item) { create(:line_item, order: order) }
+      let!(:producer) { create(:supplier_enterprise) }
+
+      before do
+        allow(basic_permissions).to receive(:coordinated_order_cycles) { Enterprise.where("1=0") }
+      end
+
+      context "as the hub through which the order was placed" do
+        before do
+          allow(basic_permissions).to receive(:managed_enterprises) { Enterprise.where(id: distributor) }
+        end
+
+        it "should let me see the order" do
+          expect(permissions.visible_orders).to include order
+        end
+      end
+
+      context "as the coordinator of the order cycle through which the order was placed" do
+        before do
+          allow(basic_permissions).to receive(:managed_enterprises) { Enterprise.where(id: coordinator) }
+          allow(basic_permissions).to receive(:coordinated_order_cycles) { OrderCycle.where(id: order_cycle) }
+        end
+
+        it "should let me see the order" do
+          expect(permissions.visible_orders).to include order
+        end
+      end
+
+      context "as a producer which has granted P-OC to the distributor of an order" do
+        before do
+          allow(basic_permissions).to receive(:managed_enterprises) { Enterprise.where(id: producer) }
+          create(:enterprise_relationship, parent: producer, child: distributor, permissions_list: [:add_to_order_cycle])
+        end
+
+        context "which contains my products" do
+          before do
+            line_item.product.supplier = producer
+            line_item.product.save
+          end
+
+          it "should let me see the order" do
+            expect(permissions.visible_orders).to include order
+          end
+        end
+
+        context "which does not contain my products" do
+          it "should not let me see the order" do
+            expect(permissions.visible_orders).to_not include order
+          end
+        end
+      end
+
+      context "as an enterprise that is a distributor in the order cycle, but not the distributor of the order" do
+        before do
+          allow(basic_permissions).to receive(:managed_enterprises) { Enterprise.where(id: random_enterprise) }
+        end
+
+        it "should not let me see the order" do
+          expect(permissions.visible_orders).to_not include order
+        end
+      end
+    end
+
+    describe "finding line items that are visible in reports" do
+      let(:distributor) { create(:distributor_enterprise) }
+      let(:coordinator) { create(:distributor_enterprise) }
+      let(:random_enterprise) { create(:distributor_enterprise) }
+      let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator, distributors: [distributor]) }
+      let(:order) { create(:order, order_cycle: order_cycle, distributor: distributor ) }
+      let!(:line_item1) { create(:line_item, order: order) }
+      let!(:line_item2) { create(:line_item, order: order) }
+      let!(:producer) { create(:supplier_enterprise) }
+
+      before do
+        allow(basic_permissions).to receive(:coordinated_order_cycles) { Enterprise.where("1=0") }
+      end
+
+      context "as the hub through which the parent order was placed" do
+        before do
+          allow(basic_permissions).to receive(:managed_enterprises) { Enterprise.where(id: distributor) }
+        end
+
+        it "should let me see the line_items" do
+          expect(permissions.visible_line_items).to include line_item1, line_item2
+        end
+      end
+
+      context "as the coordinator of the order cycle through which the parent order was placed" do
+        before do
+          allow(basic_permissions).to receive(:managed_enterprises) { Enterprise.where(id: coordinator) }
+          allow(basic_permissions).to receive(:coordinated_order_cycles) { OrderCycle.where(id: order_cycle) }
+        end
+
+        it "should let me see the line_items" do
+          expect(permissions.visible_line_items).to include line_item1, line_item2
+        end
+      end
+
+      context "as the manager producer which has granted P-OC to the distributor of the parent order" do
+        before do
+          allow(basic_permissions).to receive(:managed_enterprises) { Enterprise.where(id: producer) }
+          create(:enterprise_relationship, parent: producer, child: distributor, permissions_list: [:add_to_order_cycle])
+
+          line_item1.product.supplier = producer
+          line_item1.product.save
+        end
+
+        it "should let me see the line_items pertaining to variants I produce" do
+          ps = permissions.visible_line_items
+          expect(ps).to include line_item1
+          expect(ps).to_not include line_item2
+        end
+      end
+
+      context "as an enterprise that is a distributor in the order cycle, but not the distributor of the parent order" do
+        before do
+          allow(basic_permissions).to receive(:managed_enterprises) { Enterprise.where(id: random_enterprise) }
+        end
+
+        it "should not let me see the line_items" do
+          expect(permissions.visible_line_items).to_not include line_item1, line_item2
+        end
+      end
+    end
+  end
+end

From 5cb77c443ba3727e40e1d943bbc16883a22a1fcb Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 10:53:40 +0000
Subject: [PATCH 07/13] Fix rubocop issues

---
 app/services/permissions/order.rb    | 5 ++++-
 lib/open_food_network/permissions.rb | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/app/services/permissions/order.rb b/app/services/permissions/order.rb
index 9b15eb739c..b4c1e71a2a 100644
--- a/app/services/permissions/order.rb
+++ b/app/services/permissions/order.rb
@@ -71,7 +71,10 @@ module Permissions
     def produced_line_items
       Spree::LineItem.where(order_id: visible_orders.select(:id)).
         joins(:product).
-        where(spree_products: { supplier_id: @permissions.managed_enterprises.is_primary_producer.select("enterprises.id") })
+        where(spree_products:
+        {
+          supplier_id: @permissions.managed_enterprises.is_primary_producer.select("enterprises.id")
+        })
     end
   end
 end
diff --git a/lib/open_food_network/permissions.rb b/lib/open_food_network/permissions.rb
index a914d116c8..57127e7247 100644
--- a/lib/open_food_network/permissions.rb
+++ b/lib/open_food_network/permissions.rb
@@ -148,7 +148,8 @@ module OpenFoodNetwork
         Enterprise.scoped
       else
         Enterprise.where(
-          id: managed_enterprises.select("enterprises.id") | related_enterprises_granting(permission)
+          id: managed_enterprises.select("enterprises.id") |
+                related_enterprises_granting(permission)
         )
       end
     end

From da6d035a1dd73d0a1c788cfe827e1d18b1fd9f91 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 11:05:03 +0000
Subject: [PATCH 08/13] Rename some reports permissions to order_permissions

---
 .../spree/admin/reports_controller_decorator.rb  |  3 +--
 lib/open_food_network/bulk_coop_report.rb        | 10 +++++-----
 .../orders_and_fulfillments_report.rb            | 15 +++++++++------
 lib/open_food_network/packing_report.rb          | 10 +++++-----
 lib/open_food_network/reports/line_items.rb      | 16 ++++++++--------
 .../customer_totals_report_spec.rb               |  3 +--
 ...distributor_totals_by_supplier_report_spec.rb |  3 +--
 ...supplier_totals_by_distributor_report_spec.rb |  3 +--
 .../supplier_totals_report_spec.rb               |  3 +--
 .../orders_and_fulfillments_report_spec.rb       | 13 +++++--------
 10 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/app/controllers/spree/admin/reports_controller_decorator.rb b/app/controllers/spree/admin/reports_controller_decorator.rb
index 3a7f5217fc..cf0970e45c 100644
--- a/app/controllers/spree/admin/reports_controller_decorator.rb
+++ b/app/controllers/spree/admin/reports_controller_decorator.rb
@@ -126,8 +126,7 @@ Spree::Admin::ReportsController.class_eval do
     @include_blank = I18n.t(:all)
 
     # -- Build Report with Order Grouper
-    @report = OpenFoodNetwork::OrdersAndFulfillmentsReport.new(permissions,
-                                                               params, render_content?)
+    @report = OpenFoodNetwork::OrdersAndFulfillmentsReport.new(spree_current_user, params, render_content?)
     @table = order_grouper_table
     csv_file_name = "#{params[:report_type]}_#{timestamp}.csv"
 
diff --git a/lib/open_food_network/bulk_coop_report.rb b/lib/open_food_network/bulk_coop_report.rb
index 5339200185..fcaa3afbb8 100644
--- a/lib/open_food_network/bulk_coop_report.rb
+++ b/lib/open_food_network/bulk_coop_report.rb
@@ -46,12 +46,12 @@ module OpenFoodNetwork
     end
 
     def search
-      Reports::LineItems.search_orders(permissions, params)
+      Reports::LineItems.search_orders(order_permissions, params)
     end
 
     def table_items
       return [] unless @render_table
-      Reports::LineItems.list(permissions, report_options)
+      Reports::LineItems.list(order_permissions, report_options)
     end
 
     def rules
@@ -130,9 +130,9 @@ module OpenFoodNetwork
          variant: [{ option_values: :option_type }, { product: :supplier }] }]
     end
 
-    def permissions
-      return @permissions unless @permissions.nil?
-      @permissions = OpenFoodNetwork::Permissions.new(@user)
+    def order_permissions
+      return @order_permissions unless @order_permissions.nil?
+      @order_permissions = ::Permissions::Order.new(@user)
     end
   end
 end
diff --git a/lib/open_food_network/orders_and_fulfillments_report.rb b/lib/open_food_network/orders_and_fulfillments_report.rb
index f930ba50c6..dbecd1fc29 100644
--- a/lib/open_food_network/orders_and_fulfillments_report.rb
+++ b/lib/open_food_network/orders_and_fulfillments_report.rb
@@ -13,28 +13,26 @@ module OpenFoodNetwork
 
     delegate :header, :rules, :columns, to: :report
 
-    def initialize(permissions, options = {}, render_table = false)
+    def initialize(user, options = {}, render_table = false)
+      @user = user
       @options = options
       @report_type = options[:report_type]
-      @permissions = permissions
       @render_table = render_table
     end
 
     def search
-      Reports::LineItems.search_orders(permissions, options)
+      Reports::LineItems.search_orders(order_permissions, options)
     end
 
     def table_items
       return [] unless @render_table
 
       list_options = options.merge(line_item_includes: report.line_item_includes)
-      Reports::LineItems.list(permissions, list_options)
+      Reports::LineItems.list(order_permissions, list_options)
     end
 
     private
 
-    attr_reader :permissions
-
     def report
       @report ||= report_klass.new(self)
     end
@@ -84,5 +82,10 @@ module OpenFoodNetwork
     def scale_factor(product)
       product.variant_unit == 'weight' ? 1000 : 1
     end
+
+    def order_permissions
+      return @order_permissions unless @order_permissions.nil?
+      @order_permissions = ::Permissions::Order.new(@user)
+    end
   end
 end
diff --git a/lib/open_food_network/packing_report.rb b/lib/open_food_network/packing_report.rb
index 0ecfdfce9b..4d07c91b40 100644
--- a/lib/open_food_network/packing_report.rb
+++ b/lib/open_food_network/packing_report.rb
@@ -38,12 +38,12 @@ module OpenFoodNetwork
     end
 
     def search
-      Reports::LineItems.search_orders(permissions, params)
+      Reports::LineItems.search_orders(order_permissions, params)
     end
 
     def table_items
       return [] unless @render_table
-      Reports::LineItems.list(permissions, report_options)
+      Reports::LineItems.list(order_permissions, report_options)
     end
 
     def rules
@@ -129,9 +129,9 @@ module OpenFoodNetwork
          variant: [{ option_values: :option_type }, { product: :supplier }] }]
     end
 
-    def permissions
-      return @permissions unless @permissions.nil?
-      @permissions = Permissions::Order.new(@user)
+    def order_permissions
+      return @order_permissions unless @order_permissions.nil?
+      @order_permissions = ::Permissions::Order.new(@user)
     end
 
     def is_temperature_controlled?(line_item)
diff --git a/lib/open_food_network/reports/line_items.rb b/lib/open_food_network/reports/line_items.rb
index 7b85065b1f..a8209056b5 100644
--- a/lib/open_food_network/reports/line_items.rb
+++ b/lib/open_food_network/reports/line_items.rb
@@ -2,21 +2,21 @@ module OpenFoodNetwork
   module Reports
     # shared code to search and list line items
     module LineItems
-      def self.search_orders(permissions, params)
-        permissions.visible_orders.complete.not_state(:canceled).search(params[:q])
+      def self.search_orders(order_permissions, params)
+        order_permissions.visible_orders.complete.not_state(:canceled).search(params[:q])
       end
 
-      def self.list(permissions, params)
-        orders = search_orders(permissions, params).result
+      def self.list(order_permissions, params)
+        orders = search_orders(order_permissions, params).result
 
-        line_items = permissions.visible_line_items.merge(Spree::LineItem.where(order_id: orders))
+        line_items = order_permissions.visible_line_items.merge(Spree::LineItem.where(order_id: orders))
         line_items = line_items.supplied_by_any(params[:supplier_id_in]) if params[:supplier_id_in].present?
 
         if params[:line_item_includes].present?
           line_items = line_items.includes(*params[:line_item_includes])
         end
 
-        hidden_line_items = line_items_with_hidden_details(permissions, line_items)
+        hidden_line_items = line_items_with_hidden_details(order_permissions, line_items)
 
         line_items.select{ |li|
           hidden_line_items.include? li
@@ -30,8 +30,8 @@ module OpenFoodNetwork
         line_items
       end
 
-      def self.line_items_with_hidden_details(permissions, line_items)
-        editable_line_items = permissions.editable_line_items.pluck(:id)
+      def self.line_items_with_hidden_details(order_permissions, line_items)
+        editable_line_items = order_permissions.editable_line_items.pluck(:id)
 
         if editable_line_items.empty?
           line_items
diff --git a/spec/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report_spec.rb b/spec/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report_spec.rb
index 261e52a925..d224830e0b 100644
--- a/spec/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report_spec.rb
+++ b/spec/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report_spec.rb
@@ -4,11 +4,10 @@ RSpec.describe OpenFoodNetwork::OrdersAndFulfillmentsReport::CustomerTotalsRepor
   let!(:distributor) { create(:distributor_enterprise) }
   let!(:customer) { create(:customer, enterprise: distributor) }
   let(:current_user) { distributor.owner }
-  let(:permissions) { OpenFoodNetwork::Permissions.new(current_user) }
 
   let(:report) do
     report_options = { report_type: described_class::REPORT_TYPE }
-    OpenFoodNetwork::OrdersAndFulfillmentsReport.new(permissions, report_options, true)
+    OpenFoodNetwork::OrdersAndFulfillmentsReport.new(current_user, report_options, true)
   end
 
   let(:report_table) do
diff --git a/spec/lib/open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report_spec.rb b/spec/lib/open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report_spec.rb
index 4b0b721caf..feb2903014 100644
--- a/spec/lib/open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report_spec.rb
+++ b/spec/lib/open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report_spec.rb
@@ -8,11 +8,10 @@ RSpec.describe OpenFoodNetwork::OrdersAndFulfillmentsReport::DistributorTotalsBy
   end
 
   let(:current_user) { distributor.owner }
-  let(:permissions) { OpenFoodNetwork::Permissions.new(current_user) }
 
   let(:report) do
     report_options = { report_type: described_class::REPORT_TYPE }
-    OpenFoodNetwork::OrdersAndFulfillmentsReport.new(permissions, report_options, true)
+    OpenFoodNetwork::OrdersAndFulfillmentsReport.new(current_user, report_options, true)
   end
 
   let(:report_table) do
diff --git a/spec/lib/open_food_network/orders_and_fulfillments_report/supplier_totals_by_distributor_report_spec.rb b/spec/lib/open_food_network/orders_and_fulfillments_report/supplier_totals_by_distributor_report_spec.rb
index f22ec80bae..654a1ed901 100644
--- a/spec/lib/open_food_network/orders_and_fulfillments_report/supplier_totals_by_distributor_report_spec.rb
+++ b/spec/lib/open_food_network/orders_and_fulfillments_report/supplier_totals_by_distributor_report_spec.rb
@@ -8,11 +8,10 @@ RSpec.describe OpenFoodNetwork::OrdersAndFulfillmentsReport::SupplierTotalsByDis
   end
 
   let(:current_user) { distributor.owner }
-  let(:permissions) { OpenFoodNetwork::Permissions.new(current_user) }
 
   let(:report) do
     report_options = { report_type: described_class::REPORT_TYPE }
-    OpenFoodNetwork::OrdersAndFulfillmentsReport.new(permissions, report_options, true)
+    OpenFoodNetwork::OrdersAndFulfillmentsReport.new(current_user, report_options, true)
   end
 
   let(:report_table) do
diff --git a/spec/lib/open_food_network/orders_and_fulfillments_report/supplier_totals_report_spec.rb b/spec/lib/open_food_network/orders_and_fulfillments_report/supplier_totals_report_spec.rb
index 143bc953e5..21d886cbb0 100644
--- a/spec/lib/open_food_network/orders_and_fulfillments_report/supplier_totals_report_spec.rb
+++ b/spec/lib/open_food_network/orders_and_fulfillments_report/supplier_totals_report_spec.rb
@@ -8,11 +8,10 @@ RSpec.describe OpenFoodNetwork::OrdersAndFulfillmentsReport::SupplierTotalsRepor
   end
 
   let(:current_user) { distributor.owner }
-  let(:permissions) { OpenFoodNetwork::Permissions.new(current_user) }
 
   let(:report) do
     report_options = { report_type: described_class::REPORT_TYPE }
-    OpenFoodNetwork::OrdersAndFulfillmentsReport.new(permissions, report_options, true)
+    OpenFoodNetwork::OrdersAndFulfillmentsReport.new(current_user, report_options, true)
   end
 
   let(:report_table) do
diff --git a/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb b/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb
index ce9a90ce29..62bf00593e 100644
--- a/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb
+++ b/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb
@@ -23,7 +23,7 @@ describe OpenFoodNetwork::OrdersAndFulfillmentsReport do
     before { order.line_items << line_item }
 
     context "as a site admin" do
-      subject { described_class.new OpenFoodNetwork::Permissions.new(admin_user), {}, true }
+      subject { described_class.new(admin_user, {}, true) }
 
       it "fetches completed orders" do
         o2 = create(:order)
@@ -39,7 +39,7 @@ describe OpenFoodNetwork::OrdersAndFulfillmentsReport do
     end
 
     context "as a manager of a supplier" do
-      subject { described_class.new OpenFoodNetwork::Permissions.new(user), {}, true }
+      subject { described_class.new(user, {}, true) }
 
       let(:s1) { create(:supplier_enterprise) }
 
@@ -98,7 +98,7 @@ describe OpenFoodNetwork::OrdersAndFulfillmentsReport do
     end
 
     context "as a manager of a distributor" do
-      subject { described_class.new OpenFoodNetwork::Permissions.new(user), {}, true }
+      subject { described_class.new(user, {}, true) }
 
       before do
         distributor.enterprise_roles.create!(user: user)
@@ -133,8 +133,7 @@ describe OpenFoodNetwork::OrdersAndFulfillmentsReport do
       ]
 
       report_types.each do |report_type|
-        report = described_class.new OpenFoodNetwork::Permissions.new(admin_user),
-                                     report_type: report_type
+        report = described_class.new(admin_user, report_type: report_type)
         expect(report.header.size).to eq(report.columns.size)
       end
     end
@@ -150,9 +149,7 @@ describe OpenFoodNetwork::OrdersAndFulfillmentsReport do
     end
 
     let(:items) {
-      report = described_class.new(OpenFoodNetwork::Permissions.new(admin_user),
-                                   { report_type: "order_cycle_customer_totals" },
-                                   true)
+      report = described_class.new(admin_user, { report_type: "order_cycle_customer_totals" }, true)
       OpenFoodNetwork::OrderGrouper.new(report.rules, report.columns).table(report.table_items)
     }
 

From beaa8ffa273d524d44ceff0b2784d1891a434d19 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 11:45:22 +0000
Subject: [PATCH 09/13] Use more specific selector to avoid ambigous column
 error

---
 lib/open_food_network/permissions.rb | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/open_food_network/permissions.rb b/lib/open_food_network/permissions.rb
index 57127e7247..fe132a9be2 100644
--- a/lib/open_food_network/permissions.rb
+++ b/lib/open_food_network/permissions.rb
@@ -90,14 +90,16 @@ module OpenFoodNetwork
     end
 
     def editable_schedules
-      Schedule.joins(:order_cycles).
-        where(order_cycles: { id: OrderCycle.managed_by(@user).select(:id) }).
+      Schedule.
+        joins(:order_cycles).
+        where(order_cycles: { id: OrderCycle.managed_by(@user).select("order_cycles.id") }).
         select("DISTINCT schedules.*")
     end
 
     def visible_schedules
-      Schedule.joins(:order_cycles).
-        where(order_cycles: { id: OrderCycle.accessible_by(@user).select(:id) }).
+      Schedule.
+        joins(:order_cycles).
+        where(order_cycles: { id: OrderCycle.managed_by(@user).select("order_cycles.id") }).
         select("DISTINCT schedules.*")
     end
 

From 3959f16d6532abc77a95e6d714a7c3c3357a0fc3 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 12:22:50 +0000
Subject: [PATCH 10/13] Switch some more references from Permissions to
 Permissions::Order

---
 app/controllers/admin/bulk_line_items_controller.rb   | 5 +++--
 app/services/search_orders.rb                         | 2 +-
 lib/open_food_network/order_and_distributor_report.rb | 2 +-
 lib/open_food_network/sales_tax_report.rb             | 2 +-
 lib/open_food_network/xero_invoices_report.rb         | 2 +-
 5 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/app/controllers/admin/bulk_line_items_controller.rb b/app/controllers/admin/bulk_line_items_controller.rb
index e7b4237652..86a2f539e2 100644
--- a/app/controllers/admin/bulk_line_items_controller.rb
+++ b/app/controllers/admin/bulk_line_items_controller.rb
@@ -5,10 +5,11 @@ module Admin
     def index
       order_params = params[:q].andand.delete :order
 
-      orders = OpenFoodNetwork::Permissions.new(spree_current_user).
+      order_permissions = ::Permissions::Order.new(spree_current_user)
+      orders = order_permissions.
         editable_orders.ransack(order_params).result
 
-      line_items = OpenFoodNetwork::Permissions.new(spree_current_user).
+      line_items = order_permissions.
         editable_line_items.where(order_id: orders).
         includes(variant: { option_values: :option_type }).
         ransack(params[:q]).result.
diff --git a/app/services/search_orders.rb b/app/services/search_orders.rb
index 5d4f834cc0..4ff0f717da 100644
--- a/app/services/search_orders.rb
+++ b/app/services/search_orders.rb
@@ -24,7 +24,7 @@ class SearchOrders
   attr_reader :params, :current_user
 
   def fetch_orders
-    @search = OpenFoodNetwork::Permissions.new(current_user).editable_orders.ransack(params[:q])
+    @search = ::Permissions::Order.new(current_user).editable_orders.ransack(params[:q])
 
     return paginated_results if using_pagination?
 
diff --git a/lib/open_food_network/order_and_distributor_report.rb b/lib/open_food_network/order_and_distributor_report.rb
index d0b00d08b0..529cf7a6f9 100644
--- a/lib/open_food_network/order_and_distributor_report.rb
+++ b/lib/open_food_network/order_and_distributor_report.rb
@@ -5,7 +5,7 @@ module OpenFoodNetwork
       @user = user
       @render_table = render_table
 
-      @permissions = OpenFoodNetwork::Permissions.new(user)
+      @permissions = ::Permissions::Order.new(user)
     end
 
     def header
diff --git a/lib/open_food_network/sales_tax_report.rb b/lib/open_food_network/sales_tax_report.rb
index af6ef44bf7..1ce3f860dd 100644
--- a/lib/open_food_network/sales_tax_report.rb
+++ b/lib/open_food_network/sales_tax_report.rb
@@ -34,7 +34,7 @@ module OpenFoodNetwork
     end
 
     def search
-      permissions = OpenFoodNetwork::Permissions.new(user)
+      permissions = ::Permissions::Order.new(user)
       permissions.editable_orders.complete.not_state(:canceled).search(params[:q])
     end
 
diff --git a/lib/open_food_network/xero_invoices_report.rb b/lib/open_food_network/xero_invoices_report.rb
index 36bbdf9fe8..6a05c3e296 100644
--- a/lib/open_food_network/xero_invoices_report.rb
+++ b/lib/open_food_network/xero_invoices_report.rb
@@ -19,7 +19,7 @@ module OpenFoodNetwork
     end
 
     def search
-      permissions = OpenFoodNetwork::Permissions.new(@user)
+      permissions = ::Permissions::Order.new(@user)
       permissions.editable_orders.complete.not_state(:canceled).search(@opts[:q])
     end
 

From cc3368704a43589220cc14ddaecda7368bd78393 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 13:50:08 +0000
Subject: [PATCH 11/13] Fix rubocop issues in reports_controller_decorator and
 in report line_items

---
 .rubocop_manual_todo.yml                      |  2 --
 .../admin/reports_controller_decorator.rb     | 29 ++++++++++++++-----
 lib/open_food_network/reports/line_items.rb   | 19 +++++++++---
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml
index e8dc590f17..1a0ec4404f 100644
--- a/.rubocop_manual_todo.yml
+++ b/.rubocop_manual_todo.yml
@@ -45,7 +45,6 @@ Metrics/LineLength:
   - app/controllers/spree/admin/base_controller_decorator.rb
   - app/controllers/spree/admin/orders_controller_decorator.rb
   - app/controllers/spree/admin/payments_controller_decorator.rb
-  - app/controllers/spree/admin/reports_controller_decorator.rb
   - app/controllers/spree/credit_cards_controller.rb
   - app/controllers/spree/paypal_controller_decorator.rb
   - app/controllers/stripe/callbacks_controller.rb
@@ -121,7 +120,6 @@ Metrics/LineLength:
   - lib/open_food_network/order_cycle_management_report.rb
   - lib/open_food_network/payments_report.rb
   - lib/open_food_network/reports/bulk_coop_allocation_report.rb
-  - lib/open_food_network/reports/line_items.rb
   - lib/open_food_network/sales_tax_report.rb
   - lib/open_food_network/variant_and_line_item_naming.rb
   - lib/open_food_network/xero_invoices_report.rb
diff --git a/app/controllers/spree/admin/reports_controller_decorator.rb b/app/controllers/spree/admin/reports_controller_decorator.rb
index cf0970e45c..848fb8ec0d 100644
--- a/app/controllers/spree/admin/reports_controller_decorator.rb
+++ b/app/controllers/spree/admin/reports_controller_decorator.rb
@@ -23,7 +23,8 @@ Spree::Admin::ReportsController.class_eval do
 
   before_filter :cache_search_state
   # Fetches user's distributors, suppliers and order_cycles
-  before_filter :load_data, only: [:customers, :products_and_inventory, :order_cycle_management, :packing]
+  before_filter :load_data,
+                only: [:customers, :products_and_inventory, :order_cycle_management, :packing]
 
   def report_types
     OpenFoodNetwork::Reports::List.all
@@ -49,7 +50,9 @@ Spree::Admin::ReportsController.class_eval do
     @report_type = params[:report_type]
 
     # -- Build Report with Order Grouper
-    @report = OpenFoodNetwork::OrderCycleManagementReport.new spree_current_user, params, render_content?
+    @report = OpenFoodNetwork::OrderCycleManagementReport.new spree_current_user,
+                                                              params,
+                                                              render_content?
     @table = @report.table_items
 
     render_report(@report.header, @table, params[:csv], "order_cycle_management_#{timestamp}.csv")
@@ -69,7 +72,9 @@ Spree::Admin::ReportsController.class_eval do
   end
 
   def orders_and_distributors
-    @report = OpenFoodNetwork::OrderAndDistributorReport.new spree_current_user, params, render_content?
+    @report = OpenFoodNetwork::OrderAndDistributorReport.new spree_current_user,
+                                                             params,
+                                                             render_content?
     @search = @report.search
     csv_file_name = "orders_and_distributors_#{timestamp}.csv"
     render_report(@report.header, @report.table, params[:csv], csv_file_name)
@@ -126,7 +131,9 @@ Spree::Admin::ReportsController.class_eval do
     @include_blank = I18n.t(:all)
 
     # -- Build Report with Order Grouper
-    @report = OpenFoodNetwork::OrdersAndFulfillmentsReport.new(spree_current_user, params, render_content?)
+    @report = OpenFoodNetwork::OrdersAndFulfillmentsReport.new spree_current_user,
+                                                               params,
+                                                               render_content?
     @table = order_grouper_table
     csv_file_name = "#{params[:report_type]}_#{timestamp}.csv"
 
@@ -136,16 +143,24 @@ Spree::Admin::ReportsController.class_eval do
   def products_and_inventory
     @report_types = report_types[:products_and_inventory]
     @report = if params[:report_type] != 'lettuce_share'
-                OpenFoodNetwork::ProductsAndInventoryReport.new spree_current_user, params, render_content?
+                OpenFoodNetwork::ProductsAndInventoryReport.new spree_current_user,
+                                                                params,
+                                                                render_content?
               else
                 OpenFoodNetwork::LettuceShareReport.new spree_current_user, params, render_content?
               end
-    render_report(@report.header, @report.table, params[:csv], "products_and_inventory_#{timestamp}.csv")
+    render_report(@report.header,
+                  @report.table,
+                  params[:csv],
+                  "products_and_inventory_#{timestamp}.csv")
   end
 
   def users_and_enterprises
     @report = OpenFoodNetwork::UsersAndEnterprisesReport.new params, render_content?
-    render_report(@report.header, @report.table, params[:csv], "users_and_enterprises_#{timestamp}.csv")
+    render_report(@report.header,
+                  @report.table,
+                  params[:csv],
+                  "users_and_enterprises_#{timestamp}.csv")
   end
 
   def xero_invoices
diff --git a/lib/open_food_network/reports/line_items.rb b/lib/open_food_network/reports/line_items.rb
index a8209056b5..30e86f21e2 100644
--- a/lib/open_food_network/reports/line_items.rb
+++ b/lib/open_food_network/reports/line_items.rb
@@ -9,8 +9,13 @@ module OpenFoodNetwork
       def self.list(order_permissions, params)
         orders = search_orders(order_permissions, params).result
 
-        line_items = order_permissions.visible_line_items.merge(Spree::LineItem.where(order_id: orders))
-        line_items = line_items.supplied_by_any(params[:supplier_id_in]) if params[:supplier_id_in].present?
+        line_items = order_permissions.
+          visible_line_items.
+          merge(Spree::LineItem.where(order_id: orders))
+
+        if params[:supplier_id_in].present?
+          line_items = line_items.supplied_by_any(params[:supplier_id_in])
+        end
 
         if params[:line_item_includes].present?
           line_items = line_items.includes(*params[:line_item_includes])
@@ -23,8 +28,14 @@ module OpenFoodNetwork
         }.each do |line_item|
           # 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
-          line_item.order.bill_address.andand.assign_attributes(firstname: I18n.t('admin.reports.hidden'), lastname: "", phone: "", address1: "", address2: "", city: "", zipcode: "", state: nil)
-          line_item.order.ship_address.andand.assign_attributes(firstname: I18n.t('admin.reports.hidden'), lastname: "", phone: "", address1: "", address2: "", city: "", zipcode: "", state: nil)
+          line_item.order.bill_address.andand.
+            assign_attributes(firstname: I18n.t('admin.reports.hidden'),
+                              lastname: "", phone: "", address1: "", address2: "",
+                              city: "", zipcode: "", state: nil)
+          line_item.order.ship_address.andand.
+            assign_attributes(firstname: I18n.t('admin.reports.hidden'),
+                              lastname: "", phone: "", address1: "", address2: "",
+                              city: "", zipcode: "", state: nil)
           line_item.order.assign_attributes(email: I18n.t('admin.reports.hidden'))
         end
         line_items

From 0ef424791445e438cdf431fabc90c3faf7c11d26 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 21:51:54 +0000
Subject: [PATCH 12/13] Convert Report::LineItems to class and memoize orders
 so it's only executed once (this improves the report in 3secs for the case I
 am testing)

---
 lib/open_food_network/bulk_coop_report.rb     | 12 +++---
 .../orders_and_fulfillments_report.rb         | 10 +++--
 lib/open_food_network/packing_report.rb       | 12 +++---
 lib/open_food_network/reports/line_items.rb   | 38 ++++++++++++-------
 4 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/lib/open_food_network/bulk_coop_report.rb b/lib/open_food_network/bulk_coop_report.rb
index fcaa3afbb8..e4a89dbbe5 100644
--- a/lib/open_food_network/bulk_coop_report.rb
+++ b/lib/open_food_network/bulk_coop_report.rb
@@ -46,12 +46,12 @@ module OpenFoodNetwork
     end
 
     def search
-      Reports::LineItems.search_orders(order_permissions, params)
+      report_line_items.orders
     end
 
     def table_items
       return [] unless @render_table
-      Reports::LineItems.list(order_permissions, report_options)
+      report_line_items.list(line_item_includes)
     end
 
     def rules
@@ -121,10 +121,6 @@ module OpenFoodNetwork
 
     private
 
-    def report_options
-      @params.merge(line_item_includes: line_item_includes)
-    end
-
     def line_item_includes
       [{ order: [:bill_address],
          variant: [{ option_values: :option_type }, { product: :supplier }] }]
@@ -134,5 +130,9 @@ module OpenFoodNetwork
       return @order_permissions unless @order_permissions.nil?
       @order_permissions = ::Permissions::Order.new(@user)
     end
+
+    def report_line_items
+      @report_line_items ||= Reports::LineItems.new(order_permissions, @params)
+    end
   end
 end
diff --git a/lib/open_food_network/orders_and_fulfillments_report.rb b/lib/open_food_network/orders_and_fulfillments_report.rb
index dbecd1fc29..45d36b2a80 100644
--- a/lib/open_food_network/orders_and_fulfillments_report.rb
+++ b/lib/open_food_network/orders_and_fulfillments_report.rb
@@ -21,14 +21,12 @@ module OpenFoodNetwork
     end
 
     def search
-      Reports::LineItems.search_orders(order_permissions, options)
+      report_line_items.orders
     end
 
     def table_items
       return [] unless @render_table
-
-      list_options = options.merge(line_item_includes: report.line_item_includes)
-      Reports::LineItems.list(order_permissions, list_options)
+      report_line_items.list(report.line_item_includes)
     end
 
     private
@@ -87,5 +85,9 @@ module OpenFoodNetwork
       return @order_permissions unless @order_permissions.nil?
       @order_permissions = ::Permissions::Order.new(@user)
     end
+
+    def report_line_items
+      @report_line_items ||= Reports::LineItems.new(order_permissions, options)
+    end
   end
 end
diff --git a/lib/open_food_network/packing_report.rb b/lib/open_food_network/packing_report.rb
index 4d07c91b40..997059a666 100644
--- a/lib/open_food_network/packing_report.rb
+++ b/lib/open_food_network/packing_report.rb
@@ -38,12 +38,12 @@ module OpenFoodNetwork
     end
 
     def search
-      Reports::LineItems.search_orders(order_permissions, params)
+      report_line_items.orders
     end
 
     def table_items
       return [] unless @render_table
-      Reports::LineItems.list(order_permissions, report_options)
+      report_line_items.list(line_item_includes)
     end
 
     def rules
@@ -120,10 +120,6 @@ module OpenFoodNetwork
 
     private
 
-    def report_options
-      @params.merge(line_item_includes: line_item_includes)
-    end
-
     def line_item_includes
       [{ order: [:bill_address, :distributor],
          variant: [{ option_values: :option_type }, { product: :supplier }] }]
@@ -150,5 +146,9 @@ module OpenFoodNetwork
       customer = Customer.where(email: email).first
       customer.nil? ? "" : customer.code
     end
+
+    def report_line_items
+      @report_line_items ||= Reports::LineItems.new(order_permissions, @params)
+    end
   end
 end
diff --git a/lib/open_food_network/reports/line_items.rb b/lib/open_food_network/reports/line_items.rb
index 30e86f21e2..6a0c320dbf 100644
--- a/lib/open_food_network/reports/line_items.rb
+++ b/lib/open_food_network/reports/line_items.rb
@@ -1,27 +1,30 @@
 module OpenFoodNetwork
   module Reports
     # shared code to search and list line items
-    module LineItems
-      def self.search_orders(order_permissions, params)
-        order_permissions.visible_orders.complete.not_state(:canceled).search(params[:q])
+    class LineItems
+      def initialize(order_permissions, params)
+        @order_permissions = order_permissions
+        @params = params
       end
 
-      def self.list(order_permissions, params)
-        orders = search_orders(order_permissions, params).result
+      def orders
+        @orders ||= search_orders
+      end
 
-        line_items = order_permissions.
+      def list(line_item_includes = nil)
+        line_items = @order_permissions.
           visible_line_items.
-          merge(Spree::LineItem.where(order_id: orders))
+          merge(Spree::LineItem.where(order_id: orders.result))
 
-        if params[:supplier_id_in].present?
-          line_items = line_items.supplied_by_any(params[:supplier_id_in])
+        if @params[:supplier_id_in].present?
+          line_items = line_items.supplied_by_any(@params[:supplier_id_in])
         end
 
-        if params[:line_item_includes].present?
-          line_items = line_items.includes(*params[:line_item_includes])
+        if line_item_includes.present?
+          line_items = line_items.includes(*line_item_includes)
         end
 
-        hidden_line_items = line_items_with_hidden_details(order_permissions, line_items)
+        hidden_line_items = line_items_with_hidden_details(line_items)
 
         line_items.select{ |li|
           hidden_line_items.include? li
@@ -38,11 +41,18 @@ module OpenFoodNetwork
                               city: "", zipcode: "", state: nil)
           line_item.order.assign_attributes(email: I18n.t('admin.reports.hidden'))
         end
+
         line_items
       end
 
-      def self.line_items_with_hidden_details(order_permissions, line_items)
-        editable_line_items = order_permissions.editable_line_items.pluck(:id)
+      private
+
+      def search_orders
+        @order_permissions.visible_orders.complete.not_state(:canceled).search(@params[:q])
+      end
+
+      def line_items_with_hidden_details(line_items)
+        editable_line_items = @order_permissions.editable_line_items.pluck(:id)
 
         if editable_line_items.empty?
           line_items

From 1e948735fb0573002fd974e04085a058732358f9 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 29 Nov 2019 23:10:39 +0000
Subject: [PATCH 13/13] Fix major performance problem by inverting the logic,
 instead of looking for line_items that are hidden, it looks for line items
 that are not editable using a merge statement that performs much better

Also, remove unnecessary if clause, merge will return an empty relation if no items are found, no need to test for empty.

The test report runs in a little over one minute instead of 8minutes
---
 lib/open_food_network/reports/line_items.rb   | 20 ++++++++++---------
 .../bulk_coop_report_spec.rb                  |  2 +-
 .../orders_and_fulfillments_report_spec.rb    |  2 +-
 .../open_food_network/packing_report_spec.rb  |  2 +-
 4 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/lib/open_food_network/reports/line_items.rb b/lib/open_food_network/reports/line_items.rb
index 6a0c320dbf..4e272a7c13 100644
--- a/lib/open_food_network/reports/line_items.rb
+++ b/lib/open_food_network/reports/line_items.rb
@@ -24,10 +24,10 @@ module OpenFoodNetwork
           line_items = line_items.includes(*line_item_includes)
         end
 
-        hidden_line_items = line_items_with_hidden_details(line_items)
+        editable_line_items = editable_line_items(line_items)
 
         line_items.select{ |li|
-          hidden_line_items.include? li
+          !editable_line_items.include? li
         }.each do |line_item|
           # 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
@@ -51,14 +51,16 @@ module OpenFoodNetwork
         @order_permissions.visible_orders.complete.not_state(:canceled).search(@params[:q])
       end
 
-      def line_items_with_hidden_details(line_items)
-        editable_line_items = @order_permissions.editable_line_items.pluck(:id)
+      # From the line_items given, returns the ones that are editable by the user
+      def editable_line_items(line_items)
+        editable_line_items_ids = @order_permissions.editable_line_items.select(:id)
 
-        if editable_line_items.empty?
-          line_items
-        else
-          line_items.where('"spree_line_items"."id" NOT IN (?)', editable_line_items)
-        end
+        # Although merge could take a relation, here we convert line_items to array
+        #   because, if we pass a relation, merge will overwrite the conditions on the same field
+        #   In this case: the IN clause on spree_line_items.order_id from line_items
+        #     overwrites the IN clause on spree_line_items.order_id on editable_line_items_ids
+        # We convert to array the relation with less elements: line_items
+        editable_line_items_ids.merge(line_items.to_a)
       end
     end
   end
diff --git a/spec/lib/open_food_network/bulk_coop_report_spec.rb b/spec/lib/open_food_network/bulk_coop_report_spec.rb
index 4eeeae09dd..2d42928414 100644
--- a/spec/lib/open_food_network/bulk_coop_report_spec.rb
+++ b/spec/lib/open_food_network/bulk_coop_report_spec.rb
@@ -62,7 +62,7 @@ module OpenFoodNetwork
             o2.line_items << li2
           end
 
-          it "shows line items supplied by my producers, with names hidden" do
+          it "does not show line items supplied by my producers" do
             expect(subject.table_items).to eq([])
           end
         end
diff --git a/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb b/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb
index 62bf00593e..210392d380 100644
--- a/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb
+++ b/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb
@@ -91,7 +91,7 @@ describe OpenFoodNetwork::OrdersAndFulfillmentsReport do
           o2.line_items << li2
         end
 
-        it "shows line items supplied by my producers, with names hidden" do
+        it "does not show line items supplied by my producers" do
           expect(subject.table_items).to eq([])
         end
       end
diff --git a/spec/lib/open_food_network/packing_report_spec.rb b/spec/lib/open_food_network/packing_report_spec.rb
index 6bbb1b2687..1c39d24b6a 100644
--- a/spec/lib/open_food_network/packing_report_spec.rb
+++ b/spec/lib/open_food_network/packing_report_spec.rb
@@ -62,7 +62,7 @@ module OpenFoodNetwork
             o2.line_items << li2
           end
 
-          it "shows line items supplied by my producers, with names hidden" do
+          it "does not show line items supplied by my producers" do
             expect(subject.table_items).to eq([])
           end
         end