Added handling for invalid phone numbers (emails) #3449
Conversation
|
WalkthroughThe Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/backoffice-v2/src/common/utils/get-phone-number-formatter/get-phone-number-formatter.unit.test.ts (1)
1-44: Expand test coverage to include more edge cases and validation scenarios.The current tests cover basic scenarios but lack comprehensive edge case testing for the new validation logic. Consider expanding the test suite to ensure robustness.
Add these additional test cases to improve coverage:
+ describe('when invalid phone formats are passed', () => { + it.each([ + ['empty string', ''], + ['just plus sign', '+'], + ['just hyphens', '---'], + ['email with numbers', '[email protected]'], + ['starts with hyphen', '-1234567890'], + ['ends with hyphen', '1234567890-'], + ['double hyphens', '123--456-7890'], + ['hyphen before plus', '123-+456'], + ])('should return undefined for %s', (_, value) => { + const result = getPhoneNumberFormatter(value); + expect(result).toBeUndefined(); + }); + }); + + describe('when valid phone formats are passed', () => { + it.each([ + ['US format without country code', '1234567890'], + ['international with spaces', '+1 234 567 8900'], + ['with parentheses', '+1 (234) 567-8900'], + ['UK format', '+44 20 7946 0958'], + ])('should return a formatter for %s', (_, value) => { + const result = getPhoneNumberFormatter(value); + expect(result).toBeDefined(); + }); + }); + + describe('when testing actual formatting', () => { + it('should format phone numbers correctly', () => { + const result = getPhoneNumberFormatter('+972-052-625-3312'); + expect(result?.formatInternational()).toBe('+972 52-625-3312'); + }); + });The current tests also don't verify that the formatted output is correct - only that something is returned. Consider testing the actual formatting behavior to ensure the function works end-to-end.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/backoffice-v2/src/common/utils/get-phone-number-formatter/get-phone-number-formatter.ts(1 hunks)apps/backoffice-v2/src/common/utils/get-phone-number-formatter/get-phone-number-formatter.unit.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`apps/backoffice-v2/**/*.{ts,tsx}`: Use functional components with TypeScript. Implement smart/dumb component pattern. Place components in feature-based directories. Use compound c...
apps/backoffice-v2/**/*.{ts,tsx}: Use functional components with TypeScript.
Implement smart/dumb component pattern.
Place components in feature-based directories.
Use compound components for complex UIs.
Follow atomic design principles.
Use React Query for server state and API calls.
Use Context for shared state.
Implement state machines for complex flows.
Use local state for UI-only state.
Follow unidirectional data flow.
Use strict TypeScript configuration.
Define interfaces for all props.
Use discriminated unions for state.
Leverage type inference.
Use Radix UI for accessible components.
Implement proper ARIA attributes.
Follow consistent styling patterns.
Use composition over inheritance.
Keep components small and focused.
Use React Hook Form for forms.
Implement Zod for validation.
Handle form submission states.
Show validation feedback.
Use controlled inputs when needed.
Implement proper loading states for data fetching.
Handle error states gracefully.
Cache responses appropriately.
Type API responses.
Use error boundaries.
Implement fallback UI.
Handle async errors.
Show user-friendly error messages.
Log errors appropriately.
Use React.memo wisely.
Implement proper code splitting.
Use lazy loading for routes.
Optimize re-renders.
Profile performance regularly.
Write unit tests for components.
Use React Testing Library.
Mock external dependencies in tests.
Maintain good test coverage.
Follow feature-based organization.
Use index files for exports.
Keep related files together.
Use consistent naming.
Implement barrel exports.
Use Tailwind CSS for styling.
Follow utility-first approach for styling.
Use CSS variables for theming.
Keep styles maintainable.
Use CSS modules when needed.
Document complex logic.
Write clear component documentation.
Keep documentation up to date.
Use JSDoc when helpful.
Follow ESLint rules.
Use consistent formatting.
Write clear variable names.
Keep functions pure.
Use meaningful types.
Validate user input.
Implement proper authentication.
Handle sensitive data carefully.
Follow security best practices.
Use HTTPS for API calls.
Follow WCAG guidelines for accessibility.
Use semantic HTML.
Test with screen readers.
Ensure keyboard navigation.
Provide proper focus management.
apps/backoffice-v2/src/common/utils/get-phone-number-formatter/get-phone-number-formatter.unit.test.tsapps/backoffice-v2/src/common/utils/get-phone-number-formatter/get-phone-number-formatter.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Analyze (javascript)
- GitHub Check: test_linux
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: format
| if (!/^(?!-)(?!\+?$)(?!-+$)(?!.*-\+)(?!.*--)\+?[\d-]+(?<!-)$/.test(value)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Verification correct
Simplify the complex regex validation and add documentation.
The regex pattern is extremely complex with multiple negative lookaheads and lookbehinds, making it difficult to understand, maintain, and verify for correctness. This could lead to maintenance issues and potential bugs.
Consider these improvements:
- Add JSDoc documentation explaining the validation rules:
/**
* Wraps {@link parsePhoneNumber} with try catch.
+ * Validates input format before parsing to reject non-phone number strings.
* @param value
+ * @returns PhoneNumber instance or undefined if invalid/unparseable
*/- Simplify the regex or use a more readable approach:
- if (!/^(?!-)(?!\+?$)(?!-+$)(?!.*-\+)(?!.*--)\+?[\d-]+(?<!-)$/.test(value)) {
+ // Basic validation: should contain only digits, hyphens, and optional leading plus
+ // Reject obvious non-phone formats like emails
+ if (!/^\+?[\d\s\-\(\)]+$/.test(value) || value.includes('@')) {- Extract validation logic to a separate function for better testability:
+const isValidPhoneNumberFormat = (value: string): boolean => {
+ // Reject emails and other obvious non-phone formats
+ if (value.includes('@') || value.includes('.com')) return false;
+
+ // Allow only phone-like characters
+ return /^\+?[\d\s\-\(\)]+$/.test(value);
+};
export const getPhoneNumberFormatter = (value: string) => {
try {
- if (!/^(?!-)(?!\+?$)(?!-+$)(?!.*-\+)(?!.*--)\+?[\d-]+(?<!-)$/.test(value)) {
+ if (!isValidPhoneNumberFormat(value)) {Let me verify the current regex behavior with edge cases:
🏁 Script executed:
#!/bin/bash
# Test the regex against various phone number formats to ensure it works correctly
cat > test_regex.js << 'EOF'
const regex = /^(?!-)(?!\+?$)(?!-+$)(?!.*-\+)(?!.*--)\+?[\d-]+(?<!-)$/;
const testCases = [
// Should pass
'+1234567890',
'1234567890',
'+1-234-567-890',
'+972-052-625-3312',
// Should fail
'[email protected]',
'[email protected]',
'+',
'-',
'--',
'+-123',
'123-+456',
'123--456',
'123-',
'-123'
];
testCases.forEach(test => {
const result = regex.test(test);
console.log(`"${test}": ${result}`);
});
EOF
node test_regex.jsLength of output: 278
Simplify the complex regex validation and add documentation.
The regex pattern is extremely complex with multiple negative lookaheads and lookbehinds, making it difficult to understand, maintain, and verify for correctness. This could lead to maintenance issues and potential bugs.
Consider these improvements:
- Add JSDoc documentation explaining the validation rules:
/**
* Wraps {@link parsePhoneNumber} with try catch.
+ * Validates input format before parsing to reject non-phone number strings.
* @param value
+ * @returns PhoneNumber instance or undefined if invalid/unparseable
*/- Simplify the regex or use a more readable approach:
- if (!/^(?!-)(?!\+?$)(?!-+$)(?!.*-\+)(?!.*--)\+?[\d-]+(?<!-)$/.test(value)) {
+ // Basic validation: should contain only digits, hyphens, and optional leading plus
+ // Reject obvious non-phone formats like emails
+ if (!/^\+?[\d\s\-\(\)]+$/.test(value) || value.includes('@')) {- Extract validation logic to a separate function for better testability:
+const isValidPhoneNumberFormat = (value: string): boolean => {
+ // Reject emails and other obvious non-phone formats
+ if (value.includes('@') || value.includes('.com')) return false;
+
+ // Allow only phone-like characters
+ return /^\+?[\d\s\-\(\)]+$/.test(value);
+};
export const getPhoneNumberFormatter = (value: string) => {
try {
- if (!/^(?!-)(?!\+?$)(?!-+$)(?!.*-\+)(?!.*--)\+?[\d-]+(?<!-)$/.test(value)) {
+ if (!isValidPhoneNumberFormat(value)) {Let me verify the current regex behavior with edge cases:
#!/bin/bash
# Test the regex against various phone number formats to ensure it works correctly
cat > test_regex.js << 'EOF'
const regex = /^(?!-)(?!\+?$)(?!-+$)(?!.*-\+)(?!.*--)\+?[\d-]+(?<!-)$/;
const testCases = [
// Should pass
'+1234567890',
'1234567890',
'+1-234-567-890',
'+972-052-625-3312',
// Should fail
'[email protected]',
'[email protected]',
'+',
'-',
'--',
'+-123',
'123-+456',
'123--456',
'123-',
'-123'
];
testCases.forEach(test => {
const result = regex.test(test);
console.log(`"${test}": ${result}`);
});
EOF
node test_regex.js🤖 Prompt for AI Agents
In
apps/backoffice-v2/src/common/utils/get-phone-number-formatter/get-phone-number-formatter.ts
around lines 9 to 11, the current regex for phone number validation is overly
complex and hard to maintain. Refactor by extracting the validation logic into a
separate well-named function, simplify the regex to cover the necessary
validation rules more clearly, and add JSDoc comments explaining the validation
criteria and expected input formats for better readability and maintainability.
Summary by CodeRabbit