From cf03d25934d3760ac407e1cbe7f7dccc43216a3d Mon Sep 17 00:00:00 2001 From: Daniel Meltzer Date: Tue, 17 Jul 2018 19:46:08 -0400 Subject: [PATCH] Fix importer emailformat (#5871) * Fix Importer emailformat Str::slug() strips periods from the string, which caused our existing logic to misbehave when generating a user's email on an import. Adjust logic to use generateEmail() helper on user instead. Also clean up some of the logic in this method. * Remove dead code. * More refactor/cleanup of the user create method. I think it is almost readable now. --- app/Importer/Importer.php | 131 ++++++++++++++-------------- app/Importer/ItemImporter.php | 4 +- app/Models/User.php | 43 +++------ database/factories/ModelFactory.php | 1 + tests/unit/ImporterTest.php | 2 +- 5 files changed, 81 insertions(+), 100 deletions(-) diff --git a/app/Importer/Importer.php b/app/Importer/Importer.php index 221e2b9860..bcb80e7072 100644 --- a/app/Importer/Importer.php +++ b/app/Importer/Importer.php @@ -249,82 +249,83 @@ abstract class Importer * @since 3.0 * @param $row array * @return User Model w/ matching name - * @internal param string $user_username Username extracted from CSV - * @internal param string $user_email Email extracted from CSV - * @internal param string $first_name - * @internal param string $last_name + * @internal param array $user_array User details parsed from csv */ protected function createOrFetchUser($row) { - $user_name = $this->findCsvMatch($row, "full_name"); - $user_email = $this->findCsvMatch($row, "email"); - $user_username = $this->findCsvMatch($row, "username"); - $first_name = ''; - $last_name = ''; - if(empty($user_name) && empty($user_email) && empty($user_username)) { - $this->log('No user data provided - skipping user creation, just adding asset'); + $user_array = [ + 'full_name' => $this->findCsvMatch($row, "full_name"), + 'email' => $this->findCsvMatch($row, "email"), + 'username' => $this->findCsvMatch($row, "username") + ]; + // If the full name is empty, bail out--we need this to extract first name (at the very least) + if(empty($user_array['full_name'])) { + $this->log('Insufficient user data provided (Full name is required)- skipping user creation, just adding asset'); return false; } - if( !empty($user_username)) { - // A username was given. - $user = User::where('username', $user_username)->first(); - if($user) { - return $user; + // Is the user actually an ID? + if($user = $this->findUserByNumber($user_array['full_name'])) { + return $user; + } + $this->log('User does not appear to be an id with number: '.$user_array['full_name'].'. Continuing through our processes'); + + // Populate email if it does not exist. + if(empty($user_array['email'])) { + $user_array['email'] = User::generateEmailFromFullName($user_array['full_name']); + } + + $user_formatted_array = User::generateFormattedNameFromFullName(Setting::getSettings()->username_format, $user_array['full_name']); + $user_array['first_name'] = $user_formatted_array['first_name']; + $user_array['last_name'] = $user_formatted_array['last_name']; + if (empty($user_array['username'])) { + $user_array['username'] = $user_formatted_array['username']; + if ($this->usernameFormat =='email') { + $user_array['username'] = $user_array['email']; } } + + // If at this point we have not found a username or first name, bail out in shame. + if(empty($user_array['username']) || empty($user_array['first_name'])) { + return false; + } + + // Check for a matching user after trying to guess username. + if($user = User::where('username', $user_array['username'])->first()) { + $this->log('User '.$user_array['username'].' already exists'); + return $user; + } + + + // No Luck, let's create one. + $user = new User; + $user->first_name = $user_array['first_name']; + $user->last_name = $user_array['last_name']; + $user->username = $user_array['username']; + $user->email = $user_array['email']; + $user->activated = 1; + $user->password = $this->tempPassword; + + if ($user->save()) { + $this->log('User '.$user_array['username'].' created'); + return $user; + } + $this->logError($user, 'User "' . $user_array['username'] . '" was not able to be created.'); + return false; + } + + /** + * Matches a user by user_id if user_name provided is a number + * @param string $user_name users full name from csv + * @return User User Matching ID + */ + protected function findUserByNumber($user_name) + { // A number was given instead of a name if (is_numeric($user_name)) { - $this->log('User '.$user_name.' is not a name - assume this user already exists'); - $user = User::find($user_name); - if($user) { - return $user; - } - $this->log('User with id'.$user_name.' does not exist. Continuing through our processes'); + $this->log('User '.$user_name.' is a number - lets see if it matches a user id'); + return User::find($user_name); } - // Generate data based on user name. - $user_email_array = User::generateFormattedNameFromFullName(Setting::getSettings()->email_format, $user_name); - $first_name = $user_email_array['first_name']; - $last_name = $user_email_array['last_name']; - - if (empty($user_email)) { - if (Setting::getSettings()->email_domain) { - $user_email = str_slug($user_email_array['username']).'@'.Setting::getSettings()->email_domain; - } - } - - if (empty($user_username)) { - if ($this->usernameFormat =='email') { - $user_username = $user_email; - } else { - $user_name_array = User::generateFormattedNameFromFullName(Setting::getSettings()->username_format, $user_name); - $user_username = $user_name_array['username']; - } - } - $user = new User; - - if (!empty($user_username)) { - - if ($user = User::MatchEmailOrUsername($user_username, $user_email) - ->whereNotNull('username')->first()) { - $this->log('User '.$user_username.' already exists'); - } elseif (( $first_name != '') && ($last_name != '') && ($user_username != '')) { - $user = new User; - $user->first_name = $first_name; - $user->last_name = $last_name; - $user->username = $user_username; - $user->email = $user_email; - $user->activated = 1; - $user->password = $this->tempPassword; - - if ($user->save()) { - $this->log('User '.$first_name.' created'); - } else { - $this->logError($user, 'User "' . $first_name . '"'); - } - } - } - return $user; } /** diff --git a/app/Importer/ItemImporter.php b/app/Importer/ItemImporter.php index 5877ad81dd..9a47948668 100644 --- a/app/Importer/ItemImporter.php +++ b/app/Importer/ItemImporter.php @@ -84,15 +84,13 @@ class ItemImporter extends Importer */ protected function determineCheckout($row) { - // We only supporty checkout-to-location for asset, so short circuit otherw. + // We only support checkout-to-location for asset, so short circuit otherw. if(get_class($this) != AssetImporter::class) { return $this->createOrFetchUser($row); } if ($this->item['checkout_class'] === 'location') { - // dd($this->findCsvMatch($row, 'checkout_location')); return Location::findOrFail($this->createOrFetchLocation($this->findCsvMatch($row, 'checkout_location'))); - // dd('here'); } return $this->createOrFetchUser($row); diff --git a/app/Models/User.php b/app/Models/User.php index 9ba915e5b1..0204317e54 100755 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -71,20 +71,20 @@ class User extends SnipeModel implements AuthenticatableContract, CanResetPasswo ]; use Searchable; - + /** * The attributes that should be included when searching the model. - * + * * @var array */ protected $searchableAttributes = [ - 'first_name', - 'last_name', - 'email', - 'username', - 'notes', - 'phone', - 'jobtitle', + 'first_name', + 'last_name', + 'email', + 'username', + 'notes', + 'phone', + 'jobtitle', 'employee_num' ]; @@ -201,7 +201,6 @@ class User extends SnipeModel implements AuthenticatableContract, CanResetPasswo public function assets() { return $this->morphMany('App\Models\Asset', 'assigned', 'assigned_type', 'assigned_to')->withTrashed(); - // return $this->hasMany('\App\Models\Asset', 'assigned_to')->withTrashed(); } /** @@ -342,24 +341,6 @@ class User extends SnipeModel implements AuthenticatableContract, CanResetPasswo return $query->whereNull('deleted_at'); } - /** - * Override the SentryUser getPersistCode method for - * multiple logins at one time - **/ - public function getPersistCode() - { - - if (!config('session.multi_login') || (!$this->persist_code)) { - $this->persist_code = $this->getRandomString(); - - // Our code got hashed - $persistCode = $this->persist_code; - $this->save(); - return $persistCode; - } - return $this->persist_code; - } - public function scopeMatchEmailOrUsername($query, $user_username, $user_email) { return $query->where('email', '=', $user_email) @@ -438,7 +419,7 @@ class User extends SnipeModel implements AuthenticatableContract, CanResetPasswo /** * Run additional, advanced searches. - * + * * @param Illuminate\Database\Eloquent\Builder $query * @param array $term The search terms * @return Illuminate\Database\Eloquent\Builder @@ -448,9 +429,9 @@ class User extends SnipeModel implements AuthenticatableContract, CanResetPasswo foreach($terms as $term) { $query = $query->orWhereRaw('CONCAT('.DB::getTablePrefix().'users.first_name," ",'.DB::getTablePrefix().'users.last_name) LIKE ?', ["%$term%", "%$term%"]); } - + return $query; - } + } public function scopeByGroup($query, $id) { diff --git a/database/factories/ModelFactory.php b/database/factories/ModelFactory.php index f50ac916b5..76dd0617b0 100644 --- a/database/factories/ModelFactory.php +++ b/database/factories/ModelFactory.php @@ -91,5 +91,6 @@ $factory->define(App\Models\Setting::class, function ($faker) { 'default_currency' => $faker->currencyCode, 'locale' => $faker->locale, 'pwd_secure_min' => 10, // Match web setup + 'email_domain' => 'test.com', ]; }); diff --git a/tests/unit/ImporterTest.php b/tests/unit/ImporterTest.php index f996503962..ae80239d0b 100644 --- a/tests/unit/ImporterTest.php +++ b/tests/unit/ImporterTest.php @@ -97,6 +97,7 @@ Mildred Gibson,mgibson2@wiley.com,mgibson2,,user,morbi quis tortor id,nunc nisl EOT; $this->import(new AssetImporter($csv)); + $user = User::where('username', 'bnelson0')->firstOrFail(); $this->tester->seeRecord('assets', [ @@ -278,7 +279,6 @@ EOT; $this->import(new AssetImporter($csv), $customFieldMap); // Did we create a user? - $this->tester->seeRecord('users', [ 'first_name' => 'Bonnie', 'last_name' => 'Nelson',