Member update endpoint can now change ownership

This commit is contained in:
Constantin Graf
2024-07-01 16:56:18 +02:00
committed by Gregor Vostrak
parent 264b7c9b8d
commit 3a17ae83ae
16 changed files with 352 additions and 85 deletions

View File

@@ -27,7 +27,7 @@ class UpdateMemberRole
*/
public function update(User $actingUser, Organization $organization, string $userId, string $role): void
{
if (! app(PermissionStore::class)->has($organization, 'members:change-role')) {
if (! app(PermissionStore::class)->has($organization, 'members:change-ownership')) {
throw new AuthorizationException();
}

View File

@@ -11,5 +11,4 @@ enum Role: string
case Manager = 'manager';
case Employee = 'employee';
case Placeholder = 'placeholder';
}

View File

@@ -0,0 +1,10 @@
<?php
declare(strict_types=1);
namespace App\Exceptions\Api;
class ChangingRoleToPlaceholderIsNotAllowed extends ApiException
{
public const string KEY = 'changing_role_to_placeholder_is_not_allowed';
}

View File

@@ -0,0 +1,10 @@
<?php
declare(strict_types=1);
namespace App\Exceptions\Api;
class OnlyOwnerCanChangeOwnership extends ApiException
{
public const string KEY = 'only_owner_can_change_ownership';
}

View File

@@ -0,0 +1,10 @@
<?php
declare(strict_types=1);
namespace App\Exceptions\Api;
class OrganizationNeedsAtLeastOneOwner extends ApiException
{
public const string KEY = 'organization_needs_at_least_one_owner';
}

View File

@@ -6,7 +6,10 @@ namespace App\Http\Controllers\Api\V1;
use App\Enums\Role;
use App\Exceptions\Api\CanNotRemoveOwnerFromOrganization;
use App\Exceptions\Api\ChangingRoleToPlaceholderIsNotAllowed;
use App\Exceptions\Api\EntityStillInUseApiException;
use App\Exceptions\Api\OnlyOwnerCanChangeOwnership;
use App\Exceptions\Api\OrganizationNeedsAtLeastOneOwner;
use App\Exceptions\Api\UserNotPlaceholderApiException;
use App\Http\Requests\V1\Member\MemberIndexRequest;
use App\Http\Requests\V1\Member\MemberUpdateRequest;
@@ -18,6 +21,7 @@ use App\Models\Organization;
use App\Models\ProjectMember;
use App\Models\TimeEntry;
use App\Service\BillableRateService;
use App\Service\MemberService;
use Illuminate\Auth\Access\AuthorizationException;
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Request;
@@ -57,18 +61,37 @@ class MemberController extends Controller
* Update a member of the organization
*
* @throws AuthorizationException
* @throws OrganizationNeedsAtLeastOneOwner
* @throws OnlyOwnerCanChangeOwnership
* @throws ChangingRoleToPlaceholderIsNotAllowed
*
* @operationId updateMember
*/
public function update(Organization $organization, Member $member, MemberUpdateRequest $request, BillableRateService $billableRateService): JsonResource
public function update(Organization $organization, Member $member, MemberUpdateRequest $request, BillableRateService $billableRateService, MemberService $memberService): JsonResource
{
$this->checkPermission($organization, 'members:update', $member);
if ($request->has('billable_rate')) {
$member->billable_rate = $request->getBillableRate();
}
if ($request->has('role')) {
$member->role = $request->getRole()->value;
if ($request->has('role') && $member->role !== $request->getRole()->value) {
$newRole = $request->getRole();
$oldRole = Role::from($member->role);
if ($oldRole === Role::Owner) {
throw new OrganizationNeedsAtLeastOneOwner();
}
if ($newRole === Role::Placeholder) {
throw new ChangingRoleToPlaceholderIsNotAllowed();
}
if ($newRole === Role::Owner) {
if ($this->hasPermission($organization, 'members:change-ownership')) {
$memberService->changeOwnership($organization, $member);
} else {
throw new OnlyOwnerCanChangeOwnership();
}
} else {
$member->role = $request->getRole()->value;
}
}
$member->save();

View File

@@ -25,8 +25,7 @@ class MemberUpdateRequest extends FormRequest
return [
'role' => [
'string',
Rule::enum(Role::class)
->except([Role::Owner, Role::Placeholder]),
Rule::enum(Role::class),
],
'billable_rate' => [
'nullable',

View File

@@ -116,7 +116,7 @@ class JetstreamServiceProvider extends ServiceProvider
'invitations:remove',
'members:view',
'members:invite-placeholder',
'members:change-role',
'members:change-ownership',
'members:update',
'members:delete',
])->description('Owner users can perform any action. There is only one owner per organization.');
@@ -160,6 +160,7 @@ class JetstreamServiceProvider extends ServiceProvider
'invitations:resend',
'invitations:remove',
'members:view',
'members:update',
'members:invite-placeholder',
])->description('Administrator users can perform any action, except accessing the billing dashboard.');

View File

@@ -24,9 +24,12 @@ class DeletionService
{
private UserService $userService;
public function __construct(UserService $userService)
private MemberService $memberService;
public function __construct(UserService $userService, MemberService $memberService)
{
$this->userService = $userService;
$this->memberService = $memberService;
}
public function deleteOrganization(Organization $organization, bool $inTransaction = true, ?User $ignoreUser = null): void
@@ -145,7 +148,7 @@ class DeletionService
if ($member->role === Role::Owner->value) {
$this->deleteOrganization($member->organization, false, $user);
} else {
$this->userService->makeMemberToPlaceholder($member);
$this->memberService->makeMemberToPlaceholder($member);
}
}

View File

@@ -0,0 +1,61 @@
<?php
declare(strict_types=1);
namespace App\Service;
use App\Enums\Role;
use App\Models\Member;
use App\Models\Organization;
use App\Models\User;
use InvalidArgumentException;
class MemberService
{
private UserService $userService;
public function __construct(UserService $userService)
{
$this->userService = $userService;
}
/**
* Change the ownership of an organization to a new user.
* The previous owner will be demoted to an admin.
*/
public function changeOwnership(Organization $organization, Member $newOwner): void
{
$organization->update([
'user_id' => $newOwner->user_id,
]);
if ($newOwner->organization_id !== $organization->getKey()) {
throw new InvalidArgumentException('Member is not part of the organization');
}
$newOwner->role = Role::Owner->value;
$newOwner->save();
$oldOwners = Member::query()
->whereBelongsTo($organization, 'organization')
->where('role', '=', Role::Owner->value)
->where('id', '!=', $newOwner->getKey())
->get();
foreach ($oldOwners as $oldOwner) {
$oldOwner->role = Role::Admin->value;
$oldOwner->save();
}
}
public function makeMemberToPlaceholder(Member $member): void
{
$user = $member->user;
$placeholderUser = $user->replicate();
$placeholderUser->is_placeholder = true;
$placeholderUser->save();
$member->user()->associate($placeholderUser);
$member->role = Role::Placeholder->value;
$member->save();
$this->userService->assignOrganizationEntitiesToDifferentMember($member->organization, $user, $placeholderUser, $member);
$this->userService->makeSureUserHasAtLeastOneOrganization($user);
}
}

View File

@@ -31,7 +31,7 @@ class UserService
$this->assignOrganizationEntitiesToDifferentMember($organization, $fromUser, $toUser, $toMember);
}
private function assignOrganizationEntitiesToDifferentMember(Organization $organization, User $fromUser, User $toUser, Member $toMember): void
public function assignOrganizationEntitiesToDifferentMember(Organization $organization, User $fromUser, User $toUser, Member $toMember): void
{
// Time entries
TimeEntry::query()
@@ -52,21 +52,6 @@ class UserService
]);
}
public function makeMemberToPlaceholder(Member $member): void
{
$user = $member->user;
$placeholderUser = $user->replicate();
$placeholderUser->is_placeholder = true;
$placeholderUser->save();
$member->user()->associate($placeholderUser);
$member->role = Role::Placeholder->value;
$member->save();
$this->assignOrganizationEntitiesToDifferentMember($member->organization, $user, $placeholderUser, $member);
$this->makeSureUserHasAtLeastOneOrganization($user);
}
public function makeSureUserHasAtLeastOneOrganization(User $user): void
{
if ($user->organizations()->count() > 0) {

View File

@@ -4,8 +4,11 @@ declare(strict_types=1);
use App\Exceptions\Api\CanNotDeleteUserWhoIsOwnerOfOrganizationWithMultipleMembers;
use App\Exceptions\Api\CanNotRemoveOwnerFromOrganization;
use App\Exceptions\Api\ChangingRoleToPlaceholderIsNotAllowed;
use App\Exceptions\Api\EntityStillInUseApiException;
use App\Exceptions\Api\InactiveUserCanNotBeUsedApiException;
use App\Exceptions\Api\OnlyOwnerCanChangeOwnership;
use App\Exceptions\Api\OrganizationNeedsAtLeastOneOwner;
use App\Exceptions\Api\TimeEntryCanNotBeRestartedApiException;
use App\Exceptions\Api\TimeEntryStillRunningApiException;
use App\Exceptions\Api\UserIsAlreadyMemberOfProjectApiException;
@@ -21,6 +24,9 @@ return [
EntityStillInUseApiException::KEY => 'The :modelToDelete is still used by a :modelInUse and can not be deleted.',
CanNotRemoveOwnerFromOrganization::KEY => 'Can not remove owner from organization',
CanNotDeleteUserWhoIsOwnerOfOrganizationWithMultipleMembers::KEY => 'Can not delete user who is owner of organization with multiple members. Please delete the organization first.',
OnlyOwnerCanChangeOwnership::KEY => 'Only owner can change ownership',
OrganizationNeedsAtLeastOneOwner::KEY => 'Organization needs at least one owner',
ChangingRoleToPlaceholderIsNotAllowed::KEY => 'Changing role to placeholder is not allowed',
],
'unknown_error_in_admin_panel' => 'An unknown error occurred. Please check the logs.',
];

View File

@@ -4,6 +4,7 @@ declare(strict_types=1);
namespace Tests;
use App\Enums\Role;
use App\Models\Member;
use App\Models\Organization;
use App\Models\User;
@@ -18,7 +19,7 @@ abstract class TestCaseWithDatabase extends TestCase
/**
* @param array<string> $permissions
* @return object{user: User, organization: Organization, member: Member}
* @return object{user: User, organization: Organization, member: Member, owner: User, ownerMember: Member}
*/
protected function createUserWithPermission(array $permissions = [], bool $isOwner = false): object
{
@@ -29,7 +30,11 @@ abstract class TestCaseWithDatabase extends TestCase
if ($isOwner) {
$organization = Organization::factory()->withOwner($user)->create();
} else {
$organization = Organization::factory()->create();
$owner = User::factory()->create();
$organization = Organization::factory()->withOwner($owner)->create();
$ownerMember = Member::factory()->forUser($owner)->forOrganization($organization)->create([
'role' => Role::Owner->value,
]);
}
$member = Member::factory()->forUser($user)->forOrganization($organization)->create([
'role' => $roleName,
@@ -39,6 +44,8 @@ abstract class TestCaseWithDatabase extends TestCase
'user' => $user,
'organization' => $organization,
'member' => $member,
'owner' => $isOwner ? $user : $owner,
'ownerMember' => $isOwner ? $member : $ownerMember,
];
}

View File

@@ -91,18 +91,18 @@ class MemberEndpointTest extends ApiEndpointTestAbstract
$data = $this->createUserWithPermission([
'members:update',
]);
$member = Member::factory()->forOrganization($data->organization)->role(Role::Admin)->create();
$this->assertBillableRateServiceIsUnused();
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.members.update', [$data->organization->id, $data->member]), [
$response = $this->putJson(route('api.v1.members.update', [$data->organization->id, $member]), [
'billable_rate' => 10001,
'role' => Role::Employee->value,
]);
// Assert
$response->assertStatus(200);
$member = $data->member;
$member->refresh();
$this->assertSame(10001, $member->billable_rate);
$this->assertSame(Role::Employee->value, $member->role);
@@ -155,7 +155,7 @@ class MemberEndpointTest extends ApiEndpointTestAbstract
$this->assertSame(Role::Admin->value, $otherMember->role);
}
public function test_update_member_role_fails_if_role_is_owner(): void
public function test_update_member_allows_role_owner_in_request_if_that_would_not_the_role(): void
{
// Arrange
$data = $this->createUserWithPermission([
@@ -164,13 +164,97 @@ class MemberEndpointTest extends ApiEndpointTestAbstract
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $data->member->getKey()]), [
$response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $data->ownerMember->getKey()]), [
'role' => Role::Owner->value,
]);
// Assert
$response->assertStatus(422);
$response->assertJsonPath('message', 'The selected role is invalid.');
$response->assertStatus(200);
}
public function test_update_member_fails_if_user_tries_to_change_role_to_owner(): void
{
// Arrange
$data = $this->createUserWithPermission([
'members:update',
]);
$member = Member::factory()->forOrganization($data->organization)->role(Role::Employee)->create();
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $member->getKey()]), [
'role' => Role::Owner->value,
]);
// Assert
$response->assertStatus(400);
$response->assertJsonPath('message', 'Only owner can change ownership');
}
public function test_update_member_fails_if_user_tries_to_change_role_of_the_current_owner(): void
{
// Arrange
$data = $this->createUserWithPermission([
'members:update',
'members:change-ownership',
]);
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $data->ownerMember->getKey()]), [
'role' => Role::Admin->value,
]);
// Assert
$response->assertStatus(400);
$response->assertJsonPath('message', 'Organization needs at least one owner');
}
public function test_update_member_can_change_role_to_everything_expect_owner_with_the_member_update_permission(): void
{
// Arrange
$data = $this->createUserWithPermission([
'members:update',
]);
$member = Member::factory()->forOrganization($data->organization)->role(Role::Employee)->create();
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $member->getKey()]), [
'role' => Role::Admin->value,
]);
// Assert
$response->assertStatus(200);
$member->refresh();
$this->assertSame(Role::Admin->value, $member->role);
}
public function test_update_member_can_change_role_to_owner_if_auth_user_has_change_ownership_permission(): void
{
// Arrange
$data = $this->createUserWithPermission([
'members:update',
'members:change-ownership',
]);
$oldOwner = $data->ownerMember;
$organization = $data->organization;
$member = Member::factory()->forOrganization($data->organization)->role(Role::Employee)->create();
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $member->getKey()]), [
'role' => Role::Owner->value,
]);
// Assert
$response->assertStatus(200);
$member->refresh();
$organization->refresh();
$oldOwner->refresh();
$this->assertSame(Role::Owner->value, $member->role);
$this->assertSame($member->user_id, $organization->user_id);
$this->assertSame(Role::Admin->value, $oldOwner->role);
}
public function test_update_member_role_fails_if_role_is_placeholder(): void
@@ -179,16 +263,17 @@ class MemberEndpointTest extends ApiEndpointTestAbstract
$data = $this->createUserWithPermission([
'members:update',
]);
$member = Member::factory()->forOrganization($data->organization)->role(Role::Employee)->create();
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $data->member->getKey()]), [
$response = $this->putJson(route('api.v1.members.update', [$data->organization->getKey(), $member->getKey()]), [
'role' => Role::Placeholder->value,
]);
// Assert
$response->assertStatus(422);
$response->assertJsonPath('message', 'The selected role is invalid.');
$response->assertStatus(400);
$response->assertJsonPath('message', 'Changing role to placeholder is not allowed');
}
public function test_invite_placeholder_succeeds_if_data_is_valid(): void

