From 7e37d1001607a3deb3db575eb66b53127b032329 Mon Sep 17 00:00:00 2001 From: James Cole Date: Tue, 30 Jul 2024 18:04:39 +0200 Subject: [PATCH] New way to store transfers --- .../Models/Transaction/UpdateController.php | 2 +- .../Commands/Correction/FixUnevenAmount.php | 85 ++++++++++++++----- app/Factory/TransactionJournalFactory.php | 19 ++++- .../Internal/Update/JournalUpdateService.php | 26 +++++- 4 files changed, 108 insertions(+), 24 deletions(-) diff --git a/app/Api/V1/Controllers/Models/Transaction/UpdateController.php b/app/Api/V1/Controllers/Models/Transaction/UpdateController.php index 36c4aba133..64ded3723b 100644 --- a/app/Api/V1/Controllers/Models/Transaction/UpdateController.php +++ b/app/Api/V1/Controllers/Models/Transaction/UpdateController.php @@ -69,7 +69,7 @@ class UpdateController extends Controller */ public function update(UpdateRequest $request, TransactionGroup $transactionGroup): JsonResponse { - app('log')->debug('Now in update routine for transaction group!'); + app('log')->debug('Now in update routine for transaction group'); $data = $request->getAll(); // Fixes 8750. diff --git a/app/Console/Commands/Correction/FixUnevenAmount.php b/app/Console/Commands/Correction/FixUnevenAmount.php index 83f8cc9b5c..018a2a5e8b 100644 --- a/app/Console/Commands/Correction/FixUnevenAmount.php +++ b/app/Console/Commands/Correction/FixUnevenAmount.php @@ -26,6 +26,8 @@ namespace FireflyIII\Console\Commands\Correction; use FireflyIII\Console\Commands\ShowsFriendlyMessages; use FireflyIII\Models\Transaction; use FireflyIII\Models\TransactionJournal; +use FireflyIII\Models\TransactionType; +use FireflyIII\Support\Facades\Steam; use Illuminate\Console\Command; use Illuminate\Support\Facades\Log; @@ -38,12 +40,14 @@ class FixUnevenAmount extends Command protected $description = 'Fix journals with uneven amounts.'; protected $signature = 'firefly-iii:fix-uneven-amount'; + private int $count; /** * Execute the console command. */ public function handle(): int { + $this->count = 0; $this->fixUnevenAmounts(); $this->matchCurrencies(); @@ -71,7 +75,7 @@ class FixUnevenAmount extends Command ); Transaction::where('transaction_journal_id', $journal->id ?? 0)->forceDelete(); TransactionJournal::where('id', $journal->id ?? 0)->forceDelete(); - + ++$this->count; return; } @@ -92,20 +96,31 @@ class FixUnevenAmount extends Command Transaction::where('transaction_journal_id', $journal->id ?? 0)->forceDelete(); TransactionJournal::where('id', $journal->id ?? 0)->forceDelete(); - + ++$this->count; return; } + // may still be able to salvage this journal if it is a transfer with foreign currency info + if($this->isForeignCurrencyTransfer($journal)) { + Log::debug(sprintf('Can skip foreign currency transfer #%d.', $journal->id)); + return; + } + + $message = sprintf('Sum of journal #%d is not zero, journal is broken and now fixed.', $journal->id); + + $this->friendlyWarning($message); + app('log')->warning($message); + $destination->amount = $amount; $destination->save(); $message = sprintf('Corrected amount in transaction journal #%d', $param); $this->friendlyInfo($message); + ++$this->count; } private function fixUnevenAmounts(): void { - $count = 0; $journals = \DB::table('transactions') ->groupBy('transaction_journal_id') ->whereNull('deleted_at') @@ -126,7 +141,7 @@ class FixUnevenAmount extends Command ); $this->friendlyWarning($message); app('log')->warning($message); - ++$count; + ++$this->count; continue; } @@ -140,18 +155,10 @@ class FixUnevenAmount extends Command Log::error($e->getTraceAsString()); } if (0 !== $res) { - $message = sprintf( - 'Sum of journal #%d is %s instead of zero.', - $entry->transaction_journal_id, - $entry->the_sum - ); - $this->friendlyWarning($message); - app('log')->warning($message); $this->fixJournal($entry->transaction_journal_id); - ++$count; } } - if (0 === $count) { + if (0 === $this->count) { $this->friendlyPositive('Database amount integrity is OK'); } } @@ -160,18 +167,56 @@ class FixUnevenAmount extends Command { $journals = TransactionJournal::leftJoin('transactions', 'transaction_journals.id', 'transactions.transaction_journal_id') ->where('transactions.transaction_currency_id', '!=', \DB::raw('transaction_journals.transaction_currency_id')) - ->get(['transaction_journals.*']) - ; - if (0 === $journals->count()) { + ->get(['transaction_journals.*']); + + $count = 0; + /** @var TransactionJournal $journal */ + foreach ($journals as $journal) { + if(!$this->isForeignCurrencyTransfer($journal)) { + Transaction::where('transaction_journal_id', $journal->id)->update(['transaction_currency_id' => $journal->transaction_currency_id]); + $count++; + continue; + } + Log::debug(sprintf('Can skip foreign currency transfer #%d.', $journal->id)); + } + if (0 === $count) { $this->friendlyPositive('Journal currency integrity is OK'); return; } - /** @var TransactionJournal $journal */ - foreach ($journals as $journal) { - Transaction::where('transaction_journal_id', $journal->id)->update(['transaction_currency_id' => $journal->transaction_currency_id]); - } $this->friendlyPositive(sprintf('Fixed %d journal(s) with mismatched currencies.', $journals->count())); } + + private function isForeignCurrencyTransfer(TransactionJournal $journal): bool + { + if(TransactionType::TRANSFER !== $journal->transactionType->type) { + return false; + } + /** @var Transaction $destination */ + $destination = $journal->transactions()->where('amount', '>', 0)->first(); + /** @var Transaction $source */ + $source = $journal->transactions()->where('amount', '<', 0)->first(); + + // safety catch on NULL should not be necessary, we just had that catch. + // source amount = dest foreign amount + // source currency = dest foreign currency + // dest amount = source foreign currency + // dest currency = source foreign currency + +// Log::debug(sprintf('[a] %s', bccomp(app('steam')->positive($source->amount), app('steam')->positive($destination->foreign_amount)))); +// Log::debug(sprintf('[b] %s', bccomp(app('steam')->positive($destination->amount), app('steam')->positive($source->foreign_amount)))); +// Log::debug(sprintf('[c] %s', var_export($source->transaction_currency_id === $destination->foreign_currency_id,true))); +// Log::debug(sprintf('[d] %s', var_export((int) $destination->transaction_currency_id ===(int) $source->foreign_currency_id, true))); + + if(0 === bccomp(app('steam')->positive($source->amount), app('steam')->positive($destination->foreign_amount)) && + $source->transaction_currency_id === $destination->foreign_currency_id && + 0 === bccomp(app('steam')->positive($destination->amount), app('steam')->positive($source->foreign_amount)) && + (int) $destination->transaction_currency_id === (int) $source->foreign_currency_id + ) { + return true; + } + return false; + + } } diff --git a/app/Factory/TransactionJournalFactory.php b/app/Factory/TransactionJournalFactory.php index bbe9ce33b1..063ce5eeb4 100644 --- a/app/Factory/TransactionJournalFactory.php +++ b/app/Factory/TransactionJournalFactory.php @@ -47,6 +47,7 @@ use FireflyIII\Support\NullArrayObject; use FireflyIII\User; use FireflyIII\Validation\AccountValidator; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\Log; /** * Class TransactionJournalFactory @@ -261,8 +262,24 @@ class TransactionJournalFactory $transactionFactory->setForeignCurrency($foreignCurrency); $transactionFactory->setReconciled($row['reconciled'] ?? false); + // if the foreign currency is set and is different, and the transaction type is a transfer, + // Firefly III will save the foreign currency information in such a way that both + // asset accounts can look at the "amount" and "transaction_currency_id" column and + // see the currency they expect to see. + $amount = (string)$row['amount']; + $foreignAmount = (string)$row['foreign_amount']; + if(null !== $foreignCurrency && $foreignCurrency->id !== $currency->id && + TransactionType::TRANSFER === $type->type + ) { + $transactionFactory->setCurrency($foreignCurrency); + $transactionFactory->setForeignCurrency($currency); + $amount = (string)$row['foreign_amount']; + $foreignAmount = (string)$row['amount']; + Log::debug('Swap native/foreign amounts in transfer for new save method.'); + } + try { - $transactionFactory->createPositive((string)$row['amount'], (string)$row['foreign_amount']); + $transactionFactory->createPositive($amount, $foreignAmount); } catch (FireflyException $e) { app('log')->error(sprintf('Exception creating positive transaction: %s', $e->getMessage())); $this->forceTrDelete($negative); diff --git a/app/Services/Internal/Update/JournalUpdateService.php b/app/Services/Internal/Update/JournalUpdateService.php index 43a920bac5..a2692dd71b 100644 --- a/app/Services/Internal/Update/JournalUpdateService.php +++ b/app/Services/Internal/Update/JournalUpdateService.php @@ -33,6 +33,7 @@ use FireflyIII\Factory\TransactionJournalMetaFactory; use FireflyIII\Factory\TransactionTypeFactory; use FireflyIII\Models\Account; use FireflyIII\Models\Transaction; +use FireflyIII\Models\TransactionCurrency; use FireflyIII\Models\TransactionGroup; use FireflyIII\Models\TransactionJournal; use FireflyIII\Models\TransactionType; @@ -44,6 +45,7 @@ use FireflyIII\Repositories\UserGroups\Currency\CurrencyRepositoryInterface; use FireflyIII\Services\Internal\Support\JournalServiceTrait; use FireflyIII\Support\NullArrayObject; use FireflyIII\Validation\AccountValidator; +use Illuminate\Support\Facades\Log; /** * Class to centralise code that updates a journal given the input by system. @@ -698,8 +700,23 @@ class JournalUpdateService $source->foreign_currency_id = $foreignCurrency->id; $source->foreign_amount = app('steam')->negative($foreignAmount); $source->save(); - $dest->foreign_currency_id = $foreignCurrency->id; - $dest->foreign_amount = app('steam')->positive($foreignAmount); + + // 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. + + if(TransactionType::TRANSFER === $this->transactionJournal->transactionType->type) { + Log::debug('Switch amounts, store in amount and not foreign_amount'); + $dest->transaction_currency_id = $foreignCurrency->id; + $dest->amount = app('steam')->positive($foreignAmount); + } + if(TransactionType::TRANSFER !== $this->transactionJournal->transactionType->type) { + $dest->foreign_currency_id = $foreignCurrency->id; + $dest->foreign_amount = app('steam')->positive($foreignAmount); + } + + + $dest->save(); app('log')->debug( @@ -733,4 +750,9 @@ class JournalUpdateService $this->sourceTransaction->refresh(); $this->destinationTransaction->refresh(); } + + private function collectCurrency(): TransactionCurrency + { + + } }