Refactor getUser controller function based on query parameter #2370
Conversation
pankajjs
changed the title
pankajjs
force-pushed
the
refactor/user-controller-2274
branch
from
26351df to
3b2fefe
Compare
Prakhar-FF13
force-pushed
the
refactor/user-controller-2274
branch
from
3b2fefe to
47b8ba9
Compare
…ameters Signed-off-by: Prakhar Kumar <[email protected]>
Signed-off-by: Prakhar Kumar <[email protected]>
Signed-off-by: Prakhar Kumar <[email protected]>
…for reusability. 2. Updated the integration tests after refactoring of code. Signed-off-by: Prakhar Kumar <[email protected]>
Prakhar-FF13
force-pushed
the
refactor/user-controller-2274
branch
from
b733ceb to
220e533
Compare
…ryparam Signed-off-by: Prakhar Kumar <[email protected]>
| return res.json({ | ||
| message: "User returned successfully!", | ||
| user, | ||
| message: user ? "User returned successfully!" : "User not found", |
| count: usersData.length, | ||
| users: usersData, | ||
| message: "No users found", | ||
| users: [], |
There was a problem hiding this comment.
why we removed the count field?
| user = result.user; | ||
| if (!result.userExists) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
why are we not throwing error here?
There was a problem hiding this comment.
This is the original functionality haven't changed the code, just refactored it, if i throw error here. Tests will fail as it would require null user but we would return a 404 status code.
2. Using const instead of let for direct return values. Signed-off-by: Prakhar Kumar <[email protected]>
Summary by CodeRabbit
WalkthroughThe pull request refactors user-related functionality by delegating data retrieval from controllers to a dedicated user service. In the controllers, direct calls to the data access layer are replaced with corresponding functions from the user service, and error handling now employs specific HTTP error classes. The user service file now includes new asynchronous functions for various user queries. Additionally, modifications in test flows reflect a shift to stubbing the data access layer, and a logger import has been added to the utilities file. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant C as Controller
participant U as UserService
alt Normal Flow
Client->>C: Send user request (e.g., getUserById)
C->>U: Call corresponding user service function
U-->>C: Return user data
C->>Client: Respond with HTTP 200 and JSON data
else Error Occurs
Client->>C: Send user request
C->>U: Call user service function
U-->>C: Return error (NotFound, BadRequest, InternalServerError)
C->>Client: Respond with appropriate HTTP error response
end
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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:
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
controllers/users.js(2 hunks)services/users.js(2 hunks)test/integration/users.test.js(2 hunks)utils/users.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.10.0)
🔇 Additional comments (22)
utils/users.js (1)
7-7: No issues with the new logger import.Adding a logger import here is succinct and aligns with your global logging strategy.
test/integration/users.test.js (2)
50-50: Consistent refactor to the new data access layer.Importing
dataAccessat line 50 is aligned with the broader refactoring efforts. No issues found.
1491-1491:❓ Verification inconclusive
Verify that this test properly reflects the new error path.
This line stubs
retrieveUserson thedataAccesslayer to throw an error, matching the recent architectural changes. Ensure all dependent tests still correctly handle thrown errors and produce the expected HTTP status codes and responses.Run a quick local test to confirm that no unhandled rejections occur:
🏁 Script executed:
#!/bin/bash npm run testLength of output: 385
Action Required: Manual Verification of Error Path Needed
- The test stubs
dataAccess.retrieveUsersto throw an error, which should trigger the new error handling code and produce the correct HTTP response.- However, the test run did not complete due to a Node version mismatch (expected "22.10.0" but got "22.9.0"). This prevented full verification.
- Next Steps: Update your Node environment to version 22.10.0 and re-run the test suite. Then manually confirm that:
- The error is correctly caught.
- The test returns the expected HTTP status code and response.
services/users.js (7)
5-12: Imports look good.These new imports, especially
NotFound,BadRequest, andInternalServerError, improve error handling clarity. Always ensure they’re used consistently across all methods.
57-72: Ensure consistent exception handling.By throwing
NotFoundwhen the user doesn’t exist, the caller can directly map this to a 404 response. This flow is appropriate, but remember to guard calls in the controllers/tests so that uncaught errors don't cascade.
74-91: Good handling of missing user ID.Throwing a
BadRequesterror at line 80 is clear and direct. Ensure you have coverage to confirm that the client properly sees this as a 400 response.
116-137: Graceful null user return.Returning
nullif the user is not found ensures a clearer distinction from throwing an error. Verify that downstream callers handle thenullcase properly.
139-153: Use of InternalServerError is consistent.This properly maps to an HTTP 500 in the event of a DB or logic failure. Good job logging the error context before throwing.
155-196: Efficient filtering with potential improvement.The approach to filter out archived users and optionally attach tasks (when
devis true) is clear. If the number of tasks or users is large, you might consider indexing or partial queries.
201-206: Exports updated successfully.Including the newly introduced functions in module exports is correct.
controllers/users.js (12)
34-34: Explicit error handling import.Using
NotFound,BadRequest, andInternalServerErrorhelps maintain consistent HTTP semantics.
37-37: Shifting to userService.Replacing direct data access calls with
userServiceis a good move toward better separation of concerns. Keep an eye on replicated logic to ensure the service layer fully abstracts data retrieval.
90-99: Clear parameter destructuring.Separating flags like
profileParamanddevParamclarifies the logic. This heightens code readability.
106-112: Manual 200 status code usage.Returning a 200 status code is explicit and consistent. Continue verifying parameter validity (e.g., ensure
idis not empty or ill-formed).
114-117: Retrieve user by profile data.Your usage of
userService.getUserByProfileDatakeeps the logic centralized. No concerns here.
123-130: Inactive user retrieval is well-structured.
getUsersByUnmergedPrsand returning the count fosters clarity. Good to see a distinct “Inactive users returned successfully!” message.
132-142: Graceful path for discord ID queries.The addition of
userService.getUserByDiscordIdplus a fallback to a 404 route is a concise approach.
148-158: Departed user logic complements new service approach.Refactoring to call
userService.getDepartedUsersis consistent with the new layered architecture. The usage of 204 for no content is well-chosen.
160-173: Handling overdue tasks with clarity.Line 162 returning "No users found" is consistent and conveys an explicit message. The subsequent success response includes a user count. Nicely done.
179-183: No direct issues.This fallback to direct data access for GitHub-based user lookups is presumably transitional. Looks sound.
185-195: Final fallback to standard user retrieval.At line 185, calling
dataAccess.retrieveUsers({ query: reqQueryObject })covers the default get logic. Straightforward approach.
195-206: Error classification.The switch statement for
NotFound,BadRequest,InternalServerError, and a final fallback is well-structured: each case is mapped to the correct response.
| /** | ||
| * @param days {number}: days since last unmerged pr. | ||
| * @returns Promise<object[]> | ||
| */ | ||
| const getUsersByUnmergedPrs = async (days) => { | ||
| try { | ||
| const inDiscordUser = await dataAccess.retrieveUsersWithRole(ROLES.INDISCORD); | ||
| const users = []; | ||
|
|
||
| for (const user of inDiscordUser) { | ||
| const username = user.github_id; | ||
| const isMerged = await isLastPRMergedWithinDays(username, days); | ||
| if (!isMerged) { | ||
| users.push(user.id); | ||
| } | ||
| } | ||
|
|
||
| return users; | ||
| } catch (error) { | ||
| logger.error(`Error while fetching all users: ${error}`); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider parallelizing external calls.
Within getUsersByUnmergedPrs, each user’s last PR merge status is fetched sequentially. If inDiscordUser can be large, wrapping these lookups in a Promise.all can yield performance gains.
-for (const user of inDiscordUser) {
- ...
- const isMerged = await isLastPRMergedWithinDays(username, days);
- ...
-}
+await Promise.all(inDiscordUser.map(async (user) => {
+ const isMerged = await isLastPRMergedWithinDays(user.github_id, days);
+ if (!isMerged) {
+ users.push(user.id);
+ }
+}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @param days {number}: days since last unmerged pr. | |
| * @returns Promise<object[]> | |
| */ | |
| const getUsersByUnmergedPrs = async (days) => { | |
| try { | |
| const inDiscordUser = await dataAccess.retrieveUsersWithRole(ROLES.INDISCORD); | |
| const users = []; | |
| for (const user of inDiscordUser) { | |
| const username = user.github_id; | |
| const isMerged = await isLastPRMergedWithinDays(username, days); | |
| if (!isMerged) { | |
| users.push(user.id); | |
| } | |
| } | |
| return users; | |
| } catch (error) { | |
| logger.error(`Error while fetching all users: ${error}`); | |
| throw error; | |
| } | |
| }; | |
| /** | |
| * @param days {number}: days since last unmerged pr. | |
| * @returns Promise<object[]> | |
| */ | |
| const getUsersByUnmergedPrs = async (days) => { | |
| try { | |
| const inDiscordUser = await dataAccess.retrieveUsersWithRole(ROLES.INDISCORD); | |
| const users = []; | |
| await Promise.all(inDiscordUser.map(async (user) => { | |
| const isMerged = await isLastPRMergedWithinDays(user.github_id, days); | |
| if (!isMerged) { | |
| users.push(user.id); | |
| } | |
| })); | |
| return users; | |
| } catch (error) { | |
| logger.error(`Error while fetching all users: ${error}`); | |
| throw error; | |
| } | |
| }; |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| try { | ||
| const result = await dataAccess.retrieveUsers({ id: userId }); | ||
| if (!result.userExists) { | ||
| throw NotFound("User doesn't exist"); |
There was a problem hiding this comment.
The NotFound error constructor is missing the new keyword. This will cause a runtime error when the function is called. The correct syntax should be:
throw new NotFound("User doesn't exist");This pattern needs to be checked throughout the file for all error types (BadRequest, InternalServerError, etc.) to ensure consistent error handling.
| throw NotFound("User doesn't exist"); | |
| throw new NotFound("User doesn't exist"); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| try { | ||
| const result = await dataAccess.retrieveUsers({ query: queryObject }); | ||
| const departedUsers = await getUsersWithIncompleteTasks(result.users); | ||
| if (!departedUsers || departedUsers.length === 0) return { departedUsers: [] }; |
There was a problem hiding this comment.
The return value is missing the result object when no departed users are found. The controller expects both result and departedUsers properties, but this function only returns { departedUsers: [] } in the empty case. Consider returning { result, departedUsers: [] } to maintain a consistent return structure and avoid potential undefined errors in the controller.
| if (!departedUsers || departedUsers.length === 0) return { departedUsers: [] }; | |
| if (!departedUsers || departedUsers.length === 0) return { result, departedUsers: [] }; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
@Prakhar-FF13 there are some unresolved chat , look into it |
Date: 26 Jan 2025
Developer Name: Prakhar Kumar
Issue Ticket Number
Description
Refactored get users function in user controller by decomposing into smaller functions based on query parameters.
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Test Coverage
Additional Notes
Tejas Sir has asked for a peer review for the PR and then design doc can be discussed.
Design Doc - https://docs.google.com/document/d/19cs4FndY-Rh37A7ptZ2db9S-1Mqd2BICwpWV-SZJQv4/edit?tab=t.0#heading=h.o43h9priqbg
TCR - https://dashboard.realdevsquad.com/task-requests/details/?id=65zapL8S5y4mm9M2bPzC