View File

@@ -0,0 +1,116 @@
<?php
declare(strict_types=1);
namespace Tests\Unit\Service;
use App\Enums\Role;
use App\Models\Member;
use App\Models\Organization;
use App\Models\Project;
use App\Models\ProjectMember;
use App\Models\TimeEntry;
use App\Models\User;
use App\Service\MemberService;
use App\Service\UserService;
use InvalidArgumentException;
use PHPUnit\Framework\Attributes\CoversClass;
use PHPUnit\Framework\Attributes\UsesClass;
use Tests\TestCaseWithDatabase;
#[CoversClass(MemberService::class)]
#[CoversClass(UserService::class)]
#[UsesClass(MemberService::class)]
class MemberServiceTest extends TestCaseWithDatabase
{
private MemberService $memberService;
protected function setUp(): void
{
parent::setUp();
$this->memberService = app(MemberService::class);
}
public function test_change_ownership_fails_if_member_is_not_part_of_organization(): void
{
// Arrange
$organization = Organization::factory()->create();
$otherOrganization = Organization::factory()->create();
$newOwner = Member::factory()->forOrganization($otherOrganization)->create();
// Act
$this->expectException(InvalidArgumentException::class);
$this->memberService->changeOwnership($organization, $newOwner);
// Assert
$this->assertDatabaseHas(Organization::class, [
'id' => $organization->getKey(),
'user_id' => null,
]);
}
public function test_change_ownership_changes_ownership_to_new_member(): void
{
$organization = Organization::factory()->create();
$newOwner = User::factory()->create();
$oldOwner = User::factory()->create();
$newOwnerMember = Member::factory()->forUser($newOwner)->forOrganization($organization)->role(Role::Admin)->create();
$oldOwnerMember = Member::factory()->forUser($oldOwner)->forOrganization($organization)->role(Role::Owner)->create();
// Act
$this->memberService->changeOwnership($organization, $newOwnerMember);
// Assert
$this->assertSame($newOwner->getKey(), $organization->refresh()->user_id);
$this->assertSame(Role::Owner->value, $newOwnerMember->refresh()->role);
$this->assertSame(Role::Admin->value, $oldOwnerMember->refresh()->role);
}
public function test_make_member_to_placeholder_creates_new_user_based_on_member_and_changes_member_to_placeholder(): void
{
// Arrange
$user = User::factory()->create();
$organization = Organization::factory()->create();
$member = Member::factory()->forOrganization($organization)->forUser($user)->role(Role::Employee)->create();
$timeEntry = TimeEntry::factory()->forOrganization($organization)->forMember($member)->create();
$project = Project::factory()->forOrganization($organization)->create();
$projectMember = ProjectMember::factory()->forProject($project)->forMember($member)->create();
// Note: create other user, organization, member, time entry and project member to check that they are not changed
$otherUser = User::factory()->create();
$otherOrganization = Organization::factory()->create();
$otherMember = Member::factory()->forOrganization($otherOrganization)->forUser($otherUser)->role(Role::Employee)->create();
$otherTimeEntry = TimeEntry::factory()->forOrganization($otherOrganization)->forMember($otherMember)->create();
$otherProject = Project::factory()->forOrganization($otherOrganization)->create();
$otherProjectMember = ProjectMember::factory()->forProject($otherProject)->forMember($otherMember)->create();
// Act
$this->memberService->makeMemberToPlaceholder($member);
// Assert
$member->refresh();
$timeEntry->refresh();
$projectMember->refresh();
$placeholderUser = $member->user;
$this->assertTrue($placeholderUser->is_placeholder);
$this->assertSame(Role::Placeholder->value, $member->role);
$this->assertSame($organization->getKey(), $member->organization_id);
$this->assertSame($placeholderUser->getKey(), $projectMember->user_id);
$this->assertSame($member->getKey(), $projectMember->member_id);
$this->assertSame($placeholderUser->getKey(), $timeEntry->user_id);
$this->assertSame($member->getKey(), $timeEntry->member_id);
$this->assertSame(1, $user->organizations()->count());
// Note: check that other user did not change
$otherMember->refresh();
$otherTimeEntry->refresh();
$otherProjectMember->refresh();
$otherUser->refresh();
$this->assertFalse($otherUser->is_placeholder);
$this->assertSame(Role::Employee->value, $otherMember->role);
$this->assertSame($otherOrganization->getKey(), $otherMember->organization_id);
$this->assertSame($otherUser->getKey(), $otherProjectMember->user_id);
$this->assertSame($otherMember->getKey(), $otherProjectMember->member_id);
$this->assertSame($otherUser->getKey(), $otherTimeEntry->user_id);
$this->assertSame($otherMember->getKey(), $otherTimeEntry->member_id);
$this->assertSame(1, $otherUser->organizations()->count());
}
}

