[server] don't always throw exception in deleteUser #5289
Conversation
|
/lgtm |
|
LGTM label has been added. Git tree hash: afc681ebe5a329e2074dea9647ad3403adea8cae
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: laushinka Associated issue: #5262 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thanks @JanKoehnlein for fixing this and @laushinka for reviewing! 🏓 |
| @@ -61,7 +61,7 @@ export class UserDeletionServiceEE extends UserDeletionService { | |||
| } | |||
|
|
|||
| await super.deleteUser(id); | |||
| if (errors) { | |||
| if (errors.length > 0) { | |||
There was a problem hiding this comment.
@JanKoehnlein, actually, after seeing more instances of this issue, I belief it's not a good idea in general to throw an error after super.deleteUser is actually done.
This leads to a state, where one's account is locked in a deleted state. This definitely requires proper handling in the frontend.
There was a problem hiding this comment.
I was mostly worried about users that have a GitHub-based subscription. They actually cannot cancel that subscription using our service, so they should get a message. Similar when Chargebee fails any reason. In any case, users wanted to delete their account so I thought it would be OK if that is the state on the Gitpod side.
But I am all in for proper error propagation to the frontend. Just unsure which actions should be taken/rolled back on the Gitpod side if some of the error conditions are met.
There was a problem hiding this comment.
That's not an easy issue to solve. While error propagation isn't the real adventure, to decide on the loose end is, especially because the account data will be deleted.
Maybe we should not call super.deleteUser if there are issues to cancel subscription. AND propagate an error.
Fixes #5262