From 50882f309b7a105c533a20051f3ab1a1b75c1b7d Mon Sep 17 00:00:00 2001 From: James Cole Date: Sat, 13 Jan 2018 07:36:44 +0100 Subject: [PATCH] Make sure number of steps is always correct. --- .../Controllers/Import/StatusController.php | 20 +++++--- app/Import/Configuration/FileConfigurator.php | 26 ++++++++++ app/Import/FileProcessor/CsvProcessor.php | 21 ++++---- app/Import/Routine/FileRoutine.php | 48 +++++++++++++++-- app/Import/Storage/ImportStorage.php | 51 ++++++++++++++----- app/Import/Storage/ImportSupport.php | 10 ++-- app/Models/Account.php | 2 +- app/Models/ImportJob.php | 24 --------- .../Account/AccountRepository.php | 1 + .../ImportJob/ImportJobRepository.php | 31 +++++++++-- .../ImportJobRepositoryInterface.php | 8 +++ public/js/ff/import/status.js | 21 +++++++- 12 files changed, 194 insertions(+), 69 deletions(-) diff --git a/app/Http/Controllers/Import/StatusController.php b/app/Http/Controllers/Import/StatusController.php index fd54229a95..18782dafbb 100644 --- a/app/Http/Controllers/Import/StatusController.php +++ b/app/Http/Controllers/Import/StatusController.php @@ -93,14 +93,20 @@ class StatusController extends Controller $result['percentage'] = round(($job->extended_status['done'] / $job->extended_status['steps']) * 100, 0); $result['show_percentage'] = true; } - if ('finished' === $job->status) { - $tagId = $job->extended_status['tag']; - /** @var TagRepositoryInterface $repository */ - $repository = app(TagRepositoryInterface::class); - $tag = $repository->find($tagId); - $result['finished'] = true; - $result['finishedText'] = trans('import.status_finished_job', ['link' => route('tags.show', [$tag->id, 'all']), 'tag' => $tag->tag]); + $result['finished'] = true; + $tagId = intval($job->extended_status['tag']); + if ($tagId !== 0) { + /** @var TagRepositoryInterface $repository */ + $repository = app(TagRepositoryInterface::class); + $tag = $repository->find($tagId); + $count = $tag->transactionJournals()->count(); + $result['finishedText'] = trans('import.status_finished_job', ['count' => $count,'link' => route('tags.show', [$tag->id, 'all']), 'tag' => $tag->tag]); + } + + if ($tagId === 0) { + $result['finishedText'] = trans('import.status_finished_no_tag'); + } } if ('running' === $job->status) { diff --git a/app/Import/Configuration/FileConfigurator.php b/app/Import/Configuration/FileConfigurator.php index eb70bebee3..5806f3c143 100644 --- a/app/Import/Configuration/FileConfigurator.php +++ b/app/Import/Configuration/FileConfigurator.php @@ -187,6 +187,12 @@ class FileConfigurator implements ConfiguratorInterface $this->repository = app(ImportJobRepositoryInterface::class); $this->repository->setUser($job->user); + // set number of steps to 100: + $extendedStatus = $this->getExtendedStatus(); + $extendedStatus['steps'] = 6; + $extendedStatus['done'] = 0; + $this->setExtendedStatus($extendedStatus); + $config = $this->getConfig(); $newConfig = array_merge($this->defaultConfig, $config); $this->repository->setConfiguration($job, $newConfig); @@ -241,4 +247,24 @@ class FileConfigurator implements ConfiguratorInterface return $class; } + + /** + * @return array + */ + private function getExtendedStatus(): array + { + return $this->repository->getExtendedStatus($this->job); + } + + /** + * Shorthand method. + * + * @param array $extended + */ + private function setExtendedStatus(array $extended): void + { + $this->repository->setExtendedStatus($this->job, $extended); + + return; + } } diff --git a/app/Import/FileProcessor/CsvProcessor.php b/app/Import/FileProcessor/CsvProcessor.php index 7f335c4f15..af298e6837 100644 --- a/app/Import/FileProcessor/CsvProcessor.php +++ b/app/Import/FileProcessor/CsvProcessor.php @@ -90,6 +90,7 @@ class CsvProcessor implements FileProcessorInterface Log::debug('Now in CsvProcessor run(). Job is now running...'); $entries = new Collection($this->getImportArray()); + $this->addStep(); Log::notice('Building importable objects from CSV file.'); Log::debug(sprintf('Number of entries: %d', $entries->count())); $notImported = $entries->filter( @@ -97,7 +98,6 @@ class CsvProcessor implements FileProcessorInterface $row = array_values($row); if ($this->rowAlreadyImported($row)) { $message = sprintf('Row #%d has already been imported.', $index); - $this->repository->addStepsDone($this->job, 5); $this->repository->addError($this->job, $index, $message); Log::info($message); @@ -107,22 +107,16 @@ class CsvProcessor implements FileProcessorInterface return $row; } ); + $this->addStep(); Log::debug(sprintf('Number of entries left: %d', $notImported->count())); - // set (new) number of steps: - $extended = $this->getExtendedStatus(); - $steps = $notImported->count() * 5; - $extended['steps'] = $steps; - $this->setExtendedStatus($extended); - Log::debug(sprintf('Number of steps: %d', $steps)); - $notImported->each( function (array $row, int $index) { $journal = $this->importRow($index, $row); $this->objects->push($journal); - $this->repository->addStepsDone($this->job, 1); } ); + $this->addStep(); return true; } @@ -154,6 +148,14 @@ class CsvProcessor implements FileProcessorInterface return $this; } + /** + * Shorthand method. + */ + private function addStep() + { + $this->repository->addStepsDone($this->job, 1); + } + /** * Add meta data to the individual value and verify that it can be handled in a later stage. * @@ -212,7 +214,6 @@ class CsvProcessor implements FileProcessorInterface * * @throws \League\Csv\Exception * @throws \League\Csv\Exception - * @throws \Illuminate\Contracts\Filesystem\FileNotFoundException */ private function getImportArray(): Iterator { diff --git a/app/Import/Routine/FileRoutine.php b/app/Import/Routine/FileRoutine.php index 9edfc4be04..828dbd5d0f 100644 --- a/app/Import/Routine/FileRoutine.php +++ b/app/Import/Routine/FileRoutine.php @@ -96,17 +96,24 @@ class FileRoutine implements RoutineInterface set_time_limit(0); Log::info(sprintf('Start with import job %s', $this->job->key)); + // total steps: 6 + $this->setTotalSteps(6); + $importObjects = $this->getImportObjects(); $this->lines = $importObjects->count(); + $this->addStep(); + + // total steps can now be extended. File has been scanned. 7 steps per line: + $this->addTotalSteps(7 * $this->lines); // once done, use storage thing to actually store them: Log::info(sprintf('Returned %d valid objects from file processor', $this->lines)); $storage = $this->storeObjects($importObjects); + $this->addStep(); Log::debug('Back in run()'); - // update job: - $this->setStatus('finished'); + Log::debug('Updated job...'); Log::debug(sprintf('%d journals in $storage->journals', $storage->journals->count())); @@ -117,6 +124,10 @@ class FileRoutine implements RoutineInterface // create tag, link tag to all journals: $this->createImportTag(); + $this->addStep(); + + // update job: + $this->setStatus('finished'); Log::info(sprintf('Done with import job %s', $this->job->key)); @@ -158,6 +169,14 @@ class FileRoutine implements RoutineInterface return $objects; } + /** + * Shorthand method. + */ + private function addStep() + { + $this->repository->addStepsDone($this->job, 1); + } + /** * */ @@ -170,6 +189,7 @@ class FileRoutine implements RoutineInterface return new Tag; } + $this->addTotalSteps($this->journals->count() + 2); /** @var TagRepositoryInterface $repository */ $repository = app(TagRepositoryInterface::class); @@ -184,6 +204,7 @@ class FileRoutine implements RoutineInterface 'tagMode' => 'nothing', ]; $tag = $repository->store($data); + $this->addStep(); $extended = $this->getExtendedStatus(); $extended['tag'] = $tag->id; $this->setExtendedStatus($extended); @@ -195,9 +216,10 @@ class FileRoutine implements RoutineInterface foreach ($journalIds as $journalId) { Log::debug(sprintf('Linking journal #%d to tag #%d...', $journalId, $tagId)); DB::table('tag_transaction_journal')->insert(['transaction_journal_id' => $journalId, 'tag_id' => $tagId]); + $this->addStep(); } Log::info(sprintf('Linked %d journals to tag #%d ("%s")', $this->journals->count(), $tag->id, $tag->tag)); - + $this->addStep(); return $tag; } @@ -249,6 +271,26 @@ class FileRoutine implements RoutineInterface $this->repository->setStatus($this->job, $status); } + /** + * Shorthand + * + * @param int $steps + */ + private function setTotalSteps(int $steps) + { + $this->repository->setTotalSteps($this->job, $steps); + } + + /** + * Shorthand + * + * @param int $steps + */ + private function addTotalSteps(int $steps) + { + $this->repository->addTotalSteps($this->job, $steps); + } + /** * @param Collection $objects * diff --git a/app/Import/Storage/ImportStorage.php b/app/Import/Storage/ImportStorage.php index 1be3c5d9d8..1f6bc60b7b 100644 --- a/app/Import/Storage/ImportStorage.php +++ b/app/Import/Storage/ImportStorage.php @@ -36,6 +36,15 @@ use Log; /** * Is capable of storing individual ImportJournal objects. + * Adds 7 steps per object stored: + * 1. get all import data from import journal + * 2. is not a duplicate + * 3. create the journal + * 4. store journal + * 5. run rules + * 6. run bills + * 7. finished storing object + * * Class ImportStorage. */ class ImportStorage @@ -141,6 +150,7 @@ class ImportStorage function (ImportJournal $importJournal, int $index) { try { $this->storeImportJournal($index, $importJournal); + $this->addStep(); } catch (FireflyException | ErrorException | Exception $e) { $this->errors->push($e->getMessage()); Log::error(sprintf('Cannot import row #%d because: %s', $index, $e->getMessage())); @@ -171,9 +181,7 @@ class ImportStorage $opposingAccount = $this->getOpposingAccount($importJournal->opposing, $assetAccount->id, $amount); $transactionType = $this->getTransactionType($amount, $opposingAccount); $description = $importJournal->getDescription(); - - // First step done! - $this->repository->addStepsDone($this->job, 1); + $this->addStep(); /** * Check for double transfer. @@ -187,13 +195,17 @@ class ImportStorage 'opposing' => $opposingAccount->name, ]; if ($this->isDoubleTransfer($parameters) || $this->hashAlreadyImported($importJournal->hash)) { - $this->repository->addStepsDone($this->job, 3); // throw error $message = sprintf('Detected a possible duplicate, skip this one (hash: %s).', $importJournal->hash); Log::error($message, $parameters); + + // add five steps to keep the pace: + $this->addSteps(5); + throw new FireflyException($message); } unset($parameters); + $this->addStep(); // store journal and create transactions: $parameters = [ @@ -209,9 +221,7 @@ class ImportStorage ]; $journal = $this->storeJournal($parameters); unset($parameters); - - // Another step done! - $this->repository->addStepsDone($this->job, 1); + $this->addStep(); // store meta object things: $this->storeCategory($journal, $importJournal->category->getCategory()); @@ -233,18 +243,18 @@ class ImportStorage // set journal completed: $journal->completed = true; $journal->save(); - - // Another step done! - $this->repository->addStepsDone($this->job, 1); + $this->addStep(); // run rules if config calls for it: if (true === $this->applyRules) { Log::info('Will apply rules to this journal.'); $this->applyRules($journal); } + if (!(true === $this->applyRules)) { Log::info('Will NOT apply rules to this journal.'); } + $this->addStep(); // match bills if config calls for it. if (true === $this->matchBills) { @@ -255,9 +265,8 @@ class ImportStorage if (!(true === $this->matchBills)) { Log::info('Cannot match bills (yet), but do not have to.'); } + $this->addStep(); - // Another step done! - $this->repository->addStepsDone($this->job, 1); $this->journals->push($journal); Log::info(sprintf('Imported new journal #%d: "%s", amount %s %s.', $journal->id, $journal->description, $journal->transactionCurrency->code, $amount)); @@ -265,6 +274,24 @@ class ImportStorage return true; } + /** + * Shorthand method. + */ + private function addStep() + { + $this->repository->addStepsDone($this->job, 1); + } + + /** + * Shorthand method + * + * @param int $steps + */ + private function addSteps(int $steps) + { + $this->repository->addStepsDone($this->job, $steps); + } + /** * @param array $parameters * diff --git a/app/Import/Storage/ImportSupport.php b/app/Import/Storage/ImportSupport.php index 177831e4ba..2861dc9f6a 100644 --- a/app/Import/Storage/ImportSupport.php +++ b/app/Import/Storage/ImportSupport.php @@ -124,8 +124,8 @@ trait ImportSupport $transaction->transaction_journal_id = intval($parameters['id']); $transaction->transaction_currency_id = intval($parameters['currency']); $transaction->amount = $parameters['amount']; - $transaction->foreign_currency_id = intval($parameters['foreign_currency']); - $transaction->foreign_amount = $parameters['foreign_amount']; + $transaction->foreign_currency_id = intval($parameters['foreign_currency']) === 0 ? null : intval($parameters['foreign_currency']); + $transaction->foreign_amount = null === $transaction->foreign_currency_id ? null : $parameters['foreign_amount']; $transaction->save(); if (null === $transaction->id) { $errorText = join(', ', $transaction->getErrors()->all()); @@ -155,6 +155,7 @@ trait ImportSupport * @param ImportJournal $importJournal * * @return int + * @throws FireflyException */ private function getCurrencyId(ImportJournal $importJournal): int { @@ -192,7 +193,7 @@ trait ImportSupport { // use given currency by import journal. $currency = $importJournal->currency->getTransactionCurrency(); - if (null !== $currency->id && $currency->id !== $currencyId) { + if (null !== $currency->id && intval($currency->id) !== intval($currencyId)) { return $currency->id; } @@ -216,6 +217,7 @@ trait ImportSupport * @see ImportSupport::getTransactionType * * @return Account + * @throws FireflyException */ private function getOpposingAccount(ImportAccount $account, int $forbiddenAccount, string $amount): Account { @@ -435,8 +437,6 @@ trait ImportSupport if (!$journal->save()) { $errorText = join(', ', $journal->getErrors()->all()); - // add three steps: - $this->repository->addStepsDone($this->job, 3); // throw error throw new FireflyException($errorText); } diff --git a/app/Models/Account.php b/app/Models/Account.php index b04ec43d8e..81ad0d8016 100644 --- a/app/Models/Account.php +++ b/app/Models/Account.php @@ -212,7 +212,7 @@ class Account extends Model * * @return string */ - public function getNameAttribute($value): string + public function getNameAttribute($value): ?string { if ($this->encrypted) { return Crypt::decrypt($value); diff --git a/app/Models/ImportJob.php b/app/Models/ImportJob.php index c68e6464d7..9ad55c7c31 100644 --- a/app/Models/ImportJob.php +++ b/app/Models/ImportJob.php @@ -173,30 +173,6 @@ class ImportJob extends Model } } - /** - * @param int $steps - */ - public function setStepsDone(int $steps) - { - $status = $this->extended_status; - $status['done'] = $steps; - $this->extended_status = $status; - $this->save(); - Log::debug(sprintf('Set steps done for job "%s" to %d', $this->key, $steps)); - } - - /** - * @param int $count - */ - public function setTotalSteps(int $count) - { - $status = $this->extended_status; - $status['steps'] = $count; - $this->extended_status = $status; - $this->save(); - Log::debug(sprintf('Set total steps for job "%s" to %d', $this->key, $count)); - } - /** * @return string * diff --git a/app/Repositories/Account/AccountRepository.php b/app/Repositories/Account/AccountRepository.php index 328cfc21e7..36ca48e872 100644 --- a/app/Repositories/Account/AccountRepository.php +++ b/app/Repositories/Account/AccountRepository.php @@ -699,6 +699,7 @@ class AccountRepository implements AccountRepositoryInterface return null; } + return $iban; } } diff --git a/app/Repositories/ImportJob/ImportJobRepository.php b/app/Repositories/ImportJob/ImportJobRepository.php index 6573344311..722bb1b1f9 100644 --- a/app/Repositories/ImportJob/ImportJobRepository.php +++ b/app/Repositories/ImportJob/ImportJobRepository.php @@ -72,6 +72,23 @@ class ImportJobRepository implements ImportJobRepositoryInterface return $this->setExtendedStatus($job, $status); } + /** + * @param ImportJob $job + * @param int $steps + * + * @return ImportJob + */ + public function addTotalSteps(ImportJob $job, int $steps = 1): ImportJob + { + $extended = $this->getExtendedStatus($job); + $total = $extended['steps'] ?? 0; + $total += $steps; + $extended['steps'] = $total; + + return $this->setExtendedStatus($job, $extended); + + } + /** * Return number of imported rows with this hash value. * @@ -320,15 +337,17 @@ class ImportJobRepository implements ImportJobRepositoryInterface /** * @param ImportJob $job - * @param int $count + * @param int $steps * * @return ImportJob */ public function setStepsDone(ImportJob $job, int $steps): ImportJob { - $job->setStepsDone($steps); + $status = $this->getExtendedStatus($job); + $status['done'] = $steps; + Log::debug(sprintf('Set steps done for job "%s" to %d', $job->key, $steps)); - return $job; + return $this->setExtendedStatus($job, $status); } /** @@ -339,9 +358,11 @@ class ImportJobRepository implements ImportJobRepositoryInterface */ public function setTotalSteps(ImportJob $job, int $count): ImportJob { - $job->setTotalSteps($count); + $status = $this->getExtendedStatus($job); + $status['steps'] = $count; + Log::debug(sprintf('Set total steps for job "%s" to %d', $job->key, $count)); - return $job; + return $this->setExtendedStatus($job, $status); } /** diff --git a/app/Repositories/ImportJob/ImportJobRepositoryInterface.php b/app/Repositories/ImportJob/ImportJobRepositoryInterface.php index 9d20f30ee0..2c0efe97f2 100644 --- a/app/Repositories/ImportJob/ImportJobRepositoryInterface.php +++ b/app/Repositories/ImportJob/ImportJobRepositoryInterface.php @@ -49,6 +49,14 @@ interface ImportJobRepositoryInterface */ public function addStepsDone(ImportJob $job, int $steps = 1): ImportJob; + /** + * @param ImportJob $job + * @param int $steps + * + * @return ImportJob + */ + public function addTotalSteps(ImportJob $job, int $steps = 1): ImportJob; + /** * Return number of imported rows with this hash value. * diff --git a/public/js/ff/import/status.js b/public/js/ff/import/status.js index 77a4b25d80..abcc976e03 100644 --- a/public/js/ff/import/status.js +++ b/public/js/ff/import/status.js @@ -28,6 +28,7 @@ var interval = 500; var numberOfSteps = 0; var numberOfReports = 0; var jobFailed = false; +var pressedStart = false; // counts how many errors have been detected var knownErrors = 0; @@ -37,7 +38,11 @@ $(function () { console.log('in start'); timeOutId = setTimeout(checkJobStatus, startInterval); - $('.start-job').click(startJob); + $('.start-job').click(function () { + // notify (extra) that start button is pressed. + pressedStart = true; + startJob(); + }); if (job.configuration['auto-start']) { console.log('Called startJob()!'); startJob(); @@ -89,6 +94,15 @@ function reportOnJobStatus(data) { console.log('Job is auto start. Check status again in 500ms.'); timeOutId = setTimeout(checkJobStatus, interval); } + if (pressedStart) { + console.log('pressedStart = true, will check extra.'); + // do a time out just in case. Could be that job is running or is even done already. + timeOutId = setTimeout(checkJobStatus, 2000); + pressedStart = false; + } + if (!pressedStart) { + console.log('pressedStart = false, will do nothing.'); + } break; case "running": // job is running! Show the running box: @@ -115,6 +129,8 @@ function reportOnJobStatus(data) { case "finished": $('.statusbox').hide(); $('.status_finished').show(); + // report on detected errors: + reportOnErrors(data); // show text: $('#import-status-more-info').html(data.finishedText); break; @@ -176,8 +192,9 @@ function jobIsStalled(data) { * Only when job is in "configured" state. */ function startJob() { + console.log("In startJob()"); if (job.status === "configured") { - console.log("Job auto started!"); + console.log("Job started!"); // disable the button, add loading thing. $('.start-job').prop('disabled', true).text('...'); $.post(jobStartUri, {_token: token}).fail(reportOnSubmitError);