Skip to content

fix(PM-578): QA feedbacks on apply copilot opportunity functionality #1072

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 13, 2025
Merged

fix(PM-578): QA feedbacks on apply copilot opportunity functionality #1072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2a9166
fix: QA feedbacks on apply copilot opportunity functionality
hentrymartin May 12, 2025
9e3d8ed
fix: QA feedbacks on apply copilot opportunity functionality
hentrymartin May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ...s/src/pages/copilot-opportunity-details/apply-opportunity-modal/ApplyOpportunityModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await applyCopilotOpportunity(props.copilotOpportunityId, {
await applyCopilotOpportunity(props.copilotOpportunityId, notes ? {
notes,
})
} : undefined)

props.onApplied()
setSuccess(true)
} catch (e) {
setSuccess(true)
} catch (e: any) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

setSuccess(false)
setError(e.message)
}
}, [props.copilotOpportunityId, notes])

Expand Down Expand Up @@ -77,6 +79,8 @@ const ApplyOpportunityModal: FC<ApplyOpportunityModalProps> = props => {
name='Notes'
onChange={onChange}
value={notes}
error={error}
dirty={!!error}
/>
)
}
Expand Down
33 changes: 29 additions & 4 deletions src/apps/copilots/src/pages/copilot-opportunity-details/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ import {
ButtonProps,
ContentLayout,
IconOutline,
IconSolid,
LoadingSpinner,
PageTitle,
TabsNavbar,
} from '~/libs/ui'
import { profileContext, ProfileContextData, UserRole } from '~/libs/core'
import { textFormatDateLocaleShortString } from '~/libs/shared'

import { CopilotApplication } from '../../models/CopilotApplication'
import {
Expand Down Expand Up @@ -77,8 +79,12 @@ const CopilotOpportunityDetails: FC<{}> = () => {
const [activeTab, setActiveTab]: [string, Dispatch<SetStateAction<string>>] = useState<string>(activeTabHash)

useEffect(() => {
setActiveTab(activeTabHash)
}, [activeTabHash])
if (isAdminOrPM) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a default value for isAdminOrPM to ensure it is defined before use. This will prevent potential runtime errors if isAdminOrPM is undefined.

setActiveTab(activeTabHash)
} else {
setActiveTab('0')
}
}, [activeTabHash, isAdminOrPM])

const handleTabChange = useCallback((tabId: string): void => {
setActiveTab(tabId)
Expand All @@ -97,6 +103,7 @@ const CopilotOpportunityDetails: FC<{}> = () => {

const onApplied: () => void = useCallback(() => {
mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}/applications`)
mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}`)
}, [])

const onCloseApplyModal: () => void = useCallback(() => {
Expand All @@ -117,16 +124,34 @@ const CopilotOpportunityDetails: FC<{}> = () => {
onClick: () => setShowApplyOpportunityModal(true),
}

const application = copilotApplications && copilotApplications[0]

return (
<ContentLayout
title='Copilot Opportunity'
buttonConfig={
isCopilot
&& copilotApplications
&& copilotApplications.length === 0 ? applyCopilotOpportunityButton : undefined
&& copilotApplications.length === 0
&& opportunity?.status === 'active' ? applyCopilotOpportunityButton : undefined
Copy link

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 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&& copilotApplications.length === 0
&& opportunity?.status === 'active'
) && !!application) && (
<div className={styles.applied}>
<IconSolid.CheckCircleIcon className={styles.appliedIcon} />
<span
className={styles.appliedText}
>
{`Applied on ${textFormatDateLocaleShortString(new Date(application.createdAt))}`}
</span>
</div>
)}
>
<PageTitle>Copilot Opportunity</PageTitle>
<PageTitle>
Copilot Opportunity
</PageTitle>
{isValidating && !showNotFound && (
<LoadingSpinner />
) }
Expand Down
16 changes: 16 additions & 0 deletions src/apps/copilots/src/pages/copilot-opportunity-details/styles.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,22 @@
color: $teal-100;
}

.applied {
display: flex;
align-items: center;
.appliedIcon {
width: 30px;
height: 30px;
color: $green-1;
}

.appliedText {
font-size: 14px;
font-weight: 600;
color: #333;
}
}

.textCaps {
text-transform: capitalize;
}
2 changes: 1 addition & 1 deletion src/apps/copilots/src/services/copilot-opportunities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?: {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

notes?: string;
}): Promise<CopilotApplication> => {
const url = `${copilotBaseUrl}/copilots/opportunity/${opportunityId}/apply`
Expand Down
7 changes: 7 additions & 0 deletions src/libs/ui/lib/components/content-layout/ContentLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export interface ContentLayoutProps {
outerClass?: string
title?: string
titleClass?: string
infoComponent?: ReactNode
}

const ContentLayout: FC<ContentLayoutProps> = (props: ContentLayoutProps) => (
Expand Down Expand Up @@ -50,6 +51,12 @@ const ContentLayout: FC<ContentLayoutProps> = (props: ContentLayoutProps) => (
</div>
)}

{!!props.infoComponent && (
<div>
{props.infoComponent}
</div>
)}

</div>
)}

Expand Down
1 change: 0 additions & 1 deletion src/libs/ui/lib/styles/_modals.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@

.react-responsive-modal-closeButton {
top: $sp-5;
padding: $sp-1;
border-radius: 50%;

svg {
Expand Down