From e83416d84de70cd8bd373b2366092e8619bf2076 Mon Sep 17 00:00:00 2001 From: James Cole Date: Tue, 26 Jan 2021 19:27:49 +0100 Subject: [PATCH] Add rule for unique account number. --- app/Api/V1/Requests/AccountStoreRequest.php | 8 +- app/Api/V1/Requests/AccountUpdateRequest.php | 7 +- app/Rules/UniqueAccountNumber.php | 160 +++++++++++++++++++ app/Rules/UniqueIban.php | 21 ++- 4 files changed, 185 insertions(+), 11 deletions(-) create mode 100644 app/Rules/UniqueAccountNumber.php diff --git a/app/Api/V1/Requests/AccountStoreRequest.php b/app/Api/V1/Requests/AccountStoreRequest.php index 7c497beb81..29274f6e57 100644 --- a/app/Api/V1/Requests/AccountStoreRequest.php +++ b/app/Api/V1/Requests/AccountStoreRequest.php @@ -26,6 +26,8 @@ namespace FireflyIII\Api\V1\Requests; use FireflyIII\Models\Location; use FireflyIII\Rules\IsBoolean; +use FireflyIII\Rules\UniqueAccountNumber; +use FireflyIII\Rules\UniqueIban; use FireflyIII\Support\Request\AppendsLocationData; use FireflyIII\Support\Request\ChecksLogin; use FireflyIII\Support\Request\ConvertsDataTypes; @@ -98,12 +100,13 @@ class AccountStoreRequest extends FormRequest $accountRoles = implode(',', config('firefly.accountRoles')); $types = implode(',', array_keys(config('firefly.subTitlesByIdentifier'))); $ccPaymentTypes = implode(',', array_keys(config('firefly.ccTypes'))); + $type = $this->string('type'); $rules = [ 'name' => 'required|min:1|uniqueAccountForUser', 'type' => 'required|' . sprintf('in:%s', $types), - 'iban' => 'iban|nullable', + 'iban' => ['iban', 'nullable', new UniqueIban(null, $type)], 'bic' => 'bic|nullable', - 'account_number' => 'between:1,255|nullable|uniqueAccountNumberForUser', + 'account_number' => ['between:1,255', 'nullable', new UniqueAccountNumber(null, $type)], 'opening_balance' => 'numeric|required_with:opening_balance_date|nullable', 'opening_balance_date' => 'date|required_with:opening_balance|nullable', 'virtual_balance' => 'numeric|nullable', @@ -122,6 +125,7 @@ class AccountStoreRequest extends FormRequest 'interest_period' => 'required_if:type,liability|in:daily,monthly,yearly', 'notes' => 'min:0|max:65536', ]; + return Location::requestRules($rules); } } diff --git a/app/Api/V1/Requests/AccountUpdateRequest.php b/app/Api/V1/Requests/AccountUpdateRequest.php index 10cafe6b63..e9a9267638 100644 --- a/app/Api/V1/Requests/AccountUpdateRequest.php +++ b/app/Api/V1/Requests/AccountUpdateRequest.php @@ -26,6 +26,8 @@ namespace FireflyIII\Api\V1\Requests; use FireflyIII\Models\Location; use FireflyIII\Rules\IsBoolean; +use FireflyIII\Rules\UniqueAccountNumber; +use FireflyIII\Rules\UniqueIban; use FireflyIII\Support\Request\AppendsLocationData; use FireflyIII\Support\Request\ChecksLogin; use FireflyIII\Support\Request\ConvertsDataTypes; @@ -103,9 +105,9 @@ class AccountUpdateRequest extends FormRequest $rules = [ 'name' => sprintf('min:1|uniqueAccountForUser:%d', $account->id), 'type' => sprintf('in:%s', $types), - 'iban' => 'iban|nullable', + 'iban' => ['iban', 'nullable', new UniqueIban($account, $this->nullableString('type'))], 'bic' => 'bic|nullable', - 'account_number' => sprintf('between:1,255|nullable|uniqueAccountNumberForUser:%d', $account->id), + 'account_number' => ['between:1,255', 'nullable', new UniqueAccountNumber($account, $this->nullableString('type'))], 'opening_balance' => 'numeric|required_with:opening_balance_date|nullable', 'opening_balance_date' => 'date|required_with:opening_balance|nullable', 'virtual_balance' => 'numeric|nullable', @@ -124,6 +126,7 @@ class AccountUpdateRequest extends FormRequest 'interest_period' => 'required_if:type,liability|in:daily,monthly,yearly', 'notes' => 'min:0|max:65536', ]; + return Location::requestRules($rules); } } diff --git a/app/Rules/UniqueAccountNumber.php b/app/Rules/UniqueAccountNumber.php new file mode 100644 index 0000000000..256c073489 --- /dev/null +++ b/app/Rules/UniqueAccountNumber.php @@ -0,0 +1,160 @@ +. + */ + +declare(strict_types=1); + +namespace FireflyIII\Rules; + +use FireflyIII\Models\Account; +use FireflyIII\Models\AccountMeta; +use FireflyIII\Models\AccountType; +use Illuminate\Contracts\Validation\Rule; +use Log; + +/** + * Class UniqueAccountNumber + */ +class UniqueAccountNumber implements Rule +{ + private ?Account $account; + private ?string $expectedType; + + /** + * Create a new rule instance. + * + * @codeCoverageIgnore + * + * @param Account|null $account + * @param string|null $expectedType + */ + public function __construct(?Account $account, ?string $expectedType) + { + $this->account = $account; + $this->expectedType = $expectedType; + // a very basic fix to make sure we get the correct account type: + if ('expense' === $expectedType) { + $this->expectedType = AccountType::EXPENSE; + } + if ('revenue' === $expectedType) { + $this->expectedType = AccountType::REVENUE; + } + if ('asset' === $expectedType) { + $this->expectedType = AccountType::ASSET; + } + } + + /** + * Get the validation error message. + * + * @codeCoverageIgnore + * + * @return string + */ + public function message(): string + { + return (string)trans('validation.unique_account_number_for_user'); + } + + /** + * Determine if the validation rule passes. + * + * @param string $attribute + * @param mixed $value + * + * @return bool + * + */ + public function passes($attribute, $value): bool + { + if (!auth()->check()) { + return true; // @codeCoverageIgnore + } + if (null === $this->expectedType) { + return true; // @codeCoverageIgnore + } + $maxCounts = $this->getMaxOccurrences(); + + foreach ($maxCounts as $type => $max) { + $count = $this->countHits($type, $value); + Log::debug(sprintf('Count for "%s" and account number "%s" is %d', $type, $value, $count)); + if ($count > $max) { + Log::debug( + sprintf( + 'account number "%s" is in use with %d account(s) of type "%s", which is too much for expected type "%s"', + $value, $count, $type, $this->expectedType + ) + ); + + return false; + } + } + + return true; + } + + /** + * @param string $type + * @param string $accountNumber + * + * @return int + */ + private function countHits(string $type, string $accountNumber): int + { + $query = AccountMeta + ::leftJoin('accounts','accounts.id','=','account_meta.account_id') + ->leftJoin('account_types', 'account_types.id', '=', 'accounts.account_type_id') + ->where('accounts.user_id', auth()->user()->id) + ->where('account_meta.name','=','account_number') + ->where('account_meta.data',json_encode($accountNumber)); + + if (null !== $this->account) { + $query->where('accounts.id', '!=', $this->account->id); + } + + return $query->count(); + } + + /** + * @return array + * + */ + private function getMaxOccurrences(): array + { + $maxCounts = [ + AccountType::ASSET => 0, + AccountType::EXPENSE => 0, + AccountType::REVENUE => 0, + ]; + + if ('expense' === $this->expectedType || AccountType::EXPENSE === $this->expectedType) { + // IBAN should be unique amongst expense and asset accounts. + // may appear once in revenue accounts + $maxCounts[AccountType::REVENUE] = 1; + } + if ('revenue' === $this->expectedType || AccountType::REVENUE === $this->expectedType) { + // IBAN should be unique amongst revenue and asset accounts. + // may appear once in expense accounts + $maxCounts[AccountType::EXPENSE] = 1; + } + + return $maxCounts; + } +} diff --git a/app/Rules/UniqueIban.php b/app/Rules/UniqueIban.php index a270285beb..6afb487518 100644 --- a/app/Rules/UniqueIban.php +++ b/app/Rules/UniqueIban.php @@ -33,11 +33,8 @@ use Log; */ class UniqueIban implements Rule { - /** @var Account */ - private $account; - - /** @var string */ - private $expectedType; + private ?Account $account; + private ?string $expectedType; /** * Create a new rule instance. @@ -51,6 +48,16 @@ class UniqueIban implements Rule { $this->account = $account; $this->expectedType = $expectedType; + // a very basic fix to make sure we get the correct account type: + if ('expense' === $expectedType) { + $this->expectedType = AccountType::EXPENSE; + } + if ('revenue' === $expectedType) { + $this->expectedType = AccountType::REVENUE; + } + if ('asset' === $expectedType) { + $this->expectedType = AccountType::ASSET; + } } /** @@ -68,8 +75,8 @@ class UniqueIban implements Rule /** * Determine if the validation rule passes. * - * @param string $attribute - * @param mixed $value + * @param string $attribute + * @param mixed $value * * @return bool *