fix(PM-578): QA feedbacks on apply copilot opportunity functionality #1072
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| props.onApplied() | ||
| setSuccess(true) | ||
| } catch (e) { | ||
| setSuccess(true) | ||
| } catch (e: any) { |
There was a problem hiding this comment.
The setSuccess(true) call in the catch block seems incorrect. It should likely be setSuccess(false) to indicate the application was not successful due to an error.
| @@ -17,17 +17,19 @@ interface ApplyOpportunityModalProps { | |||
| const ApplyOpportunityModal: FC<ApplyOpportunityModalProps> = props => { | |||
| const [notes, setNotes] = useState('') | |||
| const [success, setSuccess] = useState(false) | |||
| const [error, setError] = useState('') | |||
|
|
|||
| const onApply = useCallback(async () => { | |||
| try { | |||
There was a problem hiding this comment.
Consider adding a type for the e parameter in the catch block instead of using any. This will help with type safety and better error handling.
| @@ -77,8 +79,12 @@ const CopilotOpportunityDetails: FC<{}> = () => { | |||
| const [activeTab, setActiveTab]: [string, Dispatch<SetStateAction<string>>] = useState<string>(activeTabHash) | |||
|
|
|||
| useEffect(() => { | |||
| setActiveTab(activeTabHash) | |||
| }, [activeTabHash]) | |||
| if (isAdminOrPM) { | |||
There was a problem hiding this comment.
Consider adding a default value for isAdminOrPM to ensure it is defined before use. This will prevent potential runtime errors if isAdminOrPM is undefined.
| return ( | ||
| <ContentLayout | ||
| title='Copilot Opportunity' | ||
| buttonConfig={ | ||
| isCopilot | ||
| && copilotApplications | ||
| && copilotApplications.length === 0 ? applyCopilotOpportunityButton : undefined | ||
| && copilotApplications.length === 0 | ||
| && opportunity?.status === 'active' ? applyCopilotOpportunityButton : undefined |
There was a problem hiding this comment.
Consider using a more descriptive variable name for application to improve readability, especially since it represents the first element of copilotApplications. This will make the code easier to understand for other developers.
| } | ||
| infoComponent={(isCopilot && !(copilotApplications |
There was a problem hiding this comment.
The logic for infoComponent is complex and could benefit from being extracted into a separate function or component for better readability and maintainability. This would also help in isolating the logic for easier testing.
| @@ -97,7 +97,7 @@ export const useCopilotOpportunity = (opportunityId?: string): CopilotOpportunit | |||
| * @param request | |||
| * @returns | |||
| */ | |||
| export const applyCopilotOpportunity = async (opportunityId: number, request: { | |||
| export const applyCopilotOpportunity = async (opportunityId: number, request?: { | |||
There was a problem hiding this comment.
The request parameter is now optional, but the function implementation should handle the case where request is undefined. Ensure that the code inside the function checks for request being undefined before accessing its properties.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-578
What's in this PR?