From 39e3346d73e0340f474a19cd97d78a0795d08167 Mon Sep 17 00:00:00 2001 From: Alexis Saettler Date: Sun, 27 Jan 2019 21:17:31 +0100 Subject: [PATCH] feat: use iterator reader for vcard imports (#2351) --- CHANGELOG | 1 + app/Models/Account/ImportJob.php | 37 ++-- app/Services/VCard/ImportVCard.php | 42 ++++- app/Traits/VCardImporter.php | 260 ---------------------------- tests/Unit/Models/ImportJobTest.php | 26 +-- 5 files changed, 72 insertions(+), 294 deletions(-) delete mode 100644 app/Traits/VCardImporter.php diff --git a/CHANGELOG b/CHANGELOG index 090f3c75a..0e10b0047 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -13,6 +13,7 @@ New features: Enhancements: +* Use iterator reader for vcard imports * Accept last name when using contact search field * Register all app services as singleton * Docker image: add sentry-cli and run sentry:release command if sentry is enabled diff --git a/app/Models/Account/ImportJob.php b/app/Models/Account/ImportJob.php index 3b5016128..2a499e95e 100644 --- a/app/Models/Account/ImportJob.php +++ b/app/Models/Account/ImportJob.php @@ -4,11 +4,14 @@ namespace App\Models\Account; use Exception; use App\Models\User\User; +use Sabre\VObject\Reader; +use Sabre\VObject\ParseException; use Sabre\VObject\Component\VCard; use App\Services\VCard\ImportVCard; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Facades\Storage; use Illuminate\Validation\ValidationException; +use Sabre\VObject\Splitter\VCard as VCardReader; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Contracts\Filesystem\FileNotFoundException; @@ -170,15 +173,7 @@ class ImportJob extends Model */ private function getEntries() { - $this->contacts_found = preg_match_all('/(BEGIN:VCARD.*?END:VCARD)/s', - $this->physicalFile, - $this->entries); - - $this->contacts_found = count($this->entries[0]); - - if ($this->contacts_found == 0) { - $this->fail(trans('settings.import_vcard_file_no_entries')); - } + $this->entries = new VCardReader($this->physicalFile, Reader::OPTION_FORGIVING + Reader::OPTION_IGNORE_INVALID_LINES); } /** @@ -188,15 +183,31 @@ class ImportJob extends Model */ private function processEntries($behaviour = ImportVCard::BEHAVIOUR_ADD) { - collect($this->entries[0])->each(function ($entry) use ($behaviour) { + while (true) { + try { + $entry = $this->entries->getNext(); + if (! $entry) { + // file end + break; + } + $this->contacts_found++; + } catch (ParseException $e) { + $this->skipEntry('?', (string) $e); + continue; + } + $this->processSingleEntry($entry, $behaviour); - }); + } + + if ($this->contacts_found == 0) { + $this->fail(trans('settings.import_vcard_file_no_entries')); + } } /** * Process a single vCard entry. * - * @param string $entry + * @param string|VCard $entry * @param string $behaviour */ private function processSingleEntry($entry, $behaviour = ImportVCard::BEHAVIOUR_ADD): void @@ -209,7 +220,7 @@ class ImportJob extends Model 'behaviour' => $behaviour, ]); } catch (ValidationException $e) { - $this->fail((string) $e); + $this->fail(implode(',', $e->validator->errors()->all())); return; } diff --git a/app/Services/VCard/ImportVCard.php b/app/Services/VCard/ImportVCard.php index 0e252fee2..f16228b58 100644 --- a/app/Services/VCard/ImportVCard.php +++ b/app/Services/VCard/ImportVCard.php @@ -75,6 +75,7 @@ class ImportVCard extends BaseService /** * Get the validation rules that apply to the service. * + * * @return array */ public function rules() @@ -83,7 +84,14 @@ class ImportVCard extends BaseService 'account_id' => 'required|integer|exists:accounts,id', 'user_id' => 'required|integer|exists:users,id', 'contact_id' => 'nullable|integer|exists:contacts,id', - 'entry' => 'required|string', + 'entry' => [ + 'required', + function ($attribute, $value, $fail) { + if (! is_string($value) && ! $value instanceof VCard) { + $fail($attribute.' must be a string or a VCard object.'); + } + }, + ], 'behaviour' => [ 'required', Rule::in(self::$behaviourTypes), @@ -204,10 +212,14 @@ class ImportVCard extends BaseService */ private function getEntry($data) { - try { - $entry = Reader::read($data['entry'], Reader::OPTION_FORGIVING + Reader::OPTION_IGNORE_INVALID_LINES); - } catch (ParseException $e) { - return; + $entry = $data['entry']; + + if (! $entry instanceof VCard) { + try { + $entry = Reader::read($entry, Reader::OPTION_FORGIVING + Reader::OPTION_IGNORE_INVALID_LINES); + } catch (ParseException $e) { + return; + } } return $entry; @@ -670,13 +682,19 @@ class ImportVCard extends BaseService return; } + $contactFieldTypeId = $this->getContactFieldTypeId('email'); + if (! $contactFieldTypeId) { + // Case of contact field type email does not exist + return; + } + foreach ($entry->EMAIL as $email) { if ($this->isValidEmail($email)) { ContactField::firstOrCreate([ 'account_id' => $contact->account_id, 'contact_id' => $contact->id, 'data' => $this->formatValue($email), - 'contact_field_type_id' => $this->getContactFieldTypeId('email'), + 'contact_field_type_id' => $contactFieldTypeId, ]); } } @@ -693,6 +711,12 @@ class ImportVCard extends BaseService return; } + $contactFieldTypeId = $this->getContactFieldTypeId('phone'); + if (! $contactFieldTypeId) { + // Case of contact field type phone does not exist + return; + } + foreach ($entry->TEL as $tel) { $tel = (string) $entry->TEL; @@ -704,7 +728,7 @@ class ImportVCard extends BaseService 'account_id' => $contact->account_id, 'contact_id' => $contact->id, 'data' => $this->formatValue($tel), - 'contact_field_type_id' => $this->getContactFieldTypeId('phone'), + 'contact_field_type_id' => $contactFieldTypeId, ]); } } @@ -765,9 +789,9 @@ class ImportVCard extends BaseService * Get the contact field type id for the $type. * * @param string $type The type of the ContactFieldType, or the name - * @return int + * @return int|null */ - private function getContactFieldTypeId(string $type): int + private function getContactFieldTypeId(string $type) { if (! array_has($this->contactFields, $type)) { $contactFieldType = ContactFieldType::where([ diff --git a/app/Traits/VCardImporter.php b/app/Traits/VCardImporter.php deleted file mode 100644 index c5618b39c..000000000 --- a/app/Traits/VCardImporter.php +++ /dev/null @@ -1,260 +0,0 @@ -workInit($matchCount)) { - return; - } - - // create special gender for this import - // we don't know which gender all the contacts are, so we need to create a special status for them, as we - // can't guess whether they are men, women or else. - $gender = Gender::where('name', 'vCard')->first(); - if (! $gender) { - $gender = new Gender; - $gender->account_id = $account_id; - $gender->name = 'vCard'; - $gender->save(); - } - - collect($matches[0])->map(function ($vcard) { - return Reader::read($vcard); - })->each(function (VCard $vcard) use ($account_id, $gender) { - if ($this->contactExists($vcard, $account_id)) { - $this->workContactExists($vcard); - $this->skippedContacts++; - - return; - } - - // Skip contact if there isn't a first name or a nickname - if (! $this->contactHasName($vcard)) { - $this->workContactNoFirstname($vcard); - $this->skippedContacts++; - - return; - } - - $this->vCardToContact($vcard, $account_id, $gender->id); - - $this->importedContacts++; - - $this->workNext($vcard); - }); - - $this->workEnd($matchCount, $this->skippedContacts, $this->importedContacts); - } - - private function vCardToContact($vcard, $account_id, $gender_id) - { - $contact = new Contact(); - $contact->account_id = $account_id; - $contact->gender_id = $gender_id; - - if ($vcard->N && ! empty($vcard->N->getParts()[1])) { - $contact->first_name = $this->formatValue($vcard->N->getParts()[1]); - $contact->middle_name = $this->formatValue($vcard->N->getParts()[2]); - $contact->last_name = $this->formatValue($vcard->N->getParts()[0]); - } else { - $contact->first_name = $this->formatValue($vcard->NICKNAME); - } - - $contact->company = $this->formatValue($vcard->ORG); - if ($vcard->ROLE) { - $contact->job = $this->formatValue($vcard->ROLE); - } - if ($vcard->TITLE) { - $contact->job = $this->formatValue($vcard->TITLE); - } - - $contact->setAvatarColor(); - - $contact->save(); - - if ($vcard->BDAY && ! empty((string) $vcard->BDAY)) { - $birthdate = new \DateTime((string) $vcard->BDAY); - - $specialDate = $contact->setSpecialDate('birthdate', $birthdate->format('Y'), $birthdate->format('m'), $birthdate->format('d')); - app(CreateReminder::class)->execute([ - 'account_id' => $contact->account_id, - 'contact_id' => $contact->id, - 'initial_date' => $specialDate->date->toDateString(), - 'frequency_type' => 'year', - 'frequency_number' => 1, - 'title' => trans( - 'people.people_add_birthday_reminder', - ['name' => $contact->first_name] - ), - 'delible' => false, - ]); - } - - if ($vcard->ADR) { - $request = [ - 'account_id' => $contact->account_id, - 'contact_id' => $contact->id, - 'street' => $this->formatValue($vcard->ADR->getParts()[2]), - 'city' => $this->formatValue($vcard->ADR->getParts()[3]), - 'province' => $this->formatValue($vcard->ADR->getParts()[4]), - 'postal_code' => $this->formatValue($vcard->ADR->getParts()[5]), - 'country' => CountriesHelper::find($vcard->ADR->getParts()[6]), - ]; - - app(CreateAddress::class)->execute($request); - } - - if (! is_null($this->formatValue($vcard->EMAIL))) { - // Saves the email - - $isValidEmail = filter_var($this->formatValue($vcard->EMAIL), FILTER_VALIDATE_EMAIL); - - if ($isValidEmail) { - $contactField = new ContactField; - $contactField->contact_id = $contact->id; - $contactField->account_id = $contact->account_id; - $contactField->data = $this->formatValue($vcard->EMAIL); - $contactField->contact_field_type_id = $this->contactFieldEmailId(); - $contactField->save(); - } - } - - if (! is_null($this->formatValue($vcard->TEL))) { - $tel = (string) $vcard->TEL; - - $countryISO = VCardHelper::getCountryISOFromSabreVCard($vcard); - $tel = LocaleHelper::formatTelephoneNumberByISO($tel, $countryISO); - - // Saves the phone number - $contactField = new ContactField; - $contactField->contact_id = $contact->id; - $contactField->account_id = $contact->account_id; - $contactField->data = $this->formatValue($tel); - $contactField->contact_field_type_id = $this->contactFieldPhoneId(); - $contactField->save(); - } - - $contact->updateGravatar(); - } - - private function contactFieldEmailId() - { - if (! $this->contactFieldEmailId) { - $contactFieldType = ContactFieldType::where('type', 'email')->first(); - $this->contactFieldEmailId = $contactFieldType->id; - } - - return $this->contactFieldEmailId; - } - - private function contactFieldPhoneId() - { - if (! $this->contactFieldPhoneId) { - $contactFieldType = ContactFieldType::where('type', 'phone')->first(); - $this->contactFieldPhoneId = $contactFieldType->id; - } - - return $this->contactFieldPhoneId; - } - - protected function workInit($matchCount) - { - return true; - } - - protected function workContactExists($vcard) - { - } - - protected function workContactNoFirstname($vcard) - { - } - - protected function workNext($vcard) - { - } - - protected function workEnd($matchCount, $skippedContacts, $importedContacts) - { - } - - /** - * Formats and returns a string for the contact. - * - * @param null|string $value - * @return null|string - */ - private function formatValue($value) - { - return ! empty((string) $value) ? (string) $value : null; - } - - /** - * Checks whether a contact already exists for a given account. - * - * @param VCard $vcard - * @param int $account_id - * @return bool - */ - private function contactExists(VCard $vcard, int $account_id) - { - $email = (string) $vcard->EMAIL; - - $isValidEmail = filter_var($email, FILTER_VALIDATE_EMAIL); - - if (! $isValidEmail) { - return false; - } - - $contactFieldType = ContactFieldType::where([ - ['account_id', $account_id], - ['type', 'email'], - ])->first(); - - $contactField = null; - - if ($contactFieldType) { - $contactField = ContactField::where([ - ['account_id', $account_id], - ['data', $email], - ['contact_field_type_id', $contactFieldType->id], - ])->first(); - } - - return $email && $contactField; - } - - /** - * Checks whether a contact has a first name or a nickname. - * Nickname is used as a fallback if no first name is provided. - * - * @param VCard $vcard - * @return bool - */ - public function contactHasName(VCard $vcard): bool - { - return ($vcard->N !== null && ! empty($vcard->N->getParts()[1])) || ! empty((string) $vcard->NICKNAME); - } -} diff --git a/tests/Unit/Models/ImportJobTest.php b/tests/Unit/Models/ImportJobTest.php index d244e6b76..affeac813 100644 --- a/tests/Unit/Models/ImportJobTest.php +++ b/tests/Unit/Models/ImportJobTest.php @@ -42,7 +42,7 @@ N:;;;; NICKNAME:Johnny ADR:;;17 Shakespeare Ave.;Southampton;;SO17 2HB;United Kingdom END:VCARD - '; +'; public function test_it_belongs_to_a_user() { @@ -176,9 +176,8 @@ END:VCARD public function test_it_calculates_how_many_entries_there_are_and_populate_the_entries_array() { Storage::fake('public'); - $importJob = factory(ImportJob::class)->create([ - 'filename' => 'testfile.vcf', - ]); + $importJob = $this->createImportJob(); + $importJob->filename = 'testfile.vcf'; Storage::disk('public')->put( 'testfile.vcf', @@ -187,16 +186,13 @@ END:VCARD $this->invokePrivateMethod($importJob, 'getPhysicalFile'); $this->invokePrivateMethod($importJob, 'getEntries'); + $this->invokePrivateMethod($importJob, 'processEntries'); + $this->assertJobSuccess($importJob); $this->assertEquals( 3, $importJob->contacts_found ); - - $this->assertEquals( - 3, - count($importJob->entries[0]) - ); } public function test_it_doesnt_process_an_entry_if_import_is_not_feasible() @@ -210,6 +206,7 @@ END:VCARD $this->invokePrivateMethod($importJob, 'processSingleEntry', [ $vcard->serialize(), ]); + $this->assertJobSuccess($importJob); $this->assertEquals( 1, $importJob->contacts_skipped @@ -238,9 +235,8 @@ END:VCARD 'EMAIL' => 'john@doe.com', ]); - $this->invokePrivateMethod($importJob, 'processSingleEntry', [ - $vcard->serialize(), - ]); + $this->invokePrivateMethod($importJob, 'processSingleEntry', [$vcard]); + $this->assertJobSuccess($importJob); $this->assertEquals( 1, $importJob->contacts_skipped @@ -325,6 +321,12 @@ END:VCARD return factory(ImportJob::class)->create([ 'account_id' => $account->id, 'user_id' => $user->id, + 'failed' => false, ]); } + + private function assertJobSuccess($importJob) + { + $this->assertFalse($importJob->failed, 'Job has failed, reason: '.$importJob->failed_reason); + } }