fix: Password validation on account #1282
Conversation
peterpardo
changed the title
WalkthroughThe changes in this pull request focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
Client/src/Components/TabPanels/Account/PasswordPanel.jsx (3)
15-19: Mom's spaghetti time! Let's freeze that default state! 🍝The default state object should be frozen to prevent accidental mutations.
-const defaultPasswordsState = { +const defaultPasswordsState = Object.freeze({ password: "", newPassword: "", confirm: "", -}; +});
Line range hint
76-103: Cleanup after success, just like mom taught us! 🍝Two issues need attention in the submit handler:
- The validation context issue persists here
- The touchedFields state isn't reset after successful submission
if (error) { const newErrors = {}; error.details.forEach((err) => { newErrors[err.path[0]] = err.message; }); setErrors(newErrors); } else { const action = await dispatch(update({ authToken, localData })); if (action.payload.success) { createToast({ body: "Your password was changed successfully.", }); setLocalData({ password: "", newPassword: "", confirm: "", }); + setTouchedFields({ + password: false, + newPassword: false, + confirm: false, + }); + setErrors({}); }
235-236: Let's make that disabled condition more readable! 🍝The button's disabled condition could be more explicit.
-disabled={ - Object.keys(errors).length > 0 || - Object.values(localData).filter((value) => value === "").length > 0 -} +disabled={ + Object.keys(errors).length > 0 || + Object.values(localData).some(value => !value) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
Client/src/Components/TabPanels/Account/PasswordPanel.jsx(3 hunks)Client/src/Validation/error.js(2 hunks)
| const getTouchedFieldErrors = (validation, touchedErrors) => { | ||
| let newErrors = {}; | ||
|
|
||
| if (validation?.error) { | ||
| newErrors = validation.error.details.reduce((errors, detail) => { | ||
| const fieldId = detail.path[0]; | ||
| if (touchedErrors[fieldId] && !(fieldId in errors)) { | ||
| errors[fieldId] = detail.message; | ||
| } | ||
| return errors; | ||
| }, {}); | ||
| } | ||
|
|
||
| return newErrors; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo dawg, let's make this function more robust! 🍝
The function needs some defensive programming to handle edge cases and improve efficiency.
Here's a more robust implementation:
const getTouchedFieldErrors = (validation, touchedErrors) => {
+ if (!validation?.error?.details || !touchedErrors) {
+ return {};
+ }
- let newErrors = {};
-
- if (validation?.error) {
- newErrors = validation.error.details.reduce((errors, detail) => {
- const fieldId = detail.path[0];
- if (touchedErrors[fieldId] && !(fieldId in errors)) {
- errors[fieldId] = detail.message;
- }
- return errors;
- }, {});
- }
-
- return newErrors;
+ return validation.error.details.reduce((errors, detail) => {
+ const fieldId = detail.path?.[0];
+ if (fieldId && touchedErrors[fieldId] && !(fieldId in errors)) {
+ errors[fieldId] = detail.message;
+ }
+ return errors;
+ }, {});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getTouchedFieldErrors = (validation, touchedErrors) => { | |
| let newErrors = {}; | |
| if (validation?.error) { | |
| newErrors = validation.error.details.reduce((errors, detail) => { | |
| const fieldId = detail.path[0]; | |
| if (touchedErrors[fieldId] && !(fieldId in errors)) { | |
| errors[fieldId] = detail.message; | |
| } | |
| return errors; | |
| }, {}); | |
| } | |
| return newErrors; | |
| }; | |
| const getTouchedFieldErrors = (validation, touchedErrors) => { | |
| if (!validation?.error?.details || !touchedErrors) { | |
| return {}; | |
| } | |
| return validation.error.details.reduce((errors, detail) => { | |
| const fieldId = detail.path?.[0]; | |
| if (fieldId && touchedErrors[fieldId] && !(fieldId in errors)) { | |
| errors[fieldId] = detail.message; | |
| } | |
| return errors; | |
| }, {}); | |
| }; |
| const validation = credentials.validate( | ||
| { [name]: value }, | ||
| { abortEarly: false, context: { password: localData.newPassword } } | ||
| { ...updatedData }, | ||
| { abortEarly: false, context: { password: updatedData.newPassword } } |
There was a problem hiding this comment.
Critical: Fix the password confirmation validation! 🍝
The validation context is not properly handling the password confirmation match. The current implementation passes the new password as context, but it should also validate against the confirm field.
const validation = credentials.validate(
{ ...updatedData },
- { abortEarly: false, context: { password: updatedData.newPassword } }
+ {
+ abortEarly: false,
+ context: {
+ password: updatedData.newPassword,
+ confirmPassword: updatedData.confirm
+ }
+ }
);This change requires updating the Joi validation schema to check if confirm matches newPassword when both fields are touched.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Great work @peterpardo, thank you again for your contribution!
| * @param {Object} touchedErrors - An object representing which fields have been interacted with. Keys are field IDs (field names), and values are booleans indicating whether the field has been touched. | ||
| * @returns {Object} - An object where keys are the field IDs (if they exist in `touchedErrors` and are in the error details) and values are their corresponding error messages. | ||
| */ | ||
| const getTouchedFieldErrors = (validation, touchedErrors) => { |
There was a problem hiding this comment.
Nicely done, really appreciate the JSdocs
Summary
This PR fixes the issue for the password validation under
Account > PasswordChanges Made
getTouchedFieldErrors()util function for errorsRelated Issue/s
Fixes #1274
Screenshots / Demo Video
checkmate-issue-1274-demo-1.mp4
Additional Context
N/A