-
Notifications
You must be signed in to change notification settings - Fork 15
[PROD RELEASE] - Copilot Portal #1117
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
fix(PM-1273): remove members from copilot response and use canApplyAsCopilot
fix(PM-1310): show types propertly in opportunity details page
fix(PM-1373, PM-1375, PM-1376, PM-1383, PM-1380, PM-1378): demo feedbacks implementation
@@ -22,5 +22,5 @@ export interface CopilotOpportunity { | |||
startDate: Date, | |||
tzRestrictions: 'yes' | 'no', | |||
createdAt: Date, | |||
members: Array<number>, | |||
canApplyAsCopilot: boolean, |
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 type of the members
property has been changed to canApplyAsCopilot
, which is a boolean. Ensure that this change is intentional and that all references to members
in the codebase are updated accordingly to prevent any runtime errors.
: `We're excited to see your interest in joining our team as a copilot | ||
for the ${props.projectName} project! Before we proceed, we want to | ||
for the "${props.projectName}" project! Before we proceed, we want to |
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 consistent quotation marks for the project name. Either use single quotes or double quotes throughout the codebase for consistency.
} | ||
</div> | ||
{ | ||
!success && ( | ||
<InputTextarea | ||
name='Notes' | ||
name='Reason' |
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 name attribute has been changed from 'Notes' to 'Reason'. Ensure that this change is reflected wherever this component is used or referenced to avoid potential issues.
@@ -12,6 +12,7 @@ import { | |||
} from 'react' | |||
import { useLocation, useNavigate, useParams } from 'react-router-dom' | |||
import { mutate } from 'swr' | |||
import { toast } from 'react-toastify' |
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 verifying if toast
is used in the component. If not, the import might be unnecessary and could be removed to keep the code clean.
@@ -28,13 +29,15 @@ import { textFormatDateLocaleShortString } from '~/libs/shared' | |||
|
|||
import { CopilotApplication } from '../../models/CopilotApplication' | |||
import { | |||
cancelCopilotOpportunity, |
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 cancelCopilotOpportunity
import is added but not used in the current code. Consider removing it if it's unnecessary to avoid unused imports.
@@ -103,7 +106,7 @@ const CopilotOpportunityDetails: FC<{}> = () => { | |||
|
|||
const onApplied: () => void = useCallback(() => { | |||
mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}/applications`) | |||
mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}`) | |||
mutate(`${copilotBaseUrl}/copilot/opportunity/${opportunityId}`) |
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 URL path has been changed from /copilots/opportunity/
to /copilot/opportunity/
. Ensure that this change is intentional and that the new endpoint is correct and available, as this could affect the data fetching logic.
case 'datascience': | ||
return ProjectType.datascience | ||
case 'dev': | ||
return ProjectType.developement |
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.
Typo in 'developement'. It should be 'development'.
@@ -135,7 +167,11 @@ const CopilotOpportunityDetails: FC<{}> = () => { | |||
&& copilotApplications | |||
&& copilotApplications.length === 0 | |||
&& opportunity?.status === 'active' | |||
&& !isAlreadyMemberOfTheProject ? applyCopilotOpportunityButton : undefined | |||
&& opportunity?.canApplyAsCopilot ? applyCopilotOpportunityButton : 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.
Consider adding a null check for opportunity
before accessing canApplyAsCopilot
to prevent potential runtime errors if opportunity
is null or undefined.
&& opportunity?.canApplyAsCopilot ? applyCopilotOpportunityButton : undefined | ||
} | ||
secondaryButtonConfig={ | ||
opportunity?.status === 'active' |
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 a null check for opportunity
before accessing status
to prevent potential runtime errors if opportunity
is null or undefined.
<span | ||
className={styles.infoValue} | ||
> | ||
{opportunity?.type && getOpportunityType(opportunity?.type)} |
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 consistent approach for rendering the opportunity?.type
. The function getOpportunityType
is used here, but it's not clear if this is necessary or if it should be applied elsewhere. Ensure consistency across similar data rendering.
@@ -15,7 +15,8 @@ const CopilotApplicationAction = ( | |||
): JSX.Element => { | |||
const { opportunityId }: {opportunityId?: string} = useParams<{ opportunityId?: string }>() | |||
const isInvited = useMemo( | |||
() => allCopilotApplications.findIndex(item => item.status === CopilotApplicationStatus.INVITED) > -1, | |||
() => allCopilotApplications |
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 optional chaining (?.
) to safely access allCopilotApplications
instead of using a logical AND (&&
). This can make the code more concise and readable: allCopilotApplications?.findIndex(item => item.status === CopilotApplicationStatus.INVITED) > -1
.
{skill.name} | ||
</div> | ||
))} | ||
{props.opportunity?.skills.map(item => item.name) |
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 instead of item
for better readability, such as skill
.
</div> | ||
))} | ||
{props.opportunity?.skills.map(item => item.name) | ||
.join(',')} |
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 current implementation joins skill names into a single string separated by commas. If the intention is to display skills as a list, consider using a list or similar structure to maintain the previous visual separation.
@@ -141,12 +130,6 @@ const CopilotRequestForm: FC<{}> = () => { | |||
|
|||
const fieldValidations: { condition: boolean; key: string; message: string }[] = [ | |||
{ condition: !formValues.projectId, key: 'projectId', message: 'Project is required' }, |
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 removal of the validation for existingCopilot
and copilotUsername
might lead to missing checks for these fields. Ensure that these validations are not required for the current functionality or that they are handled elsewhere.
@@ -220,7 +203,6 @@ const CopilotRequestForm: FC<{}> = () => { | |||
toast.success('Copilot request sent successfully') | |||
setFormValues({ | |||
complexity: '', |
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 removal of copilotUsername
from setFormValues
might lead to issues if this field is expected to be reset. Ensure that this change is intentional and that copilotUsername
is not required to be reset in this context.
*/ | ||
export const cancelCopilotOpportunity = async ( | ||
opportunityId: string, | ||
): Promise<{applicationId: number}> => { |
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 specifying the expected structure of the response from xhrDeleteAsync
in the return type of the function. Currently, it is defined as Promise<{applicationId: number}>
, but the implementation does not indicate that the response will contain an applicationId
. Ensure that the return type accurately reflects the actual response structure.
fix(PM-1373): sorting for duration column
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-555
What's in this PR?