From caf1c9ecd94967560da17a4feceed211f1a60cb1 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 13 Jan 2021 16:35:04 +0100
Subject: [PATCH 01/13] Move data fetching from injection helper to action

Data fetching is a controller action responsibility. We shouldn't couple
the controller with it too much but it should trigger it, not the
view-layer.
---
 app/controllers/spree/users_controller.rb | 5 +++++
 app/views/spree/users/show.html.haml      | 5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/app/controllers/spree/users_controller.rb b/app/controllers/spree/users_controller.rb
index 1fb03af1f92..5be52a09102 100644
--- a/app/controllers/spree/users_controller.rb
+++ b/app/controllers/spree/users_controller.rb
@@ -15,6 +15,11 @@ class UsersController < ::BaseController
     # Ignores invoice orders, only order where state: 'complete'
     def show
       @orders = @user.orders.where(state: 'complete').order('completed_at desc')
+
+      customers = spree_current_user.customers
+      @shops = Enterprise
+        .where(id: @orders.pluck(:distributor_id).uniq | customers.pluck(:enterprise_id))
+
       @unconfirmed_email = spree_current_user.unconfirmed_email
     end
 
diff --git a/app/views/spree/users/show.html.haml b/app/views/spree/users/show.html.haml
index bddce94ce9d..ec816851569 100644
--- a/app/views/spree/users/show.html.haml
+++ b/app/views/spree/users/show.html.haml
@@ -1,7 +1,8 @@
 - content_for :injection_data do
-  = inject_orders
-  = inject_shops
+  = inject_json_array("orders", @orders.all, Api::OrderSerializer)
+  = inject_json_array("shops", @shops.all, Api::ShopForOrdersSerializer)
   = inject_saved_credit_cards
+
   - if Stripe.publishable_key
     :javascript
       angular.module('Darkswarm').value("stripeObject", Stripe("#{Stripe.publishable_key}"))

From 681a009eb61232fe671d680b7c1780d814209175 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 13 Jan 2021 16:37:43 +0100
Subject: [PATCH 02/13] Extract outstanding balance SQL CASE/WHEN

---
 app/controllers/spree/users_controller.rb |  8 +++++--
 app/models/spree/order.rb                 |  8 +++++++
 app/queries/outstanding_balance.rb        | 27 +++++++++++++++++++++++
 app/services/customers_with_balance.rb    |  6 ++---
 4 files changed, 44 insertions(+), 5 deletions(-)
 create mode 100644 app/queries/outstanding_balance.rb

diff --git a/app/controllers/spree/users_controller.rb b/app/controllers/spree/users_controller.rb
index 5be52a09102..a4029d33cb3 100644
--- a/app/controllers/spree/users_controller.rb
+++ b/app/controllers/spree/users_controller.rb
@@ -12,9 +12,13 @@ class UsersController < ::BaseController
     before_action :set_locale
     before_action :enable_embedded_shopfront
 
-    # Ignores invoice orders, only order where state: 'complete'
+    # Ignores invoice orders
     def show
-      @orders = @user.orders.where(state: 'complete').order('completed_at desc')
+      @orders = @user.orders
+        .where.not(Spree::Order.in_incomplete_state.where_values_hash)
+        .select('spree_orders.*')
+        .select(OutstandingBalance.new.query)
+        .order('completed_at desc')
 
       customers = spree_current_user.customers
       @shops = Enterprise
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 07cd78dac76..ce279ec58b4 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -130,6 +130,14 @@ def states
       where("state != ?", state)
     }
 
+    # All the states an order can be in before completing the checkout
+    PRIOR_TO_COMPLETION_STATES = %w(cart address delivery payment).freeze
+    # All the states of a complete order but that shouldn't count towards the balance. Those that the
+    # customer won't enjoy.
+    NON_FULFILLED_STATES = %w(canceled returned).freeze
+
+    scope :in_incomplete_state, -> { where(state: PRIOR_TO_COMPLETION_STATES) }
+
     def self.by_number(number)
       where(number: number)
     end
diff --git a/app/queries/outstanding_balance.rb b/app/queries/outstanding_balance.rb
new file mode 100644
index 00000000000..cc9ba7ab588
--- /dev/null
+++ b/app/queries/outstanding_balance.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+# Encapsulates the SQL statement that computes the balance of an order as a new column in the result
+# set. This can then be reused chaining it with the ActiveRecord::Relation objects you pass in the
+# constructor.
+#
+# Alternatively, you can get the SQL by calling #statement, which is suitable for more complex
+# cases.
+#
+# See CompleteOrdersWithBalance or CustomersWithBalance as examples.
+class OutstandingBalance
+  def query
+    <<-SQL.strip_heredoc
+      CASE WHEN state IN #{non_fulfilled_states_group.to_sql} THEN payment_total
+            WHEN state IS NOT NULL THEN payment_total - total
+      ELSE 0 END
+        AS balance_value
+    SQL
+  end
+
+  private
+
+  def non_fulfilled_states_group
+    states = Spree::Order::NON_FULFILLED_STATES.map { |state| Arel::Nodes.build_quoted(state) }
+    Arel::Nodes::Grouping.new(states)
+  end
+end
diff --git a/app/services/customers_with_balance.rb b/app/services/customers_with_balance.rb
index 56a3c2d1717..5548fa45313 100644
--- a/app/services/customers_with_balance.rb
+++ b/app/services/customers_with_balance.rb
@@ -39,12 +39,12 @@ def left_join_complete_orders
   end
 
   def complete_orders
