From 9d409a7412d13a17bb53d3bfb909c0da2e58d82c Mon Sep 17 00:00:00 2001
From: Aniruddha Maru <maru@standard.ai>
Date: Sat, 25 Jul 2020 14:45:40 -0700
Subject: [PATCH] Fix recurring transactions create

- If there's a lot of accounts to calculate balances for, then recurring transactions create page
  doesn't load. Partly because it has to calculate a lot of balances, but partly because the cache
  isn't being used at all because date is `new Date` rather than say, end of month.

Fix: Change Steam balance calculator to always default cache using end of month. Since cache is
'invalidated' upon any edit, there's no reason to use current datetime anywhere its not explicitly
required by user flow.
Fix: Don't calculate balances for revenue / expense accounts since those are unbounded.

Issue: #3597
---
 .../Transaction/ConvertController.php         |   4 +-
 app/Support/Form/AccountForm.php              | 216 +++++-------------
 app/Support/Steam.php                         |  22 +-
 3 files changed, 76 insertions(+), 166 deletions(-)

diff --git a/app/Http/Controllers/Transaction/ConvertController.php b/app/Http/Controllers/Transaction/ConvertController.php
index 42c0cee6a2..ae6556038a 100644
--- a/app/Http/Controllers/Transaction/ConvertController.php
+++ b/app/Http/Controllers/Transaction/ConvertController.php
@@ -260,7 +260,7 @@ class ConvertController extends Controller
         // group accounts:
         /** @var Account $account */
         foreach ($accountList as $account) {
-            $balance  = app('steam')->balance($account, new Carbon);
+            $balance  = app('steam')->balance($account);
             $currency = $repository->getAccountCurrency($account) ?? $defaultCurrency;
             $role     = (string) $repository->getMetaValue($account, 'account_role');
             if ('' === $role) {
@@ -289,7 +289,7 @@ class ConvertController extends Controller
         // group accounts:
         /** @var Account $account */
         foreach ($accountList as $account) {
-            $balance                     = app('steam')->balance($account, new Carbon);
+            $balance                     = app('steam')->balance($account);
             $currency                    = $repository->getAccountCurrency($account) ?? $defaultCurrency;
             $role                        = 'l_' . $account->accountType->type;
             $key                         = (string) trans('firefly.opt_group_' . $role);
diff --git a/app/Support/Form/AccountForm.php b/app/Support/Form/AccountForm.php
index b8e4d73289..112df4317c 100644
--- a/app/Support/Form/AccountForm.php
+++ b/app/Support/Form/AccountForm.php
@@ -26,6 +26,8 @@ namespace FireflyIII\Support\Form;
 
 use FireflyIII\Models\Account;
 use FireflyIII\Models\AccountType;
+use FireflyIII\Repositories\Account\AccountRepositoryInterface;
+use Illuminate\Support\Collection;
 use Log;
 use Throwable;
 
@@ -41,6 +43,46 @@ class AccountForm
 {
     use FormSupport;
 
+    private function getAccountsGrouped(array $types, AccountRepositoryInterface $repository = null): array
+    {
+        if (null === $repository) {
+            $repository = $this->getAccountRepository();
+        }
+        $accountList     = $repository->getActiveAccountsByType($types);
+        $liabilityTypes  = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN,];
+        $balanceTypes    = [AccountType::ASSET, AccountType::DEFAULT, AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN,];
+        $defaultCurrency = app('amount')->getDefaultCurrency();
+        $grouped         = [];
+
+        /** @var Account $account */
+        foreach ($accountList as $account) {
+            $accountWithBalance = $account->name;
+
+            if (in_array($account->accountType->type, $balanceTypes, true)) {
+                $balance            = app('steam')->balance($account);
+                $currency           = $repository->getAccountCurrency($account) ?? $defaultCurrency;
+                $formatted          = app('amount')->formatAnything($currency, $balance, false);
+                $accountWithBalance = sprintf('%s (%s)', $account->name, $formatted);
+            }
+            $role       = (string)$repository->getMetaValue($account, 'account_role');
+            if (in_array($account->accountType->type, $liabilityTypes, true)) {
+                $role = sprintf('l_%s', $account->accountType->type);
+            } elseif ('' === $role) {
+                if (AccountType::EXPENSE === $account->accountType->type) {
+                    $role = 'expense_account';
+                } elseif (AccountType::REVENUE === $account->accountType->type) {
+                    $role = 'revenue_account';
+                } else {
+                    $role = 'no_account_type';
+                }
+            }
+            $key                         = (string)trans(sprintf('firefly.opt_group_%s', $role));
+            $grouped[$key][$account->id] = $accountWithBalance;
+        }
+
+        return $grouped;
+    }
+
     /**
      * Shows a <select> with all active asset accounts.
      *
@@ -52,22 +94,8 @@ class AccountForm
      */
     public function activeAssetAccountList(string $name, $value = null, array $options = null): string
     {
-        $repository      = $this->getAccountRepository();
-        $accountList     = $repository->getActiveAccountsByType([AccountType::ASSET, AccountType::DEFAULT]);
-        $defaultCurrency = app('amount')->getDefaultCurrency();
-        $grouped         = [];
-        $date            = $this->getDate();
-
-        /** @var Account $account */
-        foreach ($accountList as $account) {
-            $balance                     = app('steam')->balance($account, $date);
-            $role                        = $repository->getMetaValue($account, 'account_role');
-            $currency                    = $repository->getAccountCurrency($account) ?? $defaultCurrency;
-            $role                        = '' === $role ? 'no_account_type' : $role;
-            $key                         = (string)trans(sprintf('firefly.opt_group_%s', $role));
-            $formatted                   = app('amount')->formatAnything($currency, $balance, false);
-            $grouped[$key][$account->id] = sprintf('%s (%s)', $account->name, $formatted);
-        }
+        $types   = [AccountType::ASSET, AccountType::DEFAULT];
+        $grouped = $this->getAccountsGrouped($types);
 
         return $this->select($name, $grouped, $value, $options);
     }
@@ -84,33 +112,8 @@ class AccountForm
      */
     public function activeLongAccountList(string $name, $value = null, array $options = null): string
     {
-        $types           = [AccountType::ASSET, AccountType::DEFAULT, AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN,];
-        $liabilityTypes  = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN];
-        $repository      = $this->getAccountRepository();
-        $accountList     = $repository->getActiveAccountsByType($types);
-        $defaultCurrency = app('amount')->getDefaultCurrency();
-        $grouped         = [];
-        $date            = $this->getDate();
-
-        /** @var Account $account */
-        foreach ($accountList as $account) {
-            $balance  = app('steam')->balance($account, $date);
-            $currency = $repository->getAccountCurrency($account) ?? $defaultCurrency;
-            $role     = $repository->getMetaValue($account, 'account_role');
-
-            if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) {
-                $role = 'no_account_type';
-            }
-
-            if (in_array($account->accountType->type, $liabilityTypes, true)) {
-                $role = sprintf('l_%s', $account->accountType->type);
-            }
-
-            $key                         = (string)trans(sprintf('firefly.opt_group_%s', $role));
-            $formatted                   = app('amount')->formatAnything($currency, $balance, false);
-            $grouped[$key][$account->id] = sprintf('%s (%s)', $account->name, $formatted);
-        }
-
+        $types   = [AccountType::ASSET, AccountType::DEFAULT, AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN,];
+        $grouped = $this->getAccountsGrouped($types);
 
         return $this->select($name, $grouped, $value, $options);
     }
@@ -126,40 +129,14 @@ class AccountForm
      */
     public function activeWithdrawalDestinations(string $name, $value = null, array $options = null): string
     {
-        $types           = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN, AccountType::EXPENSE,];
-        $liabilityTypes  = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN];
-        $repository      = $this->getAccountRepository();
-        $accountList     = $repository->getActiveAccountsByType($types);
-        $defaultCurrency = app('amount')->getDefaultCurrency();
-        $grouped         = [];
-        $date            = $this->getDate();
+        $types      = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN, AccountType::EXPENSE,];
+        $repository = $this->getAccountRepository();
+        $grouped    = $this->getAccountsGrouped($types, $repository);
 
         $cash                     = $repository->getCashAccount();
         $key                      = (string)trans('firefly.cash_account_type');
         $grouped[$key][$cash->id] = sprintf('(%s)', (string)trans('firefly.cash'));
 
