Delete users without PIREPS; pilot leave fix (#1171)

* Delete users without PIREPS; pilot leave fix

* Style fixes

app/Models/User.php

@@ -3,7 +3,6 @@
 namespace App\Models;
 
 use App\Models\Enums\JournalType;
-use App\Models\Enums\PirepState;
 use App\Models\Traits\JournalTrait;
 use Illuminate\Foundation\Auth\User as Authenticatable;
 use Illuminate\Notifications\Notifiable;
@@ -35,6 +34,7 @@ use Laratrust\Traits\LaratrustUserTrait;
  * @property int              rank_id
  * @property int              state
  * @property bool             opt_in
+ * @property Pirep[]          pireps
  * @property string           last_pirep_id
  * @property Pirep            last_pirep
  * @property UserFieldValue[] fields
@@ -251,8 +251,7 @@ class User extends Authenticatable
 
     public function pireps()
     {
-        return $this->hasMany(Pirep::class, 'user_id')
+        return $this->hasMany(Pirep::class, 'user_id');
-            ->where('state', '!=', PirepState::CANCELLED);
     }
 
     public function rank()

app/Services/UserService.php

@@ -139,13 +139,6 @@ class UserService extends Service
      */
     public function removeUser(User $user)
     {
-        $user->name = 'Deleted User';
-        $user->email = Utils::generateApiKey().'@deleted-user.com';
-        $user->api_key = Utils::generateApiKey();
-        $user->password = Hash::make(Utils::generateApiKey());
-        $user->state = UserState::DELETED;
-        $user->save();
-
         // Detach all roles from this user
         $user->detachRoles($user->roles);
 
@@ -154,6 +147,18 @@ class UserService extends Service
 
         // Remove any bids
         Bid::where('user_id', $user->id)->delete();
+
+        // If this user has PIREPs, do a soft delete. Otherwise, just delete them outright
+        if ($user->pireps->count() > 0) {
+            $user->name = 'Deleted User';
+            $user->email = Utils::generateApiKey().'@deleted-user.com';
+            $user->api_key = Utils::generateApiKey();
+            $user->password = Hash::make(Utils::generateApiKey());
+            $user->state = UserState::DELETED;
+            $user->save();
+        } else {
+            $user->delete();
+        }
     }
 
     /**
@@ -299,49 +304,41 @@ class UserService extends Service
      * currently active users. If the user doesn't have a PIREP, then the creation date
      * of the user record is used to determine the difference
      */
-    public function findUsersOnLeave(): array
+    public function findUsersOnLeave()
     {
         $leave_days = setting('pilots.auto_leave_days');
         if ($leave_days === 0) {
             return [];
         }
 
-        $return_users = [];
-
         $date = Carbon::now('UTC');
-        $users = User::with(['last_pirep'])->where('state', UserState::ACTIVE)->get();
+        $users = User::where('state', UserState::ACTIVE)->get();
 
         /** @var User $user */
-        foreach ($users as $user) {
+        return $users->filter(function ($user, $i) use ($date, $leave_days) {
-            // If they haven't submitted a PIREP, use the date that the user was created
-            if (!$user->last_pirep) {
-                $diff_date = $user->created_at;
-            } else {
-                $diff_date = $user->last_pirep->submitted_at;
-            }
-
-            // See if the difference is larger than what the setting calls for
-            if ($date->diffInDays($diff_date) <= $leave_days) {
-                continue;
-            }
-
-            $skip = false;
             // If any role for this user has the "disable_activity_check" feature activated, skip this user
             foreach ($user->roles()->get() as $role) {
                 /** @var Role $role */
                 if ($role->disable_activity_checks) {
-                    $skip = true;
+                    return false;
-                    break;
                 }
             }
 
-            if ($skip) {
+            // If they haven't submitted a PIREP, use the date that the user was created
-                continue;
+            $last_pirep = Pirep::where(['user_id' => $user->id])->latest('submitted_at')->first();
+            if (!$last_pirep) {
+                $diff_date = $user->created_at;
+            } else {
+                $diff_date = $last_pirep->created_at;
             }
-            $return_users[] = $user;
-        }
 
-        return $return_users;
+            // See if the difference is larger than what the setting calls for
+            if ($date->diffInDays($diff_date) <= $leave_days) {
+                return false;
+            }
+
+            return true;
+        });
     }
 
     /**

tests/TestData.php

@@ -47,12 +47,15 @@ trait TestData
     {
         $subfleet = $this->createSubfleetWithAircraft(2);
         $rank = $this->createRank(10, [$subfleet['subfleet']->id]);
-        $this->user = factory(\App\Models\User::class)->create(array_merge([
+
+        /** @var User user */
+        $this->user = factory(User::class)->create(array_merge([
             'rank_id' => $rank->id,
         ], $user_attrs));
 
         // Return a Pirep model
         return factory(Pirep::class)->make(array_merge([
+            'user_id'     => $this->user->id,
             'aircraft_id' => $subfleet['aircraft']->random()->id,
         ], $pirep_attrs));
     }

tests/UserTest.php

@@ -328,6 +328,33 @@ class UserTest extends TestCase
         $response = $this->get('/api/user/'.$user->id, [], $admin_user);
         $response->assertStatus(404);
 
+        // Get from the DB
+        $user = User::find($user->id);
+        $this->assertNull($user);
+    }
+
+    public function testUserPilotDeletedWithPireps()
+    {
+        $new_user = factory(User::class)->make()->toArray();
+        $new_user['password'] = Hash::make('secret');
+        $admin_user = $this->userSvc->createUser($new_user);
+
+        $new_user = factory(User::class)->make()->toArray();
+        $new_user['password'] = Hash::make('secret');
+        $user = $this->userSvc->createUser($new_user);
+        $this->assertEquals($user->id, $user->pilot_id);
+
+        /** @var Pirep $pirep */
+        factory(Pirep::class)->create([
+            'user_id' => $user->id,
+        ]);
+
+        // Delete the user
+        $this->userSvc->removeUser($user);
+
+        $response = $this->get('/api/user/'.$user->id, [], $admin_user);
+        $response->assertStatus(404);
+
         // Get from the DB
         $user = User::find($user->id);
         $this->assertEquals('Deleted User', $user->name);
@@ -359,53 +386,64 @@ class UserTest extends TestCase
         $this->createUser(['status' => UserState::ACTIVE]);
 
         $users_on_leave = $this->userSvc->findUsersOnLeave();
-        $this->assertEquals(0, count($users_on_leave));
+        $this->assertCount(0, $users_on_leave);
 
         $this->updateSetting('pilots.auto_leave_days', 1);
         $user = $this->createUser([
+            'state'      => UserState::ACTIVE,
             'status'     => UserState::ACTIVE,
             'created_at' => Carbon::now('UTC')->subDays(5),
         ]);
 
         $users_on_leave = $this->userSvc->findUsersOnLeave();
-        $this->assertEquals(1, count($users_on_leave));
+        $this->assertCount(1, $users_on_leave);
-        $this->assertEquals($user->id, $users_on_leave[0]->id);
+        $this->assertEquals($user->id, $users_on_leave->first()->id);
 
         // Give that user a new PIREP, still old
-        /** @var \App\Models\Pirep $pirep */
+        /** @var Pirep $pirep */
-        $pirep = factory(Pirep::class)->create(['submitted_at' => Carbon::now('UTC')->subDays(5)]);
+        $pirep = factory(Pirep::class)->create([
+            'user_id'      => $user->id,
+            'created_at'   => Carbon::now('UTC')->subDays(5),
+            'submitted_at' => Carbon::now('UTC')->subDays(5),
+        ]);
 
         $user->last_pirep_id = $pirep->id;
         $user->save();
         $user->refresh();
 
         $users_on_leave = $this->userSvc->findUsersOnLeave();
-        $this->assertEquals(1, count($users_on_leave));
+        $this->assertCount(1, $users_on_leave);
-        $this->assertEquals($user->id, $users_on_leave[0]->id);
+        $this->assertEquals($user->id, $users_on_leave->first()->id);
 
         // Create a new PIREP
-        /** @var \App\Models\Pirep $pirep */
+        /** @var Pirep $pirep */
-        $pirep = factory(Pirep::class)->create(['submitted_at' => Carbon::now('UTC')]);
+        $pirep = factory(Pirep::class)->create([
+            'user_id'      => $user->id,
+            'created_at'   => Carbon::now('UTC'),
+            'submitted_at' => Carbon::now('UTC'),
+        ]);
 
         $user->last_pirep_id = $pirep->id;
         $user->save();
         $user->refresh();
 
         $users_on_leave = $this->userSvc->findUsersOnLeave();
-        $this->assertEquals(0, count($users_on_leave));
+        $this->assertCount(0, $users_on_leave);
 
         // Check disable_activity_checks
         $user = $this->createUser([
             'status'     => UserState::ACTIVE,
             'created_at' => Carbon::now('UTC')->subDays(5),
         ]);
+
         $role = $this->createRole([
             'disable_activity_checks' => true,
         ]);
+
         $user->attachRole($role);
         $user->save();
 
         $users_on_leave = $this->userSvc->findUsersOnLeave();
-        $this->assertEquals(0, count($users_on_leave));
+        $this->assertCount(0, $users_on_leave);
     }
 }