-    states_group = prior_to_completion_states.map { |state| Arel::Nodes.build_quoted(state) }
-    Arel::Nodes::NotIn.new(Spree::Order.arel_table[:state], states_group)
+    states = Spree::Order::PRIOR_TO_COMPLETION_STATES.map { |state| Arel::Nodes.build_quoted(state) }
+    Arel::Nodes::NotIn.new(Spree::Order.arel_table[:state], states)
   end
 
   def non_fulfilled_states_group
-    states_group = non_fulfilled_states.map { |state| Arel::Nodes.build_quoted(state) }
+    states_group = Spree::Order::NON_FULFILLED_STATES.map { |state| Arel::Nodes.build_quoted(state) }
     Arel::Nodes::Grouping.new(states_group)
   end
 

From 20abaaa950cd3afb8211b19d8661d286f47cd8db Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 13 Jan 2021 14:28:30 +0100
Subject: [PATCH 03/13] Reuse outstanding balance statement across queries

Instead of relying on Spree::Order#outstanding_balance we make us of the
result set `balance_value` computed column. So, we ask PostgreSQL to
compute it instead of Ruby and then serialize it from that computed
column. That's a bit faster to compute that way and let's reuse logic.

We hide this new implementation under this features' toggle so it's only
used when enabled. We want hit the old behaviour by default.
---
 app/controllers/spree/users_controller.rb     |  3 +-
 app/models/spree/order.rb                     |  4 +--
 app/queries/outstanding_balance.rb            | 17 +++++++++--
 app/serializers/api/order_serializer.rb       |  8 +++++
 app/services/customers_with_balance.rb        | 30 ++-----------------
 spec/serializers/api/order_serializer_spec.rb | 26 ++++++++++++++++
 6 files changed, 56 insertions(+), 32 deletions(-)

diff --git a/app/controllers/spree/users_controller.rb b/app/controllers/spree/users_controller.rb
index a4029d33cb3..7d46be6748c 100644
--- a/app/controllers/spree/users_controller.rb
+++ b/app/controllers/spree/users_controller.rb
@@ -17,9 +17,10 @@ def show
       @orders = @user.orders
         .where.not(Spree::Order.in_incomplete_state.where_values_hash)
         .select('spree_orders.*')
-        .select(OutstandingBalance.new.query)
         .order('completed_at desc')
 
+      @orders = OutstandingBalance.new(@orders).query
+
       customers = spree_current_user.customers
       @shops = Enterprise
         .where(id: @orders.pluck(:distributor_id).uniq | customers.pluck(:enterprise_id))
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index ce279ec58b4..a908c56e1e3 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -132,8 +132,8 @@ def states
 
     # All the states an order can be in before completing the checkout
     PRIOR_TO_COMPLETION_STATES = %w(cart address delivery payment).freeze
-    # All the states of a complete order but that shouldn't count towards the balance. Those that the
-    # customer won't enjoy.
+    # All the states of a complete order but that shouldn't count towards the balance. Those that
+    # the customer won't enjoy.
     NON_FULFILLED_STATES = %w(canceled returned).freeze
 
     scope :in_incomplete_state, -> { where(state: PRIOR_TO_COMPLETION_STATES) }
diff --git a/app/queries/outstanding_balance.rb b/app/queries/outstanding_balance.rb
index cc9ba7ab588..a31bb1b2dfb 100644
--- a/app/queries/outstanding_balance.rb
+++ b/app/queries/outstanding_balance.rb
@@ -9,17 +9,30 @@
 #
 # See CompleteOrdersWithBalance or CustomersWithBalance as examples.
 class OutstandingBalance
+  # The relation must be an ActiveRecord::Relation object with `spree_orders` in the SQL statement
+  # FROM for #statement to work.
+  def initialize(relation = nil)
+    @relation = relation
+  end
+
   def query
+    relation.select("#{statement} AS balance_value")
+  end
+
+  # Arel doesn't support CASE statements until v7.1.0 so we'll have to wait with SQL literals
+  # a little longer. See https://github.com/rails/arel/pull/400 for details.
+  def statement
     <<-SQL.strip_heredoc
       CASE WHEN state IN #{non_fulfilled_states_group.to_sql} THEN payment_total
-            WHEN state IS NOT NULL THEN payment_total - total
+           WHEN state IS NOT NULL THEN payment_total - total
       ELSE 0 END
-        AS balance_value
     SQL
   end
 
   private
 
+  attr_reader :relation
+
   def non_fulfilled_states_group
     states = Spree::Order::NON_FULFILLED_STATES.map { |state| Arel::Nodes.build_quoted(state) }
     Arel::Nodes::Grouping.new(states)
diff --git a/app/serializers/api/order_serializer.rb b/app/serializers/api/order_serializer.rb
index 427c222ea21..73532a1704b 100644
--- a/app/serializers/api/order_serializer.rb
+++ b/app/serializers/api/order_serializer.rb
@@ -7,6 +7,14 @@ class OrderSerializer < ActiveModel::Serializer
 
     has_many :payments, serializer: Api::PaymentSerializer
 
+    def outstanding_balance
+      if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, object.user)
+        -object.balance_value
+      else
+        object.outstanding_balance
+      end
+    end
+
     def payments
       object.payments.joins(:payment_method).completed
     end
