From b495ef0614cd389ff99dc5a016eed38ee4821a21 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Thu, 18 Jan 2018 09:44:48 +0100
Subject: [PATCH 1/2] Add specs for Account#update_balance

---
 spec/models/account_spec.rb | 64 +++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb
index d6f54005a..ba6176fd1 100644
--- a/spec/models/account_spec.rb
+++ b/spec/models/account_spec.rb
@@ -39,4 +39,68 @@
     expect(organization.account.organization).to eq organization
   end
 
+  describe '#update_balance' do
+    let(:account) { member.account }
+
+    context 'when the balance did not change since last balance update' do
+      before do
+        account.movements << Movement.new(amount: 5)
+        account.save
+      end
+
+      it 'updates the account balance' do
+        # a callback in Movement already called #update_balance after creating
+        # the movement above
+        account.update_balance
+        expect(account.balance).to eq(5)
+      end
+
+      it 'does not flag the account' do
+        # a callback in Movement already called #update_balance after creating
+        # the movement above
+        account.update_balance
+        expect(account.flagged).to be_falsy
+      end
+    end
+
+    context 'when the balance changed since last balance update' do
+      context 'and the new balance falls within the limits allowed' do
+        before do
+          account.max_allowed_balance = 100
+          account.min_allowed_balance = 0
+
+          account.movements << Movement.new(amount: 5)
+          account.save
+        end
+
+        it 'updates the account balance' do
+          # a callback in Movement calls #update_balance after creating the
+          # movement above
+          expect(account.balance).to eq(5)
+        end
+
+        it 'does not flag the account' do
+          # a callback in Movement calls #update_balance after creating the
+          # movement above
+          expect(account.flagged).to be_falsy
+        end
+      end
+
+      context 'and the new balance does not fall within the limits allowed' do
+        before do
+          account.max_allowed_balance = 0
+          account.min_allowed_balance = 0
+
+          account.movements << Movement.new(amount: 5)
+          account.save
+        end
+
+        it 'does not flag the account' do
+          # a callback in Movement calls #update_balance after creating the
+          # movement above
+          expect(account.flagged).to be_truthy
+        end
+      end
+    end
+  end
 end

From 36a1063310efcfa18448915ae86351ca184595a6 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Thu, 18 Jan 2018 09:51:08 +0100
Subject: [PATCH 2/2] Refactor and doc Account#update_balance

It extracts compound conditionals, adds some doc and emphasizes problems
that arise from the current implementation such as the fact that
max_allowed_balance it's either Integer or Float.
---
 app/models/account.rb | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/app/models/account.rb b/app/models/account.rb
index 26e0ab9de..973010148 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -13,12 +13,12 @@ class Account < ActiveRecord::Base
 
   before_create :add_organization
 
+  # Denormalizes the account's balance summing the amount in all its movements.
+  # It will flag the account if its balance falls out of the min or max limits.
   def update_balance
     new_balance = movements.sum(:amount)
     self.balance = new_balance
-    if balance_changed?
-      self.flagged = !allowed?(balance)
-    end
+    self.flagged = !within_allowed_limits? if balance_changed?
     save
   end
 
@@ -51,8 +51,34 @@ def add_organization
 
   private
 
-  def allowed?(new_balance)
-    new_balance < (max_allowed_balance || Float::INFINITY) &&
-      new_balance > (min_allowed_balance || -Float::INFINITY)
+  # Checks whether the balance falls within the max and min allowed balance,
+  # none of these included
+  #
+  # @return [Boolean]
+  def within_allowed_limits?
+    less_than_upper_limit? && greater_than_lower_limit?
+  end
+
+  def less_than_upper_limit?
+    balance < max_allowed_balance
+  end
+
+  def greater_than_lower_limit?
+    balance > min_allowed_balance
+  end
+
+  # Returns the maximum allowed balance, falling back to infinite it not set
+  #
+  # @return [Integer | Float]
+  def max_allowed_balance
+    super || Float::INFINITY
+  end
+
+  # Returns the minimum allowed balance, falling back to minus infinite it not
+  # set
+  #
+  # @return [Integer | Float]
+  def min_allowed_balance
+    super || -Float::INFINITY
   end
 end