Fixes issue where rule actions would find the wrong account.

This commit is contained in:
James Cole 2023-03-25 13:42:26 +01:00
parent 976128a435
commit eebfb0cb40
No known key found for this signature in database
GPG Key ID: B49A324B7EAD6D80
6 changed files with 127 additions and 36 deletions

View File

@ -30,6 +30,9 @@ use FireflyIII\Models\RuleAction;
use FireflyIII\Models\TransactionJournal;
use Illuminate\Support\Facades\Log;
/**
* Class AppendDescriptionToNotes
*/
class AppendDescriptionToNotes implements ActionInterface
{
private RuleAction $action;

View File

@ -57,6 +57,7 @@ class ConvertToDeposit implements ActionInterface
/**
* @inheritDoc
* @throws FireflyException
* @throws JsonException
*/
public function actOnArray(array $journal): bool
{
@ -115,21 +116,21 @@ class ConvertToDeposit implements ActionInterface
// get the action value, or use the original destination name in case the action value is empty:
// this becomes a new or existing (revenue) account, which is the source of the new deposit.
$revenueName = '' === $this->action->action_value ? $journal['destination_account_name'] : $this->action->action_value;
$opposingName = '' === $this->action->action_value ? $journal['destination_account_name'] : $this->action->action_value;
// we check all possible source account types if one exists:
$validTypes = config('firefly.expected_source_types.source.Deposit');
$revenue = $repository->findByName($revenueName, $validTypes);
if (null === $revenue) {
$revenue = $factory->findOrCreate($revenueName, AccountType::REVENUE);
$opposingAccount = $repository->findByName($opposingName, $validTypes);
if (null === $opposingAccount) {
$opposingAccount = $factory->findOrCreate($opposingName, AccountType::REVENUE);
}
Log::debug(sprintf('ConvertToDeposit. Action value is "%s", revenue name is "%s"', $this->action->action_value, $journal['destination_account_name']));
Log::debug(sprintf('ConvertToDeposit. Action value is "%s", new opposing name is "%s"', $this->action->action_value, $journal['destination_account_name']));
// update the source transaction and put in the new revenue ID.
DB::table('transactions')
->where('transaction_journal_id', '=', $journal['transaction_journal_id'])
->where('amount', '<', 0)
->update(['account_id' => $revenue->id]);
->update(['account_id' => $opposingAccount->id]);
// update the destination transaction and put in the original source account ID.
DB::table('transactions')
@ -152,6 +153,7 @@ class ConvertToDeposit implements ActionInterface
/**
* Input is a transfer from A to B.
* Output is a deposit from C to B.
* The source account is replaced.
*
* @param array $journal
*
@ -167,10 +169,18 @@ class ConvertToDeposit implements ActionInterface
$factory = app(AccountFactory::class);
$factory->setUser($user);
$repository = app(AccountRepositoryInterface::class);
$repository->setUser($user);
// 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);
// this becomes a new or existing (revenue) account, which is the source of the new deposit.
$opposingName = '' === $this->action->action_value ? $journal['source_account_name'] : $this->action->action_value;
// we check all possible source account types if one exists:
$validTypes = config('firefly.expected_source_types.source.Deposit');
$opposingAccount = $repository->findByName($opposingName, $validTypes);
if (null === $opposingAccount) {
$opposingAccount = $factory->findOrCreate($opposingName, AccountType::REVENUE);
}
Log::debug(sprintf('ConvertToDeposit. Action value is "%s", revenue name is "%s"', $this->action->action_value, $journal['source_account_name']));
@ -178,7 +188,7 @@ class ConvertToDeposit implements ActionInterface
DB::table('transactions')
->where('transaction_journal_id', '=', $journal['transaction_journal_id'])
->where('amount', '<', 0)
->update(['account_id' => $revenue->id]);
->update(['account_id' => $opposingAccount->id]);
// change transaction type of journal:
$newType = TransactionType::whereType(TransactionType::DEPOSIT)->first();

View File

@ -26,7 +26,6 @@ namespace FireflyIII\TransactionRules\Actions;
use DB;
use FireflyIII\Events\TriggeredAuditLog;
use FireflyIII\Models\Account;
use FireflyIII\Models\AccountType;
use FireflyIII\Models\RuleAction;
use FireflyIII\Models\TransactionJournal;
use FireflyIII\Models\TransactionType;
@ -78,11 +77,23 @@ class ConvertToTransfer implements ActionInterface
/** @var AccountRepositoryInterface $repository */
$repository = app(AccountRepositoryInterface::class);
$repository->setUser($user);
$asset = $repository->findByName($this->action->action_value, [AccountType::ASSET, AccountType::LOAN, AccountType::DEBT, AccountType::MORTGAGE]);
if (null === $asset) {
$opposing = null;
$expectedType = null;
if (TransactionType::WITHDRAWAL === $type) {
$expectedType = $this->getSourceType($journal['transaction_journal_id']);
// Withdrawal? Replace destination with account with same type as source.
}
if (TransactionType::DEPOSIT === $type) {
$expectedType = $this->getDestinationType($journal['transaction_journal_id']);
// Deposit? Replace source with account with same type as destination.
}
$opposing = $repository->findByName($this->action->action_value, [$expectedType]);
if (null === $opposing) {
Log::error(
sprintf(
'Journal #%d cannot be converted because no asset with name "%s" exists (rule #%d).',
'Journal #%d cannot be converted because no valid %s account with name "%s" exists (rule #%d).',
$expectedType,
$journal['transaction_journal_id'],
$this->action->action_value,
$this->action->rule_id
@ -96,7 +107,7 @@ class ConvertToTransfer implements ActionInterface
$object = TransactionJournal::where('user_id', $journal['user_id'])->find($journal['transaction_journal_id']);
event(new TriggeredAuditLog($this->action->rule, $object, 'update_transaction_type', TransactionType::WITHDRAWAL, TransactionType::TRANSFER));
return $this->convertWithdrawalArray($journal, $asset);
return $this->convertWithdrawalArray($journal, $opposing);
}
if (TransactionType::DEPOSIT === $type) {
Log::debug('Going to transform a deposit to a transfer.');
@ -104,7 +115,7 @@ class ConvertToTransfer implements ActionInterface
$object = TransactionJournal::where('user_id', $journal['user_id'])->find($journal['transaction_journal_id']);
event(new TriggeredAuditLog($this->action->rule, $object, 'update_transaction_type', TransactionType::DEPOSIT, TransactionType::TRANSFER));
return $this->convertDepositArray($journal, $asset);
return $this->convertDepositArray($journal, $opposing);
}
return false;
@ -113,19 +124,20 @@ class ConvertToTransfer implements ActionInterface
/**
* A withdrawal is from Asset to Expense.
* We replace the Expense with another asset.
* So this replaces the destination
*
* @param array $journal
* @param Account $asset
* @param Account $opposing
*
* @return bool
*/
private function convertWithdrawalArray(array $journal, Account $asset): bool
private function convertWithdrawalArray(array $journal, Account $opposing): bool
{
if ($journal['source_account_id'] === $asset->id) {
if ($journal['source_account_id'] === $opposing->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]
[$journal['transaction_journal_id'], $opposing->name, $this->action->rule_id]
)
);
@ -136,7 +148,7 @@ class ConvertToTransfer implements ActionInterface
DB::table('transactions')
->where('transaction_journal_id', '=', $journal['transaction_journal_id'])
->where('amount', '>', 0)
->update(['account_id' => $asset->id]);
->update(['account_id' => $opposing->id]);
// change transaction type of journal:
$newType = TransactionType::whereType(TransactionType::TRANSFER)->first();
@ -155,17 +167,17 @@ class ConvertToTransfer implements ActionInterface
* We replace the Revenue with another asset.
*
* @param array $journal
* @param Account $asset
* @param Account $opposing
*
* @return bool
*/
private function convertDepositArray(array $journal, Account $asset): bool
private function convertDepositArray(array $journal, Account $opposing): bool
{
if ($journal['destination_account_id'] === $asset->id) {
if ($journal['destination_account_id'] === $opposing->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]
[$journal['transaction_journal_id'], $opposing->name, $this->action->rule_id]
)
);
@ -176,7 +188,7 @@ class ConvertToTransfer implements ActionInterface
DB::table('transactions')
->where('transaction_journal_id', '=', $journal['transaction_journal_id'])
->where('amount', '<', 0)
->update(['account_id' => $asset->id]);
->update(['account_id' => $opposing->id]);
// change transaction type of journal:
$newType = TransactionType::whereType(TransactionType::TRANSFER)->first();
@ -189,4 +201,34 @@ class ConvertToTransfer implements ActionInterface
return true;
}
/**
* @param int $journalId
* @return string
*/
private function getSourceType(int $journalId): string
{
/** @var TransactionJournal $journal */
$journal = TransactionJournal::find($journalId);
if (null === $journal) {
Log::error(sprintf('Journal #%d does not exist. Cannot convert to transfer.', $journalId));
return '';
}
return (string)$journal->transactions()->where('amount', '<', 0)->first()?->account?->accountType?->type;
}
/**
* @param int $journalId
* @return string
*/
private function getDestinationType(int $journalId): string
{
/** @var TransactionJournal $journal */
$journal = TransactionJournal::find($journalId);
if (null === $journal) {
Log::error(sprintf('Journal #%d does not exist. Cannot convert to transfer.', $journalId));
return '';
}
return (string)$journal->transactions()->where('amount', '>', 0)->first()?->account?->accountType?->type;
}
}

View File

@ -31,6 +31,7 @@ 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 JsonException;
use Log;
@ -89,6 +90,12 @@ class ConvertToWithdrawal implements ActionInterface
return false;
}
/**
* @param array $journal
* @return bool
* @throws FireflyException
* @throws JsonException
*/
private function convertDepositArray(array $journal): bool
{
$user = User::find($journal['user_id']);
@ -96,10 +103,21 @@ class ConvertToWithdrawal implements ActionInterface
$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);
$repository = app(AccountRepositoryInterface::class);
$repository->setUser($user);
// get the action value, or use the original source name in case the action value is empty:
// this becomes a new or existing (expense) account, which is the destination of the new withdrawal.
$opposingName = '' === $this->action->action_value ? $journal['source_account_name'] : $this->action->action_value;
// we check all possible source account types if one exists:
$validTypes = config('firefly.expected_source_types.destination.Withdrawal');
$opposingAccount = $repository->findByName($opposingName, $validTypes);
if (null === $opposingAccount) {
$opposingAccount = $factory->findOrCreate($opposingName, AccountType::EXPENSE);
}
$destinationId = $journal['destination_account_id'];
Log::debug(sprintf('ConvertToWithdrawal. Action value is "%s", expense name is "%s"', $this->action->action_value, $expenseName));
Log::debug(sprintf('ConvertToWithdrawal. Action value is "%s", expense name is "%s"', $this->action->action_value, $opposingName));
// update source transaction(s) to be the original destination account
DB::table('transactions')
@ -111,7 +129,7 @@ class ConvertToWithdrawal implements ActionInterface
DB::table('transactions')
->where('transaction_journal_id', '=', $journal['transaction_journal_id'])
->where('amount', '>', 0)
->update(['account_id' => $expense->id]);
->update(['account_id' => $opposingAccount->id]);
// change transaction type of journal:
$newType = TransactionType::whereType(TransactionType::WITHDRAWAL)->first();
@ -141,16 +159,27 @@ class ConvertToWithdrawal implements ActionInterface
/** @var AccountFactory $factory */
$factory = app(AccountFactory::class);
$factory->setUser($user);
$expenseName = '' === $this->action->action_value ? $journal['destination_account_name'] : $this->action->action_value;
$expense = $factory->findOrCreate($expenseName, AccountType::EXPENSE);
Log::debug(sprintf('ConvertToWithdrawal. Action value is "%s", expense name is "%s"', $this->action->action_value, $expenseName));
$repository = app(AccountRepositoryInterface::class);
$repository->setUser($user);
// get the action value, or use the original source name in case the action value is empty:
// this becomes a new or existing (expense) account, which is the destination of the new withdrawal.
$opposingName = '' === $this->action->action_value ? $journal['destination_account_name'] : $this->action->action_value;
// we check all possible source account types if one exists:
$validTypes = config('firefly.expected_source_types.destination.Withdrawal');
$opposingAccount = $repository->findByName($opposingName, $validTypes);
if (null === $opposingAccount) {
$opposingAccount = $factory->findOrCreate($opposingName, AccountType::EXPENSE);
}
Log::debug(sprintf('ConvertToWithdrawal. Action value is "%s", destination name is "%s"', $this->action->action_value, $opposingName));
// 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]);
->update(['account_id' => $opposingAccount->id]);
// change transaction type of journal:
$newType = TransactionType::whereType(TransactionType::WITHDRAWAL)->first();

View File

@ -30,6 +30,13 @@ use FireflyIII\Models\TransactionJournal;
use FireflyIII\Support\Request\ConvertsDataTypes;
use Illuminate\Support\Facades\Log;
/**
* Class MoveNotesToDescription
*/
/**
* Class MoveNotesToDescription
*/
class MoveNotesToDescription implements ActionInterface
{
use ConvertsDataTypes;

View File

@ -554,11 +554,11 @@ return [
],
'destination' => [
TransactionTypeModel::WITHDRAWAL => [
AccountType::EXPENSE,
AccountType::CASH,
AccountType::LOAN,
AccountType::DEBT,
AccountType::MORTGAGE,
AccountType::EXPENSE,
AccountType::CASH,
],
TransactionTypeEnum::DEPOSIT->value => [AccountType::ASSET, AccountType::LOAN, AccountType::DEBT, AccountType::MORTGAGE],
TransactionTypeModel::TRANSFER => [AccountType::ASSET, AccountType::LOAN, AccountType::DEBT, AccountType::MORTGAGE],