Improve recurrences

app/Factory/RecurrenceFactory.php

@@ -31,6 +31,7 @@ use FireflyIII\Models\Recurrence;
 use FireflyIII\Services\Internal\Support\RecurringTransactionTrait;
 use FireflyIII\Services\Internal\Support\TransactionTypeTrait;
 use FireflyIII\User;
+use Illuminate\Support\MessageBag;
 use Log;
 
 /**
@@ -41,6 +42,9 @@ class RecurrenceFactory
     /** @var User */
     private $user;
 
+    /** @var MessageBag */
+    private $errors;
+
     use TransactionTypeTrait, RecurringTransactionTrait;
 
     /**
@@ -52,22 +56,25 @@ class RecurrenceFactory
         if ('testing' === config('app.env')) {
             Log::warning(sprintf('%s should not be instantiated in the TEST environment!', get_class($this)));
         }
+        $this->errors = new MessageBag;
     }
 
     /**
      * @param array $data
      *
      * @return Recurrence
+     * @throws FireflyException
      */
-    public function create(array $data): ?Recurrence
+    public function create(array $data): Recurrence
     {
         try {
             $type = $this->findTransactionType(ucfirst($data['recurrence']['type']));
         } catch (FireflyException $e) {
-            Log::error(sprintf('Cannot make a recurring transaction of type "%s"', $data['recurrence']['type']));
+            $message = sprintf('Cannot make a recurring transaction of type "%s"', $data['recurrence']['type']);
-            Log::error($e->getMessage());
+            Log::error($message);
+            Log::error($e->getTraceAsString());
 
-            return null;
+            throw new FireflyException($message);
         }
         /** @var Carbon $firstDate */
         $firstDate = $data['recurrence']['first_date'];
@@ -95,9 +102,14 @@ class RecurrenceFactory
             $this->createTransactions($recurrence, $data['transactions'] ?? []);
             // @codeCoverageIgnoreStart
         } catch (FireflyException $e) {
-            // TODO make sure error props to the user.
             Log::error($e->getMessage());
+            $recurrence->forceDelete();
+            $message = sprintf('Could not create recurring transaction: %s', $e->getMessage());
+            $this->errors->add('store', $message);
+            throw new FireflyException($message);
+
         }
+
         // @codeCoverageIgnoreEnd
 
         return $recurrence;

app/Http/Controllers/Recurring/CreateController.php

@@ -25,6 +25,7 @@ namespace FireflyIII\Http\Controllers\Recurring;
 
 
 use Carbon\Carbon;
+use FireflyIII\Exceptions\FireflyException;
 use FireflyIII\Http\Controllers\Controller;
 use FireflyIII\Http\Requests\RecurrenceFormRequest;
 use FireflyIII\Models\RecurrenceRepetition;
@@ -71,7 +72,6 @@ class CreateController extends Controller
      * @param Request $request
      *
      * @return \Illuminate\Contracts\View\Factory|\Illuminate\View\View