View File

@@ -115,54 +115,6 @@ class UserServiceTest extends TestCase
}
}
public function test_make_member_to_placeholder_creates_new_user_based_on_member_and_changes_member_to_placeholder(): void
{
// Arrange
$user = User::factory()->create();
$organization = Organization::factory()->create();
$member = Member::factory()->forOrganization($organization)->forUser($user)->role(Role::Employee)->create();
$timeEntry = TimeEntry::factory()->forOrganization($organization)->forMember($member)->create();
$project = Project::factory()->forOrganization($organization)->create();
$projectMember = ProjectMember::factory()->forProject($project)->forMember($member)->create();
// Note: create other user, organization, member, time entry and project member to check that they are not changed
$otherUser = User::factory()->create();
$otherOrganization = Organization::factory()->create();
$otherMember = Member::factory()->forOrganization($otherOrganization)->forUser($otherUser)->role(Role::Employee)->create();
$otherTimeEntry = TimeEntry::factory()->forOrganization($otherOrganization)->forMember($otherMember)->create();
$otherProject = Project::factory()->forOrganization($otherOrganization)->create();
$otherProjectMember = ProjectMember::factory()->forProject($otherProject)->forMember($otherMember)->create();
// Act
$this->userService->makeMemberToPlaceholder($member);
// Assert
$member->refresh();
$timeEntry->refresh();
$projectMember->refresh();
$placeholderUser = $member->user;
$this->assertTrue($placeholderUser->is_placeholder);
$this->assertSame(Role::Placeholder->value, $member->role);
$this->assertSame($organization->getKey(), $member->organization_id);
$this->assertSame($placeholderUser->getKey(), $projectMember->user_id);
$this->assertSame($member->getKey(), $projectMember->member_id);
$this->assertSame($placeholderUser->getKey(), $timeEntry->user_id);
$this->assertSame($member->getKey(), $timeEntry->member_id);
$this->assertSame(1, $user->organizations()->count());
// Note: check that other user did not change
$otherMember->refresh();
$otherTimeEntry->refresh();
$otherProjectMember->refresh();
$otherUser->refresh();
$this->assertFalse($otherUser->is_placeholder);
$this->assertSame(Role::Employee->value, $otherMember->role);
$this->assertSame($otherOrganization->getKey(), $otherMember->organization_id);
$this->assertSame($otherUser->getKey(), $otherProjectMember->user_id);
$this->assertSame($otherMember->getKey(), $otherProjectMember->member_id);
$this->assertSame($otherUser->getKey(), $otherTimeEntry->user_id);
$this->assertSame($otherMember->getKey(), $otherTimeEntry->member_id);
$this->assertSame(1, $otherUser->organizations()->count());
}
public function test_make_sure_user_has_current_organization_sets_current_organization_for_user_if_null(): void
{
// Arrange