Skip to content

Conversation

@diwosuwanto
Copy link
Collaborator


<div styleName="col hide-md">
{status && (isSimplePlan || status !== PHASE_STATUS_ACTIVE) && !this.props.currentUserRoles.includes(PROJECT_ROLE_CUSTOMER) &&
{status && (isSimplePlan || status !== PHASE_STATUS_ACTIVE) && !this.props.currentUserRoles.includes(PROJECT_ROLE_CUSTOMER) && this.props.currentUserRoles.length > 2 &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vikasrohit I dont think this solution is good enough. The currentUserRoles sometime has 2 and 3 values.

@diwosuwanto account
WhatsApp Image 2021-04-16 at 13 37 00

@mirzailhami account
WhatsApp Image 2021-04-16 at 13 43 10

Any suggestion @vikasrohit?

Choose a reason for hiding this comment

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

You are right @mirzailhami You don't need to check currentUserRoles. We have to check project role of the logged in user because a user can not see without joining a project (unless he/she is admin). And best way to achieve this is following our new permission model. Example code here https://github.com/appirio-tech/connect-app/blob/7d640885c00323ebef298f022a079384decdb8af/src/projects/detail/containers/DashboardContainer.jsx#L100 and https://github.com/appirio-tech/connect-app/blob/7d640885c00323ebef298f022a079384decdb8af/src/config/permissions.js#L178

Copy link
Collaborator

Choose a reason for hiding this comment

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

@diwosuwanto Please look into the code reference & suggestion above. You can commit the update, then. Thanks.

Copy link
Collaborator Author

@diwosuwanto diwosuwanto Apr 20, 2021

Choose a reason for hiding this comment

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

Thank you for the feedback sir @mirzailhami @vikasrohit, i already commit the update. Please check my commit, thank you.

Copy link

@vikasrohit vikasrohit Apr 21, 2021

Choose a reason for hiding this comment

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

@mirzailhami @diwosuwanto We need to create new permission instead of reusing EXPAND_ACTIVE_PHASES_BY_DEFAULT. Lets call it SHOW_PHASE_STATUS

@mirzailhami mirzailhami merged commit cc2de86 into topcoder-archive:feature/project-plan-simplification-0 Apr 20, 2021
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.

3 participants