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 security step before account deletion #1705

Conversation

nmro184
Copy link
Contributor

@nmro184 nmro184 commented May 30, 2024

resolving #1699

Copy link
Collaborator

@RodriSanchez1 RodriSanchez1 left a comment

Choose a reason for hiding this comment

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

@nmro184 good work! I will leave some comments:

image
This needs to be our dialog when it opens. The <TextField> should be displayed directly, eliminating the intermediate step of clicking the "Delete Account" button to reveal the text field.

Additionally, create a constant const DELETE_ACCOUNT='delete-account' and use it to compare the entered text and also to display it as a part of the label message. This approach allows you to remove both useEffect hooks.

To use a constant in the Formated Message:

DeleteConfirmationDialog.js 

          <TextField
            autoFocus={true}
            label={
              <FormattedMessage
                {...messages.deleteAccountFinal}
                values={{ deleteAccount: 'delete-account' }}
              />
            }
           ......../>

People.messages.js

 deleteAccountFinal: {
    id: 'cboard.components.Settings.People.deleteAccountFinal',
    defaultMessage: 'To confirm, type "{deleteAccount}".',
  },

One more thing, ensure the confirmationText value is cleared when the dialog is closed.

src/components/Settings/People/DeleteConfirmationDialog.js Outdated Show resolved Hide resolved
@@ -67,6 +95,34 @@ const DeleteConfirmationDialog = ({
</Button>
</DialogActions>
)}
{!isDeletingAccount && showConfirmationInput && (
<>
<TextField
Copy link
Collaborator

Choose a reason for hiding this comment

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

This component needs to be inside the <DialogContent>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here I had a problem when I put the text field element inside DialogContent it shrinks and sticks to the left , also if its inside DialogActions , or outside of DialogContent in general it sticks to both sides of the dialog which doesn't look good , I know using inline css is not a best practice but I didn't know whether to create a new css file and if so where should I put 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.

also made sense to me to put it inside DialogActions because its kinda serves a role of a button , you know what I mean ? anyway its ok Im here to learn :)

Copy link
Collaborator

@RodriSanchez1 RodriSanchez1 Jun 1, 2024

Choose a reason for hiding this comment

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

I see!! Take a look on material ui documentation under form dialog here. The text field it's inside the dialog content. Use that as a reference. Also you can take a look on the text field props.
You can add the fullWidth prop to deal with your problem!

The css file is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh got it , I have committed the changes @RodriSanchez1

src/components/Settings/People/People.messages.js Outdated Show resolved Hide resolved
src/components/Settings/People/DeleteConfirmationDialog.js Outdated Show resolved Hide resolved
src/components/Settings/People/People.messages.js Outdated Show resolved Hide resolved
@nmro184 nmro184 requested a review from RodriSanchez1 June 1, 2024 13:47
@RodriSanchez1 RodriSanchez1 merged commit 295f0f6 into cboard-org:master Jul 22, 2024
5 checks passed
@RodriSanchez1 RodriSanchez1 linked an issue Jul 22, 2024 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add security step before delete user account
2 participants