diff --git a/app/services/customers_with_balance.rb b/app/services/customers_with_balance.rb
index 5548fa45313..bf6cf70396f 100644
--- a/app/services/customers_with_balance.rb
+++ b/app/services/customers_with_balance.rb
@@ -10,23 +10,15 @@ def query
       joins(left_join_complete_orders).
       group("customers.id").
       select("customers.*").
-      select(outstanding_balance)
+      select(outstanding_balance_sum)
   end
 
   private
 
   attr_reader :enterprise
 
-  # Arel doesn't support CASE statements until v7.1.0 so we'll have to wait with SQL literals
-  # a little longer. See https://github.com/rails/arel/pull/400 for details.
-  def outstanding_balance
-    <<-SQL.strip_heredoc
-       SUM(
-         CASE WHEN state IN #{non_fulfilled_states_group.to_sql} THEN payment_total
-              WHEN state IS NOT NULL THEN payment_total - total
-         ELSE 0 END
-       ) AS balance_value
-    SQL
+  def outstanding_balance_sum
+    "SUM(#{OutstandingBalance.new.statement}) AS balance_value"
   end
 
   # The resulting orders are in states that belong after the checkout. Only these can be considered
@@ -42,20 +34,4 @@ def complete_orders
     states = Spree::Order::PRIOR_TO_COMPLETION_STATES.map { |state| Arel::Nodes.build_quoted(state) }
     Arel::Nodes::NotIn.new(Spree::Order.arel_table[:state], states)
   end
-
-  def non_fulfilled_states_group
-    states_group = Spree::Order::NON_FULFILLED_STATES.map { |state| Arel::Nodes.build_quoted(state) }
-    Arel::Nodes::Grouping.new(states_group)
-  end
-
-  # All the states an order can be in before completing the checkout
-  def prior_to_completion_states
-    %w(cart address delivery payment)
-  end
-
-  # All the states of a complete order but that shouldn't count towards the balance. Those that the
-  # customer won't enjoy.
-  def non_fulfilled_states
-    %w(canceled returned)
-  end
 end
diff --git a/spec/serializers/api/order_serializer_spec.rb b/spec/serializers/api/order_serializer_spec.rb
index 04ef728d594..9b32b754e47 100644
--- a/spec/serializers/api/order_serializer_spec.rb
+++ b/spec/serializers/api/order_serializer_spec.rb
@@ -23,4 +23,30 @@
     expect(serializer.to_json).to match completed_payment.amount.to_s
     expect(serializer.to_json).to_not match payment.amount.to_s
   end
+
+  describe '#outstanding_balance' do
+    context 'when the customer_balance feature is enabled' do
+      before do
+        allow(OpenFoodNetwork::FeatureToggle)
+          .to receive(:enabled?).with(:customer_balance, order.user) { true }
+
+        allow(order).to receive(:balance_value).and_return(-1.23)
+      end
+
+      it "returns the object's balance_value from the users perspective" do
+        expect(serializer.serializable_hash[:outstanding_balance]).to eq(1.23)
+      end
+    end
+
+    context 'when the customer_balance is not enabled' do
+      before do
+        allow(OpenFoodNetwork::FeatureToggle)
+          .to receive(:enabled?).with(:customer_balance, order.user) { false }
+      end
+
+      it 'calls #outstanding_balance on the object' do
+        expect(serializer.serializable_hash[:outstanding_balance]).to eq(1.0)
+      end
+    end
+  end
 end

From a124f93b20353b8dd769e8ff18c58d445222c903 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 13 Jan 2021 17:33:50 +0100
Subject: [PATCH 04/13] Remove old commented out code

---
 spec/serializers/api/order_serializer_spec.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/spec/serializers/api/order_serializer_spec.rb b/spec/serializers/api/order_serializer_spec.rb
index 9b32b754e47..bc1115fc90a 100644
--- a/spec/serializers/api/order_serializer_spec.rb
+++ b/spec/serializers/api/order_serializer_spec.rb
@@ -14,7 +14,6 @@
   end
 
   it "convert the state attributes to translatable keys" do
-    # byebug if serializer.to_json =~ /balance_due/
     expect(serializer.to_json).to match "complete"
     expect(serializer.to_json).to match "balance_due"
   end
@@ -42,10 +41,12 @@
       before do
         allow(OpenFoodNetwork::FeatureToggle)
           .to receive(:enabled?).with(:customer_balance, order.user) { false }
+
+        allow(order).to receive(:outstanding_balance).and_return(123.0)
       end
 
       it 'calls #outstanding_balance on the object' do
-        expect(serializer.serializable_hash[:outstanding_balance]).to eq(1.0)
+        expect(serializer.serializable_hash[:outstanding_balance]).to eq(123.0)
       end
     end
   end

From 37b7340eb134bc723783fc2fc56f2a00664f6c4d Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 13 Jan 2021 17:41:16 +0100
Subject: [PATCH 05/13] Refactor specs to speed them up

We don't care about the conversion from hash to JSON (that's an
ActiveModel::Serializer responsibility that is thoroughly tested) but
our logic so we can skip that step which only slows down tests.

It consistently reduced ~1.5s on my machine but it's still too slow to wait
~8.5s to get feedback from them.
---
 spec/serializers/api/order_serializer_spec.rb | 31 ++++++++++++-------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/spec/serializers/api/order_serializer_spec.rb b/spec/serializers/api/order_serializer_spec.rb