-     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
      */
     public function create(Request $request)
     {
@@ -120,12 +120,16 @@ class CreateController extends Controller
      * @param RecurrenceFormRequest $request
      *
      * @return \Illuminate\Http\RedirectResponse|\Illuminate\Routing\Redirector
-     * @throws \FireflyIII\Exceptions\FireflyException
      */
     public function store(RecurrenceFormRequest $request)
     {
-        $data       = $request->getAll();
+        $data = $request->getAll();
-        $recurrence = $this->recurring->store($data);
+        try {
+            $recurrence = $this->recurring->store($data);
+        } catch (FireflyException $e) {
+            session()->flash('error', $e->getMessage());
+            return redirect(route('recurring.create'))->withInput();
+        }
 
         $request->session()->flash('success', (string)trans('firefly.stored_new_recurrence', ['title' => $recurrence->title]));
         app('preferences')->mark();

app/Http/Requests/RecurrenceFormRequest.php

@@ -120,11 +120,11 @@ class RecurrenceFormRequest extends Request
             default:
                 throw new FireflyException(sprintf('Cannot handle transaction type "%s"', $this->string('transaction_type'))); // @codeCoverageIgnore
             case 'withdrawal':
-                $return['transactions'][0]['source_id']        = $this->integer('source_id');
+                $return['transactions'][0]['source_id']      = $this->integer('source_id');
-                $return['transactions'][0]['destination_name'] = $this->string('destination_name');
+                $return['transactions'][0]['destination_id'] = $this->integer('withdrawal_destination_id');
                 break;
             case 'deposit':
-                $return['transactions'][0]['source_name']    = $this->string('source_name');
+                $return['transactions'][0]['source_id']      = $this->integer('deposit_source_id');
                 $return['transactions'][0]['destination_id'] = $this->integer('destination_id');
                 break;
             case 'transfer':

app/Repositories/Recurring/RecurringRepository.php

@@ -429,13 +429,19 @@ class RecurringRepository implements RecurringRepositoryInterface
      * @param array $data
      *
      * @return Recurrence
+     * @throws FireflyException
      */
     public function store(array $data): Recurrence
     {
-        $factory = new RecurrenceFactory;
+        /** @var RecurrenceFactory $factory */
+        $factory = app(RecurrenceFactory::class);
         $factory->setUser($this->user);
+        $result = $factory->create($data);
+        if (null === $result) {
+            throw new FireflyException($factory->getErrors()->first());
+        }
 
-        return $factory->create($data);
+        return $result;
     }
 
     /**

app/Repositories/Recurring/RecurringRepositoryInterface.php

@@ -176,10 +176,9 @@ interface RecurringRepositoryInterface
 
     /**
      * Store a new recurring transaction.
-     *\
      *
      * @param array $data
-     *
+     * @throws FireflyException
      * @return Recurrence
      */
     public function store(array $data): Recurrence;

app/Services/Internal/Support/RecurringTransactionTrait.php

@@ -154,7 +154,7 @@ trait RecurringTransactionTrait
             }
 
             if (!$validator->validateDestination($destination->id, null)) {
-                throw new FireflyException(sprintf('Destination invalid: %s', $validator->sourceError)); // @codeCoverageIgnore
+                throw new FireflyException(sprintf('Destination invalid: %s', $validator->destError)); // @codeCoverageIgnore
             }
 
             $transaction = new RecurrenceTransaction(

app/Support/ExpandedForm.php

@@ -118,7 +118,7 @@ class ExpandedForm
             $balance    = app('steam')->balance($account, new Carbon);
             $currencyId = (int)$repository->getMetaValue($account, 'currency_id');
             $currency   = $currencyRepos->findNull($currencyId);
-            $role       = $repository->getMetaValue($account, 'accountRole');
+            $role       = $repository->getMetaValue($account, 'account_role');
             if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) {
                 $role = 'no_account_type'; // @codeCoverageIgnore
             }
@@ -138,6 +138,136 @@ class ExpandedForm
         return $this->select($name, $grouped, $value, $options);
     }
 
