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 a06ab6b2a0..fe58129f1a 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -343,11 +343,17 @@ App::delete('/v1/teams/:teamId') 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 fe58129f1a..f34187ea9a 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -350,9 +350,9 @@ App::delete('/v1/teams/:teamId') $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 f34187ea9a..2a2611cc5c 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -347,13 +347,6 @@ App::delete('/v1/teams/:teamId') 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 d84d83ff77..fa92739510 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -530,6 +530,10 @@ App::get('/v1/users/:userId/memberships') $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 fa92739510..d84d83ff77 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -530,10 +530,6 @@ App::get('/v1/users/:userId/memberships') $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 2a2611cc5c..2b2f3b6920 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -344,9 +344,11 @@ App::delete('/v1/teams/:teamId') ]); 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 8e19bede17..7d1e51c08c 100644 --- a/tests/e2e/Services/Teams/TeamsBase.php +++ b/tests/e2e/Services/Teams/TeamsBase.php @@ -428,4 +428,67 @@ trait TeamsBase $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 2b2f3b6920..5c5c128f57 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -343,8 +343,8 @@ App::delete('/v1/teams/:teamId') 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 7d1e51c08c..8e19bede17 100644 --- a/tests/e2e/Services/Teams/TeamsBase.php +++ b/tests/e2e/Services/Teams/TeamsBase.php @@ -428,67 +428,4 @@ trait TeamsBase $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 aa1be49f41..569170f890 100644 --- a/tests/e2e/Services/Teams/TeamsBaseServer.php +++ b/tests/e2e/Services/Teams/TeamsBaseServer.php @@ -4,6 +4,7 @@ namespace Tests\E2E\Services\Teams; use Tests\E2E\Client; use Utopia\Database\Validator\Datetime as DatetimeValidator; +use Utopia\Database\Helpers\ID; trait TeamsBaseServer { @@ -281,4 +282,101 @@ trait TeamsBaseServer $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 569170f890..88bee1d62b 100644 --- a/tests/e2e/Services/Teams/TeamsBaseServer.php +++ b/tests/e2e/Services/Teams/TeamsBaseServer.php @@ -285,32 +285,9 @@ trait TeamsBaseServer 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 @@ trait TeamsBaseServer $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 @@ trait TeamsBaseServer $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 88bee1d62b..5950824da3 100644 --- a/tests/e2e/Services/Teams/TeamsBaseServer.php +++ b/tests/e2e/Services/Teams/TeamsBaseServer.php @@ -308,7 +308,7 @@ trait TeamsBaseServer $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++) {