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

@@ -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;
     }
 
     /**

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);

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) {

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',
     ];
 });

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',