Skip to content

Show PR actions only to users with push access (fixes #967)#1404

Merged
RMacfarlane merged 1 commit intomicrosoft:masterfrom
IllusionMH:pr-actions-only-with-merge-permission
Oct 25, 2019
Merged

Show PR actions only to users with push access (fixes #967)#1404
RMacfarlane merged 1 commit intomicrosoft:masterfrom
IllusionMH:pr-actions-only-with-merge-permission

Conversation

@IllusionMH
Copy link
Contributor

@IllusionMH IllusionMH commented Oct 24, 2019

This PR changes visibility rules for PR actions (Merge, Mark as ready for review) which were previously available to all users.

  • "Merge Pull Request" button, merge method dropdown, and "Close Pull Request" button only available to users with push permission

  • "Ready for review" button only available for PR author and users with push permission

With canMerge in place it is now possible to change visibility of more controls - edit title/description #1065 and add/remove reviewers and labels (not sure if there is tracking issue).
Should I change visibility rules for these places too or in separate PR?

@RMacfarlane
Copy link
Contributor

RMacfarlane commented Oct 24, 2019

Really nice!

Yeah, let's handle the other visibility changes in a different PR. (Currently, there is no issue for hiding the add/remove buttons for reviewers/labels UI when there aren't sufficient permissions.)

Would it make more sense to name canMerge as canPush? I suppose in either case there needs to be some explanations in comments about what that permission grants

@IllusionMH
Copy link
Contributor Author

IllusionMH commented Oct 24, 2019

I had several options for name, however this PR was limited to scope of Merge controls so I proceed with canMerge which is similar to existed canEdit.
However may be hasWriteAccess be more suitable that canPush?
And about comments - it will be enough to add JSDoc comment in cache.ts or what places do you have in mind?
image

@RMacfarlane
Copy link
Contributor

RMacfarlane commented Oct 24, 2019

That makes sense. My preference is forhasWriteAccess, I think that is the clearest descriptor of what it is. Adding the JSDoc comment in cache.ts should be enough

@IllusionMH IllusionMH force-pushed the pr-actions-only-with-merge-permission branch from 740f0ea to 77e7099 Compare October 24, 2019 23:33
@IllusionMH IllusionMH mentioned this pull request Oct 25, 2019
Closed
RMacfarlane
RMacfarlane approved these changes Oct 25, 2019
View reviewed changes
Copy link
Contributor

@RMacfarlane RMacfarlane left a comment

Choose a reason for hiding this comment

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

Thank you!

@RMacfarlane RMacfarlane merged commit 1f89017 into microsoft:master Oct 25, 2019
@IllusionMH IllusionMH deleted the pr-actions-only-with-merge-permission branch November 1, 2019 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@RMacfarlane RMacfarlane RMacfarlane approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@IllusionMH @RMacfarlane