Merge pull request #65 from nafiesl/48_user_deletion_bug

Fix User Deletion Bug Related to their Couple Records

app/Couple.php

@@ -23,12 +23,12 @@ class Couple extends Model
 
     public function husband()
     {
-        return $this->belongsTo(User::class);
+        return $this->belongsTo(User::class)->withDefault(['name' => 'N/A']);
     }
 
     public function wife()
     {
-        return $this->belongsTo(User::class);
+        return $this->belongsTo(User::class)->withDefault(['name' => 'N/A']);
     }
 
     public function childs()

app/Http/Controllers/UsersController.php

@@ -2,12 +2,12 @@
 
 namespace App\Http\Controllers;
 
-use DB;
-use Storage;
-use App\User;
 use App\Couple;
-use Illuminate\Http\Request;
 use App\Http\Requests\Users\UpdateRequest;
+use App\Jobs\Users\DeleteAndReplaceUser;
+use App\User;
+use Illuminate\Http\Request;
+use Storage;
 
 class UsersController extends Controller
 {
@@ -146,11 +146,7 @@ class UsersController extends Controller
                 'replacement_user_id.required' => __('validation.user.replacement_user_id.required'),
             ]);
 
-            DB::beginTransaction();
-            $this->replaceUserOnUsersTable($user->id, $attributes['replacement_user_id']);
-            $this->replaceUserOnCouplesTable($user->id, $attributes['replacement_user_id']);
-            $user->delete();
-            DB::commit();
+            $this->dispatchNow(new DeleteAndReplaceUser($user, $attributes['replacement_user_id']));
 
             return redirect()->route('users.show', $attributes['replacement_user_id']);
         }
