Merge pull request #6857 from firefly-iii/fix-liabilities

Fix liabilities

app/Console/Commands/Upgrade/UpgradeLiabilitiesEight.php

@@ -26,6 +26,7 @@ namespace FireflyIII\Console\Commands\Upgrade;
 
 use FireflyIII\Exceptions\FireflyException;
 use FireflyIII\Models\Account;
+use FireflyIII\Models\AccountType;
 use FireflyIII\Models\Transaction;
 use FireflyIII\Models\TransactionJournal;
 use FireflyIII\Models\TransactionType;
@@ -72,8 +73,6 @@ class UpgradeLiabilitiesEight extends Command
             return 0;
         }
         $this->upgradeLiabilities();
-
-        // TODO uncomment me
         $this->markAsExecuted();
 
         $end = round(microtime(true) - $start, 2);
@@ -135,10 +134,11 @@ class UpgradeLiabilitiesEight extends Command
      */
     private function upgradeLiability(Account $account): void
     {
+        Log::debug(sprintf('Upgrade liability #%d ("%s")', $account->id, $account->name));
+
         /** @var AccountRepositoryInterface $repository */
         $repository = app(AccountRepositoryInterface::class);
         $repository->setUser($account->user);
-        Log::debug(sprintf('Upgrade liability #%d ("%s")', $account->id, $account->name));
 
         $direction = $repository->getMetaValue($account, 'liability_direction');
         if ('debit' === $direction) {
@@ -147,17 +147,69 @@ class UpgradeLiabilitiesEight extends Command
         if ('credit' === $direction && $this->hasBadOpening($account)) {
             $this->deleteCreditTransaction($account);
             $this->reverseOpeningBalance($account);
-            $this->line(sprintf('Fixed correct bad opening for liability #%d ("%s")', $account->id, $account->name));
+            $this->line(sprintf('Corrected opening balance for liability #%d ("%s")', $account->id, $account->name));
+        }
+        if ('credit' === $direction) {
+            $count = $this->deleteTransactions($account);
+            if ($count > 0) {
+                $this->line(sprintf('Removed %d old format transaction(s) for liability #%d ("%s")', $count, $account->id, $account->name));
+            }
         }
         Log::debug(sprintf('Done upgrading liability #%d ("%s")', $account->id, $account->name));
     }
 
+    /**
+     * @param $account
+     * @return void
+     */
+    private function deleteTransactions($account): int
+    {
+        $count    = 0;
+        $journals = TransactionJournal::leftJoin('transactions', 'transaction_journals.id', '=', 'transactions.transaction_journal_id')
+                                      ->where('transactions.account_id', $account->id)->get(['transaction_journals.*']);
+        Log::debug(sprintf('Found %d journals to analyse.', $journals->count()));
+        /** @var TransactionJournal $journal */
+        foreach ($journals as $journal) {
+            $delete = false;
+            /** @var Transaction $source */
+            $source = $journal->transactions()->where('amount', '<', 0)->first();
+            /** @var Transaction $dest */
+            $dest = $journal->transactions()->where('amount', '>', 0)->first();
+
+            // if source is this liability and destination is expense, remove transaction.
+            // if source is revenue and destination is liability, remove transaction.
+            if ((int)$source->account_id === (int)$account->id && $dest->account->accountType->type === AccountType::EXPENSE) {
+                $delete = true;
+            }
+            if ((int)$dest->account_id === (int)$account->id && $source->account->accountType->type === AccountType::REVENUE) {
+                $delete = true;
+            }
+            if ($delete) {
+                Log::debug(
+                    sprintf(
+                        'Deleted %s journal #%d ("%s") (%s %s).',
+                        $journal->transactionType->type,
+                        $journal->id,
+                        $journal->description,
+                        $journal->transactionCurrency->code,
+                        $dest->amount
+                    )
+                );
+                $service = app(TransactionGroupDestroyService::class);
+                $service->destroy($journal->transactionGroup);
+                $count++;
+            }
+        }
+        return $count;
+    }
+
     /**
      * @param  Account  $account
      * @return void
      */
     private function deleteCreditTransaction(Account $account): void
     {
+        Log::debug('Will delete credit transaction.');
         $liabilityType    = TransactionType::whereType(TransactionType::LIABILITY_CREDIT)->first();
         $liabilityJournal = TransactionJournal::leftJoin('transactions', 'transactions.transaction_journal_id', '=', 'transaction_journals.id')
                                               ->where('transactions.account_id', $account->id)
@@ -168,7 +220,9 @@ class UpgradeLiabilitiesEight extends Command
             $service = new TransactionGroupDestroyService();
             $service->destroy($group);
             Log::debug(sprintf('Deleted liability credit group #%d', $group->id));
+            return;
         }
+        Log::debug('No liability credit journal found.');
     }
 
 
