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

Added an option to delete a user accout #373

Merged
merged 17 commits into from
Jun 18, 2024
Merged

Conversation

tfnribeiro
Copy link
Contributor

Changes

  • Added an extra button at the settings menu to allow users to delete the account.
  • Refactored settings to have separating lines between the different options
  • Changed button styling to be in line with buttons used in the home page.
  • The user needs to confirm the deletion via a modal that pops up with a red button stating "Delete my account". This process is not reversible, once the user starts it will trigger the deletion of the account.
  • While the deletion is on-going the user is redirected to a new path: /account_deletion, where they may wait to get confirmation that the account has been deleted. If there is an error, the exception is captured and we will be informed.
  • Once complete, a confirmation is given and the user can close the tab, otherwise they are redirected to the homepage.

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

Current New
image image

Deletion Modal

image

Account Deletion Route

image

Success Screen:

image

Error Screen:

image

+ Exposes a way for users to delete their account from the front-end
+ 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.
@tfnribeiro tfnribeiro linked an issue May 14, 2024 that may be closed by this pull request
Copy link

netlify bot commented May 14, 2024

Deploy Preview for voluble-nougat-015dd1 ready!

Name Link
🔨 Latest commit f63b3b0
🔍 Latest deploy log https://app.netlify.com/sites/voluble-nougat-015dd1/deploys/667160f61f157c0008073ab9
😎 Deploy Preview https://deploy-preview-373--voluble-nougat-015dd1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@mircealungu mircealungu left a 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!

import SessionStorage from "../assorted/SessionStorage";
import { APP_DOMAIN } from "../appConstants";

export default function AcountDeletion({ api }) {
Copy link
Member

Choose a reason for hiding this comment

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

typo

export default function AcountDeletion({ api }) {
const user = useContext(UserContext);
const [headingMsg, setHeadingMsg] = useState("We are deleting your account");
const [errorOcurred, setErrorOcurred] = useState();
Copy link
Member

Choose a reason for hiding this comment

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

ditto

redirect(APP_DOMAIN);
}
}, []);
// In case the user went to the path by mistake
Copy link
Member

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?

Copy link
Contributor Author

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!

setHeadingMsg("✅ Your account has been successfully deleted!");
setTimeout(() => {
user.logoutMethod();
}, [TIME_BEFORE_REDIRECT]);
Copy link
Member

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?

Copy link
Contributor Author

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.

useEffect(() => {
console.log(user);
if (SessionStorage.hasUserConfirmationForAccountDeletion()) {
setErrorOccurred(false);
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

yes, let's!

Copy link
Contributor Author

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 {
Copy link
Member

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?

@tfnribeiro
Copy link
Contributor Author

I have added the Enum Status tracking - Let me know what you think


const TIME_BEFORE_REDIRECT = 5000;

const DeletionStatus = Object.freeze({
Copy link
Member

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 :)

Copy link
Contributor Author

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.

@mircealungu mircealungu merged commit 49bf7e4 into master Jun 18, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a way to delete user
2 participants