From d1c87e1c21f9f7ed522c4d2800c8550b2da70b60 Mon Sep 17 00:00:00 2001 From: James Cole Date: Tue, 23 Mar 2021 06:42:26 +0100 Subject: [PATCH] Fix issues where rule action would use old data. --- .../Actions/AppendDescription.php | 3 +- app/TransactionRules/Actions/AppendNotes.php | 7 +- app/TransactionRules/Actions/ClearBudget.php | 3 +- .../Actions/ClearCategory.php | 1 + app/TransactionRules/Actions/ClearNotes.php | 10 +- .../Actions/ConvertToDeposit.php | 99 ++++++------- .../Actions/ConvertToTransfer.php | 102 ++++++++------ .../Actions/ConvertToWithdrawal.php | 132 +++++++++--------- .../Actions/DeleteTransaction.php | 4 +- app/TransactionRules/Actions/LinkToBill.php | 13 +- .../Actions/PrependDescription.php | 3 +- app/TransactionRules/Actions/PrependNotes.php | 7 +- .../Actions/RemoveAllTags.php | 2 +- app/TransactionRules/Actions/RemoveTag.php | 2 +- app/TransactionRules/Actions/SetBudget.php | 13 +- app/TransactionRules/Actions/SetCategory.php | 19 ++- .../Actions/SetDescription.php | 1 + .../Actions/SetDestinationAccount.php | 51 ++++--- .../Actions/SetSourceAccount.php | 112 +++++++++------ .../Actions/UpdatePiggybank.php | 120 ++++++++-------- .../Engine/SearchRuleEngine.php | 6 +- 21 files changed, 406 insertions(+), 304 deletions(-) diff --git a/app/TransactionRules/Actions/AppendDescription.php b/app/TransactionRules/Actions/AppendDescription.php index 8aa67a4209..c7f275ebf1 100644 --- a/app/TransactionRules/Actions/AppendDescription.php +++ b/app/TransactionRules/Actions/AppendDescription.php @@ -22,8 +22,8 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; -use FireflyIII\Models\RuleAction; use DB; +use FireflyIII\Models\RuleAction; /** * Class AppendDescription. @@ -49,6 +49,7 @@ class AppendDescription implements ActionInterface { $description = sprintf('%s%s', $journal['description'], $this->action->action_value); DB::table('transaction_journals')->where('id', $journal['transaction_journal_id'])->limit(1)->update(['description' => $description]); + return true; } } diff --git a/app/TransactionRules/Actions/AppendNotes.php b/app/TransactionRules/Actions/AppendNotes.php index 949ae9721f..608515f777 100644 --- a/app/TransactionRules/Actions/AppendNotes.php +++ b/app/TransactionRules/Actions/AppendNotes.php @@ -52,19 +52,20 @@ class AppendNotes implements ActionInterface { $dbNote = Note :: - where('noteable_id', (int) $journal['transaction_journal_id']) + where('noteable_id', (int)$journal['transaction_journal_id']) ->where('noteable_type', TransactionJournal::class) ->first(['notes.*']); if (null === $dbNote) { $dbNote = new Note; - $dbNote->noteable_id = (int) $journal['transaction_journal_id']; + $dbNote->noteable_id = (int)$journal['transaction_journal_id']; $dbNote->noteable_type = TransactionJournal::class; $dbNote->text = ''; } Log::debug(sprintf('RuleAction AppendNotes appended "%s" to "%s".', $this->action->action_value, $dbNote->text)); - $text = sprintf('%s%s', $dbNote->text, $this->action->action_value); + $text = sprintf('%s%s', $dbNote->text, $this->action->action_value); $dbNote->text = $text; $dbNote->save(); + return true; } } diff --git a/app/TransactionRules/Actions/ClearBudget.php b/app/TransactionRules/Actions/ClearBudget.php index e568558cba..ebf8e89f88 100644 --- a/app/TransactionRules/Actions/ClearBudget.php +++ b/app/TransactionRules/Actions/ClearBudget.php @@ -22,9 +22,10 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; +use DB; use FireflyIII\Models\RuleAction; use Log; -use DB; + /** * Class ClearBudget. */ diff --git a/app/TransactionRules/Actions/ClearCategory.php b/app/TransactionRules/Actions/ClearCategory.php index 567d68bc5d..51133e39e4 100644 --- a/app/TransactionRules/Actions/ClearCategory.php +++ b/app/TransactionRules/Actions/ClearCategory.php @@ -21,6 +21,7 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; + use DB; use FireflyIII\Models\RuleAction; use Log; diff --git a/app/TransactionRules/Actions/ClearNotes.php b/app/TransactionRules/Actions/ClearNotes.php index 63d916b0e3..54c55546a4 100644 --- a/app/TransactionRules/Actions/ClearNotes.php +++ b/app/TransactionRules/Actions/ClearNotes.php @@ -22,10 +22,11 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; +use DB; use FireflyIII\Models\RuleAction; use FireflyIII\Models\TransactionJournal; use Log; -use DB; + /** * Class ClearNotes. */ @@ -46,10 +47,11 @@ class ClearNotes implements ActionInterface public function actOnArray(array $journal): bool { DB::table('notes') - ->where('noteable_id', $journal['transaction_journal_id']) - ->where('noteable_type', TransactionJournal::class) - ->delete(); + ->where('noteable_id', $journal['transaction_journal_id']) + ->where('noteable_type', TransactionJournal::class) + ->delete(); Log::debug(sprintf('RuleAction ClearNotes removed all notes.')); + return true; } } diff --git a/app/TransactionRules/Actions/ConvertToDeposit.php b/app/TransactionRules/Actions/ConvertToDeposit.php index 84d9227dbc..6694c6aee8 100644 --- a/app/TransactionRules/Actions/ConvertToDeposit.php +++ b/app/TransactionRules/Actions/ConvertToDeposit.php @@ -52,46 +52,31 @@ class ConvertToDeposit implements ActionInterface } /** - * Input is a transfer from A to B. - * Output is a deposit from C to B. - * - * @param array $journal - * - * @return bool + * @inheritDoc * @throws FireflyException */ - private function convertTransferArray(array $journal): bool + public function actOnArray(array $journal): bool { - $user = User::find($journal['user_id']); - // find or create revenue account. - /** @var AccountFactory $factory */ - $factory = app(AccountFactory::class); - $factory->setUser($user); + Log::debug(sprintf('Convert journal #%d to deposit.', $journal['transaction_journal_id'])); + $type = $journal['transaction_type_type']; + if (TransactionType::DEPOSIT === $type) { + Log::error(sprintf('Journal #%d is already a deposit (rule #%d).', $journal['transaction_journal_id'], $this->action->rule_id)); - // get the action value, or use the original source name in case the action value is empty: - // this becomes a new or existing revenue account. - $revenueName = '' === $this->action->action_value ? $journal['source_account_name'] : $this->action->action_value; - $revenue = $factory->findOrCreate($revenueName, AccountType::REVENUE); + return false; + } - Log::debug(sprintf('ConvertToDeposit. Action value is "%s", revenue name is "%s"', $this->action->action_value, $journal['source_account_name'])); - unset($source); + if (TransactionType::WITHDRAWAL === $type) { + Log::debug('Going to transform a withdrawal to a deposit.'); - // update source transaction(s) to be revenue account - DB::table('transactions') - ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) - ->where('amount', '<', 0) - ->update(['account_id' => $revenue->id]); + return $this->convertWithdrawalArray($journal); + } + if (TransactionType::TRANSFER === $type) { + Log::debug('Going to transform a transfer to a deposit.'); - // change transaction type of journal: - $newType = TransactionType::whereType(TransactionType::DEPOSIT)->first(); + return $this->convertTransferArray($journal); + } - DB::table('transaction_journals') - ->where('id', '=', $journal['transaction_journal_id']) - ->update(['transaction_type_id' => $newType->id]); - - Log::debug('Converted transfer to deposit.'); - - return true; + return false; } /** @@ -143,29 +128,45 @@ class ConvertToDeposit implements ActionInterface } /** - * @inheritDoc + * Input is a transfer from A to B. + * Output is a deposit from C to B. + * + * @param array $journal + * + * @return bool * @throws FireflyException */ - public function actOnArray(array $journal): bool + private function convertTransferArray(array $journal): bool { - Log::debug(sprintf('Convert journal #%d to deposit.', $journal['transaction_journal_id'])); - $type = $journal['transaction_type_type']; - if (TransactionType::DEPOSIT === $type) { - Log::error(sprintf('Journal #%d is already a deposit (rule #%d).', $journal['transaction_journal_id'], $this->action->rule_id)); + $user = User::find($journal['user_id']); + // find or create revenue account. + /** @var AccountFactory $factory */ + $factory = app(AccountFactory::class); + $factory->setUser($user); - return false; - } + // get the action value, or use the original source name in case the action value is empty: + // this becomes a new or existing revenue account. + $revenueName = '' === $this->action->action_value ? $journal['source_account_name'] : $this->action->action_value; + $revenue = $factory->findOrCreate($revenueName, AccountType::REVENUE); - if (TransactionType::WITHDRAWAL === $type) { - Log::debug('Going to transform a withdrawal to a deposit.'); + Log::debug(sprintf('ConvertToDeposit. Action value is "%s", revenue name is "%s"', $this->action->action_value, $journal['source_account_name'])); + unset($source); - return $this->convertWithdrawalArray($journal); - } - if (TransactionType::TRANSFER === $type) { - Log::debug('Going to transform a transfer to a deposit.'); + // update source transaction(s) to be revenue account + DB::table('transactions') + ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) + ->where('amount', '<', 0) + ->update(['account_id' => $revenue->id]); - return $this->convertTransferArray($journal); - } - return false; + // change transaction type of journal: + $newType = TransactionType::whereType(TransactionType::DEPOSIT)->first(); + + DB::table('transaction_journals') + ->where('id', '=', $journal['transaction_journal_id']) + ->update(['transaction_type_id' => $newType->id]); + + Log::debug('Converted transfer to deposit.'); + + return true; } } diff --git a/app/TransactionRules/Actions/ConvertToTransfer.php b/app/TransactionRules/Actions/ConvertToTransfer.php index d4533c499b..b198df51ba 100644 --- a/app/TransactionRules/Actions/ConvertToTransfer.php +++ b/app/TransactionRules/Actions/ConvertToTransfer.php @@ -24,14 +24,15 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; +use DB; use FireflyIII\Models\Account; use FireflyIII\Models\AccountType; use FireflyIII\Models\RuleAction; +use FireflyIII\Models\TransactionJournal; use FireflyIII\Models\TransactionType; use FireflyIII\Repositories\Account\AccountRepositoryInterface; use FireflyIII\User; use Log; -use DB; /** * @@ -56,10 +57,12 @@ class ConvertToTransfer implements ActionInterface */ public function actOnArray(array $journal): bool { - $type = $journal['transaction_type_type']; - $user = User::find($journal['user_id']); + $type = $journal['transaction_type_type']; + $user = User::find($journal['user_id']); if (TransactionType::TRANSFER === $type) { - Log::error(sprintf('Journal #%d is already a transfer so cannot be converted (rule #%d).', $journal['transaction_journal_id'], $this->action->rule_id)); + Log::error( + sprintf('Journal #%d is already a transfer so cannot be converted (rule #%d).', $journal['transaction_journal_id'], $this->action->rule_id) + ); return false; } @@ -70,7 +73,12 @@ class ConvertToTransfer implements ActionInterface $repository->setUser($user); $asset = $repository->findByName($this->action->action_value, [AccountType::ASSET, AccountType::LOAN, AccountType::DEBT, AccountType::MORTGAGE]); if (null === $asset) { - Log::error(sprintf('Journal #%d cannot be converted because no asset with name "%s" exists (rule #%d).', $journal['transaction_journal_id'], $this->action->action_value, $this->action->rule_id)); + Log::error( + sprintf( + 'Journal #%d cannot be converted because no asset with name "%s" exists (rule #%d).', $journal['transaction_journal_id'], + $this->action->action_value, $this->action->rule_id + ) + ); return false; } @@ -88,49 +96,25 @@ class ConvertToTransfer implements ActionInterface return false; // @codeCoverageIgnore } - /** - * A deposit is from Revenue to Asset. - * We replace the Revenue with another asset. - * @param array $journal - * @param Account $asset - * @return bool - */ - private function convertDepositArray(array $journal, Account $asset): bool - { - if ($journal['destination_account_id'] === $asset->id) { - Log::error(vsprintf('Journal #%d has already has "%s" as a destination asset. ConvertToTransfer failed. (rule #%d).', [$journal['transaction_journal_id'], $asset->name, $this->action->rule_id])); - return false; - } - - // update source transaction: - DB::table('transactions') - ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) - ->where('amount', '<', 0) - ->update(['account_id' => $asset->id]); - - // change transaction type of journal: - $newType = TransactionType::whereType(TransactionType::TRANSFER)->first(); - - DB::table('transaction_journals') - ->where('id', '=', $journal['transaction_journal_id']) - ->update(['transaction_type_id' => $newType->id]); - - Log::debug('Converted deposit to transfer.'); - - return true; - } - /** * A withdrawal is from Asset to Expense. * We replace the Expense with another asset. + * * @param array $journal * @param Account $asset + * * @return bool */ private function convertWithdrawalArray(array $journal, Account $asset): bool { if ($journal['source_account_id'] === $asset->id) { - Log::error(vsprintf('Journal #%d has already has "%s" as a source asset. ConvertToTransfer failed. (rule #%d).', [$journal['transaction_journal_id'], $asset->name, $this->action->rule_id])); + Log::error( + vsprintf( + 'Journal #%d has already has "%s" as a source asset. ConvertToTransfer failed. (rule #%d).', + [$journal['transaction_journal_id'], $asset->name, $this->action->rule_id] + ) + ); + return false; } @@ -141,7 +125,7 @@ class ConvertToTransfer implements ActionInterface ->update(['account_id' => $asset->id]); // change transaction type of journal: - $newType = TransactionType::whereType(TransactionType::TRANSFER)->first(); + $newType = TransactionType::whereType(TransactionType::TRANSFER)->first(); DB::table('transaction_journals') ->where('id', '=', $journal['transaction_journal_id']) @@ -152,4 +136,44 @@ class ConvertToTransfer implements ActionInterface return true; } + /** + * A deposit is from Revenue to Asset. + * We replace the Revenue with another asset. + * + * @param array $journal + * @param Account $asset + * + * @return bool + */ + private function convertDepositArray(array $journal, Account $asset): bool + { + if ($journal['destination_account_id'] === $asset->id) { + Log::error( + vsprintf( + 'Journal #%d has already has "%s" as a destination asset. ConvertToTransfer failed. (rule #%d).', + [$journal['transaction_journal_id'], $asset->name, $this->action->rule_id] + ) + ); + + return false; + } + + // update source transaction: + DB::table('transactions') + ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) + ->where('amount', '<', 0) + ->update(['account_id' => $asset->id]); + + // change transaction type of journal: + $newType = TransactionType::whereType(TransactionType::TRANSFER)->first(); + + DB::table('transaction_journals') + ->where('id', '=', $journal['transaction_journal_id']) + ->update(['transaction_type_id' => $newType->id]); + + Log::debug('Converted deposit to transfer.'); + + return true; + } + } diff --git a/app/TransactionRules/Actions/ConvertToWithdrawal.php b/app/TransactionRules/Actions/ConvertToWithdrawal.php index 6d1d60fcc8..0143d8394b 100644 --- a/app/TransactionRules/Actions/ConvertToWithdrawal.php +++ b/app/TransactionRules/Actions/ConvertToWithdrawal.php @@ -24,6 +24,7 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; +use DB; use FireflyIII\Exceptions\FireflyException; use FireflyIII\Factory\AccountFactory; use FireflyIII\Models\AccountType; @@ -31,7 +32,6 @@ use FireflyIII\Models\RuleAction; use FireflyIII\Models\TransactionType; use FireflyIII\User; use Log; -use DB; /** * @@ -51,11 +51,76 @@ class ConvertToWithdrawal implements ActionInterface $this->action = $action; } + /** + * @inheritDoc + */ + public function actOnArray(array $journal): bool + { + $type = $journal['transaction_type_type']; + if (TransactionType::WITHDRAWAL === $type) { + Log::error(sprintf('Journal #%d is already a withdrawal (rule #%d).', $journal['transaction_journal_id'], $this->action->rule_id)); + + return false; + } + + if (TransactionType::DEPOSIT === $type) { + Log::debug('Going to transform a deposit to a withdrawal.'); + + return $this->convertDepositArray($journal); + } + if (TransactionType::TRANSFER === $type) { + Log::debug('Going to transform a transfer to a withdrawal.'); + + return $this->convertTransferArray($journal); + } + + return false; // @codeCoverageIgnore + } + + private function convertDepositArray(array $journal): bool + { + $user = User::find($journal['user_id']); + /** @var AccountFactory $factory */ + $factory = app(AccountFactory::class); + $factory->setUser($user); + + $expenseName = '' === $this->action->action_value ? $journal['source_account_name'] : $this->action->action_value; + $expense = $factory->findOrCreate($expenseName, AccountType::EXPENSE); + $destinationId = $journal['destination_account_id']; + + + Log::debug(sprintf('ConvertToWithdrawal. Action value is "%s", expense name is "%s"', $this->action->action_value, $expenseName)); + + // update source transaction(s) to be the original destination account + DB::table('transactions') + ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) + ->where('amount', '<', 0) + ->update(['account_id' => $destinationId]); + + // update destination transaction(s) to be new expense account. + DB::table('transactions') + ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) + ->where('amount', '>', 0) + ->update(['account_id' => $expense->id]); + + // change transaction type of journal: + $newType = TransactionType::whereType(TransactionType::WITHDRAWAL)->first(); + DB::table('transaction_journals') + ->where('id', '=', $journal['transaction_journal_id']) + ->update(['transaction_type_id' => $newType->id]); + + Log::debug('Converted deposit to withdrawal.'); + + return true; + + } + /** * Input is a transfer from A to B. * Output is a withdrawal from A to C. * * @param array $journal + * * @return bool * @throws FireflyException */ @@ -82,7 +147,7 @@ class ConvertToWithdrawal implements ActionInterface ->update(['account_id' => $expense->id]); // change transaction type of journal: - $newType = TransactionType::whereType(TransactionType::WITHDRAWAL)->first(); + $newType = TransactionType::whereType(TransactionType::WITHDRAWAL)->first(); DB::table('transaction_journals') ->where('id', '=', $journal['transaction_journal_id']) ->update(['transaction_type_id' => $newType->id]); @@ -91,67 +156,4 @@ class ConvertToWithdrawal implements ActionInterface return true; } - - /** - * @inheritDoc - */ - public function actOnArray(array $journal): bool - { - $type = $journal['transaction_type_type']; - if (TransactionType::WITHDRAWAL === $type) { - Log::error(sprintf('Journal #%d is already a withdrawal (rule #%d).', $journal['transaction_journal_id'], $this->action->rule_id)); - return false; - } - - if (TransactionType::DEPOSIT === $type) { - Log::debug('Going to transform a deposit to a withdrawal.'); - - return $this->convertDepositArray($journal); - } - if (TransactionType::TRANSFER === $type) { - Log::debug('Going to transform a transfer to a withdrawal.'); - - return $this->convertTransferArray($journal); - } - - return false; // @codeCoverageIgnore - } - - private function convertDepositArray(array $journal): bool - { - $user = User::find($journal['user_id']); - /** @var AccountFactory $factory */ - $factory = app(AccountFactory::class); - $factory->setUser($user); - - $expenseName = '' === $this->action->action_value ? $journal['source_account_name'] : $this->action->action_value; - $expense = $factory->findOrCreate($expenseName, AccountType::EXPENSE); - $destinationId = $journal['destination_account_id']; - - - Log::debug(sprintf('ConvertToWithdrawal. Action value is "%s", expense name is "%s"', $this->action->action_value, $expenseName)); - - // update source transaction(s) to be the original destination account - DB::table('transactions') - ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) - ->where('amount', '<', 0) - ->update(['account_id' => $destinationId]); - - // update destination transaction(s) to be new expense account. - DB::table('transactions') - ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) - ->where('amount', '>', 0) - ->update(['account_id' => $expense->id]); - - // change transaction type of journal: - $newType = TransactionType::whereType(TransactionType::WITHDRAWAL)->first(); - DB::table('transaction_journals') - ->where('id', '=', $journal['transaction_journal_id']) - ->update(['transaction_type_id' => $newType->id]); - - Log::debug('Converted deposit to withdrawal.'); - - return true; - - } } diff --git a/app/TransactionRules/Actions/DeleteTransaction.php b/app/TransactionRules/Actions/DeleteTransaction.php index 2cb6982025..c372216f4f 100644 --- a/app/TransactionRules/Actions/DeleteTransaction.php +++ b/app/TransactionRules/Actions/DeleteTransaction.php @@ -64,7 +64,9 @@ class DeleteTransaction implements ActionInterface return true; } - Log::debug(sprintf('RuleAction DeleteTransaction DELETED transaction journal #%d ("%s").', $journal['transaction_journal_id'], $journal['description'])); + Log::debug( + sprintf('RuleAction DeleteTransaction DELETED transaction journal #%d ("%s").', $journal['transaction_journal_id'], $journal['description']) + ); // trigger delete factory: $journal = TransactionJournal::find($journal['transaction_group_id']); diff --git a/app/TransactionRules/Actions/LinkToBill.php b/app/TransactionRules/Actions/LinkToBill.php index f0fee3d104..390a813912 100644 --- a/app/TransactionRules/Actions/LinkToBill.php +++ b/app/TransactionRules/Actions/LinkToBill.php @@ -58,19 +58,26 @@ class LinkToBill implements ActionInterface /** @var BillRepositoryInterface $repository */ $repository = app(BillRepositoryInterface::class); $repository->setUser($user); - $billName = (string) $this->action->action_value; + $billName = (string)$this->action->action_value; $bill = $repository->findByName($billName); if (null !== $bill && $journal['transaction_type_type'] === TransactionType::WITHDRAWAL) { DB::table('transaction_journals') ->where('id', '=', $journal['transaction_journal_id']) ->update(['bill_id' => $bill->id]); - Log::debug(sprintf('RuleAction LinkToBill set the bill of journal #%d to bill #%d ("%s").', $journal['transaction_journal_id'], $bill->id, $bill->name)); + Log::debug( + sprintf('RuleAction LinkToBill set the bill of journal #%d to bill #%d ("%s").', $journal['transaction_journal_id'], $bill->id, $bill->name) + ); return true; } - Log::error(sprintf('RuleAction LinkToBill could not set the bill of journal #%d to bill "%s": no such bill found or not a withdrawal.', $journal['transaction_journal_id'], $billName)); + Log::error( + sprintf( + 'RuleAction LinkToBill could not set the bill of journal #%d to bill "%s": no such bill found or not a withdrawal.', + $journal['transaction_journal_id'], $billName + ) + ); return false; diff --git a/app/TransactionRules/Actions/PrependDescription.php b/app/TransactionRules/Actions/PrependDescription.php index 04f7eb4625..4640d72031 100644 --- a/app/TransactionRules/Actions/PrependDescription.php +++ b/app/TransactionRules/Actions/PrependDescription.php @@ -22,8 +22,8 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; -use FireflyIII\Models\RuleAction; use DB; +use FireflyIII\Models\RuleAction; /** * Class PrependDescription. @@ -50,6 +50,7 @@ class PrependDescription implements ActionInterface { $description = sprintf('%s%s', $this->action->action_value, $journal['description']); DB::table('transaction_journals')->where('id', $journal['transaction_journal_id'])->limit(1)->update(['description' => $description]); + return true; } } diff --git a/app/TransactionRules/Actions/PrependNotes.php b/app/TransactionRules/Actions/PrependNotes.php index c7d817c84d..569b332dce 100644 --- a/app/TransactionRules/Actions/PrependNotes.php +++ b/app/TransactionRules/Actions/PrependNotes.php @@ -52,19 +52,20 @@ class PrependNotes implements ActionInterface { $dbNote = Note :: - where('noteable_id', (int) $journal['transaction_journal_id']) + where('noteable_id', (int)$journal['transaction_journal_id']) ->where('noteable_type', TransactionJournal::class) ->first(['notes.*']); if (null === $dbNote) { $dbNote = new Note; - $dbNote->noteable_id = (int) $journal['transaction_journal_id']; + $dbNote->noteable_id = (int)$journal['transaction_journal_id']; $dbNote->noteable_type = TransactionJournal::class; $dbNote->text = ''; } Log::debug(sprintf('RuleAction PrependNotes prepended "%s" to "%s".', $this->action->action_value, $dbNote->text)); - $text = sprintf('%s%s', $this->action->action_value, $dbNote->text); + $text = sprintf('%s%s', $this->action->action_value, $dbNote->text); $dbNote->text = $text; $dbNote->save(); + return true; } } diff --git a/app/TransactionRules/Actions/RemoveAllTags.php b/app/TransactionRules/Actions/RemoveAllTags.php index a9665d500c..9eaf04b776 100644 --- a/app/TransactionRules/Actions/RemoveAllTags.php +++ b/app/TransactionRules/Actions/RemoveAllTags.php @@ -22,9 +22,9 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; +use DB; use FireflyIII\Models\RuleAction; use Log; -use DB; /** diff --git a/app/TransactionRules/Actions/RemoveTag.php b/app/TransactionRules/Actions/RemoveTag.php index f67a785dd3..f8bd4671e8 100644 --- a/app/TransactionRules/Actions/RemoveTag.php +++ b/app/TransactionRules/Actions/RemoveTag.php @@ -22,10 +22,10 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; +use DB; use FireflyIII\Models\RuleAction; use FireflyIII\User; use Log; -use DB; /** * Class RemoveTag. diff --git a/app/TransactionRules/Actions/SetBudget.php b/app/TransactionRules/Actions/SetBudget.php index 7a74ced953..73d032004d 100644 --- a/app/TransactionRules/Actions/SetBudget.php +++ b/app/TransactionRules/Actions/SetBudget.php @@ -22,11 +22,11 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; +use DB; use FireflyIII\Models\RuleAction; use FireflyIII\Models\TransactionType; use FireflyIII\User; use Log; -use DB; /** * Class SetBudget. @@ -55,7 +55,12 @@ class SetBudget implements ActionInterface $budget = $user->budgets()->where('name', $search)->first(); if (null === $budget) { - Log::debug(sprintf('RuleAction SetBudget could not set budget of journal #%d to "%s" because no such budget exists.', $journal['transaction_journal_id'], $search)); + Log::debug( + sprintf( + 'RuleAction SetBudget could not set budget of journal #%d to "%s" because no such budget exists.', $journal['transaction_journal_id'], + $search + ) + ); return false; } @@ -73,7 +78,9 @@ class SetBudget implements ActionInterface return true; } - Log::debug(sprintf('RuleAction SetBudget set the budget of journal #%d to budget #%d ("%s").', $journal['transaction_journal_id'], $budget->id, $budget->name)); + Log::debug( + sprintf('RuleAction SetBudget set the budget of journal #%d to budget #%d ("%s").', $journal['transaction_journal_id'], $budget->id, $budget->name) + ); DB::table('budget_transaction_journal')->where('transaction_journal_id', '=', $journal['transaction_journal_id'])->delete(); DB::table('budget_transaction_journal')->insert(['transaction_journal_id' => $journal['transaction_journal_id'], 'budget_id' => $budget->id]); diff --git a/app/TransactionRules/Actions/SetCategory.php b/app/TransactionRules/Actions/SetCategory.php index ce32338321..4b2f5a6d1e 100644 --- a/app/TransactionRules/Actions/SetCategory.php +++ b/app/TransactionRules/Actions/SetCategory.php @@ -22,11 +22,11 @@ declare(strict_types=1); namespace FireflyIII\TransactionRules\Actions; +use DB; use FireflyIII\Factory\CategoryFactory; use FireflyIII\Models\RuleAction; use FireflyIII\User; use Log; -use DB; /** * Class SetCategory. @@ -52,8 +52,9 @@ class SetCategory implements ActionInterface { $user = User::find($journal['user_id']); $search = $this->action->action_value; - if(null === $user) { + if (null === $user) { Log::error(sprintf('Journal has no valid user ID so action SetCategory("%s") cannot be applied', $search), $journal); + return false; } @@ -62,12 +63,22 @@ class SetCategory implements ActionInterface $factory->setUser($user); $category = $factory->findOrCreate(null, $search); if (null === $category) { - Log::debug(sprintf('RuleAction SetCategory could not set category of journal #%d to "%s" because no such category exists.', $journal['transaction_journal_id'], $search)); + Log::debug( + sprintf( + 'RuleAction SetCategory could not set category of journal #%d to "%s" because no such category exists.', $journal['transaction_journal_id'], + $search + ) + ); return false; } - Log::debug(sprintf('RuleAction SetCategory set the category of journal #%d to category #%d ("%s").', $journal['transaction_journal_id'], $category->id, $category->name)); + Log::debug( + sprintf( + 'RuleAction SetCategory set the category of journal #%d to category #%d ("%s").', $journal['transaction_journal_id'], $category->id, + $category->name + ) + ); DB::table('category_transaction_journal')->where('transaction_journal_id', '=', $journal['transaction_journal_id'])->delete(); DB::table('category_transaction_journal')->insert(['transaction_journal_id' => $journal['transaction_journal_id'], 'category_id' => $category->id]); diff --git a/app/TransactionRules/Actions/SetDescription.php b/app/TransactionRules/Actions/SetDescription.php index b474b21d2d..fc28d0725b 100644 --- a/app/TransactionRules/Actions/SetDescription.php +++ b/app/TransactionRules/Actions/SetDescription.php @@ -60,6 +60,7 @@ class SetDescription implements ActionInterface $this->action->action_value ) ); + return true; } } diff --git a/app/TransactionRules/Actions/SetDestinationAccount.php b/app/TransactionRules/Actions/SetDestinationAccount.php index ed68324cdc..931362a0bb 100644 --- a/app/TransactionRules/Actions/SetDestinationAccount.php +++ b/app/TransactionRules/Actions/SetDestinationAccount.php @@ -48,27 +48,6 @@ class SetDestinationAccount implements ActionInterface $this->action = $action; } - /** - * @return Account|null - */ - private function findExpenseAccount(): ?Account - { - $account = $this->repository->findByName($this->action->action_value, [AccountType::EXPENSE]); - if (null === $account) { - $data = [ - 'name' => $this->action->action_value, - 'account_type' => 'expense', - 'account_type_id' => null, - 'virtual_balance' => 0, - 'active' => true, - 'iban' => null, - ]; - $account = $this->repository->store($data); - } - Log::debug(sprintf('Found or created expense account #%d ("%s")', $account->id, $account->name)); - return $account; - } - /** * @inheritDoc */ @@ -82,7 +61,11 @@ class SetDestinationAccount implements ActionInterface // it depends on the type what kind of destination account is expected. $expectedTypes = config(sprintf('firefly.source_dests.%s.%s', $type, $journal['source_account_type'])); if (null === $expectedTypes) { - Log::error(sprintf('Configuration line "%s" is unexpectedly empty. Stopped.', sprintf('firefly.source_dests.%s.%s', $type, $journal['source_account_type']))); + Log::error( + sprintf( + 'Configuration line "%s" is unexpectedly empty. Stopped.', sprintf('firefly.source_dests.%s.%s', $type, $journal['source_account_type']) + ) + ); return false; } @@ -106,6 +89,7 @@ class SetDestinationAccount implements ActionInterface $expense = $this->findExpenseAccount(); if (null === $expense) { Log::error('Could not create expense account.'); + return false; } DB::table('transactions') @@ -122,10 +106,33 @@ class SetDestinationAccount implements ActionInterface /** * @param array $types + * * @return Account|null */ private function findAccount(array $types): ?Account { return $this->repository->findByName($this->action->action_value, $types); } + + /** + * @return Account|null + */ + private function findExpenseAccount(): ?Account + { + $account = $this->repository->findByName($this->action->action_value, [AccountType::EXPENSE]); + if (null === $account) { + $data = [ + 'name' => $this->action->action_value, + 'account_type' => 'expense', + 'account_type_id' => null, + 'virtual_balance' => 0, + 'active' => true, + 'iban' => null, + ]; + $account = $this->repository->store($data); + } + Log::debug(sprintf('Found or created expense account #%d ("%s")', $account->id, $account->name)); + + return $account; + } } diff --git a/app/TransactionRules/Actions/SetSourceAccount.php b/app/TransactionRules/Actions/SetSourceAccount.php index 91579b1935..136b42400f 100644 --- a/app/TransactionRules/Actions/SetSourceAccount.php +++ b/app/TransactionRules/Actions/SetSourceAccount.php @@ -25,6 +25,8 @@ namespace FireflyIII\TransactionRules\Actions; use DB; use FireflyIII\Models\Account; use FireflyIII\Models\RuleAction; +use FireflyIII\Models\Transaction; +use FireflyIII\Models\TransactionJournal; use FireflyIII\Models\TransactionType; use FireflyIII\Repositories\Account\AccountRepositoryInterface; use FireflyIII\User; @@ -48,6 +50,74 @@ class SetSourceAccount implements ActionInterface $this->action = $action; } + /** + * @inheritDoc + */ + public function actOnArray(array $journal): bool + { + $user = User::find($journal['user_id']); + $type = $journal['transaction_type_type']; + /** @var TransactionJournal $journal */ + $journal = TransactionJournal::find((int)$journal['transaction_journal_id']); + /** @var AccountRepositoryInterface repository */ + $this->repository = app(AccountRepositoryInterface::class); + $this->repository->setUser($user); + + // if this is a transfer or a withdrawal, the new source account must be an asset account or a default account, and it MUST exist: + $newAccount = $this->findAssetAccount($type); + if ((TransactionType::WITHDRAWAL === $type || TransactionType::TRANSFER === $type) && null === $newAccount) { + Log::error( + sprintf( + 'Cannot change source account of journal #%d because no asset account with name "%s" exists.', $journal['transaction_journal_id'], + $this->action->action_value + ) + ); + + return false; + } + // new source account must be different from the current destination: + $destinationId = (int)$journal['destination_account_id']; + /** @var Transaction $source */ + $source = $journal->transactions()->where('amount', '>', 0)->first(); + if (null !== $source) { + $destinationId = $source->account ? (int)$source->account->id : $destinationId; + } + if (TransactionType::TRANSFER === $type && null !== $newAccount && (int)$newAccount->id === $destinationId) { + Log::error( + sprintf( + 'New source account ID #%d and current destination account ID #%d are the same. Do nothing.', $newAccount->id, + $destinationId + ) + ); + + return false; + } + + + // if this is a deposit, the new source account must be a revenue account and may be created: + if (TransactionType::DEPOSIT === $type) { + $newAccount = $this->findRevenueAccount(); + } + if (null === $newAccount) { + Log::error('New account is NULL'); + + return false; + } + + Log::debug(sprintf('New source account is #%d ("%s").', $newAccount->id, $newAccount->name)); + + // update source transaction with new source account: + // get source transaction: + DB::table('transactions') + ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) + ->where('amount', '<', 0) + ->update(['account_id' => $newAccount->id]); + + Log::debug(sprintf('Updated journal #%d and gave it new source account ID.', $journal['transaction_journal_id'])); + + return true; + } + /** * @param string $type * @@ -83,47 +153,7 @@ class SetSourceAccount implements ActionInterface $account = $this->repository->store($data); } Log::debug(sprintf('Found or created revenue account #%d ("%s")', $account->id, $account->name)); + return $account; } - - /** - * @inheritDoc - */ - public function actOnArray(array $journal): bool - { - $user = User::find($journal['user_id']); - $type = $journal['transaction_type_type']; - $this->repository = app(AccountRepositoryInterface::class); - $this->repository->setUser($user); - - // if this is a transfer or a withdrawal, the new source account must be an asset account or a default account, and it MUST exist: - $newAccount = $this->findAssetAccount($type); - if ((TransactionType::WITHDRAWAL === $type || TransactionType::TRANSFER === $type) && null === $newAccount) { - Log::error(sprintf('Cannot change source account of journal #%d because no asset account with name "%s" exists.', $journal['transaction_journal_id'], $this->action->action_value)); - - return false; - } - - // if this is a deposit, the new source account must be a revenue account and may be created: - if (TransactionType::DEPOSIT === $type) { - $newAccount = $this->findRevenueAccount(); - } - if (null === $newAccount) { - Log::error('New account is NULL'); - return false; - } - - Log::debug(sprintf('New source account is #%d ("%s").', $newAccount->id, $newAccount->name)); - - // update source transaction with new source account: - // get source transaction: - DB::table('transactions') - ->where('transaction_journal_id', '=', $journal['transaction_journal_id']) - ->where('amount', '<', 0) - ->update(['account_id' => $newAccount->id]); - - Log::debug(sprintf('Updated journal #%d and gave it new source account ID.', $journal['transaction_journal_id'])); - - return true; - } } diff --git a/app/TransactionRules/Actions/UpdatePiggybank.php b/app/TransactionRules/Actions/UpdatePiggybank.php index cf29087d13..3cda581782 100644 --- a/app/TransactionRules/Actions/UpdatePiggybank.php +++ b/app/TransactionRules/Actions/UpdatePiggybank.php @@ -54,42 +54,49 @@ class UpdatePiggybank implements ActionInterface } /** - * @param array $journalArray - * @param PiggyBank $piggyBank - * @param string $amount + * @inheritDoc */ - private function addAmount(array $journalArray, PiggyBank $piggyBank, string $amount): void + public function actOnArray(array $journal): bool { - $user = User::find($journalArray['user_id']); - $journal = $user->transactionJournals()->find($journalArray['transaction_journal_id']); - $repository = app(PiggyBankRepositoryInterface::class); - $repository->setUser($journal->user); + Log::debug(sprintf('Triggered rule action UpdatePiggybank on journal #%d', $journal['transaction_journal_id'])); + if (TransactionType::TRANSFER !== $journal['transaction_type_type']) { + Log::info(sprintf('Journal #%d is a "%s" so skip this action.', $journal['transaction_journal_id'], $journal['transaction_type_type'])); - // how much can we add to the piggy bank? - $toAdd = bcsub($piggyBank->targetamount, $repository->getCurrentAmount($piggyBank)); - Log::debug(sprintf('Max amount to add to piggy bank is %s, amount is %s', $toAdd, $amount)); + return false; + } + $user = User::find($journal['user_id']); - // update amount to fit: - $amount = -1 === bccomp($amount, $toAdd) ? $amount : $toAdd; - Log::debug(sprintf('Amount is now %s', $amount)); + $piggyBank = $this->findPiggybank($user); + if (null === $piggyBank) { + Log::info( + sprintf('No piggy bank names "%s", cant execute action #%d of rule #%d', $this->action->action_value, $this->action->id, $this->action->rule_id) + ); - // if amount is zero, stop. - if (0 === bccomp('0', $amount)) { - Log::warning('Amount left is zero, stop.'); - - return; + return false; } - // make sure we can add amount: - if (false === $repository->canAddAmount($piggyBank, $amount)) { - Log::warning(sprintf('Cannot add %s to piggy bank.', $amount)); + Log::debug(sprintf('Found piggy bank #%d ("%s")', $piggyBank->id, $piggyBank->name)); - return; + /** @var Transaction $source */ + $source = Transaction::where('transaction_journal_id', $journal['transaction_journal_id'])->where('amount', '<', 0)->first(); + /** @var Transaction $destination */ + $destination = Transaction::where('transaction_journal_id', $journal['transaction_journal_id'])->where('amount', '>', 0)->first(); + + if ((int)$source->account_id === (int)$piggyBank->account_id) { + Log::debug('Piggy bank account is linked to source, so remove amount.'); + $this->removeAmount($journal, $piggyBank, $destination->amount); + + return true; } - Log::debug(sprintf('Will now add %s to piggy bank.', $amount)); + if ((int)$destination->account_id === (int)$piggyBank->account_id) { + Log::debug('Piggy bank account is linked to source, so add amount.'); + $this->addAmount($journal, $piggyBank, $destination->amount); - $repository->addAmount($piggyBank, $amount); - $repository->createEventWithJournal($piggyBank, app('steam')->positive($amount), $journal); + return true; + } + Log::info('Piggy bank is not linked to source or destination, so no action will be taken.'); + + return true; } /** @@ -109,8 +116,8 @@ class UpdatePiggybank implements ActionInterface */ private function removeAmount(array $journalArray, PiggyBank $piggyBank, string $amount): void { - $user = User::find($journalArray['user_id']); - $journal = $user->transactionJournals()->find($journalArray['transaction_journal_id']); + $user = User::find($journalArray['user_id']); + $journal = $user->transactionJournals()->find($journalArray['transaction_journal_id']); $repository = app(PiggyBankRepositoryInterface::class); $repository->setUser($journal->user); @@ -141,46 +148,41 @@ class UpdatePiggybank implements ActionInterface } /** - * @inheritDoc + * @param array $journalArray + * @param PiggyBank $piggyBank + * @param string $amount */ - public function actOnArray(array $journal): bool + private function addAmount(array $journalArray, PiggyBank $piggyBank, string $amount): void { - Log::debug(sprintf('Triggered rule action UpdatePiggybank on journal #%d', $journal['transaction_journal_id'])); - if (TransactionType::TRANSFER !== $journal['transaction_type_type']) { - Log::info(sprintf('Journal #%d is a "%s" so skip this action.', $journal['transaction_journal_id'], $journal['transaction_type_type'])); + $user = User::find($journalArray['user_id']); + $journal = $user->transactionJournals()->find($journalArray['transaction_journal_id']); + $repository = app(PiggyBankRepositoryInterface::class); + $repository->setUser($journal->user); - return false; - } - $user = User::find($journal['user_id']); + // how much can we add to the piggy bank? + $toAdd = bcsub($piggyBank->targetamount, $repository->getCurrentAmount($piggyBank)); + Log::debug(sprintf('Max amount to add to piggy bank is %s, amount is %s', $toAdd, $amount)); - $piggyBank = $this->findPiggybank($user); - if (null === $piggyBank) { - Log::info(sprintf('No piggy bank names "%s", cant execute action #%d of rule #%d', $this->action->action_value, $this->action->id, $this->action->rule_id)); + // update amount to fit: + $amount = -1 === bccomp($amount, $toAdd) ? $amount : $toAdd; + Log::debug(sprintf('Amount is now %s', $amount)); - return false; + // if amount is zero, stop. + if (0 === bccomp('0', $amount)) { + Log::warning('Amount left is zero, stop.'); + + return; } - Log::debug(sprintf('Found piggy bank #%d ("%s")', $piggyBank->id, $piggyBank->name)); + // make sure we can add amount: + if (false === $repository->canAddAmount($piggyBank, $amount)) { + Log::warning(sprintf('Cannot add %s to piggy bank.', $amount)); - /** @var Transaction $source */ - $source = Transaction::where('transaction_journal_id', $journal['transaction_journal_id'])->where('amount', '<', 0)->first(); - /** @var Transaction $destination */ - $destination = Transaction::where('transaction_journal_id', $journal['transaction_journal_id'])->where('amount', '>', 0)->first(); - - if ((int) $source->account_id === (int) $piggyBank->account_id) { - Log::debug('Piggy bank account is linked to source, so remove amount.'); - $this->removeAmount($journal, $piggyBank, $destination->amount); - - return true; + return; } - if ((int) $destination->account_id === (int) $piggyBank->account_id) { - Log::debug('Piggy bank account is linked to source, so add amount.'); - $this->addAmount($journal, $piggyBank, $destination->amount); + Log::debug(sprintf('Will now add %s to piggy bank.', $amount)); - return true; - } - Log::info('Piggy bank is not linked to source or destination, so no action will be taken.'); - - return true; + $repository->addAmount($piggyBank, $amount); + $repository->createEventWithJournal($piggyBank, app('steam')->positive($amount), $journal); } } diff --git a/app/TransactionRules/Engine/SearchRuleEngine.php b/app/TransactionRules/Engine/SearchRuleEngine.php index 8474d58ea6..007094deb2 100644 --- a/app/TransactionRules/Engine/SearchRuleEngine.php +++ b/app/TransactionRules/Engine/SearchRuleEngine.php @@ -223,7 +223,7 @@ class SearchRuleEngine implements RuleEngineInterface Log::debug(sprintf('Now in findStrictRule(#%d)', $rule->id ?? 0)); $searchArray = []; /** @var RuleTrigger $ruleTrigger */ - foreach ($rule->ruleTriggers as $ruleTrigger) { + foreach ($rule->ruleTriggers()->where('active',1)->get() as $ruleTrigger) { // if needs no context, value is different: $needsContext = config(sprintf('firefly.search.operators.%s.needs_context', $ruleTrigger->trigger_type)) ?? true; if (false === $needsContext) { @@ -370,7 +370,7 @@ class SearchRuleEngine implements RuleEngineInterface { Log::debug(sprintf('SearchRuleEngine:: Will now execute actions on transaction journal #%d', $transaction['transaction_journal_id'])); /** @var RuleAction $ruleAction */ - foreach ($rule->ruleActions as $ruleAction) { + foreach ($rule->ruleActions()->where('active',1)->get() as $ruleAction) { $break = $this->processRuleAction($ruleAction, $transaction); if (true === $break) { break; @@ -448,7 +448,7 @@ class SearchRuleEngine implements RuleEngineInterface $total = new Collection; $count = 0; /** @var RuleTrigger $ruleTrigger */ - foreach ($rule->ruleTriggers as $ruleTrigger) { + foreach ($rule->ruleTriggers()->where('active',1)->get() as $ruleTrigger) { if ('user_action' === $ruleTrigger->trigger_type) { Log::debug('Skip trigger type.'); continue;