feat: add self-service account deletion to settings page#4650
feat: add self-service account deletion to settings page#4650GMetaxakis wants to merge 9 commits intoONEARMY:masterfrom
Conversation
onearmy-community-platform
|
||||||||||||||||||||||||||||||||||
| Project |
onearmy-community-platform
|
| Branch Review |
pull/4650
|
| Run status |
|
| Run duration | 29m 33s |
| Commit |
|
| Committer | Mário Nunes |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
94
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
src/integration/SignUp.spec.ts • 1 failed test • ci-chrome
| Test | Artifacts | |
|---|---|---|
| [User sign-up] > [Update existing auth details] > Updates email, password, and deletes account |
Test Replay
Screenshots
Video
|
|
| }); | ||
|
|
||
| describe('[Delete account]', () => { | ||
| it('Rejects wrong password', () => { |
There was a problem hiding this comment.
Not sure this needs a seperate test. Add as a step to the main test.
| cy.get('[data-cy="Confirm.modal: Confirm"]').click(); | ||
|
|
||
| cy.step('Redirected to homepage'); | ||
| cy.url().should('eq', Cypress.config().baseUrl + '/'); |
There was a problem hiding this comment.
Would be good to see what happens to the content of a deleted user:
- Comments (de-author)
- Questions (de-author)
- Research (de-author)
- Projects (de-author)
- News (de-author)
- Map pins (delete)
There was a problem hiding this comment.
To do that, the best way might be with a seed. Have the profile and associated items which we delete on the test, instead of signing up a new user and creating the content each time.
If that's difficult for some reason, at least this PR must ensure those delete/set null cascades are correct.
| const data = new FormData(); | ||
| data.append('password', password); | ||
|
|
||
| return await fetch('/api/account/delete', { |
There was a problem hiding this comment.
just to double-check, using a POST instead of DELETE because we need to send FormData?
src/routes/api.account.delete.tsx
Outdated
| const authId = claims.data?.claims?.sub; | ||
|
|
||
| if (!authId) { | ||
| return Response.json({ error: 'Unauthorized' }, { headers, status: 401 }); |
There was a problem hiding this comment.
please update the error handling to use the new methods validationError, unauthorizedError etc.
Also need to add a try-catch. see other api endpoints like api.questions.$id.ts
src/routes/api.account.delete.tsx
Outdated
|
|
||
| const adminClient = createSupabaseAdminServerClient(); | ||
|
|
||
| await adminClient.from('notifications').delete().eq('owned_by_id', profile.id); |
There was a problem hiding this comment.
I'd rather use a delete cascade for all of these, so we need a schema change.
src/routes/api.account.delete.tsx
Outdated
| await adminClient.from('notifications').delete().eq('triggered_by_id', profile.id); | ||
| await adminClient.from('notifications_preferences').delete().eq('user_id', profile.id); | ||
|
|
||
| const { error } = await adminClient.auth.admin.deleteUser(authId); |
There was a problem hiding this comment.
We can't delete the user without checking if the user has a profile on other tenants.
1 user account can have multiple profiles (1 per tenant)
If the user does not have other profiles, we can delete the account, this will cascade delete the profile.
If the user has other profiles, just delete the profile for this tenant (try using the normal client, not the admin client as it is restricted to the current tenant).
mariojsnunes
left a comment
mariojsnunes
left a comment
There was a problem hiding this comment.
Great first step! Still a few things to be improved before merging :)
- Add migration to fix cascade deletes on notifications.triggered_by_id (SET NULL) and notifications_preferences.user_id (CASCADE), removing the need for manual deletes before profile removal - Rewrite API route with try-catch, consistent error response pattern using statusText, and multi-tenant check (only delete auth user if no profiles exist on other tenants, otherwise delete current profile only) - Update frontend to read errors from statusText - Merge delete account E2E test into the main auth details test flow
3 participants
PR Checklist
PR Type
What is the new behavior?
Adds a "Delete account" section to Settings > Account, replacing the old Discord link. Users can now delete their own account by entering their password and confirming via a modal.
/Does this PR introduce a DB Schema Change or Migration?
Git Issues
Closes #4163