From 86f1d8a1bc73bc4f3e5bc6cb50a6f2dc4c86861f Mon Sep 17 00:00:00 2001 From: James Cole Date: Wed, 24 Dec 2025 07:45:03 +0100 Subject: [PATCH] Fix ALE amount logging. --- .../Internal/Update/JournalUpdateService.php | 149 ++++++++++-------- resources/lang/en_US/firefly.php | 1 + resources/views/list/ale.twig | 14 +- 3 files changed, 98 insertions(+), 66 deletions(-) diff --git a/app/Services/Internal/Update/JournalUpdateService.php b/app/Services/Internal/Update/JournalUpdateService.php index 3029cf1a37..d69a70779b 100644 --- a/app/Services/Internal/Update/JournalUpdateService.php +++ b/app/Services/Internal/Update/JournalUpdateService.php @@ -69,7 +69,7 @@ class JournalUpdateService private ?Transaction $destinationTransaction = null; private array $metaDate = ['interest_date', 'book_date', 'process_date', 'due_date', 'payment_date', - 'invoice_date', ]; + 'invoice_date',]; private array $metaString = [ 'sepa_cc', @@ -113,7 +113,7 @@ class JournalUpdateService public function setTransactionGroup(TransactionGroup $transactionGroup): void { - $this->transactionGroup = $transactionGroup; + $this->transactionGroup = $transactionGroup; $this->billRepository->setUser($transactionGroup->user); $this->categoryRepository->setUser($transactionGroup->user); $this->budgetRepository->setUser($transactionGroup->user); @@ -184,8 +184,8 @@ class JournalUpdateService private function hasValidSourceAccount(): bool { - $sourceId = $this->data['source_id'] ?? null; - $sourceName = $this->data['source_name'] ?? null; + $sourceId = $this->data['source_id'] ?? null; + $sourceName = $this->data['source_name'] ?? null; Log::debug(sprintf('Now in hasValidSourceAccount("%s","%s").', $sourceId, $sourceName)); if (!$this->hasFields(['source_id', 'source_name'])) { @@ -200,11 +200,11 @@ class JournalUpdateService // make a new validator. /** @var AccountValidator $validator */ - $validator = app(AccountValidator::class); + $validator = app(AccountValidator::class); $validator->setTransactionType($expectedType); $validator->setUser($this->transactionJournal->user); - $result = $validator->validateSource(['id' => $sourceId, 'name' => $sourceName]); + $result = $validator->validateSource(['id' => $sourceId, 'name' => $sourceName]); Log::debug( sprintf('hasValidSourceAccount(%d, "%s") will return %s', $sourceId, $sourceName, var_export($result, true)) ); @@ -217,7 +217,7 @@ class JournalUpdateService private function hasFields(array $fields): bool { - return array_any($fields, fn ($field): bool => array_key_exists($field, $this->data)); + return array_any($fields, fn($field): bool => array_key_exists($field, $this->data)); } private function getOriginalSourceAccount(): Account @@ -262,8 +262,8 @@ class JournalUpdateService private function hasValidDestinationAccount(): bool { Log::debug('Now in hasValidDestinationAccount().'); - $destId = $this->data['destination_id'] ?? null; - $destName = $this->data['destination_name'] ?? null; + $destId = $this->data['destination_id'] ?? null; + $destName = $this->data['destination_name'] ?? null; if (!$this->hasFields(['destination_id', 'destination_name'])) { Log::debug('No destination info submitted, grab the original data.'); @@ -273,12 +273,12 @@ class JournalUpdateService } // make new account validator. - $expectedType = $this->getExpectedType(); + $expectedType = $this->getExpectedType(); Log::debug(sprintf('(b) Expected type (new or unchanged) is %s', $expectedType)); // make a new validator. /** @var AccountValidator $validator */ - $validator = app(AccountValidator::class); + $validator = app(AccountValidator::class); $validator->setTransactionType($expectedType); $validator->setUser($this->transactionJournal->user); $validator->source = $this->getValidSourceAccount(); @@ -333,7 +333,7 @@ class JournalUpdateService return $this->getOriginalSourceAccount(); } - $sourceInfo = [ + $sourceInfo = [ 'id' => (int)($this->data['source_id'] ?? null), 'name' => $this->data['source_name'] ?? null, 'iban' => $this->data['source_iban'] ?? null, @@ -361,8 +361,8 @@ class JournalUpdateService */ private function updateAccounts(): void { - $source = $this->getValidSourceAccount(); - $destination = $this->getValidDestinationAccount(); + $source = $this->getValidSourceAccount(); + $destination = $this->getValidDestinationAccount(); // cowardly refuse to update if both accounts are the same. if ($source->id === $destination->id) { @@ -375,7 +375,7 @@ class JournalUpdateService $origSourceTransaction->account()->associate($source); $origSourceTransaction->save(); - $destTransaction = $this->getDestinationTransaction(); + $destTransaction = $this->getDestinationTransaction(); $destTransaction->account()->associate($destination); $destTransaction->save(); @@ -397,7 +397,7 @@ class JournalUpdateService return $this->getOriginalDestinationAccount(); } - $destInfo = [ + $destInfo = [ 'id' => (int)($this->data['destination_id'] ?? null), 'name' => $this->data['destination_name'] ?? null, 'iban' => $this->data['destination_iban'] ?? null, @@ -426,7 +426,7 @@ class JournalUpdateService { Log::debug('Now in updateType()'); if ($this->hasFields(['type'])) { - $type = 'opening-balance' === $this->data['type'] ? 'opening balance' : $this->data['type']; + $type = 'opening-balance' === $this->data['type'] ? 'opening balance' : $this->data['type']; Log::debug( sprintf( 'Trying to change journal #%d from a %s to a %s.', @@ -459,9 +459,9 @@ class JournalUpdateService { $type = $this->transactionJournal->transactionType->type; if (( - array_key_exists('bill_id', $this->data) + array_key_exists('bill_id', $this->data) || array_key_exists('bill_name', $this->data) - ) + ) && TransactionTypeEnum::WITHDRAWAL->value === $type ) { $billId = (int)($this->data['bill_id'] ?? 0); @@ -478,7 +478,7 @@ class JournalUpdateService private function updateField(string $fieldName): void { if (array_key_exists($fieldName, $this->data) && '' !== (string)$this->data[$fieldName]) { - $value = $this->data[$fieldName]; + $value = $this->data[$fieldName]; if ('date' === $fieldName) { if (!$value instanceof Carbon) { @@ -575,7 +575,7 @@ class JournalUpdateService if ($this->hasFields([$field])) { $value = '' === $this->data[$field] ? null : $this->data[$field]; Log::debug(sprintf('Field "%s" is present ("%s"), try to update it.', $field, $value)); - $set = [ + $set = [ 'journal' => $this->transactionJournal, 'name' => $field, 'data' => $value, @@ -594,7 +594,7 @@ class JournalUpdateService if ($this->hasFields([$field])) { try { $value = '' === (string)$this->data[$field] ? null : new Carbon($this->data[$field]); - } catch (InvalidDateException|InvalidFormatException $e) { // @phpstan-ignore-line + } catch (InvalidDateException | InvalidFormatException $e) { // @phpstan-ignore-line Log::debug(sprintf('%s is not a valid date value: %s', $this->data[$field], $e->getMessage())); return; @@ -623,19 +623,19 @@ class JournalUpdateService if (!$this->hasFields(['currency_id', 'currency_code'])) { return; } - $currencyId = $this->data['currency_id'] ?? null; - $currencyCode = $this->data['currency_code'] ?? null; - $currency = $this->currencyRepository->findCurrency($currencyId, $currencyCode); + $currencyId = $this->data['currency_id'] ?? null; + $currencyCode = $this->data['currency_code'] ?? null; + $currency = $this->currencyRepository->findCurrency($currencyId, $currencyCode); // update currency everywhere. $this->transactionJournal->transaction_currency_id = $currency->id; $this->transactionJournal->save(); - $source = $this->getSourceTransaction(); - $source->transaction_currency_id = $currency->id; + $source = $this->getSourceTransaction(); + $source->transaction_currency_id = $currency->id; $source->save(); - $dest = $this->getDestinationTransaction(); - $dest->transaction_currency_id = $currency->id; + $dest = $this->getDestinationTransaction(); + $dest->transaction_currency_id = $currency->id; $dest->save(); // refresh transactions. @@ -651,8 +651,8 @@ class JournalUpdateService return; } - $value = $this->data['amount'] ?? ''; - Log::debug(sprintf('Amount is now "%s"', $value)); + $value = $this->data['amount'] ?? ''; + Log::debug(sprintf('[a] Amount is now "%s"', $value)); try { $amount = $this->getAmount($value); @@ -661,46 +661,71 @@ class JournalUpdateService return; } + Log::debug(sprintf('[b] Amount is now "%s"', $value)); $origSourceTransaction = $this->getSourceTransaction(); + $destTransaction = $this->getDestinationTransaction(); + $originalSourceAmount = $origSourceTransaction->amount; + $originalDestAmount = $destTransaction->amount; $origSourceTransaction->amount = Steam::negative($amount); $origSourceTransaction->balance_dirty = true; - $origSourceTransaction->save(); - $destTransaction = $this->getDestinationTransaction(); - $originalAmount = $destTransaction->amount; $destTransaction->amount = Steam::positive($amount); $destTransaction->balance_dirty = true; $destTransaction->save(); + $origSourceTransaction->save(); + // refresh transactions. $this->sourceTransaction->refresh(); $this->destinationTransaction->refresh(); Log::debug(sprintf('Updated amount to "%s"', $amount)); - $group = $this->transactionGroup; + $group = $this->transactionGroup; if (null === $group) { $group = $this->transactionJournal?->transactionGroup; } - if (null === $group) { + if (null === $group || null === $this->transactionJournal) { return; } - - // should not return in NULL but seems to do. - if (0 !== bccomp($originalAmount, $value)) { - event(new TriggeredAuditLog( - $group->user, - $group, - 'update_amount', - [ - 'currency_symbol' => $destTransaction->transactionCurrency->symbol, - 'decimal_places' => $destTransaction->transactionCurrency->decimal_places, - 'amount' => $originalAmount, - ], - [ - 'currency_symbol' => $destTransaction->transactionCurrency->symbol, - 'decimal_places' => $destTransaction->transactionCurrency->decimal_places, - 'amount' => $value, - ] - )); + if (0 === bccomp($origSourceTransaction->amount, $originalSourceAmount)) { + Log::debug('Amount was not actually changed, return.'); + return; } + Log::debug('Amount was changed.'); + $transfer = TransactionTypeEnum::TRANSFER->value === $this->transactionJournal->transactionType->type; + $withdrawal = TransactionTypeEnum::WITHDRAWAL->value === $this->transactionJournal->transactionType->type; + $deposit = TransactionTypeEnum::DEPOSIT->value === $this->transactionJournal->transactionType->type; + $makePositive = $transfer || $deposit ? true : false; + + // assume withdrawal, use the source for amount (negative), and destination for currency. + $originalAmount = $originalSourceAmount; + $recordCurrency = $destTransaction->transactionCurrency; + Log::debug(sprintf('Transaction is a %s, original amount is %s and currency is %s', $this->transactionJournal->transactionType->type, $originalAmount, $recordCurrency->code)); + if ($withdrawal || $transfer) { + Log::debug('Use these values to record a changed withdrawal amount'); + } + if (!$withdrawal && !$transfer) { + $originalAmount = $originalDestAmount; + $recordCurrency = $origSourceTransaction->transactionCurrency; + Log::debug('Use destination amount to record a changed withdrawal amount'); + Log::debug(sprintf('Transaction is a %s, original amount now is %s and currency is now %s', $this->transactionJournal->transactionType->type, $originalAmount, $recordCurrency->code)); + } + $originalAmount = $makePositive ? Steam::positive($originalAmount) : Steam::negative($originalAmount); + $value = $makePositive ? Steam::positive($value) : Steam::negative($value); + // should not return in NULL but seems to do. + event(new TriggeredAuditLog( + $group->user, + $group, + 'update_amount', + [ + 'currency_symbol' => $recordCurrency->symbol, + 'decimal_places' => $recordCurrency->decimal_places, + 'amount' => $originalAmount, + ], + [ + 'currency_symbol' => $recordCurrency->symbol, + 'decimal_places' => $recordCurrency->decimal_places, + 'amount' => $value, + ] + )); } private function updateForeignAmount(): void @@ -738,9 +763,9 @@ class JournalUpdateService // if the transaction is a TRANSFER, and the foreign amount and currency are set (like they seem to be) // the correct fields to update in the destination transaction are NOT the foreign amount and currency // but rather the normal amount and currency. This is new behavior. - $isTransfer = TransactionTypeEnum::TRANSFER->value === $this->transactionJournal->transactionType->type; + $isTransfer = TransactionTypeEnum::TRANSFER->value === $this->transactionJournal->transactionType->type; // also check if it is not between an asset account and a liability, because then the same rule applies. - $isBetween = $this->isBetweenAssetAndLiability(); + $isBetween = $this->isBetweenAssetAndLiability(); if ($isTransfer || $isBetween) { Log::debug('Switch amounts, store in amount and not foreign_amount'); @@ -776,8 +801,8 @@ class JournalUpdateService $source->foreign_amount = null; $source->save(); - $dest->foreign_currency_id = null; - $dest->foreign_amount = null; + $dest->foreign_currency_id = null; + $dest->foreign_amount = null; $dest->save(); Log::debug(sprintf('Foreign amount is "%s" so remove foreign amount info.', $amount)); } @@ -791,7 +816,7 @@ class JournalUpdateService private function isBetweenAssetAndLiability(): bool { /** @var null|Transaction $sourceTransaction */ - $sourceTransaction = $this->transactionJournal->transactions()->where('amount', '<', 0)->first(); + $sourceTransaction = $this->transactionJournal->transactions()->where('amount', '<', 0)->first(); /** @var null|Transaction $destinationTransaction */ $destinationTransaction = $this->transactionJournal->transactions()->where('amount', '>', 0)->first(); @@ -806,15 +831,15 @@ class JournalUpdateService return false; } - $source = $sourceTransaction->account; - $destination = $destinationTransaction->account; + $source = $sourceTransaction->account; + $destination = $destinationTransaction->account; if (null === $source || null === $destination) { Log::warning('Either is false, stop.'); return false; } - $sourceTypes = [AccountTypeEnum::LOAN->value, AccountTypeEnum::DEBT->value, AccountTypeEnum::MORTGAGE->value]; + $sourceTypes = [AccountTypeEnum::LOAN->value, AccountTypeEnum::DEBT->value, AccountTypeEnum::MORTGAGE->value]; // source is liability, destination is asset if (in_array($source->accountType->type, $sourceTypes, true) && AccountTypeEnum::ASSET->value === $destination->accountType->type) { diff --git a/resources/lang/en_US/firefly.php b/resources/lang/en_US/firefly.php index 25bb670e47..a26b76a875 100644 --- a/resources/lang/en_US/firefly.php +++ b/resources/lang/en_US/firefly.php @@ -2908,6 +2908,7 @@ return [ 'placeholder' => '[Placeholder]', // audit log entries + 'incomplete_ale' => 'Not all events are recorded as audit log entries.', 'audit_log_entries' => 'Audit log entries', 'ale_action_log_add' => 'Added :amount to piggy bank ":name"', 'ale_action_log_remove' => 'Removed :amount from piggy bank ":name"', diff --git a/resources/views/list/ale.twig b/resources/views/list/ale.twig index 7be50457b0..495515c4da 100644 --- a/resources/views/list/ale.twig +++ b/resources/views/list/ale.twig @@ -1,16 +1,21 @@ + + + + + + {% for logEntry in logEntries %} {% endfor %} +
{{ 'incomplete_ale'|_ }}
{# link to object: #} {% if 'FireflyIII\\Models\\Rule' == logEntry.changer_type %} - - {% endif %} + + {% endif %} {% if 'FireflyIII\\User' == logEntry.changer_type %} {% endif %} - {{ logEntry.changer_type|replace({"FireflyIII\\Models\\": ""})|replace({"FireflyIII\\": ""}) }} - #{{ logEntry.changer_id }} + {{ logEntry.changer_type|replace({"FireflyIII\\Models\\": ""})|replace({"FireflyIII\\": ""}) }} #{{ logEntry.changer_id }} @@ -103,4 +108,5 @@