diff --git a/app/Validation/Account/DepositValidation.php b/app/Validation/Account/DepositValidation.php index 17bd46b676..f78f74dfae 100644 --- a/app/Validation/Account/DepositValidation.php +++ b/app/Validation/Account/DepositValidation.php @@ -70,7 +70,7 @@ trait DepositValidation } if (null !== $search) { Log::debug(sprintf('findExistingAccount() returned #%d ("%s"), so the result is true.', $search->id, $search->name)); - $this->destination = $search; + $this->setDestination($search); $result = true; } } @@ -107,7 +107,7 @@ trait DepositValidation $accountNumber = array_key_exists('number', $array) ? $array['number'] : null; Log::debug('Now in validateDepositSource', $array); - // null = we found nothing at all or didnt even search + // null = we found nothing at all or didn't even search // false = invalid results $result = null; @@ -129,6 +129,11 @@ trait DepositValidation Log::debug(sprintf('User submitted an ID (#%d), which is a "%s", so this is not a valid source.', $accountId, $search->accountType->type)); Log::debug(sprintf('Firefly III accepts ID #%d as valid account data.', $accountId)); } + if (null !== $search && in_array($search->accountType->type, $validTypes, true)) { + Log::debug('ID result is not null and seems valid, save as source account.'); + $this->setSource($search); + $result = true; + } } // if user submits an IBAN: @@ -138,10 +143,15 @@ trait DepositValidation Log::debug(sprintf('User submitted IBAN ("%s"), which is a "%s", so this is not a valid source.', $accountIban, $search->accountType->type)); $result = false; } + if (null !== $search && in_array($search->accountType->type, $validTypes, true)) { + Log::debug('IBAN result is not null and seems valid, save as source account.'); + $this->setSource($search); + $result = true; + } } // if user submits a number: - if (null !== $accountNumber) { + if (null !== $accountNumber && '' !== $accountNumber) { $search = $this->accountRepository->findByAccountNumber($accountNumber, $validTypes); if (null !== $search && !in_array($search->accountType->type, $validTypes, true)) { Log::debug( @@ -149,6 +159,11 @@ trait DepositValidation ); $result = false; } + if (null !== $search && in_array($search->accountType->type, $validTypes, true)) { + Log::debug('Number result is not null and seems valid, save as source account.'); + $this->setSource($search); + $result = true; + } } // if the account can be created anyway we don't need to search. @@ -159,7 +174,7 @@ trait DepositValidation $account = new Account(); $accountType = AccountType::whereType(AccountType::REVENUE)->first(); $account->accountType = $accountType; - $this->source = $account; + $this->setSource($account); } return $result ?? false; diff --git a/app/Validation/Account/LiabilityValidation.php b/app/Validation/Account/LiabilityValidation.php index feb8c155dc..805271f1ba 100644 --- a/app/Validation/Account/LiabilityValidation.php +++ b/app/Validation/Account/LiabilityValidation.php @@ -91,7 +91,7 @@ trait LiabilityValidation return false; } Log::debug(sprintf('Return true, found #%d ("%s")', $result->id, $result->name)); - $this->source = $result; + $this->setSource($result); return true; } @@ -109,7 +109,7 @@ trait LiabilityValidation $account = new Account(); $accountType = AccountType::whereType(AccountType::LIABILITY_CREDIT)->first(); $account->accountType = $accountType; - $this->source = $account; + $this->setSource($account); } return $result; diff --git a/app/Validation/Account/OBValidation.php b/app/Validation/Account/OBValidation.php index 9f738c7b6c..1f1e349b27 100644 --- a/app/Validation/Account/OBValidation.php +++ b/app/Validation/Account/OBValidation.php @@ -70,7 +70,7 @@ trait OBValidation } if (null !== $search) { Log::debug(sprintf('findExistingAccount() returned #%d ("%s"), so the result is true.', $search->id, $search->name)); - $this->destination = $search; + $this->setDestination($search); $result = true; } } @@ -127,7 +127,7 @@ trait OBValidation // the source resulted in an account, AND it's of a valid type. if (null !== $search && in_array($search->accountType->type, $validTypes, true)) { Log::debug(sprintf('Found account of correct type: #%d, "%s"', $search->id, $search->name)); - $this->source = $search; + $this->setSource($search); $result = true; } } @@ -141,7 +141,7 @@ trait OBValidation $account = new Account(); $accountType = AccountType::whereType(AccountType::INITIAL_BALANCE)->first(); $account->accountType = $accountType; - $this->source = $account; + $this->setSource($account); } return $result ?? false; diff --git a/app/Validation/Account/ReconciliationValidation.php b/app/Validation/Account/ReconciliationValidation.php index f964a01585..1277b76fa3 100644 --- a/app/Validation/Account/ReconciliationValidation.php +++ b/app/Validation/Account/ReconciliationValidation.php @@ -64,7 +64,7 @@ trait ReconciliationValidation return false; } - $this->source = $search; + $this->setSource($search); Log::debug('Valid source account!'); return true; @@ -85,7 +85,7 @@ trait ReconciliationValidation // source to the asset account that is the destination. if (null === $accountId && null === $accountName) { Log::debug('The source is valid because ID and name are NULL.'); - $this->source = new Account(); + $this->setSource(new Account()); return true; } @@ -101,7 +101,7 @@ trait ReconciliationValidation return false; } - $this->source = $search; + $this->setSource($search); Log::debug('Valid source account!'); return true; diff --git a/app/Validation/Account/TransferValidation.php b/app/Validation/Account/TransferValidation.php index 9daa1c7bf4..e8032e7428 100644 --- a/app/Validation/Account/TransferValidation.php +++ b/app/Validation/Account/TransferValidation.php @@ -59,7 +59,7 @@ trait TransferValidation return false; } - $this->destination = $search; + $this->setDestination($search); // must not be the same as the source account if (null !== $this->source && $this->source->id === $this->destination->id) { @@ -116,7 +116,7 @@ trait TransferValidation return false; } - $this->source = $search; + $this->setSource($search); Log::debug('Valid source!'); return true; diff --git a/app/Validation/Account/WithdrawalValidation.php b/app/Validation/Account/WithdrawalValidation.php index 191f03087b..54af2768ee 100644 --- a/app/Validation/Account/WithdrawalValidation.php +++ b/app/Validation/Account/WithdrawalValidation.php @@ -61,7 +61,7 @@ trait WithdrawalValidation return false; } - $this->source = $search; + $this->setSource($search); Log::debug('Valid source account!'); return true; @@ -108,7 +108,7 @@ trait WithdrawalValidation if (null !== $found) { $type = $found->accountType->type; if (in_array($type, $validTypes, true)) { - $this->destination = $found; + $this->setDestination($found); return true; } $this->destError = (string)trans('validation.withdrawal_dest_bad_data', ['id' => $accountId, 'name' => $accountName]); @@ -151,7 +151,7 @@ trait WithdrawalValidation return false; } - $this->source = $search; + $this->setSource($search); Log::debug('Valid source account!'); return true; diff --git a/app/Validation/AccountValidator.php b/app/Validation/AccountValidator.php index a91ad2c261..f5cc6880fd 100644 --- a/app/Validation/AccountValidator.php +++ b/app/Validation/AccountValidator.php @@ -264,4 +264,34 @@ class AccountValidator return null; } + + /** + * @param Account|null $account + */ + public function setDestination(?Account $account): void + { + if(null === $account) { + Log::debug('AccountValidator destination is set to NULL'); + } + if(null !== $account) { + Log::debug(sprintf('AccountValidator destination is set to #%d: "%s" (%s)', $account->id, $account->name, $account?->accountType->type)); + } + $this->destination = $account; + } + + /** + * @param Account|null $account + */ + public function setSource(?Account $account): void + { + if(null === $account) { + Log::debug('AccountValidator source is set to NULL'); + } + if(null !== $account) { + Log::debug(sprintf('AccountValidator source is set to #%d: "%s" (%s)', $account->id, $account->name, $account?->accountType->type)); + } + $this->source = $account; + } + + } diff --git a/app/Validation/TransactionValidation.php b/app/Validation/TransactionValidation.php index e89c0f0b41..63c98c4eec 100644 --- a/app/Validation/TransactionValidation.php +++ b/app/Validation/TransactionValidation.php @@ -24,10 +24,12 @@ declare(strict_types=1); namespace FireflyIII\Validation; use FireflyIII\Models\Account; +use FireflyIII\Models\AccountType; use FireflyIII\Models\Transaction; use FireflyIII\Models\TransactionGroup; use FireflyIII\Models\TransactionJournal; use FireflyIII\Models\TransactionType; +use FireflyIII\Repositories\Account\AccountRepositoryInterface; use Illuminate\Validation\Validator; use Log; @@ -143,25 +145,133 @@ trait TransactionValidation // sanity check for reconciliation accounts. They can't both be null. $this->sanityCheckReconciliation($validator, $transactionType, $index, $source, $destination); - // deposit and the source is a liability or an asset account with a different - // currency than submitted? then the foreign currency info must be present as well (and filled in). - if (0 === $validator->errors()->count() && null !== $accountValidator->source && TransactionType::DEPOSIT === ucfirst($transactionType)) { - $accountType = $accountValidator?->source?->accountType?->type; - if (in_array($accountType, config('firefly.valid_currency_account_types'), true) && !$this->hasForeignCurrencyInfo($transaction)) { - $validator->errors()->add(sprintf('transactions.%d.foreign_amount', $index), (string)trans('validation.require_foreign_currency')); - } + // sanity check for currency information. + $this->sanityCheckForeignCurrency($validator, $accountValidator, $transaction, $transactionType, $index); + } + + /** + * TODO describe this method. + * @param Validator $validator + * @param AccountValidator $accountValidator + * @param array $transaction + * @param string $transactionType + * @param int $index + * @return void + */ + private function sanityCheckForeignCurrency( + Validator $validator, + AccountValidator $accountValidator, + array $transaction, + string $transactionType, + int $index + ): void { + Log::debug('Now in sanityCheckForeignCurrency()'); + if (0 !== $validator->errors()->count()) { + Log::debug('Already have errors, return'); + return; + } + if (null === $accountValidator->source) { + Log::debug('No source, return'); + return; + } + if (null === $accountValidator->destination) { + Log::debug('No destination, return'); + return; + } + $source = $accountValidator->source; + $destination = $accountValidator->destination; + + Log::debug(sprintf('Source: #%d "%s (%s)"', $source->id, $source->name, $source->accountType->type)); + Log::debug(sprintf('Destination: #%d "%s" (%s)', $destination->id, $destination->name, $source->accountType->type)); + + if (!$this->isLiabilityOrAsset($source) || !$this->isLiabilityOrAsset($destination)) { + Log::debug('Any account must be liability or asset account to continue.'); + return; } - // withdrawal or transfer and the destination is a liability or an asset account with a different - // currency than submitted? then the foreign currency info must be present as well (and filled in). - if (0 === $validator->errors()->count() && null !== $accountValidator->destination && - (TransactionType::WITHDRAWAL === ucfirst($transactionType) || (TransactionType::TRANSFER === ucfirst($transactionType)))) { - $accountType = $accountValidator?->destination?->accountType?->type; - if (in_array($accountType, config('firefly.valid_currency_account_types'), true) && !$this->hasForeignCurrencyInfo($transaction)) { + + /** @var AccountRepositoryInterface $accountRepository */ + $accountRepository = app(AccountRepositoryInterface::class); + $defaultCurrency = app('amount')->getDefaultCurrency(); + $sourceCurrency = $accountRepository->getAccountCurrency($source) ?? $defaultCurrency; + $destinationCurrency = $accountRepository->getAccountCurrency($destination) ?? $defaultCurrency; + // if both accounts have the same currency, continue. + if ($sourceCurrency->code === $destinationCurrency->code) { + Log::debug('Both accounts have the same currency, continue.'); + return; + } + + if (TransactionType::DEPOSIT === ucfirst($transactionType)) { + // use case: deposit from liability account to an asset account + // the foreign amount must be in the currency of the source + // the amount must be in the currency of the destination + + // no foreign currency information is present: + if (!$this->hasForeignCurrencyInfo($transaction)) { $validator->errors()->add(sprintf('transactions.%d.foreign_amount', $index), (string)trans('validation.require_foreign_currency')); + return; + } + + // wrong currency information is present + $foreignCurrencyCode = $transaction['foreign_currency_code'] ?? false; + if ($foreignCurrencyCode !== $sourceCurrency->code) { + $validator->errors()->add(sprintf('transactions.%d.foreign_currency_code', $index), (string)trans('validation.require_foreign_src')); + return; } } - // account validator has a valid source and a valid destination + if (TransactionType::TRANSFER === ucfirst($transactionType) || TransactionType::WITHDRAWAL === ucfirst($transactionType)) { + // use case: withdrawal from asset account to a liability account. + // the foreign amount must be in the currency of the destination + // the amount must be in the currency of the source + + // use case: transfer between accounts with different currencies. + // the foreign amount must be in the currency of the destination + // the amount must be in the currency of the source + + // no foreign currency information is present: + if (!$this->hasForeignCurrencyInfo($transaction)) { + $validator->errors()->add(sprintf('transactions.%d.foreign_amount', $index), (string)trans('validation.require_foreign_currency')); + return; + } + + // wrong currency information is present + $foreignCurrencyCode = $transaction['foreign_currency_code'] ?? false; + if ($foreignCurrencyCode !== $destinationCurrency->code) { + $validator->errors()->add(sprintf('transactions.%d.foreign_currency_code', $index), (string)trans('validation.require_foreign_dest')); + } + } + } + + /** + * @param Account $account + * @return bool + */ + private function isLiabilityOrAsset(Account $account): bool + { + return $this->isLiability($account) || $this->isAsset($account); + } + + /** + * @param Account $account + * @return bool + */ + private function isLiability(Account $account): bool + { + $type = $account->accountType?->type; + if (in_array($type, config('firefly.valid_liabilities'), true)) { + return true; + } + return false; + } + + /** + * @param Account $account + * @return bool + */ + private function isAsset(Account $account): bool + { + $type = $account->accountType?->type; + return $type === AccountType::ASSET; } /** diff --git a/resources/lang/en_US/validation.php b/resources/lang/en_US/validation.php index 924831adc8..d90ab1969b 100644 --- a/resources/lang/en_US/validation.php +++ b/resources/lang/en_US/validation.php @@ -57,6 +57,8 @@ return [ 'not_transfer_account' => 'This account is not an account that can be used for transfers.', 'require_currency_amount' => 'The content of this field is invalid without foreign amount information.', 'require_foreign_currency' => 'This field requires a number', + 'require_foreign_dest' => 'This field value must match the currency of the destination account.', + 'require_foreign_src' => 'This field value must match the currency of the source account.', 'equal_description' => 'Transaction description should not equal global description.', 'file_invalid_mime' => 'File ":name" is of type ":mime" which is not accepted as a new upload.', 'file_too_large' => 'File ":name" is too large.',