From 0fe59d457b08fbbf588a128e8a410e618d48308e Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Tue, 1 Aug 2023 22:57:59 +0400 Subject: [PATCH 01/10] fixed stale team memberships on user --- app/controllers/api/teams.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/teams.php b/app/controllers/api/teams.php index a06ab6b2a00..fe58129f1a6 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -343,11 +343,17 @@ Query::limit(2000), // TODO fix members limit ]); - // TODO delete all members individually from the user object foreach ($memberships as $membership) { if (!$dbForProject->deleteDocument('memberships', $membership->getId())) { throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Failed to remove membership for team from DB'); } + + $user = $dbForProject->getDocument('users', $membership->getAttribute('userId')); + $user->setAttribute('memberships', array_values(array_filter( + $user->getAttribute('memberships', []), + fn($um) => $um['teamId'] !== $membership->getAttribute('teamId')) + )); + $dbForProject->updateDocument('users', $user->getId(), $user); } if (!$dbForProject->deleteDocument('teams', $teamId)) { From a45c62ab24c8a9e415a46290e968a70a3741bc12 Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Tue, 1 Aug 2023 23:24:46 +0400 Subject: [PATCH 02/10] run composer scripts --- app/controllers/api/teams.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/teams.php b/app/controllers/api/teams.php index fe58129f1a6..f34187ea9a4 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -350,9 +350,9 @@ $user = $dbForProject->getDocument('users', $membership->getAttribute('userId')); $user->setAttribute('memberships', array_values(array_filter( - $user->getAttribute('memberships', []), - fn($um) => $um['teamId'] !== $membership->getAttribute('teamId')) - )); + $user->getAttribute('memberships', []), + fn($um) => $um['teamId'] !== $membership->getAttribute('teamId') + ))); $dbForProject->updateDocument('users', $user->getId(), $user); } From c5233d9ece9f9c0a82c6121a367d3f63ce032dd2 Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Wed, 2 Aug 2023 12:18:21 +0400 Subject: [PATCH 03/10] removed unnecessary code --- app/controllers/api/teams.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/controllers/api/teams.php b/app/controllers/api/teams.php index f34187ea9a4..2a2611cc5cd 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -347,13 +347,6 @@ if (!$dbForProject->deleteDocument('memberships', $membership->getId())) { throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Failed to remove membership for team from DB'); } - - $user = $dbForProject->getDocument('users', $membership->getAttribute('userId')); - $user->setAttribute('memberships', array_values(array_filter( - $user->getAttribute('memberships', []), - fn($um) => $um['teamId'] !== $membership->getAttribute('teamId') - ))); - $dbForProject->updateDocument('users', $user->getId(), $user); } if (!$dbForProject->deleteDocument('teams', $teamId)) { From 2bc2061f091eb4e3ba358292057dac1db02c4273 Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Wed, 2 Aug 2023 18:44:43 +0400 Subject: [PATCH 04/10] fix showing of stale team memberships --- app/controllers/api/users.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/controllers/api/users.php b/app/controllers/api/users.php index d84d83ff776..fa927395109 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -530,6 +530,10 @@ function createUser(string $hash, mixed $hashOptions, string $userId, ?string $e $memberships = array_map(function ($membership) use ($dbForProject, $user) { $team = $dbForProject->getDocument('teams', $membership->getAttribute('teamId')); + if ($team->isEmpty()) { + throw new Exception(Exception::TEAM_NOT_FOUND); + } + $membership ->setAttribute('teamName', $team->getAttribute('name')) ->setAttribute('userName', $user->getAttribute('name')) From a14d89eb3cc3cc7fb28d154682b33565f990d0b1 Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Thu, 3 Aug 2023 21:39:18 +0400 Subject: [PATCH 05/10] remove error-causing condition --- app/controllers/api/users.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/controllers/api/users.php b/app/controllers/api/users.php index fa927395109..d84d83ff776 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -530,10 +530,6 @@ function createUser(string $hash, mixed $hashOptions, string $userId, ?string $e $memberships = array_map(function ($membership) use ($dbForProject, $user) { $team = $dbForProject->getDocument('teams', $membership->getAttribute('teamId')); - if ($team->isEmpty()) { - throw new Exception(Exception::TEAM_NOT_FOUND); - } - $membership ->setAttribute('teamName', $team->getAttribute('name')) ->setAttribute('userName', $user->getAttribute('name')) From d142620c5e9ec7f3125e6d39f0c4c7d302bfc62a Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Thu, 3 Aug 2023 22:08:27 +0400 Subject: [PATCH 06/10] invalidate cached document of user - cache caused stale data in memberships --- app/controllers/api/teams.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/api/teams.php b/app/controllers/api/teams.php index 2a2611cc5cd..2b2f3b69205 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -344,9 +344,11 @@ ]); foreach ($memberships as $membership) { + // Memberships are deleted here instead of in the worker to make sure user permisions are updated instantly if (!$dbForProject->deleteDocument('memberships', $membership->getId())) { throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Failed to remove membership for team from DB'); } + $dbForProject->deleteCachedDocument('users', $membership->getAttribute('userId')); } if (!$dbForProject->deleteDocument('teams', $teamId)) { From 06e1191063435bb184a356aa571827301554f623 Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Fri, 4 Aug 2023 00:34:01 +0400 Subject: [PATCH 07/10] added test --- tests/e2e/Services/Teams/TeamsBase.php | 63 ++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/e2e/Services/Teams/TeamsBase.php b/tests/e2e/Services/Teams/TeamsBase.php index 8e19bede173..7d1e51c08c6 100644 --- a/tests/e2e/Services/Teams/TeamsBase.php +++ b/tests/e2e/Services/Teams/TeamsBase.php @@ -428,4 +428,67 @@ public function testUpdateAndGetUserPrefs(array $data): void $this->assertEquals($user['headers']['status-code'], 400); } + + public function testTeamDeleteUpdatesUserMembership() + { + $users = []; + $team = null; + + $team = $this->client->call(Client::METHOD_POST, '/teams', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'teamId' => ID::unique(), + 'name' => 'Demo' + ]); + + $this->assertEquals(201, $team['headers']['status-code']); + $this->assertNotEmpty($team['body']['$id']); + $this->assertEquals('Demo', $team['body']['name']); + $this->assertGreaterThan(-1, $team['body']['total']); + $this->assertIsInt($team['body']['total']); + + for ($i = 0; $i < 5; $i++) { + $mem = $this->client->call(Client::METHOD_POST, '/teams/' . $team['body']['$id'] . '/memberships', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'email' => 'email' . $i . '@example.com', + 'roles' => ['admin', 'editor'], + 'name' => 'User ' . $i, + 'url' => 'http://localhost:5000/join-us#title' + ]); + + $this->assertEquals(201, $mem['headers']['status-code']); + $this->assertNotEmpty($mem['body']['$id']); + $this->assertNotEmpty($mem['body']['userId']); + $this->assertEquals('User ' . $i, $mem['body']['userName']); + $this->assertEquals('email' . $i . '@example.com', $mem['body']['userEmail']); + $this->assertNotEmpty($mem['body']['teamId']); + $this->assertCount(2, $mem['body']['roles']); + } + + $this->client->call(Client::METHOD_DELETE, '/teams/' . $team['body']['$id'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + foreach ($users as $user) { + $user = $this->client->call(Client::METHOD_GET, '/users/' . $user['body']['$id'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals(200, $user['headers']['status-code']); + $this->assertEquals(0, $user['body']['total']); + $this->assertEquals([], $user['body']['memberships']); + } + + $team = $this->client->call(Client::METHOD_GET, '/teams/' . $team['body']['$id'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals(404, $team['headers']['status-code']); + } } From 451f4bee19d31a1408049885a2f8fe6d3fc3faa5 Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Fri, 4 Aug 2023 15:40:42 +0400 Subject: [PATCH 08/10] Fixed an incorrect test for team deletion This commit contains changes in 3 places. - First I changed the placement of an informative comment in the DELETE TEAM controller, and moved it outside of the loop. I did this when Steven pointed out that the behaviour I describe in the comment is for the whole loop. - The second change is the removal of my first test. I was facing quite a few issues with creating users in the test, and ended up using the CREATE TEAM MEMBERSHIP to perform 2 actions at once -> create a new user if one doesn't exist with the provided email, and create a membership for the user. Before this approach, I had quite a bit of code that didn't work, and it seems like I removed some things that weren't supposed to be removed, and didn't change variable names where necessary. Anyway, I figured that the problem has something to do with the user being created on the client side, so I moved the test to the server side. - The new test I implemented does the same thing as my previous failed test, but in more detailed and distinct steps. The test first creates 5 new users inside of a loop, and pushes each new user's ID to an array called 'new_users' if the response is as expected. Then a new team is created. The next step is to create memberships for all 5 users. If all these steps pass, the new team that was just created, is deleted, and we check to make sure the new users have 0 team memberships each. Formatter and linter showed no errors. Tests were successful on localhost. --- app/controllers/api/teams.php | 2 +- tests/e2e/Services/Teams/TeamsBase.php | 63 ------------- tests/e2e/Services/Teams/TeamsBaseServer.php | 98 ++++++++++++++++++++ 3 files changed, 99 insertions(+), 64 deletions(-) diff --git a/app/controllers/api/teams.php b/app/controllers/api/teams.php index 2b2f3b69205..5c5c128f573 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -343,8 +343,8 @@ Query::limit(2000), // TODO fix members limit ]); + // Memberships are deleted here instead of in the worker to make sure user permisions are updated instantly foreach ($memberships as $membership) { - // Memberships are deleted here instead of in the worker to make sure user permisions are updated instantly if (!$dbForProject->deleteDocument('memberships', $membership->getId())) { throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Failed to remove membership for team from DB'); } diff --git a/tests/e2e/Services/Teams/TeamsBase.php b/tests/e2e/Services/Teams/TeamsBase.php index 7d1e51c08c6..8e19bede173 100644 --- a/tests/e2e/Services/Teams/TeamsBase.php +++ b/tests/e2e/Services/Teams/TeamsBase.php @@ -428,67 +428,4 @@ public function testUpdateAndGetUserPrefs(array $data): void $this->assertEquals($user['headers']['status-code'], 400); } - - public function testTeamDeleteUpdatesUserMembership() - { - $users = []; - $team = null; - - $team = $this->client->call(Client::METHOD_POST, '/teams', array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders()), [ - 'teamId' => ID::unique(), - 'name' => 'Demo' - ]); - - $this->assertEquals(201, $team['headers']['status-code']); - $this->assertNotEmpty($team['body']['$id']); - $this->assertEquals('Demo', $team['body']['name']); - $this->assertGreaterThan(-1, $team['body']['total']); - $this->assertIsInt($team['body']['total']); - - for ($i = 0; $i < 5; $i++) { - $mem = $this->client->call(Client::METHOD_POST, '/teams/' . $team['body']['$id'] . '/memberships', array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders()), [ - 'email' => 'email' . $i . '@example.com', - 'roles' => ['admin', 'editor'], - 'name' => 'User ' . $i, - 'url' => 'http://localhost:5000/join-us#title' - ]); - - $this->assertEquals(201, $mem['headers']['status-code']); - $this->assertNotEmpty($mem['body']['$id']); - $this->assertNotEmpty($mem['body']['userId']); - $this->assertEquals('User ' . $i, $mem['body']['userName']); - $this->assertEquals('email' . $i . '@example.com', $mem['body']['userEmail']); - $this->assertNotEmpty($mem['body']['teamId']); - $this->assertCount(2, $mem['body']['roles']); - } - - $this->client->call(Client::METHOD_DELETE, '/teams/' . $team['body']['$id'], array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders())); - - foreach ($users as $user) { - $user = $this->client->call(Client::METHOD_GET, '/users/' . $user['body']['$id'], array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders())); - - $this->assertEquals(200, $user['headers']['status-code']); - $this->assertEquals(0, $user['body']['total']); - $this->assertEquals([], $user['body']['memberships']); - } - - $team = $this->client->call(Client::METHOD_GET, '/teams/' . $team['body']['$id'], array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders())); - - $this->assertEquals(404, $team['headers']['status-code']); - } } diff --git a/tests/e2e/Services/Teams/TeamsBaseServer.php b/tests/e2e/Services/Teams/TeamsBaseServer.php index aa1be49f413..569170f890f 100644 --- a/tests/e2e/Services/Teams/TeamsBaseServer.php +++ b/tests/e2e/Services/Teams/TeamsBaseServer.php @@ -4,6 +4,7 @@ use Tests\E2E\Client; use Utopia\Database\Validator\Datetime as DatetimeValidator; +use Utopia\Database\Helpers\ID; trait TeamsBaseServer { @@ -281,4 +282,101 @@ public function testDeleteUserUpdatesTeamMembershipCount($data) $this->assertIsInt($response['body']['total']); $this->assertEquals(true, $dateValidator->isValid($response['body']['$createdAt'])); } + + public function testTeamDeleteUpdatesUserMembership() + { + $new_users = []; + + /** + * Create 5 new users and add their IDs to an array + */ + for ($i = 0; $i < 5; $i++) { + $user = $this->client->call(Client::METHOD_POST, '/users', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'userId' => ID::unique(), + 'email' => 'newuser' . $i . '@localhost.test', + 'password' => 'password', + 'name' => 'New User ' . $i, + ], false); + + $user_body = json_decode($user['body'], true); + + $this->assertEquals(201, $user['headers']['status-code']); + $this->assertEquals('newuser' . $i . '@localhost.test', $user_body['email']); + $this->assertEquals('New User ' . $i, $user_body['name']); + $this->assertEquals($user_body['status'], true); + + array_push($new_users, $user_body['$id']); + } + + /** + * Create a new team + */ + $new_team = $this->client->call(Client::METHOD_POST, '/teams', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'teamId' => ID::unique(), + 'name' => 'New Team Test', + 'roles' => ['admin', 'editor'], + ]); + + $this->assertEquals(201, $new_team['headers']['status-code']); + $this->assertNotEmpty($new_team['body']['$id']); + $this->assertEquals('New Team Test', $new_team['body']['name']); + $this->assertGreaterThan(-1, $new_team['body']['total']); + $this->assertIsInt($new_team['body']['total']); + $this->assertArrayHasKey('prefs', $new_team['body']); + + /** + * Create team memberships for each of the new users + */ + for ($i = 0; $i < 5; $i++) { + $new_membership = $this->client->call(Client::METHOD_POST, '/teams/' . $new_team['body']['$id'] . '/memberships', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'email' => 'newuser' . $i . '@localhost.test', + 'name' => 'New User ' . $i, + 'roles' => ['admin', 'editor'], + 'url' => 'http://localhost:5000/join-us#title' + ]); + + $this->assertEquals(201, $new_membership['headers']['status-code']); + $this->assertNotEmpty($new_membership['body']['$id']); + $this->assertNotEmpty($new_membership['body']['userId']); + $this->assertEquals('New User ' . $i, $new_membership['body']['userName']); + $this->assertEquals('newuser' . $i . '@localhost.test', $new_membership['body']['userEmail']); + $this->assertNotEmpty($new_membership['body']['teamId']); + $this->assertCount(2, $new_membership['body']['roles']); + $dateValidator = new DatetimeValidator(); + $this->assertEquals(true, $dateValidator->isValid($new_membership['body']['joined'])); + $this->assertEquals(true, $new_membership['body']['confirm']); + } + + /** + * Delete the team + */ + $team_del_response = $this->client->call(Client::METHOD_DELETE, '/teams/' . $new_team['body']['$id'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals(204, $team_del_response['headers']['status-code']); + + /** + * Check that the team memberships for each of the new users has been deleted + */ + for ($i = 0; $i < 5; $i++) { + $membership = $this->client->call(Client::METHOD_GET, '/users/' . $new_users[$i] . '/memberships', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals(200, $membership['headers']['status-code']); + $this->assertEquals(0, $membership['body']['total']); + } + } } From 0295c6ec1b2919385e516d4b86441fee5ff8e9df Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Fri, 4 Aug 2023 23:17:41 +0400 Subject: [PATCH 09/10] improve test by removing user creation loop The CREATE TEAM MEMBERSHIP endpoint requires the email of the user to be added to the team. If the user does not exist in the project, a new user is created with the specified email and added to the team. The first version of the test creates 5 users, and then adds them to the newly created team. This process is more streamlined now, by using the CREATE TEAM MEMBERSHIPS behaviour to create a user on the go and create a membership for them immediately after. I also change the way I add user IDs to the array, by using the shorthand notation instead of the `array_push` function. --- tests/e2e/Services/Teams/TeamsBaseServer.php | 30 ++++---------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/tests/e2e/Services/Teams/TeamsBaseServer.php b/tests/e2e/Services/Teams/TeamsBaseServer.php index 569170f890f..88bee1d62bf 100644 --- a/tests/e2e/Services/Teams/TeamsBaseServer.php +++ b/tests/e2e/Services/Teams/TeamsBaseServer.php @@ -285,32 +285,9 @@ public function testDeleteUserUpdatesTeamMembershipCount($data) public function testTeamDeleteUpdatesUserMembership() { + // Array to store the IDs of newly created users $new_users = []; - /** - * Create 5 new users and add their IDs to an array - */ - for ($i = 0; $i < 5; $i++) { - $user = $this->client->call(Client::METHOD_POST, '/users', array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders()), [ - 'userId' => ID::unique(), - 'email' => 'newuser' . $i . '@localhost.test', - 'password' => 'password', - 'name' => 'New User ' . $i, - ], false); - - $user_body = json_decode($user['body'], true); - - $this->assertEquals(201, $user['headers']['status-code']); - $this->assertEquals('newuser' . $i . '@localhost.test', $user_body['email']); - $this->assertEquals('New User ' . $i, $user_body['name']); - $this->assertEquals($user_body['status'], true); - - array_push($new_users, $user_body['$id']); - } - /** * Create a new team */ @@ -331,7 +308,8 @@ public function testTeamDeleteUpdatesUserMembership() $this->assertArrayHasKey('prefs', $new_team['body']); /** - * Create team memberships for each of the new users + * Use the Create Team Membership endpoint + * to create 5 new users and add them to the team immediately */ for ($i = 0; $i < 5; $i++) { $new_membership = $this->client->call(Client::METHOD_POST, '/teams/' . $new_team['body']['$id'] . '/memberships', array_merge([ @@ -354,6 +332,8 @@ public function testTeamDeleteUpdatesUserMembership() $dateValidator = new DatetimeValidator(); $this->assertEquals(true, $dateValidator->isValid($new_membership['body']['joined'])); $this->assertEquals(true, $new_membership['body']['confirm']); + + $new_users[] = $new_membership['body']['userId']; } /** From 4a9af13f167e938da417bf2a1eedf6cb39215e99 Mon Sep 17 00:00:00 2001 From: Safwan Parkar Date: Fri, 4 Aug 2023 23:41:29 +0400 Subject: [PATCH 10/10] run formatter and linter Run the composer format and lint commands, which I forgot to run before. --- tests/e2e/Services/Teams/TeamsBaseServer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/Services/Teams/TeamsBaseServer.php b/tests/e2e/Services/Teams/TeamsBaseServer.php index 88bee1d62bf..5950824da31 100644 --- a/tests/e2e/Services/Teams/TeamsBaseServer.php +++ b/tests/e2e/Services/Teams/TeamsBaseServer.php @@ -308,7 +308,7 @@ public function testTeamDeleteUpdatesUserMembership() $this->assertArrayHasKey('prefs', $new_team['body']); /** - * Use the Create Team Membership endpoint + * Use the Create Team Membership endpoint * to create 5 new users and add them to the team immediately */ for ($i = 0; $i < 5; $i++) {