Avoid loss of all permissions/roles pivots on sync error

spatie · Mar 6, 2023 · e8de9c3 · e8de9c3
1 parent 490fa00
commit e8de9c3
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 8 deletions.
diff --git a/src/Traits/HasPermissions.php b/src/Traits/HasPermissions.php
@@ -345,7 +345,7 @@ public function getAllPermissions(): Collection
      * @param  string|int|array|\Spatie\Permission\Contracts\Permission|\Illuminate\Support\Collection  $permissions
      * @return array
      */
-    public function collectPermissions(...$permissions)
+    public function collectPermissions(...$permissions): array
     {
         return collect($permissions)
             ->flatten()
@@ -412,9 +412,11 @@ function ($object) use ($permissions, $model) {
      */
     public function syncPermissions(...$permissions)
     {
+        $this->collectPermissions(...$permissions);
+
         $this->permissions()->detach();
 
-        return $this->givePermissionTo($permissions);
+        return $this->givePermissionTo(...$permissions);
     }
 
     /**

diff --git a/src/Traits/HasRoles.php b/src/Traits/HasRoles.php
@@ -96,14 +96,13 @@ public function scopeRole(Builder $query, $roles, $guard = null): Builder
     }
 
     /**
-     * Assign the given role to the model.
+     * Returns roles ids as array keys
      *
-     * @param  array|string|int|\Spatie\Permission\Contracts\Role|\Illuminate\Support\Collection  ...$roles
-     * @return $this
+     * @param  array|string|int|\Spatie\Permission\Contracts\Role|\Illuminate\Support\Collection  $roles
      */
-    public function assignRole(...$roles)
+    private function collectRoles(...$roles): array
     {
-        $roles = collect($roles)
+        return collect($roles)
             ->flatten()
             ->reduce(function ($array, $role) {
                 if (empty($role)) {
@@ -122,6 +121,17 @@ public function assignRole(...$roles)
 
                 return $array;
             }, []);
+    }
+
+    /**
+     * Assign the given role to the model.
+     *
+     * @param  array|string|int|\Spatie\Permission\Contracts\Role|\Illuminate\Support\Collection  ...$roles
+     * @return $this
+     */
+    public function assignRole(...$roles)
+    {
+        $roles = $this->collectRoles(...$roles);
 
         $model = $this->getModel();
 
@@ -175,9 +185,11 @@ public function removeRole($role)
      */
     public function syncRoles(...$roles)
     {
+        $this->collectRoles(...$roles);
+
         $this->roles()->detach();
 
-        return $this->assignRole($roles);
+        return $this->assignRole(...$roles);
     }
 
     /**

diff --git a/tests/HasPermissionsTest.php b/tests/HasPermissionsTest.php
@@ -476,6 +476,20 @@ public function sync_permission_ignores_null_inputs()
         $this->assertFalse($this->testUser->hasDirectPermission('edit-news'));
     }
 
+    /** @test */
+    public function sync_permission_does_not_delete_permissions_on_error()
+    {
+        $this->testUser->givePermissionTo('edit-news');
+
+        $this->expectException(PermissionDoesNotExist::class);
+
+        try {
+            $this->testUser->syncPermissions('edit-news', 'edit-something-that-does-not-exist');
+        } finally {
+            $this->assertTrue($this->testUser->fresh()->hasDirectPermission('edit-news'));
+        }
+    }
+
     /** @test */
     public function it_does_not_remove_already_associated_permissions_when_assigning_new_permissions()
     {

diff --git a/tests/HasRolesTest.php b/tests/HasRolesTest.php
@@ -227,6 +227,20 @@ public function it_will_remove_all_roles_when_an_empty_array_is_passed_to_sync_r
         $this->assertFalse($this->testUser->hasRole('testRole2'));
     }
 
+    /** @test */
+    public function sync_permission_does_not_delete_roles_on_error()
+    {
+        $this->testUser->assignRole('testRole');
+
+        $this->expectException(RoleDoesNotExist::class);
+
+        try {
+            $this->testUser->syncRoles('testRole2', 'role-that-does-not-exist');
+        } finally {
+            $this->assertTrue($this->testUser->fresh()->hasRole('testRole'));
+        }
+    }
+
     /** @test */
     public function it_will_sync_roles_to_a_model_that_is_not_persisted()
     {