From f344764f52f0aabf58ea7b446fab3ed48c65349d Mon Sep 17 00:00:00 2001 From: Chirag Chhatrala <60499540+chiragchhatrala@users.noreply.github.com> Date: Mon, 28 Apr 2025 21:33:38 +0530 Subject: [PATCH] =?UTF-8?q?Implement=20EnsureUserHasWorkspace=20Trait=20an?= =?UTF-8?q?d=20Integrate=20into=20Controllers=E2=80=A6=20(#741)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Implement EnsureUserHasWorkspace Trait and Integrate into Controllers and Jobs - Introduced the EnsureUserHasWorkspace trait to ensure users have at least one workspace when they are detached from a workspace. - Integrated the trait into WorkspaceUserController to enforce workspace checks during user detachment. - Updated RemoveWorkspaceGuests job to utilize the new trait for ensuring users have a workspace after detachment. - Modified UserWorkspace model to call the ensureUserHasWorkspace method upon deletion, maintaining workspace integrity. These changes enhance user management by ensuring that users always have a workspace, improving overall application stability. * Add test --------- Co-authored-by: Julien Nahum --- .../Controllers/WorkspaceUserController.php | 9 +- .../Jobs/Billing/RemoveWorkspaceGuests.php | 6 +- api/app/Models/UserWorkspace.php | 12 + api/app/Traits/EnsureUserHasWorkspace.php | 34 +++ .../Feature/EnsureUserHasWorkspaceTest.php | 211 ++++++++++++++++++ 5 files changed, 270 insertions(+), 2 deletions(-) create mode 100644 api/app/Traits/EnsureUserHasWorkspace.php create mode 100644 api/tests/Feature/EnsureUserHasWorkspaceTest.php diff --git a/api/app/Http/Controllers/WorkspaceUserController.php b/api/app/Http/Controllers/WorkspaceUserController.php index f58269bb..690d38e7 100644 --- a/api/app/Http/Controllers/WorkspaceUserController.php +++ b/api/app/Http/Controllers/WorkspaceUserController.php @@ -4,6 +4,7 @@ namespace App\Http\Controllers; use App\Jobs\Billing\WorkspaceUsersUpdated; use App\Models\UserInvite; +use App\Traits\EnsureUserHasWorkspace; use Illuminate\Http\Request; use App\Models\Workspace; use App\Models\User; @@ -11,6 +12,8 @@ use App\Service\WorkspaceHelper; class WorkspaceUserController extends Controller { + use EnsureUserHasWorkspace; + public function __construct() { $this->middleware('auth'); @@ -106,7 +109,9 @@ class WorkspaceUserController extends Controller $workspace = Workspace::findOrFail($workspaceId); $this->authorize('adminAction', $workspace); + $user = User::findOrFail($userId); $workspace->users()->detach($userId); + $this->ensureUserHasWorkspace($user); WorkspaceUsersUpdated::dispatch($workspace); return $this->success([ @@ -119,7 +124,9 @@ class WorkspaceUserController extends Controller $workspace = Workspace::findOrFail($workspaceId); $this->authorize('view', $workspace); - $workspace->users()->detach($request->user()->id); + $user = $request->user(); + $workspace->users()->detach($user->id); + $this->ensureUserHasWorkspace($user); return $this->success([ 'message' => 'You have left the workspace successfully.' diff --git a/api/app/Jobs/Billing/RemoveWorkspaceGuests.php b/api/app/Jobs/Billing/RemoveWorkspaceGuests.php index f09dad27..8bd2df8e 100644 --- a/api/app/Jobs/Billing/RemoveWorkspaceGuests.php +++ b/api/app/Jobs/Billing/RemoveWorkspaceGuests.php @@ -4,11 +4,13 @@ namespace App\Jobs\Billing; use App\Models\User; use App\Models\Workspace; +use App\Traits\EnsureUserHasWorkspace; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\Log; class RemoveWorkspaceGuests implements ShouldQueue { @@ -16,6 +18,7 @@ class RemoveWorkspaceGuests implements ShouldQueue use InteractsWithQueue; use Queueable; use SerializesModels; + use EnsureUserHasWorkspace; /** * Create a new job instance. @@ -50,13 +53,14 @@ class RemoveWorkspaceGuests implements ShouldQueue // Detach all users from the workspace (except the owner) foreach ($workspace->users()->where('users.id', '!=', $this->user->id)->get() as $user) { - \Log::info('Detaching user from workspace', [ + Log::info('Detaching user from workspace', [ 'workspace_id' => $workspace->id, 'workspace_name' => $workspace->name, 'user_id' => $user->id, 'user_email' => $user->email, ]); $workspace->users()->detach($user); + $this->ensureUserHasWorkspace($user); } }); } diff --git a/api/app/Models/UserWorkspace.php b/api/app/Models/UserWorkspace.php index de9dfc0f..c2fd71d3 100644 --- a/api/app/Models/UserWorkspace.php +++ b/api/app/Models/UserWorkspace.php @@ -2,12 +2,15 @@ namespace App\Models; +use App\Traits\EnsureUserHasWorkspace; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; class UserWorkspace extends Model { use HasFactory; + use EnsureUserHasWorkspace; + protected $table = 'user_workspace'; protected $fillable = [ @@ -25,4 +28,13 @@ class UserWorkspace extends Model { return $this->belongsTo(Workspace::class); } + + protected static function boot() + { + parent::boot(); + + static::deleted(function (UserWorkspace $userWorkspace) { + $userWorkspace->ensureUserHasWorkspace($userWorkspace->user); + }); + } } diff --git a/api/app/Traits/EnsureUserHasWorkspace.php b/api/app/Traits/EnsureUserHasWorkspace.php new file mode 100644 index 00000000..32f90772 --- /dev/null +++ b/api/app/Traits/EnsureUserHasWorkspace.php @@ -0,0 +1,34 @@ +workspaces()->count() === 0) { + Log::info('Creating new workspace for user with no workspaces', [ + 'user_id' => $user->id, + 'user_email' => $user->email, + ]); + + $newWorkspace = Workspace::create([ + 'name' => 'My Workspace', + ]); + + $user->workspaces()->sync([ + $newWorkspace->id => [ + 'role' => 'admin', + ], + ], false); + } + } +} diff --git a/api/tests/Feature/EnsureUserHasWorkspaceTest.php b/api/tests/Feature/EnsureUserHasWorkspaceTest.php new file mode 100644 index 00000000..2267be88 --- /dev/null +++ b/api/tests/Feature/EnsureUserHasWorkspaceTest.php @@ -0,0 +1,211 @@ +create(); + $workspace = Workspace::factory()->create(); + $userToDetach = User::factory()->create(); + $workspace->users()->attach($admin, ['role' => 'admin']); + $workspace->users()->attach($userToDetach, ['role' => 'member']); + + expect($userToDetach->workspaces()->count())->toBe(1); + + // Act: Admin detaches the user + $this->actingAs($admin) + ->deleteJson(route('open.workspaces.users.remove', ['workspaceId' => $workspace->id, 'userId' => $userToDetach->id])) + ->assertOk(); + + // Assert: User now has 1 workspace, named 'My Workspace' + $userToDetach->refresh(); + expect($userToDetach->workspaces()->count())->toBe(1); + + // Check workspace name + $newWorkspace = $userToDetach->workspaces()->first(); + expect($newWorkspace->name)->toBe('My Workspace'); + + // Check role by querying the pivot table directly + $pivotData = DB::table('user_workspace') + ->where('user_id', $userToDetach->id) + ->where('workspace_id', $newWorkspace->id) + ->first(); + expect($pivotData->role)->toBe('admin'); +}); + +test('detaching user from one workspace when they have others does not create default one via controller', function () { + // Arrange: User with two workspaces, acting user is admin + $admin = User::factory()->create(); + $workspace1 = Workspace::factory()->create(); + $workspace2 = Workspace::factory()->create(); + $userToDetach = User::factory()->create(); + + $workspace1->users()->attach($admin, ['role' => 'admin']); + $workspace1->users()->attach($userToDetach, ['role' => 'member']); + $workspace2->users()->attach($userToDetach, ['role' => 'member']); // User belongs to a second workspace + + expect($userToDetach->workspaces()->count())->toBe(2); + $initialWorkspaceIds = $userToDetach->workspaces()->pluck('workspaces.id')->toArray(); + + // Act: Admin detaches the user from workspace1 + $this->actingAs($admin) + ->deleteJson(route('open.workspaces.users.remove', ['workspaceId' => $workspace1->id, 'userId' => $userToDetach->id])) + ->assertOk(); + + // Assert: User now has 1 workspace (workspace2), no default created + $userToDetach->refresh(); + expect($userToDetach->workspaces()->count())->toBe(1); + $remainingWorkspace = $userToDetach->workspaces()->first(); + expect($remainingWorkspace->id)->toBe($workspace2->id); + expect($userToDetach->workspaces()->where('name', 'My Workspace')->exists())->toBeFalse(); + expect($userToDetach->workspaces()->pluck('workspaces.id')->toArray())->not->toContain($workspace1->id); +}); + +test('user leaving last workspace creates a default one', function () { + // Arrange: User with one workspace + $workspace = Workspace::factory()->create(); + $user = User::factory()->create(); + $workspace->users()->attach($user, ['role' => 'member']); // Can be any role to leave + + expect($user->workspaces()->count())->toBe(1); + + // Act: User leaves the workspace + $this->actingAs($user) + ->postJson(route('open.workspaces.leave', ['workspaceId' => $workspace->id])) + ->assertOk(); + + // Assert: User now has 1 workspace, named 'My Workspace' + $user->refresh(); + expect($user->workspaces()->count())->toBe(1); + + // Check workspace name + $newWorkspace = $user->workspaces()->first(); + expect($newWorkspace->name)->toBe('My Workspace'); + + // Check role by querying the pivot table directly + $pivotData = DB::table('user_workspace') + ->where('user_id', $user->id) + ->where('workspace_id', $newWorkspace->id) + ->first(); + expect($pivotData->role)->toBe('admin'); +}); + +test('user leaving one workspace when they have others does not create default one', function () { + // Arrange: User with two workspaces + $workspace1 = Workspace::factory()->create(); + $workspace2 = Workspace::factory()->create(); + $user = User::factory()->create(); + + $workspace1->users()->attach($user, ['role' => 'member']); + $workspace2->users()->attach($user, ['role' => 'member']); + + expect($user->workspaces()->count())->toBe(2); + + // Act: User leaves workspace1 + $this->actingAs($user) + ->postJson(route('open.workspaces.leave', ['workspaceId' => $workspace1->id])) + ->assertOk(); + + // Assert: User now has 1 workspace (workspace2), no default created + $user->refresh(); + expect($user->workspaces()->count())->toBe(1); + $remainingWorkspace = $user->workspaces()->first(); + expect($remainingWorkspace->id)->toBe($workspace2->id); + expect($user->workspaces()->where('name', 'My Workspace')->exists())->toBeFalse(); +}); + +// Tests for RemoveWorkspaceGuests job +test('removing guest from last workspace via job creates a default one', function () { + // Arrange: Guest user with one workspace (the one they'll be removed from), Admin user + $admin = User::factory()->create(); + $workspace = Workspace::factory()->create(); + $guest = User::factory()->create(); + $workspace->users()->attach($admin, ['role' => 'admin']); + $workspace->users()->attach($guest, ['role' => 'guest']); + + expect($guest->workspaces()->count())->toBe(1); + + // Act: Dispatch the job to remove guests from the workspace + // Pass the admin user as required by the job constructor + RemoveWorkspaceGuests::dispatchSync($admin, $workspace); // Fixed: Job expects User first, then additional param + + // Assert: Guest user now has 1 workspace, named 'My Workspace' + $guest->refresh(); + expect($guest->workspaces()->count())->toBe(1); + + // Check workspace name + $newWorkspace = $guest->workspaces()->first(); + expect($newWorkspace->name)->toBe('My Workspace'); + + // Check role by querying the pivot table directly + $pivotData = DB::table('user_workspace') + ->where('user_id', $guest->id) + ->where('workspace_id', $newWorkspace->id) + ->first(); + expect($pivotData->role)->toBe('admin'); +}); + +test('removing guest from one workspace when they have others via job does not create default one', function () { + // Arrange: Guest user with two workspaces, Admin user + $admin = User::factory()->create(); + $workspaceToRemoveFrom = Workspace::factory()->create(); + $otherWorkspace = Workspace::factory()->create(); + $guest = User::factory()->create(); + + $workspaceToRemoveFrom->users()->attach($admin, ['role' => 'admin']); + $workspaceToRemoveFrom->users()->attach($guest, ['role' => 'guest']); + $otherWorkspace->users()->attach($guest, ['role' => 'member']); // Belongs to another workspace + + expect($guest->workspaces()->count())->toBe(2); + + // Act: Dispatch the job to remove guests from the first workspace + // Pass the admin user as required by the job constructor + RemoveWorkspaceGuests::dispatchSync($admin, $workspaceToRemoveFrom); // Fixed: Job expects User first, then additional param + + // Assert: Guest user now has 1 workspace (the other one), no default created + $guest->refresh(); + expect($guest->workspaces()->count())->toBe(1); + $remainingWorkspace = $guest->workspaces()->first(); + expect($remainingWorkspace->id)->toBe($otherWorkspace->id); + expect($guest->workspaces()->where('name', 'My Workspace')->exists())->toBeFalse(); +}); + +test('ensures user with no workspaces gets a default one', function () { + // Arrange: Create user without any workspaces + $user = User::factory()->create(); + expect($user->workspaces()->count())->toBe(0); + + // Create a controller that uses the trait + $controller = new class () { + use \App\Traits\EnsureUserHasWorkspace; + + public function ensureWorkspace(User $user) + { + $this->ensureUserHasWorkspace($user); + } + }; + + // Act: Call the trait method + $controller->ensureWorkspace($user); + + // Assert: User now has a workspace + $user->refresh(); + expect($user->workspaces()->count())->toBe(1); + + // Check workspace properties + $newWorkspace = $user->workspaces()->first(); + expect($newWorkspace->name)->toBe('My Workspace'); + + // Check role + $pivotData = DB::table('user_workspace') + ->where('user_id', $user->id) + ->where('workspace_id', $newWorkspace->id) + ->first(); + expect($pivotData->role)->toBe('admin'); +});