Skip to content

Conversation

@diwosuwanto
Copy link
Collaborator

Show the Invite Copilot button (in the Manage copilots popup) for the admins #4341
2021-05-06_14-02

@vikasrohit
Copy link

@diwosuwanto @mirzailhami I think the requirement says Should not see the "Invite Copilot" capability at the bottom of the pop-up that means, we do NOT need to show the Invite Copilot feature for the Manager only users (users who are manager but are also admins would continue to see this feature).

@vikasrohit
Copy link

To be 100% clear, we need to hide Invite more copilots section completely for every one except the admins.

@diwosuwanto
Copy link
Collaborator Author

diwosuwanto commented May 6, 2021

@vikasrohit so we need to show invite more copilots section only for this admin role? is it right?
image

@vikasrohit
Copy link

Yes

@mirzailhami
Copy link
Collaborator

mirzailhami commented May 6, 2021

Show the Invite Copilot button (in the Manage copilots popup) for the admins #4341
2021-05-06_14-02

@diwosuwanto Which role are you using here?

@diwosuwanto
Copy link
Collaborator Author

Show the Invite Copilot button (in the Manage copilots popup) for the admins #4341
2021-05-06_14-02

@diwosuwanto Which role are you using here?

@mirzailhami I'm using role in REMOVE_COPILOTS permission in this commit https://github.com/appirio-tech/connect-app/pull/4377/commits/24e50a2ffc9d2ede5b86dfdc8641e9bd3160111e
image

@vikasrohit
Copy link

@diwosuwanto @mirzailhami we need to show the Invite More Copilots only for topcoderRoles and that should match what is there in screenshot attached by @diwosuwanto i.e. TOPCODER_ADMINS and Copilot manager. And please add new permission if does not exist yet for controlling this feature, avoid reusing REMOVE_COPILOT permission as this permission might have different roles.

@diwosuwanto
Copy link
Collaborator Author

@vikasrohit @mirzailhami I already commit the latest update, invite more copilots section will be show only for this Topcoder Admin level roles.
image

Logged in as Manager:
Screenshot from 2021-05-07 13-59-50

Logged in as Admin:
Screenshot from 2021-05-07 13-59-57

Please check it sir, thank you.

@vikasrohit
Copy link

Screenshots looks good to me @diwosuwanto, however, please add ROLE_CONNECT_COPILOT_MANAGER role to the topcoderRoles list for the new permission. This role is specifically for Copilot Managers who are admins as well for this particular operation.

@diwosuwanto
Copy link
Collaborator Author

diwosuwanto commented May 7, 2021

Screenshots looks good to me @diwosuwanto, however, please add ROLE_CONNECT_COPILOT_MANAGER role to the topcoderRoles list for the new permission. This role is specifically for Copilot Managers who are admins as well for this particular operation.

If i add ROLE_CONNECT_COPILOT_MANAGER role to the topcoderRoles, i think i don't need to do pull request or commit new changes sir @vikasrohit, because the Copilot Invitation Permissions already setup in dev branch. Is it right?

https://github.com/appirio-tech/connect-app/blob/d38f65e237782820e5aa6757c8aeee6f3571cca2/src/config/permissions.js#L233-L243

https://github.com/appirio-tech/connect-app/blob/d38f65e237782820e5aa6757c8aeee6f3571cca2/src/components/TeamManagement/CopilotManagementDialog.js#L94

https://github.com/appirio-tech/connect-app/blob/d38f65e237782820e5aa6757c8aeee6f3571cca2/src/components/TeamManagement/CopilotManagementDialog.js#L185-L211

@mirzailhami
Copy link
Collaborator

If i add ROLE_CONNECT_COPILOT_MANAGER role to the topcoderRoles, i think i don't need to do pull request or commit new changes sir @vikasrohit, because the Copilot Invitation Permissions already setup in dev branch. Is it right?

@diwosuwanto You can commit change into you current issue-4341 branch, no need to send PR since this PR not merged yet.

@vikasrohit
Copy link

I got it what you are saying @diwosuwanto and it seems you are right that permission is already there to allow only admin and copilot manager to view the invite more copilots section. Let me come back to you guys after discussing it with issue reporter.

@vikasrohit
Copy link

@mirzailhami Please don't merge the PR until confirmation.

@mirzailhami
Copy link
Collaborator

mirzailhami commented May 7, 2021

@vikasrohit Sure. The variable was already there, that is what confuse @diwosuwanto.

@vikasrohit
Copy link

@mirzailhami not only the variable is there, it seems the changes we described in the original issue are already there, we don't need any thing to change here except the first part of the requirements which is

Update 1
If there are no Copilots on a project, users in the Manager role should:

Only see the "Request Copilot" button.
Not see the "Manage" button.

@mirzailhami mirzailhami self-requested a review May 15, 2021 06:42
@diwosuwanto
Copy link
Collaborator Author

Manager role when no Copilot on the project (Update 1):
image

Manager role when there is a Copilot on the project (Update 2):
image
image

Admin role when no Copilot on the project:
image
image

Admin role when there is a Copilot on the project:
image
image

@mirzailhami
Copy link
Collaborator

Look goods to me for the update 1. Please confirm @vikasrohit, any issues left here?

@mirzailhami mirzailhami merged commit 2d03ead into topcoder-archive:feature/cf-2.20 May 16, 2021
@vikasrohit
Copy link

Seems good to me. Thanks. Lets wrap this one as we don't have to do Update 2 because it is already there.

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