From 78122a7d4b022c7d3907a36be75acfc224f8a9d6 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sun, 1 Oct 2023 11:17:57 +0200 Subject: [PATCH] Optimize validation command. --- .../Commands/Correction/FixAccountTypes.php | 167 ++++++++++++++---- app/Providers/EventServiceProvider.php | 1 - 2 files changed, 133 insertions(+), 35 deletions(-) diff --git a/app/Console/Commands/Correction/FixAccountTypes.php b/app/Console/Commands/Correction/FixAccountTypes.php index 019cb655ee..104c8221c8 100644 --- a/app/Console/Commands/Correction/FixAccountTypes.php +++ b/app/Console/Commands/Correction/FixAccountTypes.php @@ -24,6 +24,7 @@ declare(strict_types=1); namespace FireflyIII\Console\Commands\Correction; use FireflyIII\Console\Commands\ShowsFriendlyMessages; +use FireflyIII\Enums\AccountTypeEnum; use FireflyIII\Exceptions\FireflyException; use FireflyIII\Factory\AccountFactory; use FireflyIII\Models\AccountType; @@ -31,7 +32,8 @@ use FireflyIII\Models\Transaction; use FireflyIII\Models\TransactionJournal; use FireflyIII\Models\TransactionType; use Illuminate\Console\Command; -use Illuminate\Support\Facades\Log; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Query\JoinClause; use JsonException; /** @@ -41,8 +43,8 @@ class FixAccountTypes extends Command { use ShowsFriendlyMessages; - protected $description = 'Make sure all journals have the correct from/to account types.'; - protected $signature = 'firefly-iii:fix-account-types'; + protected $description = 'Make sure all journals have the correct from/to account types.'; + protected $signature = 'firefly-iii:fix-account-types'; private int $count; private array $expected; private AccountFactory $factory; @@ -51,22 +53,79 @@ class FixAccountTypes extends Command * Execute the console command. * * @return int - * @throws FireflyException + * @throws FireflyException|JsonException */ public function handle(): int { $this->stupidLaravel(); $this->factory = app(AccountFactory::class); $this->expected = config('firefly.source_dests'); - $journals = TransactionJournal::with(['TransactionType', 'transactions', 'transactions.account', 'transactions.account.accounttype'])->get(); - foreach ($journals as $journal) { - $this->inspectJournal($journal); + $expected = config('firefly.source_dests'); + + $query = TransactionJournal:: + leftJoin('transaction_types', 'transaction_journals.transaction_type_id', '=', 'transaction_types.id') + ->leftJoin( + 'transactions as source', + static function (JoinClause $join) { + $join->on('transaction_journals.id', '=', 'source.transaction_journal_id')->where('source.amount', '<', 0); + } + ) + ->leftJoin( + 'transactions as destination', + static function (JoinClause $join) { + $join->on('transaction_journals.id', '=', 'destination.transaction_journal_id')->where('destination.amount', '>', 0); + } + ) + ->leftJoin('accounts as source_account', 'source.account_id', '=', 'source_account.id') + ->leftJoin('accounts as destination_account', 'destination.account_id', '=', 'destination_account.id') + ->leftJoin('account_types as source_account_type', 'source_account.account_type_id', '=', 'source_account_type.id') + ->leftJoin('account_types as destination_account_type', 'destination_account.account_type_id', '=', 'destination_account_type.id'); + + // list all valid combinations, those are allowed. So we select those which are broken. + $query->where(function (Builder $q) use ($expected) { + foreach ($expected as $transactionType => $info) { + foreach ($info as $source => $destinations) { + foreach ($destinations as $destination) { + $q->whereNot(function (Builder $q1) use ($transactionType, $source, $destination) { + $q1->where('transaction_types.type', $transactionType); + $q1->where('source_account_type.type', $source); + $q1->where('destination_account_type.type', $destination); + }); + } + } + } + }); + + $resultSet = $query->get( + [ + 'transaction_journals.id', + //'transaction_type_id as type_id', + 'transaction_types.type as journal_type', + //'source.id as source_transaction_id', + //'source_account.id as source_account_id', + //'source_account_type.id as source_account_type_id', + 'source_account_type.type as source_account_type', + //'destination.id as destination_transaction_id', + //'destination_account.id as destination_account_id', + //'destination_account_type.id as destination_account_type_id', + 'destination_account_type.type as destination_account_type', + ] + ); + if ($resultSet->count() > 0) { + $this->friendlyLine(sprintf('Found %d journals that need to be fixed.', $resultSet->count())); + foreach ($resultSet as $entry) { + app('log')->debug(sprintf('Now fixing journal #%d', $entry->id)); + $journal = TransactionJournal::find((int)$entry->id); + if (null !== $journal) { + $this->inspectJournal($journal); + } + } } if (0 === $this->count) { $this->friendlyPositive('All account types are OK'); } if (0 !== $this->count) { - Log::debug(sprintf('%d journals had to be fixed.', $this->count)); + app('log')->debug(sprintf('%d journals had to be fixed.', $this->count)); $this->friendlyInfo(sprintf('Acted on %d transaction(s)', $this->count)); } @@ -74,11 +133,7 @@ class FixAccountTypes extends Command } /** - * Laravel will execute ALL __construct() methods for ALL commands whenever a SINGLE command is - * executed. This leads to noticeable slow-downs and class calls. To prevent this, this method should - * be called from the handle method instead of using the constructor to initialize the command. - * - + * @return void */ private function stupidLaravel(): void { @@ -93,9 +148,10 @@ class FixAccountTypes extends Command */ private function inspectJournal(TransactionJournal $journal): void { + app('log')->debug(sprintf('Now inspecting journal #%d', $journal->id)); $transactions = $journal->transactions()->count(); if (2 !== $transactions) { - Log::debug(sprintf('Journal has %d transactions, so can\'t fix.', $transactions)); + app('log')->debug(sprintf('Journal has %d transactions, so can\'t fix.', $transactions)); $this->friendlyError(sprintf('Cannot inspect transaction journal #%d because it has %d transaction(s) instead of 2.', $journal->id, $transactions)); return; @@ -109,20 +165,20 @@ class FixAccountTypes extends Command $destAccountType = $destAccount->accountType->type; if (!array_key_exists($type, $this->expected)) { - Log::info(sprintf('No source/destination info for transaction type %s.', $type)); + app('log')->info(sprintf('No source/destination info for transaction type %s.', $type)); $this->friendlyError(sprintf('No source/destination info for transaction type %s.', $type)); return; } if (!array_key_exists($sourceAccountType, $this->expected[$type])) { - Log::debug(sprintf('Going to fix journal #%d', $journal->id)); + app('log')->debug(sprintf('[a] Going to fix journal #%d', $journal->id)); $this->fixJournal($journal, $type, $sourceTransaction, $destTransaction); return; } $expectedTypes = $this->expected[$type][$sourceAccountType]; if (!in_array($destAccountType, $expectedTypes, true)) { - Log::debug(sprintf('Going to fix journal #%d', $journal->id)); + app('log')->debug(sprintf('[b] Going to fix journal #%d', $journal->id)); $this->fixJournal($journal, $type, $sourceTransaction, $destTransaction); } } @@ -158,10 +214,11 @@ class FixAccountTypes extends Command */ private function fixJournal(TransactionJournal $journal, string $type, Transaction $source, Transaction $dest): void { + app('log')->debug(sprintf('Going to fix journal #%d', $journal->id)); $this->count++; // variables: $combination = sprintf('%s%s%s', $type, $source->account->accountType->type, $dest->account->accountType->type); - + app('log')->debug(sprintf('Combination is "%s"', $combination)); switch ($combination) { case sprintf('%s%s%s', TransactionType::TRANSFER, AccountType::ASSET, AccountType::LOAN): case sprintf('%s%s%s', TransactionType::TRANSFER, AccountType::ASSET, AccountType::DEBT): @@ -172,10 +229,10 @@ class FixAccountTypes extends Command $journal->save(); $message = sprintf('Converted transaction #%d from a transfer to a withdrawal.', $journal->id); $this->friendlyInfo($message); - Log::debug($message); + app('log')->debug($message); // check it again: $this->inspectJournal($journal); - break; + return; case sprintf('%s%s%s', TransactionType::TRANSFER, AccountType::LOAN, AccountType::ASSET): case sprintf('%s%s%s', TransactionType::TRANSFER, AccountType::DEBT, AccountType::ASSET): case sprintf('%s%s%s', TransactionType::TRANSFER, AccountType::MORTGAGE, AccountType::ASSET): @@ -185,11 +242,11 @@ class FixAccountTypes extends Command $journal->save(); $message = sprintf('Converted transaction #%d from a transfer to a deposit.', $journal->id); $this->friendlyInfo($message); - Log::debug($message); + app('log')->debug($message); // check it again: $this->inspectJournal($journal); - break; + return; case sprintf('%s%s%s', TransactionType::WITHDRAWAL, AccountType::ASSET, AccountType::REVENUE): // withdrawals with a revenue account as destination instead of an expense account. $this->factory->setUser($journal->user); @@ -206,9 +263,9 @@ class FixAccountTypes extends Command $result->name ); $this->friendlyWarning($message); - Log::debug($message); + app('log')->debug($message); $this->inspectJournal($journal); - break; + return; case sprintf('%s%s%s', TransactionType::DEPOSIT, AccountType::EXPENSE, AccountType::ASSET): // deposits with an expense account as source instead of a revenue account. // find revenue account. @@ -226,19 +283,61 @@ class FixAccountTypes extends Command $result->name ); $this->friendlyWarning($message); - Log::debug($message); + app('log')->debug($message); $this->inspectJournal($journal); - break; - default: - $message = sprintf('The source account of %s #%d cannot be of type "%s".', $type, $journal->id, $source->account->accountType->type); - $this->friendlyError($message); - Log::debug($message); + return; + } + app('log')->debug(sprintf('Fallback to fix transaction journal #%d of type "%s".', $journal->id, $type)); - $message = sprintf('The destination account of %s #%d cannot be of type "%s".', $type, $journal->id, $dest->account->accountType->type); - $this->friendlyError($message); - Log::debug($message); + // transaction has no valid source. + $validSources = array_keys($this->expected[$type]); + if (!in_array($source->account->accountType->type, $validSources, true)) { + app('log')->debug('Journal has no valid source.'); + // perhaps we can create the account of type we need: - break; + if (in_array(AccountTypeEnum::REVENUE->value, $validSources, true)) { + app('log')->debug(sprintf('An account of type "%s" could be a valid source.', AccountTypeEnum::REVENUE->value)); + $this->factory->setUser($journal->user); + $newSource = $this->factory->findOrCreate($source->account->name, AccountTypeEnum::REVENUE->value); + $source->account()->associate($newSource); + $source->save(); + $this->friendlyPositive(sprintf('Firefly III gave transaction #%d a new source %s: #%d ("%s").', $journal->transaction_group_id, AccountTypeEnum::REVENUE->value, $newSource->id, $newSource->name)); + app('log')->debug(sprintf('Associated account #%d with transaction #%d', $newSource->id, $source->id)); + $this->inspectJournal($journal); + return; + } + if (!in_array(AccountTypeEnum::REVENUE->value, $validSources, true)) { + app('log')->debug('This transaction type has no source we can create. Just give error.'); + $message = sprintf('The source account of %s #%d cannot be of type "%s". Firefly III cannot fix this. You may have to remove the transaction yourself.', $type, $journal->id, $source->account->accountType->type); + $this->friendlyError($message); + app('log')->debug($message); + } + } + + // transaction has no valid destination: + $sourceType = $source->account->accountType->type; + $validDestinations = $this->expected[$type][$sourceType] ?? []; + if (!in_array($dest->account->accountType->type, $validDestinations, true)) { + app('log')->debug('Journal has no valid destination (perhaps because the source is also broken).'); + // perhaps we can create the account of type we need: + if (in_array(AccountTypeEnum::EXPENSE->value, $validDestinations, true)) { + app('log')->debug(sprintf('An account of type "%s" could be a valid destination.', AccountTypeEnum::EXPENSE->value)); + $this->factory->setUser($journal->user); + $newDestination = $this->factory->findOrCreate($dest->account->name, AccountTypeEnum::EXPENSE->value); + $dest->account()->associate($newDestination); + $dest->save(); + $this->friendlyPositive(sprintf('Firefly III gave transaction #%d a new destination %s: #%d ("%s").', $journal->transaction_group_id, AccountTypeEnum::EXPENSE->value, $newDestination->id, $newDestination->name)); + app('log')->debug(sprintf('Associated account #%d with transaction #%d', $newDestination->id, $source->id)); + $this->inspectJournal($journal); + return; + } + if (!in_array(AccountTypeEnum::EXPENSE->value, $validSources, true)) { + app('log')->debug('This transaction type has no destination we can create. Just give error.'); + $message = sprintf('The destination account of %s #%d cannot be of type "%s". Firefly III cannot fix this. You may have to remove the transaction yourself.', $type, $journal->id, $dest->account->accountType->type); + $this->friendlyError($message); + app('log')->debug($message); + return; + } } } } diff --git a/app/Providers/EventServiceProvider.php b/app/Providers/EventServiceProvider.php index ed7df41697..0d93ba7b23 100644 --- a/app/Providers/EventServiceProvider.php +++ b/app/Providers/EventServiceProvider.php @@ -227,7 +227,6 @@ class EventServiceProvider extends ServiceProvider */ private function registerObservers(): void { - app('log')->debug('Register observers'); Attachment::observe(new AttachmentObserver()); PiggyBank::observe(new PiggyBankObserver()); Account::observe(new AccountObserver());