Feat/user ooo approve reject #2425
Conversation
Summary by CodeRabbit
WalkthroughThis update introduces the ability to acknowledge Out-of-Office (OOO) requests. It adds new controller, service, model, middleware, and type definitions to support this process, along with validation logic and error handling. Comprehensive tests are included for both the new functionality and its validation, covering both positive and negative cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Validator
participant Controller
participant Service
participant Model
participant Logger
Client->>Validator: Send PATCH /requests/:id (OOO)
Validator->>Controller: Validate request body
Controller->>Service: acknowledgeOooRequest(requestId, body, userId)
Service->>Model: getRequestById(requestId)
Model-->>Service: Return request data
Service->>Service: validateOooAcknowledgeRequest(type, status)
Service->>Model: updateRequest(requestId, updateData)
Model-->>Service: Return update result
Service->>Logger: Log approval or rejection
Service->>Controller: Return success response
Controller->>Client: Respond with status/message
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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:
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: 14
🔭 Outside diff range comments (3)
test/unit/middlewares/oooRequests.test.ts (1)
31-33: 🧹 Nitpick (assertive)Make assertion explicit
expect(nextSpy.calledOnce);passes as long as the value is truthy, but it doesn’t explicitly assert the boolean outcome.
Changing it toexpect(nextSpy.calledOnce).to.be.true;will fail loudly when the spy is not called once and makes the intent crystal-clear.- await createOooStatusRequestValidator(req as any, res as any, nextSpy); - expect(nextSpy.calledOnce); + await createOooStatusRequestValidator(req as any, res as any, nextSpy); + expect(nextSpy.calledOnce).to.be.true;types/oooRequest.d.ts (1)
28-35:⚠️ Potential issueBroken composite type – causes TypeScript compilation failure
OooRequestUpdateRequestreferences a non-existent identifieroooRequestUpdateBodyand omits abodykey. TS will error with “Cannot find name ‘oooRequestUpdateBody’”.-export type OooRequestUpdateRequest = Request & { oooRequestUpdateBody , userData: userData , query: RequestQuery , params: RequestParams }; +export type OooRequestUpdateRequest = Request & { + body: OooRequestUpdateBody; + userData: userData; + query: RequestQuery; + params: RequestParams; +};Additionally,
admin.firestore.Timestampis referenced butadminisn’t imported, producing another ambient-type error whenskipLibCheckis false.-import { Request, Response } from "express"; +import { Request, Response } from "express"; +import * as admin from "firebase-admin";Without these fixes the build will break.
Also applies to: 44-45
test/unit/services/oooRequest.test.ts (1)
117-138: 🧹 Nitpick (assertive)Consider using try/catch for consistency
The test for invalid request type validation successfully verifies the error message, but it uses the await/catch pattern which differs from other error handling patterns in the file (like in the createOooRequest tests). For consistency, consider using try/catch.
- await validateOooAcknowledgeRequest( - REQUEST_TYPE.ONBOARDING, - testOooRequest.status - ).catch((error) => { - expect(error).to.be.not.undefined; - expect(error.message).to.equal(INVALID_REQUEST_TYPE); - }); + try { + await validateOooAcknowledgeRequest( + REQUEST_TYPE.ONBOARDING, + testOooRequest.status + ); + expect.fail("Expected an error to be thrown"); + } catch (error) { + expect(error).to.be.not.undefined; + expect(error.message).to.equal(INVALID_REQUEST_TYPE); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
constants/requests.ts(2 hunks)controllers/oooRequests.ts(3 hunks)controllers/requests.ts(3 hunks)middlewares/validators/oooRequests.ts(2 hunks)middlewares/validators/requests.ts(3 hunks)models/requests.ts(2 hunks)services/oooRequest.ts(2 hunks)test/fixtures/oooRequest/oooRequest.ts(1 hunks)test/integration/requests.test.ts(13 hunks)test/unit/middlewares/oooRequests.test.ts(2 hunks)test/unit/models/requests.test.ts(2 hunks)test/unit/services/oooRequest.test.ts(5 hunks)types/oooRequest.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
test/fixtures/oooRequest/oooRequest.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.
test/integration/requests.test.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.
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.
test/unit/services/oooRequest.test.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 (6)
models/requests.ts (1)
constants/requests.ts (2)
REQUEST_DOES_NOT_EXIST(47-47)ERROR_WHILE_FETCHING_REQUEST(41-41)
test/integration/requests.test.ts (2)
test/fixtures/oooRequest/oooRequest.ts (1)
testAcknowledgeOooRequest(171-175)constants/requests.ts (2)
UNAUTHORIZED_TO_UPDATE_REQUEST(74-74)REQUEST_STATE(1-5)
test/unit/models/requests.test.ts (3)
models/requests.ts (1)
getRequestById(73-86)test/fixtures/oooRequest/oooRequest.ts (1)
createOooRequests3(162-169)constants/requests.ts (1)
REQUEST_DOES_NOT_EXIST(47-47)
middlewares/validators/requests.ts (4)
types/onboardingExtension.d.ts (1)
UpdateOnboardingExtensionRequest(66-71)types/oooRequest.d.ts (2)
AcknowledgeOooRequest(56-61)OooRequestResponse(37-37)constants/requests.ts (1)
REQUEST_TYPE(13-19)middlewares/validators/oooRequests.ts (1)
acknowledgeOooRequest(70-83)
types/oooRequest.d.ts (2)
types/requests.d.ts (2)
RequestQuery(11-20)RequestParams(22-24)constants/requests.ts (2)
REQUEST_TYPE(13-19)REQUEST_STATE(1-5)
controllers/oooRequests.ts (4)
middlewares/validators/oooRequests.ts (1)
acknowledgeOooRequest(70-83)services/oooRequest.ts (1)
acknowledgeOooRequest(148-203)types/oooRequest.d.ts (2)
AcknowledgeOooRequest(56-61)OooRequestResponse(37-37)constants/requests.ts (3)
UNAUTHORIZED_TO_UPDATE_REQUEST(74-74)REQUEST_ID_REQUIRED(46-46)ERROR_WHILE_ACKNOWLEDGING_REQUEST(44-44)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.10.0)
🔇 Additional comments (22)
constants/requests.ts (3)
29-30: Well-implemented request log type constants.These new log types follow the established naming pattern and will be useful for tracking when requests are attempted to be acknowledged but are already in a finalized state.
44-44: Appropriate error message for the new acknowledge feature.This error message aligns well with the existing error message pattern and clearly describes the error context.
46-46: Good validation message for request ID requirement.This validation message follows the existing pattern and will be helpful for providing clear feedback when a request ID is missing.
test/fixtures/oooRequest/oooRequest.ts (1)
171-171: Properly renamed test fixture for better clarity.Renaming from
acknowledgeOooRequesttotestAcknowledgeOooRequestclearly indicates this is a test fixture, improving code readability.As per the previous feedback, I note that the inconsistent property naming in these fixtures (
message/statevsreason/status) will be addressed in a dedicated PR.models/requests.ts (1)
11-11: Appropriate import for HTTP error handling.The NotFound error type is correctly imported from http-errors package to support the new functionality.
test/integration/requests.test.ts (4)
18-18: Updated imports to support acknowledgment functionality.The imports have been correctly updated to use the renamed test fixture and to include the appropriate authorization error constant.
Also applies to: 33-33
326-326: Test suite reactivated appropriately.The PATCH
/requests/:idtest suite has been properly reactivated from being skipped, which is good for ensuring the new functionality is tested.
355-355: Consistently updated test payload references.All instances of the test payload variable have been updated to use
testAcknowledgeOooRequestinstead ofacknowledgeOooRequest, ensuring consistency with the renamed fixture.Also applies to: 369-369, 385-385, 401-401, 417-417, 433-433, 449-449, 465-465, 481-481, 498-498
407-407: Updated error message assertion.The error message assertion has been correctly updated to use
UNAUTHORIZED_TO_UPDATE_REQUEST, which aligns with the current authorization error handling in the controllers.test/unit/models/requests.test.ts (1)
183-198: Test coverage for getRequestById looks good.The implementation provides thorough coverage by testing both successful retrieval and error handling scenarios. The tests verify that:
- The function correctly returns request data with all expected properties
- The function properly handles non-existent requests with the appropriate error message
This complements the implementation of
getRequestByIdin the models layer, which is essential for the new OOO request acknowledgment functionality.middlewares/validators/oooRequests.ts (1)
42-60: Validation schema looks good.The schema appropriately validates the acknowledge OOO request payload with proper constraints:
- Optional comment that cannot be empty if provided
- Required status limited to APPROVED or REJECTED
- Required type that must equal OOO
middlewares/validators/requests.ts (2)
4-5: Imports look good.The imports for the new OOO acknowledgment functionality are correctly added.
133-137: Type signature correctly updated.The function signature is properly updated to support the new OOO request type, which is essential for proper type checking and IDE support.
controllers/requests.ts (1)
127-129: OOO request acknowledgment integration looks good.The OOO request case is properly integrated into the controller switch statement, enabling proper routing of OOO acknowledgment requests to the specialized controller.
test/unit/services/oooRequest.test.ts (8)
17-20: Function imports look appropriateThe new imports for
acknowledgeOooRequestandvalidateOooAcknowledgeRequestare correctly added to support the OOO request validation and acknowledgment functionality.
28-32: Good import choices for testing requirementsThe additional imports provide the necessary test fixtures and error types needed for proper testing of the OOO request acknowledgment flow. The HTTP error classes will allow for proper error type validation.
160-166: Well-structured happy path testThe test for successful validation is clear and correctly verifies that no error is returned when all validation checks pass.
187-197: Well-implemented test for non-existent request scenarioThe test correctly uses sinon to stub the dependency and verifies the appropriate error is thrown when a non-existent request ID is provided.
199-210: Good validation of already approved requestsThe test effectively verifies the behavior when a request has already been approved by stubbing the validation function to throw the appropriate error.
212-224: Complete error handling testThis test covers the scenario where the update operation fails, showing that the service properly propagates errors from its dependencies.
239-250: Good test for rejection flowThe test effectively verifies the behavior when rejecting an OOO request, including checking the appropriate response message and status.
252-263: Effective error propagation testThis test verifies that unexpected errors from dependencies are properly caught and propagated, ensuring robust error handling in the service.
| export const getRequestById = async (id: string) => { | ||
| try { | ||
| const requestDoc = await requestModel.doc(id).get(); | ||
|
|
||
| if (!requestDoc.exists) { | ||
| throw new NotFound(REQUEST_DOES_NOT_EXIST); | ||
| } | ||
|
|
||
| return requestDoc.data(); | ||
| } catch (error) { | ||
| logger.error(ERROR_WHILE_FETCHING_REQUEST, error); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Well-implemented getRequestById function.
The function correctly retrieves a request by ID and returns appropriate errors when the document doesn't exist. The error handling follows the established pattern in the codebase.
Consider adding JSDoc documentation to clarify the function's purpose and parameters:
+/**
+ * Retrieves a request document by its ID
+ * @param {string} id - The ID of the request to retrieve
+ * @returns {Promise<any>} The request document data
+ * @throws {NotFound} When the request document doesn't exist
+ */
export const getRequestById = async (id: string) => {
try {
const requestDoc = await requestModel.doc(id).get();
if (!requestDoc.exists) {
throw new NotFound(REQUEST_DOES_NOT_EXIST);
}
return requestDoc.data();
} catch (error) {
logger.error(ERROR_WHILE_FETCHING_REQUEST, error);
throw error;
}
};📝 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.
| export const getRequestById = async (id: string) => { | |
| try { | |
| const requestDoc = await requestModel.doc(id).get(); | |
| if (!requestDoc.exists) { | |
| throw new NotFound(REQUEST_DOES_NOT_EXIST); | |
| } | |
| return requestDoc.data(); | |
| } catch (error) { | |
| logger.error(ERROR_WHILE_FETCHING_REQUEST, error); | |
| throw error; | |
| } | |
| }; | |
| /** | |
| * Retrieves a request document by its ID | |
| * @param {string} id - The ID of the request to retrieve | |
| * @returns {Promise<any>} The request document data | |
| * @throws {NotFound} When the request document doesn't exist | |
| */ | |
| export const getRequestById = async (id: string) => { | |
| try { | |
| const requestDoc = await requestModel.doc(id).get(); | |
| if (!requestDoc.exists) { | |
| throw new NotFound(REQUEST_DOES_NOT_EXIST); | |
| } | |
| return requestDoc.data(); | |
| } catch (error) { | |
| logger.error(ERROR_WHILE_FETCHING_REQUEST, error); | |
| throw error; | |
| } | |
| }; |
| export const acknowledgeOooRequest = async ( | ||
| req: AcknowledgeOooRequest, | ||
| res: OooRequestResponse, | ||
| next: NextFunction | ||
| ): Promise<void> => { | ||
| try { | ||
| await schema.validateAsync(req.body, { abortEarly: false }); | ||
| next(); | ||
| } catch (error) { | ||
| const errorMessages = error.details.map((detail:{message: string}) => detail.message); | ||
| logger.error(`Error while validating request payload : ${errorMessages}`); | ||
| return res.boom.badRequest(errorMessages); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing logger import.
The middleware uses logger.error() on line 80, but there's no import statement for the logger.
Add the missing import at the top of the file:
import joi from "joi";
import { NextFunction } from "express";
import { REQUEST_STATE, REQUEST_TYPE } from "../../constants/requests";
import { AcknowledgeOooRequest, OooRequestCreateRequest, OooRequestResponse } from "../../types/oooRequest";
+import logger from "../../utils/logger";📝 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.
| export const acknowledgeOooRequest = async ( | |
| req: AcknowledgeOooRequest, | |
| res: OooRequestResponse, | |
| next: NextFunction | |
| ): Promise<void> => { | |
| try { | |
| await schema.validateAsync(req.body, { abortEarly: false }); | |
| next(); | |
| } catch (error) { | |
| const errorMessages = error.details.map((detail:{message: string}) => detail.message); | |
| logger.error(`Error while validating request payload : ${errorMessages}`); | |
| return res.boom.badRequest(errorMessages); | |
| } | |
| }; | |
| // middlewares/validators/oooRequests.ts | |
| import joi from "joi"; | |
| import { NextFunction } from "express"; | |
| import { REQUEST_STATE, REQUEST_TYPE } from "../../constants/requests"; | |
| import { AcknowledgeOooRequest, OooRequestCreateRequest, OooRequestResponse } from "../../types/oooRequest"; | |
| +import logger from "../../utils/logger"; | |
| export const acknowledgeOooRequest = async ( | |
| req: AcknowledgeOooRequest, | |
| res: OooRequestResponse, | |
| next: NextFunction | |
| ): Promise<void> => { | |
| try { | |
| await schema.validateAsync(req.body, { abortEarly: false }); | |
| next(); | |
| } catch (error) { | |
| const errorMessages = error.details.map((detail:{message: string}) => detail.message); | |
| logger.error(`Error while validating request payload : ${errorMessages}`); | |
| return res.boom.badRequest(errorMessages); | |
| } | |
| }; |
| case REQUEST_TYPE.OOO: | ||
| await acknowledgeOooRequest( | ||
| req, | ||
| res as OooRequestResponse, next); | ||
| break; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
OOO request handling integration looks good, minor formatting issue.
The OOO request case is properly integrated into the validator switch statement, but there's inconsistent spacing after the comma.
Improve formatting consistency:
case REQUEST_TYPE.OOO:
await acknowledgeOooRequest(
req,
- res as OooRequestResponse, next);
+ res as OooRequestResponse,
+ next);
break;📝 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.
| case REQUEST_TYPE.OOO: | |
| await acknowledgeOooRequest( | |
| req, | |
| res as OooRequestResponse, next); | |
| break; | |
| case REQUEST_TYPE.OOO: | |
| await acknowledgeOooRequest( | |
| req, | |
| res as OooRequestResponse, | |
| next); | |
| break; |
| @@ -121,9 +120,13 @@ export const getRequestsController = async (req: any, res: any) => { | |||
| * @param {CustomResponse} res - The response object. | |||
| * @returns {Promise<void>} Resolves or sends an error for invalid types. | |||
| */ | |||
| export const updateRequestBeforeAcknowledgedController = async (req: Request, res: CustomResponse) => { | |||
| export const updateRequestBeforeAcknowledgedController = async (req: Request, res: CustomResponse, next: NextFunction) => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Updated function signature with missing JSDoc.
The function signature has been updated to include the next parameter needed for middleware chaining, but the JSDoc comment doesn't reflect this change.
Update the JSDoc comment to include the next parameter:
/**
* Processes update requests before acknowledgment based on type.
*
* @param {Request} req - The request object.
* @param {CustomResponse} res - The response object.
+ * @param {NextFunction} next - The next middleware function.
* @returns {Promise<void>} Resolves or sends an error for invalid types.
*/
export const updateRequestBeforeAcknowledgedController = async (req: Request, res: CustomResponse, next: NextFunction) => {📝 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.
| export const updateRequestBeforeAcknowledgedController = async (req: Request, res: CustomResponse, next: NextFunction) => { | |
| /** | |
| * Processes update requests before acknowledgment based on type. | |
| * | |
| * @param {Request} req - The request object. | |
| * @param {CustomResponse} res - The response object. | |
| * @param {NextFunction} next - The next middleware function. | |
| * @returns {Promise<void>} Resolves or sends an error for invalid types. | |
| */ | |
| export const updateRequestBeforeAcknowledgedController = async (req: Request, res: CustomResponse, next: NextFunction) => { |
| await acknowledgeOooRequest(req, res, nextSpy); | ||
| expect(nextSpy.notCalled).to.be.true; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replicate explicit assertion style for consistency
For readability and uniformity, assert .calledOnce the same way you assert .notCalled.
- await acknowledgeOooRequest(req, res, nextSpy);
- expect(nextSpy.calledOnce);
+ await acknowledgeOooRequest(req, res, nextSpy);
+ expect(nextSpy.calledOnce).to.be.true;Apply the same change to lines 118-120 & 126-129.
This keeps the test suite consistent and easier to scan.
Also applies to: 118-120, 126-129
| export const validateOooAcknowledgeRequest = async ( | ||
| requestType: string, | ||
| requestStatus: string, | ||
| ) => { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
async keyword unnecessary
validateOooAcknowledgeRequest performs only synchronous checks. Dropping async avoids wrapping the return value in an unnecessary promise and makes intent clearer.
-export const validateOooAcknowledgeRequest = async (
+export const validateOooAcknowledgeRequest = (📝 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.
| export const validateOooAcknowledgeRequest = async ( | |
| requestType: string, | |
| requestStatus: string, | |
| ) => { | |
| export const validateOooAcknowledgeRequest = ( | |
| requestType: string, | |
| requestStatus: string, | |
| ) => { |
| /** | ||
| * Validates an Out-Of-Office (OOO) acknowledge request | ||
| * | ||
| * @param {string} requestId - The unique identifier of the request. | ||
| * @param {string} requestType - The type of the request (expected to be 'OOO'). | ||
| * @param {string} requestStatus - The current status of the request. | ||
| * @throws {Error} Throws an error if an issue occurs during validation. | ||
| */ | ||
| export const validateOooAcknowledgeRequest = async ( | ||
| requestType: string, | ||
| requestStatus: string, | ||
| ) => { | ||
|
|
||
| try { | ||
|
|
||
| if (requestType !== REQUEST_TYPE.OOO) { | ||
| throw new BadRequest(INVALID_REQUEST_TYPE); | ||
| } | ||
|
|
||
| if (requestStatus === REQUEST_STATE.APPROVED) { | ||
| throw new Conflict(REQUEST_ALREADY_APPROVED); | ||
| } | ||
|
|
||
| if (requestStatus === REQUEST_STATE.REJECTED) { | ||
| throw new Conflict(REQUEST_ALREADY_REJECTED); | ||
| } | ||
| } catch (error) { | ||
| logger.error("Error while validating OOO acknowledge request", error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
JSDoc parameters don’t match implementation
The comment lists @param {string} requestId, requestType, requestStatus, but the function actually receives only requestType & requestStatus.
Keeping docs accurate prevents false assumptions:
- * @param {string} requestId - The unique identifier of the request.
- * @param {string} requestType - The type of the request (expected to be 'OOO').
- * @param {string} requestStatus - The current status of the request.
+ * @param {string} requestType - The type of the request (expected to be 'OOO').
+ * @param {string} requestStatus - The current status of the request.📝 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.
| /** | |
| * Validates an Out-Of-Office (OOO) acknowledge request | |
| * | |
| * @param {string} requestId - The unique identifier of the request. | |
| * @param {string} requestType - The type of the request (expected to be 'OOO'). | |
| * @param {string} requestStatus - The current status of the request. | |
| * @throws {Error} Throws an error if an issue occurs during validation. | |
| */ | |
| export const validateOooAcknowledgeRequest = async ( | |
| requestType: string, | |
| requestStatus: string, | |
| ) => { | |
| try { | |
| if (requestType !== REQUEST_TYPE.OOO) { | |
| throw new BadRequest(INVALID_REQUEST_TYPE); | |
| } | |
| if (requestStatus === REQUEST_STATE.APPROVED) { | |
| throw new Conflict(REQUEST_ALREADY_APPROVED); | |
| } | |
| if (requestStatus === REQUEST_STATE.REJECTED) { | |
| throw new Conflict(REQUEST_ALREADY_REJECTED); | |
| } | |
| } catch (error) { | |
| logger.error("Error while validating OOO acknowledge request", error); | |
| throw error; | |
| } | |
| } | |
| /** | |
| * Validates an Out-Of-Office (OOO) acknowledge request | |
| * | |
| * @param {string} requestType - The type of the request (expected to be 'OOO'). | |
| * @param {string} requestStatus - The current status of the request. | |
| * @throws {Error} Throws an error if an issue occurs during validation. | |
| */ | |
| export const validateOooAcknowledgeRequest = async ( | |
| requestType: string, | |
| requestStatus: string, | |
| ) => { | |
| try { | |
| if (requestType !== REQUEST_TYPE.OOO) { | |
| throw new BadRequest(INVALID_REQUEST_TYPE); | |
| } | |
| if (requestStatus === REQUEST_STATE.APPROVED) { | |
| throw new Conflict(REQUEST_ALREADY_APPROVED); | |
| } | |
| if (requestStatus === REQUEST_STATE.REJECTED) { | |
| throw new Conflict(REQUEST_ALREADY_REJECTED); | |
| } | |
| } catch (error) { | |
| logger.error("Error while validating OOO acknowledge request", error); | |
| throw error; | |
| } | |
| } |
| it("should return REQUEST_ALREADY_APPROVED if request is already approved", async function() { | ||
| // const validationResponse = await validateOOOAcknowledgeRequest( | ||
| // testOooRequest.id, | ||
| // testOooRequest.type, | ||
| // REQUEST_STATE.APPROVED | ||
| // ); | ||
| // expect(validationResponse.error).to.be.not.undefined; | ||
| // expect(validationResponse.error).to.equal(REQUEST_ALREADY_APPROVED); | ||
| await validateOooAcknowledgeRequest( | ||
| testOooRequest.type, | ||
| REQUEST_STATE.APPROVED | ||
| ).catch((error) => { | ||
| expect(error).to.be.not.undefined; | ||
| expect(error.message).to.equal(REQUEST_ALREADY_APPROVED); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider asserting error type
The test verifies the error message for already approved requests, but it would be beneficial to also assert the specific error type (like Conflict) to ensure the correct error classification.
await validateOooAcknowledgeRequest(
testOooRequest.type,
REQUEST_STATE.APPROVED
).catch((error) => {
expect(error).to.be.not.undefined;
expect(error.message).to.equal(REQUEST_ALREADY_APPROVED);
+ expect(error).to.be.an.instanceOf(Conflict);
});📝 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.
| it("should return REQUEST_ALREADY_APPROVED if request is already approved", async function() { | |
| // const validationResponse = await validateOOOAcknowledgeRequest( | |
| // testOooRequest.id, | |
| // testOooRequest.type, | |
| // REQUEST_STATE.APPROVED | |
| // ); | |
| // expect(validationResponse.error).to.be.not.undefined; | |
| // expect(validationResponse.error).to.equal(REQUEST_ALREADY_APPROVED); | |
| await validateOooAcknowledgeRequest( | |
| testOooRequest.type, | |
| REQUEST_STATE.APPROVED | |
| ).catch((error) => { | |
| expect(error).to.be.not.undefined; | |
| expect(error.message).to.equal(REQUEST_ALREADY_APPROVED); | |
| }); | |
| }); | |
| it("should return REQUEST_ALREADY_APPROVED if request is already approved", async function() { | |
| await validateOooAcknowledgeRequest( | |
| testOooRequest.type, | |
| REQUEST_STATE.APPROVED | |
| ).catch((error) => { | |
| expect(error).to.be.not.undefined; | |
| expect(error.message).to.equal(REQUEST_ALREADY_APPROVED); | |
| expect(error).to.be.an.instanceOf(Conflict); | |
| }); | |
| }); |
| it("should approve OOO request", async function() { | ||
| // const response = await acknowledgeOOORequest( | ||
| // testOooRequest.id, | ||
| // acknowledgeOooRequest, | ||
| // testSuperUserId | ||
| // ); | ||
| // expect(response).to.deep.include({ | ||
| // message: REQUEST_APPROVED_SUCCESSFULLY, | ||
| // data: { | ||
| // ...acknowledgeOooRequest, | ||
| // id: testOooRequest.id, | ||
| // lastModifiedBy: testSuperUserId, | ||
| // updatedAt: response.data.updatedAt | ||
| // } | ||
| // }); | ||
| sinon.stub(requestModel, "getRequestById").returns(testOooRequest); | ||
| sinon.stub(oooRequestService, "validateOooAcknowledgeRequest"); | ||
| sinon.stub(requestModel, "updateRequest").returns({ ...testOooRequest, status: REQUEST_STATE.APPROVED}); | ||
| const response = await acknowledgeOooRequest( | ||
| testOooRequest.id, | ||
| testAcknowledgeOooRequest, | ||
| testSuperUserId | ||
| ); | ||
| expect(response.message).to.equal(REQUEST_APPROVED_SUCCESSFULLY); | ||
| expect(response.data.status).to.equal(REQUEST_STATE.APPROVED); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Verify return object structure for approval
The test for approving an OOO request checks the message and status, but could be enhanced by verifying the complete structure of the returned object.
expect(response.message).to.equal(REQUEST_APPROVED_SUCCESSFULLY);
expect(response.data.status).to.equal(REQUEST_STATE.APPROVED);
+ expect(response).to.have.property('data');
+ expect(response.data).to.have.all.keys(['id', 'status', 'type', 'userId', /* other expected properties */]);| it("should return REQUEST_ALREADY_REJECTED if request is already rejected", async function() { | ||
| // const validationResponse = await validateOOOAcknowledgeRequest( | ||
| // testOooRequest.id, | ||
| // testOooRequest.type, | ||
| // REQUEST_STATE.REJECTED | ||
| // ); | ||
| // expect(validationResponse.error).to.be.not.undefined; | ||
| // expect(validationResponse.error).to.equal(REQUEST_ALREADY_REJECTED); | ||
| await validateOooAcknowledgeRequest( | ||
| testOooRequest.type, | ||
| REQUEST_STATE.REJECTED | ||
| ).catch((error) => { | ||
| expect(error).to.be.not.undefined; | ||
| expect(error.message).to.equal(REQUEST_ALREADY_REJECTED); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider asserting error type
Similar to the previous test, consider verifying both the error message and type for consistency and complete validation.
await validateOooAcknowledgeRequest(
testOooRequest.type,
REQUEST_STATE.REJECTED
).catch((error) => {
expect(error).to.be.not.undefined;
expect(error.message).to.equal(REQUEST_ALREADY_REJECTED);
+ expect(error).to.be.an.instanceOf(Conflict);
});📝 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.
| it("should return REQUEST_ALREADY_REJECTED if request is already rejected", async function() { | |
| // const validationResponse = await validateOOOAcknowledgeRequest( | |
| // testOooRequest.id, | |
| // testOooRequest.type, | |
| // REQUEST_STATE.REJECTED | |
| // ); | |
| // expect(validationResponse.error).to.be.not.undefined; | |
| // expect(validationResponse.error).to.equal(REQUEST_ALREADY_REJECTED); | |
| await validateOooAcknowledgeRequest( | |
| testOooRequest.type, | |
| REQUEST_STATE.REJECTED | |
| ).catch((error) => { | |
| expect(error).to.be.not.undefined; | |
| expect(error.message).to.equal(REQUEST_ALREADY_REJECTED); | |
| }); | |
| }); | |
| it("should return REQUEST_ALREADY_REJECTED if request is already rejected", async function() { | |
| await validateOooAcknowledgeRequest( | |
| testOooRequest.type, | |
| REQUEST_STATE.REJECTED | |
| ).catch((error) => { | |
| expect(error).to.be.not.undefined; | |
| expect(error.message).to.equal(REQUEST_ALREADY_REJECTED); | |
| expect(error).to.be.an.instanceOf(Conflict); | |
| }); | |
| }); |
Date:
Developer Name:
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes