-
Notifications
You must be signed in to change notification settings - Fork 429
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
fix: user delete social auth #3693
fix: user delete social auth #3693
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@shubham-padia is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
@@ -126,6 +128,7 @@ class TheComponent extends Component { | |||
render() { | |||
const { | |||
state: { | |||
auth_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using snake case here, since first_name, last_name and similar fields are also doing the same for fields fetched from the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
{ isError: isMutationError, isSuccess: updateSuccess }, | ||
] = useDeleteAccountMutation() | ||
const skipPasswordConfirmation = | ||
auth_type === 'GOOGLE' || auth_type === 'GITHUB' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using string literals here, since I could not find enums being used elsewhere in the frontend codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
Uffizzi Preview |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3693 +/- ##
==========================================
- Coverage 95.92% 95.89% -0.03%
==========================================
Files 1103 1103
Lines 34812 34848 +36
==========================================
+ Hits 33392 33419 +27
- Misses 1420 1429 +9 ☔ View full report in Codecov by Sentry. |
9bd0e3f
to
69626b5
Compare
69626b5
to
0277681
Compare
Not sure why all of these 9 lines of code are not being covered, they should be called by the testing functions 🤔 , except the fail message. I can add tests for the fail message, but not sure if it should be this PR or another one since it is related to the original PR introducing the delete user functionality. https://app.codecov.io/gh/Flagsmith/flagsmith/commit/9bd0e3fba28f33d7c96b6d76c1fcf408e2474e78 (screenshot below) |
bump for review |
# create a couple of users | ||
email1 = "[email protected]" | ||
email2 = "[email protected]" | ||
email3 = "[email protected]" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove any unnecessary line breaks from the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham-padia are you able to resolve this and we can get this merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, with a minor comment
@novakzaballa Replied to your comment. Let me know if there are any other blockers to getting this merged :) ! |
0277681
to
d1fe382
Compare
Fixes Flagsmith#2782. We are enabling passwordless deletions only for Github and Google accounts and not for non email/password auth accounts, since it doesn't feel right to assume the behaviour of the next social auth that might be added.
We are only checking the email match on the frontend, because if user is directly using the API, we can assume they know their stuff enough not to accidentally delete their account. Social auth users with password will also get the email confirmation screen.
d1fe382
to
087e8d7
Compare
Fixes #2782
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
We are enabling passwordless deletions only for Github and Google accounts and not for non email/password auth accounts, since it doesn't feel right to assume the behaviour of the next social auth that might be added. It's been a while since I dabbled with React, so please lmk if I should structure the code in a different way.
I'm also just ignoring the password field altogether for social auth as you can see in the API tests. Lmk if there are any concerns there.
How did you test this code?
Delete user with email/password:
email.account.delete.flagsmith.mp4
Delete user with social auth (only tested on google for now):
flagsmith.google.delete.mp4