+    /**
+     * Grouped dropdown list of all accounts that are valid as the destination of a withdrawal.
+     *
+     * @param string $name
+     * @param mixed $value
+     * @param array $options
+     *
+     * @return string
+     */
+    public function activeWithdrawalDestinations(string $name, $value = null, array $options = null): string
+    {
+        // make repositories
+        /** @var AccountRepositoryInterface $repository */
+        $repository = app(AccountRepositoryInterface::class);
+        /** @var CurrencyRepositoryInterface $currencyRepos */
+        $currencyRepos = app(CurrencyRepositoryInterface::class);
+
+        $accountList     = $repository->getActiveAccountsByType(
+            [
+                AccountType::MORTGAGE,
+                AccountType::DEBT,
+                AccountType::CREDITCARD,
+                AccountType::LOAN,
+                AccountType::EXPENSE,
+            ]
+        );
+        $liabilityTypes  = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN];
+        $defaultCurrency = app('amount')->getDefaultCurrency();
+        $grouped         = [];
+
+        // add cash account first:
+        $cash                     = $repository->getCashAccount();
+        $key                      = (string)trans('firefly.cash_account_type');
+        $grouped[$key][$cash->id] = sprintf('(%s)', (string)trans('firefly.cash'));
+
+        // group accounts:
+        /** @var Account $account */
+        foreach ($accountList as $account) {
+            $balance    = app('steam')->balance($account, new Carbon);
+            $currencyId = (int)$repository->getMetaValue($account, 'currency_id');
+            $currency   = $currencyRepos->findNull($currencyId);
+            $role       = (string)$repository->getMetaValue($account, 'account_role');
+            if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) {
+                $role = 'no_account_type'; // @codeCoverageIgnore
+            }
+            if ('no_account_type' === $role && AccountType::EXPENSE === $account->accountType->type) {
+                $role = 'expense_account'; // @codeCoverageIgnore
+
+            }
+
+            if (in_array($account->accountType->type, $liabilityTypes, true)) {
+                $role = 'l_' . $account->accountType->type; // @codeCoverageIgnore
+            }
+
+            if (null === $currency) {
+                $currency = $defaultCurrency;
+            }
+
+            $key                         = (string)trans('firefly.opt_group_' . $role);
+            $grouped[$key][$account->id] = $account->name . ' (' . app('amount')->formatAnything($currency, $balance, false) . ')';
+        }
+
+        return $this->select($name, $grouped, $value, $options);
+    }
+
+    /**
+     * Grouped dropdown list of all accounts that are valid as the destination of a withdrawal.
+     *
+     * @param string $name
+     * @param mixed $value
+     * @param array $options
+     *
+     * @return string
+     */
+    public function activeDepositDestinations(string $name, $value = null, array $options = null): string
+    {
+        // make repositories
+        /** @var AccountRepositoryInterface $repository */
+        $repository = app(AccountRepositoryInterface::class);
+        /** @var CurrencyRepositoryInterface $currencyRepos */
+        $currencyRepos = app(CurrencyRepositoryInterface::class);
+
+        $accountList     = $repository->getActiveAccountsByType(
+            [
+                AccountType::MORTGAGE,
+                AccountType::DEBT,
+                AccountType::CREDITCARD,
+                AccountType::LOAN,
+                AccountType::REVENUE,
+            ]
+        );
+        $liabilityTypes  = [AccountType::MORTGAGE, AccountType::DEBT, AccountType::CREDITCARD, AccountType::LOAN];
+        $defaultCurrency = app('amount')->getDefaultCurrency();
+        $grouped         = [];
+
+        // add cash account first:
+        $cash                     = $repository->getCashAccount();
+        $key                      = (string)trans('firefly.cash_account_type');
+        $grouped[$key][$cash->id] = sprintf('(%s)', (string)trans('firefly.cash'));
+
+        // group accounts:
+        /** @var Account $account */
+        foreach ($accountList as $account) {
+            $balance    = app('steam')->balance($account, new Carbon);
+            $currencyId = (int)$repository->getMetaValue($account, 'currency_id');
+            $currency   = $currencyRepos->findNull($currencyId);
+            $role       = (string)$repository->getMetaValue($account, 'account_role');
+            if ('' === $role && !in_array($account->accountType->type, $liabilityTypes, true)) {
+                $role = 'no_account_type'; // @codeCoverageIgnore
+            }
+            if ('no_account_type' === $role && AccountType::REVENUE === $account->accountType->type) {
+                $role = 'revenue_account'; // @codeCoverageIgnore
+
+            }
+
+            if (in_array($account->accountType->type, $liabilityTypes, true)) {
+                $role = 'l_' . $account->accountType->type; // @codeCoverageIgnore
+            }
+
+            if (null === $currency) {
+                $currency = $defaultCurrency;
+            }
+
+            $key                         = (string)trans('firefly.opt_group_' . $role);
+            $grouped[$key][$account->id] = $account->name . ' (' . app('amount')->formatAnything($currency, $balance, false) . ')';
+        }
+
+        return $this->select($name, $grouped, $value, $options);
+    }
+
     /**
      * @param string $name
      * @param mixed $value

config/twigbridge.php

@@ -197,7 +197,11 @@ return [
                 'is_safe' => ['date', 'text', 'select', 'balance', 'optionsList', 'checkbox', 'amount', 'tags', 'integer', 'textarea', 'location', 'file',
                               'staticText', 'password', 'nonSelectableAmount', 'number', 'assetAccountList', 'amountNoCurrency', 'currencyList',
                               'ruleGroupList', 'assetAccountCheckList', 'ruleGroupListWithEmpty', 'piggyBankList', 'currencyListEmpty',
-                              'activeAssetAccountList', 'percentage', 'activeLongAccountList', 'longAccountList','balanceAll'],],
+                              'activeAssetAccountList', 'percentage', 'activeLongAccountList', 'longAccountList','balanceAll',
+                              'activeWithdrawalDestinations','activeDepositDestinations'
+
+
+                ],],
             'Form'         => ['is_safe' => ['input', 'select', 'checkbox', 'model', 'open', 'radio', 'textarea', 'file',],
             ],
         ],

public/v1/js/ff/recurring/create.js

@@ -183,7 +183,8 @@ function updateFormFields() {
 
     if (transactionType === 'withdrawal') {
         // hide source account name:
-        $('#source_name_holder').hide();
+        // $('#source_name_holder').hide(); // source_name no longer exists
+        $('#deposit_source_id_holder').hide(); // new one!
 
         // show source account ID:
         $('#source_id_holder').show();
@@ -192,7 +193,8 @@ function updateFormFields() {
         $('#destination_id_holder').hide();
 
         // show destination name:
-        $('#destination_name_holder').show();
+        //$('#destination_name_holder').show(); // old one
+        $('#withdrawal_destination_id_holder').show();
 
         // show budget
         $('#budget_id_holder').show();
@@ -202,18 +204,29 @@ function updateFormFields() {
     }
 
     if (transactionType === 'deposit') {
-        $('#source_name_holder').show();
+        // $('#source_name_holder').show(); // source name no longer exists.
+        $('#deposit_source_id_holder').show(); // new one!
+
         $('#source_id_holder').hide();
-        $('#destination_name_holder').hide();
+
+        // $('#destination_name_holder').hide(); // old one
+        $('#withdrawal_destination_id_holder').hide();
+
+
         $('#destination_id_holder').show();
         $('#budget_id_holder').hide();
         $('#piggy_bank_id_holder').hide();
     }
 
     if (transactionType === 'transfer') {
-        $('#source_name_holder').hide();
+        // $('#source_name_holder').hide(); // source name no longer exists.
+        $('#deposit_source_id_holder').hide(); // new one!
+
         $('#source_id_holder').show();
-        $('#destination_name_holder').hide();
+
+        // $('#destination_name_holder').hide(); // old one
+        $('#withdrawal_destination_id_holder').hide();
+
         $('#destination_id_holder').show();
         $('#budget_id_holder').hide();
         $('#piggy_bank_id_holder').show();

resources/lang/en_US/firefly.php

@@ -758,6 +758,7 @@ return [
     'reconcile_options'                       => 'Reconciliation options',
     'reconcile_range'                         => 'Reconciliation range',
     'start_reconcile'                         => 'Start reconciling',
+    'cash_account_type'                       => 'Cash',
     'cash'                                    => 'cash',
     'account_type'                            => 'Account type',
     'save_transactions_by_moving'             => 'Save these transaction(s) by moving them to another account:',
@@ -857,6 +858,8 @@ return [
     'opt_group_sharedAsset'                   => 'Shared asset accounts',
     'opt_group_ccAsset'                       => 'Credit cards',
     'opt_group_cashWalletAsset'               => 'Cash wallets',
+    'opt_group_expense_account'               => 'Expense accounts',
+    'opt_group_revenue_account'               => 'Revenue accounts',
     'opt_group_l_Loan'                        => 'Liability: Loan',
     'opt_group_l_Debt'                        => 'Liability: Debt',
     'opt_group_l_Mortgage'                    => 'Liability: Mortgage',

resources/lang/en_US/form.php

@@ -66,7 +66,7 @@ return [
     'opening_balance'             => 'Opening balance',
     'tagMode'                     => 'Tag mode',
     'tag_position'                => 'Tag location',
-    'virtual_balance'              => 'Virtual balance',
+    'virtual_balance'             => 'Virtual balance',
     'targetamount'                => 'Target amount',
     'account_role'                => 'Account role',
     'opening_balance_date'        => 'Opening balance date',
@@ -253,4 +253,7 @@ return [
     'weekend'                 => 'Weekend',
     'client_secret'           => 'Client secret',
 
+    'withdrawal_destination_id' => 'Destination account',
+    'deposit_source_id'         => 'Source account',
+
 ];

resources/views/v1/recurring/create.twig

@@ -93,13 +93,19 @@
                         {{ ExpandedForm.activeLongAccountList('source_id', null, {label: trans('form.asset_source_account')}) }}
 
                         {# source account name for deposits: #}
-                        {{ ExpandedForm.text('source_name', null, {label: trans('form.revenue_account')}) }}
+                        {#{{ ExpandedForm.text('source_name', null, {label: trans('form.revenue_account')}) }}#}
+
+                        {# for deposits, a drop down with revenue accounts, loan debt cash and mortgage #}
+                        {{ ExpandedForm.activeDepositDestinations('deposit_source_id', null, {label: trans('form.deposit_source_id')}) }}
 
                         {# destination if deposit or transfer: #}
                         {{ ExpandedForm.activeLongAccountList('destination_id', null, {label: trans('form.asset_destination_account')} ) }}
 
                         {# destination account name for withdrawals #}
-                        {{ ExpandedForm.text('destination_name', null, {label: trans('form.expense_account')}) }}
+                        {#{{ ExpandedForm.text('destination_name', null, {label: trans('form.expense_account')}) }} #}
+
+                        {# for withdrawals, also a drop down with expense accounts, loans, debts, mortgages or (cash). #}
+                        {{ ExpandedForm.activeWithdrawalDestinations('withdrawal_destination_id', null, {label: trans('form.withdrawal_destination_id')}) }}
                     </div>
                 </div>
             </div>

routes/web.php

@@ -532,18 +532,14 @@ Route::group(
 
     // for auto complete
     Route::get('accounts', ['uses' => 'Json\AutoCompleteController@accounts', 'as' => 'autocomplete.accounts']);
-    Route::get('currencies', ['uses' => 'Json\AutoCompleteController@currencies', 'as' => 'autocomplete.currencies']);
     Route::get('budgets', ['uses' => 'Json\AutoCompleteController@budgets', 'as' => 'autocomplete.budgets']);
     Route::get('categories', ['uses' => 'Json\AutoCompleteController@categories', 'as' => 'autocomplete.categories']);
+    Route::get('currencies', ['uses' => 'Json\AutoCompleteController@currencies', 'as' => 'autocomplete.currencies']);
     Route::get('piggy-banks', ['uses' => 'Json\AutoCompleteController@piggyBanks', 'as' => 'autocomplete.piggy-banks']);
     Route::get('tags', ['uses' => 'Json\AutoCompleteController@tags', 'as' => 'autocomplete.tags']);
 
 
-    // TODO improve 3 routes:
+
-    //Route::get('transaction-journals/all', ['uses' => 'Json\AutoCompleteController@allTransactionJournals', 'as' => 'all-transaction-journals']);
-    //Route::get('transaction-journals/with-id/{tj}', ['uses' => 'Json\AutoCompleteController@journalsWithId', 'as' => 'journals-with-id']);
-    //Route::get('transaction-journals/{what}', ['uses' => 'Json\AutoCompleteController@transactionJournals', 'as' => 'transaction-journals']);
-    // TODO end of improvement
     Route::get('transaction-types', ['uses' => 'Json\AutoCompleteController@transactionTypes', 'as' => 'transaction-types']);
 
     // boxes
@@ -567,8 +563,6 @@ Route::group(
     Route::post('intro/enable/{route}/{specificPage?}', ['uses' => 'Json\IntroController@postEnable', 'as' => 'intro.enable']);
     Route::get('intro/{route}/{specificPage?}', ['uses' => 'Json\IntroController@getIntroSteps', 'as' => 'intro']);
 
-    //Route::get('/{subject}', ['uses' => 'Json\AutoCompleteController@autoComplete', 'as' => 'autocomplete']);
-
 }
 );