From 6197c7730315fdeec2125bd1dafdf67095aafa29 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sat, 29 Jun 2019 19:47:40 +0200 Subject: [PATCH] Improve recurrences --- app/Factory/RecurrenceFactory.php | 22 ++- .../Recurring/CreateController.php | 12 +- app/Http/Requests/RecurrenceFormRequest.php | 6 +- .../Recurring/RecurringRepository.php | 10 +- .../RecurringRepositoryInterface.php | 3 +- .../Support/RecurringTransactionTrait.php | 2 +- app/Support/ExpandedForm.php | 132 +++++++++++++++++- config/twigbridge.php | 6 +- public/v1/js/ff/recurring/create.js | 25 +++- resources/lang/en_US/firefly.php | 3 + resources/lang/en_US/form.php | 5 +- resources/views/v1/recurring/create.twig | 10 +- routes/web.php | 10 +- 13 files changed, 210 insertions(+), 36 deletions(-) diff --git a/app/Factory/RecurrenceFactory.php b/app/Factory/RecurrenceFactory.php index 4467a412f8..acfe1aa586 100644 --- a/app/Factory/RecurrenceFactory.php +++ b/app/Factory/RecurrenceFactory.php @@ -31,6 +31,7 @@ use FireflyIII\Models\Recurrence; use FireflyIII\Services\Internal\Support\RecurringTransactionTrait; use FireflyIII\Services\Internal\Support\TransactionTypeTrait; use FireflyIII\User; +use Illuminate\Support\MessageBag; use Log; /** @@ -41,6 +42,9 @@ class RecurrenceFactory /** @var User */ private $user; + /** @var MessageBag */ + private $errors; + use TransactionTypeTrait, RecurringTransactionTrait; /** @@ -52,22 +56,25 @@ class RecurrenceFactory if ('testing' === config('app.env')) { Log::warning(sprintf('%s should not be instantiated in the TEST environment!', get_class($this))); } + $this->errors = new MessageBag; } /** * @param array $data * * @return Recurrence + * @throws FireflyException */ - public function create(array $data): ?Recurrence + public function create(array $data): Recurrence { try { $type = $this->findTransactionType(ucfirst($data['recurrence']['type'])); } catch (FireflyException $e) { - Log::error(sprintf('Cannot make a recurring transaction of type "%s"', $data['recurrence']['type'])); - Log::error($e->getMessage()); + $message = sprintf('Cannot make a recurring transaction of type "%s"', $data['recurrence']['type']); + Log::error($message); + Log::error($e->getTraceAsString()); - return null; + throw new FireflyException($message); } /** @var Carbon $firstDate */ $firstDate = $data['recurrence']['first_date']; @@ -95,9 +102,14 @@ class RecurrenceFactory $this->createTransactions($recurrence, $data['transactions'] ?? []); // @codeCoverageIgnoreStart } catch (FireflyException $e) { - // TODO make sure error props to the user. Log::error($e->getMessage()); + $recurrence->forceDelete(); + $message = sprintf('Could not create recurring transaction: %s', $e->getMessage()); + $this->errors->add('store', $message); + throw new FireflyException($message); + } + // @codeCoverageIgnoreEnd return $recurrence; diff --git a/app/Http/Controllers/Recurring/CreateController.php b/app/Http/Controllers/Recurring/CreateController.php index 88e5abd5a4..4a946fadc3 100644 --- a/app/Http/Controllers/Recurring/CreateController.php +++ b/app/Http/Controllers/Recurring/CreateController.php @@ -25,6 +25,7 @@ namespace FireflyIII\Http\Controllers\Recurring; use Carbon\Carbon; +use FireflyIII\Exceptions\FireflyException; use FireflyIII\Http\Controllers\Controller; use FireflyIII\Http\Requests\RecurrenceFormRequest; use FireflyIII\Models\RecurrenceRepetition; @@ -71,7 +72,6 @@ class CreateController extends Controller * @param Request $request * * @return \Illuminate\Contracts\View\Factory|\Illuminate\View\View - * @SuppressWarnings(PHPMD.CyclomaticComplexity) */ public function create(Request $request) { @@ -120,12 +120,16 @@ class CreateController extends Controller * @param RecurrenceFormRequest $request * * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector - * @throws \FireflyIII\Exceptions\FireflyException */ public function store(RecurrenceFormRequest $request) { - $data = $request->getAll(); - $recurrence = $this->recurring->store($data); + $data = $request->getAll(); + try { + $recurrence = $this->recurring->store($data); + } catch (FireflyException $e) { + session()->flash('error', $e->getMessage()); + return redirect(route('recurring.create'))->withInput(); + } $request->session()->flash('success', (string)trans('firefly.stored_new_recurrence', ['title' => $recurrence->title])); app('preferences')->mark(); diff --git a/app/Http/Requests/RecurrenceFormRequest.php b/app/Http/Requests/RecurrenceFormRequest.php index 1e1bc4d7bf..24cd086e7e 100644 --- a/app/Http/Requests/RecurrenceFormRequest.php +++ b/app/Http/Requests/RecurrenceFormRequest.php @@ -120,11 +120,11 @@ class RecurrenceFormRequest extends Request default: throw new FireflyException(sprintf('Cannot handle transaction type "%s"', $this->string('transaction_type'))); // @codeCoverageIgnore case 'withdrawal': - $return['transactions'][0]['source_id'] = $this->integer('source_id'); - $return['transactions'][0]['destination_name'] = $this->string('destination_name'); + $return['transactions'][0]['source_id'] = $this->integer('source_id'); + $return['transactions'][0]['destination_id'] = $this->integer('withdrawal_destination_id'); break; case 'deposit': - $return['transactions'][0]['source_name'] = $this->string('source_name'); + $return['transactions'][0]['source_id'] = $this->integer('deposit_source_id'); $return['transactions'][0]['destination_id'] = $this->integer('destination_id'); break; case 'transfer': diff --git a/app/Repositories/Recurring/RecurringRepository.php b/app/Repositories/Recurring/RecurringRepository.php index 3a1f65d303..e6074daad6 100644 --- a/app/Repositories/Recurring/RecurringRepository.php +++ b/app/Repositories/Recurring/RecurringRepository.php @@ -429,13 +429,19 @@ class RecurringRepository implements RecurringRepositoryInterface * @param array $data * * @return Recurrence + * @throws FireflyException */ public function store(array $data): Recurrence { - $factory = new RecurrenceFactory; + /** @var RecurrenceFactory $factory */ + $factory = app(RecurrenceFactory::class); $factory->setUser($this->user); + $result = $factory->create($data); + if (null === $result) { + throw new FireflyException($factory->getErrors()->first()); + } - return $factory->create($data); + return $result; } /** diff --git a/app/Repositories/Recurring/RecurringRepositoryInterface.php b/app/Repositories/Recurring/RecurringRepositoryInterface.php index be8648f43b..15e89f53d3 100644 --- a/app/Repositories/Recurring/RecurringRepositoryInterface.php +++ b/app/Repositories/Recurring/RecurringRepositoryInterface.php @@ -176,10 +176,9 @@ interface RecurringRepositoryInterface /** * Store a new recurring transaction. - *\ * * @param array $data - * + * @throws FireflyException * @return Recurrence */ public function store(array $data): Recurrence; diff --git a/app/Services/Internal/Support/RecurringTransactionTrait.php b/app/Services/Internal/Support/RecurringTransactionTrait.php index 9f19e51a3d..452f308213 100644 --- a/app/Services/Internal/Support/RecurringTransactionTrait.php +++ b/app/Services/Internal/Support/RecurringTransactionTrait.php @@ -154,7 +154,7 @@ trait RecurringTransactionTrait } if (!$validator->validateDestination($destination->id, null)) { - throw new FireflyException(sprintf('Destination invalid: %s', $validator->sourceError)); // @codeCoverageIgnore + throw new FireflyException(sprintf('Destination invalid: %s', $validator->destError)); // @codeCoverageIgnore } $transaction = new RecurrenceTransaction( diff --git a/app/Support/ExpandedForm.php b/app/Support/ExpandedForm.php index feb4b65f39..6bd00d27a2 100644 --- a/app/Support/ExpandedForm.php +++ b/app/Support/ExpandedForm.php @@ -118,7 +118,7 @@ class ExpandedForm $balance = app('steam')->balance($account, new Carbon); $currencyId = (int)$repository->getMetaValue($account, 'currency_id'); $currency = $currencyRepos->findNull($currencyId); - $role = $repository->getMetaValue($account, 'accountRole'); + $role = $repository->getMetaValue($account, 'account_role'); if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) { $role = 'no_account_type'; // @codeCoverageIgnore } @@ -138,6 +138,136 @@ class ExpandedForm return $this->select($name, $grouped, $value, $options); } + /** + * Grouped dropdown list of all accounts that are valid as the destination of a withdrawal. + * + * @param string $name + * @param mixed $value + * @param array $options + * + * @return string + */ + public function activeWithdrawalDestinations(string $name, $value = null, array $options = null): string + { + // make repositories + /** @var AccountRepositoryInterface $repository */ + $repository = app(AccountRepositoryInterface::class); + /** @var CurrencyRepositoryInterface $currencyRepos */ + $currencyRepos = app(CurrencyRepositoryInterface::class); + + $accountList = $repository->getActiveAccountsByType( + [ + AccountType::MORTGAGE, + AccountType::DEBT, + AccountType::CREDITCARD, + AccountType::LOAN, + AccountType::EXPENSE, + ] + ); + $liabilityTypes = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN]; + $defaultCurrency = app('amount')->getDefaultCurrency(); + $grouped = []; + + // add cash account first: + $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, new Carbon); + $currencyId = (int)$repository->getMetaValue($account, 'currency_id'); + $currency = $currencyRepos->findNull($currencyId); + $role = (string)$repository->getMetaValue($account, 'account_role'); + if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) { + $role = 'no_account_type'; // @codeCoverageIgnore + } + if ('no_account_type' === $role && AccountType::EXPENSE === $account->accountType->type) { + $role = 'expense_account'; // @codeCoverageIgnore + + } + + if (in_array($account->accountType->type, $liabilityTypes, true)) { + $role = 'l_' . $account->accountType->type; // @codeCoverageIgnore + } + + if (null === $currency) { + $currency = $defaultCurrency; + } + + $key = (string)trans('firefly.opt_group_' . $role); + $grouped[$key][$account->id] = $account->name . ' (' . app('amount')->formatAnything($currency, $balance, false) . ')'; + } + + return $this->select($name, $grouped, $value, $options); + } + + /** + * Grouped dropdown list of all accounts that are valid as the destination of a withdrawal. + * + * @param string $name + * @param mixed $value + * @param array $options + * + * @return string + */ + public function activeDepositDestinations(string $name, $value = null, array $options = null): string + { + // make repositories + /** @var AccountRepositoryInterface $repository */ + $repository = app(AccountRepositoryInterface::class); + /** @var CurrencyRepositoryInterface $currencyRepos */ + $currencyRepos = app(CurrencyRepositoryInterface::class); + + $accountList = $repository->getActiveAccountsByType( + [ + AccountType::MORTGAGE, + AccountType::DEBT, + AccountType::CREDITCARD, + AccountType::LOAN, + AccountType::REVENUE, + ] + ); + $liabilityTypes = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN]; + $defaultCurrency = app('amount')->getDefaultCurrency(); + $grouped = []; + + // add cash account first: + $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, new Carbon); + $currencyId = (int)$repository->getMetaValue($account, 'currency_id'); + $currency = $currencyRepos->findNull($currencyId); + $role = (string)$repository->getMetaValue($account, 'account_role'); + if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) { + $role = 'no_account_type'; // @codeCoverageIgnore + } + if ('no_account_type' === $role && AccountType::REVENUE === $account->accountType->type) { + $role = 'revenue_account'; // @codeCoverageIgnore + + } + + if (in_array($account->accountType->type, $liabilityTypes, true)) { + $role = 'l_' . $account->accountType->type; // @codeCoverageIgnore + } + + if (null === $currency) { + $currency = $defaultCurrency; + } + + $key = (string)trans('firefly.opt_group_' . $role); + $grouped[$key][$account->id] = $account->name . ' (' . app('amount')->formatAnything($currency, $balance, false) . ')'; + } + + return $this->select($name, $grouped, $value, $options); + } + /** * @param string $name * @param mixed $value diff --git a/config/twigbridge.php b/config/twigbridge.php index 7941bef37c..02f8f8c206 100644 --- a/config/twigbridge.php +++ b/config/twigbridge.php @@ -197,7 +197,11 @@ return [ 'is_safe' => ['date', 'text', 'select', 'balance', 'optionsList', 'checkbox', 'amount', 'tags', 'integer', 'textarea', 'location', 'file', 'staticText', 'password', 'nonSelectableAmount', 'number', 'assetAccountList', 'amountNoCurrency', 'currencyList', 'ruleGroupList', 'assetAccountCheckList', 'ruleGroupListWithEmpty', 'piggyBankList', 'currencyListEmpty', - 'activeAssetAccountList', 'percentage', 'activeLongAccountList', 'longAccountList','balanceAll'],], + 'activeAssetAccountList', 'percentage', 'activeLongAccountList', 'longAccountList','balanceAll', + 'activeWithdrawalDestinations','activeDepositDestinations' + + + ],], 'Form' => ['is_safe' => ['input', 'select', 'checkbox', 'model', 'open', 'radio', 'textarea', 'file',], ], ], diff --git a/public/v1/js/ff/recurring/create.js b/public/v1/js/ff/recurring/create.js index 0df274889d..8eb77f162c 100644 --- a/public/v1/js/ff/recurring/create.js +++ b/public/v1/js/ff/recurring/create.js @@ -183,7 +183,8 @@ function updateFormFields() { if (transactionType === 'withdrawal') { // hide source account name: - $('#source_name_holder').hide(); + // $('#source_name_holder').hide(); // source_name no longer exists + $('#deposit_source_id_holder').hide(); // new one! // show source account ID: $('#source_id_holder').show(); @@ -192,7 +193,8 @@ function updateFormFields() { $('#destination_id_holder').hide(); // show destination name: - $('#destination_name_holder').show(); + //$('#destination_name_holder').show(); // old one + $('#withdrawal_destination_id_holder').show(); // show budget $('#budget_id_holder').show(); @@ -202,18 +204,29 @@ function updateFormFields() { } if (transactionType === 'deposit') { - $('#source_name_holder').show(); + // $('#source_name_holder').show(); // source name no longer exists. + $('#deposit_source_id_holder').show(); // new one! + $('#source_id_holder').hide(); - $('#destination_name_holder').hide(); + + // $('#destination_name_holder').hide(); // old one + $('#withdrawal_destination_id_holder').hide(); + + $('#destination_id_holder').show(); $('#budget_id_holder').hide(); $('#piggy_bank_id_holder').hide(); } if (transactionType === 'transfer') { - $('#source_name_holder').hide(); + // $('#source_name_holder').hide(); // source name no longer exists. + $('#deposit_source_id_holder').hide(); // new one! + $('#source_id_holder').show(); - $('#destination_name_holder').hide(); + + // $('#destination_name_holder').hide(); // old one + $('#withdrawal_destination_id_holder').hide(); + $('#destination_id_holder').show(); $('#budget_id_holder').hide(); $('#piggy_bank_id_holder').show(); diff --git a/resources/lang/en_US/firefly.php b/resources/lang/en_US/firefly.php index b850760b28..06ffab34e4 100644 --- a/resources/lang/en_US/firefly.php +++ b/resources/lang/en_US/firefly.php @@ -758,6 +758,7 @@ return [ 'reconcile_options' => 'Reconciliation options', 'reconcile_range' => 'Reconciliation range', 'start_reconcile' => 'Start reconciling', + 'cash_account_type' => 'Cash', 'cash' => 'cash', 'account_type' => 'Account type', 'save_transactions_by_moving' => 'Save these transaction(s) by moving them to another account:', @@ -857,6 +858,8 @@ return [ 'opt_group_sharedAsset' => 'Shared asset accounts', 'opt_group_ccAsset' => 'Credit cards', 'opt_group_cashWalletAsset' => 'Cash wallets', + 'opt_group_expense_account' => 'Expense accounts', + 'opt_group_revenue_account' => 'Revenue accounts', 'opt_group_l_Loan' => 'Liability: Loan', 'opt_group_l_Debt' => 'Liability: Debt', 'opt_group_l_Mortgage' => 'Liability: Mortgage', diff --git a/resources/lang/en_US/form.php b/resources/lang/en_US/form.php index cee7f9c065..661ac8b976 100644 --- a/resources/lang/en_US/form.php +++ b/resources/lang/en_US/form.php @@ -66,7 +66,7 @@ return [ 'opening_balance' => 'Opening balance', 'tagMode' => 'Tag mode', 'tag_position' => 'Tag location', - 'virtual_balance' => 'Virtual balance', + 'virtual_balance' => 'Virtual balance', 'targetamount' => 'Target amount', 'account_role' => 'Account role', 'opening_balance_date' => 'Opening balance date', @@ -253,4 +253,7 @@ return [ 'weekend' => 'Weekend', 'client_secret' => 'Client secret', + 'withdrawal_destination_id' => 'Destination account', + 'deposit_source_id' => 'Source account', + ]; diff --git a/resources/views/v1/recurring/create.twig b/resources/views/v1/recurring/create.twig index c74d674995..654aecc5da 100644 --- a/resources/views/v1/recurring/create.twig +++ b/resources/views/v1/recurring/create.twig @@ -93,13 +93,19 @@ {{ ExpandedForm.activeLongAccountList('source_id', null, {label: trans('form.asset_source_account')}) }} {# source account name for deposits: #} - {{ ExpandedForm.text('source_name', null, {label: trans('form.revenue_account')}) }} + {#{{ ExpandedForm.text('source_name', null, {label: trans('form.revenue_account')}) }}#} + + {# for deposits, a drop down with revenue accounts, loan debt cash and mortgage #} + {{ ExpandedForm.activeDepositDestinations('deposit_source_id', null, {label: trans('form.deposit_source_id')}) }} {# destination if deposit or transfer: #} {{ ExpandedForm.activeLongAccountList('destination_id', null, {label: trans('form.asset_destination_account')} ) }} {# destination account name for withdrawals #} - {{ ExpandedForm.text('destination_name', null, {label: trans('form.expense_account')}) }} + {#{{ ExpandedForm.text('destination_name', null, {label: trans('form.expense_account')}) }} #} + + {# for withdrawals, also a drop down with expense accounts, loans, debts, mortgages or (cash). #} + {{ ExpandedForm.activeWithdrawalDestinations('withdrawal_destination_id', null, {label: trans('form.withdrawal_destination_id')}) }} diff --git a/routes/web.php b/routes/web.php index a909ded271..050373120a 100644 --- a/routes/web.php +++ b/routes/web.php @@ -532,18 +532,14 @@ Route::group( // for auto complete Route::get('accounts', ['uses' => 'Json\AutoCompleteController@accounts', 'as' => 'autocomplete.accounts']); - Route::get('currencies', ['uses' => 'Json\AutoCompleteController@currencies', 'as' => 'autocomplete.currencies']); Route::get('budgets', ['uses' => 'Json\AutoCompleteController@budgets', 'as' => 'autocomplete.budgets']); Route::get('categories', ['uses' => 'Json\AutoCompleteController@categories', 'as' => 'autocomplete.categories']); + Route::get('currencies', ['uses' => 'Json\AutoCompleteController@currencies', 'as' => 'autocomplete.currencies']); Route::get('piggy-banks', ['uses' => 'Json\AutoCompleteController@piggyBanks', 'as' => 'autocomplete.piggy-banks']); Route::get('tags', ['uses' => 'Json\AutoCompleteController@tags', 'as' => 'autocomplete.tags']); - // TODO improve 3 routes: - //Route::get('transaction-journals/all', ['uses' => 'Json\AutoCompleteController@allTransactionJournals', 'as' => 'all-transaction-journals']); - //Route::get('transaction-journals/with-id/{tj}', ['uses' => 'Json\AutoCompleteController@journalsWithId', 'as' => 'journals-with-id']); - //Route::get('transaction-journals/{what}', ['uses' => 'Json\AutoCompleteController@transactionJournals', 'as' => 'transaction-journals']); - // TODO end of improvement + Route::get('transaction-types', ['uses' => 'Json\AutoCompleteController@transactionTypes', 'as' => 'transaction-types']); // boxes @@ -567,8 +563,6 @@ Route::group( Route::post('intro/enable/{route}/{specificPage?}', ['uses' => 'Json\IntroController@postEnable', 'as' => 'intro.enable']); Route::get('intro/{route}/{specificPage?}', ['uses' => 'Json\IntroController@getIntroSteps', 'as' => 'intro']); - //Route::get('/{subject}', ['uses' => 'Json\AutoCompleteController@autoComplete', 'as' => 'autocomplete']); - } );