From 200366f5be099bb7948c6384ff769bc315fa52e1 Mon Sep 17 00:00:00 2001 From: James Cole Date: Wed, 10 Aug 2016 18:49:16 +0200 Subject: [PATCH] Extended the validator and created more code to handle exceptions. Signed-off-by: James Cole --- app/Console/Commands/Import.php | 18 +- app/Crud/Account/AccountCrud.php | 18 + app/Crud/Account/AccountCrudInterface.php | 25 +- app/Import/Converter/AssetAccountNumber.php | 17 +- app/Import/Converter/ExternalId.php | 37 ++ .../Converter/OpposingAccountNumber.php | 17 +- app/Import/ImportEntry.php | 12 +- app/Import/ImportValidator.php | 343 ++++++++++++++++++ app/Import/Importer/CsvImporter.php | 83 ++--- app/Import/Importer/ImporterInterface.php | 5 +- app/Import/Logging/CommandHandler.php | 2 +- app/Import/notes.txt | 3 +- resources/lang/en_US/csv.php | 1 + 13 files changed, 510 insertions(+), 71 deletions(-) create mode 100644 app/Import/Converter/ExternalId.php create mode 100644 app/Import/ImportValidator.php diff --git a/app/Console/Commands/Import.php b/app/Console/Commands/Import.php index 33be3c8e06..483eb179d9 100644 --- a/app/Console/Commands/Import.php +++ b/app/Console/Commands/Import.php @@ -11,8 +11,9 @@ declare(strict_types = 1); namespace FireflyIII\Console\Commands; +use FireflyIII\Crud\Account\AccountCrud; use FireflyIII\Import\Importer\ImporterInterface; -use FireflyIII\Import\Setup\SetupInterface; +use FireflyIII\Import\ImportValidator; use FireflyIII\Import\Logging\CommandHandler; use FireflyIII\Models\ImportJob; use Illuminate\Console\Command; @@ -79,7 +80,20 @@ class Import extends Command $monolog = Log::getMonolog(); $handler = new CommandHandler($this); $monolog->pushHandler($handler); - $importer->start(); + + // create import entries + $collection = $importer->createImportEntries(); + + // validate / clean collection: + $validator = new ImportValidator($collection); + $validator->setUser($job->user); + if ($job->configuration['import-account'] != 0) { + $repository = app(AccountCrud::class, [$job->user]); + $validator->setDefaultImportAccount($repository->find($job->configuration['import-account'])); + } + + $validator->clean(); + $this->line('Something something import: ' . $jobKey); } diff --git a/app/Crud/Account/AccountCrud.php b/app/Crud/Account/AccountCrud.php index f81bbb3263..075374efe5 100644 --- a/app/Crud/Account/AccountCrud.php +++ b/app/Crud/Account/AccountCrud.php @@ -275,6 +275,24 @@ class AccountCrud implements AccountCrudInterface return $account; } + /** + * @param Account $account + * @param string $type + * + * @return Account + */ + public function updateAccountType(Account $account, string $type): Account + { + $type = AccountType::whereType($type)->first(); + if (!is_null($type)) { + $account->account_type_id = $type->id; + $account->save(); + } + + return $account; + + } + /** * @param array $data * diff --git a/app/Crud/Account/AccountCrudInterface.php b/app/Crud/Account/AccountCrudInterface.php index 41b6956cfa..95e794418d 100644 --- a/app/Crud/Account/AccountCrudInterface.php +++ b/app/Crud/Account/AccountCrudInterface.php @@ -37,6 +37,14 @@ interface AccountCrudInterface */ public function find(int $accountId): Account; + /** + * @param string $number + * @param array $types + * + * @return Account + */ + public function findByAccountNumber(string $number, array $types): Account; + /** * @param string $iban * @param array $types @@ -53,14 +61,6 @@ interface AccountCrudInterface */ public function findByName(string $name, array $types): Account; - /** - * @param string $number - * @param array $types - * - * @return Account - */ - public function findByAccountNumber(string $number, array $types): Account; - /** * @param array $accountIds * @@ -91,7 +91,6 @@ interface AccountCrudInterface */ public function storeMeta(Account $account, string $name, $value): AccountMeta; - /** * @param Account $account * @param array $data @@ -99,4 +98,12 @@ interface AccountCrudInterface * @return Account */ public function update(Account $account, array $data): Account; + + /** + * @param Account $account + * @param string $type + * + * @return Account + */ + public function updateAccountType(Account $account, string $type): Account; } \ No newline at end of file diff --git a/app/Import/Converter/AssetAccountNumber.php b/app/Import/Converter/AssetAccountNumber.php index aacef8ae7a..cc478ca94a 100644 --- a/app/Import/Converter/AssetAccountNumber.php +++ b/app/Import/Converter/AssetAccountNumber.php @@ -12,7 +12,6 @@ declare(strict_types = 1); namespace FireflyIII\Import\Converter; use FireflyIII\Crud\Account\AccountCrudInterface; -use FireflyIII\Exceptions\FireflyException; use FireflyIII\Models\Account; use FireflyIII\Models\AccountType; use Log; @@ -58,13 +57,25 @@ class AssetAccountNumber extends BasicConverter implements ConverterInterface if (!is_null($account->id)) { Log::debug('Found account by name', ['id' => $account->id]); $this->setCertainty(50); + + return $account; + } + + // try to find by the name we would give it: + $accountName = 'Asset account with number ' . e($value); + $account = $repository->findByName($accountName, [AccountType::ASSET]); + if (!is_null($account->id)) { + Log::debug('Found account by name', ['id' => $account->id]); + $this->setCertainty(50); + return $account; } $account = $repository->store( - ['name' => 'Account with number ' . $value, 'openingBalance' => 0, 'iban' => null, 'user' => $this->user->id, 'accountType' => 'asset', - 'virtualBalance' => 0, 'active' => true] + ['name' => $accountName, 'openingBalance' => 0, 'iban' => null, 'user' => $this->user->id, + 'accountType' => 'asset', + 'virtualBalance' => 0, 'accountNumber' => $value, 'active' => true] ); $this->setCertainty(100); diff --git a/app/Import/Converter/ExternalId.php b/app/Import/Converter/ExternalId.php new file mode 100644 index 0000000000..272dc5b7d7 --- /dev/null +++ b/app/Import/Converter/ExternalId.php @@ -0,0 +1,37 @@ +setCertainty(100); + + return strval(trim($value)); + + } +} \ No newline at end of file diff --git a/app/Import/Converter/OpposingAccountNumber.php b/app/Import/Converter/OpposingAccountNumber.php index e47e596a38..cb97af5cf0 100644 --- a/app/Import/Converter/OpposingAccountNumber.php +++ b/app/Import/Converter/OpposingAccountNumber.php @@ -14,6 +14,7 @@ namespace FireflyIII\Import\Converter; use FireflyIII\Crud\Account\AccountCrudInterface; use FireflyIII\Exceptions\FireflyException; use FireflyIII\Models\Account; +use FireflyIII\Models\AccountType; use Log; /** @@ -61,13 +62,23 @@ class OpposingAccountNumber extends BasicConverter implements ConverterInterface return $account; } + // try to find by the name we would give it: + $accountName = 'Import account with number ' . e($value); + $account = $repository->findByName($accountName, [AccountType::IMPORT]); + if (!is_null($account->id)) { + Log::debug('Found account by name', ['id' => $account->id]); + $this->setCertainty(50); + + return $account; + } + $account = $repository->store( - ['name' => 'Account with number ' . $value, 'openingBalance' => 0, 'iban' => null, 'user' => $this->user->id, 'accountType' => 'asset', - 'virtualBalance' => 0, 'active' => true] + ['name' => $accountName, 'openingBalance' => 0, 'iban' => null, 'user' => $this->user->id, + 'accountType' => 'import', + 'virtualBalance' => 0, 'accountNumber' => $value, 'active' => true] ); $this->setCertainty(100); - return $account; } diff --git a/app/Import/ImportEntry.php b/app/Import/ImportEntry.php index 05b3d76a10..5d30621124 100644 --- a/app/Import/ImportEntry.php +++ b/app/Import/ImportEntry.php @@ -14,10 +14,6 @@ namespace FireflyIII\Import; use Carbon\Carbon; use FireflyIII\Exceptions\FireflyException; use FireflyIII\Models\Account; -use FireflyIII\Models\TransactionCurrency; -use FireflyIII\Models\TransactionJournal; -use FireflyIII\Models\TransactionType; -use FireflyIII\Repositories\Currency\CurrencyRepositoryInterface; use FireflyIII\User; use Illuminate\Support\Collection; use Log; @@ -33,10 +29,10 @@ class ImportEntry public $certain = []; /** @var array */ public $fields = []; - /** @var User */ public $user; - + /** @var bool */ + public $valid = true; /** @var array */ private $validFields = ['amount', @@ -45,6 +41,7 @@ class ImportEntry 'date-book', 'description', 'date-process', + 'transaction-type', 'currency', 'asset-account', 'opposing-account', 'bill', 'budget', 'category', 'tags']; /** @@ -154,6 +151,9 @@ class ImportEntry case 'tags-comma': case 'tags-space': $this->appendCollection('tags', $convertedValue); + case 'external-id': + // ignore for now. + break; } } diff --git a/app/Import/ImportValidator.php b/app/Import/ImportValidator.php new file mode 100644 index 0000000000..761f421b13 --- /dev/null +++ b/app/Import/ImportValidator.php @@ -0,0 +1,343 @@ +entries = $entries; + } + + /** + * Clean collection by filling in all the blanks. + */ + public function clean() + { + /** @var ImportEntry $entry */ + foreach ($this->entries as $entry) { + /* + * X Adds the date (today) if no date is present. + * X Determins the types of accounts involved (asset, expense, revenue). + * X Determins the type of transaction (withdrawal, deposit, transfer). + * - Determins the currency of the transaction. + * X Adds a default description if there isn't one present. + */ + $this->checkAmount($entry); + $this->setDate($entry); + $this->setAssetAccount($entry); + $this->setOpposingAccount($entry); + $this->cleanDescription($entry); + $this->setTransactionType($entry); + $this->setTransactionCurrency($entry); + } + + + /** @var ImportEntry $entry */ + foreach ($this->entries as $entry) { + Log::debug('Description: ' . $entry->fields['description']); + } + + } + + /** + * @param Account $defaultImportAccount + */ + public function setDefaultImportAccount(Account $defaultImportAccount) + { + $this->defaultImportAccount = $defaultImportAccount; + } + + /** + * @param User $user + */ + public function setUser(User $user) + { + $this->user = $user; + } + + /** + * @param ImportEntry $entry + */ + private function checkAmount(ImportEntry $entry) + { + if ($entry->fields['amount'] == 0) { + $entry->valid = false; + Log::error('Amount of transaction is zero, cannot handle.'); + } + Log::debug('Amount is OK.'); + } + + /** + * @param ImportEntry $entry + */ + private function cleanDescription(ImportEntry $entry) + { + if (!isset($entry->fields['description'])) { + Log::debug('Set empty transaction description because field was not set.'); + $entry->fields['description'] = '(empty transaction description)'; + + return; + } + if (is_null($entry->fields['description'])) { + Log::debug('Set empty transaction description because field was null.'); + $entry->fields['description'] = '(empty transaction description)'; + + return; + } + + if (strlen($entry->fields['description']) == 0) { + Log::debug('Set empty transaction description because field was empty.'); + $entry->fields['description'] = '(empty transaction description)'; + + return; + } + Log::debug('Transaction description is OK.'); + $entry->fields['description'] = trim($entry->fields['description']); + } + + /** + * @param Account $account + * @param string $type + * + * @return Account + */ + private function convertAccount(Account $account, string $type): Account + { + $accountType = $account->accountType->type; + if ($accountType === $type) { + return $account; + } + // find it first by new type: + /** @var AccountCrudInterface $repository */ + $repository = app(AccountCrudInterface::class, [$this->user]); + $result = $repository->findByName($account->name, [$type]); + if (is_null($result->id)) { + // can convert account: + $result = $repository->updateAccountType($account, $type); + } + + return $result; + + + } + + /** + * @return Account + */ + private function fallbackExpenseAccount(): Account + { + + /** @var AccountCrudInterface $repository */ + $repository = app(AccountCrudInterface::class, [$this->user]); + $name = 'Unknown expense account'; + $result = $repository->findByName($name, [AccountType::EXPENSE]); + if (is_null($result->id)) { + $result = $repository->store( + ['name' => $name, 'iban' => null, 'openingBalance' => 0, 'user' => $this->user->id, 'accountType' => 'expense', 'virtualBalance' => 0, + 'active' => true] + ); + } + + return $result; + } + + /** + * @return Account + */ + private function fallbackRevenueAccount(): Account + { + + /** @var AccountCrudInterface $repository */ + $repository = app(AccountCrudInterface::class, [$this->user]); + $name = 'Unknown revenue account'; + $result = $repository->findByName($name, [AccountType::REVENUE]); + if (is_null($result->id)) { + $result = $repository->store( + ['name' => $name, 'iban' => null, 'openingBalance' => 0, 'user' => $this->user->id, 'accountType' => 'revenue', 'virtualBalance' => 0, + 'active' => true] + ); + } + + return $result; + } + + /** + * @param ImportEntry $entry + */ + private function setAssetAccount(ImportEntry $entry) + { + if (is_null($entry->fields['asset-account'])) { + if (!is_null($this->defaultImportAccount)) { + Log::debug('Set asset account from default asset account'); + $entry->fields['asset-account'] = $this->defaultImportAccount; + + return; + } + // default import is null? should not happen. Entry cannot be imported. + // set error message and block. + $entry->valid = false; + Log::error('Cannot import entry. Asset account is NULL and import account is also NULL.'); + } + Log::debug('Asset account is OK.'); + } + + + /** + * @param ImportEntry $entry + */ + private function setDate(ImportEntry $entry) + { + if (is_null($entry->fields['date-transaction'])) { + // empty date field? find alternative. + $alternatives = ['date-book', 'date-interest', 'date-process']; + foreach ($alternatives as $alternative) { + if (!is_null($entry->fields[$alternative])) { + Log::debug(sprintf('Copied date-transaction from %s.', $alternative)); + $entry->fields['date-transaction'] = clone $entry->fields[$alternative]; + + return; + } + } + // date is still null at this point + Log::debug('Set date-transaction to today.'); + $entry->fields['date-transaction'] = new Carbon; + + return; + } + Log::debug('Date-transaction is OK'); + + + } + + /** + * @param ImportEntry $entry + */ + private function setOpposingAccount(ImportEntry $entry) + { + // empty opposing account. Create one based on amount. + if (is_null($entry->fields['opposing-account'])) { + + if ($entry->fields['amount'] < 0) { + // create or find general opposing expense account. + Log::debug('Created fallback expense account'); + $entry->fields['opposing-account'] = $this->fallbackExpenseAccount(); + + return; + } + Log::debug('Created fallback revenue account'); + $entry->fields['opposing-account'] = $this->fallbackRevenueAccount(); + + return; + } + + // opposing is of type "import". Convert to correct type (by amount): + $type = $entry->fields['opposing-account']->accountType->type; + if ($type == AccountType::IMPORT && $entry->fields['amount'] < 0) { + $account = $this->convertAccount($entry->fields['opposing-account'], AccountType::EXPENSE); + $entry->fields['opposing-account'] = $account; + Log::debug('Converted import account to expense account'); + + return; + } + if ($type == AccountType::IMPORT && $entry->fields['amount'] > 0) { + $account = $this->convertAccount($entry->fields['opposing-account'], AccountType::REVENUE); + $entry->fields['opposing-account'] = $account; + Log::debug('Converted import account to revenue account'); + + return; + } + // amount < 0, but opposing is revenue + if ($type == AccountType::REVENUE && $entry->fields['amount'] < 0) { + $account = $this->convertAccount($entry->fields['opposing-account'], AccountType::EXPENSE); + $entry->fields['opposing-account'] = $account; + Log::debug('Converted revenue account to expense account'); + + return; + } + + // amount > 0, but opposing is expense + if ($type == AccountType::EXPENSE && $entry->fields['amount'] < 0) { + $account = $this->convertAccount($entry->fields['opposing-account'], AccountType::REVENUE); + $entry->fields['opposing-account'] = $account; + Log::debug('Converted expense account to revenue account'); + + return; + } + // account type is OK + Log::debug('Opposing account is OK.'); + + } + + /** + * @param ImportEntry $entry + */ + private function setTransactionCurrency(ImportEntry $entry) + { + if (is_null($entry->fields['currency'])) { + /** @var CurrencyRepositoryInterface $repository */ + $repository = app(CurrencyRepositoryInterface::class); + $entry->fields['currency'] = $repository->findByCode(env('DEFAULT_CURRENCY', 'EUR')); + Log::debug('Set currency to EUR'); + return; + } + Log::debug('Currency is OK'); + } + + /** + * @param ImportEntry $entry + */ + private function setTransactionType(ImportEntry $entry) + { + $type = $entry->fields['opposing-account']->accountType->type; + switch ($type) { + case AccountType::EXPENSE: + $entry->fields['transaction-type'] = TransactionType::WITHDRAWAL; + break; + case AccountType::REVENUE: + $entry->fields['transaction-type'] = TransactionType::DEPOSIT; + break; + case AccountType::ASSET: + $entry->fields['transaction-type'] = TransactionType::TRANSFER; + break; + } + } + + +} \ No newline at end of file diff --git a/app/Import/Importer/CsvImporter.php b/app/Import/Importer/CsvImporter.php index e989d603a6..9f74eab0ec 100644 --- a/app/Import/Importer/CsvImporter.php +++ b/app/Import/Importer/CsvImporter.php @@ -14,9 +14,9 @@ namespace FireflyIII\Import\Importer; use FireflyIII\Crud\Account\AccountCrud; use FireflyIII\Import\Converter\ConverterInterface; use FireflyIII\Import\ImportEntry; -use FireflyIII\Import\ImportResult; use FireflyIII\Models\Account; use FireflyIII\Models\ImportJob; +use Illuminate\Support\Collection; use League\Csv\Reader; use Log; @@ -30,8 +30,33 @@ class CsvImporter implements ImporterInterface /** @var ImportJob */ public $job; - /** @var Account */ - public $defaultImportAccount; + /** + * Run the actual import + * + * @return Collection + */ + public function createImportEntries(): Collection + { + $config = $this->job->configuration; + $content = $this->job->uploadFileContents(); + + // create CSV reader. + $reader = Reader::createFromString($content); + $start = $config['has-headers'] ? 1 : 0; + $results = $reader->fetch(); + $collection = new Collection; + foreach ($results as $index => $row) { + if ($index >= $start) { + Log::debug(sprintf('Now going to import row %d.', $index)); + $importEntry = $this->importSingleRow($index, $row); + $collection->push($importEntry); + } + } + Log::debug(sprintf('Collection contains %d entries', $collection->count())); + Log::debug('This call should be intercepted somehow.'); + + return $collection; + } /** * @param ImportJob $job @@ -41,45 +66,13 @@ class CsvImporter implements ImporterInterface $this->job = $job; } - /** - * Run the actual import - * - * @return bool - */ - public function start(): bool - { - $config = $this->job->configuration; - $content = $this->job->uploadFileContents(); - - if ($config['import-account'] != 0) { - $repository = app(AccountCrud::class, [$this->job->user]); - $this->defaultImportAccount = $repository->find($config['import-account']); - } - - // create CSV reader. - $reader = Reader::createFromString($content); - $start = $config['has-headers'] ? 1 : 0; - $results = $reader->fetch(); - foreach ($results as $index => $row) { - if ($index >= $start) { - Log::debug(sprintf('Now going to import row %d.', $index)); - $this->importSingleRow($index, $row); - } - } - - Log::debug('This call should be intercepted somehow.'); - - return true; - } - - /** * @param int $index * @param array $row * - * @return ImportResult + * @return ImportEntry */ - private function importSingleRow(int $index, array $row): ImportResult + private function importSingleRow(int $index, array $row): ImportEntry { // create import object: $object = new ImportEntry; @@ -113,13 +106,15 @@ class CsvImporter implements ImporterInterface $object->importValue($role, $value, $certainty, $convertedValue); } - $result = $object->import(); - if ($result->failed()) { - Log::error(sprintf('Import of row %d has failed.', $index), $result->errors->toArray()); - } + return $object; - exit; - - return true; + // $result = $object->import(); + // if ($result->failed()) { + // Log::error(sprintf('Import of row %d has failed.', $index), $result->errors->toArray()); + // } + // + // exit; + // + // return true; } } \ No newline at end of file diff --git a/app/Import/Importer/ImporterInterface.php b/app/Import/Importer/ImporterInterface.php index 308d495040..08fbb591fe 100644 --- a/app/Import/Importer/ImporterInterface.php +++ b/app/Import/Importer/ImporterInterface.php @@ -11,6 +11,7 @@ declare(strict_types = 1); namespace FireflyIII\Import\Importer; use FireflyIII\Models\ImportJob; +use Illuminate\Support\Collection; /** * Interface ImporterInterface @@ -22,9 +23,9 @@ interface ImporterInterface /** * Run the actual import * - * @return bool + * @return Collection */ - public function start(): bool; + public function createImportEntries(): Collection; /** * @param ImportJob $job diff --git a/app/Import/Logging/CommandHandler.php b/app/Import/Logging/CommandHandler.php index 6b4d3106c1..d4f2021948 100644 --- a/app/Import/Logging/CommandHandler.php +++ b/app/Import/Logging/CommandHandler.php @@ -44,6 +44,6 @@ class CommandHandler extends AbstractProcessingHandler */ protected function write(array $record) { - $this->command->line((string) $record['formatted']); + $this->command->line((string) trim($record['formatted'])); } } \ No newline at end of file diff --git a/app/Import/notes.txt b/app/Import/notes.txt index 045c762e4a..603e185f84 100644 --- a/app/Import/notes.txt +++ b/app/Import/notes.txt @@ -36,11 +36,12 @@ In any case, this newly minted set of ImportEntries (it cannot be saved or store this has to be done in one go) is then fed to the ImportValidator which will reject entries (almost never) and corrects fields if necessary. -- Adds a default description if there isn't one present. + - Adds the date (today) if no date is present. - Determins the type of transaction (withdrawal, deposit, transfer). - Determins the types of accounts involved (asset, expense, revenue). - Determins the currency of the transaction. +- Adds a default description if there isn't one present. This set of corrected ImportEntries is then fed to the ImportStorage class which will generate TransactionJournals, Transactions and other related objects. \ No newline at end of file diff --git a/resources/lang/en_US/csv.php b/resources/lang/en_US/csv.php index dc9b75e148..231c3c54fc 100644 --- a/resources/lang/en_US/csv.php +++ b/resources/lang/en_US/csv.php @@ -66,6 +66,7 @@ return [ 'column_description' => 'Description', 'column_opposing-iban' => 'Opposing account (IBAN)', 'column_opposing-id' => 'Opposing account ID (matching Firefly)', + 'column_external-id' => 'External ID', 'column_opposing-name' => 'Opposing account (name)', 'column_rabo-debet-credit' => 'Rabobank specific debet/credit indicator', 'column_ing-debet-credit' => 'ING specific debet/credit indicator',