Fix: feature to create OOO request#2383
Fix: feature to create OOO request#2383prakashchoudhary07 merged 9 commits intoRealDevSquad:developfrom
Conversation
surajmaity1
force-pushed
the
feat/user-ooo-create
branch
5 times, most recently
from
7528d1b to
2201ae4
Compare
|
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 WalkthroughThis pull request enhances out-of-office request handling. It introduces new error and log constants, refines controller logic for OOO requests (with explicit Discord user checks and NextFunction parameters), and updates the Joi validation schema and associated test fixtures to replace "message" with "reason" and remove the "state" field. Additionally, type definitions have been strengthened, and new service functions for user status validation and request creation have been added. Test cases across integration and unit tests have been updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Controller (createOooRequestController)
participant S as Service (createOOORequest)
participant V as Service (validateUserStatus)
participant DB as DataStore/Logger
C->>S: Invoke createOOORequest(body, username, userId)
S->>V: Call validateUserStatus(userId, userStatusExists, userStatus)
V-->>S: Return validation result / throw error
S->>DB: Call createRequest & addLog
DB-->>S: Return success confirmation
S-->>C: Return created OOO request response
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 (
|
surajmaity1
force-pushed
the
feat/user-ooo-create
branch
from
2201ae4 to
aa44d5b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
controllers/oooRequests.ts (2)
31-33: Confirm the development-only feature toggle.
Requiringdev=trueto access this functionality is a strong gate. If this feature is intended for production soon, consider using environment-based feature flags for better clarity.
55-63: Possible race condition risk.
Between checking for a pending OOO request and creating one, another request could slip in. Consider a unique constraint on userId + status or a transaction-based approach to avoid duplicates in a high-concurrency scenario.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
constants/requests.ts(1 hunks)controllers/oooRequests.ts(1 hunks)middlewares/validators/oooRequests.ts(1 hunks)test/fixtures/oooRequest/oooRequest.ts(2 hunks)test/integration/requests.test.ts(9 hunks)test/unit/middlewares/oooRequests.test.ts(2 hunks)types/oooRequest.d.ts(2 hunks)
🔇 Additional comments (19)
middlewares/validators/oooRequests.ts (1)
29-36: Field rename and validation improvements.The field renaming from
messagetoreasonimproves semantic clarity for OOO requests. The validation has been enhanced with clear error messages and strict type validation.test/unit/middlewares/oooRequests.test.ts (2)
45-56: Test updated for the renamed field.The test has been correctly updated to check for validation of the
reasonfield instead ofmessage.
64-75: Updated test case for all invalid values.The test now correctly checks for the new field name and has adjusted the expected error count from 4 to 3.
constants/requests.ts (1)
44-47:Details
✅ Verification successful
New error constants for OOO requests.
These new constants provide clearer, more specific error messages for OOO request validation scenarios. This improves maintainability and user experience.
Let's verify these constants are being correctly used in the controller:
🏁 Script executed:
#!/bin/bash # Search for usages of the new constants in the controllers rg "UNAUTHORIZED_TO_CREATE_OOO_REQUEST|ONLY_DISCORD_USER_CREATE_OOO_REQUEST|USER_STATUS_NOT_FOUND|OOO_STATUS_ALREADY_EXIST" controllers/Length of output: 741
Usage of new error constants confirmed in the OOO request controller.
The grep output shows that all four constants fromconstants/requests.tsare properly imported and used incontrollers/oooRequests.tsto provide specific error responses. No further changes are required.test/integration/requests.test.ts (5)
32-32: New imports and constants.Good addition of the user status update functionality and centralizing the endpoint with a query parameter.
Also applies to: 48-48
69-75: Added user status initialization.Setting the user status to ACTIVE before tests is a good practice to ensure consistent test environment. The implementation correctly uses the imported updateUserStatus function.
108-122: Status code change for duplicate requests.Changed from 400 (Bad Request) to 409 (Conflict) for duplicate OOO requests, which is semantically more appropriate for this scenario.
168-180: Updated tests for reason field.Tests have been correctly updated to validate the new
reasonfield instead ofmessage.
182-194: Updated tests for status field.Tests have been correctly updated to validate the field name change from
statetostatus.test/fixtures/oooRequest/oooRequest.ts (1)
18-18: Looks consistent with the new naming convention.
Renamingmessagetoreasonaligns well with the updated schema.controllers/oooRequests.ts (6)
12-15: New error constants integrated correctly.
The introduced constants (e.g.,OOO_STATUS_ALREADY_EXIST) improve clarity and maintainability of error handling.
17-23: Imports are well-aligned with usage.
Bringing instatusState,userState,addFutureStatus,getUserStatus, and the new type definitions is consistent with the updated logic below.
27-30: Type-safe request body and user data usage.
Castingreq.bodytoOooStatusRequestBodyensures correctness, and extractingusernameandrolesfromuserDatais straightforward.
36-40: Access checks look correct.
ReturningunauthorizedifuserIdis missing or if the user is not in Discord helps ensure only valid and authorized requests pass through.
45-51: User status checks are well-handled.
The logic ensures the user must exist and not already be in OOO status before creating new OOO records. This is a robust reduction of misuse.
65-72: Request creation is properly structured.
The request object includes all necessary fields, includingrequestedBy,userId, and the pending status. This step appears correct and consistent with the updated schema.types/oooRequest.d.ts (3)
11-19: Greater clarity with required fields.
Enforcinguntil,reason,status,requestedBy,userId, etc. as required inOooStatusRequestimproves correctness and consistency.
24-25: Renaming toreasonaligns with the updated fields.
Changing frommessagetoreasonensures consistency between the type definitions and the rest of the codebase.
38-42: Cleaner request shape.
TheOooRequestCreateRequesttype neatly groups thebody,userData, andquery, improving type safety in the controller.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (19)
test/unit/services/oooRequest.test.ts (11)
5-5: Improve import statement formatting.The import statement is very long and could be better formatted for readability. Consider breaking it into multiple lines.
-import { OOO_STATUS_ALREADY_EXIST, REQUEST_ALREADY_PENDING, REQUEST_TYPE, USER_STATUS_NOT_FOUND } from "../../../constants/requests"; +import { + OOO_STATUS_ALREADY_EXIST, + REQUEST_ALREADY_PENDING, + REQUEST_TYPE, + USER_STATUS_NOT_FOUND, +} from "../../../constants/requests";🧰 Tools
🪛 ESLint
[error] 5-5: Replace
·OOO_STATUS_ALREADY_EXIST,·REQUEST_ALREADY_PENDING,·REQUEST_TYPE,·USER_STATUS_NOT_FOUND·with⏎··OOO_STATUS_ALREADY_EXIST,⏎··REQUEST_ALREADY_PENDING,⏎··REQUEST_TYPE,⏎··USER_STATUS_NOT_FOUND,⏎(prettier/prettier)
61-61: Add comma after state property.Missing comma after the state property in the object literal.
- state: "OOO" + state: "OOO",🧰 Tools
🪛 ESLint
[error] 61-61: Replace
··········state:·"OOO"withstate:·"OOO",(prettier/prettier)
70-71: Fix assertion style.Use more explicit assertion styles to avoid test passes when they should fail.
- expect(response).to.be.not.undefined; + expect(response).to.exist; expect(response.error).to.equal(OOO_STATUS_ALREADY_EXIST);🧰 Tools
🪛 ESLint
[error] 70-70: Delete
······(prettier/prettier)
[error] 70-70: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 71-71: Replace
············with······(prettier/prettier)
82-82: Add comma after state property.Missing comma after the state property in the object literal.
- state: "ACTIVE" + state: "ACTIVE",🧰 Tools
🪛 ESLint
[error] 82-82: Replace
····················state:·"ACTIVE"with··········state:·"ACTIVE",(prettier/prettier)
91-92: Fix assertion style.Use more explicit assertion styles for clearer test failures.
- expect(response).to.be.not.undefined; + expect(response).to.exist; expect(response.error).to.equal(REQUEST_ALREADY_PENDING);🧰 Tools
🪛 ESLint
[error] 91-91: Delete
······(prettier/prettier)
[error] 91-91: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 92-92: Replace
············with······(prettier/prettier)
103-103: Add comma after state property.Missing comma after the state property in the object literal.
- state: "ACTIVE" + state: "ACTIVE",🧰 Tools
🪛 ESLint
[error] 103-103: Replace
····················state:·"ACTIVE"with··········state:·"ACTIVE",(prettier/prettier)
112-112: Fix assertion style.Use more explicit assertion styles for boolean checks.
- expect(response).to.be.undefined; + expect(response).to.not.exist;🧰 Tools
🪛 ESLint
[error] 112-112: Delete
······(prettier/prettier)
[error] 112-112: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
125-125: Add comma after state property.Missing comma after the state property in the object literal.
- state: "ACTIVE" + state: "ACTIVE",🧰 Tools
🪛 ESLint
[error] 125-125: Replace
····················state:·"ACTIVE"with··········state:·"ACTIVE",(prettier/prettier)
146-147: Correct typo in username parameter.There's a typo in the username parameter, which could cause issues in real tests.
- "test-usernmae-1", + "test-username-1",
149-150: Fix assertion style.Use more explicit assertion styles for clearer test failures.
- expect(response).to.be.not.undefined; + expect(response).to.exist; expect(response.type).to.equal(validOooStatusRequests.type);🧰 Tools
🪛 ESLint
[error] 149-149: Delete
······(prettier/prettier)
[error] 149-149: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 150-150: Delete
······(prettier/prettier)
167-168: Correct typo in username parameter.There's a typo in the username parameter, which could cause issues in real tests.
- "test-usernmae-1", + "test-username-1",services/oooRequest.ts (2)
11-11: Use proper typing instead of any.Replace the
anytype with a more specific type to improve type safety.- userStatus: any, + userStatus: { userId: string; currentStatus: { state: string; [key: string]: any } },
55-65: Enhance type safety in createRequest call.Consider defining a specific type for the request object to improve type safety.
Create a new interface for the request parameter:
interface OooRequest { from: number; until: number; type: string; requestedBy: string; userId: string; reason: string; comment: null | string; status: string; lastModifiedBy: null | string; }Then use this type in your function call:
- const requestResult = await createRequest({ + const requestParams: OooRequest = { from: body.from, until: body.until, type: body.type, requestedBy: username, userId: userId, reason: body.reason, comment: null, status: REQUEST_STATE.PENDING, lastModifiedBy: null, - }); + }; + const requestResult = await createRequest(requestParams);test/integration/requests.test.ts (6)
89-103: Improve test clarity and error handling.The error handling in this test could be improved for better clarity.
- it("should return Feature not implemented when dev is not true", function (done) { + it("should return 501 and 'Feature not implemented' message when dev is false", function (done) { chai .request(app) .post(`${requestsEndpoint}?dev=false`) .set("cookie", `${cookieName}=${authToken}`) .send(validOooStatusRequests) .end(function (err, res) { if (err) { return done(err); } expect(res.statusCode).to.equal(501); expect(res.body.message).to.equal("Feature not implemented"); done(); }); });
118-133: Fix typo in variable name.There's a typo in the variable name for the auth token.
- const authTokeForNotDiscordUser = authService.generateAuthToken( + const authTokenForNotDiscordUser = authService.generateAuthToken( { userId: testUserIdForNotDiscordUser } ); chai .request(app) .post(`${requestsEndpoint}?dev=true`) - .set("cookie", `${cookieName}=${authTokeForNotDiscordUser}`) + .set("cookie", `${cookieName}=${authTokenForNotDiscordUser}`) .send(validOooStatusRequests) .end(function (err, res) { expect(res).to.have.status(401); expect(res.body.error).to.equal("Unauthorized"); expect(res.body.message).to.equal("Only discord user can create an OOO request"); done(); });
135-148: Improve error handling and use consistent arrow function style.The error handling in this test could be improved, and the use of arrow functions should be consistent with the rest of the test file.
- it("should return 500 response when fails to create OOO request", function (done) { + it("should return 500 response when creating OOO request fails", function (done) { sinon.stub(requestsQuery, "createRequest") .throws("Error while creating OOO request"); chai.request(app) .post(`${requestsEndpoint}?dev=true`) .set("cookie", `${cookieName}=${authToken}`) .send(validOooStatusRequests) - .end((err, res)=>{ + .end(function(err, res) { if (err) return done(err); expect(res.statusCode).to.equal(500); expect(res.body.message).to.equal("An internal server error occurred"); done(); }); });
150-163: Add additional assertions for the response body.The test could include more assertions to verify the response structure and content.
it("should create a new request when dev is true", function (done) { chai .request(app) .post(`${requestsEndpoint}?dev=true`) .set("cookie", `${cookieName}=${authToken}`) .send(validOooStatusRequests) .end(function (err, res) { + if (err) return done(err); expect(res).to.have.status(201); expect(res.body).to.have.property("message"); expect(Object.keys(res.body)).to.have.lengthOf(1); expect(res.body.message).to.equal(REQUEST_CREATED_SUCCESSFULLY); + expect(res.body).to.not.have.property("data"); // Ensure no data is returned done(); }); });
180-192: Update test description to match code changes.Update the test description to clearly indicate what is being tested.
- it("should return error if reason is not present in body", function (done) { + it("should return 400 with 'reason is required' message when reason is missing", function (done) { chai .request(app) .post(`${requestsEndpoint}?dev=true`) .set("cookie", `${cookieName}=${authToken}`) .send(_.omit(validOooStatusRequests, "reason")) .end(function (err, res) { + if (err) return done(err); expect(res).to.have.status(400); expect(res.body).to.have.property("message"); expect(res.body.message).to.equal("reason is required"); done(); }); });
194-206: Update test description to match code changes.Update the test description to clearly indicate what is being tested.
- it("should return error if status is passed in request body", function (done) { + it("should return 400 with error when status field is included in request body", function (done) { chai .request(app) .post(`${requestsEndpoint}?dev=true`) .set("cookie", `${cookieName}=${authToken}`) .send({ ...validOooStatusRequests, status: REQUEST_STATE.APPROVED }) .end(function (err, res) { + if (err) return done(err); expect(res).to.have.status(400); expect(res.body).to.have.property("message"); expect(res.body.message).to.equal(`"status" is not allowed`); done(); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
constants/logs.ts(1 hunks)constants/requests.ts(2 hunks)controllers/oooRequests.ts(1 hunks)services/oooRequest.ts(1 hunks)test/integration/requests.test.ts(6 hunks)test/unit/services/oooRequest.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/oooRequests.ts
- constants/requests.ts
🧰 Additional context used
🪛 ESLint
test/unit/services/oooRequest.test.ts
[error] 1-2: Delete ⏎
(prettier/prettier)
[error] 5-5: Replace ·OOO_STATUS_ALREADY_EXIST,·REQUEST_ALREADY_PENDING,·REQUEST_TYPE,·USER_STATUS_NOT_FOUND· with ⏎··OOO_STATUS_ALREADY_EXIST,⏎··REQUEST_ALREADY_PENDING,⏎··REQUEST_TYPE,⏎··USER_STATUS_NOT_FOUND,⏎
(prettier/prettier)
[error] 14-175: Do not pass arrow functions to describe()
(mocha/no-mocha-arrows)
[error] 15-15: Delete ··
(prettier/prettier)
[error] 16-16: Replace ···· with ··
(prettier/prettier)
[error] 17-17: Replace ···· with ··
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 19-19: Replace ···· with ··
(prettier/prettier)
[error] 21-21: Replace ··afterEach(async·() with afterEach(async·()·
(prettier/prettier)
[error] 21-24: Do not pass arrow functions to afterEach()
(mocha/no-mocha-arrows)
[error] 22-22: Replace ········ with ····
(prettier/prettier)
[error] 23-23: Replace ········ with ····
(prettier/prettier)
[error] 24-24: Delete ··
(prettier/prettier)
[error] 26-26: Delete ··
(prettier/prettier)
[error] 26-140: Do not pass arrow functions to describe()
(mocha/no-mocha-arrows)
[error] 27-28: Replace ⏎········beforeEach(async·() with ····beforeEach(async·()·
(prettier/prettier)
[error] 28-35: Do not pass arrow functions to beforeEach()
(mocha/no-mocha-arrows)
[error] 29-29: Delete ······
(prettier/prettier)
[error] 30-30: Replace ················ with ········
(prettier/prettier)
[error] 31-31: Replace ················ with ········
(prettier/prettier)
[error] 32-32: Replace ················ with ········
(prettier/prettier)
[error] 33-33: Delete ········
(prettier/prettier)
[error] 34-34: Replace ············ with ······
(prettier/prettier)
[error] 35-35: Replace ········ with ····
(prettier/prettier)
[error] 37-37: Replace ········afterEach(async·() with ····afterEach(async·()·
(prettier/prettier)
[error] 37-40: Do not pass arrow functions to afterEach()
(mocha/no-mocha-arrows)
[error] 38-38: Replace ············ with ······
(prettier/prettier)
[error] 39-39: Replace ············ with ······
(prettier/prettier)
[error] 40-40: Delete ····
(prettier/prettier)
[error] 42-42: Replace ········ with ····
(prettier/prettier)
[error] 42-51: Do not pass arrow functions to it()
(mocha/no-mocha-arrows)
[error] 43-48: Replace ······const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················false,⏎················null,⏎················validOOORequest⏎············ with const·response·=·await·validateOOOCreateRequest(testUserId,·false,·null,·validOOORequest
(prettier/prettier)
[error] 49-49: Replace ············ with ······
(prettier/prettier)
[error] 49-49: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 50-50: Delete ······
(prettier/prettier)
[error] 51-51: Delete ····
(prettier/prettier)
[error] 52-52: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 52-52: Delete ····
(prettier/prettier)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 53-72: Do not pass arrow functions to it()
(mocha/no-mocha-arrows)
[error] 54-54: Delete ······
(prettier/prettier)
[error] 55-55: Replace ················ with ········
(prettier/prettier)
[error] 56-56: Replace ················ with ········
(prettier/prettier)
[error] 57-57: Delete ··········
(prettier/prettier)
[error] 58-58: Replace ···················· with ··········
(prettier/prettier)
[error] 59-59: Delete ··········
(prettier/prettier)
[error] 60-60: Replace ···················· with ··········
(prettier/prettier)
[error] 61-61: Replace ··········state:·"OOO" with state:·"OOO",
(prettier/prettier)
[error] 62-62: Replace ················} with ········},
(prettier/prettier)
[error] 63-63: Delete ······
(prettier/prettier)
[error] 64-69: Replace ············const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················true,⏎················testUserStatus,⏎················validOOORequest⏎············ with ······const·response·=·await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 70-70: Delete ······
(prettier/prettier)
[error] 70-70: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 71-71: Replace ············ with ······
(prettier/prettier)
[error] 72-72: Delete ····
(prettier/prettier)
[error] 73-73: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 73-73: Delete ····
(prettier/prettier)
[error] 74-74: Replace ········ with ····
(prettier/prettier)
[error] 74-93: Do not pass arrow functions to it()
(mocha/no-mocha-arrows)
[error] 75-75: Delete ······
(prettier/prettier)
[error] 76-76: Replace ················ with ········
(prettier/prettier)
[error] 77-77: Replace ················ with ········
(prettier/prettier)
[error] 78-78: Replace ···················· with ··········
(prettier/prettier)
[error] 79-79: Replace ···················· with ··········
(prettier/prettier)
[error] 80-80: Replace ···················· with ··········
(prettier/prettier)
[error] 81-81: Replace ···················· with ··········
(prettier/prettier)
[error] 82-82: Replace ····················state:·"ACTIVE" with ··········state:·"ACTIVE",
(prettier/prettier)
[error] 83-83: Replace ················} with ········},
(prettier/prettier)
[error] 84-84: Replace ············ with ······
(prettier/prettier)
[error] 85-90: Replace ············const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················true,⏎················testUserStatus,⏎················validOOORequest⏎············ with ······const·response·=·await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 91-91: Delete ······
(prettier/prettier)
[error] 91-91: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 92-92: Replace ············ with ······
(prettier/prettier)
[error] 93-93: Delete ····
(prettier/prettier)
[error] 95-95: Replace ········ with ····
(prettier/prettier)
[error] 95-113: Do not pass arrow functions to it()
(mocha/no-mocha-arrows)
[error] 96-96: Delete ······
(prettier/prettier)
[error] 97-97: Replace ················ with ········
(prettier/prettier)
[error] 98-98: Replace ················ with ········
(prettier/prettier)
[error] 99-99: Replace ···················· with ··········
(prettier/prettier)
[error] 100-100: Replace ···················· with ··········
(prettier/prettier)
[error] 101-101: Replace ···················· with ··········
(prettier/prettier)
[error] 102-102: Replace ···················· with ··········
(prettier/prettier)
[error] 103-103: Replace ····················state:·"ACTIVE" with ··········state:·"ACTIVE",
(prettier/prettier)
[error] 104-104: Replace ················} with ········},
(prettier/prettier)
[error] 105-105: Replace ············ with ······
(prettier/prettier)
[error] 106-111: Replace ············const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················true,⏎················testUserStatus,⏎················null⏎············ with ······const·response·=·await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·null
(prettier/prettier)
[error] 112-112: Delete ······
(prettier/prettier)
[error] 112-112: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 113-113: Replace ········ with ····
(prettier/prettier)
[error] 115-115: Delete ····
(prettier/prettier)
[error] 115-139: Do not pass arrow functions to it()
(mocha/no-mocha-arrows)
[error] 116-116: Replace ············ with ······
(prettier/prettier)
[error] 118-118: Delete ······
(prettier/prettier)
[error] 119-119: Replace ················ with ········
(prettier/prettier)
[error] 120-120: Replace ················ with ········
(prettier/prettier)
[error] 121-121: Replace ···················· with ··········
(prettier/prettier)
[error] 122-122: Replace ···················· with ··········
(prettier/prettier)
[error] 123-123: Replace ···················· with ··········
(prettier/prettier)
[error] 124-124: Replace ···················· with ··········
(prettier/prettier)
[error] 125-125: Replace ····················state:·"ACTIVE" with ··········state:·"ACTIVE",
(prettier/prettier)
[error] 126-126: Replace ················} with ········},
(prettier/prettier)
[error] 127-127: Replace ············ with ······
(prettier/prettier)
[error] 129-129: Replace ············ with ······
(prettier/prettier)
[error] 130-135: Replace ················await·validateOOOCreateRequest(⏎····················testUserId,⏎····················true,⏎····················testUserStatus,⏎····················validOOORequest⏎················ with ········await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 136-136: Replace ············}·catch with ······}·catch·
(prettier/prettier)
[error] 137-137: Replace ················ with ········
(prettier/prettier)
[error] 138-138: Delete ······
(prettier/prettier)
[error] 139-139: Replace ········ with ····
(prettier/prettier)
[error] 140-140: Replace ···· with ··
(prettier/prettier)
[error] 142-142: Delete ··
(prettier/prettier)
[error] 142-174: Do not pass arrow functions to describe()
(mocha/no-mocha-arrows)
[error] 143-143: Replace ········ with ····
(prettier/prettier)
[error] 143-159: Do not pass arrow functions to it()
(mocha/no-mocha-arrows)
[error] 144-148: Replace ······const·response·=·await·createOOORequest(⏎················validOooStatusRequests,⏎················"test-usernmae-1",⏎················testUserId⏎············ with const·response·=·await·createOOORequest(validOooStatusRequests,·"test-usernmae-1",·testUserId
(prettier/prettier)
[error] 149-149: Delete ······
(prettier/prettier)
[error] 149-149: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 150-150: Delete ······
(prettier/prettier)
[error] 151-151: Delete ······
(prettier/prettier)
[error] 152-152: Replace ············ with ······
(prettier/prettier)
[error] 153-153: Replace ············ with ······
(prettier/prettier)
[error] 154-154: Replace ············ with ······
(prettier/prettier)
[error] 155-155: Replace ············ with ······
(prettier/prettier)
[error] 156-156: Replace ············ with ······
(prettier/prettier)
[error] 157-157: Replace ············ with ······
(prettier/prettier)
[error] 158-158: Delete ······
(prettier/prettier)
[error] 159-159: Replace ········ with ····
(prettier/prettier)
[error] 161-161: Delete ····
(prettier/prettier)
[error] 161-173: Do not pass arrow functions to it()
(mocha/no-mocha-arrows)
[error] 162-162: Replace ············ with ······
(prettier/prettier)
[error] 164-164: Delete ······
(prettier/prettier)
[error] 165-169: Replace ················await·createOOORequest(⏎····················validOooStatusRequests,⏎····················"test-usernmae-1",⏎····················testUserId⏎················ with ········await·createOOORequest(validOooStatusRequests,·"test-usernmae-1",·testUserId
(prettier/prettier)
[error] 170-170: Replace ············}·catch with ······}·catch·
(prettier/prettier)
[error] 171-171: Replace ················ with ········
(prettier/prettier)
[error] 172-172: Delete ······
(prettier/prettier)
[error] 173-173: Replace ········ with ····
(prettier/prettier)
[error] 174-174: Delete ··
(prettier/prettier)
[error] 175-175: Insert ⏎
(prettier/prettier)
🔇 Additional comments (1)
constants/logs.ts (1)
25-26: New log type constants added for OOO request functionality.These constants provide specific error messages for OOO request scenarios, following the existing pattern in the codebase. The additions are well-aligned with the PR objective of implementing OOO request functionality.
surajmaity1
force-pushed
the
feat/user-ooo-create
branch
from
764d85c to
cb14cde
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
controllers/oooRequests.ts (1)
102-119:⚠️ Potential issueFix inconsistent property names.
In
updateOooRequestController, the propertiesmessageand possibly others don't match the updated property names being used in the rest of the codebase (likereasoninstead ofmessage).if (requestData) { const { from, until, requestedBy, message } = requestData as any; const userFutureStatusData = { requestId, status: REQUEST_TYPE.OOO, state: statusState.UPCOMING, from, endsOn: until, userId: requestedBy, - message, + reason: message, // Or directly use 'reason' property from requestData }; await createUserFutureStatus(userFutureStatusData); await addFutureStatus(userFutureStatusData); }
♻️ Duplicate comments (1)
controllers/oooRequests.ts (1)
38-61: 🛠️ Refactor suggestionService layer should handle all validation logic.
The controller handles too much validation logic. Move all the validation and error response mapping to the service layer for better separation of concerns.
Currently, the validation logic is split between the service layer and controller, which makes it harder to test and maintain. Consider restructuring so that the service layer returns complete error objects that can be directly used by the controller.
try { - const { data: userStatus, userStatusExists } = await getUserStatus(userId); - - const latestOOORequest: OooStatusRequest = await getRequestByKeyValues({ - userId: userId, - type: REQUEST_TYPE.OOO, - status: REQUEST_STATE.PENDING, - }); - - const validationResponse = await validateOOOCreateRequest(userId, userStatusExists, userStatus, latestOOORequest); - - if (validationResponse) { - if (validationResponse.error === USER_STATUS_NOT_FOUND) { - return res.boom.notFound(validationResponse.error); - } - if (validationResponse.error === OOO_STATUS_ALREADY_EXIST) { - return res.boom.forbidden(validationResponse.error); - } - if (validationResponse.error === REQUEST_ALREADY_PENDING) { - return res.boom.conflict(validationResponse.error); - } - } + // Validate and create the OOO request + const validationResult = await validateAndCreateOOORequest(userId, username, requestBody); + + // Handle validation errors + if (validationResult.error) { + switch (validationResult.error) { + case USER_STATUS_NOT_FOUND: + return res.boom.notFound(validationResult.error); + case OOO_STATUS_ALREADY_EXIST: + return res.boom.forbidden(validationResult.error); + case REQUEST_ALREADY_PENDING: + return res.boom.conflict(validationResult.error); + default: + return res.boom.badRequest(validationResult.error); + } + }
🧹 Nitpick comments (15)
test/unit/services/oooRequest.test.ts (8)
54-56: Use assertion method that explicitly verifies a defined value.Using
to.existcan pass for both defined non-null values and truthy values. For error validation, it's better to be more explicit.- expect(response).to.exist; + expect(response).to.not.be.undefined; expect(response.error).to.equal(USER_STATUS_NOT_FOUND);🧰 Tools
🪛 ESLint
[error] 54-54: Replace
············with······(prettier/prettier)
[error] 54-54: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 55-55: Delete
······(prettier/prettier)
[error] 56-56: Delete
····(prettier/prettier)
75-77: Use assertion method that explicitly verifies a defined value.Same issue with using
to.existas above.- expect(response).to.exist; + expect(response).to.not.be.undefined; expect(response.error).to.equal(OOO_STATUS_ALREADY_EXIST);🧰 Tools
🪛 ESLint
[error] 75-75: Delete
······(prettier/prettier)
[error] 75-75: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 76-76: Replace
············with······(prettier/prettier)
[error] 77-77: Delete
····(prettier/prettier)
96-98: Use assertion method that explicitly verifies a defined value.Same issue with using
to.existas above.- expect(response).to.exist; + expect(response).to.not.be.undefined; expect(response.error).to.equal(REQUEST_ALREADY_PENDING);🧰 Tools
🪛 ESLint
[error] 96-96: Delete
······(prettier/prettier)
[error] 96-96: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 97-97: Delete
······(prettier/prettier)
[error] 98-98: Delete
····(prettier/prettier)
117-118: Improve clarity of assertion method.The current assertion is using a double negative which can be confusing. It's clearer to check for
undefineddirectly.- expect(response).to.not.exist; + expect(response).to.be.undefined;🧰 Tools
🪛 ESLint
[error] 117-117: Delete
······(prettier/prettier)
[error] 117-117: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 118-118: Delete
····(prettier/prettier)
154-164: Use better response validation methods.The
existassertion is less clear than explicitly validating the object's properties. Consider a more comprehensive way to validate the response object.- expect(response).to.exist; - expect(response.type).to.equal(validOooStatusRequests.type); - expect(response.from).to.equal(validOooStatusRequests.from); - expect(response.until).to.equal(validOooStatusRequests.until); - expect(response.reason).to.equal(validOooStatusRequests.reason); - expect(response.status).to.equal("PENDING"); - expect(response.lastModifiedBy).to.equal(null); - expect(response.requestedBy).to.equal("test-username-1"); - expect(response.userId).to.equal(testUserId); - expect(response.comment).to.equal(null); + expect(response).to.deep.include({ + type: validOooStatusRequests.type, + from: validOooStatusRequests.from, + until: validOooStatusRequests.until, + reason: validOooStatusRequests.reason, + status: "PENDING", + lastModifiedBy: null, + requestedBy: "test-username-1", + userId: testUserId, + comment: null + });🧰 Tools
🪛 ESLint
[error] 154-154: Delete
······(prettier/prettier)
[error] 154-154: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 155-155: Delete
······(prettier/prettier)
[error] 156-156: Replace
············with······(prettier/prettier)
[error] 157-157: Replace
············with······(prettier/prettier)
[error] 158-158: Replace
············with······(prettier/prettier)
[error] 159-159: Replace
············with······(prettier/prettier)
[error] 160-160: Replace
············with······(prettier/prettier)
[error] 161-161: Replace
············with······(prettier/prettier)
[error] 162-162: Delete
······(prettier/prettier)
[error] 163-163: Delete
······(prettier/prettier)
[error] 164-164: Delete
····(prettier/prettier)
120-144: Add assertion for expected code path execution.The error test doesn't verify that the code actually attempted to call
validateOOOCreateRequest. Add an assertion to verify the function was called with the expected arguments.sinon.stub(logService, "addLog").throws(new Error(errorMessage)); + // Create a spy to verify that the try block executes + const validateSpy = sinon.spy(require("../../../services/oooRequest"), "validateOOOCreateRequest"); + testUserStatus = { userId: testUserId, currentStatus: { updatedAt: Date.now(), from: Date.now(), until: "", message: "", state: "ACTIVE", } }; try { await validateOOOCreateRequest( testUserId, true, testUserStatus, validOOORequest ); } catch(error) { expect(error.message).to.equal(errorMessage); + expect(validateSpy.calledOnce).to.be.true; }🧰 Tools
🪛 ESLint
[error] 120-120: Replace
····it("should·throw·error",·async·functionwithit("should·throw·error",·async·function·(prettier/prettier)
[error] 121-121: Replace
············with······(prettier/prettier)
[error] 123-123: Delete
······(prettier/prettier)
[error] 124-124: Replace
················with········(prettier/prettier)
[error] 125-125: Replace
················with········(prettier/prettier)
[error] 126-126: Replace
····················with··········(prettier/prettier)
[error] 127-127: Replace
····················with··········(prettier/prettier)
[error] 128-128: Replace
····················with··········(prettier/prettier)
[error] 129-129: Replace
····················with··········(prettier/prettier)
[error] 130-130: Replace
····················with··········(prettier/prettier)
[error] 131-131: Replace
········}with},(prettier/prettier)
[error] 132-132: Delete
······(prettier/prettier)
[error] 134-134: Replace
············with······(prettier/prettier)
[error] 135-140: Replace
················await·validateOOOCreateRequest(⏎····················testUserId,⏎····················true,⏎····················testUserStatus,⏎····················validOOORequest⏎················with········await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest(prettier/prettier)
[error] 141-141: Replace
············}·catchwith······}·catch·(prettier/prettier)
[error] 142-142: Replace
················with········(prettier/prettier)
[error] 143-143: Delete
······(prettier/prettier)
[error] 144-144: Delete
····(prettier/prettier)
166-178: Add assertion for expected code path execution.Similar to the previous comment, add an assertion to verify
createOOORequestwas called with the expected arguments.sinon.stub(logService, "addLog").throws(new Error(errorMessage)); + // Create a spy to verify that the try block executes + const createSpy = sinon.spy(require("../../../services/oooRequest"), "createOOORequest"); + try { await createOOORequest( validOooStatusRequests, "test-usernmae-1", testUserId ); } catch(error) { expect(error.message).to.equal(errorMessage); + expect(createSpy.calledOnce).to.be.true; }🧰 Tools
🪛 ESLint
[error] 166-166: Replace
····it("should·throw·error",·async·functionwithit("should·throw·error",·async·function·(prettier/prettier)
[error] 167-167: Replace
············with······(prettier/prettier)
[error] 169-169: Delete
······(prettier/prettier)
[error] 170-174: Replace
················await·createOOORequest(⏎····················validOooStatusRequests,⏎····················"test-usernmae-1",⏎····················testUserId⏎················with········await·createOOORequest(validOooStatusRequests,·"test-usernmae-1",·testUserId(prettier/prettier)
[error] 175-175: Replace
············}·catchwith······}·catch·(prettier/prettier)
[error] 176-176: Replace
················with········(prettier/prettier)
[error] 177-177: Delete
······(prettier/prettier)
[error] 178-178: Replace
········with····(prettier/prettier)
172-172: Fix typo in username parameter.There's a typo in the username parameter used in the test - "test-usernmae-1" instead of "test-username-1".
- "test-usernmae-1", + "test-username-1",test/integration/requests.test.ts (4)
89-103: Add more comprehensive tests for feature flag disabled case.The current test only checks if the feature returns a "not implemented" message. Consider adding tests for different scenarios when the feature flag is disabled to ensure consistent behavior.
For example, test with different request bodies or when the user is not in Discord, to verify the behavior is consistent when the feature flag is off.
118-133: Enhance Discord user verification test.This test correctly checks that non-Discord users can't create OOO requests, but it would be good to add more verification of the response structure and status code.
.end(function (err, res) { expect(res).to.have.status(401); expect(res.body.error).to.equal("Unauthorized"); expect(res.body.message).to.equal("Only discord user can create an OOO request"); + expect(res.body).to.not.have.property("data"); + expect(Object.keys(res.body)).to.have.lengthOf(2); done(); });
135-148: Improve server error test by checking exact request path.The server error test correctly stubs the createRequest function, but it would be more precise to verify the exact parameters that would be passed to it, ensuring the test is accurately simulating the expected failure point.
sinon.stub(requestsQuery, "createRequest") - .throws("Error while creating OOO request"); + .withArgs(sinon.match({ + type: validOooStatusRequests.type, + from: validOooStatusRequests.from, + until: validOooStatusRequests.until, + reason: validOooStatusRequests.reason + })) + .throws(new Error("Error while creating OOO request"));
182-195: Fix inconsistent error handling approach.In this test, there's an inconsistent approach to error handling compared to other tests. Some tests use
if (err) return done(err)while others don't check for errors at all.chai .request(app) .post(`${requestsEndpoint}?dev=true`) .set("cookie", `${cookieName}=${authToken}`) .send(_.omit(validOooStatusRequests, "reason")) .end(function (err, res) { - if (err) return done(err); + if (err) { + return done(err); + } expect(res).to.have.status(400); expect(res.body).to.have.property("message"); expect(res.body.message).to.equal("reason is required"); done(); });controllers/oooRequests.ts (3)
28-30: Validate user data presence before destructuring.The code destructures
req.userDatawithout first checking if it exists. While middleware likely ensures it's present, it's safer to add a validation check.- const { id: userId, username} = req.userData; - const isUserPartOfDiscord = req.userData.roles.in_discord; + if (!req.userData) { + return res.boom.unauthorized("Unauthenticated User"); + } + + const { id: userId, username } = req.userData; + const isUserPartOfDiscord = req.userData.roles?.in_discord;
32-36: Consistent use of spaces in code blocks.There's inconsistent spacing in code blocks. The first
ifblock has no space after the condition, while the second one does.- if(!dev) return res.boom.notImplemented("Feature not implemented"); + if (!dev) { + return res.boom.notImplemented("Feature not implemented"); + }
62-65: Return response data for better client experience.The response doesn't include the created request data, which would be helpful for clients. Consider returning the created request data.
await createOOORequest(requestBody, username, userId); return res.status(201).json({ message: REQUEST_CREATED_SUCCESSFULLY, + data: { + type: requestBody.type, + from: requestBody.from, + until: requestBody.until, + reason: requestBody.reason, + status: REQUEST_STATE.PENDING, + requestedBy: username, + userId: userId + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
constants/logs.ts(1 hunks)constants/requests.ts(2 hunks)controllers/oooRequests.ts(1 hunks)services/oooRequest.ts(1 hunks)test/integration/requests.test.ts(6 hunks)test/unit/services/oooRequest.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- constants/logs.ts
- constants/requests.ts
- services/oooRequest.ts
🧰 Additional context used
🪛 ESLint
test/unit/services/oooRequest.test.ts
[error] 1-2: Delete ⏎
(prettier/prettier)
[error] 5-5: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 5-5: Delete ·
(prettier/prettier)
[error] 6-6: Delete ··
(prettier/prettier)
[error] 7-7: Delete ··
(prettier/prettier)
[error] 8-8: Delete ··
(prettier/prettier)
[error] 9-9: Delete ··
(prettier/prettier)
[error] 19-19: Insert ·
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Delete ··
(prettier/prettier)
[error] 22-22: Replace ···· with ··
(prettier/prettier)
[error] 23-23: Delete ··
(prettier/prettier)
[error] 24-24: Delete ··
(prettier/prettier)
[error] 26-26: Replace ··afterEach(async·function with afterEach(async·function·
(prettier/prettier)
[error] 27-27: Replace ········ with ····
(prettier/prettier)
[error] 28-28: Replace ········ with ····
(prettier/prettier)
[error] 29-29: Delete ··
(prettier/prettier)
[error] 31-31: Replace ····describe("validateOOOCreateRequest",·function with ··describe("validateOOOCreateRequest",·function·
(prettier/prettier)
[error] 32-33: Replace ⏎········beforeEach(async·function with ····beforeEach(async·function·
(prettier/prettier)
[error] 34-34: Delete ······
(prettier/prettier)
[error] 35-35: Replace ················ with ········
(prettier/prettier)
[error] 36-36: Replace ················ with ········
(prettier/prettier)
[error] 37-37: Replace ················ with ········
(prettier/prettier)
[error] 38-38: Delete ········
(prettier/prettier)
[error] 39-39: Replace ············ with ······
(prettier/prettier)
[error] 40-40: Replace ········ with ····
(prettier/prettier)
[error] 42-42: Replace ········afterEach(async·function with ····afterEach(async·function·
(prettier/prettier)
[error] 43-43: Replace ············ with ······
(prettier/prettier)
[error] 44-44: Replace ············ with ······
(prettier/prettier)
[error] 45-45: Delete ····
(prettier/prettier)
[error] 47-47: Replace ········it("should·return·USER_STATUS_NOT_FOUND·if·user·status·not·found",·async·function with ····it("should·return·USER_STATUS_NOT_FOUND·if·user·status·not·found",·async·function·
(prettier/prettier)
[error] 48-53: Replace ············const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················false,⏎················null,⏎················validOOORequest⏎············ with ······const·response·=·await·validateOOOCreateRequest(testUserId,·false,·null,·validOOORequest
(prettier/prettier)
[error] 54-54: Replace ············ with ······
(prettier/prettier)
[error] 54-54: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 55-55: Delete ······
(prettier/prettier)
[error] 56-56: Delete ····
(prettier/prettier)
[error] 57-57: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 57-57: Delete ····
(prettier/prettier)
[error] 58-58: Replace ········it("should·return·OOO_STATUS_ALREADY_EXIST·if·user·status·is·already·OOO",·async·function with ····it("should·return·OOO_STATUS_ALREADY_EXIST·if·user·status·is·already·OOO",·async·function·
(prettier/prettier)
[error] 59-59: Delete ······
(prettier/prettier)
[error] 60-60: Replace ················ with ········
(prettier/prettier)
[error] 61-61: Replace ················ with ········
(prettier/prettier)
[error] 62-62: Delete ··········
(prettier/prettier)
[error] 63-63: Replace ···················· with ··········
(prettier/prettier)
[error] 64-64: Delete ··········
(prettier/prettier)
[error] 65-65: Replace ···················· with ··········
(prettier/prettier)
[error] 66-66: Delete ··········
(prettier/prettier)
[error] 67-67: Replace ················} with ········},
(prettier/prettier)
[error] 68-68: Delete ······
(prettier/prettier)
[error] 69-74: Replace ············const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················true,⏎················testUserStatus,⏎················validOOORequest⏎············ with ······const·response·=·await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 75-75: Delete ······
(prettier/prettier)
[error] 75-75: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 76-76: Replace ············ with ······
(prettier/prettier)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 78-78: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 78-78: Delete ····
(prettier/prettier)
[error] 79-79: Replace ········it("should·return·REQUEST_ALREADY_PENDING·if·user·has·already·pending·request",·async·function with ····it("should·return·REQUEST_ALREADY_PENDING·if·user·has·already·pending·request",·async·function·
(prettier/prettier)
[error] 80-80: Delete ······
(prettier/prettier)
[error] 81-81: Replace ················ with ········
(prettier/prettier)
[error] 82-82: Replace ················ with ········
(prettier/prettier)
[error] 83-83: Replace ···················· with ··········
(prettier/prettier)
[error] 84-84: Replace ···················· with ··········
(prettier/prettier)
[error] 85-85: Replace ···················· with ··········
(prettier/prettier)
[error] 86-86: Replace ···················· with ··········
(prettier/prettier)
[error] 87-87: Replace ···················· with ··········
(prettier/prettier)
[error] 88-88: Replace ················} with ········},
(prettier/prettier)
[error] 89-89: Replace ············ with ······
(prettier/prettier)
[error] 90-95: Replace ············const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················true,⏎················testUserStatus,⏎················validOOORequest⏎············ with ······const·response·=·await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 96-96: Delete ······
(prettier/prettier)
[error] 96-96: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 97-97: Delete ······
(prettier/prettier)
[error] 98-98: Delete ····
(prettier/prettier)
[error] 100-100: Replace ········it("should·return·undefined·when·all·validation·checks·passes",·async·function with ····it("should·return·undefined·when·all·validation·checks·passes",·async·function·
(prettier/prettier)
[error] 101-101: Delete ······
(prettier/prettier)
[error] 102-102: Replace ················ with ········
(prettier/prettier)
[error] 103-103: Replace ················ with ········
(prettier/prettier)
[error] 104-104: Replace ···················· with ··········
(prettier/prettier)
[error] 105-105: Replace ···················· with ··········
(prettier/prettier)
[error] 106-106: Replace ···················· with ··········
(prettier/prettier)
[error] 107-107: Replace ···················· with ··········
(prettier/prettier)
[error] 108-108: Replace ···················· with ··········
(prettier/prettier)
[error] 109-109: Replace ················} with ········},
(prettier/prettier)
[error] 110-110: Replace ············ with ······
(prettier/prettier)
[error] 111-116: Replace ············const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················true,⏎················testUserStatus,⏎················null⏎············ with ······const·response·=·await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·null
(prettier/prettier)
[error] 117-117: Delete ······
(prettier/prettier)
[error] 117-117: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 118-118: Delete ····
(prettier/prettier)
[error] 120-120: Replace ····it("should·throw·error",·async·function with it("should·throw·error",·async·function·
(prettier/prettier)
[error] 121-121: Replace ············ with ······
(prettier/prettier)
[error] 123-123: Delete ······
(prettier/prettier)
[error] 124-124: Replace ················ with ········
(prettier/prettier)
[error] 125-125: Replace ················ with ········
(prettier/prettier)
[error] 126-126: Replace ···················· with ··········
(prettier/prettier)
[error] 127-127: Replace ···················· with ··········
(prettier/prettier)
[error] 128-128: Replace ···················· with ··········
(prettier/prettier)
[error] 129-129: Replace ···················· with ··········
(prettier/prettier)
[error] 130-130: Replace ···················· with ··········
(prettier/prettier)
[error] 131-131: Replace ········} with },
(prettier/prettier)
[error] 132-132: Delete ······
(prettier/prettier)
[error] 134-134: Replace ············ with ······
(prettier/prettier)
[error] 135-140: Replace ················await·validateOOOCreateRequest(⏎····················testUserId,⏎····················true,⏎····················testUserStatus,⏎····················validOOORequest⏎················ with ········await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 141-141: Replace ············}·catch with ······}·catch·
(prettier/prettier)
[error] 142-142: Replace ················ with ········
(prettier/prettier)
[error] 143-143: Delete ······
(prettier/prettier)
[error] 144-144: Delete ····
(prettier/prettier)
[error] 145-145: Delete ··
(prettier/prettier)
[error] 147-147: Replace ····describe("createOOORequest",·function with ··describe("createOOORequest",·function·
(prettier/prettier)
[error] 148-148: Replace ····it("should·create·OOO·request",·async·function with it("should·create·OOO·request",·async·function·
(prettier/prettier)
[error] 149-153: Replace ······const·response·=·await·createOOORequest(⏎················validOooStatusRequests,⏎················"test-username-1",⏎················testUserId⏎············ with const·response·=·await·createOOORequest(validOooStatusRequests,·"test-username-1",·testUserId
(prettier/prettier)
[error] 154-154: Delete ······
(prettier/prettier)
[error] 154-154: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 155-155: Delete ······
(prettier/prettier)
[error] 156-156: Replace ············ with ······
(prettier/prettier)
[error] 157-157: Replace ············ with ······
(prettier/prettier)
[error] 158-158: Replace ············ with ······
(prettier/prettier)
[error] 159-159: Replace ············ with ······
(prettier/prettier)
[error] 160-160: Replace ············ with ······
(prettier/prettier)
[error] 161-161: Replace ············ with ······
(prettier/prettier)
[error] 162-162: Delete ······
(prettier/prettier)
[error] 163-163: Delete ······
(prettier/prettier)
[error] 164-164: Delete ····
(prettier/prettier)
[error] 166-166: Replace ····it("should·throw·error",·async·function with it("should·throw·error",·async·function·
(prettier/prettier)
[error] 167-167: Replace ············ with ······
(prettier/prettier)
[error] 169-169: Delete ······
(prettier/prettier)
[error] 170-174: Replace ················await·createOOORequest(⏎····················validOooStatusRequests,⏎····················"test-usernmae-1",⏎····················testUserId⏎················ with ········await·createOOORequest(validOooStatusRequests,·"test-usernmae-1",·testUserId
(prettier/prettier)
[error] 175-175: Replace ············}·catch with ······}·catch·
(prettier/prettier)
[error] 176-176: Replace ················ with ········
(prettier/prettier)
[error] 177-177: Delete ······
(prettier/prettier)
[error] 178-178: Replace ········ with ····
(prettier/prettier)
[error] 179-179: Delete ··
(prettier/prettier)
[error] 180-180: Insert ⏎
(prettier/prettier)
🔇 Additional comments (1)
controllers/oooRequests.ts (1)
42-46: Update property names in query to match new schema.The query is using
statusinstead ofstate, which reflects a change in the schema. Make sure this is consistent across the codebase.Good update from
statetostatusto maintain consistency with the schema changes.
surajmaity1
force-pushed
the
feat/user-ooo-create
branch
from
6d814d3 to
c80bdba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
test/unit/services/oooRequest.test.ts (3)
1-16: 🛠️ Refactor suggestionFix code style issues in the imports section and throughout the file.
The file has numerous indentation and spacing issues flagged by ESLint. The imports section and the overall file structure should follow the project's style guide.
-import firestore from "../../../utils/firestore"; -import sinon from "sinon"; -import cleanDb from "../../utils/cleanDb"; -import { - OOO_STATUS_ALREADY_EXIST, - REQUEST_ALREADY_PENDING, - REQUEST_TYPE, - USER_STATUS_NOT_FOUND, -} from "../../../constants/requests"; +import firestore from "../../../utils/firestore"; +import sinon from "sinon"; +import cleanDb from "../../utils/cleanDb"; +import { + OOO_STATUS_ALREADY_EXIST, + REQUEST_ALREADY_PENDING, + REQUEST_TYPE, + USER_STATUS_NOT_FOUND, +} from "../../../constants/requests";🧰 Tools
🪛 ESLint
[error] 5-5: Delete
··(prettier/prettier)
[error] 6-6: Delete
··(prettier/prettier)
[error] 7-7: Delete
··(prettier/prettier)
[error] 8-8: Delete
··(prettier/prettier)
47-48:⚠️ Potential issueMissing logger import.
The file uses the logger object but doesn't import it, which will cause a runtime error.
import firestore from "../../../utils/firestore"; import sinon from "sinon"; import cleanDb from "../../utils/cleanDb"; import { OOO_STATUS_ALREADY_EXIST, REQUEST_ALREADY_PENDING, REQUEST_TYPE, USER_STATUS_NOT_FOUND, } from "../../../constants/requests"; import { convertDaysToMilliseconds } from "../../../utils/time"; import { createOOORequest, validateOOOCreateRequest } from "../../../services/oooRequest"; import { expect } from "chai"; import * as logService from "../../../services/logService"; import { validOooStatusRequests } from "../../fixtures/oooRequest/oooRequest"; import { updateUserStatus } from "../../../models/userStatus"; +import logger from "../../../utils/logger";
19-29: 🛠️ Refactor suggestionUse function expressions in Mocha tests and fix indentation issues.
Replace arrow functions with function expressions for better context binding in Mocha tests and fix indentation throughout the file.
-describe("Test OOO Request Service", function() { - let validOOORequest; - let testUserStatus; - const testReason = "Due to some personal reason"; - const testUserId = "11111"; - const errorMessage = "Unexpected error occured"; - - afterEach(async function() { - await cleanDb(); - sinon.restore(); - }); +describe("Test OOO Request Service", function() { + let validOOORequest; + let testUserStatus; + const testReason = "Due to some personal reason"; + const testUserId = "11111"; + const errorMessage = "Unexpected error occured"; + + afterEach(async function() { + await cleanDb(); + sinon.restore(); + });🧰 Tools
🪛 ESLint
[error] 19-19: Insert
·(prettier/prettier)
[error] 20-20: Delete
··(prettier/prettier)
[error] 21-21: Delete
··(prettier/prettier)
[error] 22-22: Replace
····with··(prettier/prettier)
[error] 23-23: Delete
··(prettier/prettier)
[error] 24-24: Delete
··(prettier/prettier)
[error] 26-26: Replace
··afterEach(async·functionwithafterEach(async·function·(prettier/prettier)
[error] 27-27: Replace
········with····(prettier/prettier)
[error] 28-28: Replace
········with····(prettier/prettier)
[error] 29-29: Delete
··(prettier/prettier)
services/oooRequest.ts (3)
17-17:⚠️ Potential issueAdd missing logger import.
The logger is used in the catch blocks but not imported, which will cause runtime errors.
import { addLog } from "./logService"; import { NotFound, Forbidden, Conflict } from "http-errors"; +import logger from "../utils/logger";
96-97:⚠️ Potential issueFix logger usage in catch block.
Similar to the previous catch block, this one also has issues with logger usage.
} catch (error) { - logger.error("Error while creating OOO request", error); + logger.error("Error while creating OOO request", { error, userId }); throw error; }
47-48:⚠️ Potential issueFix logger usage in catch block.
The error logging in the catch block is using the logger, but lacks proper imports.
} catch (error) { - logger.error("Error while validating OOO create request", error); + logger.error("Error while validating OOO create request", { error, userId }); throw error; }
🧹 Nitpick comments (13)
test/unit/services/oooRequest.test.ts (6)
42-55: Fix assertion style and improve test clarity.The test uses a try/catch block but the validation logic could be made clearer with expect-to-throw patterns. Also ensure proper assertion styles are used.
- it("should return USER_STATUS_NOT_FOUND if user status not found", async function() { - try { - await validateOOOCreateRequest( - testUserId, - false, - null, - validOOORequest - ); - } catch (error) { - expect(error).to.be.an.instanceOf(Error); - expect(error.statusCode).to.equal(404); - expect(error.message).to.equal(USER_STATUS_NOT_FOUND); - } - }); + it("should return USER_STATUS_NOT_FOUND if user status not found", async function() { + let error; + try { + await validateOOOCreateRequest( + testUserId, + false, + null, + validOOORequest + ); + expect.fail("Expected function to throw an error"); + } catch (err) { + error = err; + } + expect(error).to.be.an.instanceOf(Error); + expect(error.statusCode).to.equal(404); + expect(error.message).to.equal(USER_STATUS_NOT_FOUND); + });🧰 Tools
🪛 ESLint
[error] 42-42: Replace
········it("should·return·USER_STATUS_NOT_FOUND·if·user·status·not·found",·async·functionwith····it("should·return·USER_STATUS_NOT_FOUND·if·user·status·not·found",·async·function·(prettier/prettier)
[error] 43-43: Replace
············with······(prettier/prettier)
[error] 44-49: Replace
················await·validateOOOCreateRequest(⏎····················testUserId,⏎····················false,⏎····················null,⏎····················validOOORequest⏎················with········await·validateOOOCreateRequest(testUserId,·false,·null,·validOOORequest(prettier/prettier)
[error] 50-50: Replace
············with······(prettier/prettier)
[error] 51-51: Delete
········(prettier/prettier)
[error] 52-52: Replace
················with········(prettier/prettier)
[error] 53-53: Delete
········(prettier/prettier)
[error] 54-54: Delete
······(prettier/prettier)
[error] 55-55: Delete
····(prettier/prettier)
124-125: Fix no-unused-expressions ESLint error.The test is using
expect(response).to.not.existwhich triggers a no-unused-expressions ESLint error.- expect(response).to.not.exist; + expect(response).to.equal(undefined);🧰 Tools
🪛 ESLint
[error] 124-124: Replace
············with······(prettier/prettier)
[error] 124-124: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 125-125: Replace
········with····(prettier/prettier)
150-152: Fix no-unused-expressions ESLint error in spy assertion.The test is using
expect(validateSpy.calledOnce).to.be.truewhich triggers a no-unused-expressions ESLint error.- expect(validateSpy.calledOnce).to.be.true; + expect(validateSpy.calledOnce).to.equal(true);🧰 Tools
🪛 ESLint
[error] 150-150: Replace
················with········(prettier/prettier)
[error] 151-151: Replace
················with········(prettier/prettier)
[error] 151-151: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 152-152: Replace
············with······(prettier/prettier)
174-175: Fix no-unused-expressions ESLint error in response assertion.The test is using
expect(response).to.existwhich triggers a no-unused-expressions ESLint error.- expect(response).to.exist; + expect(response).to.not.equal(undefined);🧰 Tools
🪛 ESLint
[error] 174-174: Delete
······(prettier/prettier)
[error] 174-174: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 175-175: Delete
······(prettier/prettier)
184-185: Fix trailing comma in expectations object.The test is missing a trailing comma in the expectations object which can cause issues with some code formatters.
- comment: null + comment: null,🧰 Tools
🪛 ESLint
[error] 184-184: Replace
················comment:·nullwith········comment:·null,(prettier/prettier)
[error] 185-185: Replace
············with······(prettier/prettier)
200-201: Fix no-unused-expressions ESLint error in spy assertion.The test is using
expect(createSpy.calledOnce).to.be.truewhich triggers a no-unused-expressions ESLint error.- expect(createSpy.calledOnce).to.be.true; + expect(createSpy.calledOnce).to.equal(true);🧰 Tools
🪛 ESLint
[error] 200-200: Delete
········(prettier/prettier)
[error] 200-200: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 201-201: Delete
······(prettier/prettier)
services/oooRequest.ts (4)
1-16: Fix indentation in imports section.There are indentation inconsistencies in the import statements. Please align them with the project's style guide.
-import { logType } from "../constants/logs"; -import { - LOG_ACTION, - OOO_STATUS_ALREADY_EXIST, - REQUEST_ALREADY_PENDING, - REQUEST_LOG_TYPE, - REQUEST_STATE, - REQUEST_TYPE, - USER_STATUS_NOT_FOUND, -} from "../constants/requests"; +import { logType } from "../constants/logs"; +import { + LOG_ACTION, + OOO_STATUS_ALREADY_EXIST, + REQUEST_ALREADY_PENDING, + REQUEST_LOG_TYPE, + REQUEST_STATE, + REQUEST_TYPE, + USER_STATUS_NOT_FOUND, +} from "../constants/requests";
18-50: Fix function parameter indentation and type any.The parameters have inconsistent indentation, and the
userStatusparameter uses theanytype, which should be replaced with a more specific type.export const validateOOOCreateRequest = async ( - userId: string, - userStatusExists: boolean, - userStatus: any, - latestOOORequest: OooStatusRequest, + userId: string, + userStatusExists: boolean, + userStatus: Record<string, any>, + latestOOORequest: OooStatusRequest, ) => {
32-37: Enhance error logging for better traceability.Consider enhancing the error logging in the
validateOOOCreateRequestfunction to include more context about the OOO request that's being validated.if (userStatus.currentStatus.state === userState.OOO) { await addLog(logType.STATUS_ALREADY_EXIST, { userId, userStatus: userState.OOO }, - { message: OOO_STATUS_ALREADY_EXIST } + { + message: OOO_STATUS_ALREADY_EXIST, + currentState: userStatus.currentStatus.state + } ); throw Forbidden(OOO_STATUS_ALREADY_EXIST); }
69-79: Consider adding input validation for required fields.The function assumes all required fields exist in the body parameter. Consider adding explicit validation for required fields before creating the request.
const requestResult: OooStatusRequest = await createRequest({ + // Validate that required fields exist + ...(body.from ? { from: body.from } : { from: Date.now() }), + ...(body.until ? { until: body.until } : { until: null }), + ...(body.type ? { type: body.type } : { type: REQUEST_TYPE.OOO }), + ...(body.reason ? { reason: body.reason } : { reason: "" }), - from: body.from, - until: body.until, - type: body.type, - reason: body.reason, requestedBy: username, userId: userId, comment: null, status: REQUEST_STATE.PENDING, lastModifiedBy: null, });controllers/oooRequests.ts (3)
28-31: Consider using a more robust feature flag mechanism.The current implementation using a query parameter may not be the most secure or manageable approach for feature flags.
Consider using a server-side configuration system or environment variable for feature flags instead of relying on query parameters which can be easily manipulated by clients.
32-34: Consider moving business rule to service layer.The Discord user check is a business rule that could be better placed in the service layer.
Move this validation to the
createOOORequestservice function to maintain separation of concerns - controllers handle HTTP requests/responses while services handle business logic.
49-107: Consider updating error handling in updateOooRequestController.While you've updated
createOooRequestControllerto usenext(err)for error handling,updateOooRequestControllerstill returns errors directly, creating an inconsistency.For consistency, consider updating the
updateOooRequestControllerto use the same error handling pattern withnext(err)as implemented increateOooRequestController.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
constants/logs.ts(1 hunks)constants/requests.ts(2 hunks)controllers/oooRequests.ts(1 hunks)controllers/requests.ts(1 hunks)services/oooRequest.ts(1 hunks)test/integration/requests.test.ts(5 hunks)test/unit/middlewares/oooRequests.test.ts(3 hunks)test/unit/services/oooRequest.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- constants/logs.ts
- constants/requests.ts
- test/unit/middlewares/oooRequests.test.ts
🧰 Additional context used
🪛 ESLint
test/unit/services/oooRequest.test.ts
[error] 5-5: Delete ··
(prettier/prettier)
[error] 6-6: Delete ··
(prettier/prettier)
[error] 7-7: Delete ··
(prettier/prettier)
[error] 8-8: Delete ··
(prettier/prettier)
[error] 19-19: Insert ·
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Delete ··
(prettier/prettier)
[error] 22-22: Replace ···· with ··
(prettier/prettier)
[error] 23-23: Delete ··
(prettier/prettier)
[error] 24-24: Delete ··
(prettier/prettier)
[error] 26-26: Replace ··afterEach(async·function with afterEach(async·function·
(prettier/prettier)
[error] 27-27: Replace ········ with ····
(prettier/prettier)
[error] 28-28: Replace ········ with ····
(prettier/prettier)
[error] 29-29: Delete ··
(prettier/prettier)
[error] 31-31: Replace ····describe("validateOOOCreateRequest",·function with ··describe("validateOOOCreateRequest",·function·
(prettier/prettier)
[error] 32-33: Replace ⏎········beforeEach(async·function with ····beforeEach(async·function·
(prettier/prettier)
[error] 34-34: Delete ······
(prettier/prettier)
[error] 35-35: Replace ················ with ········
(prettier/prettier)
[error] 36-36: Delete ········
(prettier/prettier)
[error] 37-37: Delete ········
(prettier/prettier)
[error] 38-38: Replace ················ with ········
(prettier/prettier)
[error] 39-39: Replace ············ with ······
(prettier/prettier)
[error] 40-40: Delete ····
(prettier/prettier)
[error] 42-42: Replace ········it("should·return·USER_STATUS_NOT_FOUND·if·user·status·not·found",·async·function with ····it("should·return·USER_STATUS_NOT_FOUND·if·user·status·not·found",·async·function·
(prettier/prettier)
[error] 43-43: Replace ············ with ······
(prettier/prettier)
[error] 44-49: Replace ················await·validateOOOCreateRequest(⏎····················testUserId,⏎····················false,⏎····················null,⏎····················validOOORequest⏎················ with ········await·validateOOOCreateRequest(testUserId,·false,·null,·validOOORequest
(prettier/prettier)
[error] 50-50: Replace ············ with ······
(prettier/prettier)
[error] 51-51: Delete ········
(prettier/prettier)
[error] 52-52: Replace ················ with ········
(prettier/prettier)
[error] 53-53: Delete ········
(prettier/prettier)
[error] 54-54: Delete ······
(prettier/prettier)
[error] 55-55: Delete ····
(prettier/prettier)
[error] 57-57: Replace ········it("should·return·OOO_STATUS_ALREADY_EXIST·if·user·status·is·already·OOO",·async·function with ····it("should·return·OOO_STATUS_ALREADY_EXIST·if·user·status·is·already·OOO",·async·function·
(prettier/prettier)
[error] 58-58: Delete ······
(prettier/prettier)
[error] 59-59: Replace ················ with ········
(prettier/prettier)
[error] 60-60: Delete ········
(prettier/prettier)
[error] 61-61: Delete ··········
(prettier/prettier)
[error] 62-62: Delete ··········
(prettier/prettier)
[error] 63-63: Replace ···················· with ··········
(prettier/prettier)
[error] 64-64: Replace ···················· with ··········
(prettier/prettier)
[error] 65-65: Replace ···················· with ··········
(prettier/prettier)
[error] 66-66: Replace ········} with },
(prettier/prettier)
[error] 67-67: Replace ············ with ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-74: Replace ················await·validateOOOCreateRequest(⏎····················testUserId,⏎····················true,⏎····················testUserStatus,⏎····················validOOORequest⏎················ with ········await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 75-75: Delete ······
(prettier/prettier)
[error] 76-76: Replace ················ with ········
(prettier/prettier)
[error] 77-77: Replace ················ with ········
(prettier/prettier)
[error] 78-78: Delete ········
(prettier/prettier)
[error] 79-79: Replace ············ with ······
(prettier/prettier)
[error] 80-80: Replace ········ with ····
(prettier/prettier)
[error] 81-81: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 81-81: Delete ····
(prettier/prettier)
[error] 82-82: Replace ········it("should·return·REQUEST_ALREADY_PENDING·if·user·has·already·pending·request",·async·function with ····it("should·return·REQUEST_ALREADY_PENDING·if·user·has·already·pending·request",·async·function·
(prettier/prettier)
[error] 83-83: Delete ······
(prettier/prettier)
[error] 84-84: Replace ················ with ········
(prettier/prettier)
[error] 85-85: Replace ················ with ········
(prettier/prettier)
[error] 86-86: Delete ··········
(prettier/prettier)
[error] 87-87: Replace ···················· with ··········
(prettier/prettier)
[error] 88-88: Replace ···················· with ··········
(prettier/prettier)
[error] 89-89: Replace ···················· with ··········
(prettier/prettier)
[error] 90-90: Replace ···················· with ··········
(prettier/prettier)
[error] 91-91: Replace ········} with },
(prettier/prettier)
[error] 92-92: Replace ············ with ······
(prettier/prettier)
[error] 93-93: Replace ············ with ······
(prettier/prettier)
[error] 94-99: Replace ················await·validateOOOCreateRequest(⏎····················testUserId,⏎····················true,⏎····················testUserStatus,⏎····················validOOORequest⏎················ with ········await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 100-100: Delete ······
(prettier/prettier)
[error] 101-101: Replace ················ with ········
(prettier/prettier)
[error] 102-102: Replace ················ with ········
(prettier/prettier)
[error] 103-103: Delete ········
(prettier/prettier)
[error] 104-104: Replace ············ with ······
(prettier/prettier)
[error] 105-105: Replace ········ with ····
(prettier/prettier)
[error] 107-107: Replace ········it("should·return·undefined·when·all·validation·checks·passes",·async·function with ····it("should·return·undefined·when·all·validation·checks·passes",·async·function·
(prettier/prettier)
[error] 108-108: Delete ······
(prettier/prettier)
[error] 109-109: Replace ················ with ········
(prettier/prettier)
[error] 110-110: Replace ················ with ········
(prettier/prettier)
[error] 111-111: Replace ···················· with ··········
(prettier/prettier)
[error] 112-112: Replace ···················· with ··········
(prettier/prettier)
[error] 113-113: Replace ···················· with ··········
(prettier/prettier)
[error] 114-114: Delete ··········
(prettier/prettier)
[error] 115-115: Delete ··········
(prettier/prettier)
[error] 116-116: Replace ················} with ········},
(prettier/prettier)
[error] 117-117: Replace ············ with ······
(prettier/prettier)
[error] 118-123: Replace ············const·response·=·await·validateOOOCreateRequest(⏎················testUserId,⏎················true,⏎················testUserStatus,⏎················null⏎············ with ······const·response·=·await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·null
(prettier/prettier)
[error] 124-124: Replace ············ with ······
(prettier/prettier)
[error] 124-124: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 125-125: Replace ········ with ····
(prettier/prettier)
[error] 127-127: Replace ········it("should·throw·error",·async·function with ····it("should·throw·error",·async·function·
(prettier/prettier)
[error] 128-128: Delete ······
(prettier/prettier)
[error] 129-129: Delete ······
(prettier/prettier)
[error] 131-131: Delete ······
(prettier/prettier)
[error] 132-132: Replace ················ with ········
(prettier/prettier)
[error] 133-133: Replace ················ with ········
(prettier/prettier)
[error] 134-134: Replace ···················· with ··········
(prettier/prettier)
[error] 135-135: Replace ···················· with ··········
(prettier/prettier)
[error] 136-136: Replace ···················· with ··········
(prettier/prettier)
[error] 137-137: Delete ··········
(prettier/prettier)
[error] 138-138: Delete ··········
(prettier/prettier)
[error] 139-139: Replace ················} with ········},
(prettier/prettier)
[error] 140-140: Delete ······
(prettier/prettier)
[error] 142-142: Delete ······
(prettier/prettier)
[error] 143-148: Replace ················await·validateOOOCreateRequest(⏎····················testUserId,⏎····················true,⏎····················testUserStatus,⏎····················validOOORequest⏎················ with ········await·validateOOOCreateRequest(testUserId,·true,·testUserStatus,·validOOORequest
(prettier/prettier)
[error] 149-149: Replace ············}·catch with ······}·catch·
(prettier/prettier)
[error] 150-150: Replace ················ with ········
(prettier/prettier)
[error] 151-151: Replace ················ with ········
(prettier/prettier)
[error] 151-151: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 152-152: Replace ············ with ······
(prettier/prettier)
[error] 153-153: Replace ········ with ····
(prettier/prettier)
[error] 154-154: Delete ··
(prettier/prettier)
[error] 156-156: Replace ····describe("createOOORequest",·function with ··describe("createOOORequest",·function·
(prettier/prettier)
[error] 157-157: Replace ········it("should·create·OOO·request",·async·function with ····it("should·create·OOO·request",·async·function·
(prettier/prettier)
[error] 158-158: Replace ············ with ······
(prettier/prettier)
[error] 159-159: Replace ················ with ········
(prettier/prettier)
[error] 160-160: Replace ················ with ········
(prettier/prettier)
[error] 161-161: Replace ···················· with ··········
(prettier/prettier)
[error] 162-162: Delete ··········
(prettier/prettier)
[error] 163-163: Replace ···················· with ··········
(prettier/prettier)
[error] 164-164: Replace ···················· with ··········
(prettier/prettier)
[error] 165-165: Replace ···················· with ··········
(prettier/prettier)
[error] 166-166: Replace ················} with ········},
(prettier/prettier)
[error] 167-167: Replace ············ with ······
(prettier/prettier)
[error] 168-168: Replace ············ with ······
(prettier/prettier)
[error] 169-173: Replace ······const·response·=·await·createOOORequest(⏎················validOooStatusRequests,⏎················"test-username-1",⏎················testUserId⏎············ with const·response·=·await·createOOORequest(validOooStatusRequests,·"test-username-1",·testUserId
(prettier/prettier)
[error] 174-174: Delete ······
(prettier/prettier)
[error] 174-174: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 175-175: Delete ······
(prettier/prettier)
[error] 176-176: Replace ················ with ········
(prettier/prettier)
[error] 177-177: Replace ················ with ········
(prettier/prettier)
[error] 178-178: Replace ················ with ········
(prettier/prettier)
[error] 179-179: Replace ················ with ········
(prettier/prettier)
[error] 180-180: Delete ········
(prettier/prettier)
[error] 181-181: Replace ················ with ········
(prettier/prettier)
[error] 182-182: Replace ················ with ········
(prettier/prettier)
[error] 183-183: Delete ········
(prettier/prettier)
[error] 184-184: Replace ················comment:·null with ········comment:·null,
(prettier/prettier)
[error] 185-185: Replace ············ with ······
(prettier/prettier)
[error] 186-186: Replace ········ with ····
(prettier/prettier)
[error] 188-188: Replace ········it("should·throw·error",·async·function with ····it("should·throw·error",·async·function·
(prettier/prettier)
[error] 189-189: Replace ············ with ······
(prettier/prettier)
[error] 190-190: Delete ······
(prettier/prettier)
[error] 192-192: Replace ············ with ······
(prettier/prettier)
[error] 193-197: Replace ················await·createOOORequest(⏎····················validOooStatusRequests,⏎····················"test-username-1",⏎····················testUserId⏎················ with ········await·createOOORequest(validOooStatusRequests,·"test-username-1",·testUserId
(prettier/prettier)
[error] 198-198: Replace ············}·catch with ······}·catch·
(prettier/prettier)
[error] 199-199: Replace ················ with ········
(prettier/prettier)
[error] 200-200: Delete ········
(prettier/prettier)
[error] 200-200: Expected an assignment or function call and instead saw an expression.
(no-unused-expressions)
[error] 201-201: Delete ······
(prettier/prettier)
[error] 202-202: Delete ····
(prettier/prettier)
[error] 203-203: Delete ··
(prettier/prettier)
🔇 Additional comments (20)
controllers/requests.ts (3)
19-19: LGTM: New import for NextFunction.The NextFunction import from express is properly added to support error handling middleware in the request controller.
23-24: Function signature updated to include NextFunction parameter.The controller function signature now includes the NextFunction parameter, which aligns with Express middleware pattern and enables better error handling. This change is well structured.
29-29: Updated call to createOooRequestController to pass the next parameter.The controller call now correctly passes the next parameter, enabling middleware chaining and error handling for OOO requests.
test/integration/requests.test.ts (8)
51-52: Good: Improved variable naming for clarity.Using a more descriptive constant name
requestsEndpointinstead of a hardcoded string improves code readability and maintainability.
73-79: Good: Added user status initialization.Setting up the user status as "ACTIVE" before tests ensures a consistent test environment.
89-103: Good: Added test for feature flag.This test properly verifies that the feature is disabled when the dev flag is set to false, returning a 501 status code.
118-133: Good: Added test for Discord user validation.Testing the case where a user is not part of Discord is important for validating authorization rules.
135-148: Good: Added test for server error handling.Testing the error handling path when request creation fails helps ensure robustness.
164-172: Good: Added database verification.Verifying that data was actually persisted in the database is a better test practice than just checking the response.
191-204: Good: Updated test for schema validation.The test now correctly checks for the "reason" field instead of "message", aligning with schema changes.
206-219: Good: Updated test for field validation.The test now correctly validates that the "status" field is not allowed instead of "state", aligning with schema changes.
controllers/oooRequests.ts (9)
1-1: NextFunction import added for error handling middleware.This addition allows for passing errors to Express middleware, aligning with best practices for error handling in Express applications.
12-12: Clear error messaging for Discord user requirement.Good addition of a specific error constant for unauthorized users who aren't part of Discord.
16-16: Import updated to include specific functions.The import statement is now more explicit about which functions are being imported from the requests model.
19-19: Service layer implementation - Good job!You've successfully implemented the service layer pattern as suggested in previous feedback.
21-21: Type definitions imported for better type safety.The addition of specific type imports improves type safety and code clarity.
24-24: Controller signature updated with NextFunction parameter.This change enables proper error handling through Express middleware chain.
25-27: Clean data extraction from request.Good job removing optional chaining and directly destructuring the required fields from userData.
38-38: Service layer handles core business logic.Nice implementation of delegating the OOO request creation to a dedicated service.
45-45: Proper error handling with middleware.Using
next(err)allows for centralized error handling through Express middleware.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
test/fixtures/oooRequest/oooRequest.ts (3)
21-26: 🛠️ Refactor suggestion
invalidOooStatusRequestsshould also usereasoninstead ofmessage.To maintain consistency with the schema changes, this fixture should be updated too.
export const invalidOooStatusRequests = { type: "OOO", from: Date.now() + 100000, until: Date.now() + 200000, - message: "Out of office for personal reasons.", + reason: "Out of office for personal reasons.", };
53-61: 🛠️ Refactor suggestionUpdate
createOooRequeststo usereasoninstead ofmessageand removestate.The fixtures should be consistent with the new schema changes to avoid confusion.
export const createOooRequests = { requestedBy: "testUser", type: "OOO", from: Date.now() + 100000, until: Date.now() + 200000, - message: "Out of office for personal reasons.", - state: REQUEST_STATE.PENDING, + reason: "Out of office for personal reasons.", };
62-69: 🛠️ Refactor suggestionUpdate
createOooRequests2to align with the schema changes.For consistency, this fixture should also be updated.
export const createOooRequests2 = { requestedBy: "testUser2", type: "OOO", from: Date.now() + 100000, until: Date.now() + 200000, - message: "Out of office for personal reasons.", - state: REQUEST_STATE.PENDING, + reason: "Out of office for personal reasons.", };
♻️ Duplicate comments (1)
services/oooRequest.ts (1)
1-17:⚠️ Potential issueMissing import for the logger utility.
The file uses
loggeron lines 39 and 96, but there's no import statement for it. This would cause a runtime error.import { logType } from "../constants/logs"; import { LOG_ACTION, OOO_STATUS_ALREADY_EXIST, REQUEST_ALREADY_PENDING, REQUEST_LOG_TYPE, REQUEST_STATE, REQUEST_TYPE, USER_STATUS_NOT_FOUND, } from "../constants/requests"; import { userState } from "../constants/userStatus"; import { createRequest, getRequestByKeyValues } from "../models/requests"; import { getUserStatus } from "../models/userStatus"; import { OooStatusRequest, OooStatusRequestBody } from "../types/oooRequest"; import { currentStatus } from "../types/userCurrentStatus"; import { addLog } from "./logService"; import { NotFound, Forbidden, Conflict } from "http-errors"; +import logger from "../utils/logger";
🧹 Nitpick comments (3)
types/userCurrentStatus.d.ts (1)
3-9: Consider using PascalCase for the type name to follow TypeScript conventions.In TypeScript, the conventional naming pattern for interface and type definitions is PascalCase. Consider renaming
currentStatustoCurrentStatusfor better consistency with TypeScript best practices.-export type currentStatus = { +export type CurrentStatus = { from: number, until: number, state: REQUEST_TYPE, message: string, updatedAt: number, };test/fixtures/oooRequest/oooRequest.ts (1)
130-130: Add a newline at the end of the file.Adding a newline at the end of the file is a common convention in many codebases and would resolve the ESLint warning.
]; +🧰 Tools
🪛 ESLint
[error] 130-130: Insert
⏎(prettier/prettier)
services/oooRequest.ts (1)
19-23: Improve type definition for theuserStatusparameter.The current type is imprecise. Consider using a proper type definition that leverages the imported
currentStatustype.export const validateUserStatus = async ( userId: string, userStatusExists: boolean, - userStatus: { currentStatus }, + userStatus: { currentStatus: currentStatus }, ) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/oooRequest.ts(1 hunks)test/fixtures/oooRequest/oooRequest.ts(2 hunks)test/integration/requests.test.ts(6 hunks)test/unit/middlewares/oooRequests.test.ts(5 hunks)types/userCurrentStatus.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/middlewares/oooRequests.test.ts
- test/integration/requests.test.ts
🧰 Additional context used
🪛 ESLint
test/fixtures/oooRequest/oooRequest.ts
[error] 130-130: Insert ⏎
(prettier/prettier)
🔇 Additional comments (8)
test/fixtures/oooRequest/oooRequest.ts (2)
18-18: Field name change frommessagetoreasonlooks good.This change aligns with the updated schema. Note that there are still other fixtures in this file using the old
messagefield name that should eventually be updated for consistency.
3-12: 🛠️ Refactor suggestion
createOooStatusRequestsshould also usereasoninstead ofmessage.For consistency with the changes made to
validOooStatusRequests, consider updating this fixture to use the new field name.export const createOooStatusRequests = { type: "OOO", requestedBy: "user123", from: Date.now() + 100000, until: Date.now() + 200000, - message: "Out of office for personal reasons.", + reason: "Out of office for personal reasons.", state: REQUEST_STATE.PENDING, createdAt: 1234567890, updatedAt: 1234567890, };services/oooRequest.ts (6)
31-37: Good error handling for OOO status validation.The validation logic correctly checks if the user is already in OOO state and throws an appropriate error with a clear message.
55-60: Consider using object property shorthand foruserId.When the property name and variable name are identical, you can use the object shorthand syntax.
const latestOOORequest: OooStatusRequest = await getRequestByKeyValues({ - userId: userId, + userId, type: REQUEST_TYPE.OOO, status: REQUEST_STATE.PENDING, });
51-53: Good optimization: Get user status before validation.This approach avoids duplicate database calls by retrieving the user status once and passing it to the validation function.
61-67: Good implementation of pending request validation.The code correctly checks for existing pending OOO requests before creating a new one, preventing duplicate requests and providing clear error messages.
69-79: Appropriate request creation with all required fields.The implementation properly creates a new OOO request with all the necessary fields, including setting the initial status to PENDING.
81-92: Good audit logging implementation.The code properly logs the request creation action with detailed metadata, which is important for tracking and audit purposes.
AnujChhikara
left a comment
AnujChhikara
left a comment
There was a problem hiding this comment.
If the test PR is going to be merged first and it includes skipped tests, then ideally, those skipped tests should be unskipped in this feature PR.
surajmaity1
force-pushed
the
feat/user-ooo-create
branch
from
82ed0b8 to
fa7f490
Compare
surajmaity1
force-pushed
the
feat/user-ooo-create
branch
from
b1166d5 to
e86148e
Compare
surajmaity1
force-pushed
the
feat/user-ooo-create
branch
from
e86148e to
165a405
Compare
surajmaity1
force-pushed
the
feat/user-ooo-create
branch
from
165a405 to
9bfb615
Compare
| const dev = req.query.dev === "true"; | ||
|
|
||
| if (!userId) { | ||
| return res.boom.unauthorized(); | ||
| if (!dev) return res.boom.notImplemented("Feature not implemented"); |
There was a problem hiding this comment.
NIT: we should have checked this condition at the beginning it self
There was a problem hiding this comment.
I'll fix this.
Thank you.
11 participants
Date: 23 Feb, 2025
Developer Name: @surajmaity1
Issue Ticket Number
Description
This PR contains a feature for creating OOO requests.
Introduced enhanced error messages for request submissions, including specific notifications for OOO requests and user status checks.
Added a new type definition for user current status to improve data handling.
Updated request payloads by standardizing on a "reason" field and enforcing required fields.
Fixed integration and unit tests to verify the updated validations and response formats, including checks for the new "reason" field.
User OOO Management Design Document
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Feature.to.create.OOO.mp4
Screenshot 2
Test Coverage
Screenshot 1
Additional Notes
Summary by CodeRabbit