-
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