Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete account route #331

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Delete account route #331

wants to merge 1 commit into from

Conversation

farisashai
Copy link
Member

Leaving as a draft PR for now - basic functionality seems to work to remove all sensitive data and update account state. Still want to discuss tests and edge cases later.

One peculiar thing is that a deleted account can continue to use their bearer token to make authenticated requests, however this would be a FE issue to clear the bearer token since re-logging in would no longer work.

@github-actions
Copy link

Thanks for contributing! If you've made changes to the API's functionality, please make sure to bump the package version—see this guide to semantic versioning for details—and document those changes as appropriate.

@farisashai farisashai changed the title Start draft for delete route Delete account route Feb 26, 2023
@farisashai farisashai requested a review from dowhep February 26, 2023 07:46
@shravanhariharan2
Copy link
Collaborator

One peculiar thing is that a deleted account can continue to use their bearer token to make authenticated requests, however this would be a FE issue to clear the bearer token since re-logging in would no longer work.

I agree, I think what FE can do is delete the bearer token after making this API call. The jwt package doesn't allow us to 'delete' or 'invalidate' a jwt token.

A few things we should also discuss:

  • What should happen to the non-profile user data such as attendances, orders, activity, etc? We probably wouldn't want to delete those because then things like attendance counts for events could drop retroactively even though that user technically did attend the event.
  • If we don't delete this data, how should we handle displaying deleted users' data? E.g. someone orders a merch item for a future pickup event, but then decides to delete their account. Will the store distributors/admins see "Deleted User" on their pickup events admin page? Should the API not even return deleted users in that case? Or should the API still return the deleted users but the UI have some sort of disambiguation that marks a deleted user with a separate color palette or something similar?
  • Should we introduce a separate UserState called DELETED rather than overloading BLOCKED? Since blocked users are semantically meant to be users who are blocked/banned by admins for some malicious reason, though that functionality does not exist yet.
  • We should likely send a confirmation email that the user has successfully deleted their account. Should there be a recover option in case someone else hacked into their portal account and maliciously/accidentally deleted that person's account? Should deletes go through email confirmation instead before the delete happens?

Comment on lines +90 to +95
profilePicture: null,
firstName: 'Deleted',
lastName: 'User',
graduationYear: 0,
major: 'Deleted',
points: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's make the string values here all uppercased so that visually it's easier to distinguish deleted users and active users in e.g. a list of orders, a database table, etc.

lastLogin: new Date(0),
bio: null,
accessType: UserAccessType.RESTRICTED,
state: UserState.BLOCKED,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: my earlier comment about whether we should use a new DELETED state if we plan on implementing separate BLOCKED state functionality later. I think it makes more sense to have a DELETED state because otherwise, a user could actually reach this state without even deleting their account (e.g. they fill "DELETED" as their first name, "USER" as last name, "DELETED" as major, their email as following the template above, and if they end up getting blocked by an admin that sets their access type and state to the above (again, only if the blocked functionality is implemented)). So I think it could be useful to have a state DELETED with the only state transition to it being if a user actually specifies to delete their account.

@shravanhariharan2
Copy link
Collaborator

Discussed offline that we'll be handing this off to someone else, let's put this on hold until the more urgent PRs are merged and new tasks are ready to be taken on

@farisashai farisashai linked an issue Apr 4, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete Account route
2 participants