-        // group accounts:
-        /** @var Account $account */
-        foreach ($accountList as $account) {
-            $balance  = app('steam')->balance($account, $date);
-            $currency = $repository->getAccountCurrency($account) ?? $defaultCurrency;
-            $role     = (string)$repository->getMetaValue($account, 'account_role');
-            if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) {
-                $role = 'no_account_type';
-            }
-
-            if ('no_account_type' === $role && AccountType::EXPENSE === $account->accountType->type) {
-                $role = 'expense_account';
-            }
-
-            if (in_array($account->accountType->type, $liabilityTypes, true)) {
-                $role = sprintf('l_%s', $account->accountType->type);
-            }
-            $key                         = (string)trans('firefly.opt_group_' . $role);
-            $formatted                   = app('amount')->formatAnything($currency, $balance, false);
-            $grouped[$key][$account->id] = sprintf('%s (%s)', $account->name, $formatted);
-        }
-
         return $this->select($name, $grouped, $value, $options);
     }
 
@@ -174,37 +151,14 @@ class AccountForm
      */
     public function activeDepositDestinations(string $name, $value = null, array $options = null): string
     {
-        $types                    = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN, AccountType::REVENUE,];
-        $liabilityTypes           = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN];
-        $repository               = $this->getAccountRepository();
-        $accountList              = $repository->getActiveAccountsByType($types);
-        $defaultCurrency          = app('amount')->getDefaultCurrency();
-        $grouped                  = [];
-        $date                     = $this->getDate();
+        $types      = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN, AccountType::REVENUE,];
+        $repository = $this->getAccountRepository();
+        $grouped    = $this->getAccountsGrouped($types, $repository);
+
         $cash                     = $repository->getCashAccount();
         $key                      = (string)trans('firefly.cash_account_type');
         $grouped[$key][$cash->id] = sprintf('(%s)', (string)trans('firefly.cash'));
 
