issue: ENG-6502
We want to have a singe action for deleting identity account and keyserver accounts, and an action for deleting just a keyserver account (for when the user disconnects from a keyserver)
Details
Details
Tested that it is possible to delete account from both web and native and no errors apprear. Tested that if callback returned from useDeleteAccount throws on web, an error message is displayed in the modal.
Tested that when the deletion appears to have been succeful, the user is removed from keyserver db
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
This comment was removed by ashoat.
| lib/actions/user-actions.js | ||
|---|---|---|
| 200 ↗ | (On Diff #35911) | In what scenarios would we want to support overriding keyserverIDs here? |
Comment Actions
This diff hides a lot of changes:
- Changes order to have identity account deletion first
- Changes error messages so they are the same for both identity failure and keyserver failure
- Removes the call to deleteNativeCredentialsFor
This diff should have been separated into a pure refactor, and then a separate diff for each of the opinionated changes, to make it easier for the reviewer to realize what you were doing.
I won't insist on it now, but please be thoughtful about this going forward. If I hadn't done an extra thorough review, I worry that your serious changes would have snuck through unnoticed because you combined them with other, more innocent changes.
| native/profile/delete-account.react.js | ||
|---|---|---|
| 69 ↗ | (On Diff #35911) | This was removed. Did you notice that you removed it? What's your plan for it? |
Comment Actions
Bring back deleteNativeCredentialsFor, call deleteKeyserverAccount and identityClient.deleteUser in parallel - before we would dispatch the actions one after another. dispatchActionPromise dispatches started, and then awaits the promise. So if two actions are dispatchActionPromised one after another, their promises are run in parallel.
Comment Actions
Changes error messages so they are the same for both identity failure and keyserver failure
This is hard to keep in this refactor, because both calls are hidden inside one action now. But if it's important, I can refactor this further to have different error messages
| lib/actions/user-actions.js | ||
|---|---|---|
| 200 ↗ | (On Diff #35911) | We don't have any use cases for this currently |
Comment Actions
Once the use of the Object type is fixed I can accept. Requesting changes now because I'd like to review the updated code before you land
| lib/actions/user-actions.js | ||
|---|---|---|
| 210 ↗ | (On Diff #36031) | You should not use Object here. It is basically any. Take a look at other Promise.all calls in the codebase to see how you can type this correctly (avoid const promises, and just construct array inline at the Promise.all invocation) |
| lib/actions/user-actions.js | ||
|---|---|---|
| 200 ↗ | (On Diff #36031) | If we have no use cases for setting this, I think it should be removed for now. It can always be added back later if we need it, right? |
| lib/actions/user-actions.js | ||
|---|---|---|
| 207–219 ↗ | (On Diff #36126) | I think it would be cleaner to do this. What do you think? |
Comment Actions
Make code cleaner
| lib/actions/user-actions.js | ||
|---|---|---|
| 209 ↗ | (On Diff #36235) | eslint consistent-return forces this undefined |