index bc1115fc90a..d635cf0ada8 100644
--- a/spec/serializers/api/order_serializer_spec.rb
+++ b/spec/serializers/api/order_serializer_spec.rb
@@ -6,21 +6,28 @@
   let(:serializer) { Api::OrderSerializer.new order }
   let(:order) { create(:completed_order_with_totals) }
 
-  let!(:completed_payment) { create(:payment, order: order, state: 'completed', amount: order.total - 1) }
-  let!(:payment) { create(:payment, order: order, state: 'checkout', amount: 123.45) }
+  describe '#serializable_hash' do
+    let!(:completed_payment) do
+      create(:payment, order: order, state: 'completed', amount: order.total - 1)
+    end
+    let!(:payment) { create(:payment, order: order, state: 'checkout', amount: 123.45) }
 
-  it "serializes an order" do
-    expect(serializer.to_json).to match order.number.to_s
-  end
+    it "serializes an order" do
+      expect(serializer.serializable_hash[:number]).to eq(order.number)
+    end
 
-  it "convert the state attributes to translatable keys" do
-    expect(serializer.to_json).to match "complete"
-    expect(serializer.to_json).to match "balance_due"
-  end
+    it "convert the state attributes to translatable keys" do
+      hash = serializer.serializable_hash
 
-  it "only serializes completed payments" do
-    expect(serializer.to_json).to match completed_payment.amount.to_s
-    expect(serializer.to_json).to_not match payment.amount.to_s
+      expect(hash[:state]).to eq("complete")
+      expect(hash[:payment_state]).to eq("balance_due")
+    end
+
+    it "only serializes completed payments" do
+      hash = serializer.serializable_hash
+
+      expect(hash[:payments].first[:amount]).to eq(completed_payment.amount)
+    end
   end
 
   describe '#outstanding_balance' do

From e8ef4acb2b2fd15d28de8b51312eb6a53ff2c20a Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 13 Jan 2021 19:13:27 +0100
Subject: [PATCH 06/13] Hide new data fetching implementation under toggle

---
 app/controllers/spree/users_controller.rb     | 20 +++++++++++++------
 .../spree/users_controller_spec.rb            | 16 +++++++++++++++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/app/controllers/spree/users_controller.rb b/app/controllers/spree/users_controller.rb
index 7d46be6748c..2f7722c4015 100644
--- a/app/controllers/spree/users_controller.rb
+++ b/app/controllers/spree/users_controller.rb
@@ -14,12 +14,7 @@ class UsersController < ::BaseController
 
     # Ignores invoice orders
     def show
-      @orders = @user.orders
-        .where.not(Spree::Order.in_incomplete_state.where_values_hash)
-        .select('spree_orders.*')
-        .order('completed_at desc')
-
-      @orders = OutstandingBalance.new(@orders).query
+      @orders = orders_collection
 
       customers = spree_current_user.customers
       @shops = Enterprise
@@ -64,6 +59,19 @@ def update
 
     private
 
+    def orders_collection
+      if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, spree_current_user)
+        orders = @user.orders
+          .where.not(Spree::Order.in_incomplete_state.where_values_hash)
+          .select('spree_orders.*')
+          .order('completed_at desc')
+
+        OutstandingBalance.new(orders).query
+      else
+        @user.orders.where(state: 'complete').order('completed_at desc')
+      end
+    end
+
     def load_object
       @user ||= spree_current_user
       if @user
diff --git a/spec/controllers/spree/users_controller_spec.rb b/spec/controllers/spree/users_controller_spec.rb
index 8c5c6b80b1e..8f7b4257c75 100644
--- a/spec/controllers/spree/users_controller_spec.rb
+++ b/spec/controllers/spree/users_controller_spec.rb
@@ -40,6 +40,22 @@
       # Doesn't return uncompleted orders" do
       expect(orders).not_to include d1o3
     end
+
+    context 'when the customer_balance feature is enabled' do
+      let(:outstanding_balance) { double(:outstanding_balance) }
+
+      before do
+        allow(OpenFoodNetwork::FeatureToggle)
+          .to receive(:enabled?).with(:customer_balance, controller.spree_current_user) { true }
+      end
+
+      it 'calls OutstandingBalance' do
+        allow(OutstandingBalance).to receive(:new).and_return(outstanding_balance)
+        expect(outstanding_balance).to receive(:query) { Spree::Order.none }
+
+        spree_get :show
+      end
+    end
   end
 
   describe "registered_email" do

From d18e79ab19a4a40e8c1e54f52ff0ecfdbb2838da Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Thu, 14 Jan 2021 10:47:25 +0100
Subject: [PATCH 07/13] Move query object to app/queries/ and doc it

---
 app/{services => queries}/customers_with_balance.rb       | 2 ++
 spec/{services => queries}/customers_with_balance_spec.rb | 0
 2 files changed, 2 insertions(+)
 rename app/{services => queries}/customers_with_balance.rb (83%)
 rename spec/{services => queries}/customers_with_balance_spec.rb (100%)

diff --git a/app/services/customers_with_balance.rb b/app/queries/customers_with_balance.rb
similarity index 83%
rename from app/services/customers_with_balance.rb
rename to app/queries/customers_with_balance.rb
index bf6cf70396f..2303c286879 100644
--- a/app/services/customers_with_balance.rb
+++ b/app/queries/customers_with_balance.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+# Fetches the customers of the specified enterprise including the aggregated balance across the
+# customer's orders. That is, we get the total balance for each customer with this enterprise.
 class CustomersWithBalance
   def initialize(enterprise)
     @enterprise = enterprise
