Skip to content

Added account management features#58

Merged
Prakhar896 merged 10 commits intomainfrom
junhan
Jul 19, 2024
Merged

Added account management features#58
Prakhar896 merged 10 commits intomainfrom
junhan

Conversation

@JunHammy
Copy link
Copy Markdown
Contributor

What was done in this PR

  1. Added updateAccountDetails
  2. Added deleteAccount
  3. Added changePassword

New features description

  1. All changing and updating of account data includes thorough validations
  2. Updating of account details checks if the inputs are unique and in the correct format
  3. Delete account removes the entire row of data from the database
  4. Change password checks if the current password input matches the password of the user

@JunHammy JunHammy requested a review from Prakhar896 July 14, 2024 03:21
@JunHammy JunHammy changed the title Junhan Added account management features Jul 14, 2024
Copy link
Copy Markdown
Contributor

@Prakhar896 Prakhar896 left a comment

Choose a reason for hiding this comment

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

Generally well-written; please remember that for caught exceptions in a try-catch are meant to be Logger.loged with the error message of the caught exception. In all other scenarios, there should just be a ERROR or UERROR response to the client, no console.logs.

Comment thread routes/identity/CreateAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
@JunHammy JunHammy requested a review from Prakhar896 July 19, 2024 02:05
Copy link
Copy Markdown
Contributor

@Prakhar896 Prakhar896 left a comment

Choose a reason for hiding this comment

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

Please try to keep in mind the whole project when coding; high cognitive and logical reasoning is a requirement to write good, reliable and secure code.

Comment thread routes/identity/CreateAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
Comment thread routes/identity/myAccount.js Outdated
@JunHammy JunHammy requested a review from Prakhar896 July 19, 2024 09:40

try {
await changePasswordSchema.validate(req.body);
const data = await changePasswordSchema.validate(req.body, { abortEarly: false });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If validation fails, yup will throw an array of errors (because you configured it to not abortEarly) which will be caught by your catch block. But, your catch block is considering it to be an internal server error, which is misleading. Must be changed in the next PR. @JunHammy

Copy link
Copy Markdown
Contributor

@Prakhar896 Prakhar896 left a comment

Choose a reason for hiding this comment

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

LGTM

@Prakhar896 Prakhar896 merged commit 6f473cc into main Jul 19, 2024
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.

2 participants