-
Notifications
You must be signed in to change notification settings - Fork 16
PM-1499 edit copilot request #1148
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
Conversation
const { data: copilotRequestData }: CopilotRequestResponse = useCopilotRequest(routeParams.requestId) | ||
|
||
useEffect(() => { | ||
if (copilotRequestData) { |
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.
Ensure that copilotRequestData
has the expected structure before setting it to formValues
to prevent potential runtime errors.
}, [copilotRequestData]) | ||
|
||
const { data: projects = [] }: ProjectsResponse = useProjects(undefined, { | ||
filter: { id: copilotRequestData?.projectId }, |
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
object is using optional chaining with copilotRequestData?.projectId
. Ensure that the logic correctly handles cases where projectId
is undefined.
|
||
const { data: projects = [] }: ProjectsResponse = useProjects(undefined, { | ||
filter: { id: copilotRequestData?.projectId }, | ||
isPaused: () => !copilotRequestData?.projectId, |
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 isPaused
function uses optional chaining with copilotRequestData?.projectId
. Ensure that the logic correctly handles cases where projectId
is undefined.
@@ -198,9 +235,10 @@ const CopilotRequestForm: FC<{}> = () => { | |||
if (isEmpty(updatedFormErrors)) { | |||
const cleanedFormValues: any = Object.fromEntries( | |||
Object.entries(formValues) | |||
.filter(([, value]) => value !== ''), // Excludes null and undefined | |||
// Excludes null and undefined | |||
.filter(([field, value]) => editableFields.includes(field) && value !== ''), |
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 editableFields
is defined and is an array before using includes
to avoid potential runtime errors.
) | ||
saveCopilotRequest(cleanedFormValues) | ||
saveCopilotRequest({ ...cleanedFormValues, id: copilotRequestData?.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.
Ensure that copilotRequestData?.id
is defined and valid before spreading it into cleanedFormValues
to prevent unintended behavior.
{' '} | ||
{ | ||
copilotRequestData?.id | ||
? 'Use this form to update the copilot request for your 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 simplifying the conditional rendering logic for the message. The current ternary operator with nested strings can be hard to read. You might want to extract the message into a separate variable for clarity.
label='Title' | ||
name='opportunityTitle' | ||
placeholder='Enter a title for the opportunity' | ||
value={formValues.opportunityTitle?.toString()} |
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.
Ensure that formValues.opportunityTitle
is properly initialized to avoid potential undefined
errors when calling toString()
.
name='opportunityTitle' | ||
placeholder='Enter a title for the opportunity' | ||
value={formValues.opportunityTitle?.toString()} | ||
onChange={bind(handleFormValueChange, this, 'opportunityTitle')} |
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 function name instead of handleFormValueChange
to clearly indicate its purpose or the specific form field it handles.
@@ -275,6 +335,7 @@ const CopilotRequestForm: FC<{}> = () => { | |||
dirty | |||
isClearable | |||
error={formErrors.projectId} | |||
options={projectOptions} |
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.
Ensure that projectOptions
is properly defined and populated with the expected data structure before being passed to the Select
component. This will prevent potential runtime errors if projectOptions
is undefined or incorrectly formatted.
@@ -13,3 +13,7 @@ | |||
background: $black-10; | |||
} | |||
} | |||
|
|||
.mrAuto { |
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 renaming the class .mrAuto
to a more descriptive name that reflects its purpose or the element it is applied to, for better readability and maintainability.
@@ -1,4 +1,5 @@ | |||
import { FC, useCallback, useMemo } from 'react' | |||
import { useNavigate } from 'react-router-dom' |
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 useNavigate
hook is imported but not used in the code. Consider removing it if it's not needed to avoid unnecessary imports.
@@ -18,6 +19,11 @@ interface CopilotRequestModalProps { | |||
|
|||
const CopilotRequestModal: FC<CopilotRequestModalProps> = props => { | |||
const confirmModal = useConfirmationModal() | |||
const navigate = useNavigate() | |||
|
|||
const editRequest = useCallback(() => { |
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 editRequest
function is defined but not used within the component. Consider removing it if it's not needed or ensure it is invoked appropriately.
<> | ||
<Button primary onClick={confirmApprove} label='Approve Request' /> | ||
<Button primary variant='danger' onClick={confirmReject} label='Reject Request' /> | ||
<Button primary onClick={editRequest} label='Edit Request' className={styles.mrAuto} /> |
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 className
prop is added to the Edit Request
button. Ensure that styles.mrAuto
is defined in the styles and is necessary for the layout. If not, consider removing it to keep the code clean.
@@ -58,6 +58,10 @@ const CopilotTableActions: FC<{request: CopilotRequest}> = props => { | |||
navigate(copilotRoutesMap.CopilotRequestDetails.replace(':requestId', `${props.request.id}`)) | |||
}, [navigate, props.request.id]) | |||
|
|||
const editRequest = useCallback(() => { |
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 in the editRequest
function to manage potential navigation failures. This will improve the robustness of the function.
@@ -78,6 +82,14 @@ const CopilotTableActions: FC<{request: CopilotRequest}> = props => { | |||
<IconSolid.EyeIcon className='icon-lg' /> | |||
</Tooltip> | |||
</div> | |||
<div className={styles.viewRequestIcon} onClick={editRequest}> |
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 editRequest
function is used here, but it's not clear from the diff if this function is defined or imported correctly. Please ensure that editRequest
is defined and handles the edit action appropriately.
@@ -23,6 +23,7 @@ function copilotRequestFactory(data: any): CopilotRequest { | |||
createdAt: new Date(data.createdAt), | |||
data: undefined, | |||
opportunity: data.copilotOpportunity?.[0], | |||
startDate: new Date(data.data?.startDate), |
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.
Ensure that data.data?.startDate
is a valid date string before passing it to the Date
constructor to avoid potential Invalid Date
issues. Consider adding a check or using a date parsing library for better validation.
@@ -54,8 +55,8 @@ export type CopilotRequestResponse = SWRResponse<CopilotRequest, CopilotRequest> | |||
* @param {string} requestId - The unique identifier of the copilot request. | |||
* @returns {CopilotRequestResponse} - The response containing the copilot request data. | |||
*/ | |||
export const useCopilotRequest = (requestId: string): CopilotRequestResponse => { | |||
const url = buildUrl(`${baseUrl}/copilots/requests/${requestId}`) | |||
export const useCopilotRequest = (requestId?: string): CopilotRequestResponse => { |
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 requestId
parameter is now optional, but the url
variable will be undefined
if requestId
is not provided. Ensure that the rest of the code handles this scenario appropriately to avoid potential runtime errors.
const requestData = { | ||
data: request, | ||
data: { ...request, id: undefined }, |
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 id
property is being set to undefined
in the request data. Ensure this is intentional and does not affect the server-side processing of the request.
} | ||
|
||
return xhrPostAsync(url, requestData, {}) | ||
return request.id ? xhrPatchAsync(url, requestData) : xhrPostAsync(url, requestData, {}) |
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 handling potential errors from xhrPatchAsync
and xhrPostAsync
to ensure that any issues during the request are properly managed and communicated.
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.
Looks good.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1499
What's in this PR?