Skip to content

fix(PM-574): show update/add billing address for project manager who belongs to the project #1640

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 1 commit into from
Apr 16, 2025

Conversation

hentrymartin
Copy link
Collaborator

What's in this PR?

  • show update/add billing address for project manager who belongs to the project

@@ -25,7 +25,7 @@ import Loader from '../../Loader'
import UpdateBillingAccount from '../../UpdateBillingAccount'

import { CHALLENGE_STATUS, PAGE_SIZE, PAGINATION_PER_PAGE_OPTIONS, PROJECT_ROLES } from '../../../config/constants'
import { checkAdmin, checkReadOnlyRoles } from '../../../util/tc'
import { checkAdmin, checkManager, checkReadOnlyRoles } from '../../../util/tc'

Choose a reason for hiding this comment

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

The function checkManager is newly imported, but it is not used anywhere in the current diff. Ensure that checkManager is utilized appropriately in the code where necessary, or remove the import if it is not needed.

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

checkManager function usage is introduced here, but ensure that this function is defined and imported correctly to avoid runtime errors.

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

Consider adding a null check for activeProject.members to prevent potential runtime errors if members is undefined.

@@ -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 adding a comment to explain the logic behind the condition (isAdmin || (isManager && isMemberOfActiveProject)) to improve code readability.

@@ -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 adding a test case to ensure that the condition (isAdmin || (isManager && isMemberOfActiveProject)) behaves as expected in different scenarios. This will help prevent any future regressions.

@hentrymartin hentrymartin requested a review from kkartunov April 16, 2025 08:17
Copy link
Contributor

@kkartunov kkartunov left a comment

Choose a reason for hiding this comment

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

Looks good.

@hentrymartin hentrymartin merged commit 3c464c0 into develop Apr 16, 2025
3 checks passed
@hentrymartin hentrymartin deleted the pm-574 branch April 16, 2025 10:58
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