From e48eb2ce2feea1e18b42d51ff64e90978aa5d7e2 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sun, 16 Apr 2017 22:15:05 +0200 Subject: [PATCH] Clean up account taker amount inconsistencies. --- app/Http/Controllers/AccountController.php | 45 ++++--- .../Controllers/Chart/ReportController.php | 32 +++-- app/Http/Controllers/JsonController.php | 29 +++-- app/Repositories/Account/AccountTasker.php | 117 ------------------ .../Account/AccountTaskerInterface.php | 24 ---- 5 files changed, 71 insertions(+), 176 deletions(-) diff --git a/app/Http/Controllers/AccountController.php b/app/Http/Controllers/AccountController.php index 02599350c6..fe2dffa71f 100644 --- a/app/Http/Controllers/AccountController.php +++ b/app/Http/Controllers/AccountController.php @@ -22,8 +22,8 @@ use FireflyIII\Http\Requests\AccountFormRequest; use FireflyIII\Models\Account; use FireflyIII\Models\AccountType; use FireflyIII\Models\Transaction; +use FireflyIII\Models\TransactionType; use FireflyIII\Repositories\Account\AccountRepositoryInterface; -use FireflyIII\Repositories\Account\AccountTaskerInterface; use FireflyIII\Repositories\Currency\CurrencyRepositoryInterface; use FireflyIII\Repositories\Journal\JournalRepositoryInterface; use FireflyIII\Support\CacheProperties; @@ -326,7 +326,8 @@ class AccountController extends Controller return view( - 'accounts.show', compact('account','currency', 'moment', 'accountType', 'periods', 'subTitleIcon', 'journals', 'subTitle', 'start', 'end', 'chartUri') + 'accounts.show', + compact('account', 'currency', 'moment', 'accountType', 'periods', 'subTitleIcon', 'journals', 'subTitle', 'start', 'end', 'chartUri') ); } @@ -418,14 +419,11 @@ class AccountController extends Controller { /** @var AccountRepositoryInterface $repository */ $repository = app(AccountRepositoryInterface::class); - /** @var AccountTaskerInterface $tasker */ - $tasker = app(AccountTaskerInterface::class); - - $start = $repository->oldestJournalDate($account); - $range = Preferences::get('viewRange', '1M')->data; - $start = Navigation::startOfPeriod($start, $range); - $end = Navigation::endOfX(new Carbon, $range); - $entries = new Collection; + $start = $repository->oldestJournalDate($account); + $range = Preferences::get('viewRange', '1M')->data; + $start = Navigation::startOfPeriod($start, $range); + $end = Navigation::endOfX(new Carbon, $range); + $entries = new Collection; // properties for cache $cache = new CacheProperties; @@ -438,19 +436,28 @@ class AccountController extends Controller return $cache->get(); // @codeCoverageIgnore } - // only include asset accounts when this account is an asset: - $assets = new Collection; - if (in_array($account->accountType->type, [AccountType::ASSET, AccountType::DEFAULT])) { - $assets = $repository->getAccountsByType([AccountType::ASSET, AccountType::DEFAULT]); - } Log::debug('Going to get period expenses and incomes.'); while ($end >= $start) { $end = Navigation::startOfPeriod($end, $range); $currentEnd = Navigation::endOfPeriod($end, $range); - $spent = $tasker->amountOutInPeriod(new Collection([$account]), $assets, $end, $currentEnd); - $earned = $tasker->amountInInPeriod(new Collection([$account]), $assets, $end, $currentEnd); - $dateStr = $end->format('Y-m-d'); - $dateName = Navigation::periodShow($end, $range); + + // try a collector for income: + /** @var JournalCollectorInterface $collector */ + $collector = app(JournalCollectorInterface::class); + $collector->setAccounts(new Collection([$account]))->setRange($end, $currentEnd) + ->setTypes([TransactionType::DEPOSIT]) + ->withOpposingAccount(); + $earned = strval($collector->getJournals()->sum('transaction_amount')); + + // try a collector for expenses: + /** @var JournalCollectorInterface $collector */ + $collector = app(JournalCollectorInterface::class); + $collector->setAccounts(new Collection([$account]))->setRange($end, $currentEnd) + ->setTypes([TransactionType::WITHDRAWAL]) + ->withOpposingAccount(); + $spent = strval($collector->getJournals()->sum('transaction_amount')); + $dateStr = $end->format('Y-m-d'); + $dateName = Navigation::periodShow($end, $range); $entries->push( [ 'string' => $dateStr, diff --git a/app/Http/Controllers/Chart/ReportController.php b/app/Http/Controllers/Chart/ReportController.php index 3e93d69d4e..9fd9dd7843 100644 --- a/app/Http/Controllers/Chart/ReportController.php +++ b/app/Http/Controllers/Chart/ReportController.php @@ -16,10 +16,12 @@ namespace FireflyIII\Http\Controllers\Chart; use Carbon\Carbon; use FireflyIII\Generator\Chart\Basic\GeneratorInterface; +use FireflyIII\Helpers\Collector\JournalCollectorInterface; use FireflyIII\Http\Controllers\Controller; -use FireflyIII\Repositories\Account\AccountTaskerInterface; +use FireflyIII\Models\TransactionType; use FireflyIII\Support\CacheProperties; use Illuminate\Support\Collection; +use Log; use Navigation; use Response; use Steam; @@ -103,8 +105,9 @@ class ReportController extends Controller $cache->addProperty($accounts); $cache->addProperty($end); if ($cache->has()) { - return Response::json($cache->get()); // @codeCoverageIgnore + //return Response::json($cache->get()); // @codeCoverageIgnore } + Log::debug('Going to do operations for accounts ', $accounts->pluck('id')->toArray()); $format = Navigation::preferredCarbonLocalizedFormat($start, $end); $source = $this->getChartData($accounts, $start, $end); $chartData = [ @@ -249,16 +252,31 @@ class ReportController extends Controller return $cache->get(); // @codeCoverageIgnore } - - $tasker = app(AccountTaskerInterface::class); $currentStart = clone $start; $spentArray = []; $earnedArray = []; while ($currentStart <= $end) { - $currentEnd = Navigation::endOfPeriod($currentStart, '1M'); + + $currentEnd = Navigation::endOfPeriod($currentStart, '1M'); + + // try a collector for income: + /** @var JournalCollectorInterface $collector */ + $collector = app(JournalCollectorInterface::class); + $collector->setAccounts($accounts)->setRange($currentStart, $currentEnd) + ->setTypes([TransactionType::DEPOSIT, TransactionType::TRANSFER]) + ->withOpposingAccount(); + $earned = strval($collector->getJournals()->sum('transaction_amount')); + + // try a collector for expenses: + /** @var JournalCollectorInterface $collector */ + $collector = app(JournalCollectorInterface::class); + $collector->setAccounts($accounts)->setRange($currentStart, $currentEnd) + ->setTypes([TransactionType::WITHDRAWAL, TransactionType::TRANSFER]) + ->withOpposingAccount(); + $spent = strval($collector->getJournals()->sum('transaction_amount')); + + $label = $currentStart->format('Y-m') . '-01'; - $spent = $tasker->amountOutInPeriod($accounts, $accounts, $currentStart, $currentEnd); - $earned = $tasker->amountInInPeriod($accounts, $accounts, $currentStart, $currentEnd); $spentArray[$label] = bcmul($spent, '-1'); $earnedArray[$label] = $earned; $currentStart = Navigation::addPeriod($currentStart, '1M', 0); diff --git a/app/Http/Controllers/JsonController.php b/app/Http/Controllers/JsonController.php index a9502f5fdb..c8e916e231 100644 --- a/app/Http/Controllers/JsonController.php +++ b/app/Http/Controllers/JsonController.php @@ -18,6 +18,7 @@ use Carbon\Carbon; use FireflyIII\Exceptions\FireflyException; use FireflyIII\Helpers\Collector\JournalCollectorInterface; use FireflyIII\Models\AccountType; +use FireflyIII\Models\TransactionType; use FireflyIII\Repositories\Account\AccountRepositoryInterface; use FireflyIII\Repositories\Account\AccountTaskerInterface; use FireflyIII\Repositories\Bill\BillRepositoryInterface; @@ -145,7 +146,7 @@ class JsonController extends Controller * @return \Illuminate\Http\JsonResponse * */ - public function boxIn(AccountTaskerInterface $accountTasker, AccountRepositoryInterface $repository) + public function boxIn() { $start = session('start', Carbon::now()->startOfMonth()); $end = session('end', Carbon::now()->endOfMonth()); @@ -158,10 +159,16 @@ class JsonController extends Controller if ($cache->has()) { return Response::json($cache->get()); // @codeCoverageIgnore } - $accounts = $repository->getAccountsByType([AccountType::DEFAULT, AccountType::ASSET, AccountType::CASH]); - $assets = $repository->getAccountsByType([AccountType::DEFAULT, AccountType::ASSET]); - $amount = $accountTasker->amountInInPeriod($accounts, $assets, $start, $end); - $data = ['box' => 'in', 'amount' => Amount::format($amount, false), 'amount_raw' => $amount]; + + // try a collector for income: + /** @var JournalCollectorInterface $collector */ + $collector = app(JournalCollectorInterface::class); + $collector->setAllAssetAccounts()->setRange($start, $end) + ->setTypes([TransactionType::DEPOSIT]) + ->withOpposingAccount(); + + $amount = strval($collector->getJournals()->sum('transaction_amount')); + $data = ['box' => 'in', 'amount' => Amount::format($amount, false), 'amount_raw' => $amount]; $cache->store($data); return Response::json($data); @@ -173,7 +180,7 @@ class JsonController extends Controller * * @return \Symfony\Component\HttpFoundation\Response */ - public function boxOut(AccountTaskerInterface $accountTasker, AccountRepositoryInterface $repository) + public function boxOut() { $start = session('start', Carbon::now()->startOfMonth()); $end = session('end', Carbon::now()->endOfMonth()); @@ -187,9 +194,13 @@ class JsonController extends Controller return Response::json($cache->get()); // @codeCoverageIgnore } - $accounts = $repository->getAccountsByType([AccountType::DEFAULT, AccountType::ASSET, AccountType::CASH]); - $assets = $repository->getAccountsByType([AccountType::DEFAULT, AccountType::ASSET]); - $amount = $accountTasker->amountOutInPeriod($accounts, $assets, $start, $end); + // try a collector for expenses: + /** @var JournalCollectorInterface $collector */ + $collector = app(JournalCollectorInterface::class); + $collector->setAllAssetAccounts()->setRange($start, $end) + ->setTypes([TransactionType::WITHDRAWAL]) + ->withOpposingAccount(); + $amount = strval($collector->getJournals()->sum('transaction_amount')); $data = ['box' => 'out', 'amount' => Amount::format($amount, false), 'amount_raw' => $amount]; $cache->store($data); diff --git a/app/Repositories/Account/AccountTasker.php b/app/Repositories/Account/AccountTasker.php index 89f51e9217..7e31de5c4e 100644 --- a/app/Repositories/Account/AccountTasker.php +++ b/app/Repositories/Account/AccountTasker.php @@ -31,64 +31,6 @@ class AccountTasker implements AccountTaskerInterface /** @var User */ private $user; - /** - * @see self::amountInPeriod - * - * @param Collection $accounts - * @param Collection $excluded - * @param Carbon $start - * @param Carbon $end - * - * @return string - */ - public function amountInInPeriod(Collection $accounts, Collection $excluded, Carbon $start, Carbon $end): string - { - $idList = [ - 'accounts' => $accounts->pluck('id')->toArray(), - 'exclude' => $excluded->pluck('id')->toArray(), - ]; - - Log::debug( - 'Now calling amountInInPeriod.', - ['accounts' => $idList['accounts'], 'excluded' => $idList['exclude'], - 'start' => $start->format('Y-m-d'), - 'end' => $end->format('Y-m-d'), - ] - ); - - return $this->amountInPeriod($idList, $start, $end, true); - - } - - /** - * @see self::amountInPeriod - * - * @param Collection $accounts - * @param Collection $excluded - * @param Carbon $start - * @param Carbon $end - * - * @return string - */ - public function amountOutInPeriod(Collection $accounts, Collection $excluded, Carbon $start, Carbon $end): string - { - $idList = [ - 'accounts' => $accounts->pluck('id')->toArray(), - 'exclude' => $excluded->pluck('id')->toArray(), - ]; - - Log::debug( - 'Now calling amountOutInPeriod.', - ['accounts' => $idList['accounts'], 'excluded' => $idList['exclude'], - 'start' => $start->format('Y-m-d'), - 'end' => $end->format('Y-m-d'), - ] - ); - - return $this->amountInPeriod($idList, $start, $end, false); - - } - /** * @param Collection $accounts * @param Carbon $start @@ -155,63 +97,4 @@ class AccountTasker implements AccountTaskerInterface $this->user = $user; } - /** - * Will return how much money has been going out (ie. spent) by the given account(s). - * Alternatively, will return how much money has been coming in (ie. earned) by the given accounts. - * - * Enter $incoming=true for any money coming in (income) - * Enter $incoming=false for any money going out (expenses) - * - * This means any money going out or in. You can also submit accounts to exclude, - * so transfers between accounts are not included. - * - * As a general rule: - * - * - Asset accounts should return both expenses and earnings. But could return 0. - * - Expense accounts (where money is spent) should only return earnings (the account gets money). - * - Revenue accounts (where money comes from) should only return expenses (they spend money). - * - * - * - * @param array $accounts - * @param Carbon $start - * @param Carbon $end - * @param bool $incoming - * - * @return string - */ - protected function amountInPeriod(array $accounts, Carbon $start, Carbon $end, bool $incoming): string - { - $joinModifier = $incoming ? '<' : '>'; - $selection = $incoming ? '>' : '<'; - - $query = Transaction::distinct() - ->leftJoin('transaction_journals', 'transaction_journals.id', '=', 'transactions.transaction_journal_id') - ->leftJoin( - 'transactions as other_side', function (JoinClause $join) use ($joinModifier) { - $join->on('transaction_journals.id', '=', 'other_side.transaction_journal_id')->where('other_side.amount', $joinModifier, 0); - } - ) - ->where('transaction_journals.date', '>=', $start->format('Y-m-d')) - ->where('transaction_journals.date', '<=', $end->format('Y-m-d')) - ->where('transaction_journals.user_id', $this->user->id) - ->whereNull('transactions.deleted_at') - ->whereNull('transaction_journals.deleted_at') - ->whereIn('transactions.account_id', $accounts['accounts']) - ->where('transactions.amount', $selection, 0); - if (count($accounts['exclude']) > 0) { - $query->whereNotIn('other_side.account_id', $accounts['exclude']); - } - - $result = $query->get(['transactions.id', 'transactions.amount']); - $sum = strval($result->sum('amount')); - if (strlen($sum) === 0) { - Log::debug('Sum is empty.'); - $sum = '0'; - } - Log::debug(sprintf('Result is %s', $sum)); - - return $sum; - } - } diff --git a/app/Repositories/Account/AccountTaskerInterface.php b/app/Repositories/Account/AccountTaskerInterface.php index 4927a264b0..7ab6fea905 100644 --- a/app/Repositories/Account/AccountTaskerInterface.php +++ b/app/Repositories/Account/AccountTaskerInterface.php @@ -24,30 +24,6 @@ use Illuminate\Support\Collection; */ interface AccountTaskerInterface { - /** - * @param Collection $accounts - * @param Collection $excluded - * @param Carbon $start - * @param Carbon $end - * - * @see AccountTasker::amountInPeriod() - * - * @return string - */ - public function amountInInPeriod(Collection $accounts, Collection $excluded, Carbon $start, Carbon $end): string; - - /** - * @param Collection $accounts - * @param Collection $excluded - * @param Carbon $start - * @param Carbon $end - * - * @see AccountTasker::amountInPeriod() - * - * @return string - */ - public function amountOutInPeriod(Collection $accounts, Collection $excluded, Carbon $start, Carbon $end): string; - /** * @param Collection $accounts * @param Carbon $start