diff --git a/spec/services/customers_with_balance_spec.rb b/spec/queries/customers_with_balance_spec.rb
similarity index 100%
rename from spec/services/customers_with_balance_spec.rb
rename to spec/queries/customers_with_balance_spec.rb

From 783863056d75779c5332c77384ad39828ea9b025 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Thu, 14 Jan 2021 10:54:28 +0100
Subject: [PATCH 08/13] Extract query object out of UsersController

It improves the overall readability of the code and as a result, things
became easier to manage already.
---
 app/controllers/spree/users_controller.rb     |  7 +--
 app/queries/complete_orders_with_balance.rb   | 21 +++++++
 .../complete_orders_with_balance_spec.rb      | 61 +++++++++++++++++++
 3 files changed, 83 insertions(+), 6 deletions(-)
 create mode 100644 app/queries/complete_orders_with_balance.rb
 create mode 100644 spec/queries/complete_orders_with_balance_spec.rb

diff --git a/app/controllers/spree/users_controller.rb b/app/controllers/spree/users_controller.rb
index 2f7722c4015..1d9931410b3 100644
--- a/app/controllers/spree/users_controller.rb
+++ b/app/controllers/spree/users_controller.rb
@@ -61,12 +61,7 @@ def update
 
     def orders_collection
       if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, spree_current_user)
-        orders = @user.orders
-          .where.not(Spree::Order.in_incomplete_state.where_values_hash)
-          .select('spree_orders.*')
-          .order('completed_at desc')
-
-        OutstandingBalance.new(orders).query
+        CompleteOrdersWithBalance.new(@user).query
       else
         @user.orders.where(state: 'complete').order('completed_at desc')
       end
diff --git a/app/queries/complete_orders_with_balance.rb b/app/queries/complete_orders_with_balance.rb
new file mode 100644
index 00000000000..31e89d69fb3
--- /dev/null
+++ b/app/queries/complete_orders_with_balance.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+# Fetches complete orders of the specified user including their balance as a computed column
+class CompleteOrdersWithBalance
+  def initialize(user)
+    @user = user
+  end
+
+  def query
+    OutstandingBalance.new(sorted_complete_orders).query
+  end
+
+  private
+
+  def sorted_complete_orders
+    @user.orders
+      .where.not(Spree::Order.in_incomplete_state.where_values_hash)
+      .select('spree_orders.*')
+      .order(completed_at: :desc)
+  end
+end
diff --git a/spec/queries/complete_orders_with_balance_spec.rb b/spec/queries/complete_orders_with_balance_spec.rb
new file mode 100644
index 00000000000..9d46feb7daa
--- /dev/null
+++ b/spec/queries/complete_orders_with_balance_spec.rb
@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe CompleteOrdersWithBalance do
+  let(:complete_orders_with_balance) { described_class.new(user) }
+
+  describe '#query' do
+    let(:user) { order.user }
+    let(:outstanding_balance) { instance_double(OutstandingBalance) }
+
+    context 'when the user has complete orders' do
+      let(:order) do
+        create(:order, state: 'complete', total: 2.0, payment_total: 1.0, completed_at: 2.day.ago)
+      end
+      let!(:other_order) do
+        create(
+          :order,
+          user: user,
+          state: 'complete',
+          total: 2.0,
+          payment_total: 1.0,
+          completed_at: 1.days.ago
+        )
+      end
+
+      it 'calls OutstandingBalance#query' do
+        allow(OutstandingBalance).to receive(:new).and_return(outstanding_balance)
+        expect(outstanding_balance).to receive(:query)
+
+        complete_orders_with_balance.query
+      end
+
+      it 'returns complete orders including their balance' do
+        order = complete_orders_with_balance.query.first
+        expect(order[:balance_value]).to eq(-1.0)
+      end
+
+      it 'sorts them by their completed_at with the most recent first' do
+        orders = complete_orders_with_balance.query
+        expect(orders.pluck(:id)).to eq([other_order.id, order.id])
+      end
+    end
+
+    context 'when the user has no complete orders' do
+      let(:order) { create(:order) }
+
+      it 'calls OutstandingBalance' do
+        allow(OutstandingBalance).to receive(:new).and_return(outstanding_balance)
+        expect(outstanding_balance).to receive(:query)
+
+        complete_orders_with_balance.query
+      end
+
+      it 'returns an empty array' do
+        order = complete_orders_with_balance.query
+        expect(order).to be_empty
+      end
+    end
+  end
+end

From db23428832e3df2a5adb255f2183711fc8a0e460 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Mon, 18 Jan 2021 11:06:19 +0100
Subject: [PATCH 09/13] Test OutstandingBalance

This duplicates the scenarios tested for CustomersWithBalance.
---
 spec/queries/outstanding_balance_spec.rb | 197 +++++++++++++++++++++++
 1 file changed, 197 insertions(+)
 create mode 100644 spec/queries/outstanding_balance_spec.rb

