Add validation for phone number and PAN card on contribution form#447
Add validation for phone number and PAN card on contribution form#447
Conversation
WalkthroughThe changes introduce a consolidated event listener for the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const fields = [ | ||
| { | ||
| labelText: "Mobile Number", | ||
| regex: /^\d{10}$/, | ||
| errorMessage: "Please enter a valid 10-digit mobile number.", | ||
| }, | ||
| { | ||
| labelText: "Enter PAN Card Number", | ||
| regex: /^[A-Z]{5}[0-9]{4}[A-Z]$/, | ||
| errorMessage: "Please enter a valid PAN card number in the format ABCDE1234F.", | ||
| } | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving validation configuration maintainability and security
The current implementation has several areas for improvement:
- The regex patterns lack documentation explaining their rules
- Validation rules are hardcoded in JS, making them difficult to maintain
- Missing input sanitization before validation
Consider this improved structure:
const fields = [
{
labelText: "Mobile Number",
+ // Matches exactly 10 digits, no spaces or special characters
regex: /^\d{10}$/,
errorMessage: "Please enter a valid 10-digit mobile number.",
+ sanitize: (value) => value.replace(/\D/g, ''), // Remove non-digits
},
{
labelText: "Enter PAN Card Number",
+ // Format: 5 uppercase letters + 4 digits + 1 uppercase letter
regex: /^[A-Z]{5}[0-9]{4}[A-Z]$/,
errorMessage: "Please enter a valid PAN card number in the format ABCDE1234F.",
+ sanitize: (value) => value.toUpperCase().replace(/[^A-Z0-9]/g, ''), // Convert to uppercase and remove special chars
}
];📝 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 fields = [ | |
| { | |
| labelText: "Mobile Number", | |
| regex: /^\d{10}$/, | |
| errorMessage: "Please enter a valid 10-digit mobile number.", | |
| }, | |
| { | |
| labelText: "Enter PAN Card Number", | |
| regex: /^[A-Z]{5}[0-9]{4}[A-Z]$/, | |
| errorMessage: "Please enter a valid PAN card number in the format ABCDE1234F.", | |
| } | |
| ]; | |
| const fields = [ | |
| { | |
| labelText: "Mobile Number", | |
| // Matches exactly 10 digits, no spaces or special characters | |
| regex: /^\d{10}$/, | |
| errorMessage: "Please enter a valid 10-digit mobile number.", | |
| sanitize: (value) => value.replace(/\D/g, ''), // Remove non-digits | |
| }, | |
| { | |
| labelText: "Enter PAN Card Number", | |
| // Format: 5 uppercase letters + 4 digits + 1 uppercase letter | |
| regex: /^[A-Z]{5}[0-9]{4}[A-Z]$/, | |
| errorMessage: "Please enter a valid PAN card number in the format ABCDE1234F.", | |
| sanitize: (value) => value.toUpperCase().replace(/[^A-Z0-9]/g, ''), // Convert to uppercase and remove special chars | |
| } | |
| ]; |
| // Attach validation on form submission | ||
| if (form) { | ||
| form.addEventListener("submit", function (event) { | ||
| const value = input.value; | ||
| if (!field.regex.test(value)) { | ||
| event.preventDefault(); | ||
| alert(field.errorMessage); | ||
| input.focus(); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance validation UX and event handling
The current validation implementation has several UX and technical issues:
- Using alert() for error messages provides poor user experience
- No real-time validation feedback
- Multiple event listeners could be attached to the same form
- No cleanup of event listeners
Consider this improved implementation:
-if (form) {
- form.addEventListener("submit", function (event) {
- const value = input.value;
- if (!field.regex.test(value)) {
- event.preventDefault();
- alert(field.errorMessage);
- input.focus();
- }
- });
-}
+// Create error message element
+const errorSpan = document.createElement('span');
+errorSpan.className = 'error-message';
+errorSpan.style.color = 'red';
+errorSpan.style.display = 'none';
+input.parentNode.insertBefore(errorSpan, input.nextSibling);
+
+// Real-time validation
+input.addEventListener('input', validateField);
+
+// Form submission validation
+if (!form.hasAttribute('data-validation-attached')) {
+ form.setAttribute('data-validation-attached', 'true');
+ form.addEventListener('submit', function(event) {
+ const isValid = Array.from(form.querySelectorAll('input'))
+ .every(input => !input.getAttribute('aria-invalid'));
+
+ if (!isValid) {
+ event.preventDefault();
+ form.querySelector('[aria-invalid="true"]')?.focus();
+ }
+ });
+}
+
+function validateField() {
+ const value = field.sanitize ? field.sanitize(input.value) : input.value;
+ const isValid = field.regex.test(value);
+
+ input.setAttribute('aria-invalid', !isValid);
+ errorSpan.textContent = isValid ? '' : field.errorMessage;
+ errorSpan.style.display = isValid ? 'none' : 'block';
+}This implementation provides:
- Immediate visual feedback
- Accessible error messages
- Input sanitization
- Prevention of duplicate event listeners
Consider moving the validation logic to a separate module for better maintainability.
Committable suggestion skipped: line range outside the PR's diff.
| fields.forEach(field => { | ||
| // Select the label by labelText and find the associated input | ||
| const label = Array.from(document.querySelectorAll("label")).find(el => el.textContent.includes(field.labelText)); | ||
| if (label) { | ||
| const input = document.querySelector(`input[name="${label.getAttribute("for")}"]`); | ||
|
|
||
| if (input) { | ||
| const form = input.closest("form"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve field selection robustness and performance
The current field selection approach has potential issues:
- Inefficient DOM traversal using Array.from and find
- No error handling for missing form elements
- Possible race conditions with dynamically loaded forms
Consider this more robust approach:
-fields.forEach(field => {
- const label = Array.from(document.querySelectorAll("label")).find(el => el.textContent.includes(field.labelText));
- if (label) {
- const input = document.querySelector(`input[name="${label.getAttribute("for")}"]`);
-
- if (input) {
- const form = input.closest("form");
+fields.forEach(field => {
+ // More efficient selector
+ const label = document.querySelector(`label:contains("${field.labelText}")`);
+ if (!label) {
+ console.warn(`Validation field "${field.labelText}" not found in the form`);
+ return;
+ }
+
+ const input = document.getElementById(label.getAttribute("for"));
+ if (!input) {
+ console.warn(`Input for "${field.labelText}" not found`);
+ return;
+ }
+
+ const form = input.closest("form");
+ if (!form) {
+ console.warn(`Form not found for "${field.labelText}"`);
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
| // Add validation for the phone number and PAN card on contribution form. | ||
|
|
||
| document.addEventListener("DOMContentLoaded", function () { | ||
| // Define validation configurations for each field | ||
| const fields = [ | ||
| { | ||
| labelText: "Mobile Number", | ||
| regex: /^\d{10}$/, | ||
| errorMessage: "Please enter a valid 10-digit mobile number.", | ||
| }, | ||
| { | ||
| labelText: "Enter PAN Card Number", | ||
| regex: /^[A-Z]{5}[0-9]{4}[A-Z]$/, | ||
| errorMessage: "Please enter a valid PAN card number in the format ABCDE1234F.", | ||
| } | ||
| ]; | ||
|
|
||
| fields.forEach(field => { | ||
| // Select the label by labelText and find the associated input | ||
| const label = Array.from(document.querySelectorAll("label")).find(el => el.textContent.includes(field.labelText)); | ||
| if (label) { | ||
| const input = document.querySelector(`input[name="${label.getAttribute("for")}"]`); | ||
|
|
||
| if (input) { | ||
| const form = input.closest("form"); | ||
|
|
||
| // Attach validation on form submission | ||
| if (form) { | ||
| form.addEventListener("submit", function (event) { | ||
| const value = input.value; | ||
| if (!field.regex.test(value)) { | ||
| event.preventDefault(); | ||
| alert(field.errorMessage); | ||
| input.focus(); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Server-side validation is missing for sensitive data
The current implementation only includes client-side validation for PAN card and mobile numbers. While the client-side validation is well-structured, it's insufficient for sensitive data like PAN numbers. Here's what needs attention:
- No server-side validation found in the codebase for PAN card numbers
- Form submission endpoints are not implementing any validation checks
- Relying solely on client-side JavaScript validation which can be bypassed
The code should be enhanced with proper server-side validation before accepting these sensitive fields.
🔗 Analysis chain
Verify server-side validation implementation
While client-side validation is implemented, it's crucial to ensure proper server-side validation exists for these sensitive fields.
Let me search for the server-side form handling code to verify if proper validation exists.
Let me check for any server-side form processing code that might handle these validations.
Let me check the validation.js file and any related server-side processing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for server-side validation of mobile and PAN card numbers
# Look for validation in PHP files
echo "Searching for server-side validation..."
rg -t php "(mobile|phone|pan|PAN).*valid" ./wp-content
# Look for form processing endpoints
echo "Searching for form processing endpoints..."
rg -t php "contribution.*form" ./wp-content
Length of output: 31124
Script:
#!/bin/bash
# Search for form handling code in PHP files
echo "Searching for form handling code..."
rg -t php "material.*contribution.*form" ./wp-content/themes/goonj-crm --context 10
# Search for validation related code
echo "Searching for validation related code..."
rg -t php "(mobile|phone|pan).*valid" ./wp-content/themes/goonj-crm
Length of output: 2386
Script:
#!/bin/bash
# Search for form processing endpoints and validation logic
echo "Searching for form processing endpoints..."
rg -t php "add_action.*wp_ajax" ./wp-content/themes/goonj-crm/functions.php -A 10
# Look for any validation functions
echo "Searching for validation functions..."
rg -t php "function.*valid" ./wp-content/themes/goonj-crm
# Check if there's a dedicated validation file
echo "Checking for validation files..."
fd validation ./wp-content/themes/goonj-crm
Length of output: 589
Script:
#!/bin/bash
# Check the validation.js file content
echo "Checking validation.js content..."
cat ./wp-content/themes/goonj-crm/validation.js
# Look for form submission handling
echo "Searching for form submission handling..."
rg -t php "handle.*form.*submit" ./wp-content/themes/goonj-crm
# Check for any AJAX endpoints that might handle the form
echo "Searching for AJAX handlers..."
rg -t php "wp_ajax_(nopriv_)?(material|contribution)" ./wp-content/themes/goonj-crm
Length of output: 2052
…d-validation-on-contribution
…d-validation-on-contribution
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/themes/goonj-crm/main.js(1 hunks)
🔇 Additional comments (2)
wp-content/themes/goonj-crm/main.js (2)
69-82: Enhance validation configuration with sanitization and documentation
The validation configuration needs improvement in several areas:
- Add documentation for regex patterns
- Implement input sanitization
- Consider moving validation rules to a configuration file
This issue was previously identified. Please refer to the earlier review comment for the detailed implementation suggestion that includes input sanitization and improved documentation.
84-106:
Improve field selection and validation implementation
Previous review comments have already addressed the technical improvements needed for:
- More efficient field selection
- Better error handling
- Prevention of duplicate event listeners
- Real-time validation
Please refer to the earlier review comments for detailed implementation suggestions.
Additionally, there's a security concern:
- The current client-side-only validation can be easily bypassed
- Sensitive data like PAN numbers require server-side validation
Let's verify if there's any server-side validation:
#!/bin/bash
# Search for server-side validation in PHP files
echo "Checking for PAN and mobile validation in server-side code..."
rg -t php "(pan|mobile|phone).*valid" ./wp-content
# Look for form submission handlers
echo "Checking form submission handlers..."
rg -t php "add_action.*wp_ajax.*contribution" ./wp-content|
|
||
| // Add validation for the phone number and PAN card on contribution form. | ||
|
|
||
| document.addEventListener("DOMContentLoaded", function () { | ||
| // Define validation configurations for each field | ||
| const fields = [ | ||
| { | ||
| labelText: "Mobile Number", | ||
| regex: /^\d{10}$/, | ||
| errorMessage: "Please enter a valid 10-digit mobile number.", | ||
| }, | ||
| { | ||
| labelText: "PAN Card Number", | ||
| regex: /^[A-Z]{5}[0-9]{4}[A-Z]$/, | ||
| errorMessage: "Please enter a valid PAN card number in the format ABCDE1234F.", | ||
| } | ||
| ]; | ||
|
|
||
| fields.forEach(field => { | ||
| // Select the label by labelText and find the associated input | ||
| const label = Array.from(document.querySelectorAll("label")).find(el => el.textContent.includes(field.labelText)); | ||
| if (label) { | ||
| const input = document.querySelector(`input[name="${label.getAttribute("for")}"]`); | ||
|
|
||
| if (input) { | ||
| const form = input.closest("form"); | ||
|
|
||
| // Attach validation on form submission | ||
| if (form) { | ||
| form.addEventListener("submit", function (event) { | ||
| const value = input.value; | ||
| if (!field.regex.test(value)) { | ||
| event.preventDefault(); | ||
| alert(field.errorMessage); | ||
| input.focus(); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate event listeners and track temporary solutions
The file structure needs improvement:
- Multiple
DOMContentLoadedevent listeners should be consolidated into a single listener - The temporary form reset solution needs proper tracking
Consider this structure:
document.addEventListener('DOMContentLoaded', function() {
+ // Message handling
handleUrlMessages();
+
+ // Form reset handling
handleFormReset();
+
+ // Form validation
setupFormValidation();
});
+// TODO: Replace with proper form reset solution
+// Issue: #<create-tracking-issue>
function handleFormReset() {
// ... reset logic ...
}Would you like me to create a tracking issue for replacing the temporary form reset solution?
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@tarunnjoshi this is a good suggestion which i think we should implement before it's too late
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
pokhiii
left a comment
pokhiii
left a comment
There was a problem hiding this comment.
I am just echoing one of the suggestions given by coderabbit regarding refactoring
2 participants
Add validation for phone number and PAN card on contribution form
Summary by CodeRabbit
New Features
Bug Fixes