Fix: GET Method of OOO Request using Feature Flag #2421
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
WalkthroughThe changes introduce a new asynchronous function, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RequestsModel
participant UtilsRequests
participant UserFetcher
Caller->>RequestsModel: getRequests(type, ...)
RequestsModel->>RequestsModel: fetch allRequests
alt type == REQUEST_TYPE.OOO
RequestsModel->>UtilsRequests: transformGetOooRequest(dev, allRequests)
alt dev == true
UtilsRequests->>UtilsRequests: Map requests to OldOooRequest if status exists
else dev == false
loop for each request with state
UtilsRequests->>UserFetcher: fetchUser(requestedBy)
UserFetcher-->>UtilsRequests: username
UtilsRequests->>UtilsRequests: Map to OooStatusRequest with username
end
end
UtilsRequests-->>RequestsModel: transformedRequests
end
RequestsModel-->>Caller: paginated (possibly transformed) requests
Suggested labels
Suggested reviewers
Poem
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: 4
🔭 Outside diff range comments (1)
models/requests.ts (1)
75-76: 🧹 Nitpick (assertive)
devflag parsing silently ignores unexpected values
devis derived from a string comparison (query.dev === "true").
• A typo such asTrue,1, or"false "will silently fall back tofalse, producing the legacy payload when the caller might expect the new one.
• Consider coercing to boolean with a helper that validates the incoming value and throws (or logs) when it is neither"true"nor"false".-const dev = query.dev === "true"; +const dev = (() => { + if (query.dev === undefined) return false; + if (query.dev === "true") return true; + if (query.dev === "false") return false; + logger.warn(`Unexpected dev flag value "${query.dev}", defaulting to false`); + return false; +})();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
models/requests.ts(2 hunks)types/oooRequest.d.ts(1 hunks)utils/requests.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
types/oooRequest.d.ts (1)
Learnt from: surajmaity1
PR: Real-Dev-Squad/website-backend#2386
File: test/fixtures/oooRequest/oooRequest.ts:59-60
Timestamp: 2025-03-16T05:23:33.460Z
Learning: The `createOooRequests2` object in test/fixtures/oooRequest/oooRequest.ts still uses the old property names `message` and `state` (instead of `reason` and `status`) because it's specifically used in acknowledging OOO request tests. This naming inconsistency will be fixed by surajmaity1 in a dedicated PR for acknowledging OOO requests.
🧬 Code Graph Analysis (3)
models/requests.ts (2)
constants/requests.ts (1)
REQUEST_TYPE(13-19)utils/requests.ts (1)
transformGetOooRequest(41-99)
types/oooRequest.d.ts (1)
constants/requests.ts (2)
REQUEST_TYPE(13-19)REQUEST_STATE(1-5)
utils/requests.ts (2)
types/oooRequest.d.ts (2)
OldOooRequest(22-34)OooStatusRequest(7-20)constants/requests.ts (1)
ERROR_WHILE_FETCHING_REQUEST(39-39)
🔇 Additional comments (3)
models/requests.ts (1)
93-99:requestedByfilter now branches ondevbut the semantics divergeWhen
devistruewe expectrequestedByto already be a user-id, while in legacy mode it is a username.
That assumption is not documented anywhere and can easily break if the front-end forgets to toggle the flag. Please:
- Add JSDoc or runtime checks clarifying what the field represents in each mode.
- Fail fast if the format does not match the expectation (e.g. a UUID when a username is expected).
- Unit-test both branches.
types/oooRequest.d.ts (1)
22-34:lastModifiedByinitialisation in implementation contradicts the declared type
OldOooRequest.lastModifiedByis typed asstring | null, yet the transformation code initialises it with an empty string ("").
Usingnullis semantically clearer (“unknown”) and avoids the need for truthy checks.- lastModifiedBy: string | null; + lastModifiedBy: string | null; // prefer null over empty string for “not set”Please align the implementation (utils/requests.ts line 54) with the type or adjust the union to include
"".utils/requests.ts (1)
41-96: Parallelise user look-ups to avoid N × serial network callsThe
for ... of+awaitpattern results in sequential calls tofetchUser, which can severely slow down pagination.
Prepare the promises first andawait Promise.all:- for (const request of allRequests) { - if (request.state) { - try { - const userResponse: any = await fetchUser({ userId: request.requestedBy }); - ... - } catch (error) { ... } - } - } + const transformed = await Promise.all( + allRequests.map(async (request) => { + if (!request.state) return request; + const { user } = await fetchUser({ userId: request.requestedBy }); + return { + ... /* build OooStatusRequest */ + }; + }) + ); + oooRequests.push(...transformed);[ suggest_optional_refactor ]
|
Also, consider creating a dedicated issue or sub-issue for things like this, just to make sure they don’t get missed |
|
Add test coverage screenshot and test PR |
surajmaity1
force-pushed
the
fix/ooo-get-request
branch
from
c6736a6 to
5f7f267
Compare
surajmaity1
force-pushed
the
fix/ooo-get-request
branch
from
5f7f267 to
05d07f2
Compare
surajmaity1
force-pushed
the
fix/ooo-get-request
branch
from
05d07f2 to
4a0e15f
Compare
surajmaity1
force-pushed
the
fix/ooo-get-request
branch
from
4a0e15f to
b6c6e1c
Compare
test/integration/requests.test.ts
Outdated
| import * as requestsQuery from "../../models/requests"; | ||
| import { userState } from "../../constants/userStatus"; | ||
| import * as logUtils from "../../services/logService"; | ||
| import { OldOooRequest, OooStatusRequest } from "../../types/oooRequest"; |
There was a problem hiding this comment.
Are we using these types anywhere? If not why are we importing this?
test/unit/utils/requests.test.ts
Outdated
| import { convertDateStringToMilliseconds, getNewDeadline, transformGetOooRequest } from "../../../utils/requests" | ||
| import { convertDaysToMilliseconds } from "../../../utils/time"; | ||
| import {expect} from "chai"; | ||
| import { createdOOORequest, oooStatusRequests } from "../../fixtures/oooRequest/oooRequest"; | ||
| import sinon from "sinon"; | ||
| import * as usersModel from "../../../models/users"; | ||
| import { ERROR_WHILE_FETCHING_REQUEST } from "../../../constants/requests"; |
There was a problem hiding this comment.
Same here, why are we importing these if not getting used anywhere?
utils/requests.ts
Outdated
| }; | ||
| }; | ||
|
|
||
| export const transformGetOooRequest = async (dev, allRequests) => { |
There was a problem hiding this comment.
If this is temporary then what is the need of putting this function inside utils?
Can't we put the entire thing on that if block in model and put a comment that this will eventually get removed once everything is fixed?
cc: @AnujChhikara
surajmaity1
force-pushed
the
fix/ooo-get-request
branch
2 times, most recently
from
aa77448 to
6226837
Compare
surajmaity1
force-pushed
the
fix/ooo-get-request
branch
from
6226837 to
2ba3e48
Compare
Date: 04 May, 2025
Developer Name: @surajmaity1
Issue Ticket Number
Description
When
dev = true, format will be:When
dev = false, format will be:Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
GET.REQEST.DEV.FLAG.mp4
dashboard.OOO.GET.request.mp4
Test Coverage
Screenshot 1
Additional Notes