diff --git a/spec/queries/outstanding_balance_spec.rb b/spec/queries/outstanding_balance_spec.rb
new file mode 100644
index 00000000000..a662bcccf94
--- /dev/null
+++ b/spec/queries/outstanding_balance_spec.rb
@@ -0,0 +1,197 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe OutstandingBalance do
+  let(:outstanding_balance) { described_class.new(relation) }
+
+  describe '#statement' do
+    let(:relation) { Spree::Order.none }
+
+    it 'returns the CASE statement necessary to compute the order balance' do
+      normalized_sql_statement = normalize(outstanding_balance.statement)
+
+      expect(normalized_sql_statement).to eq(normalize(<<-SQL))
+        CASE WHEN state IN ('canceled', 'returned') THEN payment_total
+             WHEN state IS NOT NULL THEN payment_total - total
+        ELSE 0 END
+      SQL
+    end
+
+    def normalize(sql)
+      sql.strip_heredoc.gsub("\n", '').squeeze(' ')
+    end
+  end
+
+  describe '#query' do
+    let(:relation) { Spree::Order.all }
+    let(:total) { 200.00 }
+    let(:order_total) { 100.00 }
+
+    context 'when orders are in cart state' do
+      before do
+        create(:order, total: order_total, payment_total: 0, state: 'cart')
+        create(:order, total: order_total, payment_total: 0, state: 'cart')
+      end
+
+      it 'returns the order balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(-order_total)
+      end
+    end
+
+    context 'when orders are in address state' do
+      before do
+        create(:order, total: order_total, payment_total: 0, state: 'address')
+        create(:order, total: order_total, payment_total: 50, state: 'address')
+      end
+
+      it 'returns the order balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(-order_total)
+      end
+    end
+
+    context 'when orders are in delivery state' do
+      before do
+        create(:order, total: order_total, payment_total: 0, state: 'delivery')
+        create(:order, total: order_total, payment_total: 50, state: 'delivery')
+      end
+
+      it 'returns the order balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(-order_total)
+      end
+    end
+
+    context 'when orders are in payment state' do
+      before do
+        create(:order, total: order_total, payment_total: 0, state: 'payment')
+        create(:order, total: order_total, payment_total: 50, state: 'payment')
+      end
+
+      it 'returns the order balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(-order_total)
+      end
+    end
+
+    context 'when no orders where paid' do
+      before do
+        order = create(:order, total: order_total, payment_total: 0)
+        order.update_attribute(:state, 'checkout')
+        order = create(:order, total: order_total, payment_total: 0)
+        order.update_attribute(:state, 'checkout')
+      end
+
+      it 'returns the customer balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(-order_total)
+      end
+    end
+
+    context 'when an order was paid' do
+      let(:payment_total) { order_total }
+
+      before do
+        order = create(:order, total: order_total, payment_total: 0)
+        order.update_attribute(:state, 'checkout')
+        order = create(:order, total: order_total, payment_total: payment_total)
+        order.update_attribute(:state, 'checkout')
+      end
+
+      it 'returns the customer balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(payment_total - 200.0)
+      end
+    end
+
+    context 'when an order is canceled' do
+      let(:payment_total) { order_total }
+      let(:non_canceled_orders_total) { order_total }
+
+      before do
+        create(:order, total: order_total, payment_total: order_total, state: 'canceled')
+        order = create(:order, total: order_total, payment_total: 0)
+        order.update_attribute(:state, 'checkout')
+      end
+
+      it 'returns the customer balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(payment_total)
+      end
+    end
+
+    context 'when an order is resumed' do
+      let(:payment_total) { order_total }
+
+      before do
+        order = create(:order, total: order_total, payment_total: 0)
+        order.update_attribute(:state, 'checkout')
+        order = create(:order, total: order_total, payment_total: payment_total)
+        order.update_attribute(:state, 'resumed')
+      end
+
+      it 'returns the customer balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(payment_total - 200.0)
+      end
+    end
+
+    context 'when an order is in payment' do
+      let(:payment_total) { order_total }
+
+      before do
+        order = create(:order, total: order_total, payment_total: 0)
+        order.update_attribute(:state, 'checkout')
+        order = create(:order, total: order_total, payment_total: payment_total)
+        order.update_attribute(:state, 'payment')
+      end
+
+      it 'returns the customer balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(payment_total - 200.0)
+      end
+    end
+
+    context 'when an order is awaiting_return' do
+      let(:payment_total) { order_total }
+
+      before do
+        order = create(:order, total: order_total, payment_total: 0)
+        order.update_attribute(:state, 'checkout')
+        order = create(:order, total: order_total, payment_total: payment_total)
+        order.update_attribute(:state, 'awaiting_return')
+      end
+
+      it 'returns the customer balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(payment_total - 200.0)
+      end
+    end
+
+    context 'when an order is returned' do
+      let(:payment_total) { order_total }
+      let(:non_returned_orders_total) { order_total }
+
+      before do
+        order = create(:order, total: order_total, payment_total: payment_total)
+        order.update_attribute(:state, 'returned')
+        order = create(:order, total: order_total, payment_total: 0)
+        order.update_attribute(:state, 'checkout')
+      end
+
+      it 'returns the customer balance' do
+        order = outstanding_balance.query.first
+        expect(order.balance_value).to eq(payment_total)
+      end
+    end
+
+    context 'when there are no orders' do
+      it 'returns the order balance' do
+        orders = outstanding_balance.query
+        expect(orders).to be_empty
+      end
+    end
+  end
+end

From 9bb49bb59005dc2707ab805690010e8d5d7a3242 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Mon, 18 Jan 2021 17:13:21 +0100
Subject: [PATCH 10/13] Ensure the query the class depends on is called

