fix: simplified error messages for invalid inputs at profile settings #2500
Conversation
…equired fields in user portal added more conditions to increase simplicity
WalkthroughThe pull request introduces new error messages across multiple localization files, including English, French, Hindi, Spanish, and Chinese. These messages enhance user feedback regarding input validation issues such as phone numbers, education grades, employment statuses, and marital statuses. Additionally, the error handling logic in the Changes
Assessment against linked issues
Possibly related PRs
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
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
public/locales/hi/errors.json (2)
15-15: Consider enhancing the 400 error message with more specific guidanceWhile the message is user-friendly, consider adding specific examples of what to check (e.g., "जैसे फ़ोन नंबर, शिक्षा आदि" - such as phone number, education, etc.) to make it even more helpful.
- "error400": "आपकी जानकारी सहेजी नहीं जा सकी। कृपया अपनी प्रविष्टियों की जांच करें और पुनः प्रयास करें।", + "error400": "आपकी जानकारी सहेजी नहीं जा सकी। कृपया अपनी प्रविष्टियों (जैसे फ़ोन नंबर, शिक्षा आदि) की जांच करें और पुनः प्रयास करें।",
16-16: Consider adding support contact information to default error messageThe default error message could be more helpful by including information about contacting support if the error persists.
- "defaultError": "कुछ गलत हो गया है। कृपया बाद में दोबारा प्रयास करें।" + "defaultError": "कुछ गलत हो गया है। कृपया बाद में दोबारा प्रयास करें। यदि समस्या बनी रहती है, तो कृपया सहायता से संपर्क करें।"public/locales/fr/errors.json (1)
11-14: Add periods for consistency with existing messages.The new validation messages should maintain consistency with the existing messages in the file that end with periods.
Apply this diff:
- "invalidPhoneNumber": "Veuillez entrer un numéro de téléphone valide", - "invalidEducationGrade": "Veuillez sélectionner un niveau d'études valide", - "invalidEmploymentStatus": "Veuillez sélectionner un statut d'emploi valide", - "invalidMaritalStatus": "Veuillez sélectionner un état matrimonial valide", + "invalidPhoneNumber": "Veuillez entrer un numéro de téléphone valide.", + "invalidEducationGrade": "Veuillez sélectionner un niveau d'études valide.", + "invalidEmploymentStatus": "Veuillez sélectionner un statut d'emploi valide.", + "invalidMaritalStatus": "Veuillez sélectionner un état matrimonial valide.",public/locales/en/errors.json (1)
11-14: Messages are clear and consistent, consider adding validation requirements.The validation messages are user-friendly and follow a consistent structure. However, they could be more helpful by including specific requirements.
Consider enhancing the messages with specific requirements:
- "invalidPhoneNumber": "Please enter a valid phone number", + "invalidPhoneNumber": "Please enter a valid phone number (e.g., +1-234-567-8900)", - "invalidEducationGrade": "Please select a valid education grade", + "invalidEducationGrade": "Please select an education grade from the provided options", - "invalidEmploymentStatus": "Please select a valid employment status", + "invalidEmploymentStatus": "Please select an employment status from the provided options", - "invalidMaritalStatus": "Please select a valid marital status", + "invalidMaritalStatus": "Please select a marital status from the provided options"src/utils/errorHandler.tsx (3)
13-13: Add type narrowing for better type safetyWhile the error message extraction works, consider adding explicit type narrowing for better type safety.
-const errorMessage = error.message; +const errorMessage = error.message || 'Unknown error occurred';
26-27: Enhance default error handling with more contextThe current default error handling could be more informative for debugging purposes.
Consider enhancing the default error handling:
- } else { - toast.error(tErrors('defaultError')); + } else { + // Log the unexpected error for debugging + console.debug('[ErrorHandler] Unhandled error:', errorMessage); + toast.error(tErrors('defaultError', { context: errorMessage.substring(0, 100) }));
Line range hint
1-31: Consider enhancing error message presentationWhile the current implementation improves error message clarity, consider these architectural improvements:
- Group related validation errors (e.g., all profile field validations) to show them together
- Implement progressive error display to show one error at a time
- Add error recovery suggestions where applicable
Example implementation approach:
interface ValidationError { field: string; message: string; suggestion?: string; } interface ErrorGroup { type: 'validation' | 'network' | 'server'; errors: ValidationError[]; } // Group errors by type and show them appropriately const showErrors = (errorGroup: ErrorGroup, tErrors: TFunction) => { switch(errorGroup.type) { case 'validation': // Show first error, with option to see more const [firstError, ...rest] = errorGroup.errors; toast.error(tErrors(firstError.message), { onClick: () => rest.length && showMoreErrors(rest) }); break; // ... handle other error types } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
public/locales/en/errors.json(1 hunks)public/locales/fr/errors.json(1 hunks)public/locales/hi/errors.json(1 hunks)public/locales/sp/errors.json(1 hunks)public/locales/zh/errors.json(1 hunks)src/utils/errorHandler.tsx(1 hunks)
🔇 Additional comments (8)
public/locales/zh/errors.json (2)
10-10: LGTM: Formatting is consistent
The formatting adjustment maintains consistency with the new entries while preserving the existing translation.
11-14: Validation messages are clear and consistent
The new validation messages are well-structured and use appropriate Chinese phrases. They maintain consistency by using "请选择" (please select) pattern throughout.
Let's verify consistency across all locale files:
✅ Verification successful
Let me try a different approach to verify the content of these files.
Validation messages are consistent across all locales
The validation messages for phone number, education grade, employment status, and marital status are properly translated and maintain consistent meaning across all language files (zh, sp, en, fr, hi). Each translation:
- Uses appropriate language-specific phrases for "please select/enter"
- Maintains the same validation context
- Follows similar grammatical structure within each language
- Conveys equivalent meaning across all locales
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of validation messages across all locales
# Expected: Similar structure and equivalent meanings across languages
# Find all error.json files and show the validation messages
fd 'errors.json' public/locales -x echo "=== {} ===" \; cat {} \; echo
Length of output: 462
Script:
#!/bin/bash
# Check content of each errors.json file individually
echo "=== Chinese (zh) ==="
cat public/locales/zh/errors.json
echo -e "\n=== Spanish (sp) ==="
cat public/locales/sp/errors.json
echo -e "\n=== English (en) ==="
cat public/locales/en/errors.json
echo -e "\n=== French (fr) ==="
cat public/locales/fr/errors.json
echo -e "\n=== Hindi (hi) ==="
cat public/locales/hi/errors.json
Length of output: 4906
public/locales/hi/errors.json (2)
11-14: Well-structured validation messages!
The validation messages are clear, polite, and actionable. They follow good UX practices by:
- Using consistent structure starting with "कृपया" (please)
- Clearly indicating what action the user needs to take
- Maintaining a helpful tone
11-16: Verify consistency across language files
Let's ensure these new error messages are consistently implemented across all language files.
✅ Verification successful
All error messages are consistently implemented across language files
All the required error message keys (invalidPhoneNumber, invalidEducationGrade, invalidEmploymentStatus, invalidMaritalStatus, error400, defaultError) are present and properly translated in all language files (en, fr, sp, zh). Each key has appropriate translations that convey the same meaning across languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all new error keys exist in other language files
# Expected: All keys should be present in en, fr, sp, and zh locale files
for key in "invalidPhoneNumber" "invalidEducationGrade" "invalidEmploymentStatus" "invalidMaritalStatus" "error400" "defaultError"; do
echo "Checking key: $key"
for lang in "en" "fr" "sp" "zh"; do
if [ -f "public/locales/$lang/errors.json" ]; then
echo "$lang:" $(jq ".$key" "public/locales/$lang/errors.json")
fi
done
done
Length of output: 7181
public/locales/en/errors.json (1)
10-10: LGTM!
The formatting adjustment maintains consistency while preserving the existing template variable usage.
public/locales/sp/errors.json (3)
11-14: LGTM! Well-structured validation messages
The new validation messages are clear, consistent, and follow good Spanish localization practices. They effectively guide users on how to correct their input.
16-16: LGTM! Clear and helpful default error message
The default error message is well-written, providing clear information without technical jargon and suggesting an appropriate action.
11-16: Verify translations across all language files
Let's ensure these new error messages are consistently implemented across all language files (en, fr, hi, zh).
✅ Verification successful
All error messages are properly translated across language files
The verification confirms that all the required error message keys (invalidPhoneNumber, invalidEducationGrade, invalidEmploymentStatus, invalidMaritalStatus, error400, defaultError) are present in all language files (en, fr, hi, zh). Each key returned true for every language, indicating complete coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all new error keys exist in other language files
# and verify their structure matches the Spanish version
# Define the required keys
KEYS=("invalidPhoneNumber" "invalidEducationGrade" "invalidEmploymentStatus" "invalidMaritalStatus" "error400" "defaultError")
# Check each language file
for lang in en fr hi zh; do
echo "Checking $lang translations..."
if [ -f "public/locales/$lang/errors.json" ]; then
for key in "${KEYS[@]}"; do
jq "has(\"$key\")" "public/locales/$lang/errors.json"
done
else
echo "Warning: Missing localization file for $lang"
fi
done
Length of output: 2896
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
public/locales/sp/errors.json (1)
16-16: Consider adding support contact informationThe default error message is clear and appropriate. Consider adding information about how users can get support if the error persists.
- "defaultError": "Algo salió mal. Inténtalo de nuevo más tarde." + "defaultError": "Algo salió mal. Inténtalo de nuevo más tarde o contacta a soporte si el problema persiste."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
public/locales/fr/errors.json(1 hunks)public/locales/sp/errors.json(1 hunks)public/locales/zh/errors.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locales/fr/errors.json
- public/locales/zh/errors.json
🔇 Additional comments (3)
public/locales/sp/errors.json (3)
11-14: LGTM! Well-structured validation messages
The validation messages are clear, consistent, and user-friendly. They follow a good pattern of politely instructing users what action to take.
15-15: LGTM! User-friendly error message
The HTTP 400 error message has been appropriately simplified, focusing on what users need to do rather than technical details.
11-16: Verify translations across all language files
Let's ensure these new error messages are consistently implemented across all language files.
✅ Verification successful
Let me get more detailed information about the content of these error messages across all language files.
All error messages are properly implemented across language files
The verification shows that all six error messages (invalidPhoneNumber, invalidEducationGrade, invalidEmploymentStatus, invalidMaritalStatus, error400, defaultError) are present and properly translated in all language files (en, fr, hi, sp, zh). Each translation maintains consistent meaning while being culturally appropriate for the target language.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all language files contain the new error messages
# Define the required keys
keys=("invalidPhoneNumber" "invalidEducationGrade" "invalidEmploymentStatus" "invalidMaritalStatus" "error400" "defaultError")
# Find all error.json files
fd 'errors.json' public/locales | while read -r file; do
echo "Checking $file..."
for key in "${keys[@]}"; do
if ! grep -q "\"$key\":" "$file"; then
echo "Missing key: $key in $file"
fi
done
done
Length of output: 3385
Script:
#!/bin/bash
# Get the actual content of all error messages from each language file
for file in $(fd 'errors.json' public/locales); do
echo "=== Content of $file ==="
cat "$file"
echo -e "\n"
done
Length of output: 5556
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2500 +/- ##
=====================================================
- Coverage 95.82% 83.80% -12.02%
=====================================================
Files 295 312 +17
Lines 7037 8127 +1090
Branches 1520 1837 +317
=====================================================
+ Hits 6743 6811 +68
- Misses 98 1177 +1079
+ Partials 196 139 -57 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/utils/errorHandler.test.tsx (2)
24-29: Improve test description and error message matchingThe test structure is good, but consider these improvements:
- Simplify the test description to "should handle invalid phone number errors"
- Consider using exact string matching instead of substring matching to prevent false positives
- it('should call toast.error with the correct message if error message contains this substring "Value is not a valid phone number"', () => { + it('should handle invalid phone number errors', () => {
54-59: Enhance HTTP 400 error test coverageThe test covers basic 400 error handling, but consider adding tests for:
- Different 400 error scenarios (e.g., validation errors, bad request format)
- Translation of error messages in different locales
Example addition:
test.each([ ['en', 'Bad Request'], ['fr', 'Requête Incorrecte'], ['hi', 'अमान्य अनुरोध'], ])('should handle 400 error in %s locale', (locale, expected) => { const error = new Error('Server responded with status code 400'); const tLocale = (key: string) => expected; errorHandler(tLocale, error); expect(toast.error).toHaveBeenCalledWith(expected); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/utils/errorHandler.test.tsx(1 hunks)
🔇 Additional comments (1)
src/utils/errorHandler.test.tsx (1)
Line range hint 1-85: Consider adding comprehensive test coverage
While the current tests cover the basic scenarios, consider adding:
- Edge cases for each validation type (e.g., empty strings, special characters)
- Tests for error message variations and partial matches
- Integration tests with the actual translation system
Let's verify the current test coverage:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/utils/errorHandler.test.tsx (1)
41-46: Consider adding more specific 400 error test cases.While this test covers the basic 400 error scenario, consider adding test cases for specific validation errors that might trigger a 400 response (e.g., missing required fields, invalid data formats).
Example addition:
test.each([ ['Missing required field', 'error400MissingField'], ['Invalid data format', 'error400InvalidFormat'], ['Validation failed', 'error400ValidationFailed'] ])('should handle specific 400 error: %s', (errorMessage, expectedKey) => { const error = new Error(`Server responded with status code 400: ${errorMessage}`); errorHandler(t, error); expect(toast.error).toHaveBeenCalledWith(tErrors(expectedKey)); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/utils/errorHandler.test.tsx(1 hunks)
🔇 Additional comments (2)
src/utils/errorHandler.test.tsx (2)
24-29: LGTM! Clear and focused test case.
The test case effectively validates the error handling for invalid phone numbers, following testing best practices with clear setup and assertions.
31-39: Excellent use of parameterized testing!
The implementation effectively reduces code duplication while maintaining clarity, exactly as suggested in the previous review.
|
Please fix the failing tests. Our test code coverage system will fail if any of these conditions occur:
We do this to ensure the overall reliability of the code base is maintained. This is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. |
|
Tests should be passing now. Added a screenshot of passing all the tests. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/utils/errorHandler.test.tsx (3)
14-22: Remove debug logging from test utility functionThe
console.logstatement in thetErrorsfunction appears to be debugging code. It's best to keep test utilities clean and free of debug statements.const tErrors: TFunction = ( key: string, options?: Record<string, unknown>, ) => { - if (options) { - console.log(`options are passed, but the function returns only ${key}`); - } return key; };
35-40: Remove debug logging from testThe test looks good but contains an unnecessary
console.logstatement that should be removed.it('should call toast.error with the correct message if error message contains this substring "Value is not a valid phone number"', () => { const error = new Error('This value is not a valid phone number'); errorHandler(t, error); - console.log(toast.error); expect(toast.error).toHaveBeenCalledWith(tErrors('invalidPhoneNumber')); });
52-57: Remove extra blank line for consistencyThe test logic is good, but there's an unnecessary blank line before the expect statement that should be removed for consistency with other test cases.
it('should call toast.error with the correct message if error message contains this substring "status code 400"', () => { const error = new Error('Server responded with status code 400'); errorHandler(t, error); - expect(toast.error).toHaveBeenCalledWith(tErrors('error400')); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/utils/errorHandler.test.tsx(1 hunks)src/utils/errorHandler.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/errorHandler.tsx
🔇 Additional comments (2)
src/utils/errorHandler.test.tsx (2)
24-26: LGTM! Good test hygiene
Adding jest.clearAllMocks() in beforeEach ensures proper test isolation.
42-50: LGTM! Excellent use of parameterized tests
Great implementation using test.each to reduce code duplication and improve maintainability.
|
My code changes are totally covered. Still the coverage is decreasing (maybe because of previous commits).Please let me know if it needs to be updated anywhere.
|
What kind of change does this PR introduce?
A bugfix.
Issue Number:
Fixes #2464
Snapshots/Videos:
https://www.loom.com/share/5e08e9042d43429788277eb8b30749ea?sid=c2f89415-db97-47e9-be29-0b99386cb2de
Passing all tests
Summary
While updating user profile at Talawa User Portal, user used not to get user friendly errors. I have simplified the error which will come one by one in a user friendly readable manner helping user to understand the fields they are missing/giving wrong data into. I have also added translation of errors.
Other information
Added a 400 handler too.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Tests