fix: improve onboarding, error handling, and add password reset#168
fix: improve onboarding, error handling, and add password reset#168Benbenzhouz wants to merge 13 commits intomainfrom
Conversation
…ogle OAuth error handling
- Fix unsafe return type in twilio.module.ts - Remove unnecessary condition checks in auth.controller.ts - Fix type definitions in google.strategy.ts - Add eslint-disable comments for console statements - Fix type assertions in auth.controller.ts
- Use 'as unknown as Twilio.Twilio' for mock client type assertion - Add eslint-disable comment for unsafe return type - Fixes TS2740 error for missing properties in mock object
- Add return types to mock Twilio client functions - Replace || with ?? operators in auth.controller.ts - Fix unnecessary optional chain in google.strategy.ts - Add eslint-disable for console.warn in google.strategy.ts - Use non-null assertion for validated profile fields
- Fix unnecessary optional chain error - Add non-null assertion since emails[0] is validated in the check above
- Replace profile?.emails?.[0] with explicit checks - profile parameter is not optional, so optional chain is unnecessary - Fixes @typescript-eslint/no-unnecessary-condition error
…eter - profile is a required parameter, so !profile check is unnecessary - Use profile.emails.length === 0 instead of !profile.emails[0] - Destructure name and emails directly from profile after validation - Fixes @typescript-eslint/no-unnecessary-condition error
- Add apiVersion to Stripe initialization for consistency - Fix street address parsing for 3-part and 4-part address formats - Add explicit demo step ID check (step 6) for skip action - Improve address parsing error message to show missing fields
- Create unified app-config.ts utility for frontend URL and CORS origin - Support multiple environment variable names for flexibility: - APP_URL, FRONTEND_URL, NEXT_PUBLIC_APP_URL (for frontend URL) - CORS_ORIGIN, FRONTEND_URL, APP_URL (for CORS origin) - Update all files to use centralized config functions - Improve compatibility with different environment variable naming conventions Files updated: - src/utils/app-config.ts (new) - src/main.ts - src/modules/auth/auth.controller.ts - src/modules/stripe/stripe.service.ts - src/modules/google-calendar/calendar-oauth.controller.ts
- Use only APP_URL for frontend URL (remove FRONTEND_URL, NEXT_PUBLIC_APP_URL) - Use only CORS_ORIGIN for CORS (remove FRONTEND_URL, APP_URL fallbacks) - Simplify code by using single environment variable names
- Add import statement for getAppUrl from @/utils/app-config - Fixes TypeScript error: Cannot find name 'getAppUrl'
- Add forgot-password endpoint - Add reset-password endpoint - Add ResetPasswordDto - Update auth service with password reset logic
There was a problem hiding this comment.
Pull request overview
This pull request improves the onboarding flow, enhances error handling for OAuth authentication, and adds password reset functionality. The changes centralize configuration management and add graceful degradation for third-party services in development environments.
Key Changes
- Enhanced address parsing with multiple regex patterns and manual fallback logic for Australian addresses
- Improved error handling for Google OAuth with validation of profile data and better error responses
- Added development-mode graceful degradation for Stripe, Google OAuth, and Twilio services with dummy credentials and warning messages
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/app-config.ts | New utility for centralized configuration management of APP_URL and CORS_ORIGIN |
| src/modules/stripe/stripe.service.ts | Added constructor with Stripe API version configuration and development fallback |
| src/modules/onboarding/onboarding.service.ts | Enhanced address parsing with multiple patterns, improved demo step handling, and better validation |
| src/modules/health/health.controller.ts | Formatting improvements for API documentation decorators |
| src/modules/google-calendar/calendar-oauth.controller.ts | Updated to use centralized getAppUrl() utility |
| src/modules/company/schema/company.schema.ts | Removed trailing whitespace |
| src/modules/auth/strategies/google.strategy.ts | Added validation for Google profile data and development mode fallback credentials |
| src/modules/auth/dto/reset-password.dto.ts | Removed trailing newline |
| src/modules/auth/auth.service.ts | Reorganized imports for consistency |
| src/modules/auth/auth.controller.ts | Enhanced OAuth error handling and added missing import for password reset |
| src/main.ts | Updated to use centralized getCorsOrigin() utility |
| src/lib/twilio/twilio.module.ts | Added development mode mock client with warning messages |
Comments suppressed due to low confidence (1)
src/modules/onboarding/onboarding.service.ts:119
- There's an inconsistency in storing the address. In the manual parsing paths (lines 78 and 101), the code stores
answer(the original untrimmed input) in 'answers.user.address.full'. However, at line 119 in the regex path, it also storesanswer(untrimmed). For consistency, all paths should storetrimmedAnswersince that's what was actually parsed and validated. Storing the untrimmed version could lead to inconsistencies in how addresses are displayed or compared.
update.$set['answers.user.address.full'] = answer;
return;
}
// Try to extract state and postcode separately
if (parts.length >= 4) {
const stateMatch = /^([A-Z]{2,3})$/.exec(parts[parts.length - 2]);
const postcodeMatch = /^(\d{4})$/.exec(parts[parts.length - 1]);
if (stateMatch && postcodeMatch) {
// Format: "Street, Suburb, State, Postcode" (4 parts) or more
// For 4 parts: parts[0] is street, parts[1] is suburb
// For more parts: join all but last 3 as street
const streetAddress =
parts.length === 4 ? parts[0] : parts.slice(0, -3).join(', ');
const suburb = parts[parts.length - 3];
const state = stateMatch[1];
const postcode = postcodeMatch[1];
update.$set['answers.user.address.streetAddress'] = streetAddress;
update.$set['answers.user.address.suburb'] = suburb;
update.$set['answers.user.address.state'] = state;
update.$set['answers.user.address.postcode'] = postcode;
update.$set['answers.user.address.full'] = answer;
return;
}
}
}
// If all parsing attempts fail, throw error
throw new BadRequestException(
'Unable to parse address; please check the format. Expected format: "Street Address, Suburb, State Postcode" (e.g., "123 Main St, Sydney, NSW 2000")',
);
}
// Use regex match results
update.$set['answers.user.address.streetAddress'] =
match.groups.street.trim();
update.$set['answers.user.address.suburb'] = match.groups.suburb.trim();
update.$set['answers.user.address.state'] = match.groups.state;
update.$set['answers.user.address.postcode'] = match.groups.postcode;
update.$set['answers.user.address.full'] = answer; // raw string
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if user is authenticated | ||
| if (!req.user) { | ||
| const frontendUrl = getAppUrl(); | ||
| res.redirect(`${frontendUrl}/login?error=oauth_failed`); |
There was a problem hiding this comment.
The error handling improvement for missing authentication is good, but the error query parameter 'oauth_failed' is generic and doesn't provide specific information about what failed. Consider using more descriptive error codes like 'auth_missing' or providing additional context to help users understand what went wrong.
| res.redirect(`${frontendUrl}/login?error=oauth_failed`); | |
| res.redirect(`${frontendUrl}/login?error=auth_missing`); |
| // Manually construct safe user object to preserve ObjectId | ||
| const safeUser = { | ||
| _id: user._id?.toString() ?? user._id, | ||
| email: user.email ?? '', | ||
| firstName: user.firstName, | ||
| lastName: user.lastName, | ||
| role: user.role as EUserRole, | ||
| status: user.status as UserStatus, | ||
| _id: user._id?.toString() ?? (user._id as string | undefined) ?? '', | ||
| email: (user.email as string | undefined) ?? '', | ||
| firstName: (user.firstName as string | undefined) ?? '', | ||
| lastName: (user.lastName as string | undefined) ?? '', | ||
| role: (user.role as EUserRole | undefined) ?? EUserRole.user, | ||
| status: (user.status as UserStatus | undefined) ?? UserStatus.active, |
There was a problem hiding this comment.
The default values being assigned here may mask actual authentication issues. For instance, defaulting role to EUserRole.user and status to UserStatus.active when these values are missing could allow users to proceed with incomplete profile data from OAuth. Consider whether these defaults are appropriate or if the authentication should fail when critical user data is missing.
| if (parts.length >= 2 && parts[1]) { | ||
| const key = parts[1]; | ||
| await this.userService.patch(userId, { [key]: answer.trim() }); |
There was a problem hiding this comment.
The condition checks parts.length >= 2 && parts[1] but doesn't verify that parts[1] is a valid/expected user field key. If the field format is malformed or contains unexpected data, this could result in setting incorrect user properties. Consider adding validation for the key value before using it to update user data.
| if (parts.length >= 2 && parts[1]) { | |
| const key = parts[1]; | |
| await this.userService.patch(userId, { [key]: answer.trim() }); | |
| if (parts.length >= 2 && parts[0] === 'user' && parts[1]) { | |
| const key = parts[1]; | |
| const allowedUserFields = ['phone', 'position']; | |
| if (allowedUserFields.includes(key)) { | |
| await this.userService.patch(userId, { [key]: answer.trim() }); | |
| } |
| 'user.address.full': function (answer, update) { | ||
| const match = this.AU_ADDR_REGEX.exec(answer.trim()); | ||
| const trimmedAnswer = answer.trim(); | ||
|
|
||
| // Try multiple regex patterns | ||
| let match = this.AU_ADDR_REGEX_STRICT.exec(trimmedAnswer); | ||
| if (!match?.groups) { | ||
| match = this.AU_ADDR_REGEX_FLEXIBLE.exec(trimmedAnswer); | ||
| } | ||
| if (!match?.groups) { | ||
| match = this.AU_ADDR_REGEX_SEPARATED.exec(trimmedAnswer); | ||
| } | ||
|
|
||
| // If regex doesn't match, try to parse manually from comma-separated values | ||
| if (!match?.groups) { | ||
| const parts = trimmedAnswer | ||
| .split(',') | ||
| .map(p => p.trim()) | ||
| .filter(p => p); | ||
|
|
||
| if (parts.length >= 3) { | ||
| // Try to extract state and postcode from the last part | ||
| const lastPart = parts[parts.length - 1]; | ||
| const statePostcodeMatch = /^([A-Z]{2,3})\s*(\d{4})$/.exec(lastPart); | ||
|
|
||
| if (statePostcodeMatch) { | ||
| // Format: "Street, Suburb, State Postcode" (3 parts) or more | ||
| // For 3 parts: parts[0] is street, parts[1] is suburb | ||
| // For more parts: join all but last 2 as street | ||
| const streetAddress = | ||
| parts.length === 3 ? parts[0] : parts.slice(0, -2).join(', '); | ||
| const suburb = parts[parts.length - 2]; | ||
| const state = statePostcodeMatch[1]; | ||
| const postcode = statePostcodeMatch[2]; | ||
|
|
||
| update.$set['answers.user.address.streetAddress'] = streetAddress; | ||
| update.$set['answers.user.address.suburb'] = suburb; | ||
| update.$set['answers.user.address.state'] = state; | ||
| update.$set['answers.user.address.postcode'] = postcode; | ||
| update.$set['answers.user.address.full'] = answer; | ||
| return; | ||
| } | ||
|
|
||
| // Try to extract state and postcode separately | ||
| if (parts.length >= 4) { | ||
| const stateMatch = /^([A-Z]{2,3})$/.exec(parts[parts.length - 2]); | ||
| const postcodeMatch = /^(\d{4})$/.exec(parts[parts.length - 1]); | ||
|
|
||
| if (stateMatch && postcodeMatch) { | ||
| // Format: "Street, Suburb, State, Postcode" (4 parts) or more | ||
| // For 4 parts: parts[0] is street, parts[1] is suburb | ||
| // For more parts: join all but last 3 as street | ||
| const streetAddress = | ||
| parts.length === 4 ? parts[0] : parts.slice(0, -3).join(', '); | ||
| const suburb = parts[parts.length - 3]; | ||
| const state = stateMatch[1]; | ||
| const postcode = postcodeMatch[1]; | ||
|
|
||
| update.$set['answers.user.address.streetAddress'] = streetAddress; | ||
| update.$set['answers.user.address.suburb'] = suburb; | ||
| update.$set['answers.user.address.state'] = state; | ||
| update.$set['answers.user.address.postcode'] = postcode; | ||
| update.$set['answers.user.address.full'] = answer; | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If all parsing attempts fail, throw error | ||
| throw new BadRequestException( | ||
| 'Unable to parse address; please check the format.', | ||
| 'Unable to parse address; please check the format. Expected format: "Street Address, Suburb, State Postcode" (e.g., "123 Main St, Sydney, NSW 2000")', | ||
| ); | ||
| } |
There was a problem hiding this comment.
The enhanced address parsing logic with multiple regex patterns and manual fallback is complex and handles many edge cases. However, there is no test coverage for this critical validation logic. Given the complexity and the multiple parsing paths (strict regex, flexible regex, separated regex, and manual parsing), this code should have comprehensive unit tests to ensure all address format variations are handled correctly and edge cases don't cause regressions.
| // Validate profile data - profile.emails and profile.name are optional | ||
| if (!profile.emails || profile.emails.length === 0 || !profile.name) { | ||
| done(new UnauthorizedException('Invalid Google profile data'), false); | ||
| return; | ||
| } | ||
|
|
||
| // At this point, we know profile.name and profile.emails exist and have items | ||
| const { name, emails } = profile; | ||
|
|
||
| const googleUser = { | ||
| email: emails[0].value, | ||
| firstName: name.givenName, | ||
| lastName: name.familyName, | ||
| firstName: name.givenName ?? '', | ||
| lastName: name.familyName ?? '', | ||
| }; |
There was a problem hiding this comment.
The validation checks if profile.name exists but doesn't verify that givenName and familyName are present within it. Lines 75-76 use nullish coalescing to default to empty strings, which means users could be created or updated with empty firstName and lastName if Google doesn't provide these fields. This could lead to users with incomplete profile information. Consider either requiring these fields or handling the case where they're missing more explicitly, possibly by prompting the user to complete their profile.
| */ | ||
| export function getCorsOrigin(): string | undefined { | ||
| const corsOrigin = process.env.CORS_ORIGIN; | ||
|
|
There was a problem hiding this comment.
The function returns string | undefined, but when corsOrigin is an empty string (as opposed to undefined), it will return an empty string. This might not be the intended behavior. Consider explicitly checking for empty strings and returning undefined in that case, or documenting that empty strings are valid CORS origins. An empty string as a CORS origin might have unexpected behavior.
| // Treat empty or whitespace-only values as "not configured" | |
| if (corsOrigin === undefined) { | |
| return undefined; | |
| } | |
| const normalized = corsOrigin.trim(); | |
| if (normalized === '') { | |
| return undefined; | |
| } |
| // Special case: if this is the demo step (step 6, field is empty), and user clicks Skip or Demo, | ||
| // mark onboarding as completed (nextStep will be beyond the last step) | ||
| // Only treat empty field as skip action for the specific demo step (step 6) | ||
| const DEMO_STEP_ID = 6; | ||
| if (stepId === DEMO_STEP_ID && (!field || field.trim() === '')) { | ||
| // This is the demo step (step 6), which is the last step | ||
| // After this, onboarding should be completed | ||
| nextStep = stepId + 1; // This will be 7, which is beyond the last step (6) | ||
| } |
There was a problem hiding this comment.
The logic for handling the demo step skip functionality has potential issues. The code checks if stepId === DEMO_STEP_ID (which is 6) and if the field is empty, then sets nextStep = stepId + 1 (which would be 7). However, this happens before the existing skip logic at lines 172-175 which also modifies nextStep. If both conditions are true, nextStep will be set twice, potentially causing unexpected behavior. Consider consolidating the skip logic or ensuring mutual exclusivity of these conditions.
| // Special case: if this is the demo step (step 6, field is empty), and user clicks Skip or Demo, | ||
| // mark onboarding as completed (nextStep will be beyond the last step) | ||
| // Only treat empty field as skip action for the specific demo step (step 6) | ||
| const DEMO_STEP_ID = 6; |
There was a problem hiding this comment.
The hardcoded constant DEMO_STEP_ID = 6 makes the code fragile. If the onboarding flow changes and steps are added or removed before step 6, this hardcoded value will break the demo step logic. Consider making this configurable or deriving it from a centralized onboarding configuration.
| if (field === 'user.address.full') { | ||
| // Ensure all required fields are present after parsing | ||
| const streetAddress = update.$set['answers.user.address.streetAddress']; | ||
| const suburb = update.$set['answers.user.address.suburb']; | ||
| const state = update.$set['answers.user.address.state']; | ||
| const postcode = update.$set['answers.user.address.postcode']; | ||
|
|
||
| // Collect missing fields for detailed error message | ||
| const missingFields: string[] = []; | ||
| if (!streetAddress) missingFields.push('streetAddress'); | ||
| if (!suburb) missingFields.push('suburb'); | ||
| if (!state) missingFields.push('state'); | ||
| if (!postcode) missingFields.push('postcode'); | ||
|
|
||
| if (missingFields.length > 0) { | ||
| throw new BadRequestException( | ||
| `Address parsing failed: missing required fields - ${missingFields.join(', ')}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The address field validation logic has an ordering issue. The validation at lines 225-242 checks if all required address fields are present after parsing, but this check happens after the session has already been saved to the database (line 208). If the validator successfully parses some but not all fields, the session will be updated with incomplete address data, and then an error will be thrown. This leaves the database in an inconsistent state. Consider moving this validation into the validator function itself (lines 40-111) so it throws an error before the database update occurs.
| const mockClient = { | ||
| calls: { | ||
| create: (): Promise<never> => | ||
| Promise.reject(new Error('Twilio not configured')), | ||
| list: (): Promise<never[]> => Promise.resolve([]), | ||
| }, | ||
| messages: { | ||
| create: (): Promise<never> => | ||
| Promise.reject(new Error('Twilio not configured')), | ||
| list: (): Promise<never[]> => Promise.resolve([]), | ||
| }, | ||
| }; | ||
|
|
||
| return mockClient as unknown as Twilio.Twilio; |
There was a problem hiding this comment.
The mock Twilio client created for development has methods that return rejected promises or empty arrays, but real code using this client may not be handling these rejections properly. The error message 'Twilio not configured' will be thrown at runtime when trying to use Twilio features in development, which could lead to unhandled promise rejections if the calling code doesn't have proper error handling. Consider logging a warning when these methods are called to make it clearer to developers that Twilio is not configured.
Changes
Onboarding Improvements
New Features
Code Quality
Environment Variables
Environment Variables (No Changes Required)