---
 spec/queries/customers_with_balance_spec.rb | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/spec/queries/customers_with_balance_spec.rb b/spec/queries/customers_with_balance_spec.rb
index fefd1d9090d..7942e360601 100644
--- a/spec/queries/customers_with_balance_spec.rb
+++ b/spec/queries/customers_with_balance_spec.rb
@@ -9,6 +9,14 @@
     let(:customer) { create(:customer) }
     let(:total) { 200.00 }
     let(:order_total) { 100.00 }
+    let(:outstanding_balance) { instance_double(OutstandingBalance) }
+
+    it 'calls CustomersWithBalance#statement' do
+      allow(OutstandingBalance).to receive(:new).and_return(outstanding_balance)
+      expect(outstanding_balance).to receive(:statement)
+
+      customers_with_balance.query
+    end
 
     context 'when orders are in cart state' do
       before do

From 996761da67ec8924daf7d47166ff4c34a368ed74 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Tue, 19 Jan 2021 13:26:32 +0100
Subject: [PATCH 11/13] Fix long line

---
 app/queries/customers_with_balance.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/queries/customers_with_balance.rb b/app/queries/customers_with_balance.rb
index 2303c286879..0d547f6c2cb 100644
--- a/app/queries/customers_with_balance.rb
+++ b/app/queries/customers_with_balance.rb
@@ -33,7 +33,8 @@ def left_join_complete_orders
   end
 
   def complete_orders
-    states = Spree::Order::PRIOR_TO_COMPLETION_STATES.map { |state| Arel::Nodes.build_quoted(state) }
+    states = Spree::Order::PRIOR_TO_COMPLETION_STATES
+      .map { |state| Arel::Nodes.build_quoted(state) }
     Arel::Nodes::NotIn.new(Spree::Order.arel_table[:state], states)
   end
 end

From d9c065a3111a253f192b6af803a92fd4b37dc14d Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 20 Jan 2021 13:08:06 +0100
Subject: [PATCH 12/13] Remove outdated comment

This comment was related to the feature we removed in
https://github.com/openfoodfoundation/openfoodnetwork/pull/3609.
---
 app/controllers/spree/users_controller.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/controllers/spree/users_controller.rb b/app/controllers/spree/users_controller.rb
index 1d9931410b3..c9ee2e15fe3 100644
--- a/app/controllers/spree/users_controller.rb
+++ b/app/controllers/spree/users_controller.rb
@@ -12,7 +12,6 @@ class UsersController < ::BaseController
     before_action :set_locale
     before_action :enable_embedded_shopfront
 
-    # Ignores invoice orders
     def show
       @orders = orders_collection
 

From cc9e3fe69bf14ab34db0930a7eee47b791642180 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Wed, 20 Jan 2021 14:24:59 +0100
Subject: [PATCH 13/13] Replace double negation with proper list of states

We rely now on the exhaustive list of states an order can be in after
checkout. What made this all a bit more messy is that I made up the
"checkout" order state, likely mixing it from the payment model states.

This simplifies things quite a bit and gives meaningful names to things.
---
 app/models/spree/order.rb                   |  9 +++------
 app/queries/complete_orders_with_balance.rb |  6 +++---
 app/queries/customers_with_balance.rb       |  9 ++++-----
 app/queries/outstanding_balance.rb          |  6 +++++-
 spec/queries/customers_with_balance_spec.rb | 18 +++++++++---------
 spec/queries/outstanding_balance_spec.rb    | 18 +++++++++---------
 6 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index a908c56e1e3..ac75ff2fe16 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -130,13 +130,10 @@ def states
       where("state != ?", state)
     }
 
-    # All the states an order can be in before completing the checkout
-    PRIOR_TO_COMPLETION_STATES = %w(cart address delivery payment).freeze
-    # All the states of a complete order but that shouldn't count towards the balance. Those that
-    # the customer won't enjoy.
-    NON_FULFILLED_STATES = %w(canceled returned).freeze
+    # All the states an order can be in after completing the checkout
+    FINALIZED_STATES = %w(complete canceled resumed awaiting_return returned).freeze
 
-    scope :in_incomplete_state, -> { where(state: PRIOR_TO_COMPLETION_STATES) }
+    scope :finalized, -> { where(state: FINALIZED_STATES) }
 
     def self.by_number(number)
       where(number: number)
diff --git a/app/queries/complete_orders_with_balance.rb b/app/queries/complete_orders_with_balance.rb
index 31e89d69fb3..57223e5c6ee 100644
--- a/app/queries/complete_orders_with_balance.rb
+++ b/app/queries/complete_orders_with_balance.rb
@@ -7,14 +7,14 @@ def initialize(user)
   end
 
   def query
-    OutstandingBalance.new(sorted_complete_orders).query
+    OutstandingBalance.new(sorted_finalized_orders).query
   end
 
   private
 
-  def sorted_complete_orders
+  def sorted_finalized_orders
     @user.orders
-      .where.not(Spree::Order.in_incomplete_state.where_values_hash)
+      .finalized
       .select('spree_orders.*')
       .order(completed_at: :desc)
   end