-        // group accounts:
-        /** @var Account $account */
-        foreach ($accountList as $account) {
-            $balance  = app('steam')->balance($account, $date);
-            $currency = $repository->getAccountCurrency($account) ?? $defaultCurrency;
-            $role     = (string)$repository->getMetaValue($account, 'account_role');
-            if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) {
-                $role = 'no_account_type';
-            }
-            if ('no_account_type' === $role && AccountType::REVENUE === $account->accountType->type) {
-                $role = 'revenue_account';
-            }
-            if (in_array($account->accountType->type, $liabilityTypes, true)) {
-                $role = sprintf('l_%s', $account->accountType->type); // @codeCoverageIgnore
-            }
-            $key                         = (string)trans(sprintf('firefly.opt_group_%s', $role));
-            $formatted                   = app('amount')->formatAnything($currency, $balance, false);
-            $grouped[$key][$account->id] = sprintf('%s (%s)', $account->name, $formatted);
-        }
-
         return $this->select($name, $grouped, $value, $options);
     }
 
@@ -227,24 +181,8 @@ class AccountForm
         $selected = request()->old($name) ?? [];
 
         // get all asset accounts:
-        $repository    = $this->getAccountRepository();
         $types         = [AccountType::ASSET, AccountType::DEFAULT, AccountType::LOAN, AccountType::MORTGAGE, AccountType::DEBT];
-        $assetAccounts = $repository->getAccountsByType($types);
-        $grouped       = [];
-        // group accounts:
-        /** @var Account $account */
-        foreach ($assetAccounts as $account) {
-            $role = $repository->getMetaValue($account, 'account_role');
-            if (null === $role && in_array($account->accountType->type, [AccountType::LOAN, AccountType::MORTGAGE, AccountType::DEBT], true)) {
-                $role = sprintf('l_%s', $account->accountType->type);
-            }
-            if (null === $role && !in_array($account->accountType->type, [AccountType::LOAN, AccountType::MORTGAGE, AccountType::DEBT], true)) {
-                $role = 'no_account_type';
-            }
-
-            $key                         = (string)trans(sprintf('firefly.opt_group_%s', $role));
-            $grouped[$key][$account->id] = $account->name;
-        }
+        $grouped       = $this->getAccountsGrouped($types);
 
         unset($options['class']);
         try {
@@ -268,26 +206,8 @@ class AccountForm
      */
     public function assetAccountList(string $name, $value = null, array $options = null): string
     {
-        $repository      = $this->getAccountRepository();
-        $types           = [AccountType::ASSET, AccountType::DEFAULT];
-        $accountList     = $repository->getAccountsByType($types);
-        $defaultCurrency = app('amount')->getDefaultCurrency();
-        $grouped         = [];
-        $date            = $this->getDate();
-
-        /** @var Account $account */
-        foreach ($accountList as $account) {
-            $balance  = app('steam')->balance($account, $date);
-            $currency = $repository->getAccountCurrency($account) ?? $defaultCurrency;
-            $role     = (string)$repository->getMetaValue($account, 'account_role');
-            if ('' === $role) {
-                $role = 'no_account_type';
-            }
-
-            $key                         = (string)trans(sprintf('firefly.opt_group_%s', $role));
-            $formatted                   = app('amount')->formatAnything($currency, $balance, false);
-            $grouped[$key][$account->id] = sprintf('%s (%s)', $account->name, $formatted);
-        }
+        $types   = [AccountType::ASSET, AccountType::DEFAULT];
+        $grouped = $this->getAccountsGrouped($types);
 
         return $this->select($name, $grouped, $value, $options);
     }
@@ -304,28 +224,8 @@ class AccountForm
      */
     public function longAccountList(string $name, $value = null, array $options = null): string
     {
-        $types           = [AccountType::ASSET, AccountType::DEFAULT, AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN,];
-        $liabilityTypes  = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN];
-        $repository      = $this->getAccountRepository();
-        $accountList     = $repository->getAccountsByType($types);
-        $defaultCurrency = app('amount')->getDefaultCurrency();
-        $grouped         = [];
-        $date            = $this->getDate();
-        /** @var Account $account */
-        foreach ($accountList as $account) {
-            $balance  = app('steam')->balance($account, $date);
-            $currency = $repository->getAccountCurrency($account) ?? $defaultCurrency;
-            $role     = (string)$repository->getMetaValue($account, 'account_role');
-            if ('' === $role) {
-                $role = 'no_account_type';
-            }
-            if (in_array($account->accountType->type, $liabilityTypes, true)) {
-                $role = sprintf('l_%s', $account->accountType->type);
-            }
-            $key                         = (string)trans(sprintf('firefly.opt_group_%s', $role));
-            $formatted                   = app('amount')->formatAnything($currency, $balance, false);
-            $grouped[$key][$account->id] = sprintf('%s (%s)', $account->name, $formatted);
-        }
+        $types   = [AccountType::ASSET, AccountType::DEFAULT, AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN,];
+        $grouped = $this->getAccountsGrouped($types);
 
         return $this->select($name, $grouped, $value, $options);
     }
