logging is consistent with the new group-specific preference management #4080
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Suggested Reviewers
Possibly Related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 #4080 +/- ##
========================================
Coverage 11.81% 11.81%
========================================
Files 115 115
Lines 15482 15482
Branches 319 319
========================================
Hits 1829 1829
Misses 13653 13653 |
![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 and nitpick comments (4)
src/auth-service/bin/jobs/preferences-log-job.js (4)
17-21: LGTM! Consider adding JSDoc type annotations.The variable declarations and initialization look good. The use of
mongoose.Types.ObjectIdfor the group ID is appropriate.Consider adding JSDoc type annotations for better code documentation:
+/** @type {number} */ let totalCountWithoutSelectedSites = 0; +/** @type {number} */ let totalUsersProcessed = 0; +/** @type {mongoose.Types.ObjectId} */ const defaultGroupId = mongoose.Types.ObjectId(constants.DEFAULT_GROUP);
45-51: Add error handling for preference fetching.While the preference fetching logic is well-implemented, it could benefit from better error handling.
- const preferences = await PreferenceModel("airqo") - .find({ - user_id: { $in: userIds }, - group_id: defaultGroupId, - }) - .select("_id user_id selected_sites") - .lean(); + let preferences = []; + try { + preferences = await PreferenceModel("airqo") + .find({ + user_id: { $in: userIds }, + group_id: defaultGroupId, + }) + .select("_id user_id selected_sites") + .lean(); + } catch (error) { + logger.error(`Failed to fetch preferences: ${stringify(error)}`); + // Continue with empty preferences rather than breaking the job + }
60-68: Optimize the filtering of users without selected sites.The current implementation is correct but can be made more efficient by reducing iterations.
- const usersWithoutSelectedSites = validUsers.filter((user) => { - const preference = preferencesMap.get(user._id.toString()); - return !preference || isEmpty(preference.selected_sites); - }); - - // Aggregate results - totalCountWithoutSelectedSites += usersWithoutSelectedSites.length; - totalUsersProcessed += validUsers.length; + // Combine filtering and counting in one pass + let batchUsersWithoutSites = 0; + for (const user of validUsers) { + const preference = preferencesMap.get(user._id.toString()); + if (!preference || isEmpty(preference.selected_sites)) { + batchUsersWithoutSites++; + } + } + totalCountWithoutSelectedSites += batchUsersWithoutSites; + totalUsersProcessed += validUsers.length;
74-86: Enhance logging with structured data.While the current logging is informative, structured logging would make it easier to parse and analyze the data.
- logger.info( - `💔💔 Total count of users without Customised Locations in the default group: ${totalCountWithoutSelectedSites}, which is ${percentageWithoutSelectedSites}% of processed users.` - ); + logger.info({ + message: "Completed processing users without Customised Locations", + totalUsersWithoutLocations: totalCountWithoutSelectedSites, + totalUsersProcessed, + percentageWithoutLocations: parseFloat(percentageWithoutSelectedSites), + defaultGroupId: defaultGroupId.toString() + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/auth-service/bin/jobs/preferences-log-job.js(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/auth-service/bin/jobs/preferences-log-job.js
[error] 39-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/auth-service/bin/jobs/preferences-log-job.js (1)
98-99: LGTM! Export enables testing as intended.
The module export is correctly implemented and aligns with the PR objective of facilitating testing and manual invocation.
| // Fetch users with their group_roles | ||
| const users = await UserModel("airqo") | ||
| .find() | ||
| .limit(batchSize) | ||
| .skip(skip) | ||
| .select("_id email") | ||
| .select("_id email group_roles") | ||
| .lean(); | ||
|
|
||
| if (users.length === 0) { | ||
| break; | ||
| } | ||
|
|
||
| // Fetch existing preferences for users in batch | ||
| const userIds = users.map((user) => user._id); | ||
| // Filter users who are members of the default group | ||
| const validUsers = users.filter( | ||
| (user) => | ||
| user.group_roles && | ||
| user.group_roles.some( | ||
| (role) => role.group.toString() === defaultGroupId.toString() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize group role filtering and add safety checks.
The user fetching logic is well-structured, but there are a few improvements we can make:
- Use optional chaining as suggested by static analysis:
- user.group_roles &&
- user.group_roles.some(
+ user.group_roles?.some(- Add index hint for better query performance:
const users = await UserModel("airqo")
.find()
+ .hint({ group_roles: 1 })
.limit(batchSize)
.skip(skip)
.select("_id email group_roles")
.lean();- Consider caching the defaultGroupId.toString() result:
+ const defaultGroupIdString = defaultGroupId.toString();
const validUsers = users.filter(
(user) =>
user.group_roles?.some(
- (role) => role.group.toString() === defaultGroupId.toString()
+ (role) => role.group.toString() === defaultGroupIdString
)
);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 39-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
Auth-service changes in this PR available for preview here |
Description
logging is consistent with the new group-specific preference management
Changes Made
group_roles, similar to the preference update job.constants.DEFAULT_GROUPto filter preferences.user_idandgroup_id.Testing
Affected Services
API Documentation Updated?
Additional Notes
The refactored job now:
This approach aligns with the recent changes in the preference update job and ensures that the logging is consistent with the new group-specific preference management.