Fix: Correctly Update User Details and Handle Profile Picture Length #4489
Conversation
…During Google Auth
📝 WalkthroughWalkthroughThe changes update several components of the authentication flow. In the user controller’s Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (5)
✨ Finishing Touches
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #4489 +/- ##
========================================
Coverage 11.23% 11.23%
========================================
Files 156 156
Lines 18020 18020
Branches 388 388
========================================
Hits 2025 2025
Misses 15993 15993
Partials 2 2 |
|
Auth-service changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/auth-service/models/User.js (1)
419-430: Good implementation of profile picture URL truncation.This change correctly handles the case where a profile picture URL exceeds the maximum allowed length by truncating it to 200 characters before re-validating. This is a more user-friendly approach than simply rejecting the URL.
Consider adding a log message or notification to indicate that truncation occurred, which could help with debugging user issues. For example:
// Truncate if too long if (this.profilePicture.length > 200) { + logger.info(`Profile picture URL truncated from ${this.profilePicture.length} to 200 characters for user ${this._id || 'new user'}`); this.profilePicture = this.profilePicture.substring(0, 200); }src/auth-service/controllers/user.controller.js (2)
373-374: Consider using a tenant validation check.The code uses the tenant directly from the request, but it would be safer to validate it or provide a fallback.
- await UserModel(request.query.tenant) // Use the tenant from the request. + const tenant = request.query.tenant || constants.DEFAULT_TENANT || "airqo"; + await UserModel(tenant)
377-384: Clean implementation for updating user activity metrics.The update logic handles the lastLogin, isActive, and loginCount fields correctly.
There's a commented example of a conditional update for the verified field. Either implement this feature if needed or remove the comment to keep the code clean:
- // Add any other updates as needed (e.g., verified) - // ...(req.user.analyticsVersion !== 3 && req.user.verified === false ? { $set: { verified: true } } : {}), // Example (if you have these fields) + // Add any other updates as neededsrc/auth-service/middleware/passport.js (1)
492-518: Consistent user activity tracking implementation.This code correctly updates the user's lastLogin, isActive, and loginCount after successful Google registration, consistent with the implementation in the controller.
Consider extracting this user activity update logic into a utility function since it's duplicated in multiple places (controller and middleware). This would make maintenance easier and reduce code duplication:
// In a utils file (e.g., user.utils.js) async function updateUserActivityMetrics(tenant, userId, options = {}) { const currentDate = new Date(); try { await UserModel(tenant.toLowerCase()) .findOneAndUpdate( { _id: userId }, { $set: { lastLogin: currentDate, isActive: true }, $inc: { loginCount: 1 }, ...(options.updateVerification ? { $set: { verified: true } } : {}) }, { new: true, upsert: false, runValidators: true } ) .then(() => {}) .catch((error) => { logger.error(`🐛🐛 Internal Server Error -- ${stringify(error)}`); }); } catch (error) { logger.error(`🐛🐛 Internal Server Error -- ${stringify(error)}`); } } // Then in your code: // updateUserActivityMetrics(tenant, user._id, { updateVerification: user.analyticsVersion !== 3 && user.verified === false });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/auth-service/controllers/user.controller.js(1 hunks)src/auth-service/middleware/passport.js(2 hunks)src/auth-service/models/User.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/auth-service/controllers/user.controller.js (1)
370-390:❓ Verification inconclusive
Properly updates user activity metrics after Google login.
The added code correctly updates the user's lastLogin time, active status, and login count after successful Google authentication. This ensures consistent tracking of user activity.
There appears to be a potential issue with the error logging. The
stringifyfunction is used in the error logging but its import isn't visible in the provided code segment. Let's verify this:
🏁 Script executed:
#!/bin/bash # Check if stringify is properly imported rg "stringify.*=.*require|import.*stringify" src/auth-service/controllers/user.controller.jsLength of output: 94
Verify
stringifyImport in User ControllerThe revised code correctly updates the user's last login, active status, and login count. However, please note that the error logging still calls the
stringifyfunction without any visible import or definition in this file. We need to confirm ifstringifyis provided globally or imported elsewhere. If not, please add the appropriate import statement or function definition to prevent potential runtime errors.
- Verified code snippet for user activity update remains intact.
- Action Required: Manually verify that
stringifyis imported or defined in your codebase; if it isn’t, add the necessary import.src/auth-service/middleware/passport.js (3)
456-456: Good removal of premature next() call.Removing this next() call is important to ensure the authentication flow completes properly before proceeding to the next middleware.
458-472: Proper error handling with next parameter.Adding the next parameter to the register method allows for better error propagation and handling throughout the authentication flow.
521-521: Appropriate removal of next() call.Similar to line 456, removing this next() call ensures the authentication flow completes before proceeding to the next middleware.
|
Auth-service changes in this PR available for preview here |
Description
This PR resolves issues with user account updates during Google authentication, including thelastLogin,isActive, andloginCountfields, and handles profile picture URLs that exceed the maximum allowed length, and now newly registered users and previously registered users have their fields updated correctly.
Changes Made
passport.js):next()call from theuseGoogleStrategycallback to ensure the controller runs after the Google Strategy'scb()function has completed.lastLogin,isActive,loginCountupdates.nextparameter to theUserModel.register()method.UserModel.register()method.User.js):UserSchema.pre()to truncate profile picture URLs longer than 1024 characters.UserSchema.statics.register()method to take anextparameter.jwt.sign()method.user.controller.js):req.useris set by passport.Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit
New Features
Bug Fixes