diff --git a/app/queries/customers_with_balance.rb b/app/queries/customers_with_balance.rb
index 0d547f6c2cb..4abd070d34e 100644
--- a/app/queries/customers_with_balance.rb
+++ b/app/queries/customers_with_balance.rb
@@ -28,13 +28,12 @@ def outstanding_balance_sum
   def left_join_complete_orders
     <<-SQL.strip_heredoc
       LEFT JOIN spree_orders ON spree_orders.customer_id = customers.id
-        AND #{complete_orders.to_sql}
+        AND #{finalized_states.to_sql}
     SQL
   end
 
-  def complete_orders
-    states = Spree::Order::PRIOR_TO_COMPLETION_STATES
-      .map { |state| Arel::Nodes.build_quoted(state) }
-    Arel::Nodes::NotIn.new(Spree::Order.arel_table[:state], states)
+  def finalized_states
+    states = Spree::Order::FINALIZED_STATES.map { |state| Arel::Nodes.build_quoted(state) }
+    Arel::Nodes::In.new(Spree::Order.arel_table[:state], states)
   end
 end
diff --git a/app/queries/outstanding_balance.rb b/app/queries/outstanding_balance.rb
index a31bb1b2dfb..ce4d4d78f4c 100644
--- a/app/queries/outstanding_balance.rb
+++ b/app/queries/outstanding_balance.rb
@@ -9,6 +9,10 @@
 #
 # See CompleteOrdersWithBalance or CustomersWithBalance as examples.
 class OutstandingBalance
+  # All the states of a finished order but that shouldn't count towards the balance (the customer
+  # didn't get the order for whatever reason). Note it does not include complete
+  FINALIZED_NON_SUCCESSFUL_STATES = %w(canceled returned).freeze
+
   # The relation must be an ActiveRecord::Relation object with `spree_orders` in the SQL statement
   # FROM for #statement to work.
   def initialize(relation = nil)
@@ -34,7 +38,7 @@ def statement
   attr_reader :relation
 
   def non_fulfilled_states_group
-    states = Spree::Order::NON_FULFILLED_STATES.map { |state| Arel::Nodes.build_quoted(state) }
+    states = FINALIZED_NON_SUCCESSFUL_STATES.map { |state| Arel::Nodes.build_quoted(state) }
     Arel::Nodes::Grouping.new(states)
   end
 end
diff --git a/spec/queries/customers_with_balance_spec.rb b/spec/queries/customers_with_balance_spec.rb
index 7942e360601..479be6b4102 100644
--- a/spec/queries/customers_with_balance_spec.rb
+++ b/spec/queries/customers_with_balance_spec.rb
@@ -69,9 +69,9 @@
     context 'when no orders where paid' do
       before do
         order = create(:order, customer: customer, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, customer: customer, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
       end
 
       it 'returns the customer balance' do
@@ -85,9 +85,9 @@
 
       before do
         order = create(:order, customer: customer, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
       end
 
       it 'returns the customer balance' do
@@ -102,7 +102,7 @@
 
       before do
         order = create(:order, customer: customer, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         create(
           :order,
           customer: customer,
@@ -123,7 +123,7 @@
 
       before do
         order = create(:order, customer: customer, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
         order.update_attribute(:state, 'resumed')
       end
@@ -139,7 +139,7 @@
 
       before do
         order = create(:order, customer: customer, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
         order.update_attribute(:state, 'payment')
       end
@@ -155,7 +155,7 @@
 
       before do
         order = create(:order, customer: customer, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
         order.update_attribute(:state, 'awaiting_return')
       end
@@ -172,7 +172,7 @@
 
       before do
         order = create(:order, customer: customer, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
         order.update_attribute(:state, 'returned')
       end
diff --git a/spec/queries/outstanding_balance_spec.rb b/spec/queries/outstanding_balance_spec.rb
index a662bcccf94..af880528f7c 100644
--- a/spec/queries/outstanding_balance_spec.rb
+++ b/spec/queries/outstanding_balance_spec.rb
@@ -79,9 +79,9 @@ def normalize(sql)
     context 'when no orders where paid' do
       before do
         order = create(:order, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
       end
 
       it 'returns the customer balance' do
@@ -95,9 +95,9 @@ def normalize(sql)
 
       before do
         order = create(:order, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, total: order_total, payment_total: payment_total)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
       end
 
       it 'returns the customer balance' do
@@ -113,7 +113,7 @@ def normalize(sql)
       before do
         create(:order, total: order_total, payment_total: order_total, state: 'canceled')
         order = create(:order, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
       end
 
       it 'returns the customer balance' do
@@ -127,7 +127,7 @@ def normalize(sql)
 
       before do
         order = create(:order, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, total: order_total, payment_total: payment_total)
         order.update_attribute(:state, 'resumed')
       end
@@ -143,7 +143,7 @@ def normalize(sql)
 
       before do
         order = create(:order, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, total: order_total, payment_total: payment_total)
         order.update_attribute(:state, 'payment')
       end
@@ -159,7 +159,7 @@ def normalize(sql)
 
       before do
         order = create(:order, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
         order = create(:order, total: order_total, payment_total: payment_total)
         order.update_attribute(:state, 'awaiting_return')
       end
@@ -178,7 +178,7 @@ def normalize(sql)
         order = create(:order, total: order_total, payment_total: payment_total)
         order.update_attribute(:state, 'returned')
         order = create(:order, total: order_total, payment_total: 0)
-        order.update_attribute(:state, 'checkout')
+        order.update_attribute(:state, 'complete')
       end
 
       it 'returns the customer balance' do