From d8a15e41e6be4e8b789c269f46dea9425fa9e1da Mon Sep 17 00:00:00 2001 From: James Cole Date: Sun, 2 Oct 2022 20:13:32 +0200 Subject: [PATCH] Catch various audit log things --- app/Handlers/Events/AuditEventHandler.php | 9 +- .../AuditLogEntry/ALERepository.php | 2 +- .../Actions/SetDescription.php | 2 +- .../Account/ReconciliationValidation.php | 99 +++++++++---------- app/Validation/TransactionValidation.php | 46 ++++++++- resources/lang/en_US/validation.php | 9 +- resources/views/list/ale.twig | 4 +- 7 files changed, 99 insertions(+), 72 deletions(-) diff --git a/app/Handlers/Events/AuditEventHandler.php b/app/Handlers/Events/AuditEventHandler.php index e0249e12e7..601a18df6e 100644 --- a/app/Handlers/Events/AuditEventHandler.php +++ b/app/Handlers/Events/AuditEventHandler.php @@ -22,7 +22,6 @@ namespace FireflyIII\Handlers\Events; use FireflyIII\Events\TriggeredAuditLog; -use FireflyIII\Models\AuditLogEntry; use FireflyIII\Repositories\AuditLogEntry\ALERepositoryInterface; class AuditEventHandler @@ -36,10 +35,10 @@ class AuditEventHandler { $array = [ 'auditable' => $event->auditable, - 'changer' => $event->changer, - 'action' => $event->field, - 'before' => $event->before, - 'after' => $event->after, + 'changer' => $event->changer, + 'action' => $event->field, + 'before' => $event->before, + 'after' => $event->after, ]; /** @var ALERepositoryInterface $repository */ $repository = app(ALERepositoryInterface::class); diff --git a/app/Repositories/AuditLogEntry/ALERepository.php b/app/Repositories/AuditLogEntry/ALERepository.php index 7b39d0b342..8b57a88712 100644 --- a/app/Repositories/AuditLogEntry/ALERepository.php +++ b/app/Repositories/AuditLogEntry/ALERepository.php @@ -39,7 +39,7 @@ class ALERepository implements ALERepositoryInterface $auditLogEntry->auditable()->associate($data['auditable']); $auditLogEntry->changer()->associate($data['changer']); - $auditLogEntry->action = $data['field']; + $auditLogEntry->action = $data['action']; $auditLogEntry->before = $data['before']; $auditLogEntry->after = $data['after']; $auditLogEntry->save(); diff --git a/app/TransactionRules/Actions/SetDescription.php b/app/TransactionRules/Actions/SetDescription.php index 37506e7f8f..ee18e71c97 100644 --- a/app/TransactionRules/Actions/SetDescription.php +++ b/app/TransactionRules/Actions/SetDescription.php @@ -66,7 +66,7 @@ class SetDescription implements ActionInterface $this->action->action_value ) ); - + $journal->refresh(); event(new TriggeredAuditLog($this->action->rule, $journal, 'update_description', $before, $this->action->action_value)); return true; diff --git a/app/Validation/Account/ReconciliationValidation.php b/app/Validation/Account/ReconciliationValidation.php index c0e3ddae9a..68b59ac53d 100644 --- a/app/Validation/Account/ReconciliationValidation.php +++ b/app/Validation/Account/ReconciliationValidation.php @@ -43,76 +43,67 @@ trait ReconciliationValidation */ protected function validateReconciliationDestination(array $array): bool { - $accountId = array_key_exists('id', $array) ? $array['id'] : null; - Log::debug('Now in validateReconciliationDestination', $array); - if (null === $accountId) { - Log::debug('Return FALSE'); - - return false; - } - $result = $this->accountRepository->find($accountId); - if (null === $result) { - $this->destError = (string) trans('validation.deposit_dest_bad_data', ['id' => $accountId, 'name' => '']); - Log::debug('Return FALSE'); - - return false; - } - // types depends on type of source: - $types = [AccountType::ASSET, AccountType::LOAN, AccountType::DEBT, AccountType::MORTGAGE]; - // if source is reconciliation, destination can't be. - if (null !== $this->source && AccountType::RECONCILIATION === $this->source->accountType->type) { - $types = [AccountType::ASSET, AccountType::LOAN, AccountType::DEBT, AccountType::MORTGAGE]; - } - // if source is not reconciliation, destination MUST be. - if (null !== $this->source - && in_array( - $this->source->accountType->type, [AccountType::ASSET, AccountType::LOAN, AccountType::DEBT, AccountType::MORTGAGE], true - )) { - $types = [AccountType::RECONCILIATION]; - } - - if (in_array($result->accountType->type, $types, true)) { - $this->destination = $result; - Log::debug('Return TRUE'); + $accountId = array_key_exists('id', $array) ? $array['id'] : null; + $accountName = array_key_exists('name', $array) ? $array['name'] : null; + // if both are NULL, the destination is valid because the reconciliation + // is expected to be "negative", i.e. the money flows towards the + // destination to the asset account which is the source. + if (null === $accountId && null === $accountName) { return true; } - $this->destError = (string) trans('validation.deposit_dest_wrong_type'); - Log::debug('Return FALSE'); - return false; + // after that, search for it expecting an asset account or a liability. + Log::debug('Now in validateReconciliationDestination', $array); + + // source can be any of the following types. + $validTypes = array_keys($this->combinations[$this->transactionType]); + $search = $this->findExistingAccount($validTypes, $array); + if (null === $search) { + $this->sourceError = (string) trans('validation.reconciliation_source_bad_data', ['id' => $accountId, 'name' => $accountName]); + Log::warning('Not a valid source. Cant find it.', $validTypes); + + return false; + } + $this->source = $search; + Log::debug('Valid source account!'); + + return true; } /** + * Basically the same check * @param array $array * * @return bool */ protected function validateReconciliationSource(array $array): bool { - $accountId = array_key_exists('id', $array) ? $array['id'] : null; - Log::debug('In validateReconciliationSource', $array); - if (null === $accountId) { - Log::debug('Return FALSE'); - - return false; - } - $result = $this->accountRepository->find($accountId); - $types = [AccountType::ASSET, AccountType::LOAN, AccountType::DEBT, AccountType::MORTGAGE, AccountType::RECONCILIATION]; - if (null === $result) { - Log::debug('Return FALSE'); - - return false; - } - if (in_array($result->accountType->type, $types, true)) { - $this->source = $result; - Log::debug('Return TRUE'); - + $accountId = array_key_exists('id', $array) ? $array['id'] : null; + $accountName = array_key_exists('name', $array) ? $array['name'] : null; + // if both are NULL, the source is valid because the reconciliation + // is expected to be "positive", i.e. the money flows from the + // source to the asset account that is the destination. + if (null === $accountId && null === $accountName) { return true; } - Log::debug('Return FALSE'); - return false; + // after that, search for it expecting an asset account or a liability. + Log::debug('Now in validateReconciliationSource', $array); + + // source can be any of the following types. + $validTypes = array_keys($this->combinations[$this->transactionType]); + $search = $this->findExistingAccount($validTypes, $array); + if (null === $search) { + $this->sourceError = (string) trans('validation.reconciliation_source_bad_data', ['id' => $accountId, 'name' => $accountName]); + Log::warning('Not a valid source. Cant find it.', $validTypes); + + return false; + } + $this->source = $search; + Log::debug('Valid source account!'); + + return true; } } diff --git a/app/Validation/TransactionValidation.php b/app/Validation/TransactionValidation.php index 25c7c303bd..e13afab2d7 100644 --- a/app/Validation/TransactionValidation.php +++ b/app/Validation/TransactionValidation.php @@ -27,6 +27,7 @@ use FireflyIII\Models\Account; use FireflyIII\Models\Transaction; use FireflyIII\Models\TransactionGroup; use FireflyIII\Models\TransactionJournal; +use FireflyIII\Models\TransactionType; use Illuminate\Validation\Validator; use Log; @@ -104,13 +105,13 @@ trait TransactionValidation $sourceName = array_key_exists('source_name', $transaction) ? (string) $transaction['source_name'] : null; $sourceIban = array_key_exists('source_iban', $transaction) ? (string) $transaction['source_iban'] : null; $sourceNumber = array_key_exists('source_number', $transaction) ? (string) $transaction['source_number'] : null; - $array = [ + $source = [ 'id' => $sourceId, 'name' => $sourceName, 'iban' => $sourceIban, 'number' => $sourceNumber, ]; - $validSource = $accountValidator->validateSource($array); + $validSource = $accountValidator->validateSource($source); // do something with result: if (false === $validSource) { @@ -124,18 +125,53 @@ trait TransactionValidation $destinationName = array_key_exists('destination_name', $transaction) ? (string) $transaction['destination_name'] : null; $destinationIban = array_key_exists('destination_iban', $transaction) ? (string) $transaction['destination_iban'] : null; $destinationNumber = array_key_exists('destination_number', $transaction) ? (string) $transaction['destination_number'] : null; - $array = [ + $destination = [ 'id' => $destinationId, 'name' => $destinationName, 'iban' => $destinationIban, 'number' => $destinationNumber, ]; - $validDestination = $accountValidator->validateDestination($array); + $validDestination = $accountValidator->validateDestination($destination); // do something with result: if (false === $validDestination) { $validator->errors()->add(sprintf('transactions.%d.destination_id', $index), $accountValidator->destError); $validator->errors()->add(sprintf('transactions.%d.destination_name', $index), $accountValidator->destError); } + + // sanity check for reconciliation accounts. They can't both be null. + $this->sanityCheckReconciliation($validator, $transactionType, $index, $source, $destination); + } + + /** + * @param Validator $validator + * @param string $transactionType + * @param int $index + * @param array $source + * @param array $destination + * @return void + */ + protected function sanityCheckReconciliation(Validator $validator, string $transactionType, int $index, array $source, array $destination): void + { + Log::debug('sanityCheckReconciliation'); + if (TransactionType::RECONCILIATION === ucfirst($transactionType) && + null === $source['id'] && null === $source['name'] && null === $destination['id'] && null === $destination['name'] + ) { + Log::debug('Both are NULL, error!'); + $validator->errors()->add(sprintf('transactions.%d.source_id', $index), trans('validation.reconciliation_either_account')); + $validator->errors()->add(sprintf('transactions.%d.source_name', $index), trans('validation.reconciliation_either_account')); + $validator->errors()->add(sprintf('transactions.%d.destination_id', $index), trans('validation.reconciliation_either_account')); + $validator->errors()->add(sprintf('transactions.%d.destination_name', $index), trans('validation.reconciliation_either_account')); + } + + if (TransactionType::RECONCILIATION === $transactionType && + (null !== $source['id'] || null !== $source['name']) && + (null !== $destination['id'] || null !== $destination['name'])) { + Log::debug('Both are not NULL, error!'); + $validator->errors()->add(sprintf('transactions.%d.source_id', $index), trans('validation.reconciliation_either_account')); + $validator->errors()->add(sprintf('transactions.%d.source_name', $index), trans('validation.reconciliation_either_account')); + $validator->errors()->add(sprintf('transactions.%d.destination_id', $index), trans('validation.reconciliation_either_account')); + $validator->errors()->add(sprintf('transactions.%d.destination_name', $index), trans('validation.reconciliation_either_account')); + } } /** @@ -448,7 +484,7 @@ trait TransactionValidation // I think I can get away with one combination being equal, as long as the rest // of the code picks up on this as well. // either way all fields must be blank or all equal - // but if ID's are equal don't bother with the names. + // but if IDs are equal don't bother with the names. $comparison = $this->collectComparisonData($transactions); $result = $this->compareAccountData($type, $comparison); if (false === $result) { diff --git a/resources/lang/en_US/validation.php b/resources/lang/en_US/validation.php index 0f0eb75744..191940fedc 100644 --- a/resources/lang/en_US/validation.php +++ b/resources/lang/en_US/validation.php @@ -208,10 +208,11 @@ return [ 'transfer_dest_bad_data' => 'Could not find a valid destination account when searching for ID ":id" or name ":name".', 'need_id_in_edit' => 'Each split must have transaction_journal_id (either valid ID or 0).', - 'ob_source_need_data' => 'Need to get a valid source account ID and/or valid source account name to continue.', - 'lc_source_need_data' => 'Need to get a valid source account ID to continue.', - 'ob_dest_need_data' => 'Need to get a valid destination account ID and/or valid destination account name to continue.', - 'ob_dest_bad_data' => 'Could not find a valid destination account when searching for ID ":id" or name ":name".', + 'ob_source_need_data' => 'Need to get a valid source account ID and/or valid source account name to continue.', + 'lc_source_need_data' => 'Need to get a valid source account ID to continue.', + 'ob_dest_need_data' => 'Need to get a valid destination account ID and/or valid destination account name to continue.', + 'ob_dest_bad_data' => 'Could not find a valid destination account when searching for ID ":id" or name ":name".', + 'reconciliation_either_account' => 'To submit a reconciliation, you must submit either a source or a destination account. Not both, not neither.', 'generic_invalid_source' => 'You can\'t use this account as the source account.', 'generic_invalid_destination' => 'You can\'t use this account as the destination account.', diff --git a/resources/views/list/ale.twig b/resources/views/list/ale.twig index 6145385cee..e490295275 100644 --- a/resources/views/list/ale.twig +++ b/resources/views/list/ale.twig @@ -71,9 +71,9 @@ {% endif %} {% if 'update_description' == logEntry.action %} - {{ logEntry.before }} + {{ logEntry.before }} → - {{ logEntry.after }} + {{ logEntry.after }} {% endif %} {% if 'add_to_piggy' == logEntry.action %} {{ trans('firefly.ale_action_log_add', {amount: formatAmountBySymbol(logEntry.after.amount, logEntry.after.currency_symbol, logEntry.after.decimal_places, true), name: logEntry.after.name})|raw }}