-
Notifications
You must be signed in to change notification settings - Fork 52
[PROD RELEASE] - BA selection UI exposed to PMs #1641
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
feat(PM-972): copilot invitation with email
fix(PM-574): show update/add billing address for project manager who belongs to the project
@@ -406,6 +406,9 @@ class ChallengeList extends Component { | |||
} = this.props | |||
const isReadOnly = checkReadOnlyRoles(this.props.auth.token) || loginUserRoleInProject === PROJECT_ROLES.READ | |||
const isAdmin = checkAdmin(this.props.auth.token) | |||
const isManager = checkManager(this.props.auth.token) |
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 isManager
to isProjectManager
for clarity, as it specifically checks for the Project Manager role.
@@ -406,6 +406,9 @@ class ChallengeList extends Component { | |||
} = this.props | |||
const isReadOnly = checkReadOnlyRoles(this.props.auth.token) || loginUserRoleInProject === PROJECT_ROLES.READ | |||
const isAdmin = checkAdmin(this.props.auth.token) | |||
const isManager = checkManager(this.props.auth.token) | |||
const loginUserId = this.props.auth.user.userId | |||
const isMemberOfActiveProject = activeProject && activeProject.members && activeProject.members.some(member => member.userId === loginUserId) |
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 variable isMemberOfActiveProject
could be more descriptive. Consider renaming it to userIsMemberOfActiveProject
to clearly indicate that it refers to the current user.
@@ -129,7 +131,7 @@ const UpdateBillingAccount = ({ | |||
!currentBillingAccount && ( | |||
<Fragment> | |||
<span className={styles.error}>No Billing Account set</span> | |||
{isAdmin && ( | |||
{(isAdmin || (isManager && isMemberOfActiveProject)) && ( |
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 using a more descriptive variable name for isManager
to clarify its purpose, especially if it refers specifically to a Project Manager role. This can improve code readability and maintainability.
@@ -153,7 +155,7 @@ const UpdateBillingAccount = ({ | |||
> | |||
{isBillingAccountExpired ? 'INACTIVE' : 'ACTIVE'} | |||
</span>{' '} | |||
{isAdmin && ( | |||
{(isAdmin || (isManager && isMemberOfActiveProject)) && ( |
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 extracting the condition (isAdmin || (isManager && isMemberOfActiveProject))
into a well-named variable or function to improve readability and maintainability of the code.
@@ -187,7 +189,9 @@ UpdateBillingAccount.propTypes = { | |||
isBillingAccountExpired: PropTypes.bool, | |||
isAdmin: PropTypes.bool, | |||
projectId: PropTypes.number, | |||
updateProject: PropTypes.func.isRequired | |||
updateProject: PropTypes.func.isRequired, | |||
isMemberOfActiveProject: PropTypes.bool.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.
The prop type isMemberOfActiveProject
is defined as PropTypes.bool.isRequired
, but it's not clear from the context if this prop is being used in the component. Please ensure that this prop is utilized within the component logic.
updateProject: PropTypes.func.isRequired | ||
updateProject: PropTypes.func.isRequired, | ||
isMemberOfActiveProject: PropTypes.bool.isRequired, | ||
isManager: PropTypes.bool.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.
The prop type isManager
is defined as PropTypes.bool.isRequired
, but it's not clear from the context if this prop is being used in the component. Please ensure that this prop is utilized within the component logic.
role: userPermissionToAdd | ||
}) | ||
if (failed) { | ||
const error = get(failed, '0.message', 'Unable to invite user') |
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 using a more descriptive error message or logging additional details for debugging purposes when setting the error message for a failed invitation.
setAddUserError(error) | ||
setIsAdding(false) | ||
} else { | ||
onMemberInvited(invitations[0] || {}) |
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 invitations[0]
is not undefined before accessing it to avoid potential runtime errors. Consider adding a check or default value.
@@ -169,6 +184,7 @@ const UserAddModalContent = ({ projectId, addNewProjectMember, onClose }) => { | |||
UserAddModalContent.propTypes = { | |||
projectId: PropTypes.number.isRequired, | |||
addNewProjectMember: PropTypes.func.isRequired, | |||
onMemberInvited: PropTypes.func.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.
The new prop onMemberInvited
is marked as required, but there is no context provided in this diff about its usage or necessity. Ensure that this prop is indeed required and used appropriately within the component. If it's not used or optional, consider updating the prop type accordingly.
const projectMembers = _.get(project, 'members') | ||
const invitedMembers = _.get(project, 'invites') | ||
const invitedUserIds = _.filter(_.map(invitedMembers, '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 invitedMembers
is not null or undefined before mapping over it to avoid potential runtime errors.
const projectMembers = _.get(project, 'members') | ||
const invitedMembers = _.get(project, 'invites') | ||
const invitedUserIds = _.filter(_.map(invitedMembers, 'userId')) | ||
const invitedUsers = await fetchInviteMembers(invitedUserIds) |
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 fetchInviteMembers
handles cases where invitedUserIds
might be an empty array, as this could lead to unnecessary API calls or errors.
src/containers/Users/index.js
Outdated
invitedMembers | ||
invitedMembers: invitedMembers.map(m => ({ | ||
...m, | ||
email: m.email || invitedUsers[m.userId].email |
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.
Add a check to ensure invitedUsers[m.userId]
exists before trying to access .email
to prevent potential errors if the user is not found.
@@ -68,6 +69,19 @@ export async function fetchProjectById (id) { | |||
return _.get(response, 'data') | |||
} | |||
|
|||
/** | |||
* This fetches the user corresponding to the given userIds | |||
* @param {*} userIds |
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 parameter userIds
is typed as *
, which is not specific. Consider specifying the expected type, such as Array
or Array<number>
, to improve code readability and maintainability.
* @param {*} userIds | ||
*/ | ||
export async function fetchInviteMembers (userIds) { | ||
const url = `${MEMBERS_API_URL}?${userIds.map(id => `userIds[]=${id}`).join('&')}` |
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 adding error handling for the API request to ensure that any network or server errors are properly managed and do not cause the application to crash.
emails: [email], | ||
role: role | ||
}) | ||
export async function inviteUserToProject (projectId, params) { |
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 function inviteUserToProject
now accepts params
instead of email
and role
separately. Ensure that the params
object is validated properly before being passed to createProjectMemberInvite
to prevent potential issues with missing or malformed data.
fix(PM-1077): handled errors for certain use cases
addNewProjectMember(newUserInfo) | ||
onClose() | ||
if (userPermissionToAdd === PROJECT_ROLES.COPILOT) { | ||
const { success: invitations = [], failed, ...rest } = await inviteUserToProject(projectId, { |
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 rest.message
is a string before using it in the condition on line 57 to ensure that it doesn't cause unexpected behavior if rest.message
is not a string.
role: userPermissionToAdd | ||
}) | ||
if (failed) { | ||
const error = get(failed, '0.message', 'User cannot be invited') |
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 error message has been changed from 'Unable to invite user' to 'User cannot be invited'. Ensure that this change aligns with the user experience and error messaging guidelines.
invitedMembers | ||
invitedMembers: invitedMembers.map(m => ({ | ||
...m, | ||
email: m.email || invitedUsers[m.userId].handle |
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 change from email
to handle
might affect how invited members are identified or displayed. Ensure that this change aligns with the intended functionality and that handle
provides the necessary information for the 'Select Billing Account' feature.
fix(PM-1077): show invite handle if email doesn't exist in invite
@@ -91,7 +91,7 @@ class UserCard extends Component { | |||
)} | |||
<div className={styles.item}> | |||
<div className={cn(styles.col5)}> | |||
{isInvite ? user.email : user.handle} | |||
{isInvite ? (user.email || user.handle) : user.handle} |
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 change introduces a fallback to user.handle
if user.email
is not available when isInvite
is true. Consider checking if both user.email
and user.handle
are defined before using them to prevent potential issues with undefined values. For example, you could use user.email ? user.email : user.handle
to ensure that user.handle
is only used when user.email
is not available.
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Sync Master into develop
As per https://topcoder.atlassian.net/browse/PM-1093
In Work Manager, the ‘Select Billing Account' feature to be available for 'Project Manager’ role in addition to the Admin role.