-
Notifications
You must be signed in to change notification settings - Fork 8
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
Added an option to delete a user accout #373
Conversation
+ Exposes a way for users to delete their account from the front-end
+ Fixed typo
+ Fixed resetting the session storage flag when the account is deleted. + Changed the text to let user know that the Zeeguu team is notified in case of error.
✅ Deploy Preview for voluble-nougat-015dd1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
good stuff.
i made a few changes and left a few minor requests.
tak!
src/pages/AccountDeletion.js
Outdated
import SessionStorage from "../assorted/SessionStorage"; | ||
import { APP_DOMAIN } from "../appConstants"; | ||
|
||
export default function AcountDeletion({ 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.
typo
src/pages/AccountDeletion.js
Outdated
export default function AcountDeletion({ api }) { | ||
const user = useContext(UserContext); | ||
const [headingMsg, setHeadingMsg] = useState("We are deleting your account"); | ||
const [errorOcurred, setErrorOcurred] = useState(); |
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.
ditto
src/pages/AccountDeletion.js
Outdated
redirect(APP_DOMAIN); | ||
} | ||
}, []); | ||
// In case the user went to the path by mistake |
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.
i don't get this comment. does it refer to the next line?
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.
Yes, not sure why it was down there!
src/pages/AccountDeletion.js
Outdated
setHeadingMsg("✅ Your account has been successfully deleted!"); | ||
setTimeout(() => { | ||
user.logoutMethod(); | ||
}, [TIME_BEFORE_REDIRECT]); |
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.
my linter complains about the setTimeout
second parameter being an array. Should it not be a simple int?
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.
It's right. It somehow was still working, which is strange - but it is expected to be an int.
src/pages/AccountDeletion.js
Outdated
useEffect(() => { | ||
console.log(user); | ||
if (SessionStorage.hasUserConfirmationForAccountDeletion()) { | ||
setErrorOccurred(false); |
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.
I'm getting an warning in the IDE: Argument type boolean is not assignable to parameter type (prevState: undefined) => undefine
for both errorOccurred and isAccountDeleted. I think this is because they are not initialized. Can't we simply initialize them with false
?
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.
Yes! We can set it to false.
The reason why I stopped initializing the React states is in order to use the loading component, if you set it to false it's much harder to know when you actually have updated the API. In this case, we can just render the component and respond after one of them is toggled, but in other cases I don't see an easy way without leaving it undefinied.
To be honest, I don't understand why that warning would appear, as you can assign whatever Type to a React state, as we never have to specify types when we create a react state? Or am I wrong?
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.
I know: It's really funny and I don't fully understand why this is.
One thing is to simply disable that warning.
The other would be to from now on, not use boolean variables, but a locally declared enum that tracks the possible states of the respective variables, e.g. the above would become something like: deletionStatus
would have one of the values UNDEFINED
, ERRORED
, OK
. Does this make sense?
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.
When I googled this warning it seems related with TypeScript which enforces Type declarations for Javascript so I don't know if it's relevant for our codebase since we are not using typescript. I am okay with using an Enum, but essentially we are adding one more variable that won't do much more than what the conditions do now.
The state would have to have at least 4 states:
- UNDEFINED
- WAITING_API_RESPONSE
- OK
- ERRORED
I will implemented with this and we can decide which variation we prefer.
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.
I kind of like this! What is your feeling about it?
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.
I also like it!
I am not sure if we need the React states anymore of isAccountDeleted
and ErrorOcurred
, as they are not used anymore - so if we like this I would remove those to avoid confusion.
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.
yes, let's!
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.
We should be ready then!
setShowConfirmationModal(false); | ||
SessionStorage.setUserConfirmationForAccountDeletion(true); | ||
redirect(APP_DOMAIN + "account_deletion"); | ||
} else { |
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.
I think we can simply redirect to /account_deletion
. Or?
I have added the Enum Status tracking - Let me know what you think |
|
||
const TIME_BEFORE_REDIRECT = 5000; | ||
|
||
const DeletionStatus = Object.freeze({ |
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.
I love the fact that you're freezing this object! I didn't know this function existed :)
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.
We use it other places!
ExerciseTypeConstants.js
and userBookmarkPreferences.js
- This is how I saw to do Enums in JS.
Changes
Notes on Implementation
The account deletion is performed at the path /account_deletion, but only proceeds if there is a session storage value which is assigned when the user clicks the button inside the Modal in the settings. This will write to the session storage, and allow the call to the API to be made.
Screens:
Settings Menu
Deletion Modal
Account Deletion Route
Success Screen:
Error Screen: