From 1fedc201762ae4a2a14cfdad9044d64184afa766 Mon Sep 17 00:00:00 2001 From: Nafies Luthfi Date: Sat, 11 Apr 2020 16:37:07 +0800 Subject: [PATCH 1/5] Prepare test for user deletion bug --- app/Http/Controllers/UsersController.php | 27 +++++++++++++++----- tests/Feature/UsersDeletionTest.php | 44 ++++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/app/Http/Controllers/UsersController.php b/app/Http/Controllers/UsersController.php index 5619a86..3f65816 100644 --- a/app/Http/Controllers/UsersController.php +++ b/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\User; +use DB; +use Illuminate\Http\Request; +use Storage; class UsersController extends Controller { @@ -216,9 +216,22 @@ class UsersController extends Controller private function replaceUserOnCouplesTable($oldUserId, $replacementUserId) { foreach (['husband_id', 'wife_id', 'manager_id'] as $field) { - DB::table('couples')->where($field, $oldUserId)->update([ - $field => $replacementUserId, - ]); + if (in_array($field, ['husband_id', 'wife_id'])) { + $replacementUserCouples = Couple::where('husband_id', $replacementUserId) + ->orWhere('wive_id', $replacementUserId) + ->get(); + if ($replacementUserCouples->isEmpty()) { + DB::table('couples')->where($field, $oldUserId)->update([ + $field => $replacementUserId, + ]); + } else { + $replacementUserCouples->first()->delete(); + } + } else { + DB::table('couples')->where($field, $oldUserId)->update([ + $field => $replacementUserId, + ]); + } } } diff --git a/tests/Feature/UsersDeletionTest.php b/tests/Feature/UsersDeletionTest.php index bb2fe42..6046762 100644 --- a/tests/Feature/UsersDeletionTest.php +++ b/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,44 @@ 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(); + $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' => '999999', + ]); + $duplicatedCouple = factory(Couple::class)->create([ + 'husband_id' => $replacementUser->id, + 'wife_id' => '999999', + ]); + + $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, + ]); + } } From bc6a04a6f67e36193c023813b3f7de602f787c94 Mon Sep 17 00:00:00 2001 From: Nafies Luthfi Date: Sat, 19 Dec 2020 23:44:18 +0800 Subject: [PATCH 2/5] Replace user ID on couples table on user deletion --- app/Http/Controllers/UsersController.php | 32 +++++++++++++++----------------- tests/Feature/UsersDeletionTest.php | 22 ++++++++-------------- 2 files changed, 23 insertions(+), 31 deletions(-) diff --git a/app/Http/Controllers/UsersController.php b/app/Http/Controllers/UsersController.php index 3f65816..5f76f13 100644 --- a/app/Http/Controllers/UsersController.php +++ b/app/Http/Controllers/UsersController.php @@ -215,24 +215,22 @@ class UsersController extends Controller */ private function replaceUserOnCouplesTable($oldUserId, $replacementUserId) { - foreach (['husband_id', 'wife_id', 'manager_id'] as $field) { - if (in_array($field, ['husband_id', 'wife_id'])) { - $replacementUserCouples = Couple::where('husband_id', $replacementUserId) - ->orWhere('wive_id', $replacementUserId) - ->get(); - if ($replacementUserCouples->isEmpty()) { - DB::table('couples')->where($field, $oldUserId)->update([ - $field => $replacementUserId, - ]); - } else { - $replacementUserCouples->first()->delete(); - } - } else { - DB::table('couples')->where($field, $oldUserId)->update([ - $field => $replacementUserId, - ]); - } + $replacementUserCouples = Couple::where('husband_id', $replacementUserId) + ->orWhere('wife_id', $replacementUserId) + ->orWhere('manager_id', $replacementUserId) + ->get(); + if (!$replacementUserCouples->isEmpty()) { + $replacementUserCouples->each->delete(); } + 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, + ]); } /** diff --git a/tests/Feature/UsersDeletionTest.php b/tests/Feature/UsersDeletionTest.php index 6046762..bc7b591 100644 --- a/tests/Feature/UsersDeletionTest.php +++ b/tests/Feature/UsersDeletionTest.php @@ -258,19 +258,16 @@ class UsersDeletionTest extends TestCase public function bugfix_handle_duplicated_couple_on_user_deletion() { $manager = $this->loginAsUser(); - $oldUser = factory(User::class)->states('male')->create([ - 'manager_id' => $manager->id, - ]); - $replacementUser = factory(User::class)->states('male')->create([ - 'manager_id' => $manager->id, - ]); + $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' => '999999', + 'wife_id' => $singleWife->id, ]); $duplicatedCouple = factory(Couple::class)->create([ 'husband_id' => $replacementUser->id, - 'wife_id' => '999999', + 'wife_id' => $singleWife->id, ]); $this->visit(route('users.edit', [$oldUser, 'action' => 'delete'])); @@ -280,17 +277,14 @@ class UsersDeletionTest extends TestCase 'replacement_user_id' => $replacementUser->id, ]); - $this->dontSeeInDatabase('users', [ - 'id' => $oldUser->id, - ]); + $this->dontSeeInDatabase('users', ['id' => $oldUser->id]); - $this->dontSeeInDatabase('couples', [ - 'husband_id' => $oldUser->id, - ]); + $this->dontSeeInDatabase('couples', ['husband_id' => $oldUser->id]); $this->seeInDatabase('couples', [ 'id' => $oldUserCouple->id, 'husband_id' => $replacementUser->id, + 'wife_id' => $singleWife->id, ]); } } From 44033973408193b716e9cc6c9d930058776b196c Mon Sep 17 00:00:00 2001 From: Nafies Luthfi Date: Sun, 20 Dec 2020 00:26:57 +0800 Subject: [PATCH 3/5] Prevent existing replacement user couples from deletion --- app/Http/Controllers/UsersController.php | 48 +++++++++++++++++++++++++++----- tests/Feature/UsersDeletionTest.php | 41 +++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/app/Http/Controllers/UsersController.php b/app/Http/Controllers/UsersController.php index 5f76f13..5043e0c 100644 --- a/app/Http/Controllers/UsersController.php +++ b/app/Http/Controllers/UsersController.php @@ -148,6 +148,7 @@ class UsersController extends Controller DB::beginTransaction(); $this->replaceUserOnUsersTable($user->id, $attributes['replacement_user_id']); + $this->removeDuplicatedCouples($user->id, $attributes['replacement_user_id']); $this->replaceUserOnCouplesTable($user->id, $attributes['replacement_user_id']); $user->delete(); DB::commit(); @@ -215,13 +216,6 @@ class UsersController extends Controller */ private function replaceUserOnCouplesTable($oldUserId, $replacementUserId) { - $replacementUserCouples = Couple::where('husband_id', $replacementUserId) - ->orWhere('wife_id', $replacementUserId) - ->orWhere('manager_id', $replacementUserId) - ->get(); - if (!$replacementUserCouples->isEmpty()) { - $replacementUserCouples->each->delete(); - } DB::table('couples')->where('husband_id', $oldUserId)->update([ 'husband_id' => $replacementUserId, ]); @@ -233,6 +227,46 @@ class UsersController extends Controller ]); } + private function removeDuplicatedCouples($oldUserId, $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(); + } + } + /** * Get User list based on gender. * diff --git a/tests/Feature/UsersDeletionTest.php b/tests/Feature/UsersDeletionTest.php index bc7b591..57f9344 100644 --- a/tests/Feature/UsersDeletionTest.php +++ b/tests/Feature/UsersDeletionTest.php @@ -287,4 +287,45 @@ class UsersDeletionTest extends TestCase '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, + ]); + } } From 329f955780fd308d13956e352ac921720bc63264 Mon Sep 17 00:00:00 2001 From: Nafies Luthfi Date: Mon, 21 Dec 2020 17:17:23 +0800 Subject: [PATCH 4/5] Refactor user replace and delete to a job --- app/Http/Controllers/UsersController.php | 86 +---------------------------- app/Jobs/Users/DeleteAndReplaceUser.php | 95 ++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 84 deletions(-) create mode 100644 app/Jobs/Users/DeleteAndReplaceUser.php diff --git a/app/Http/Controllers/UsersController.php b/app/Http/Controllers/UsersController.php index 5043e0c..2b2df4e 100644 --- a/app/Http/Controllers/UsersController.php +++ b/app/Http/Controllers/UsersController.php @@ -4,8 +4,8 @@ namespace App\Http\Controllers; use App\Couple; use App\Http\Requests\Users\UpdateRequest; +use App\Jobs\Users\DeleteAndReplaceUser; use App\User; -use DB; use Illuminate\Http\Request; use Storage; @@ -146,12 +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->removeDuplicatedCouples($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']); } @@ -191,83 +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) - { - 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($oldUserId, $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(); - } - } - - /** * Get User list based on gender. * * @param int $genderId diff --git a/app/Jobs/Users/DeleteAndReplaceUser.php b/app/Jobs/Users/DeleteAndReplaceUser.php new file mode 100644 index 0000000..0f775b6 --- /dev/null +++ b/app/Jobs/Users/DeleteAndReplaceUser.php @@ -0,0 +1,95 @@ +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, + ]); + } + } +} From f58d49066f0c8734611135a3f33f6c2a86777dd1 Mon Sep 17 00:00:00 2001 From: Nafies Luthfi Date: Mon, 21 Dec 2020 17:31:51 +0800 Subject: [PATCH 5/5] Resolve error exception if a couple husband or wife was deleted --- app/Couple.php | 4 ++-- tests/Unit/CoupleTest.php | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/Couple.php b/app/Couple.php index e20b942..0a2f40a 100644 --- a/app/Couple.php +++ b/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() diff --git a/tests/Unit/CoupleTest.php b/tests/Unit/CoupleTest.php index d8d1967..577ad32 100644 --- a/tests/Unit/CoupleTest.php +++ b/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();