-
Notifications
You must be signed in to change notification settings - Fork 16
[PROD] - Copilot Portal Fixes & Updates #1158
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: master
Are you sure you want to change the base?
Conversation
PM-732 - Update build script: remove "build" dir before creating a new build
Add status message for no Applications
PM-1315 Fix issues on Copilot request form
fix(PM-1320): Layout issues in copilot opportunity details page
fix(PM-1323, PM-1324): UI issues in opportunities and requests list
PM-1321 Fix alignment issues on copilot form
…tunity-title PM-1498 - support for opportunity title
fix(PM-1395): Show apply as copilot button for a user who is both admin and copilot
fix(PM-1397): notes column
PM-1384 Fix date picker in copilot form
…request PM-1499 edit copilot request
PM-1531 Fix: Trim form values before submit
@@ -141,7 +190,7 @@ const CopilotRequestForm: FC<{}> = () => { | |||
{ condition: !formValues.paymentType, key: 'paymentType', message: 'Selection is required' }, | |||
{ condition: !formValues.projectType, key: 'projectType', message: 'Selecting project type is required' }, | |||
{ | |||
condition: !formValues.overview || formValues.overview.length < 10, | |||
condition: !formValues.overview || formValues.overview.trim().length < 10, |
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.
Consider checking if formValues.overview
is a string before using trim()
to avoid potential runtime errors if overview
is not a string.
@@ -171,7 +220,7 @@ const CopilotRequestForm: FC<{}> = () => { | |||
message: 'Number of weeks should be a positive number', | |||
}, | |||
{ | |||
condition: !formValues.tzRestrictions, | |||
condition: !formValues.tzRestrictions || formValues.tzRestrictions.trim().length === 0, |
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.
The condition formValues.tzRestrictions.trim().length === 0
assumes that formValues.tzRestrictions
is always a string. Consider adding a check to ensure formValues.tzRestrictions
is indeed a string before calling trim()
to avoid potential runtime errors if it is null
or undefined
.
@@ -185,6 +234,11 @@ const CopilotRequestForm: FC<{}> = () => { | |||
key: 'numHoursPerWeek', | |||
message: 'Number of hours per week should be a positive number', | |||
}, | |||
{ | |||
condition: formValues.otherPaymentType && formValues.otherPaymentType.trim().length === 0, |
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.
Consider checking if formValues.otherPaymentType
is defined before calling trim()
to avoid potential runtime errors if otherPaymentType
is null
or undefined
.
fix(PM-1510): Fixed copy for the already user modal
project with ${props.copilotApplication.existingMembership?.role} role.`} | ||
|
||
{ | ||
props.copilotApplication.existingMembership |
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.
Consider using a more descriptive variable name for props.copilotApplication.existingMembership
to improve readability.
|
||
{ | ||
props.copilotApplication.existingMembership | ||
&& ['copilot', 'manager'].includes(props.copilotApplication.existingMembership.role) |
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.
The logic here checks for specific roles. Ensure that the roles are correctly defined and consistent with the application's role management system.
} | ||
|
||
{ | ||
props.copilotApplication.existingMembership |
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.
The logic here checks for specific roles. Ensure that the roles are correctly defined and consistent with the application's role management system.
&& ['observer', 'customer'].includes(props.copilotApplication.existingMembership.role) | ||
&& ( | ||
<div> | ||
Click 'Confirm' to accept by updating project role to 'Copilot' |
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.
Consider rephrasing the message for clarity. For example, 'Click 'Confirm' to update the project role to 'Copilot' and complete this opportunity.'
PM-1532 Fix sorting on title
fix(PM-1506): Show modal text based on existing role
|
||
{ | ||
props.copilotApplication.existingMembership | ||
&& ['observer', 'customer'].includes(props.copilotApplication.existingMembership.role) |
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.
The roles in the condition have been swapped. Ensure that this change is intentional and aligns with the business logic requirements.
|
||
{ | ||
props.copilotApplication.existingMembership | ||
&& ['copilot', 'manager'].includes(props.copilotApplication.existingMembership.role) |
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.
The roles in the condition have been swapped. Verify that this change is correct and meets the intended functionality.
fix(PM-1506): revert back to old code
PM-1518 Fix 429 too many requests error
@@ -11,6 +11,8 @@ const baseUrl = `${EnvironmentConfig.API.V5}/projects` | |||
|
|||
export type ProjectsResponse = SWRResponse<Project[], Project[]> | |||
|
|||
const sleep = (ms: number): Promise<()=> void> => new Promise(resolve => { setTimeout(resolve, ms) }) |
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.
The sleep
function is defined but not used anywhere in the code. Consider removing it if it's not needed, or ensure it's utilized appropriately within the codebase.
const allResults: Project[] = [] | ||
|
||
for (const chunkIds of idChunks) { | ||
// eslint-disable-next-line no-await-in-loop |
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.
Using await
inside a loop can lead to performance issues due to sequential execution. Consider refactoring to use Promise.all
for concurrent execution if possible, while still respecting rate limits.
|
||
// Rate limit: delay 200ms between calls | ||
// eslint-disable-next-line no-await-in-loop | ||
await sleep(200) |
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.
The use of sleep(200)
for rate limiting is not ideal as it can block the event loop. Consider using a more efficient rate limiting strategy, such as a queue or a library designed for rate limiting.
PM-1555 Remove sorting from Payment column
Remove dropdown icon if not more than one in list
@@ -125,7 +125,7 @@ const TabsNavbar: FC<TabsNavbarProps> = (props: TabsNavbarProps) => { | |||
handleActivateTab={handleActivateTab} | |||
handleActivateChildTab={handleActivateChildTab} | |||
/> | |||
<IconOutline.ChevronDownIcon /> | |||
{props.tabs.length > 1 && <IconOutline.ChevronDownIcon />} |
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.
Consider checking if props.tabs
is defined before accessing its length
property to avoid potential runtime errors if tabs
is undefined.
feat(PM-1365): Populate project field in copilot request form when query param presents
// eslint-disable-next-line | ||
const CopilotRequestForm: FC<{}> = () => { | ||
const { profile }: ProfileContextData = useContext(profileContext) | ||
const navigate = useNavigate() | ||
const routeParams: Params<string> = useParams() | ||
const [params] = useSearchParams() |
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.
Consider using a more descriptive name for the params
variable to improve code readability, especially since it is related to search parameters.
// eslint-disable-next-line | ||
const CopilotRequestForm: FC<{}> = () => { | ||
const { profile }: ProfileContextData = useContext(profileContext) | ||
const navigate = useNavigate() | ||
const routeParams: Params<string> = useParams() | ||
const [params] = useSearchParams() | ||
|
||
const [formValues, setFormValues] = useState<any>({}) |
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.
Avoid using any
type for formValues
. Consider defining a specific type or interface for the form data to ensure type safety.
// eslint-disable-next-line | ||
const CopilotRequestForm: FC<{}> = () => { | ||
const { profile }: ProfileContextData = useContext(profileContext) | ||
const navigate = useNavigate() | ||
const routeParams: Params<string> = useParams() | ||
const [params] = useSearchParams() | ||
|
||
const [formValues, setFormValues] = useState<any>({}) | ||
const [isFormChanged, setIsFormChanged] = useState(false) | ||
const [formErrors, setFormErrors] = useState<any>({}) |
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.
Avoid using any
type for formErrors
. Consider defining a specific type or interface for the form errors to ensure type safety.
|
||
const project = await getProject(projectId as string) | ||
|
||
setFormValues((prevValues: any) => ({ |
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.
Consider adding error handling for the getProject
function call to manage potential API errors or network issues.
|
||
setFormValues((prevValues: any) => ({ | ||
...prevValues, | ||
projectId: project.id, |
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.
The projectId
is being set directly from the project
object. Ensure that project
is not null or undefined before accessing project.id
to avoid potential runtime errors.
})) | ||
|
||
return projectFromQuery | ||
? [...projectsFromResponse, { label: projectFromQuery.name, value: projectFromQuery.id }] |
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.
Check for duplicate project entries when adding projectFromQuery
to projectsFromResponse
to prevent redundant options in the dropdown.
@@ -43,6 +58,11 @@ export const useProjects = (search?: string, config?: {isPaused?: () => boolean, | |||
}) | |||
} | |||
|
|||
export const getProject = (projectId: string): Promise<Project> => { |
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.
Consider adding error handling to the getProject
function to manage potential failures from the xhrGetAsync
call.
const url = `${baseUrl}/${projectId}` | ||
return xhrGetAsync<Project>(url) | ||
} | ||
|
||
export const getProjects = (search?: string, filter?: any): Promise<Project[]> => { |
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.
The filter
parameter is typed as any
. Consider using a more specific type to improve type safety and maintainability.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1522
https://topcoder.atlassian.net/browse/PM-1291
What's in this PR?