From ea4be9dd0cc4927e1551df9c729779af414a6702 Mon Sep 17 00:00:00 2001 From: James Cole Date: Fri, 6 Dec 2024 08:10:31 +0100 Subject: [PATCH] Expand forms and improve validation for multi-account piggy banks --- .../Models/PiggyBank/StoreRequest.php | 49 ++++++++++++++++--- app/Factory/PiggyBankFactory.php | 16 +++--- .../PiggyBank/CreateController.php | 5 +- app/Http/Requests/PiggyBankStoreRequest.php | 29 +++++++---- app/Models/AccountType.php | 26 +++++----- .../PiggyBank/ModifiesPiggyBanks.php | 4 +- .../PiggyBank/PiggyBankRepository.php | 4 +- .../Support/RecurringTransactionTrait.php | 6 +-- app/Support/Form/AccountForm.php | 18 ++++++- app/Support/Form/FormSupport.php | 19 +++++++ config/twigbridge.php | 1 + resources/lang/en_US/firefly.php | 1 + resources/lang/en_US/form.php | 7 +-- resources/lang/en_US/validation.php | 2 + resources/views/form/multi-select.twig | 10 ++++ resources/views/piggy-banks/create.twig | 9 ++-- 16 files changed, 149 insertions(+), 57 deletions(-) create mode 100644 resources/views/form/multi-select.twig diff --git a/app/Api/V1/Requests/Models/PiggyBank/StoreRequest.php b/app/Api/V1/Requests/Models/PiggyBank/StoreRequest.php index d73abc7b8c..2566cfe8ba 100644 --- a/app/Api/V1/Requests/Models/PiggyBank/StoreRequest.php +++ b/app/Api/V1/Requests/Models/PiggyBank/StoreRequest.php @@ -24,8 +24,11 @@ declare(strict_types=1); namespace FireflyIII\Api\V1\Requests\Models\PiggyBank; +use FireflyIII\Exceptions\FireflyException; +use FireflyIII\Models\TransactionCurrency; use FireflyIII\Repositories\Account\AccountRepositoryInterface; use FireflyIII\Rules\IsValidPositiveAmount; +use FireflyIII\Rules\IsValidZeroOrMoreAmount; use FireflyIII\Support\Request\ChecksLogin; use FireflyIII\Support\Request\ConvertsDataTypes; use Illuminate\Foundation\Http\FormRequest; @@ -73,27 +76,30 @@ class StoreRequest extends FormRequest 'accounts' => 'required', 'accounts.*' => 'array|required', 'accounts.*.account_id' => 'required|numeric|belongsToUser:accounts,id', - 'accounts.*.current_amount' => ['numeric', new IsValidPositiveAmount()], + 'accounts.*.current_amount' => ['numeric', new IsValidZeroOrMoreAmount()], 'object_group_id' => 'numeric|belongsToUser:object_groups,id', 'object_group_title' => ['min:1', 'max:255'], - 'target_amount' => ['required', new IsValidPositiveAmount()], + 'target_amount' => ['required', new IsValidZeroOrMoreAmount()], 'start_date' => 'date|nullable', - 'transaction_currency_id' => 'exists:transaction_currencies,id', - 'transaction_currency_code' => 'exists:transaction_currencies,code', + 'transaction_currency_id' => 'exists:transaction_currencies,id|required_without:transaction_currency_code', + 'transaction_currency_code' => 'exists:transaction_currencies,code|required_without:transaction_currency_id', 'target_date' => 'date|nullable|after:start_date', 'notes' => 'max:65000', ]; } /** - * Can only store money on liabilities and asset accouns. + * Can only store money on liabilities and asset accounts. */ public function withValidator(Validator $validator): void { $validator->after( function (Validator $validator): void { // validate start before end only if both are there. - $data = $validator->getData(); + $data = $validator->getData(); + $currency = $this->getCurrencyFromData($data); + $targetAmount = (string) ($data['target_amount'] ?? '0'); + $currentAmount = '0'; if (array_key_exists('accounts', $data) && is_array($data['accounts'])) { $repository = app(AccountRepositoryInterface::class); $types = config('firefly.piggy_bank_account_types'); @@ -101,6 +107,13 @@ class StoreRequest extends FormRequest $accountId = (int) ($array['account_id'] ?? 0); $account = $repository->find($accountId); if (null !== $account) { + // check currency here. + $accountCurrency = $repository->getAccountCurrency($account); + $isMultiCurrency = $repository->getMetaValue($account, 'is_multi_currency'); + $currentAmount = bcadd($currentAmount, (string)($array['current_amount'] ?? '0')); + if ($accountCurrency->id !== $currency->id && 'true' !== $isMultiCurrency) { + $validator->errors()->add(sprintf('accounts.%d', $index), trans('validation.invalid_account_currency')); + } $type = $account->accountType->type; if (!in_array($type, $types, true)) { $validator->errors()->add(sprintf('accounts.%d', $index), trans('validation.invalid_account_type')); @@ -108,6 +121,9 @@ class StoreRequest extends FormRequest } } } + if(bccomp($targetAmount, $currentAmount) === -1 && bccomp($targetAmount, '0') === 1) { + $validator->errors()->add('target_amount', trans('validation.current_amount_too_much')); + } } ); if ($validator->fails()) { @@ -126,10 +142,27 @@ class StoreRequest extends FormRequest continue; } $return[] = [ - 'account_id' => $this->integerFromValue((string)($entry['account_id'] ?? '0')), - 'current_amount' => $this->clearString($entry['current_amount'] ?? '0'), + 'account_id' => $this->integerFromValue((string) ($entry['account_id'] ?? '0')), + 'current_amount' => $this->clearString((string) ($entry['current_amount'] ?? '0')), ]; } return $return; } + + private function getCurrencyFromData(array $data): TransactionCurrency + { + if (array_key_exists('transaction_currency_code', $data) && '' !== (string) $data['transaction_currency_code']) { + $currency = TransactionCurrency::whereCode($data['transaction_currency_code'])->first(); + if (null !== $currency) { + return $currency; + } + } + if (array_key_exists('transaction_currency_id', $data) && '' !== (string) $data['transaction_currency_id']) { + $currency = TransactionCurrency::find((int) $data['transaction_currency_id']); + if (null !== $currency) { + return $currency; + } + } + throw new FireflyException('Unexpected empty currency.'); + } } diff --git a/app/Factory/PiggyBankFactory.php b/app/Factory/PiggyBankFactory.php index 7f9def7600..cd41ee1deb 100644 --- a/app/Factory/PiggyBankFactory.php +++ b/app/Factory/PiggyBankFactory.php @@ -37,7 +37,11 @@ use Illuminate\Database\QueryException; */ class PiggyBankFactory { - private User $user; + public User $user { + set(User $value) { + $this->user = $value; + } + } private CurrencyRepositoryInterface $currencyRepository; private AccountRepositoryInterface $accountRepository; private PiggyBankRepositoryInterface $piggyBankRepository; @@ -138,11 +142,6 @@ class PiggyBankFactory return $this->user->piggyBanks()->where('piggy_banks.name', $name)->first(); } - public function setUser(User $user): void - { - $this->user = $user; - } - private function getCurrency(array $data): TransactionCurrency { // currency: $defaultCurrency = app('amount')->getDefaultCurrency(); @@ -197,7 +196,8 @@ class PiggyBankFactory private function getMaxOrder(): int { - return (int)$this->user->piggyBanks()->max('piggy_banks.order'); + return (int) $this->piggyBankRepository->getPiggyBanks()->max('order'); + } private function linkToAccountIds(PiggyBank $piggyBank, array $accounts): void { @@ -207,7 +207,7 @@ class PiggyBankFactory if(null === $account) { continue; } - $piggyBank->accounts()->syncWithoutDetaching([$account->id, ['current_amount' => $info['current_amount'] ?? '0']]); + $piggyBank->accounts()->syncWithoutDetaching([$account->id => ['current_amount' => $info['current_amount'] ?? '0']]); } } } diff --git a/app/Http/Controllers/PiggyBank/CreateController.php b/app/Http/Controllers/PiggyBank/CreateController.php index 92aac833d9..a0626b5c3b 100644 --- a/app/Http/Controllers/PiggyBank/CreateController.php +++ b/app/Http/Controllers/PiggyBank/CreateController.php @@ -92,10 +92,11 @@ class CreateController extends Controller public function store(PiggyBankStoreRequest $request) { $data = $request->getPiggyBankData(); - if (null === $data['startdate']) { - $data['startdate'] = today(config('app.timezone')); + if (null === $data['start_date']) { + $data['start_date'] = today(config('app.timezone')); } $piggyBank = $this->piggyRepos->store($data); + var_dump($data);exit; session()->flash('success', (string)trans('firefly.stored_piggy_bank', ['name' => $piggyBank->name])); app('preferences')->mark(); diff --git a/app/Http/Requests/PiggyBankStoreRequest.php b/app/Http/Requests/PiggyBankStoreRequest.php index 94087fedb8..467cfe607e 100644 --- a/app/Http/Requests/PiggyBankStoreRequest.php +++ b/app/Http/Requests/PiggyBankStoreRequest.php @@ -43,15 +43,21 @@ class PiggyBankStoreRequest extends FormRequest */ public function getPiggyBankData(): array { - return [ + $data = [ 'name' => $this->convertString('name'), - 'startdate' => $this->getCarbonDate('startdate'), - 'account_id' => $this->convertInteger('account_id'), - 'targetamount' => $this->convertString('targetamount'), - 'targetdate' => $this->getCarbonDate('targetdate'), + 'start_date' => $this->getCarbonDate('start_date'), + //'account_id' => $this->convertInteger('account_id'), + 'accounts' => $this->get('accounts'), + 'target_amount' => $this->convertString('target_amount'), + 'target_date' => $this->getCarbonDate('target_date'), 'notes' => $this->stringWithNewlines('notes'), 'object_group_title' => $this->convertString('object_group'), ]; + if(!is_array($data['accounts'])) { + $data['accounts'] = []; + } + + return $data; } /** @@ -61,10 +67,11 @@ class PiggyBankStoreRequest extends FormRequest { return [ 'name' => 'required|min:1|max:255|uniquePiggyBankForUser', - 'account_id' => 'required|belongsToUser:accounts', - 'targetamount' => ['nullable', new IsValidPositiveAmount()], - 'startdate' => 'date', - 'targetdate' => 'date|nullable', + 'accounts' => 'required|array', + 'accounts.*' => 'required|belongsToUser:accounts', + 'target_amount' => ['nullable', new IsValidPositiveAmount()], + 'start_date' => 'date', + 'target_date' => 'date|nullable', 'order' => 'integer|min:1', 'object_group' => 'min:0|max:255', 'notes' => 'min:1|max:32768|nullable', @@ -73,6 +80,10 @@ class PiggyBankStoreRequest extends FormRequest public function withValidator(Validator $validator): void { + // need to have more than one account. + // accounts need to have the same currency or be multi-currency(?). + + if ($validator->fails()) { Log::channel('audit')->error(sprintf('Validation errors in %s', __CLASS__), $validator->errors()->toArray()); } diff --git a/app/Models/AccountType.php b/app/Models/AccountType.php index 0ef77adcc2..c8bc76b399 100644 --- a/app/Models/AccountType.php +++ b/app/Models/AccountType.php @@ -35,46 +35,46 @@ class AccountType extends Model { use ReturnsIntegerIdTrait; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string ASSET = 'Asset account'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string BENEFICIARY = 'Beneficiary account'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string CASH = 'Cash account'; #[\Deprecated] /** @deprecated */ public const string CREDITCARD = 'Credit card'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string DEBT = 'Debt'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string DEFAULT = 'Default account'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string EXPENSE = 'Expense account'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string IMPORT = 'Import account'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string INITIAL_BALANCE = 'Initial balance account'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string LIABILITY_CREDIT = 'Liability credit account'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string LOAN = 'Loan'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string MORTGAGE = 'Mortgage'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string RECONCILIATION = 'Reconciliation account'; - #[\Deprecated] + #[\Deprecated] /** @deprecated */ public const string REVENUE = 'Revenue account'; protected $casts diff --git a/app/Repositories/PiggyBank/ModifiesPiggyBanks.php b/app/Repositories/PiggyBank/ModifiesPiggyBanks.php index 09169ef898..f37bc9e1a2 100644 --- a/app/Repositories/PiggyBank/ModifiesPiggyBanks.php +++ b/app/Repositories/PiggyBank/ModifiesPiggyBanks.php @@ -182,8 +182,8 @@ trait ModifiesPiggyBanks */ public function store(array $data): PiggyBank { - $factory = new PiggyBankFactory(); - $factory->setUser($this->user); + $factory = new PiggyBankFactory(); + $factory->user = $this->user; return $factory->store($data); } diff --git a/app/Repositories/PiggyBank/PiggyBankRepository.php b/app/Repositories/PiggyBank/PiggyBankRepository.php index cf73589b94..bd8c78de52 100644 --- a/app/Repositories/PiggyBank/PiggyBankRepository.php +++ b/app/Repositories/PiggyBank/PiggyBankRepository.php @@ -356,8 +356,8 @@ class PiggyBankRepository implements PiggyBankRepositoryInterface #[\Override] public function resetOrder(): void { - $factory = new PiggyBankFactory(); - $factory->setUser($this->user); + $factory = new PiggyBankFactory(); + $factory->user = $this->user; $factory->resetOrder(); } } diff --git a/app/Services/Internal/Support/RecurringTransactionTrait.php b/app/Services/Internal/Support/RecurringTransactionTrait.php index 438563c98b..1ca7f701c2 100644 --- a/app/Services/Internal/Support/RecurringTransactionTrait.php +++ b/app/Services/Internal/Support/RecurringTransactionTrait.php @@ -283,9 +283,9 @@ trait RecurringTransactionTrait protected function updatePiggyBank(RecurrenceTransaction $transaction, int $piggyId): void { /** @var PiggyBankFactory $factory */ - $factory = app(PiggyBankFactory::class); - $factory->setUser($transaction->recurrence->user); - $piggyBank = $factory->find($piggyId, null); + $factory = app(PiggyBankFactory::class); + $factory->user = $transaction->recurrence->user; + $piggyBank = $factory->find($piggyId, null); if (null !== $piggyBank) { /** @var null|RecurrenceMeta $entry */ $entry = $transaction->recurrenceTransactionMeta()->where('name', 'piggy_bank_id')->first(); diff --git a/app/Support/Form/AccountForm.php b/app/Support/Form/AccountForm.php index b7bc140a79..8034bf93e6 100644 --- a/app/Support/Form/AccountForm.php +++ b/app/Support/Form/AccountForm.php @@ -24,6 +24,7 @@ declare(strict_types=1); namespace FireflyIII\Support\Form; +use FireflyIII\Enums\AccountTypeEnum; use FireflyIII\Exceptions\FireflyException; use FireflyIII\Models\Account; use FireflyIII\Models\AccountType; @@ -141,12 +142,25 @@ class AccountForm */ public function assetAccountList(string $name, $value = null, ?array $options = null): string { - $types = [AccountType::ASSET, AccountType::DEFAULT]; + $types = [AccountTypeEnum::ASSET->value, AccountTypeEnum::DEFAULT->value]; $grouped = $this->getAccountsGrouped($types); return $this->select($name, $grouped, $value, $options); } + /** + * Basic list of asset accounts. + * + * @param mixed $value + */ + public function assetLiabilityMultiAccountList(string $name, $value = null, ?array $options = null): string + { + $types = [AccountTypeEnum::ASSET->value, AccountTypeEnum::DEFAULT->value, AccountTypeEnum::MORTGAGE->value, AccountTypeEnum::DEBT->value,AccountTypeEnum::LOAN->value]; + $grouped = $this->getAccountsGrouped($types); + + return $this->multiSelect($name, $grouped, $value, $options); + } + /** * Same list but all liabilities as well. * @@ -154,7 +168,7 @@ 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]; + $types = [AccountType::ASSET, AccountType::DEFAULT, AccountType::MORTGAGE, AccountType::DEBT, AccountType::LOAN]; $grouped = $this->getAccountsGrouped($types); return $this->select($name, $grouped, $value, $options); diff --git a/app/Support/Form/FormSupport.php b/app/Support/Form/FormSupport.php index f91931cd60..09c8de4910 100644 --- a/app/Support/Form/FormSupport.php +++ b/app/Support/Form/FormSupport.php @@ -56,6 +56,25 @@ trait FormSupport return $html; } + public function multiSelect(string $name, ?array $list = null, $selected = null, ?array $options = null): string + { + $list ??= []; + $label = $this->label($name, $options); + $options = $this->expandOptionArray($name, $label, $options); + $classes = $this->getHolderClasses($name); + $selected = $this->fillFieldValue($name, $selected); + unset($options['autocomplete'], $options['placeholder']); + + try { + $html = view('form.multi-select', compact('classes', 'name', 'label', 'selected', 'options', 'list'))->render(); + } catch (\Throwable $e) { + app('log')->debug(sprintf('Could not render multi-select(): %s', $e->getMessage())); + $html = 'Could not render multi-select.'; + } + + return $html; + } + protected function label(string $name, ?array $options = null): string { $options ??= []; diff --git a/config/twigbridge.php b/config/twigbridge.php index da8d491d82..60d1e1023d 100644 --- a/config/twigbridge.php +++ b/config/twigbridge.php @@ -197,6 +197,7 @@ return [ 'assetAccountCheckList', 'assetAccountList', 'longAccountList', + 'assetLiabilityMultiAccountList', ], ], 'CurrencyForm' => [ diff --git a/resources/lang/en_US/firefly.php b/resources/lang/en_US/firefly.php index aa395cd3da..1f6c37af8e 100644 --- a/resources/lang/en_US/firefly.php +++ b/resources/lang/en_US/firefly.php @@ -2197,6 +2197,7 @@ return [ 'amount' => 'Amount', 'overview' => 'Overview', 'saveOnAccount' => 'Save on account', + 'saveOnAccounts' => 'Save on account(s)', 'unknown' => 'Unknown', 'monthly' => 'Monthly', 'profile' => 'Profile', diff --git a/resources/lang/en_US/form.php b/resources/lang/en_US/form.php index 5e6ab8d583..a83bbd764e 100644 --- a/resources/lang/en_US/form.php +++ b/resources/lang/en_US/form.php @@ -69,6 +69,7 @@ return [ // Ignore this comment 'targetamount' => 'Target amount', + 'target_amount' => 'Target amount', 'account_role' => 'Account role', 'opening_balance_date' => 'Opening balance date', 'cc_type' => 'Credit card payment plan', @@ -106,7 +107,9 @@ return [ 'deletePermanently' => 'Delete permanently', 'cancel' => 'Cancel', 'targetdate' => 'Target date', + 'target_date' => 'Target date', 'startdate' => 'Start date', + 'start_date' => 'Start date', 'tag' => 'Tag', 'under' => 'Under', 'symbol' => 'Symbol', @@ -116,7 +119,6 @@ return [ 'creditCardNumber' => 'Credit card number', 'has_headers' => 'Headers', 'date_format' => 'Date format', - 'specifix' => 'Bank- or file specific fixes', 'attachments[]' => 'Attachments', 'title' => 'Title', 'notes' => 'Notes', @@ -125,8 +127,7 @@ return [ 'size' => 'Size', 'trigger' => 'Trigger', 'stop_processing' => 'Stop processing', - 'start_date' => 'Start of range', - 'end_date' => 'End of range', + 'end_date' => 'End date', 'enddate' => 'End date', 'move_rules_before_delete' => 'Rule group', 'start' => 'Start of range', diff --git a/resources/lang/en_US/validation.php b/resources/lang/en_US/validation.php index 9f6dc2fe54..587db8cb29 100644 --- a/resources/lang/en_US/validation.php +++ b/resources/lang/en_US/validation.php @@ -26,6 +26,8 @@ declare(strict_types=1); return [ 'invalid_account_type' => 'A piggy bank can only be linked to asset accounts and liabilities', + 'invalid_account_currency' => 'This account does not use the currency you have selected', + 'current_amount_too_much' => 'The combined amount in "current_amount" cannot exceed the "target_amount".', 'filter_must_be_in' => 'Filter ":filter" must be one of: :values', 'filter_not_string' => 'Filter ":filter" is expected to be a string of text', 'bad_api_filter' => 'This API endpoint does not support ":filter" as a filter.', diff --git a/resources/views/form/multi-select.twig b/resources/views/form/multi-select.twig new file mode 100644 index 0000000000..b27359e41e --- /dev/null +++ b/resources/views/form/multi-select.twig @@ -0,0 +1,10 @@ +
+ + +
+ {{ Html.select(name~"[]", list, selected).id(options.id).class('form-control').attribute('multiple').attribute('autocomplete','off').attribute('spellcheck','false').attribute('placeholder', options.placeholder) }} + {% include 'form.help' %} + {% include 'form.feedback' %} + +
+
diff --git a/resources/views/piggy-banks/create.twig b/resources/views/piggy-banks/create.twig index f25dd211a8..84662207a4 100644 --- a/resources/views/piggy-banks/create.twig +++ b/resources/views/piggy-banks/create.twig @@ -8,7 +8,6 @@
-
@@ -16,10 +15,10 @@

{{ 'mandatoryFields'|_ }}

- {{ ExpandedForm.text('name') }} - {{ AccountForm.assetAccountList('account_id', null, {label: 'saveOnAccount'|_ }) }} - {{ ExpandedForm.amountNoCurrency('targetamount') }} + {{ AccountForm.assetLiabilityMultiAccountList('accounts', null, {label: 'saveOnAccounts'|_ }) }} + + {{ ExpandedForm.amountNoCurrency('target_amount') }}
@@ -30,7 +29,7 @@

{{ 'optionalFields'|_ }}

- {{ ExpandedForm.date('targetdate') }} + {{ ExpandedForm.date('target_date') }} {{ ExpandedForm.textarea('notes', null, {helpText: trans('firefly.field_supports_markdown')} ) }} {{ ExpandedForm.file('attachments[]', {'multiple': 'multiple','helpText': trans('firefly.upload_max_file_size', {'size': uploadSize|filesize}) }) }} {{ ExpandedForm.objectGroup() }}