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

fix: Password validation on account #1282

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 33 additions & 21 deletions Client/src/Components/TabPanels/Account/PasswordPanel.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ import Alert from "../../Alert";
import { update } from "../../../Features/Auth/authSlice";
import { useDispatch, useSelector } from "react-redux";
import { createToast } from "../../../Utils/toastUtils";
import { getTouchedFieldErrors } from "../../../Validation/error";

const defaultPasswordsState = {
password: "",
newPassword: "",
confirm: "",
};

/**
* PasswordPanel component manages the form for editing password.
Expand All @@ -30,35 +37,40 @@ const PasswordPanel = () => {
"edit-confirm-password": "confirm",
};

const [localData, setLocalData] = useState({
password: "",
newPassword: "",
confirm: "",
const [localData, setLocalData] = useState(defaultPasswordsState);
const [errors, setErrors] = useState(defaultPasswordsState);
const [touchedFields, setTouchedFields] = useState({
password: false,
newPassword: false,
confirm: false,
});
const [errors, setErrors] = useState({});

const handleChange = (event) => {
const { value, id } = event.target;
const name = idToName[id];
setLocalData((prev) => ({
...prev,

const updatedData = {
...localData,
[name]: value,
}));
};
const updatedTouchedFields = {
...touchedFields,
[name]: true,
};

const validation = credentials.validate(
{ [name]: value },
{ abortEarly: false, context: { password: localData.newPassword } }
{ ...updatedData },
{ abortEarly: false, context: { password: updatedData.newPassword } }
Comment on lines 61 to +63
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

);

setErrors((prev) => {
const updatedErrors = { ...prev };
const updatedErrors = getTouchedFieldErrors(validation, updatedTouchedFields);

if (validation.error) {
updatedErrors[name] = validation.error.details[0].message;
} else {
delete updatedErrors[name];
}
return updatedErrors;
});
if (!touchedFields[name]) {
setTouchedFields(updatedTouchedFields);
}

setLocalData(updatedData);
setErrors(updatedErrors);
};

const handleSubmit = async (event) => {
Expand Down Expand Up @@ -220,8 +232,8 @@ const PasswordPanel = () => {
loading={isLoading}
loadingIndicator="Saving..."
disabled={
Object.keys(errors).length !== 0 ||
Object.values(localData).filter((value) => value !== "").length === 0
Object.keys(errors).length > 0 ||
Object.values(localData).filter((value) => value === "").length > 0
}
sx={{
px: theme.spacing(12),
Expand Down
27 changes: 26 additions & 1 deletion Client/src/Validation/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,31 @@ const buildErrors = (prev, id, error) => {
return updatedErrors;
};

/**
* Processes Joi validation errors and returns a filtered object of error messages for fields that have been touched.
*
* @param {Object} validation - The Joi validation result object.
* @param {Object} validation.error - The error property of the validation result containing details of validation failures.
* @param {Object[]} validation.error.details - An array of error details from the Joi validation. Each item contains information about the path and the message.
* @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) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nicely done, really appreciate the JSdocs

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;
};
Comment on lines +20 to +34
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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 hasValidationErrors = (form, validation, setErrors) => {
const { error } = validation.validate(form, {
abortEarly: false,
Expand Down Expand Up @@ -53,4 +78,4 @@ const hasValidationErrors = (form, validation, setErrors) => {
}
return false;
};
export { buildErrors, hasValidationErrors };
export { buildErrors, hasValidationErrors, getTouchedFieldErrors };