Skip to content

fix(PM-1273): show apply copilot only for projects where user is not a member #1112

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

Merged
merged 4 commits into from
Jun 18, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/apps/copilots/src/copilots.routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export const childRoutes = [
authRequired: true,
element: <CopilotsRequestForm />,
id: 'CopilotRequestForm',
rolesRequired: [UserRole.administrator, UserRole.projectManager] as UserRole[],

Choose a reason for hiding this comment

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

Consider checking if the rolesRequired array should include any other roles that might be relevant for accessing the Copilot Request Form. Ensure that this change aligns with the intended access control requirements.

route: '/requests/new',
},
{
Expand Down
1 change: 1 addition & 0 deletions src/apps/copilots/src/models/CopilotOpportunity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,5 @@ export interface CopilotOpportunity {
startDate: Date,
tzRestrictions: 'yes' | 'no',
createdAt: Date,
members: Array<number>,
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ const CopilotOpportunityDetails: FC<{}> = () => {
}

const application = copilotApplications && copilotApplications[0]
const isAlreadyMemberOfTheProject = profile && opportunity?.members?.includes(profile.userId)

return (
<ContentLayout
Expand All @@ -133,7 +134,8 @@ const CopilotOpportunityDetails: FC<{}> = () => {
isCopilot
&& copilotApplications
&& copilotApplications.length === 0
&& opportunity?.status === 'active' ? applyCopilotOpportunityButton : undefined
&& opportunity?.status === 'active'
&& !isAlreadyMemberOfTheProject ? applyCopilotOpportunityButton : undefined

Choose a reason for hiding this comment

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

Consider renaming isAlreadyMemberOfTheProject to isUserAlreadyMemberOfProject for better clarity and consistency with naming conventions.

}
infoComponent={(isCopilot && !(copilotApplications
&& copilotApplications.length === 0
Expand Down
45 changes: 16 additions & 29 deletions src/apps/copilots/src/pages/copilot-request-form/index.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { FC, useContext, useMemo, useState } from 'react'
import { bind, isEmpty } from 'lodash'
import { bind, debounce, isEmpty } from 'lodash'

Choose a reason for hiding this comment

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

The debounce function is imported from lodash but is not used in the code. Consider removing it if it is not needed.

import { toast } from 'react-toastify'
import classNames from 'classnames'

import { profileContext, ProfileContextData } from '~/libs/core'
import { Button, IconSolid, InputDatePicker, InputMultiselectOption,
InputRadio, InputSelect, InputSelectOption, InputSelectReact, InputText, InputTextarea } from '~/libs/ui'
InputRadio, InputSelect, InputSelectReact, InputText, InputTextarea } from '~/libs/ui'

Choose a reason for hiding this comment

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

The InputSelectOption import has been removed, but it is unclear if this was intentional. Verify if this component is still needed elsewhere in the code.

import { InputSkillSelector } from '~/libs/shared'

import { ProjectsResponse, useProjects } from '../../services/projects'
import { getProjects } from '../../services/projects'

Choose a reason for hiding this comment

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

The useProjects hook has been replaced with getProjects. Ensure that the new implementation correctly handles any asynchronous data fetching and state management that useProjects might have provided.

import { ProjectTypes, ProjectTypeValues } from '../../constants'
import { saveCopilotRequest } from '../../services/copilot-requests'

Expand All @@ -20,20 +20,9 @@ const CopilotRequestForm: FC<{}> = () => {
const [formValues, setFormValues] = useState<any>({})
const [isFormChanged, setIsFormChanged] = useState(false)
const [formErrors, setFormErrors] = useState<any>({})
const [searchTerm, setSearchTerm] = useState<string>('')
const { data: projectsData }: ProjectsResponse = useProjects(searchTerm)
const [existingCopilot, setExistingCopilot] = useState<string>('')
const [paymentType, setPaymentType] = useState<string>('')

const projects = useMemo(
() => (
projectsData
? projectsData.map(project => ({ label: project.name, value: project.id }))
: []
),
[projectsData],
)

const projectTypes = ProjectTypes ? ProjectTypes.map(project => ({
label: project,
value: ProjectTypeValues[project],
Expand Down Expand Up @@ -63,18 +52,12 @@ const CopilotRequestForm: FC<{}> = () => {
setPaymentType(t)
}

function filterProjects(option: InputSelectOption, value: string): boolean {
setSearchTerm(value)
return (
option.label
?.toString()
.toLowerCase()
.includes(value.toLowerCase()) ?? false
)
}

function handleProjectSearch(inputValue: string): void {
setSearchTerm(inputValue)
async function handleProjectSearch(inputValue: string): Promise<Array<{
label: string;
value: string;
}>> {
const response = await getProjects(inputValue)

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 getProjects function call to manage potential API failures or network issues.

return response.map(project => ({ label: project.name, value: project.id }))
}

function handleProjectSelect(option: React.ChangeEvent<HTMLInputElement>): void {
Expand Down Expand Up @@ -268,6 +251,11 @@ const CopilotRequestForm: FC<{}> = () => {
setFormErrors(updatedFormErrors)
}

const debouncedProjectSearch = useMemo(() => debounce((inputValue: string, callback: (options: any[]) => void) => {

Choose a reason for hiding this comment

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

The debounce function is used here, but it is not imported in this file. Ensure that debounce is imported from the appropriate library, such as lodash, to avoid runtime errors.

Choose a reason for hiding this comment

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

Consider specifying the type for the options parameter in the callback function to improve type safety and readability.

handleProjectSearch(inputValue)
.then(callback)
}, 300), [])

return (
<div className={classNames('d-flex flex-column justify-content-center align-items-center', styles.container)}>
<div className={styles.form}>
Expand All @@ -290,15 +278,14 @@ const CopilotRequestForm: FC<{}> = () => {
<p className={styles.formRow}>Select the project you want the copilot for</p>
<InputSelectReact
tabIndex={0}
options={projects}
value={formValues.projectId || ''}
onChange={handleProjectSelect}
onInputChange={handleProjectSearch}
loadOptions={debouncedProjectSearch}

Choose a reason for hiding this comment

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

The async prop should be placed before loadOptions for consistency with other props.

async
name='project'
label='Project'
placeholder='Start typing the name of the project'
dirty
filterOption={filterProjects}
error={formErrors.projectId}
/>
<p className={styles.formRow}>
Expand Down
6 changes: 6 additions & 0 deletions src/apps/copilots/src/services/projects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,9 @@ export const useProjects = (search?: string, config?: {isPaused?: () => boolean,
revalidateOnFocus: false,
})
}

export const getProjects = (search?: string, filter?: any): Promise<Project[]> => {

Choose a reason for hiding this comment

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

Consider specifying a more precise type than any for the filter parameter to improve type safety and code readability.

const params = { name: `"${search}"`, ...filter }
const url = buildUrl(baseUrl, params)

Choose a reason for hiding this comment

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

Ensure that buildUrl correctly handles cases where search might be undefined or an empty string, as this could lead to malformed URLs.

return xhrGetAsync<Project[]>(url)
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {
useRef,
} from 'react'
import { find } from 'lodash'
import AsyncCreatable from 'react-select/async-creatable'

Choose a reason for hiding this comment

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

Consider checking if the newly added imports AsyncCreatable and AsyncSelect from react-select are being used in the component. If they are not used, it would be better to remove them to keep the code clean and avoid unnecessary imports.

import AsyncSelect from 'react-select/async'
import CreatableSelect from 'react-select/creatable'
import ReactSelect, { GroupBase, OptionsOrGroups } from 'react-select'
import classNames from 'classnames'
Expand All @@ -33,7 +35,7 @@ interface InputSelectReactProps {
readonly name: string
readonly onChange: (event: ChangeEvent<HTMLInputElement>) => void
readonly onInputChange?: (newValue: string) => void
readonly options: OptionsOrGroups<unknown, GroupBase<unknown>>
readonly options?: OptionsOrGroups<unknown, GroupBase<unknown>>

Choose a reason for hiding this comment

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

Suggestion

Consider providing a default value or handling the case where options is undefined to prevent potential runtime errors when the component tries to access options without checking its existence.

readonly placeholder?: string
readonly tabIndex?: number
readonly value?: string
Expand All @@ -43,6 +45,8 @@ interface InputSelectReactProps {
readonly onBlur?: (event: FocusEvent<HTMLInputElement>) => void
readonly openMenuOnClick?: boolean
readonly openMenuOnFocus?: boolean
readonly async?: boolean
readonly loadOptions?: (inputValue: string, callback: (option: any) => void) => void

Choose a reason for hiding this comment

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

Consider specifying the type for option in the loadOptions callback function to improve type safety and clarity. Instead of using any, define a specific type or interface that represents the expected structure of option.

readonly filterOption?: (option: InputSelectOption, value: string) => boolean
}

Expand Down Expand Up @@ -120,9 +124,13 @@ const InputSelectReact: FC<InputSelectReactProps> = props => {
} as FocusEvent<HTMLInputElement>)
}

const Input = useMemo(() => (
props.creatable ? CreatableSelect : ReactSelect
), [props.creatable])
const Input = useMemo(() => {
if (props.async) {

Choose a reason for hiding this comment

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

Consider adding a type annotation for the Input variable to ensure type safety and improve code readability.

return props.creatable ? AsyncCreatable : AsyncSelect
}

return props.creatable ? CreatableSelect : ReactSelect
}, [props.creatable, props.async])

return (
<InputWrapper
Expand All @@ -144,6 +152,7 @@ const InputSelectReact: FC<InputSelectReactProps> = props => {
styles.select,
)
}
loadOptions={props.loadOptions}
onChange={handleSelect}
onInputChange={props.onInputChange}
menuPortalTarget={menuPortalTarget}
Expand Down