@@ -226,13 +280,13 @@ class UpgradeLiabilitiesEight extends Command
                                             ->where('transaction_journals.transaction_type_id', $openingBalanceType->id)
                                             ->first(['transaction_journals.*']);
         /** @var Transaction $source */
-        $source         = $openingJournal->transactions()->where('amount', '<', 0)->first();
+        $source = $openingJournal->transactions()->where('amount', '<', 0)->first();
         /** @var Transaction $dest */
-        $dest           = $openingJournal->transactions()->where('amount', '>', 0)->first();
-        if($source && $dest) {
-            $sourceId = $source->account_id;
-            $destId   = $dest->account_id;
-            $dest->account_id = $sourceId;
+        $dest = $openingJournal->transactions()->where('amount', '>', 0)->first();
+        if ($source && $dest) {
+            $sourceId           = $source->account_id;
+            $destId             = $dest->account_id;
+            $dest->account_id   = $sourceId;
             $source->account_id = $destId;
             $source->save();
             $dest->save();

app/Http/Requests/AccountFormRequest.php

@@ -82,7 +82,8 @@ class AccountFormRequest extends FormRequest
             $data['account_type_name'] = null;
             $data['account_type_id']   = $this->convertInteger('liability_type_id');
             if ('' !== $data['opening_balance']) {
-                $data['opening_balance'] = app('steam')->negative($data['opening_balance']);
+                // opening balance is always positive for liabilities
+                $data['opening_balance'] = app('steam')->positive($data['opening_balance']);
             }
         }
 

app/Services/Internal/Support/CreditRecalculateService.php

@@ -202,6 +202,8 @@ class CreditRecalculateService
 
         /** @var AccountMetaFactory $factory */
         $factory = app(AccountMetaFactory::class);
+
+        // amount is positive or negative, doesn't matter.
         $factory->crud($account, 'start_of_debt', $startOfDebt);
 
         // get direction of liability:
@@ -209,6 +211,8 @@ class CreditRecalculateService
 
         // now loop all transactions (except opening balance and credit thing)
         $transactions = $account->transactions()->get();
+        Log::debug(sprintf('Going to process %d transaction(s)', $transactions->count()));
+        Log::debug(sprintf('Account currency is #%d (%s)', $account->id, $this->repository->getAccountCurrency($account)?->code));
         /** @var Transaction $transaction */
         foreach ($transactions as $transaction) {
             $leftOfDebt = $this->processTransaction($account, $direction, $transaction, $leftOfDebt);
@@ -226,32 +230,39 @@ class CreditRecalculateService
      *
      * @return string
      */
-    private function processTransaction(Account $account, string $direction, Transaction $transaction, string $amount): string
+    private function processTransaction(Account $account, string $direction, Transaction $transaction, string $leftOfDebt): string
     {
-        Log::debug(sprintf('Now in %s(#%d, %s)', __METHOD__, $transaction->id, $amount));
+        Log::debug(sprintf('Now in processTransaction(#%d, %s)', $transaction->id, $leftOfDebt));
         $journal         = $transaction->transactionJournal;
         $foreignCurrency = $transaction->foreignCurrency;
         $accountCurrency = $this->repository->getAccountCurrency($account);
         $groupId         = $journal->transaction_group_id;
         $type            = $journal->transactionType->type;
-
-        Log::debug(sprintf('Account currency is #%d (%s)', $accountCurrency->id, $accountCurrency->code));
+        /** @var Transaction $destTransaction */
+        $destTransaction = $journal->transactions()->where('amount', '>', '0')->first();
+        /** @var Transaction $sourceTransaction */
+        $sourceTransaction = $journal->transactions()->where('amount', '<', '0')->first();
 
         if ('' === $direction) {
             Log::debug('Since direction is "", do nothing.');
 
-            return $amount;
+            return $leftOfDebt;
         }
+        if (TransactionType::LIABILITY_CREDIT === $type || TransactionType::OPENING_BALANCE === $type) {
+            Log::debug(sprintf('Skip group #%d, journal #%d of type "%s"', $journal->id, $groupId, $type));
+            return $leftOfDebt;
+        }
+
         // amount to use depends on the currency:
         $usedAmount = $transaction->amount;
         if (null !== $foreignCurrency && $foreignCurrency->id === $accountCurrency->id) {
             $usedAmount = $transaction->foreign_amount;
-            Log::debug('Will now use foreign amount!');
+            Log::debug('Will use foreign amount to match account currency.');
         }
 
-
         Log::debug(sprintf('Processing group #%d, journal #%d of type "%s"', $journal->id, $groupId, $type));
 
+        // Case 1
         // it's a withdrawal into this liability (from asset).
         // if it's a credit ("I am owed"), this increases the amount due,
         // because we're lending person X more money
@@ -261,13 +272,33 @@ class CreditRecalculateService
             && 1 === bccomp($usedAmount, '0')
             && 'credit' === $direction
         ) {
-            $amount = bcadd($amount, app('steam')->positive($usedAmount));
-            Log::debug(
-                sprintf('Is withdrawal (%s) into credit liability #%d, will increase amount due to %s.', $transaction->account_id, $usedAmount, $amount)
-            );
-            return $amount;
+            $newLeftOfDebt = bcadd($leftOfDebt, app('steam')->positive($usedAmount));
+            Log::debug(sprintf('[1] Is withdrawal (%s) into liability, left of debt = %s.', $usedAmount, $newLeftOfDebt));
+            return $newLeftOfDebt;
         }
 
+        // Case 2
+        // it's a withdrawal away from this liability (into expense account).
+        // if it's a credit ("I am owed"), this decreases the amount due,
+        // because we're sending money away from the loan (like loan forgiveness)
+        if (
+            $type === TransactionType::WITHDRAWAL
+            && (int)$account->id === (int)$sourceTransaction->account_id
+            && -1 === bccomp($usedAmount, '0')
+            && 'credit' === $direction
+        ) {
+            $newLeftOfDebt = bcsub($leftOfDebt, app('steam')->positive($usedAmount));
+            Log::debug(
+                sprintf(
+                    '[2] Is withdrawal (%s) away from liability, left of debt = %s.',
+                    $usedAmount,
+                    $newLeftOfDebt
+                )
+            );
+            return $newLeftOfDebt;
+        }
+
+        // case 3
         // it's a deposit out of this liability (to asset).
         // if it's a credit ("I am owed") this decreases the amount due.
         // because the person is paying us back.
@@ -277,17 +308,36 @@ class CreditRecalculateService
             && -1 === bccomp($usedAmount, '0')
             && 'credit' === $direction
         ) {
-            $amount = bcsub($amount, app('steam')->positive($usedAmount));
-            Log::debug(sprintf('Is deposit (%s) from credit liability #%d, will decrease amount due to %s.', $transaction->account_id, $usedAmount, $amount));
-            return $amount;
+            $newLeftOfDebt = bcsub($leftOfDebt, app('steam')->positive($usedAmount));
+            Log::debug(sprintf('[3] Is deposit (%s) away from liability, left of debt = %s.', $usedAmount, $newLeftOfDebt));
+            return $newLeftOfDebt;
         }
 
+        // case 4
+        // it's a deposit into this liability (from revenue account).
+        // if it's a credit ("I am owed") this increases the amount due.
+        // because the person is having to pay more money.
+        if (
+            $type === TransactionType::DEPOSIT
+            && (int)$account->id === (int)$destTransaction->account_id
+            && 1 === bccomp($usedAmount, '0')
+            && 'credit' === $direction
+        ) {
+            $newLeftOfDebt = bcadd($leftOfDebt, app('steam')->positive($usedAmount));
+            Log::debug(sprintf('[4] Is deposit (%s) into liability, left of debt = %s.', $usedAmount, $newLeftOfDebt));
+            return $newLeftOfDebt;
+        }
+
+        // in any other case, remove amount from left of debt.
         if (in_array($type, [TransactionType::WITHDRAWAL, TransactionType::DEPOSIT, TransactionType::TRANSFER], true)) {
-            $amount = bcadd($amount, bcmul($usedAmount, '-1'));
+            $newLeftOfDebt = bcadd($leftOfDebt, bcmul($usedAmount, '-1'));
+            Log::debug(sprintf('[5] Fallback: transaction is withdrawal/deposit/transfer, remove amount %s from left of debt, = %s.', $usedAmount, $newLeftOfDebt));
+            return $newLeftOfDebt;
         }
-        Log::debug(sprintf('Amount is now %s', $amount));
 
-        return $amount;
+        Log::warning(sprintf('[6] Catch-all, should not happen. Left of debt = %s', $leftOfDebt));
+
+        return $leftOfDebt;
     }
 
     /**

app/Services/Internal/Update/AccountUpdateService.php

@@ -298,6 +298,11 @@ class AccountUpdateService
                 $openingBalance     = $data['opening_balance'];
                 $openingBalanceDate = $data['opening_balance_date'];
 
+                // if liability, make sure the amount is positive for a credit, and negative for a debit.
+                if($this->isLiability($account)) {
+                    $openingBalance = 'credit' === $data['liability_direction'] ? app('steam')->positive($openingBalance) : app('steam')->negative($openingBalance);
+                }
+
                 $this->updateOBGroupV2($account, $openingBalance, $openingBalanceDate);
             }
 

app/Validation/Api/Data/Bulk/ValidatesBulkTransactionQuery.php

@@ -71,7 +71,7 @@ trait ValidatesBulkTransactionQuery
             // must have same currency:
             // some account types (like expenses) do not have currency, so they have to be omitted
             $sourceCurrency = $repository->getAccountCurrency($source);
-            $destCurrency =  $repository->getAccountCurrency($dest);
+            $destCurrency   = $repository->getAccountCurrency($dest);
             if (
                 $sourceCurrency !== null
                 && $destCurrency !== null

changelog.md

@@ -6,7 +6,7 @@ Alpha 2
 
 
 - Data import: when you submit a transaction but give the ID of an account of the wrong type, Firefly III will try to create an account of the right type. For example: you submit a deposit but the source account is an expense account: Firefly III will try to create a revenue account instead. 
-- Security: blocked users can access API, users can unblock themselves using the API.
+- Security: blocked users can access API, users can unblock themselves using the API. Recurrent Nymph CVE-2023-0298
 - New language: catalan
 
 ## 5.8.0-alpha.1 - 2023-01-08

public/v1/js/ff/accounts/create.js

@@ -30,4 +30,21 @@ $(document).ready(function () {
             }
         );
     }
+    // change the 'ffInput_opening_balance' text based on the
+    // selection of the direction.
+    $("#ffInput_liability_direction").change(triggerDirection);
+    triggerDirection();
 });
+
+
+function triggerDirection() {
+    let obj = $("#ffInput_liability_direction");
+    let direction = obj.val();
+    console.log('Direction is now ' + direction);
+    if('credit' === direction) {
+        $('label[for="ffInput_opening_balance"]').text(iAmOwed);
+    }
+    if('debit' === direction) {
+        $('label[for="ffInput_opening_balance"]').text(iOwe);
+    }
+}

public/v1/js/ff/accounts/edit.js

@@ -30,5 +30,21 @@ $(document).ready(function () {
             }
         );
     }
+    // change the 'ffInput_opening_balance' text based on the
+    // selection of the direction.
+    $("#ffInput_liability_direction").change(triggerDirection);
+    triggerDirection();
 
 });
+
+function triggerDirection() {
+    let obj = $("#ffInput_liability_direction");
+    let direction = obj.val();
+    console.log('Direction is now ' + direction);
+    if('credit' === direction) {
+        $('label[for="ffInput_opening_balance"]').text(iAmOwed);
+    }
+    if('debit' === direction) {
+        $('label[for="ffInput_opening_balance"]').text(iOwe);
+    }
+}

resources/lang/en_US/firefly.php

@@ -1715,6 +1715,8 @@ return [
     'extension_date_is'                         => 'Extension date is {date}',
 
     // accounts:
+    'i_am_owed_amount'                          => 'I am owed amount',
+    'i_owe_amount'                              => 'I owe amount',
     'inactive_account_link'                     => 'You have :count inactive (archived) account, which you can view on this separate page.|You have :count inactive (archived) accounts, which you can view on this separate page.',
     'all_accounts_inactive'                     => 'These are your inactive accounts.',
     'active_account_link'                       => 'This link goes back to your active accounts.',
@@ -2468,7 +2470,7 @@ return [
 
     // recurring transactions
     'create_right_now'                      => 'Create right now',
-    'no_new_transaction_in_recurrence' => 'No new transaction was created. Perhaps it was already fired for this date?',
+    'no_new_transaction_in_recurrence'      => 'No new transaction was created. Perhaps it was already fired for this date?',
     'recurrences'                           => 'Recurring transactions',
     'repeat_until_in_past'                  => 'This recurring transaction stopped repeating on :date.',
     'recurring_calendar_view'               => 'Calendar',

resources/views/accounts/create.twig

@@ -84,6 +84,10 @@
     </form>
 {% endblock %}
 {% block scripts %}
+    <script type="text/javascript" nonce="{{ JS_NONCE }}">
+        var iAmOwed = '{{ 'i_am_owed_amount'|_|escape('js') }}';
+        var iOwe = '{{ 'i_owe_amount'|_|escape('js') }}';
+    </script>
     <script src="v1/lib/leaflet/leaflet.js?v={{ FF_VERSION }}" nonce="{{ JS_NONCE }}"></script>
     <script type="text/javascript" src="v1/js/lib/modernizr-custom.js?v={{ FF_VERSION }}" nonce="{{ JS_NONCE }}"></script>
     <script type="text/javascript" src="v1/js/lib/jquery-ui.min.js?v={{ FF_VERSION }}" nonce="{{ JS_NONCE }}"></script>

resources/views/accounts/edit.twig

@@ -35,7 +35,7 @@
 
                     {% if objectType == 'liabilities' %}
                         {{ ExpandedForm.select('liability_type_id', liabilityTypes) }}
-                        {{ ExpandedForm.amountNoCurrency('opening_balance', null, {label:'debt_start_amount'|_, helpText: 'debt_start_amount_help'|_}) }}
+                        {{ ExpandedForm.amountNoCurrency('opening_balance', null, {label:'debt_start_amount'|_}) }}
                         {{ ExpandedForm.select('liability_direction', liabilityDirections) }}
                         {{ ExpandedForm.date('opening_balance_date', null, {label:'debt_start_date'|_}) }}
                         {{ ExpandedForm.percentage('interest') }}
@@ -115,6 +115,10 @@
     </form>
 {% endblock %}
 {% block scripts %}
+    <script type="text/javascript" nonce="{{ JS_NONCE }}">
+        var iAmOwed = '{{ 'i_am_owed_amount'|_|escape('js') }}';
+        var iOwe = '{{ 'i_owe_amount'|_|escape('js') }}';
+    </script>
     <script src="v1/lib/leaflet/leaflet.js?v={{ FF_VERSION }}" nonce="{{ JS_NONCE }}"></script>
     <script type="text/javascript" src="v1/js/lib/modernizr-custom.js?v={{ FF_VERSION }}" nonce="{{ JS_NONCE }}"></script>
     <script type="text/javascript" src="v1/js/lib/jquery-ui.min.js?v={{ FF_VERSION }}" nonce="{{ JS_NONCE }}"></script>