Skip to content

[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

Merged
merged 11 commits into from
Apr 24, 2025
Merged

[PROD RELEASE] - BA selection UI exposed to PMs #1641

merged 11 commits into from
Apr 24, 2025

Conversation

kkartunov
Copy link
Contributor

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.

@@ -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)

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)

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)) && (

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)) && (

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,

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

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')

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] || {})

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,

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'))

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)

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.

invitedMembers
invitedMembers: invitedMembers.map(m => ({
...m,
email: m.email || invitedUsers[m.userId].email

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

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('&')}`

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) {

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.

addNewProjectMember(newUserInfo)
onClose()
if (userPermissionToAdd === PROJECT_ROLES.COPILOT) {
const { success: invitations = [], failed, ...rest } = await inviteUserToProject(projectId, {

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')

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

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.

@@ -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}

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.

@github-advanced-security
Copy link
Contributor

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.

@kkartunov kkartunov merged commit 64cfc53 into master Apr 24, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants