-
Notifications
You must be signed in to change notification settings - Fork 273
fix: change state to status #2407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 1 commit
df7cee5
640635c
c92a7ac
db5e50c
af83196
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,5 +124,4 @@ dist | |
|
||
# Local config file | ||
config/local.js | ||
|
||
package-lock.json |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import { | |
REQUEST_APPROVED_SUCCESSFULLY, | ||
REQUEST_LOG_TYPE, | ||
REQUEST_REJECTED_SUCCESSFULLY, | ||
REQUEST_STATE, | ||
REQUEST_STATUS, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Property naming inconsistency with PR intent The PR objective is to rename the field "state" to "status", but in this file, you've updated the constant from Update all property references from -if (latestExtensionRequest && latestExtensionRequest.state === REQUEST_STATUS.PENDING) {
+if (latestExtensionRequest && latestExtensionRequest.status === REQUEST_STATUS.PENDING) { -requestResult.state === REQUEST_STATUS.APPROVED
+requestResult.status === REQUEST_STATUS.APPROVED Also applies to: 67-67, 120-120 |
||
REQUEST_TYPE, | ||
} from "../constants/requests"; | ||
import { addLog } from "../models/logs"; | ||
|
@@ -64,7 +64,7 @@ export const createTaskExtensionRequest = async (req: ExtensionRequestRequest, r | |
type: REQUEST_TYPE.EXTENSION, | ||
}); | ||
|
||
if (latestExtensionRequest && latestExtensionRequest.state === REQUEST_STATE.PENDING) { | ||
if (latestExtensionRequest && latestExtensionRequest.state === REQUEST_STATUS.PENDING) { | ||
return res.boom.badRequest("An extension request for this task already exists."); | ||
} | ||
const requestNumber: number = | ||
|
@@ -117,7 +117,7 @@ export const updateTaskExtensionRequest = async (req: any, res: any) => { | |
return res.boom.badRequest(requestResult.error); | ||
} | ||
const [logType, returnMessage] = | ||
requestResult.state === REQUEST_STATE.APPROVED | ||
requestResult.state === REQUEST_STATUS.APPROVED | ||
? [REQUEST_LOG_TYPE.REQUEST_APPROVED, REQUEST_APPROVED_SUCCESSFULLY] | ||
: [REQUEST_LOG_TYPE.REQUEST_REJECTED, REQUEST_REJECTED_SUCCESSFULLY]; | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,7 +1,7 @@ | ||||||||||||||||||
import joi from "joi"; | ||||||||||||||||||
import { ExtensionRequestRequest, ExtensionRequestResponse } from "../../types/extensionRequests"; | ||||||||||||||||||
import { NextFunction } from "express"; | ||||||||||||||||||
import { REQUEST_TYPE,REQUEST_STATE } from "../../constants/requests"; | ||||||||||||||||||
import { REQUEST_TYPE,REQUEST_STATUS } from "../../constants/requests"; | ||||||||||||||||||
|
||||||||||||||||||
export const createExtensionRequestValidator = async ( | ||||||||||||||||||
req: ExtensionRequestRequest, | ||||||||||||||||||
|
@@ -32,7 +32,7 @@ export const createExtensionRequestValidator = async ( | |||||||||||||||||
message: joi.string().required().messages({ | ||||||||||||||||||
"string.empty": "message cannot be empty", | ||||||||||||||||||
}), | ||||||||||||||||||
state: joi.string().valid(REQUEST_STATE.PENDING).required().messages({ | ||||||||||||||||||
state: joi.string().valid(REQUEST_STATUS.PENDING).required().messages({ | ||||||||||||||||||
"string.empty": "state cannot be empty", | ||||||||||||||||||
"any.required": "state is required", | ||||||||||||||||||
}), | ||||||||||||||||||
|
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", | |
}), |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,6 @@ | ||||||||||
import joi from "joi"; | ||||||||||
import { NextFunction } from "express"; | ||||||||||
import { REQUEST_STATE, REQUEST_TYPE } from "../../constants/requests"; | ||||||||||
import { REQUEST_STATUS, REQUEST_TYPE } from "../../constants/requests"; | ||||||||||
import { OooRequestCreateRequest, OooRequestResponse } from "../../types/oooRequest"; | ||||||||||
|
||||||||||
export const createOooStatusRequestValidator = async ( | ||||||||||
|
@@ -30,7 +30,7 @@ export const createOooStatusRequestValidator = async ( | |||||||||
"any.required": "message is required", | ||||||||||
"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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainConstant reference updated but property name remains unchanged The validation schema now correctly uses Should the property name in the schema also be updated to
🏁 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 To fully align with the PR’s intent of renaming “state” → “status,” you’ll need to: • middlewares/validators/oooRequests.ts • test/integration/requests.test.ts • test/unit/middlewares/oooRequests.test.ts • Anywhere else in the codebase that reads or writes 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
Suggested change
|
||||||||||
"any.only": "state must be PENDING", | ||||||||||
}), | ||||||||||
type: joi.string().valid(REQUEST_TYPE.OOO).required(), | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,7 +1,7 @@ | ||||||||||||||||||
import joi from "joi"; | ||||||||||||||||||
import { TaskRequestResponse, TaskRequestRequest } from "../../types/taskRequests"; | ||||||||||||||||||
import { NextFunction } from "express"; | ||||||||||||||||||
import { REQUEST_TYPE, REQUEST_STATE } from "../../constants/requests"; | ||||||||||||||||||
import { REQUEST_TYPE, REQUEST_STATUS } from "../../constants/requests"; | ||||||||||||||||||
import { GITHUB_URL } from "../../constants/urls"; | ||||||||||||||||||
|
||||||||||||||||||
import config from "config"; | ||||||||||||||||||
|
@@ -36,7 +36,7 @@ export const createTaskRequestValidator = async ( | |||||||||||||||||
"string.empty": "type cannot be empty", | ||||||||||||||||||
"any.required": "type is required", | ||||||||||||||||||
}), | ||||||||||||||||||
state: joi.string().valid(REQUEST_STATE.PENDING).required().messages({ | ||||||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainInconsistency between property name and constant name. The property being validated is still named 🏁 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 Other parts of the codebase now use • File: middlewares/validators/taskRequests.ts 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
Suggested change
|
||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import firestore from "../utils/firestore"; | ||
const requestModel = firestore.collection("requests"); | ||
import { REQUEST_ALREADY_APPROVED, REQUEST_ALREADY_REJECTED, REQUEST_STATE } from "../constants/requests"; | ||
import { REQUEST_ALREADY_APPROVED, REQUEST_ALREADY_REJECTED, REQUEST_STATUS } from "../constants/requests"; | ||
import { | ||
ERROR_WHILE_FETCHING_REQUEST, | ||
ERROR_WHILE_CREATING_REQUEST, | ||
|
@@ -37,12 +37,12 @@ export const updateRequest = async (id: string, body: any, lastModifiedBy: strin | |
error: REQUEST_DOES_NOT_EXIST, | ||
}; | ||
} | ||
if (existingRequestDoc.data().state === REQUEST_STATE.APPROVED) { | ||
if (existingRequestDoc.data().state === REQUEST_STATUS.APPROVED) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainConstant references updated but property name remains unchanged The condition checks now correctly use If the field name is being changed from "state" to "status", these lines should be updated to: -if (existingRequestDoc.data().state === REQUEST_STATUS.APPROVED) {
+if (existingRequestDoc.data().status === REQUEST_STATUS.APPROVED) { Also, note that line 73 destructures a query parameter named Also applies to: 45-45 🏁 Script executed: #!/bin/bash
# Check for other instances where this property is accessed
rg -t ts "data\(\)\.state" Length of output: 208 🏁 Script executed: #!/bin/bash
# Display lines 50-120 to locate the query parameter destructuring at line ~73
sed -n '50,120p' models/requests.ts || echo "models/requests.ts not found" Length of output: 1810 Rename The PR updated constant names to • In the success-flow checks (around line 40–45): -if (existingRequestDoc.data().state === REQUEST_STATUS.APPROVED) {
+if (existingRequestDoc.data().status === REQUEST_STATUS.APPROVED) {
…
-if (existingRequestDoc.data().state === REQUEST_STATUS.REJECTED) {
+if (existingRequestDoc.data().status === REQUEST_STATUS.REJECTED) { • In the query handler destructuring (around line 70): -let { id, type, requestedBy, state, prev, next, page, size = SIZE } = query;
+let { id, type, requestedBy, status, prev, next, page, size = SIZE } = query; • In the paginated‐query filter that follows: -if (state) {
- requestQuery = requestQuery.where("state", "==", state);
-}
+if (status) {
+ requestQuery = requestQuery.where("status", "==", status);
+} After renaming, verify that Firestore documents and any related tests are updated to use |
||
return { | ||
error: REQUEST_ALREADY_APPROVED, | ||
}; | ||
} | ||
if (existingRequestDoc.data().state === REQUEST_STATE.REJECTED) { | ||
if (existingRequestDoc.data().state === REQUEST_STATUS.REJECTED) { | ||
return { | ||
error: REQUEST_ALREADY_REJECTED, | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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 tousers.addOrUpdate()
on line 190.🏁 Script executed:
Length of output: 391
I want to confirm whether
addOrUpdate
enforces or uses theemail
field. Let’s pull in its full implementation and see any email-related logic:🏁 Script executed:
Length of output: 7631
To confirm how
addOrUpdate
is invoked and how an undefined email propagates, let’s locate its usage incontrollers/auth.js
and inspect the surrounding logic:🏁 Script executed:
Length of output: 2025
Let’s inspect how
userData
is initialized in the GitHub callback to see ifemail
is ever set:🏁 Script executed:
Length of output: 861
Let’s inspect the entire GitHub callback handler (lines 130–260) to see how
userData
is constructed:🏁 Script executed:
Length 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 withuserData.email === undefined
. That undefined value then flows intousers.addOrUpdate()
and:email: undefined
over 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: