From 75d52b7024f073437a582cb34e6e0460624e1d20 Mon Sep 17 00:00:00 2001 From: James Cole Date: Mon, 24 Jan 2022 07:38:05 +0100 Subject: [PATCH] Catch https://github.com/firefly-iii/firefly-iii/issues/5610 --- .../Models/Transaction/UpdateRequest.php | 90 +++++++++---------- .../Internal/Update/GroupUpdateService.php | 31 ++++++- app/Validation/GroupValidation.php | 5 +- 3 files changed, 76 insertions(+), 50 deletions(-) diff --git a/app/Api/V1/Requests/Models/Transaction/UpdateRequest.php b/app/Api/V1/Requests/Models/Transaction/UpdateRequest.php index c8c47953c1..2a7d8d0f4d 100644 --- a/app/Api/V1/Requests/Models/Transaction/UpdateRequest.php +++ b/app/Api/V1/Requests/Models/Transaction/UpdateRequest.php @@ -284,76 +284,76 @@ class UpdateRequest extends FormRequest { return [ // basic fields for group: - 'group_title' => 'between:1,1000', - 'apply_rules' => [new IsBoolean], + 'group_title' => 'between:1,1000', + 'apply_rules' => [new IsBoolean], // transaction rules (in array for splits): - 'transactions.*.type' => 'in:withdrawal,deposit,transfer,opening-balance,reconciliation', - 'transactions.*.date' => [new IsDateOrTime], - 'transactions.*.order' => 'numeric|min:0', + 'transactions.*.type' => 'in:withdrawal,deposit,transfer,opening-balance,reconciliation', + 'transactions.*.date' => [new IsDateOrTime], + 'transactions.*.order' => 'numeric|min:0', // group id: - 'transactions.*.transaction_journal_id' => ['numeric','exists:transaction_journals,id', new BelongsUser], + 'transactions.*.transaction_journal_id' => ['numeric', 'exists:transaction_journals,id', new BelongsUser], // currency info - 'transactions.*.currency_id' => 'numeric|exists:transaction_currencies,id', - 'transactions.*.currency_code' => 'min:3|max:3|exists:transaction_currencies,code', - 'transactions.*.foreign_currency_id' => 'nullable|numeric|exists:transaction_currencies,id', - 'transactions.*.foreign_currency_code' => 'nullable|min:3|max:3|exists:transaction_currencies,code', + 'transactions.*.currency_id' => 'numeric|exists:transaction_currencies,id', + 'transactions.*.currency_code' => 'min:3|max:3|exists:transaction_currencies,code', + 'transactions.*.foreign_currency_id' => 'nullable|numeric|exists:transaction_currencies,id', + 'transactions.*.foreign_currency_code' => 'nullable|min:3|max:3|exists:transaction_currencies,code', // amount - 'transactions.*.amount' => 'numeric|gt:0|max:100000000000', - 'transactions.*.foreign_amount' => 'nullable|numeric|gte:0', + 'transactions.*.amount' => 'numeric|gt:0|max:100000000000', + 'transactions.*.foreign_amount' => 'nullable|numeric|gte:0', // description - 'transactions.*.description' => 'nullable|between:1,1000', + 'transactions.*.description' => 'nullable|between:1,1000', // source of transaction - 'transactions.*.source_id' => ['numeric', 'nullable', new BelongsUser], - 'transactions.*.source_name' => 'between:1,255|nullable', + 'transactions.*.source_id' => ['numeric', 'nullable', new BelongsUser], + 'transactions.*.source_name' => 'between:1,255|nullable', // destination of transaction - 'transactions.*.destination_id' => ['numeric', 'nullable', new BelongsUser], - 'transactions.*.destination_name' => 'between:1,255|nullable', + 'transactions.*.destination_id' => ['numeric', 'nullable', new BelongsUser], + 'transactions.*.destination_name' => 'between:1,255|nullable', // budget, category, bill and piggy - 'transactions.*.budget_id' => ['mustExist:budgets,id', new BelongsUser], - 'transactions.*.budget_name' => ['between:1,255', 'nullable', new BelongsUser], - 'transactions.*.category_id' => ['mustExist:categories,id', new BelongsUser], - 'transactions.*.category_name' => 'between:1,255|nullable', - 'transactions.*.bill_id' => ['numeric', 'nullable', 'mustExist:bills,id', new BelongsUser], - 'transactions.*.bill_name' => ['between:1,255', 'nullable', new BelongsUser], + 'transactions.*.budget_id' => ['mustExist:budgets,id', new BelongsUser], + 'transactions.*.budget_name' => ['between:1,255', 'nullable', new BelongsUser], + 'transactions.*.category_id' => ['mustExist:categories,id', new BelongsUser], + 'transactions.*.category_name' => 'between:1,255|nullable', + 'transactions.*.bill_id' => ['numeric', 'nullable', 'mustExist:bills,id', new BelongsUser], + 'transactions.*.bill_name' => ['between:1,255', 'nullable', new BelongsUser], // other interesting fields - 'transactions.*.reconciled' => [new IsBoolean], - 'transactions.*.notes' => 'min:1,max:50000|nullable', - 'transactions.*.tags' => 'between:0,255', + 'transactions.*.reconciled' => [new IsBoolean], + 'transactions.*.notes' => 'min:1,max:50000|nullable', + 'transactions.*.tags' => 'between:0,255', // meta info fields - 'transactions.*.internal_reference' => 'min:1,max:255|nullable', - 'transactions.*.external_id' => 'min:1,max:255|nullable', - 'transactions.*.recurrence_id' => 'min:1,max:255|nullable', - 'transactions.*.bunq_payment_id' => 'min:1,max:255|nullable', - 'transactions.*.external_uri' => 'min:1,max:255|nullable|url', + 'transactions.*.internal_reference' => 'min:1,max:255|nullable', + 'transactions.*.external_id' => 'min:1,max:255|nullable', + 'transactions.*.recurrence_id' => 'min:1,max:255|nullable', + 'transactions.*.bunq_payment_id' => 'min:1,max:255|nullable', + 'transactions.*.external_uri' => 'min:1,max:255|nullable|url', // SEPA fields: - 'transactions.*.sepa_cc' => 'min:1,max:255|nullable', - 'transactions.*.sepa_ct_op' => 'min:1,max:255|nullable', - 'transactions.*.sepa_ct_id' => 'min:1,max:255|nullable', - 'transactions.*.sepa_db' => 'min:1,max:255|nullable', - 'transactions.*.sepa_country' => 'min:1,max:255|nullable', - 'transactions.*.sepa_ep' => 'min:1,max:255|nullable', - 'transactions.*.sepa_ci' => 'min:1,max:255|nullable', - 'transactions.*.sepa_batch_id' => 'min:1,max:255|nullable', + 'transactions.*.sepa_cc' => 'min:1,max:255|nullable', + 'transactions.*.sepa_ct_op' => 'min:1,max:255|nullable', + 'transactions.*.sepa_ct_id' => 'min:1,max:255|nullable', + 'transactions.*.sepa_db' => 'min:1,max:255|nullable', + 'transactions.*.sepa_country' => 'min:1,max:255|nullable', + 'transactions.*.sepa_ep' => 'min:1,max:255|nullable', + 'transactions.*.sepa_ci' => 'min:1,max:255|nullable', + 'transactions.*.sepa_batch_id' => 'min:1,max:255|nullable', // dates - 'transactions.*.interest_date' => 'date|nullable', - 'transactions.*.book_date' => 'date|nullable', - 'transactions.*.process_date' => 'date|nullable', - 'transactions.*.due_date' => 'date|nullable', - 'transactions.*.payment_date' => 'date|nullable', - 'transactions.*.invoice_date' => 'date|nullable', + 'transactions.*.interest_date' => 'date|nullable', + 'transactions.*.book_date' => 'date|nullable', + 'transactions.*.process_date' => 'date|nullable', + 'transactions.*.due_date' => 'date|nullable', + 'transactions.*.payment_date' => 'date|nullable', + 'transactions.*.invoice_date' => 'date|nullable', ]; } diff --git a/app/Services/Internal/Update/GroupUpdateService.php b/app/Services/Internal/Update/GroupUpdateService.php index 7b0a196827..3fae6e4bc8 100644 --- a/app/Services/Internal/Update/GroupUpdateService.php +++ b/app/Services/Internal/Update/GroupUpdateService.php @@ -79,7 +79,17 @@ class GroupUpdateService $existing = $transactionGroup->transactionJournals->pluck('id')->toArray(); $updated = $this->updateTransactions($transactionGroup, $transactions); - $result = array_diff($existing, $updated); + Log::debug('Array of updated IDs: ', $updated); + + if(0 === count($updated)) { + Log::error('There were no transactions updated or created. Will not delete anything.'); + $transactionGroup->refresh(); + app('preferences')->mark(); + return $transactionGroup; + } + + $result = array_diff($existing, $updated); + Log::debug('Result of DIFF: ', $result); if (count($result) > 0) { /** @var string $deletedId */ foreach ($result as $deletedId) { @@ -127,6 +137,7 @@ class GroupUpdateService */ private function updateTransactions(TransactionGroup $transactionGroup, array $transactions): array { + // updated or created transaction journals: $updated = []; /** * @var int $index @@ -151,8 +162,14 @@ class GroupUpdateService } } Log::debug('Call createTransactionJournal'); - $this->createTransactionJournal($transactionGroup, $transaction); + $newJournal = $this->createTransactionJournal($transactionGroup, $transaction); Log::debug('Done calling createTransactionJournal'); + if (null !== $newJournal) { + $updated[] = $newJournal->id; + } + if (null === $newJournal) { + Log::error('createTransactionJournal returned NULL, indicating something went wrong.'); + } } if (null !== $journal) { Log::debug('Call updateTransactionJournal'); @@ -169,12 +186,13 @@ class GroupUpdateService * @param TransactionGroup $transactionGroup * @param array $data * + * @return TransactionJournal|null + * * @throws FireflyException * @throws DuplicateTransactionException */ - private function createTransactionJournal(TransactionGroup $transactionGroup, array $data): void + private function createTransactionJournal(TransactionGroup $transactionGroup, array $data): ?TransactionJournal { - $submission = [ 'transactions' => [ $data, @@ -195,6 +213,11 @@ class GroupUpdateService $transactionGroup->transactionJournals()->save($journal); } ); + if (0 === $collection->count()) { + return null; + } + + return $collection->first(); } } diff --git a/app/Validation/GroupValidation.php b/app/Validation/GroupValidation.php index ffe6067c65..08d90e4a3e 100644 --- a/app/Validation/GroupValidation.php +++ b/app/Validation/GroupValidation.php @@ -83,6 +83,9 @@ trait GroupValidation } } + /** + * @param Validator $validator + */ protected function preventNoAccountInfo(Validator $validator): void { $transactions = $this->getTransactionsArray($validator); @@ -92,7 +95,7 @@ trait GroupValidation /** @var array $transaction */ foreach ($transactions as $transaction) { foreach($keys as $key) { - if(array_key_exists($key, $transaction) && '' !== $transaction[$key]) { + if(array_key_exists($key, $transaction) && '' !== (string) $transaction[$key]) { $hasAccountInfo = true; } }