From e3437ba697f311f4fa33f9d86c8c5f4b66cde858 Mon Sep 17 00:00:00 2001 From: James Cole Date: Wed, 27 Apr 2016 19:21:47 +0200 Subject: [PATCH] Various code cleanup. --- app/Helpers/Report/AccountReportHelper.php | 58 ++++++------- app/Helpers/Report/BalanceReportHelper.php | 64 +++++++++------ app/Helpers/Report/ReportHelper.php | 12 +-- app/Http/Controllers/AccountController.php | 2 +- app/Http/Controllers/PiggyBankController.php | 2 +- app/Http/Controllers/RuleGroupController.php | 2 +- .../Account/AccountRepository.php | 6 +- app/Support/ExpandedForm.php | 81 ++++++++++++------- app/Support/Steam.php | 34 +++++++- 9 files changed, 154 insertions(+), 107 deletions(-) diff --git a/app/Helpers/Report/AccountReportHelper.php b/app/Helpers/Report/AccountReportHelper.php index 4e97a949c8..e1eee8f7c9 100644 --- a/app/Helpers/Report/AccountReportHelper.php +++ b/app/Helpers/Report/AccountReportHelper.php @@ -26,7 +26,12 @@ class AccountReportHelper implements AccountReportHelperInterface { /** * This method generates a full report for the given period on all - * given accounts + * given accounts. + * + * a special consideration for accounts that did exist on this exact day. + * we also grab the balance from today just in case, to see if that changes things. + * it's a fall back for users who (rightly so) start keeping score at the first of + * the month and find the first report lacking / broken. * * @param Carbon $start * @param Carbon $end @@ -42,26 +47,13 @@ class AccountReportHelper implements AccountReportHelperInterface $ids = $accounts->pluck('id')->toArray(); $yesterday = clone $start; $yesterday->subDay(); - - // get balances for start. - $startSet = $this->getSet($ids, $yesterday); - - // a special consideration for accounts that did exist on this exact day. - // we also grab the balance from today just in case, to see if that changes things. - // it's a fall back for users who (rightly so) start keeping score at the first of - // the month and find the first report lacking / broken. + $startSet = $this->getSet($ids, $yesterday); // get balances for start. $backupSet = $this->getSet($ids, $start); - - // and end: - $endSet = $this->getSet($ids, $end); + $endSet = $this->getSet($ids, $end); $accounts->each( function (Account $account) use ($startSet, $endSet, $backupSet) { - /** - * The balance for today always incorporates transactions - * made on today. So to get todays "start" balance, we sub one - * day. - */ + // The balance for today always incorporates transactions made on today. So to get todays "start" balance, we sub one day. $account->startBalance = '0'; $account->endBalance = '0'; $currentStart = $startSet->filter( @@ -69,28 +61,24 @@ class AccountReportHelper implements AccountReportHelperInterface return $account->id == $entry->id; } ); - // grab entry from current backup as well: - $currentBackup = $backupSet->filter( + + $currentBackup = $backupSet->filter( // grab entry from current backup as well: function (Account $entry) use ($account) { return $account->id == $entry->id; } ); - - - if ($currentStart->first()) { + if (!is_null($currentStart->first())) { $account->startBalance = $currentStart->first()->balance; - } else { - if (is_null($currentStart->first()) && !is_null($currentBackup->first())) { - $account->startBalance = $currentBackup->first()->balance; - } } - + if (is_null($currentStart->first()) && !is_null($currentBackup->first())) { + $account->startBalance = $currentBackup->first()->balance; + } $currentEnd = $endSet->filter( function (Account $entry) use ($account) { return $account->id == $entry->id; } ); - if ($currentEnd->first()) { + if (!is_null($currentEnd->first())) { $account->endBalance = $currentEnd->first()->balance; } } @@ -121,12 +109,12 @@ class AccountReportHelper implements AccountReportHelperInterface private function getSet(array $ids, Carbon $date): Collection { return Account::leftJoin('transactions', 'transactions.account_id', '=', 'accounts.id') - ->leftJoin('transaction_journals', 'transaction_journals.id', '=', 'transactions.transaction_journal_id') - ->whereIn('accounts.id', $ids) - ->whereNull('transaction_journals.deleted_at') - ->whereNull('transactions.deleted_at') - ->where('transaction_journals.date', '<=', $date->format('Y-m-d')) - ->groupBy('accounts.id') - ->get(['accounts.id', DB::raw('SUM(`transactions`.`amount`) as `balance`')]); + ->leftJoin('transaction_journals', 'transaction_journals.id', '=', 'transactions.transaction_journal_id') + ->whereIn('accounts.id', $ids) + ->whereNull('transaction_journals.deleted_at') + ->whereNull('transactions.deleted_at') + ->where('transaction_journals.date', '<=', $date->format('Y-m-d')) + ->groupBy('accounts.id') + ->get(['accounts.id', DB::raw('SUM(`transactions`.`amount`) as `balance`')]); } } diff --git a/app/Helpers/Report/BalanceReportHelper.php b/app/Helpers/Report/BalanceReportHelper.php index ea3ab0e36f..fbcb9e6930 100644 --- a/app/Helpers/Report/BalanceReportHelper.php +++ b/app/Helpers/Report/BalanceReportHelper.php @@ -17,6 +17,7 @@ use FireflyIII\Helpers\Collection\Balance; use FireflyIII\Helpers\Collection\BalanceEntry; use FireflyIII\Helpers\Collection\BalanceHeader; use FireflyIII\Helpers\Collection\BalanceLine; +use FireflyIII\Models\Account; use FireflyIII\Models\Budget; use FireflyIII\Models\Budget as BudgetModel; use FireflyIII\Models\Tag; @@ -172,6 +173,43 @@ class BalanceReportHelper implements BalanceReportHelperInterface return $line; } + /** + * @param Account $account + * @param Collection $spentData + * @param Collection $tagsLeft + * + * @return BalanceEntry + */ + private function createDifferenceBalanceEntry(Account $account, Collection $spentData, Collection $tagsLeft): BalanceEntry + { + $entry = $spentData->filter( + function (TransactionJournal $model) use ($account) { + return $model->account_id == $account->id && is_null($model->budget_id); + } + ); + $spent = '0'; + if (!is_null($entry->first())) { + $spent = $entry->first()->spent; + } + $leftEntry = $tagsLeft->filter( + function (Tag $tag) use ($account) { + return $tag->account_id == $account->id; + } + ); + $left = '0'; + if (!is_null($leftEntry->first())) { + $left = $leftEntry->first()->sum; + } + $diffValue = bcadd($spent, $left); + + // difference: + $diffEntry = new BalanceEntry; + $diffEntry->setAccount($account); + $diffEntry->setSpent($diffValue); + + return $diffEntry; + } + /** * @param Collection $accounts * @param Collection $spentData @@ -189,31 +227,9 @@ class BalanceReportHelper implements BalanceReportHelperInterface $diff->setRole(BalanceLine::ROLE_DIFFROLE); + /** @var Account $account */ foreach ($accounts as $account) { - $entry = $spentData->filter( - function (TransactionJournal $model) use ($account) { - return $model->account_id == $account->id && is_null($model->budget_id); - } - ); - $spent = '0'; - if (!is_null($entry->first())) { - $spent = $entry->first()->spent; - } - $leftEntry = $tagsLeft->filter( - function (Tag $tag) use ($account) { - return $tag->account_id == $account->id; - } - ); - $left = '0'; - if (!is_null($leftEntry->first())) { - $left = $leftEntry->first()->sum; - } - $diffValue = bcadd($spent, $left); - - // difference: - $diffEntry = new BalanceEntry; - $diffEntry->setAccount($account); - $diffEntry->setSpent($diffValue); + $diffEntry = $this->createDifferenceBalanceEntry($account, $spentData, $tagsLeft); $diff->addBalanceEntry($diffEntry); } diff --git a/app/Helpers/Report/ReportHelper.php b/app/Helpers/Report/ReportHelper.php index dc8c7e71a9..bf623f2336 100644 --- a/app/Helpers/Report/ReportHelper.php +++ b/app/Helpers/Report/ReportHelper.php @@ -77,7 +77,7 @@ class ReportHelper implements ReportHelperInterface $billLine->setActive(intval($bill->active) == 1); $billLine->setMin($bill->amount_min); $billLine->setMax($bill->amount_max); - + $billLine->setHit(false); // is hit in period? $entry = $journals->filter( @@ -90,8 +90,6 @@ class ReportHelper implements ReportHelperInterface $billLine->setTransactionJournalId($first->id); $billLine->setAmount($first->journalAmount); $billLine->setHit(true); - } else { - $billLine->setHit(false); } if (!(!$billLine->isHit() && !$billLine->isActive())) { $collection->addBill($billLine); @@ -219,9 +217,7 @@ class ReportHelper implements ReportHelperInterface $months = []; while ($start <= $end) { - // current year: - $year = $fiscalHelper->endOfFiscalYear($start)->year; - + $year = $fiscalHelper->endOfFiscalYear($start)->year; // current year if (!isset($months[$year])) { $months[$year] = [ 'fiscal_start' => $fiscalHelper->startOfFiscalYear($start)->format('Y-m-d'), @@ -234,7 +230,6 @@ class ReportHelper implements ReportHelperInterface $currentEnd = clone $start; $currentEnd->endOfMonth(); - $months[$year]['months'][] = [ 'formatted' => $start->formatLocalized('%B %Y'), 'start' => $start->format('Y-m-d'), @@ -243,8 +238,7 @@ class ReportHelper implements ReportHelperInterface 'year' => $year, ]; - // to make the hop to the next month properly: - $start = clone $currentEnd; + $start = clone $currentEnd; // to make the hop to the next month properly $start->addDay(); } diff --git a/app/Http/Controllers/AccountController.php b/app/Http/Controllers/AccountController.php index 59abfc1dc2..e11f45afea 100644 --- a/app/Http/Controllers/AccountController.php +++ b/app/Http/Controllers/AccountController.php @@ -65,7 +65,7 @@ class AccountController extends Controller { $typeName = config('firefly.shortNamesByFullName.' . $account->accountType->type); $subTitle = trans('firefly.delete_' . $typeName . '_account', ['name' => $account->name]); - $accountList = ExpandedForm::makeSelectList($repository->getAccounts([$account->accountType->type]), true); + $accountList = ExpandedForm::makeSelectListWithEmpty($repository->getAccounts([$account->accountType->type])); unset($accountList[$account->id]); // put previous url in session diff --git a/app/Http/Controllers/PiggyBankController.php b/app/Http/Controllers/PiggyBankController.php index a95ccd496d..19b96bc78e 100644 --- a/app/Http/Controllers/PiggyBankController.php +++ b/app/Http/Controllers/PiggyBankController.php @@ -183,7 +183,7 @@ class PiggyBankController extends Controller if (!isset($accounts[$account->id])) { $accounts[$account->id] = [ 'name' => $account->name, - 'balance' => Steam::balance($account, $end, true), + 'balance' => Steam::balanceIgnoreVirtual($account, $end), 'leftForPiggyBanks' => $repository->leftOnAccount($account, $end), 'sumOfSaved' => strval($piggyBank->savedSoFar), 'sumOfTargets' => strval(round($piggyBank->targetamount, 2)), diff --git a/app/Http/Controllers/RuleGroupController.php b/app/Http/Controllers/RuleGroupController.php index 9b4afec0f3..622f47e3e8 100644 --- a/app/Http/Controllers/RuleGroupController.php +++ b/app/Http/Controllers/RuleGroupController.php @@ -64,7 +64,7 @@ class RuleGroupController extends Controller { $subTitle = trans('firefly.delete_rule_group', ['title' => $ruleGroup->title]); - $ruleGroupList = ExpandedForm::makeSelectList($repository->get(), true); + $ruleGroupList = ExpandedForm::makeSelectListWithEmpty($repository->get()); unset($ruleGroupList[$ruleGroup->id]); // put previous url in session diff --git a/app/Repositories/Account/AccountRepository.php b/app/Repositories/Account/AccountRepository.php index 1c518a210f..25f1775c96 100644 --- a/app/Repositories/Account/AccountRepository.php +++ b/app/Repositories/Account/AccountRepository.php @@ -352,8 +352,8 @@ class AccountRepository implements AccountRepositoryInterface $accounts->each( function (Account $account) use ($start, $end) { - $account->startBalance = Steam::balance($account, $start, true); - $account->endBalance = Steam::balance($account, $end, true); + $account->startBalance = Steam::balanceIgnoreVirtual($account, $start); + $account->endBalance = Steam::balanceIgnoreVirtual($account, $end); $account->piggyBalance = 0; /** @var PiggyBank $piggyBank */ foreach ($account->piggyBanks as $piggyBank) { @@ -426,7 +426,7 @@ class AccountRepository implements AccountRepositoryInterface public function leftOnAccount(Account $account, Carbon $date): string { - $balance = Steam::balance($account, $date, true); + $balance = Steam::balanceIgnoreVirtual($account, $date); /** @var PiggyBank $p */ foreach ($account->piggybanks()->get() as $p) { $balance -= $p->currentRelevantRep()->currentamount; diff --git a/app/Support/ExpandedForm.php b/app/Support/ExpandedForm.php index a3e40cd4d6..19981d749d 100644 --- a/app/Support/ExpandedForm.php +++ b/app/Support/ExpandedForm.php @@ -20,31 +20,6 @@ use Session; class ExpandedForm { - /** - * @param $name - * @param null $value - * @param array $options - * - * @return string - */ - public function amountSmall(string $name, $value = null, array $options = []): string - { - $label = $this->label($name, $options); - $options = $this->expandOptionArray($name, $label, $options); - $classes = $this->getHolderClasses($name); - $value = $this->fillFieldValue($name, $value); - $options['step'] = 'any'; - $options['min'] = '0.01'; - $defaultCurrency = isset($options['currency']) ? $options['currency'] : Amt::getDefaultCurrency(); - $currencies = Amt::getAllCurrencies(); - unset($options['currency']); - unset($options['placeholder']); - $html = view('form.amount-small', compact('defaultCurrency', 'currencies', 'classes', 'name', 'value', 'options'))->render(); - - return $html; - - } - /** * @param $name * @param null $value @@ -70,6 +45,31 @@ class ExpandedForm } + /** + * @param $name + * @param null $value + * @param array $options + * + * @return string + */ + public function amountSmall(string $name, $value = null, array $options = []): string + { + $label = $this->label($name, $options); + $options = $this->expandOptionArray($name, $label, $options); + $classes = $this->getHolderClasses($name); + $value = $this->fillFieldValue($name, $value); + $options['step'] = 'any'; + $options['min'] = '0.01'; + $defaultCurrency = isset($options['currency']) ? $options['currency'] : Amt::getDefaultCurrency(); + $currencies = Amt::getAllCurrencies(); + unset($options['currency']); + unset($options['placeholder']); + $html = view('form.amount-small', compact('defaultCurrency', 'currencies', 'classes', 'name', 'value', 'options'))->render(); + + return $html; + + } + /** * @param $name * @param null $value @@ -196,17 +196,38 @@ class ExpandedForm * Takes any collection and tries to make a sensible select list compatible array of it. * * @param \Illuminate\Support\Collection $set - * @param bool $addEmpty * * @return array */ - public function makeSelectList(Collection $set, bool $addEmpty = false): array + public function makeSelectList(Collection $set): array { $selectList = []; - if ($addEmpty) { - $selectList[0] = '(none)'; + $fields = ['title', 'name', 'description']; + /** @var Eloquent $entry */ + foreach ($set as $entry) { + $entryId = intval($entry->id); + $title = null; + + foreach ($fields as $field) { + if (isset($entry->$field) && is_null($title)) { + $title = $entry->$field; + } + } + $selectList[$entryId] = $title; } - $fields = ['title', 'name', 'description']; + + return $selectList; + } + + /** + * @param Collection $set + * + * @return array + */ + public function makeSelectListWithEmpty(Collection $set): array + { + $selectList[0] = '(none)'; + $fields = ['title', 'name', 'description']; /** @var Eloquent $entry */ foreach ($set as $entry) { $entryId = intval($entry->id); diff --git a/app/Support/Steam.php b/app/Support/Steam.php index 3e73432113..e6d76b37db 100644 --- a/app/Support/Steam.php +++ b/app/Support/Steam.php @@ -16,7 +16,6 @@ use FireflyIII\Models\Transaction; */ class Steam { - /** * * @param \FireflyIII\Models\Account $account @@ -43,10 +42,39 @@ class Steam 'transaction_journals', 'transaction_journals.id', '=', 'transactions.transaction_journal_id' )->where('transaction_journals.date', '<=', $date->format('Y-m-d'))->sum('transactions.amount') ); + $balance = bcadd($balance, $account->virtual_balance); + $cache->store($balance); - if (!$ignoreVirtualBalance) { - $balance = bcadd($balance, $account->virtual_balance); + return $balance; + } + + /** + * + * @param \FireflyIII\Models\Account $account + * @param \Carbon\Carbon $date + * @param bool $ignoreVirtualBalance + * + * @return string + */ + public function balanceIgnoreVirtual(Account $account, Carbon $date, $ignoreVirtualBalance = false): string + { + + // abuse chart properties: + $cache = new CacheProperties; + $cache->addProperty($account->id); + $cache->addProperty('balance-no-virtual'); + $cache->addProperty($date); + $cache->addProperty($ignoreVirtualBalance); + if ($cache->has()) { + return $cache->get(); } + + $balance = strval( + $account->transactions()->leftJoin( + 'transaction_journals', 'transaction_journals.id', '=', 'transactions.transaction_journal_id' + )->where('transaction_journals.date', '<=', $date->format('Y-m-d'))->sum('transactions.amount') + ); + $cache->store($balance); return $balance;