@@ -190,39 +186,6 @@ class UsersController extends Controller
     }
 
     /**
-     * Replace User Ids on users table.
-     *
-     * @param  string  $oldUserId
-     * @param  string  $replacementUserId
-     * @return void
-     */
-    private function replaceUserOnUsersTable($oldUserId, $replacementUserId)
-    {
-        foreach (['father_id', 'mother_id', 'manager_id'] as $field) {
-            DB::table('users')->where($field, $oldUserId)->update([
-                $field => $replacementUserId,
-            ]);
-        }
-    }
-
-    /**
-     * Replace User Ids on couples table.
-     *
-     * @param string $oldUserId
-     * @param string $replacementUserId
-     *
-     * @return void
-     */
-    private function replaceUserOnCouplesTable($oldUserId, $replacementUserId)
-    {
-        foreach (['husband_id', 'wife_id', 'manager_id'] as $field) {
-            DB::table('couples')->where($field, $oldUserId)->update([
-                $field => $replacementUserId,
-            ]);
-        }
-    }
-
-    /**
      * Get User list based on gender.
      *
      * @param int $genderId

app/Jobs/Users/DeleteAndReplaceUser.php

@@ -0,0 +1,95 @@
+<?php
+
+namespace App\Jobs\Users;
+
+use App\Couple;
+use App\User;
+use Illuminate\Support\Facades\DB;
+
+class DeleteAndReplaceUser
+{
+    public $user;
+
+    public $replacementUserId;
+
+    public function __construct(User $user, string $replacementUserId)
+    {
+        $this->user = $user;
+        $this->replacementUserId = $replacementUserId;
+    }
+
+    public function handle()
+    {
+        $user = $this->user;
+        $replacementUserId = $this->replacementUserId;
+
+        DB::beginTransaction();
+        $this->replaceUserOnUsersTable($user->id, $replacementUserId);
+        $this->removeDuplicatedCouples($user->id, $replacementUserId);
+        $this->replaceUserOnCouplesTable($user->id, $replacementUserId);
+        $user->delete();
+        DB::commit();
+    }
+
+    private function replaceUserOnCouplesTable($oldUserId, $replacementUserId)
+    {
+        DB::table('couples')->where('husband_id', $oldUserId)->update([
+            'husband_id' => $replacementUserId,
+        ]);
+        DB::table('couples')->where('wife_id', $oldUserId)->update([
+            'wife_id' => $replacementUserId,
+        ]);
+        DB::table('couples')->where('manager_id', $oldUserId)->update([
+            'manager_id' => $replacementUserId,
+        ]);
+    }
+
+    private function removeDuplicatedCouples(string $oldUserId, string $replacementUserId)
+    {
+        $oldUser = User::find($oldUserId);
+        $replacementUser = User::find($replacementUserId);
+
+        if ($replacementUser->gender_id == 1) {
+            $replacementUserCouples = Couple::where('husband_id', $replacementUserId)->get();
+        } else {
+            $replacementUserCouples = Couple::where('wife_id', $replacementUserId)->get();
+        }
+        if ($oldUser->gender_id == 1) {
+            $oldUserCouples = Couple::where('husband_id', $oldUserId)->get();
+        } else {
+            $oldUserCouples = Couple::where('wife_id', $oldUserId)->get();
+        }
+
+        $couplesArray = [];
+        foreach ($replacementUserCouples as $replacementUserCouple) {
+            $couplesArray[$replacementUserCouple->id] = $replacementUserCouple->husband_id.'_'.$replacementUserCouple->wife_id;
+        }
+        foreach ($oldUserCouples as $oldUserCouple) {
+            $couplesArray[$oldUserCouple->id] = $oldUserCouple->husband_id.'_'.$oldUserCouple->wife_id;
+        }
+        $couplesArray = collect($couplesArray);
+        $deletableCouples = [];
+        if ($oldUser->gender_id == 1) {
+            foreach ($oldUserCouples as $oldUserCouple) {
+                $deletableCouples[] = $couplesArray->search($replacementUserId.'_'.$oldUserCouple->wife_id);
+            }
+        } else {
+            foreach ($oldUserCouples as $oldUserCouple) {
+                $deletableCouples[] = $couplesArray->search($oldUserCouple->husband_id.'_'.$replacementUserId);
+            }
+        }
+
+        if ($deletableCouples) {
+            Couple::whereIn('id', $deletableCouples)->delete();
+        }
+    }
+
+    private function replaceUserOnUsersTable(string $oldUserId, string $replacementUserId)
+    {
+        foreach (['father_id', 'mother_id', 'manager_id'] as $field) {
+            DB::table('users')->where($field, $oldUserId)->update([
+                $field => $replacementUserId,
+            ]);
+        }
+    }
+}

tests/Feature/UsersDeletionTest.php

@@ -2,10 +2,10 @@
 
 namespace Tests\Feature;
 
-use App\User;
 use App\Couple;
-use Tests\TestCase;
+use App\User;
 use Illuminate\Foundation\Testing\RefreshDatabase;
+use Tests\TestCase;
 
 class UsersDeletionTest extends TestCase
 {
@@ -253,4 +253,79 @@ class UsersDeletionTest extends TestCase
         $this->seeElement('option', ['value' => $replacementMaleUser->id]);
         $this->dontSeeElement('option', ['value' => $femaleUser->id]);
     }
+
+    /** @test */
+    public function bugfix_handle_duplicated_couple_on_user_deletion()
+    {
+        $manager = $this->loginAsUser();
+        $singleWife = factory(User::class)->states('female')->create(['manager_id' => $manager->id]);
+        $oldUser = factory(User::class)->states('male')->create(['manager_id' => $manager->id]);
+        $replacementUser = factory(User::class)->states('male')->create(['manager_id' => $manager->id]);
+        $oldUserCouple = factory(Couple::class)->create([
+            'husband_id' => $oldUser->id,
+            'wife_id'    => $singleWife->id,
+        ]);
+        $duplicatedCouple = factory(Couple::class)->create([
+            'husband_id' => $replacementUser->id,
+            'wife_id'    => $singleWife->id,
+        ]);
+
+        $this->visit(route('users.edit', [$oldUser, 'action' => 'delete']));
+        $this->see(__('user.replace_delete_text'));
+
+        $this->submitForm(__('user.replace_delete_button'), [
+            'replacement_user_id' => $replacementUser->id,
+        ]);
+
+        $this->dontSeeInDatabase('users', ['id' => $oldUser->id]);
+
+        $this->dontSeeInDatabase('couples', ['husband_id' => $oldUser->id]);
+
+        $this->seeInDatabase('couples', [
+            'id'         => $oldUserCouple->id,
+            'husband_id' => $replacementUser->id,
+            'wife_id'    => $singleWife->id,
+        ]);
+    }
+
+    /** @test */
+    public function bugfix_handle_duplicated_couple_on_user_deletion_with_different_marriages()
+    {
+        $manager = $this->loginAsUser();
+        $oldUserWife = factory(User::class)->states('female')->create(['manager_id' => $manager->id]);
+        $replacementUserWife = factory(User::class)->states('female')->create(['manager_id' => $manager->id]);
+        $oldUser = factory(User::class)->states('male')->create(['manager_id' => $manager->id]);
+        $replacementUser = factory(User::class)->states('male')->create(['manager_id' => $manager->id]);
+        $oldUserCouple = factory(Couple::class)->create([
+            'husband_id' => $oldUser->id,
+            'wife_id'    => $oldUserWife->id,
+        ]);
+        $newUserCouple = factory(Couple::class)->create([
+            'husband_id' => $replacementUser->id,
+            'wife_id'    => $replacementUserWife->id,
+        ]);
+
+        $this->visit(route('users.edit', [$oldUser, 'action' => 'delete']));
+        $this->see(__('user.replace_delete_text'));
+
+        $this->submitForm(__('user.replace_delete_button'), [
+            'replacement_user_id' => $replacementUser->id,
+        ]);
+
+        $this->dontSeeInDatabase('users', ['id' => $oldUser->id]);
+
+        $this->dontSeeInDatabase('couples', ['husband_id' => $oldUser->id]);
+
+        $this->seeInDatabase('couples', [
+            'id'         => $oldUserCouple->id,
+            'husband_id' => $replacementUser->id,
+            'wife_id'    => $oldUserWife->id,
+        ]);
+
+        $this->seeInDatabase('couples', [
+            'id'         => $newUserCouple->id,
+            'husband_id' => $replacementUser->id,
+            'wife_id'    => $replacementUserWife->id,
+        ]);
+    }
 }

tests/Unit/CoupleTest.php

@@ -20,6 +20,16 @@ class CoupleTest extends TestCase
     }
 
     /** @test */
+    public function a_couples_husband_or_wife_has_a_default_name()
+    {
+        $couple = factory(Couple::class)->create();
+        $couple->husband->delete();
+        $couple->wife->delete();
+        $this->assertEquals($couple->fresh()->husband->name, 'N/A');
+        $this->assertEquals($couple->fresh()->wife->name, 'N/A');
+    }
+
+    /** @test */
     public function a_couple_can_have_many_childs()
     {
         $couple = factory(Couple::class)->create();