feat: add API to fetch impersonation requests#2442
feat: add API to fetch impersonation requests#2442prakashchoudhary07 merged 12 commits intoRealDevSquad:developfrom
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
WalkthroughThis change introduces the backend infrastructure for fetching impersonation requests. It adds a controller, validation middleware, Firestore model functions, and route registration for a new GET Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant AuthMiddleware
participant Validator
participant Controller
participant Model
Client->>Router: GET /impersonation/requests
Router->>AuthMiddleware: Authenticate request
AuthMiddleware->>Validator: Validate query parameters
Validator->>Controller: Call controller if valid
Controller->>Model: Fetch request(s) from Firestore
Model-->>Controller: Return impersonation request(s)
Controller-->>Client: Respond with data or error
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
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 (
|
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Inefficient Array Operation ▹ view | ✅ Fix detected | |
| Unnecessary Async Validation ▹ view | ✅ Fix detected | |
| Missing Error Context in getImpersonationRequestById ▹ view | ✅ Fix detected | |
| Unconstrained ID Parameter Validation ▹ view | ✅ Fix detected | |
| Poor Type Safety with 'any' Usage ▹ view | ✅ Fix detected | |
| Unexplained Magic Number ▹ view | ✅ Fix detected | |
| Incomplete query parameter documentation ▹ view | ✅ Fix detected | |
| Conflicting Pagination Methods ▹ view | ✅ Fix detected | |
| Unnecessary Extra Queries for Pagination ▹ view | ||
| Inconsistent Module System Usage ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| routes/impersonation.ts | ✅ |
| routes/index.ts | ✅ |
| middlewares/validators/impersonationRequests.ts | ✅ |
| controllers/impersonationRequests.ts | ✅ |
| models/impersonationRequests.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
controllers/impersonationRequests.ts(1 hunks)middlewares/validators/impersonationRequests.ts(1 hunks)models/impersonationRequests.ts(1 hunks)routes/impersonation.ts(1 hunks)routes/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
middlewares/validators/impersonationRequests.ts (2)
types/impersonationRequest.d.ts (2)
CreateImpersonationRequest(62-66)ImpersonationRequestResponse(54-56)constants/requests.ts (1)
REQUEST_STATE(1-5)
🪛 GitHub Check: CodeQL
routes/impersonation.ts
[failure] 10-10: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
This route handler performs authorization, but is not rate-limited.
This route handler performs authorization, but is not rate-limited.
🪛 ESLint
routes/impersonation.ts
[error] 8-13: Replace ⏎··"/requests",⏎··authenticate,⏎··getImpersonationRequestsValidator,⏎··getImpersonationRequestsController⏎ with "/requests",·authenticate,·getImpersonationRequestsValidator,·getImpersonationRequestsController
(prettier/prettier)
[error] 15-15: Insert ⏎
(prettier/prettier)
🪛 Biome (1.9.4)
middlewares/validators/impersonationRequests.ts
[error] 30-30: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 35-35: Do not add then to an object.
(lint/suspicious/noThenProperty)
models/impersonationRequests.ts
[error] 77-77: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 84-84: This let declares a variable that is only assigned once.
'allRequests' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.10.0)
🔇 Additional comments (3)
controllers/impersonationRequests.ts (1)
16-19: Feature flag implementation acknowledged.The dev flag check aligns with the PR objectives for feature flag implementation.
middlewares/validators/impersonationRequests.ts (2)
28-38: Joi conditional validation is correctly implemented.The static analysis warning about the
thenproperty is a false positive. This is the standard Joi syntax for conditional validation, and the mutual exclusivity rules for pagination parameters are properly implemented.🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 35-35: Do not add then to an object.
(lint/suspicious/noThenProperty)
20-20: TODO comment acknowledged.The temporary
devvalidator aligns with the feature flag implementation mentioned in the PR objectives.
c38a745 to
0901c70
Compare
1ecb1dd to
561db31
Compare
670cf97 to
cd63722
Compare
…Suvidh-kaushik/website-backend into feat/get_impersonation_requests
b002389 to
2a53bca
Compare
Achintya-Chatterjee
left a comment
There was a problem hiding this comment.
My comments have been resolved, so I am approving this PR.
…ackend into feat/get_impersonation_requests
Achintya-Chatterjee
left a comment
There was a problem hiding this comment.
my comments has been resolved, hence approving this PR
controllers/impersonationRequests.ts
Outdated
| if (query.id) { | ||
| const request = await getImpersonationRequestById(query.id); | ||
| if (!request) { | ||
| return res.status(204).send(); | ||
| } | ||
| return res.status(200).json({ | ||
| message: REQUEST_FETCHED_SUCCESSFULLY, | ||
| data: request, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Why query param for a single record?
There was a problem hiding this comment.
if I use id as a path param instead of a query param, i would need to create another route which handles this specifically else in the same route I cannot send both the request by id and filter the requests also I have tried keeping this route consistent with the existing requests endpoint which handles this similarly
models/impersonationRequests.ts
Outdated
| if (page) { | ||
| const startAfter = (page - 1) * size; | ||
| requestQueryDoc = requestQueryDoc.offset(startAfter); |
There was a problem hiding this comment.
Why is page required?
Do you know the limitations of using page in firesotre
There was a problem hiding this comment.
Sorry sir, I did not about this but from what I learned now,
in firestore, the offset skips the documents, but it internally still reads all these skipped docs and we are billed for this extra reading also
I added this because exisitng requests has both cursor and page based pagination
I will remove page based pagination and keep only the cursor based
| if (createdBy) { | ||
| requestQuery = requestQuery.where("createdBy", "==", createdBy); | ||
| } | ||
| if (status) { | ||
| requestQuery = requestQuery.where("status", "==", status); | ||
| } | ||
| if (createdFor) { | ||
| requestQuery = requestQuery.where("createdFor", "==", createdFor); |
There was a problem hiding this comment.
Will we need to create index for the queries?
There was a problem hiding this comment.
yes, These queries require indexes I have created an issue link for this and tagged you - ISSUE LINK
8c27d4c
into
RealDevSquad:develop
Date: 11/06/2025
Developer Name: Suvidh Kaushik
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Recordings
fetch_by_createdFor.mp4
pagination_and_fetch_by_size.mp4
fetch_by_createdBy.mp4
fetch_by_status.mp4
fetch_by_id.mp4
fetch_by_createdBy.mp4
Test Coverage
Screenshot 1
Additional Notes
Design Doc - LINK
PRD - LINK
Description by Korbit AI
What change is being made?
Add new API endpoints to fetch impersonation requests, either by ID or with pagination and filtering options, including the necessary controllers, validators, models, and route integrations.
Why are these changes being made?
To enhance the API's capabilities by allowing clients to retrieve specific impersonation requests using their IDs and to support filtering and pagination for improved usability and data management. This implementation helps streamline access and organization of impersonation requests as part of our system's broader administrative and security functionalities.