diff --git a/app/Support/Steam.php b/app/Support/Steam.php
index 4b3a319346..adc6c22e05 100644
--- a/app/Support/Steam.php
+++ b/app/Support/Steam.php
@@ -40,12 +40,14 @@ class Steam
 {
 
     /**
+     * Gets balance at the end of current month by default
+     *
      * @param \FireflyIII\Models\Account $account
      * @param \Carbon\Carbon             $date
      *
      * @return string
      */
-    public function balance(Account $account, Carbon $date, ?TransactionCurrency $currency = null): string
+    public function balance(Account $account, Carbon $date = null, ?TransactionCurrency $currency = null): string
     {
         if ('testing' === config('app.env')) {
             Log::warning(sprintf('%s should NOT be called in the TEST environment!', __METHOD__));
@@ -58,6 +60,9 @@ class Steam
         if ($cache->has()) {
             return $cache->get(); // @codeCoverageIgnore
         }
+        if (null === $date) {
+            $date = Carbon::now()->endOfMonth();
+        }
         /** @var AccountRepositoryInterface $repository */
         $repository = app(AccountRepositoryInterface::class);
         if (null === $currency) {
@@ -255,12 +260,14 @@ class Steam
     }
 
     /**
+     * Gets balance at the end of current month by default
+     *
      * @param \FireflyIII\Models\Account $account
      * @param \Carbon\Carbon             $date
      *
      * @return array
      */
-    public function balancePerCurrency(Account $account, Carbon $date): array
+    public function balancePerCurrency(Account $account, Carbon $date = null): array
     {
         if ('testing' === config('app.env')) {
             Log::warning(sprintf('%s should NOT be called in the TEST environment!', __METHOD__));
@@ -273,6 +280,9 @@ class Steam
         if ($cache->has()) {
             return $cache->get(); // @codeCoverageIgnore
         }
+        if (null === $date) {
+            $date = Carbon::now()->endOfMonth();
+        }
         $query    = $account->transactions()
                             ->leftJoin('transaction_journals', 'transaction_journals.id', '=', 'transactions.transaction_journal_id')
                             ->where('transaction_journals.date', '<=', $date->format('Y-m-d 23:59:59'))
@@ -289,14 +299,14 @@ class Steam
     }
 
     /**
-     * This method always ignores the virtual balance.
+     * This method always ignores the virtual balance. Gets balance at the end of current month by default
      *
      * @param \Illuminate\Support\Collection $accounts
      * @param \Carbon\Carbon                 $date
      *
      * @return array
      */
-    public function balancesByAccounts(Collection $accounts, Carbon $date): array
+    public function balancesByAccounts(Collection $accounts, Carbon $date = null): array
     {
         if ('testing' === config('app.env')) {
             Log::warning(sprintf('%s should NOT be called in the TEST environment!', __METHOD__));
@@ -324,14 +334,14 @@ class Steam
     }
 
     /**
-     * Same as above, but also groups per currency.
+     * Same as above, but also groups per currency. Gets balance at the end of current month by default
      *
      * @param \Illuminate\Support\Collection $accounts
      * @param \Carbon\Carbon                 $date
      *
      * @return array
      */
-    public function balancesPerCurrencyByAccounts(Collection $accounts, Carbon $date): array
+    public function balancesPerCurrencyByAccounts(Collection $accounts, Carbon $date = null): array
     {
         if ('testing' === config('app.env')) {
             Log::warning(sprintf('%s should NOT be called in the TEST environment!', __METHOD__));