From 477a3c7eb2e105a1b93c511dd76d18f0a24e79bc Mon Sep 17 00:00:00 2001 From: James Cole Date: Wed, 13 Jun 2018 19:03:18 +0200 Subject: [PATCH] Fix various bugs in the import routine, discovered by Doug. --- app/Factory/TransactionFactory.php | 11 +++++++++++ app/Helpers/Collector/JournalCollector.php | 8 ++++++++ app/Helpers/Collector/JournalCollectorInterface.php | 7 +++++++ app/Import/Storage/ImportArrayStorage.php | 11 +++++++++-- app/Repositories/Journal/JournalRepository.php | 4 ++-- .../Journal/JournalRepositoryInterface.php | 3 ++- .../Internal/Support/TransactionServiceTrait.php | 1 + app/Support/Import/Placeholder/ImportTransaction.php | 4 +++- .../Import/Routine/File/ImportableConverter.php | 5 ++++- .../Import/Routine/File/MappedValuesValidator.php | 3 +++ 10 files changed, 50 insertions(+), 7 deletions(-) diff --git a/app/Factory/TransactionFactory.php b/app/Factory/TransactionFactory.php index d9199b0ab1..bb5baf3cbe 100644 --- a/app/Factory/TransactionFactory.php +++ b/app/Factory/TransactionFactory.php @@ -26,6 +26,7 @@ namespace FireflyIII\Factory; use FireflyIII\Exceptions\FireflyException; +use FireflyIII\Models\AccountType; use FireflyIII\Models\Transaction; use FireflyIII\Models\TransactionJournal; use FireflyIII\Models\TransactionType; @@ -105,6 +106,16 @@ class TransactionFactory $destinationType = $this->accountType($journal, 'destination'); Log::debug(sprintf('Expect source destination to be of type %s', $destinationType)); $destinationAccount = $this->findAccount($destinationType, $data['destination_id'], $data['destination_name']); + + Log::debug(sprintf('Source type is "%s", destination type is "%s"', $sourceType, $destinationType)); + // throw big fat error when source type === dest type + if ($sourceAccount->accountType->type === $destinationAccount->accountType->type) { + throw new FireflyException(sprintf('Source and destination account cannot be both of the type "%s"', $destinationAccount->accountType->type)); + } + if ($sourceAccount->accountType->type !== AccountType::ASSET && $destinationAccount->accountType->type !== AccountType::ASSET) { + throw new FireflyException('At least one of the accounts must be an asset account.'); + } + // first make a "negative" (source) transaction based on the data in the array. $source = $this->create( [ diff --git a/app/Helpers/Collector/JournalCollector.php b/app/Helpers/Collector/JournalCollector.php index b2eb90dc1a..a68b4cbec1 100644 --- a/app/Helpers/Collector/JournalCollector.php +++ b/app/Helpers/Collector/JournalCollector.php @@ -768,6 +768,14 @@ class JournalCollector implements JournalCollectorInterface return $this; } + /** + * @return EloquentBuilder + */ + public function getQuery(): EloquentBuilder + { + return $this->query; + } + /** * @param Collection $set * diff --git a/app/Helpers/Collector/JournalCollectorInterface.php b/app/Helpers/Collector/JournalCollectorInterface.php index b710326175..b9c29ff4bf 100644 --- a/app/Helpers/Collector/JournalCollectorInterface.php +++ b/app/Helpers/Collector/JournalCollectorInterface.php @@ -27,6 +27,7 @@ use FireflyIII\Models\Budget; use FireflyIII\Models\Category; use FireflyIII\Models\Tag; use FireflyIII\User; +use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Pagination\LengthAwarePaginator; use Illuminate\Support\Collection; @@ -35,6 +36,7 @@ use Illuminate\Support\Collection; */ interface JournalCollectorInterface { + /** * @param string $filter * @@ -78,6 +80,11 @@ interface JournalCollectorInterface */ public function getPaginatedJournals(): LengthAwarePaginator; + /** + * @return EloquentBuilder + */ + public function getQuery(): EloquentBuilder; + /** * @return JournalCollectorInterface */ diff --git a/app/Import/Storage/ImportArrayStorage.php b/app/Import/Storage/ImportArrayStorage.php index dde6971a8e..c7edae2e6f 100644 --- a/app/Import/Storage/ImportArrayStorage.php +++ b/app/Import/Storage/ImportArrayStorage.php @@ -303,7 +303,7 @@ class ImportArrayStorage 'existing' => $existingId, 'description' => $transaction['description'] ?? '', 'amount' => $transaction['transactions'][0]['amount'] ?? 0, - 'date' => isset($transaction['date']) ? $transaction['date'] : '', + 'date' => $transaction['date'] ?? '', ] ); @@ -413,7 +413,14 @@ class ImportArrayStorage $store['date'] = Carbon::createFromFormat('Y-m-d', $store['date']); $store['description'] = $store['description'] === '' ? '(empty description)' : $store['description']; // store the journal. - $journal = $this->journalRepos->store($store); + try { + $journal = $this->journalRepos->store($store); + } catch(FireflyException $e) { + Log::error($e->getMessage()); + Log::error($e->getTraceAsString()); + $this->repository->addErrorMessage($this->importJob, sprintf('Row #%d could not be imported. %s', $index, $e->getMessage())); + continue; + } Log::debug(sprintf('Stored as journal #%d', $journal->id)); $collection->push($journal); } diff --git a/app/Repositories/Journal/JournalRepository.php b/app/Repositories/Journal/JournalRepository.php index 6f71187a31..4dc900bb95 100644 --- a/app/Repositories/Journal/JournalRepository.php +++ b/app/Repositories/Journal/JournalRepository.php @@ -24,6 +24,7 @@ namespace FireflyIII\Repositories\Journal; use Carbon\Carbon; use Exception; +use FireflyIII\Exceptions\FireflyException; use FireflyIII\Factory\TransactionJournalFactory; use FireflyIII\Factory\TransactionJournalMetaFactory; use FireflyIII\Models\Account; @@ -738,8 +739,7 @@ class JournalRepository implements JournalRepositoryInterface * * @return TransactionJournal * - * @throws \FireflyIII\Exceptions\FireflyException - * @throws \FireflyIII\Exceptions\FireflyException + * @throws FireflyException */ public function store(array $data): TransactionJournal { diff --git a/app/Repositories/Journal/JournalRepositoryInterface.php b/app/Repositories/Journal/JournalRepositoryInterface.php index 1c55997a76..dc6bfe9b26 100644 --- a/app/Repositories/Journal/JournalRepositoryInterface.php +++ b/app/Repositories/Journal/JournalRepositoryInterface.php @@ -23,6 +23,7 @@ declare(strict_types=1); namespace FireflyIII\Repositories\Journal; use Carbon\Carbon; +use FireflyIII\Exceptions\FireflyException; use FireflyIII\Models\Account; use FireflyIII\Models\Note; use FireflyIII\Models\Transaction; @@ -325,7 +326,7 @@ interface JournalRepositoryInterface /** * @param array $data - * + * @throws FireflyException * @return TransactionJournal */ public function store(array $data): TransactionJournal; diff --git a/app/Services/Internal/Support/TransactionServiceTrait.php b/app/Services/Internal/Support/TransactionServiceTrait.php index 06fe1fe99c..d0cb0a3f8f 100644 --- a/app/Services/Internal/Support/TransactionServiceTrait.php +++ b/app/Services/Internal/Support/TransactionServiceTrait.php @@ -190,6 +190,7 @@ trait TransactionServiceTrait */ protected function findCategory(?int $categoryId, ?string $categoryName): ?Category { + Log::debug(sprintf('Going to find or create category #%d, with name "%s"', $categoryId, $categoryName)); /** @var CategoryFactory $factory */ $factory = app(CategoryFactory::class); $factory->setUser($this->user); diff --git a/app/Support/Import/Placeholder/ImportTransaction.php b/app/Support/Import/Placeholder/ImportTransaction.php index a9ce98af63..bdda187930 100644 --- a/app/Support/Import/Placeholder/ImportTransaction.php +++ b/app/Support/Import/Placeholder/ImportTransaction.php @@ -179,7 +179,9 @@ class ImportTransaction $this->budgetName = $columnValue->getValue(); break; case 'category-id': - $this->categoryId = $this->getMappedValue($columnValue); + $value = $this->getMappedValue($columnValue); + Log::debug(sprintf('Set category ID to %d in ImportTransaction object', $value)); + $this->categoryId = $value; break; case 'category-name': $this->categoryName = $columnValue->getValue(); diff --git a/app/Support/Import/Routine/File/ImportableConverter.php b/app/Support/Import/Routine/File/ImportableConverter.php index 5084f24743..73c3963354 100644 --- a/app/Support/Import/Routine/File/ImportableConverter.php +++ b/app/Support/Import/Routine/File/ImportableConverter.php @@ -279,11 +279,14 @@ class ImportableConverter */ private function verifyObjectId(string $key, int $objectId): ?int { + if (isset($this->mappedValues[$key]) && \in_array($objectId, $this->mappedValues[$key], true)) { + Log::debug(sprintf('verifyObjectId(%s, %d) is valid!',$key, $objectId)); return $objectId; } - return null; + Log::debug(sprintf('verifyObjectId(%s, %d) is NOT in the list, but it could still be valid.',$key, $objectId)); + return $objectId; } diff --git a/app/Support/Import/Routine/File/MappedValuesValidator.php b/app/Support/Import/Routine/File/MappedValuesValidator.php index 8526b2b1f7..e9229a275a 100644 --- a/app/Support/Import/Routine/File/MappedValuesValidator.php +++ b/app/Support/Import/Routine/File/MappedValuesValidator.php @@ -87,6 +87,7 @@ class MappedValuesValidator $return = []; Log::debug('Now in validateMappedValues()'); foreach ($mappings as $role => $values) { + Log::debug(sprintf('Now at role "%s"', $role)); $values = array_unique($values); if (\count($values) > 0) { switch ($role) { @@ -115,9 +116,11 @@ class MappedValuesValidator $return[$role] = $valid; break; case 'category-id': + Log::debug('Going to validate these category ids: ', $values); $set = $this->catRepos->getByIds($values); $valid = $set->pluck('id')->toArray(); $return[$role] = $valid; + Log::debug('Valid category IDs are: ', $valid); break; } }