From 50ab1fa3f03482b6071c6749533a2c7661e54f45 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sun, 30 Sep 2018 20:14:17 +0200 Subject: [PATCH] Test improvement for import routine. --- app/Import/Routine/BunqRoutine.php | 6 +- app/Import/Storage/ImportArrayStorage.php | 101 ++++++++---------- .../Routine/Bunq/StageImportDataHandler.php | 8 ++ tests/Unit/Import/Routine/BunqRoutineTest.php | 51 ++++++++- 4 files changed, 104 insertions(+), 62 deletions(-) diff --git a/app/Import/Routine/BunqRoutine.php b/app/Import/Routine/BunqRoutine.php index c53c75a720..f5b6deb3b3 100644 --- a/app/Import/Routine/BunqRoutine.php +++ b/app/Import/Routine/BunqRoutine.php @@ -78,13 +78,13 @@ class BunqRoutine implements RoutineInterface $handler->run(); $transactions = $handler->getTransactions(); // could be that more transactions will arrive in a second run. - if (true === $handler->stillRunning) { + if (true === $handler->isStillRunning()) { Log::debug('Handler indicates that it is still working.'); $this->repository->setStatus($this->importJob, 'ready_to_run'); $this->repository->setStage($this->importJob, 'go-for-import'); } $this->repository->appendTransactions($this->importJob, $transactions); - if (false === $handler->stillRunning) { + if (false === $handler->isStillRunning()) { Log::info('Handler indicates that its done!'); $this->repository->setStatus($this->importJob, 'provider_finished'); $this->repository->setStage($this->importJob, 'final'); @@ -98,6 +98,8 @@ class BunqRoutine implements RoutineInterface } + + /** * Set the import job. * diff --git a/app/Import/Storage/ImportArrayStorage.php b/app/Import/Storage/ImportArrayStorage.php index 179340e50e..9568c31f32 100644 --- a/app/Import/Storage/ImportArrayStorage.php +++ b/app/Import/Storage/ImportArrayStorage.php @@ -46,7 +46,7 @@ use Illuminate\Support\Collection; use Log; /** - * Creates new transactions based upon arrays. Will first check the array for duplicates. + * Creates new transactions based on arrays. * * Class ImportArrayStorage * @@ -58,11 +58,11 @@ class ImportArrayStorage private $checkForTransfers = false; /** @var ImportJob The import job */ private $importJob; - /** @var JournalRepositoryInterface */ + /** @var JournalRepositoryInterface Journal repository for storage. */ private $journalRepos; /** @var ImportJobRepositoryInterface Import job repository */ private $repository; - /** @var Collection The transfers. */ + /** @var Collection The transfers the user already has. */ private $transfers; /** @@ -152,12 +152,10 @@ class ImportArrayStorage /** * Count the number of transfers in the array. If this is zero, don't bother checking for double transfers. - * - * @SuppressWarnings(PHPMD.CyclomaticComplexity) */ private function countTransfers(): void { - Log::debug('Now in count transfers.'); + Log::debug('Now in countTransfers()'); /** @var array $array */ $array = $this->importJob->transactions; $count = 0; @@ -167,17 +165,44 @@ class ImportArrayStorage Log::debug(sprintf('Row #%d is a transfer, increase count to %d', $index + 1, $count)); } } - if (0 === $count) { - Log::debug('Count is zero, will not check for duplicate transfers.'); - } + Log::debug(sprintf('Count of transfers in import array is %d.', $count)); if ($count > 0) { - Log::debug(sprintf('Count is %d, will check for duplicate transfers.', $count)); $this->checkForTransfers = true; - + Log::debug('Will check for duplicate transfers.'); // get users transfers. Needed for comparison. $this->getTransfers(); } + } + /** + * @param int $index + * @param array $transaction + * + * @return bool + * @throws FireflyException + */ + private function duplicateDetected(int $index, array $transaction): bool + { + $hash = $this->getHash($transaction); + $existingId = $this->hashExists($hash); + if (null !== $existingId) { + $message = sprintf('Row #%d ("%s") could not be imported. It already exists.', $index, $transaction['description']); + $this->logDuplicateObject($transaction, $existingId); + $this->repository->addErrorMessage($this->importJob, $message); + + return true; + } + + // do transfer detection: + if ($this->checkForTransfers && $this->transferExists($transaction)) { + $message = sprintf('Row #%d ("%s") could not be imported. Such a transfer already exists.', $index, $transaction['description']); + $this->logDuplicateTransfer($transaction); + $this->repository->addErrorMessage($this->importJob, $message); + + return true; + } + + return false; } /** @@ -397,34 +422,17 @@ class ImportArrayStorage $count = \count($array); $toStore = []; - Log::debug(sprintf('Now in store(). Count of items is %d', $count)); + Log::debug(sprintf('Now in store(). Count of items is %d.', $count)); + /* + * Detect duplicates in initial array: + */ foreach ($array as $index => $transaction) { Log::debug(sprintf('Now at item %d out of %d', $index + 1, $count)); - $hash = $this->getHash($transaction); - $existingId = $this->hashExists($hash); - if (null !== $existingId) { - $this->logDuplicateObject($transaction, $existingId); - $this->repository->addErrorMessage( - $this->importJob, sprintf( - 'Row #%d ("%s") could not be imported. It already exists.', - $index, $transaction['description'] - ) - ); + if ($this->duplicateDetected($index, $transaction)) { continue; } - if ($this->checkForTransfers && $this->transferExists($transaction)) { - $this->logDuplicateTransfer($transaction); - $this->repository->addErrorMessage( - $this->importJob, sprintf( - 'Row #%d ("%s") could not be imported. Such a transfer already exists.', - $index, - $transaction['description'] - ) - ); - continue; - } - $transaction['importHashV2'] = $hash; + $transaction['importHashV2'] = $this->getHash($transaction); $toStore[] = $transaction; } $count = \count($toStore); @@ -438,31 +446,8 @@ class ImportArrayStorage // now actually store them: $collection = new Collection; foreach ($toStore as $index => $store) { - // do duplicate detection again! - $hash = $this->getHash($store); - $existingId = $this->hashExists($hash); - if (null !== $existingId) { - $this->logDuplicateObject($store, $existingId); - $this->repository->addErrorMessage( - $this->importJob, sprintf( - 'Row #%d ("%s") could not be imported. It already exists.', - $index, $store['description'] - ) - ); - continue; - } - - // do transfer detection again! - if ($this->checkForTransfers && $this->transferExists($store)) { - $this->logDuplicateTransfer($store); - $this->repository->addErrorMessage( - $this->importJob, sprintf( - 'Row #%d ("%s") could not be imported. Such a transfer already exists.', - $index, - $store['description'] - ) - ); + if ($this->duplicateDetected($index, $store)) { continue; } diff --git a/app/Support/Import/Routine/Bunq/StageImportDataHandler.php b/app/Support/Import/Routine/Bunq/StageImportDataHandler.php index 2f9822b9fe..704cc0bb62 100644 --- a/app/Support/Import/Routine/Bunq/StageImportDataHandler.php +++ b/app/Support/Import/Routine/Bunq/StageImportDataHandler.php @@ -80,6 +80,14 @@ class StageImportDataHandler return $this->transactions; } + /** + * @return bool + */ + public function isStillRunning(): bool + { + return $this->stillRunning; + } + /** * * @throws FireflyException diff --git a/tests/Unit/Import/Routine/BunqRoutineTest.php b/tests/Unit/Import/Routine/BunqRoutineTest.php index defe5916c0..b8f179f244 100644 --- a/tests/Unit/Import/Routine/BunqRoutineTest.php +++ b/tests/Unit/Import/Routine/BunqRoutineTest.php @@ -71,13 +71,59 @@ class BunqRoutineTest extends TestCase $repository->shouldReceive('setUser')->once(); $repository->shouldReceive('setStatus')->withArgs([Mockery::any(), 'running']); + $repository->shouldReceive('setStatus')->withArgs([Mockery::any(), 'provider_finished']); + $repository->shouldReceive('setStage')->withArgs([Mockery::any(), 'final']); + $repository->shouldReceive('appendTransactions')->withArgs([Mockery::any(), ['a' => 'c']])->once(); + $handler->shouldReceive('setImportJob')->once(); $handler->shouldReceive('run')->once(); $handler->shouldReceive('getTransactions')->once()->andReturn(['a' => 'c']); - //$repository->shouldReceive('setStatus')->withArgs([Mockery::any(), 'provider_finished'])->once(); - //$repository->shouldReceive('setStage')->withArgs([Mockery::any(), 'final'])->once(); + $handler->shouldReceive('isStillRunning')->andReturn(false); + $routine = new BunqRoutine; + $routine->setImportJob($job); + try { + $routine->run(); + } catch (FireflyException $e) { + $this->assertFalse(true, $e->getMessage()); + } + + } + + /** + * @covers \FireflyIII\Import\Routine\BunqRoutine + */ + public function testRunImportStillRunning(): void + { + $job = new ImportJob; + $job->user_id = $this->user()->id; + $job->key = 'brY_' . random_int(1, 10000); + $job->status = 'ready_to_run'; + $job->stage = 'go-for-import'; + $job->provider = 'bunq'; + $job->file_type = ''; + $job->configuration = []; + $job->save(); + + // mock stuff: + $repository = $this->mock(ImportJobRepositoryInterface::class); + $handler = $this->mock(StageImportDataHandler::class); + + + + $handler->shouldReceive('setImportJob')->once(); + $handler->shouldReceive('run')->once(); + $handler->shouldReceive('getTransactions')->once()->andReturn(['a' => 'c']); + $handler->shouldReceive('isStillRunning')->andReturn(true); + + $repository->shouldReceive('setStatus')->withArgs([Mockery::any(), 'ready_to_run']); + $repository->shouldReceive('setStage')->withArgs([Mockery::any(), 'go-for-import']); + + $repository->shouldReceive('setUser')->once(); + $repository->shouldReceive('setStatus')->withArgs([Mockery::any(), 'running']); $repository->shouldReceive('appendTransactions')->withArgs([Mockery::any(), ['a' => 'c']])->once(); + + $routine = new BunqRoutine; $routine->setImportJob($job); try { @@ -88,6 +134,7 @@ class BunqRoutineTest extends TestCase } + /** * @covers \FireflyIII\Import\Routine\BunqRoutine */