-
Notifications
You must be signed in to change notification settings - Fork 51
[PROD] - WM Copilot Portal #1662
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-1336): allow MMs to have multiple prizes
feat(PM-1506): Overwrite role if the copilot is already a member of the project with "read"/"write" access
fix(PM-1398): show error when invite comes from an closed opportunity
fix(PM-1365): added projectId query param to request form url
| filters: &filters-dev | ||
| branches: | ||
| only: ["develop", "PM-803_wm-regression-fixes", "PM-902_show-all-projects-on-challenge-page", "pm-1355_1"] | ||
| only: ["develop", "PM-803_wm-regression-fixes", "PM-902_show-all-projects-on-challenge-page", "pm-1365"] |
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 branch name 'pm-1365' should be checked for consistency with the naming conventions used in your project. Ensure that it follows the same pattern as other branch names, such as using uppercase letters or underscores if applicable.
| text='Request Copilot' | ||
| type={'info'} | ||
| url={`${COPILOTS_URL}/requests/new`} | ||
| url={`${COPILOTS_URL}/requests/new?projectId=${activeProject.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 activeProject is always defined and has an id property before using it in the URL. Consider adding validation or error handling to prevent potential runtime errors if activeProject is undefined or does not have an id.
| const [showSelectUserError, setShowSelectUserError] = useState(false) | ||
| const [addUserError, setAddUserError] = useState(null) | ||
| const [isAdding, setIsAdding] = useState(false) | ||
| const [isUserAddingFailed, setUserAddingFailed] = useState(false) |
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 isUserAddingFailed to hasUserAddingFailed for better readability and to follow boolean naming conventions.
| const [addUserError, setAddUserError] = useState(null) | ||
| const [isAdding, setIsAdding] = useState(false) | ||
| const [isUserAddingFailed, setUserAddingFailed] = useState(false) | ||
| const [existingRole, setExistingRole] = useState('') |
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 existingRole state is introduced but not used in the provided diff. Ensure that it is utilized appropriately or remove it if unnecessary.
| }) | ||
| if (failed) { | ||
| const error = get(failed, '0.message', 'User cannot be invited') | ||
| const errorCode = get(failed, '0.error') |
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 failed is an array and has elements before accessing failed[0] to avoid potential runtime errors.
| if (failed) { | ||
| const error = get(failed, '0.message', 'User cannot be invited') | ||
| const errorCode = get(failed, '0.error') | ||
| const role = get(failed, '0.role') |
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 failed is an array and has elements before accessing failed[0] to avoid potential runtime errors.
| const role = get(failed, '0.role') | ||
| setAddUserError(error) | ||
| setIsAdding(false) | ||
| setUserAddingFailed(errorCode === 'ALREADY_MEMBER') |
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 errorCode is always defined before comparing it to 'ALREADY_MEMBER' to prevent unexpected behavior.
| setAddUserError(error) | ||
| setIsAdding(false) | ||
| setUserAddingFailed(errorCode === 'ALREADY_MEMBER') | ||
| setExistingRole(role) |
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 role is always defined before using it to prevent unexpected behavior.
| } | ||
|
|
||
| const onConfirmCopilotRoleChange = async () => { | ||
| const member = projectMembers.find(item => item.userId === userToAdd.userId) |
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 member is undefined before accessing its properties to avoid potential runtime errors.
|
|
||
| const onConfirmCopilotRoleChange = async () => { | ||
| const member = projectMembers.find(item => item.userId === userToAdd.userId) | ||
| const action = member.role === 'manager' ? 'complete-copilot-requests' : '' |
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 logic for determining the action string could be more explicit. Consider using a switch statement or a more descriptive conditional structure for clarity.
| const onConfirmCopilotRoleChange = async () => { | ||
| const member = projectMembers.find(item => item.userId === userToAdd.userId) | ||
| const action = member.role === 'manager' ? 'complete-copilot-requests' : '' | ||
| const response = await updateProjectMemberRole(projectId, member.id, 'copilot', action) |
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.
Handle potential errors from the updateProjectMemberRole function call. Consider using a try-catch block to manage exceptions and provide feedback to the user if the update fails.
| { | ||
| isUserAddingFailed && (['observer', 'customer', 'copilot', 'manager'].includes(existingRole)) && ( | ||
| <div className={cn(styles.contentContainer, styles.confirm)}> | ||
| <div className={styles.textContent}>{`The copilot ${userToAdd.handle} is part of ${projectOption.label} project with ${existingRole} role.`}</div> |
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 userToAdd and projectOption are defined before accessing their properties to prevent potential errors.
| </label> | ||
| {showSelectUserError && ( | ||
| <div className={styles.row}> | ||
| <div className={styles.errorMesssage}>Please select a member.</div> |
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.
There is a typo in the class name styles.errorMesssage. It should be styles.errorMessage.
| onMemberInvited: PropTypes.func.isRequired, | ||
| onClose: PropTypes.func.isRequired | ||
| onClose: PropTypes.func.isRequired, | ||
| projectOption: PropTypes.any.isRequired, |
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 a more precise PropType for projectOption instead of PropTypes.any to improve type safety and maintainability.
| await delay(1000) | ||
| history.push(status === PROJECT_MEMBER_INVITE_STATUS_ACCEPTED ? `/projects/${projectId}/challenges` : '/projects') | ||
| } catch (e) { | ||
| toastr.error('Error', e.response.data.message) |
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 e.response and e.response.data are defined before accessing e.response.data.message to avoid potential runtime errors.
| * @returns {Promise<*>} | ||
| */ | ||
| export async function updateProjectMemberRole (projectId, memberRecordId, newRole) { | ||
| export async function updateProjectMemberRole (projectId, memberRecordId, newRole, action) { |
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 updateProjectMemberRole function signature has been updated to include a new parameter action. Ensure that this change is reflected in all places where this function is called, and that the action parameter is properly handled and validated.
https://topcoder.atlassian.net/browse/PM-555