Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Coverage Report for CI Build 40710379612Coverage remained the same at 55.922%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
1d4bf3c to
56bcc6a
Compare
e75f617 to
bf85b84
Compare
|
|
||
| _, err = s.testBench.Client.DeleteUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteUserRequest{Id: userID})) | ||
| _, violations := requireDeleteRefused(s.T(), err) | ||
| // stripe-mock answers every invoice list with the same draft invoice, so |
There was a problem hiding this comment.
This is a new code comment. Can we drop it? The PR description already explains the stripe-mock behavior.
| s.Assert().False(userExists(userID)) | ||
| s.Assert().False(orgIsLive(aloneOrgID)) | ||
| s.Assert().True(orgIsLive(sharedOrgID)) | ||
| s.Assert().False(orgIsLive(debtOrgID)) |
There was a problem hiding this comment.
Can we also assert that the user left the shared org here, with orgHasUser(sharedOrgID, userID) false?
There was a problem hiding this comment.
Added s.Assert().False(orgHasUser(sharedOrgID, userID)) after the delete in 16df0ba. With the role_pairs change below, it also catches a policy left on the shared org.
| s.Assert().NoError(err) | ||
| }() | ||
| requireFailedDelete := func() { | ||
| s.Require().Equal(connect.CodeInternal, connect.CodeOf(deleteUser(userID))) |
There was a problem hiding this comment.
This passes on any internal error. Can we also match the injected failure message, so we know it stopped at this step?
There was a problem hiding this comment.
The server replaces the text of every internal error with a generic message (UnaryConnectErrorSanitizerInterceptor), so the injected text never reaches the test. Instead, 16df0ba pins each stop by what the steps before it leave behind:
- after the project failure, the project has no live policy but its row is live;
- after the removal failure, the alone org is gone, and the user is still in the shared org and still signed in;
- after the user row failure, the user's session no longer works but the row is live.
Moving the project failure or the user row failure one step earlier now fails the test.
| orgHasUser := func(orgID, userID string) bool { | ||
| resp, err := s.testBench.Client.ListOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationUsersRequest{Id: orgID})) | ||
| s.Require().NoError(err) | ||
| return slices.ContainsFunc(resp.Msg.GetUsers(), func(u *frontierv1beta1.User) bool { return u.GetId() == userID }) |
There was a problem hiding this comment.
Nit: also check GetRolePairs(), so the deleted user is missing from both users and role_pairs.
There was a problem hiding this comment.
orgHasUser now checks role_pairs too, in 16df0ba. users hides deleted users, so only role_pairs can show a policy left behind after the delete. Bringing the user's policy on the shared org back after the delete fails the new check and passes the old one.
| s.Assert().False(orgIsLive(debtOrgID)) | ||
| }) | ||
|
|
||
| s.Run("a disabled member keeps the only owner from leaving", func() { |
There was a problem hiding this comment.
Nit: there's no case for leaving a disabled org, or for a plain member leaving an org with other members. Worth adding one of each?
There was a problem hiding this comment.
A plain member leaving an org with other members is covered by TestUserDeleteLeavesNoAccessAndNoLivePolicy: the user joins as a viewer, and after the delete there's no live policy and no entry in users or role_pairs. Added a case for leaving a disabled org in 16df0ba.
The admin user delete is refused with one violation per blocker across the user's orgs, including a negative token balance and a paid subscription, and nothing changes until the blockers are cleared. Orgs where the user is the only member are deleted with the user, including a disabled org and one whose only other member is a service user. A failure injected at the org delete, the removal from another org and the user row stops the delete, and each retry continues from where the last one stopped.
bf85b84 to
16df0ba
Compare
Stacked on #2027. Merge #2027 first.
Summary
End-to-end tests for the user delete check in #2027. The admin user delete is refused while anything blocks it, and nothing changes. It deletes the orgs where the user is the only member. After a failed step, the next try continues from where the last one stopped.
Changes
test/e2e/regression/api_test.go:TestUserDeleteWithOrgs, six cases:SOLE_OWNER_WITH_MEMBERSandNEGATIVE_TOKEN_BALANCE. The user, the orgs, the balance and the user's membership stay as they were. Once that org has a second owner and the usage is refunded, the delete goes through: the orgs where the user was alone are deleted, and the shared org stays without the user.RESOURCES_EXIST, and the message names the org:user cannot be deleted yet: organization[user-delete-check-disk]: RESOURCES_EXIST. The user's other, empty org is not deleted. Once the disk is gone, the delete goes through.test/e2e/regression/billing_test.go:TestUserDeleteRefusedWhileAnOrgHasAPaidSubscription. A paid subscription in an org where the user is the only member refuses the delete withACTIVE_SUBSCRIPTION. The user, the org and the subscription stay.signUpUserandrequireDeleteRefusedare shared by both suites.Technical Details
orgHasUserlooks at both lists ofListOrganizationUsers.usershides deleted users, whilerole_pairscomes from the org's live policies, so onlyrole_pairswould still show a policy left behind after the delete.ListOrganizationUsersrefuses a disabled org, so the disabled org case counts the user's live policies on it in the database.TestUserDeleteLeavesNoAccessAndNoLivePolicy, from fix(deleter): user delete removes every policy the user holds #2022.UNPAID_INVOICE. The test only checks thatACTIVE_SUBSCRIPTIONis among the violations. An unpaid invoice cannot be set up locally for the delete.TestConsentInsertFailureRollsBackTheUser: fixed names and emails in the function bodies, no SQL built from strings, and a deferred cleanup that drops them even when the test fails. The trigger onusersmatches both a delete and a soft delete, so it keeps working after feat(store): user delete keeps the row #2023.project.Service.DeleteModelremoves a project's relations before its row. After the injected failure on the row, the API answers permission denied for that project, even to an admin, until a retry finishes the delete. The test checks the project row in the database instead.Test Plan
role_pairs; theuserslist alone misses it), the removal from a disabled org failing, and an injected failure moved one step earlier (the project's policies, or the sessions).go test ./test/e2e/...passes.make lintreports no issues.