fix: change state to status #2407
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
WalkthroughThis update standardizes the terminology for request status across the codebase by renaming the constant object Changes
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (2)
types/requests.d.ts (1)
11-20: 🛠️ Refactor suggestionProperty name should be updated for consistency
The
stateproperty inRequestQuerytype should be renamed tostatusto maintain consistency with the PR's objective of replacing "state" with "status" throughout the codebase.export type RequestQuery = { type?: string; requestedBy?: string; - state?: REQUEST_STATUS.APPROVED | REQUEST_STATUS.PENDING | REQUEST_STATUS.REJECTED; + status?: REQUEST_STATUS.APPROVED | REQUEST_STATUS.PENDING | REQUEST_STATUS.REJECTED; id?: string; prev?: string; next?: string; page?: number; size?: number; };controllers/oooRequests.ts (1)
99-106:⚠️ Potential issueFix duplicate status property in userFutureStatusData object
There's a duplicate
statusproperty in the userFutureStatusData object, which will cause the first value to be overwritten.Fix the duplicate property by removing one of the status assignments or renaming one of them:
const userFutureStatusData = { requestId, - status: REQUEST_TYPE.OOO, status: statusState.UPCOMING, from, endsOn: until, userId: requestedBy, message, };🧰 Tools
🪛 Biome (1.9.4)
[error] 100-100: This property value named status is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named status(lint/suspicious/noDuplicateObjectKeys)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (30)
.gitignore(0 hunks)config/default.js(0 hunks)constants/requests.ts(1 hunks)controllers/auth.js(1 hunks)controllers/extensionRequestsv2.ts(3 hunks)controllers/onboardingExtension.ts(5 hunks)controllers/oooRequests.ts(4 hunks)controllers/taskRequestsv2.ts(5 hunks)middlewares/validators/extensionRequestsv2.ts(2 hunks)middlewares/validators/oooRequests.ts(2 hunks)middlewares/validators/requests.ts(3 hunks)middlewares/validators/taskRequests.ts(2 hunks)models/discordactions.js(2 hunks)models/requests.ts(2 hunks)services/onboardingExtension.ts(2 hunks)test/fixtures/extension-requests/extensionRequests.ts(2 hunks)test/fixtures/oooRequest/oooRequest.ts(8 hunks)test/fixtures/taskRequests/taskRequests.ts(3 hunks)test/integration/discordactions.test.js(2 hunks)test/integration/onboardingExtension.test.ts(15 hunks)test/integration/requests.test.ts(5 hunks)test/unit/middlewares/extensionRequests.test.ts(2 hunks)test/unit/models/discordactions.test.js(5 hunks)test/unit/models/requests.test.ts(3 hunks)test/unit/services/onboardingExtension.test.ts(3 hunks)types/extensionRequests.d.ts(4 hunks)types/onboardingExtension.d.ts(3 hunks)types/oooRequest.d.ts(4 hunks)types/requests.d.ts(1 hunks)types/taskRequests.d.ts(4 hunks)
💤 Files with no reviewable changes (2)
- .gitignore
- config/default.js
🧰 Additional context used
🧠 Learnings (1)
test/fixtures/oooRequest/oooRequest.ts (1)
Learnt from: surajmaity1
PR: Real-Dev-Squad/website-backend#2386
File: test/fixtures/oooRequest/oooRequest.ts:59-60
Timestamp: 2025-03-16T05:23:33.460Z
Learning: The `createOooRequests2` object in test/fixtures/oooRequest/oooRequest.ts still uses the old property names `message` and `state` (instead of `reason` and `status`) because it's specifically used in acknowledging OOO request tests. This naming inconsistency will be fixed by surajmaity1 in a dedicated PR for acknowledging OOO requests.
🧬 Code Graph Analysis (22)
middlewares/validators/taskRequests.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
test/fixtures/extension-requests/extensionRequests.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
test/unit/middlewares/extensionRequests.test.ts (2)
test/fixtures/extension-requests/extensionRequests.ts (1)
extensionCreateObject(3-11)constants/requests.ts (1)
REQUEST_STATUS(1-5)
test/fixtures/oooRequest/oooRequest.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
middlewares/validators/extensionRequestsv2.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
test/unit/models/discordactions.test.js (2)
models/discordactions.js (16)
require(1-1)require(7-7)require(8-8)require(9-9)require(10-10)require(11-11)require(12-12)require(16-16)require(23-23)require(24-24)require(25-25)require(27-27)require(29-29)require(30-30)require(31-31)require(32-32)constants/requests.ts (1)
REQUEST_STATUS(1-5)
test/integration/discordactions.test.js (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
middlewares/validators/oooRequests.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
test/fixtures/taskRequests/taskRequests.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
models/discordactions.js (2)
test/unit/models/discordactions.test.js (14)
require(15-15)require(17-17)require(20-44)require(45-53)require(55-55)require(58-58)require(61-61)require(62-62)require(63-63)require(64-64)require(65-65)require(66-66)require(67-67)require(68-68)constants/requests.ts (1)
REQUEST_STATUS(1-5)
models/requests.ts (1)
constants/requests.ts (2)
REQUEST_STATUS(1-5)REQUEST_ALREADY_APPROVED(35-35)
services/onboardingExtension.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
controllers/onboardingExtension.ts (2)
constants/requests.ts (1)
REQUEST_STATUS(1-5)constants/logs.ts (1)
logType(3-26)
test/unit/models/requests.test.ts (3)
constants/requests.ts (1)
REQUEST_STATUS(1-5)models/requests.ts (1)
getRequests(72-162)test/fixtures/oooRequest/oooRequest.ts (1)
createOooStatusRequests(3-12)
test/unit/services/onboardingExtension.test.ts (1)
constants/requests.ts (2)
REQUEST_STATUS(1-5)REQUEST_TYPE(13-19)
middlewares/validators/requests.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
controllers/extensionRequestsv2.ts (2)
controllers/extensionRequests.js (2)
latestExtensionRequest(52-54)latestExtensionRequest(155-157)constants/requests.ts (1)
REQUEST_STATUS(1-5)
types/taskRequests.d.ts (3)
constants/requests.ts (1)
REQUEST_STATUS(1-5)types/extensionRequests.d.ts (1)
RequestQuery(44-54)types/requests.d.ts (1)
RequestQuery(11-20)
types/extensionRequests.d.ts (3)
constants/requests.ts (1)
REQUEST_STATUS(1-5)types/requests.d.ts (1)
RequestQuery(11-20)types/taskRequests.d.ts (1)
RequestQuery(50-60)
types/oooRequest.d.ts (3)
constants/requests.ts (1)
REQUEST_STATUS(1-5)types/requests.d.ts (2)
RequestQuery(11-20)RequestParams(22-24)types/onboardingExtension.d.ts (1)
RequestParams(49-51)
types/onboardingExtension.d.ts (1)
constants/requests.ts (1)
REQUEST_STATUS(1-5)
types/requests.d.ts (3)
constants/requests.ts (2)
REQUEST_TYPE(13-19)REQUEST_STATUS(1-5)types/extensionRequests.d.ts (1)
RequestQuery(44-54)types/taskRequests.d.ts (1)
RequestQuery(50-60)
🪛 ESLint
test/fixtures/oooRequest/oooRequest.ts
[error] 43-43: Insert ·
(prettier/prettier)
[error] 70-71: Delete ⏎
(prettier/prettier)
test/unit/services/onboardingExtension.test.ts
[error] 6-6: Replace ····REQUEST_STATUS,· with ··REQUEST_STATUS,
(prettier/prettier)
[error] 6-6: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 32-32: Replace ············ with ······
(prettier/prettier)
[error] 58-58: Delete ········
(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: Replace ············ with ······
(prettier/prettier)
[error] 63-63: Delete ······
(prettier/prettier)
[error] 64-64: Replace ················ with ········
(prettier/prettier)
[error] 65-65: Replace ················ with ········
(prettier/prettier)
🪛 Biome (1.9.4)
controllers/oooRequests.ts
[error] 100-100: This property value named status is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named status
(lint/suspicious/noDuplicateObjectKeys)
types/taskRequests.d.ts
[error] 4-4: Shouldn't redeclare 'REQUEST_STATUS'. Consider to delete it or rename it.
'REQUEST_STATUS' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (55)
constants/requests.ts (1)
1-1: LGTM! Good change to renameREQUEST_STATEtoREQUEST_STATUS.This change aligns with the PR objective to improve naming consistency throughout the codebase.
test/fixtures/extension-requests/extensionRequests.ts (2)
1-1: Import statement updated correctly.The import statement has been properly updated to reference
REQUEST_STATUSinstead ofREQUEST_STATE.
10-10: Reference toREQUEST_STATUS.PENDINGlooks good.The property correctly uses the renamed constant.
middlewares/validators/taskRequests.ts (2)
4-4: Import statement updated correctly.The import statement has been properly updated to reference
REQUEST_STATUSinstead ofREQUEST_STATE.
39-39: Validation schema updated correctly.The validation schema has been updated to reference
REQUEST_STATUS.PENDING, which is consistent with the constant renaming.middlewares/validators/extensionRequestsv2.ts (2)
4-4: Import statement updated correctly.The import statement has been properly updated to reference
REQUEST_STATUSinstead ofREQUEST_STATE.
35-35: Validation schema updated correctly.The validation schema has been updated to reference
REQUEST_STATUS.PENDING, which is consistent with the constant renaming.test/unit/middlewares/extensionRequests.test.ts (1)
7-7: Import update for consistencyThe import has been correctly updated from
REQUEST_STATEtoREQUEST_STATUSto align with the constant renaming in the constants file.middlewares/validators/oooRequests.ts (1)
3-3: Import update for consistencyThe import has been correctly updated from
REQUEST_STATEtoREQUEST_STATUSto align with the constant renaming in the constants file.models/requests.ts (2)
3-3: Import update for consistencyThe import has been correctly updated from
REQUEST_STATEtoREQUEST_STATUSto align with the constant renaming in the constants file.
73-73:Details
❌ Incorrect review comment
Potential missed property rename
This line destructures a query parameter named
statewhich appears to be related to the request status. If the property name is being changed from "state" to "status" throughout the codebase, this variable name might need to be updated as well.
🏁 Script executed:
#!/bin/bash # Check how this query parameter is used in API calls rg -t ts "state: REQUEST_STATUS" --glob "**/controllers/**"Length of output: 346
🏁 Script executed:
#!/bin/bash # Display the context around the destructuring in models/requests.ts sed -n '1,150p' models/requests.tsLength of output: 4057
No change needed: “state” is still the correct query property
The
statefield remains in use throughout the codebase (models, controllers, Firestore documents) and hasn’t been renamed tostatus. The destructuring inmodels/requests.tsis consistent with:
- Firestore document field (
.data().state)- Constants (
REQUEST_STATUS)- Controller filters (
.where("state", "==", state))You can safely ignore the suggested rename.
Likely an incorrect or invalid review comment.
test/fixtures/taskRequests/taskRequests.ts (1)
1-1: Import update for consistencyThe import has been correctly updated from
REQUEST_STATEtoREQUEST_STATUSto align with the constant renaming in the constants file.models/discordactions.js (2)
32-32: Update import to useREQUEST_STATUS.
The import has been updated fromREQUEST_STATEtoREQUEST_STATUS, matching the renamed constant inconstants/requests.ts. Ensure all references to this constant elsewhere in the file and related modules are aligned.
781-781: ReplaceREQUEST_STATEreference withREQUEST_STATUS.
The filter now usesREQUEST_STATUS.APPROVEDto check for approved extension requests, aligning with the renamed constant. Confirm that all calls togetRequestByKeyValuesinclude the correct status parameter.test/unit/models/discordactions.test.js (2)
67-67: Align import with renamed constantREQUEST_STATUS.
The test import has been updated to pull inREQUEST_STATUSinstead ofREQUEST_STATE. Ensure that test fixtures and related assertions reference the updated constant name across the test suite.
1404-1404: Update test to useREQUEST_STATUS.
The test now usesREQUEST_STATUS.APPROVEDwhen creating a fixture, matching the new constant. Verify that all fixtures (test/fixtures/...) are also updated accordingly.test/unit/models/requests.test.ts (4)
11-11: ImportREQUEST_STATUSandREQUEST_TYPEfrom constants.
The test file now importsREQUEST_STATUS(renamed) instead of the oldREQUEST_STATE. Confirm that all references to request state in test cases useREQUEST_STATUS.
118-118: FiltergetRequestsbyREQUEST_STATUS.APPROVED.
The query now usesstate: REQUEST_STATUS.APPROVED. Ensure the model handles this filter correctly and returns the expected results.
126-126: FiltergetRequestsbyREQUEST_STATUS.PENDING.
Updated test to filter with the new constant for pending state. Validate that the service logic remains consistent with this change.
147-147: Expectnullwhen no matchingREQUEST_STATUS.PENDINGrequests exist.
The test assertsnullresults for pending state queries. Confirm thatgetRequestsreturnsnullin this scenario per implementation.controllers/taskRequestsv2.ts (5)
1-1: UseREQUEST_STATUSinstead ofREQUEST_STATE.
The import now referencesREQUEST_STATUSfromconstants/requests, reflecting the renamed constant. Verify other controllers for similar imports.
67-67: CheckexistingRequest.stateagainstREQUEST_STATUS.PENDING.
ReplacedREQUEST_STATE.PENDINGcheck withREQUEST_STATUS.PENDING. Ensure this condition correctly prevents duplicate pending requests.
73-73: AssignREQUEST_STATUS.PENDINGon new requestors.
The branch handling additional requestors now pushesstate: REQUEST_STATUS.PENDING. Confirm that audit logs and downstream logic interpret this status correctly.
119-119: Initialize new request withREQUEST_STATUS.PENDING.
Setting the initial state for newly created task requests. Verify the repository layer persists this correctly.
132-132: Set user entrystatetoREQUEST_STATUS.PENDING.
Within theusersarray, each entry now hasstate: REQUEST_STATUS.PENDING. Confirm client code and type definitions expect this shape.test/unit/services/onboardingExtension.test.ts (3)
32-32: UseREQUEST_STATUS.PENDINGfor valid extension.
The setup now marks the initialstateasREQUEST_STATUS.PENDING. Confirm thatvalidateOnboardingExtensionUpdateRequesttreats this as a valid baseline.🧰 Tools
🪛 ESLint
[error] 32-32: Replace
············with······(prettier/prettier)
58-58: Validate error for approved extension state.
Invalid state request fixture usesREQUEST_STATUS.APPROVED. Ensure the service returnsPENDING_REQUEST_UPDATEDfor this case.🧰 Tools
🪛 ESLint
[error] 58-58: Delete
········(prettier/prettier)
65-65: Test invalid deadline withREQUEST_STATUS.PENDING.
The fixture usesREQUEST_STATUS.PENDINGbut with a futureoldEndsOn. Confirm this triggers theINVALID_REQUEST_DEADLINEerror.🧰 Tools
🪛 ESLint
[error] 65-65: Replace
················with········(prettier/prettier)
middlewares/validators/requests.ts (3)
3-3: Import renamed fromREQUEST_STATEtoREQUEST_STATUSThis change aligns with the PR objective to rename "state" to "status" for improved consistency throughout the codebase.
62-64: Updated validation to useREQUEST_STATUSinstead ofREQUEST_STATEThe validation logic remains the same, but now references the renamed constant. This properly maintains functionality while improving terminology consistency.
91-94: Updated validation to useREQUEST_STATUSinstead ofREQUEST_STATEThe request state validation in the
getRequestsMiddlewarefunction now uses the renamed constant, maintaining the same validation logic.test/fixtures/oooRequest/oooRequest.ts (3)
1-1: Import renamed fromREQUEST_STATEtoREQUEST_STATUSThis change aligns with the PR objective to rename "state" to "status" for improved consistency.
9-9: Updated all uses ofREQUEST_STATEtoREQUEST_STATUSAll references to the constant have been consistently updated throughout the test fixtures, ensuring that the test code uses the same terminology as the implementation code.
Also applies to: 19-19, 30-30, 37-37, 44-44, 61-61, 69-69, 77-77, 89-89, 102-102, 113-113, 126-126
63-70: Note about the existing property name inconsistencyAccording to previous learnings, the
createOooRequests2object still uses the old property names (messageandstateinstead ofreasonandstatus). This is a known inconsistency that will be addressed in a separate PR specifically for acknowledging OOO requests.test/integration/discordactions.test.js (2)
52-52: Import renamed fromREQUEST_STATEtoREQUEST_STATUSThis change aligns with the PR objective to rename "state" to "status" for improved consistency. The import now correctly references the renamed constant.
917-917: Updated usage fromREQUEST_STATE.APPROVEDtoREQUEST_STATUS.APPROVEDThis update ensures the test uses the correct renamed constant when creating a request object with approved status.
services/onboardingExtension.ts (2)
2-12: Import renamed fromREQUEST_STATEtoREQUEST_STATUSThis change aligns with the PR objective to rename "state" to "status" for improved consistency. The import statement now correctly references the renamed constant.
66-66: Updated condition check to useREQUEST_STATUS.PENDINGinstead ofREQUEST_STATE.PENDINGThe validation logic in
validateOnboardingExtensionUpdateRequestnow uses the renamed constant, maintaining the same functionality with improved terminology consistency.controllers/onboardingExtension.ts (5)
11-11: Import update looks goodThe import statement has been updated to use
REQUEST_STATUSinstead ofREQUEST_STATE, which aligns with the PR objective of changing state to status throughout the codebase.
76-76: State reference update is consistentSuccessfully updated to use the new
REQUEST_STATUSconstant instead ofREQUEST_STATEwhile maintaining the same functionality.
96-96: Status check update looks goodReplaced
REQUEST_STATE.REJECTEDwithREQUEST_STATUS.REJECTEDcorrectly in the conditional check.
108-108: Request creation parameter updated correctlyUpdated to use the new
REQUEST_STATUSconstant for creating a request with PENDING status.
182-182: Conditional check updated appropriatelyThe status check in the conditional expression has been updated from
REQUEST_STATEtoREQUEST_STATUSwhile maintaining the same logic.test/integration/requests.test.ts (5)
21-21: Import update is consistent with other filesUpdated import from
REQUEST_STATEtoREQUEST_STATUScorrectly.
59-59: Request update function call updated correctlyThe update request function call now uses the new
REQUEST_STATUS.APPROVEDconstant.
125-125: Test assertion updated to use the new constantSuccessfully updated the test assertion to check against
REQUEST_STATUS.PENDINGinstead ofREQUEST_STATE.PENDING.
179-179: POST request test updated correctlyThe test for sending state in POST request body now uses the new constant.
595-596: Request objects updated consistentlyBoth approved and rejected extension request objects now use the new
REQUEST_STATUSconstants.Also applies to: 600-601
types/requests.d.ts (3)
2-2: Import statement updated correctlyImport statement now correctly references
REQUEST_STATUSinstead ofREQUEST_STATE.
8-8: Type definition updated appropriatelyThe type definition for
stateinUpdateRequestBodyhas been updated to useREQUEST_STATUSenum values.
14-14: Type constraint updated correctlyThe type constraint for the
statequery parameter is now using the newREQUEST_STATUSconstant, maintaining the same functionality.test/integration/onboardingExtension.test.ts (3)
11-11: Import statement updated correctlyThe import statement now references
REQUEST_STATUSinstead ofREQUEST_STATE.
45-45: Property name updated to 'status'The property name has been updated from
statetostatusin this extension request object, which aligns with the PR objective.
1-842:Details
❌ Incorrect review comment
Comprehensive property name check needed
While the constant
REQUEST_STATEhas been successfully renamed toREQUEST_STATUSthroughout the file, the property namestateis inconsistently updated tostatus. Only a few occurrences of the property name have been changed (like line 45), but most remain asstate. A comprehensive review and update of all property names fromstatetostatusis needed to complete the PR objective of standardizing terminology.Run this script to find all occurrences of the property name
statein the test file:
🏁 Script executed:
#!/bin/bash # Find all occurrences of the property 'state' in the file cat test/integration/onboardingExtension.test.ts | grep -n 'state:' || echo "No occurrences of 'state:' found" cat test/integration/onboardingExtension.test.ts | grep -n 'state)' || echo "No occurrences of 'state)' found" cat test/integration/onboardingExtension.test.ts | grep -n 'state ==' || echo "No occurrences of 'state ==' found" cat test/integration/onboardingExtension.test.ts | grep -n '.state' || echo "No occurrences of '.state' found" cat test/integration/onboardingExtension.test.ts | grep -n '"state"' || echo "No occurrences of '\"state\"' found"Length of output: 3514
No changes needed for the
stateproperty in testsI’ve searched the entire test file and confirmed that every reference to the request’s property remains
state(no occurrences of astatus:property). Since the code still usesstateon the request objects, all of thestate:keys and.stateassertions in the tests are correct as written. You can safely ignore the suggestion to rename them tostatus.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 41-41: This let declares a variable that is only assigned once.
'testUserDiscordIdForInvalidDiscordJoinedDate' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
types/oooRequest.d.ts (1)
43-45: LGTM: Interface type improvementsThe changes to
OooRequestCreateRequestandOooRequestUpdateRequesttypes improve the explicitness of the property types, which is a good practice.
| // if (!userData.email) { | ||
| // const githubBaseUrl = config.get("githubApi.baseUrl"); | ||
| // const res = await fetch(`${githubBaseUrl}/user/emails`, { | ||
| // headers: { | ||
| // Authorization: `token ${accessToken}`, | ||
| // }, | ||
| // }); | ||
| // const emails = await res.json(); | ||
| // const primaryEmails = emails.filter((item) => item.primary); | ||
|
|
||
| // if (primaryEmails.length > 0) { | ||
| // userData.email = primaryEmails[0].email; | ||
| // } | ||
| // } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commenting out email fallback mechanism doesn't align with PR objectives
This change removes a fallback mechanism that retrieves a user's primary email when not provided in the initial GitHub response. This modification doesn't relate to the PR's stated objective of renaming "state" to "status" in onboarding-related files.
Removing this fallback could potentially cause authentication failures when users have private email settings on GitHub, as userData.email might be undefined when passed to users.addOrUpdate() on line 190.
🏁 Script executed:
#!/bin/bash
# Check if the addOrUpdate function requires an email field
rg -A 5 "addOrUpdate" models/users.jsLength of output: 391
I want to confirm whether addOrUpdate enforces or uses the email field. Let’s pull in its full implementation and see any email-related logic:
🏁 Script executed:
#!/bin/bash
# Show the full addOrUpdate implementation
rg -A 200 -n "const addOrUpdate" models/users.js
# Highlight any usage of userData.email within that function
rg -n "userData.email" -A 5 models/users.jsLength of output: 7631
To confirm how addOrUpdate is invoked and how an undefined email propagates, let’s locate its usage in controllers/auth.js and inspect the surrounding logic:
🏁 Script executed:
#!/bin/bash
# Find where addOrUpdate is called in the auth controller
rg -n "addOrUpdate" controllers/auth.js -A5
# Show the context around line 190 of controllers/auth.js
sed -n '170,210p' controllers/auth.jsLength of output: 2025
Let’s inspect how userData is initialized in the GitHub callback to see if email is ever set:
🏁 Script executed:
#!/bin/bash
# Show the initialization of userData in controllers/auth.js
rg -n "const userData" -A15 controllers/auth.jsLength of output: 861
Let’s inspect the entire GitHub callback handler (lines 130–260) to see how userData is constructed:
🏁 Script executed:
#!/bin/bash
# Dump lines 130–260 of the auth controller to view GitHub callback and userData init
sed -n '130,260p' controllers/auth.jsLength of output: 4307
Re-enable GitHub email fallback or properly handle profile.emails
By commenting out the block that fetches a user’s private email via /user/emails, any GitHub user without a public email will arrive with userData.email === undefined. That undefined value then flows into users.addOrUpdate() and:
- Skips the email‐based lookup altogether (so new users get created every time if their GitHub IDs don’t match)
- Or, when updating an existing user, spreads
email: undefinedover the existing record (potentially wiping out the stored email or even throwing a Firestore error)
This change is unrelated to renaming “state”→“status” and introduces a critical break in both user‐matching and data integrity. Please restore or replace this fallback (e.g. pull from profile.emails or re-enable the /user/emails call).
Locations to fix:
- controllers/auth.js, lines ~174–188 (the commented-out email fetch block)
| state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | ||
| "string.empty": "state cannot be empty", | ||
| "any.required": "state is required", | ||
| }), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistency between property name and constant name.
The property being validated is still named state while the constant has been renamed to REQUEST_STATUS. Consider renaming this property to status to maintain complete consistency with the PR objective of renaming from "state" to "status".
🏁 Script executed:
#!/bin/bash
# Check if other files follow the pattern of renaming the property
echo "Checking if other files renamed the property from 'state' to 'status'..."
grep -r "status: REQUEST_STATUS" --include="*.ts" --include="*.js" .Length of output: 719
Rename state to status in the request validator
Other parts of the codebase now use status: REQUEST_STATUS, so this validator should be updated for consistency.
• File: middlewares/validators/taskRequests.ts
Lines: 39–42
Diff:
- state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({
- "string.empty": "state cannot be empty",
- "any.required": "state is required",
- }),
+ status: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({
+ "string.empty": "status cannot be empty",
+ "any.required": "status is required",
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | |
| "string.empty": "state cannot be empty", | |
| "any.required": "state is required", | |
| }), | |
| status: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | |
| "string.empty": "status cannot be empty", | |
| "any.required": "status is required", | |
| }), |
| state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | ||
| "string.empty": "state cannot be empty", | ||
| "any.required": "state is required", | ||
| }), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistency between property name and constant name.
The property being validated is still named state while the constant has been renamed to REQUEST_STATUS. To fully achieve the PR objective of renaming "state" to "status", consider renaming this property to status as well.
🏁 Script executed:
#!/bin/bash
# Check if the actual property name needs to be updated
echo "Checking property usage in schema definitions..."
grep -r "state:" --include="*.ts" --include="*.d.ts" .
echo "Checking actual DB models and interfaces..."
grep -r "interface" -A 10 --include="*.d.ts" . | grep -A 10 -B 2 "state[?:]"Length of output: 67220
Rename state to status in the Joi validator
To keep the schema aligned with the REQUEST_STATUS enum and your PR’s goal of renaming “state” to “status,” update the field key and error messages in middlewares/validators/extensionRequestsv2.ts:
• File: middlewares/validators/extensionRequestsv2.ts (around lines 35–38)
- Change the property name from
statetostatus - Update the validation messages accordingly
Suggested diff:
- state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({
- "string.empty": "state cannot be empty",
- "any.required": "state is required",
- }),
+ status: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({
+ "string.empty": "status cannot be empty",
+ "any.required": "status is required",
+ }),• After renaming here, ensure all consumers of body.state (controllers, services, types, tests, fixtures, etc.) are updated to use body.status to avoid breaking changes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | |
| "string.empty": "state cannot be empty", | |
| "any.required": "state is required", | |
| }), | |
| status: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | |
| "string.empty": "status cannot be empty", | |
| "any.required": "status is required", | |
| }), |
| it("should not validate for an invalid extension request on wrong status", async function () { | ||
| const req = { | ||
| body: { ...extensionCreateObject, state: REQUEST_STATE.APPROVED }, | ||
| body: { ...extensionCreateObject, state: REQUEST_STATUS.APPROVED }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Constant reference updated
The test case correctly uses REQUEST_STATUS.APPROVED instead of the previous REQUEST_STATE.APPROVED.
However, I notice that while the constant has been renamed from REQUEST_STATE to REQUEST_STATUS, the property name in the test object is still state instead of status. This appears inconsistent with the PR objective of renaming "state" to "status".
🏁 Script executed:
#!/bin/bash
# Check if there are other instances where property names should be updated
rg -t ts "state: REQUEST_STATUS" --glob "!**/constants/**"Length of output: 4497
🏁 Script executed:
#!/bin/bash
# Check for 'status' property definitions in type declarations
rg -n "status:" --glob "types/**/*.d.ts"Length of output: 423
Rename all state properties to status to match the new constant
The constant rename from REQUEST_STATE → REQUEST_STATUS is correct, but there are still many object literals and type declarations using the old state key. These must be updated to status for consistency and to avoid type mismatches.
Examples needing updates:
- test/unit/middlewares/extensionRequests.test.ts (line 38):
- body: { ...extensionCreateObject, state: REQUEST_STATUS.APPROVED },
- body: { ...extensionCreateObject, status: REQUEST_STATUS.APPROVED },
- test/unit/services/onboardingExtension.test.ts
- test/unit/models/requests.test.ts
- test/integration/requests.test.ts
- test/integration/onboardingExtension.test.ts
- type declarations in:
- types/taskRequests.d.ts
- types/requests.d.ts
- types/onboardingExtension.d.ts
- types/extensionRequests.d.ts
(※ test/fixtures/oooRequest/oooRequest.ts remains on `state` per its dedicated OOO‐PR plan.)
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
`````suggestion
body: { ...extensionCreateObject, status: REQUEST_STATUS.APPROVED },
| "string.empty": "message cannot be empty", | ||
| }), | ||
| state: joi.string().valid(REQUEST_STATE.PENDING).required().messages({ | ||
| state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Constant reference updated but property name remains unchanged
The validation schema now correctly uses REQUEST_STATUS.PENDING instead of the previous REQUEST_STATE.PENDING. However, the property being validated is still named state rather than status, which seems inconsistent with the PR objective of renaming "state" to "status".
Should the property name in the schema also be updated to status? This would require updating:
- The schema definition
- The error message on line 34
- Any dependent code that accesses this property
🏁 Script executed:
#!/bin/bash
# Check for instances of the validation error message mentioning "state"
rg -t ts '"state must be PENDING"'Length of output: 341
Rename the state field to status in the validator and update tests
To fully align with the PR’s intent of renaming “state” → “status,” you’ll need to:
• middlewares/validators/oooRequests.ts
– Change the schema key from state to status
– Update the validation message from "state must be PENDING" to "status must be PENDING"
• test/integration/requests.test.ts
– Update the assertion to expect "status must be PENDING"
• test/unit/middlewares/oooRequests.test.ts
– Update the assertion to expect "status must be PENDING"
• Anywhere else in the codebase that reads or writes request.state, adjust to request.status
Example diff in oooRequests.ts:
- state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({
- "any.only": "state must be PENDING",
+ status: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({
+ "any.only": "status must be PENDING",After making these changes, rerun your test suite to confirm everything passes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | |
| status: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | |
| "any.only": "status must be PENDING", | |
| }), |
| type: REQUEST_TYPE.ONBOARDING; | ||
| message?: string; | ||
| state: REQUEST_STATE.APPROVED | REQUEST_STATE.REJECTED; | ||
| state: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent property naming in type definition
While you've renamed the property from state to status in the OnboardingExtension type (line 15), the property is still named state in the UpdateOnboardingExtensionStateRequestBody type.
For consistency, rename the property in the UpdateOnboardingExtensionStateRequestBody type:
export type UpdateOnboardingExtensionStateRequestBody = {
type: REQUEST_TYPE.ONBOARDING;
message?: string;
- state: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED;
+ status: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED;
};Also, consider renaming the type itself from UpdateOnboardingExtensionStateRequestBody to UpdateOnboardingExtensionStatusRequestBody for complete consistency.
| message?: string; | ||
| requestedBy?: string; | ||
| state?: REQUEST_STATE; | ||
| state?: REQUEST_STATUS; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent property naming in type definitions
The PR objective is to rename the field "state" to "status", but in this file, only the imported constant has been updated from REQUEST_STATE to REQUEST_STATUS while all property names remain as state.
Update all property names from state to status for consistency:
export type ExtensionRequest = {
// ...other properties
- state?: REQUEST_STATUS;
+ status?: REQUEST_STATUS;
// ...other properties
};
export type ExtensionRequestCreateBody = {
// ...other properties
- state: REQUEST_STATUS.PENDING;
+ status: REQUEST_STATUS.PENDING;
// ...other properties
};
export type ExtensionRequestUpdateBody = {
// ...other properties
- state: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED;
+ status: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED;
};
export type RequestQuery = {
// ...other properties
- state?: REQUEST_STATUS.APPROVED | REQUEST_STATUS.PENDING | REQUEST_STATUS.REJECTED;
+ status?: REQUEST_STATUS.APPROVED | REQUEST_STATUS.PENDING | REQUEST_STATUS.REJECTED;
// ...other properties
};Also applies to: 31-31, 41-41, 48-48
| @@ -1,7 +1,7 @@ | |||
| import { REQUEST_STATE } from "./../constants/requests"; | |||
| import { REQUEST_STATUS } from "./../constants/requests"; | |||
There was a problem hiding this comment.
Fix duplicate import of REQUEST_STATUS
The constant REQUEST_STATUS is being imported twice - first on line 1 and again on line 4. This creates a redeclaration error.
-import { REQUEST_STATUS } from "./../constants/requests";
+// Import moved to line 4
import { Request, Response } from "express";
import { Boom } from "express-boom";
import { REQUEST_STATUS, REQUEST_TYPE } from "../constants/requests";Also applies to: 4-4
| userId?: string; | ||
| taskId?: string; | ||
| state: REQUEST_STATE; | ||
| state: REQUEST_STATUS; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Property name 'state' should be renamed to 'status'
While you've updated the constant from REQUEST_STATE to REQUEST_STATUS, the property name state should also be renamed to status for consistency. This matches the PR objectives of renaming "state" to "status" throughout the codebase.
// Line 16
- state: REQUEST_STATUS;
+ status: REQUEST_STATUS;
// Line 31
- state: REQUEST_STATUS.PENDING;
+ status: REQUEST_STATUS.PENDING;
// Line 46
- state: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED;
+ status: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED;
// Line 54
- state?: REQUEST_STATUS.APPROVED | REQUEST_STATUS.PENDING | REQUEST_STATUS.REJECTED;
+ status?: REQUEST_STATUS.APPROVED | REQUEST_STATUS.PENDING | REQUEST_STATUS.REJECTED;Also applies to: 31-31, 46-46, 54-54
🏁 Script executed:
#!/bin/bash
# Check for other usages of "status" property in related files
echo "Checking for 'status' usage in other files:"
rg "status: REQUEST_STATUS" --type tsLength of output: 614
Rename ‘state’ to ‘status’ in TaskRequests definitions
The TaskRequests type in types/taskRequests.d.ts still declares its property as state: REQUEST_STATUS;. To stay consistent with the renamed constant and the rest of the codebase, update all occurrences of this property to status.
Locations to update:
- types/taskRequests.d.ts, line 16
- types/taskRequests.d.ts, line 31
- types/taskRequests.d.ts, line 46
- types/taskRequests.d.ts, line 54
Suggested diff:
--- a/types/taskRequests.d.ts
+++ b/types/taskRequests.d.ts
@@ -16,7 +16,7 @@ export interface TaskRequest {
- state: REQUEST_STATUS;
+ status: REQUEST_STATUS;
@@ -31,7 +31,7 @@ export interface TaskRequestDetails {
- state: REQUEST_STATUS.PENDING;
+ status: REQUEST_STATUS.PENDING;
@@ -46,7 +46,7 @@ export interface TaskRequestUpdate {
- state: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED;
+ status: REQUEST_STATUS.APPROVED | REQUEST_STATUS.REJECTED;
@@ -54,7 +54,7 @@ export interface TaskRequestFilter {
- state?: REQUEST_STATUS.APPROVED | REQUEST_STATUS.PENDING | REQUEST_STATUS.REJECTED;
+ status?: REQUEST_STATUS.APPROVED | REQUEST_STATUS.PENDING | REQUEST_STATUS.REJECTED;| status: userState; | ||
| state?: REQUEST_STATE; | ||
| status?: REQUEST_STATUS; |
There was a problem hiding this comment.
Property 'status' is defined twice with different types
There are two status properties defined on the OooStatusRequest type with different types:
- Line 14:
status: userState - Line 15:
status?: REQUEST_STATUS
This will cause confusion and potential runtime issues. You need to either:
- Rename one of the properties to avoid collision
- Remove one of the properties if it's redundant
status: userState;
- status?: REQUEST_STATUS;
+ requestStatus?: REQUEST_STATUS; // If you need both propertiesAlternatively, if the first status property should be replaced with the REQUEST_STATUS type:
- status: userState;
- status?: REQUEST_STATUS;
+ status: REQUEST_STATUS;
AnujChhikara
left a comment
AnujChhikara
left a comment
There was a problem hiding this comment.
- Test Coverage Missing
- Working Proof Missing
AnujChhikara
left a comment
AnujChhikara
left a comment
There was a problem hiding this comment.
Please fix merge conflicts
still not updated |
2 participants
Date: 29/04/2025
Developer Name: aditya-zende-1
Issue Ticket Number
#2399
Description
We need to rename the state field to status in our backend application, which handles requests like OOO, ONBOARDING, etc. Currently, the state field tracks values like PENDING, APPROVED, and REJECTED. This change will require updates to our database schema, API endpoints, and any related code that references this field. The goal is to ensure consistency and clarity by using status instead of state.
Still, now I have changed a state field to status in the onboarding files only. Test are running ok.
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes