Skip to content
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

Validate PR review state before allowing submission. #1582

Merged

Conversation

@grokys
Copy link
Contributor

@grokys grokys commented Apr 4, 2018

Split the PullRequestReviewAuthoringViewModel.Submit command into 2 separate commands and add a CanExecute observable for them:

  • Approve can always be submitted
  • Comment needs either a review body or >0 review comments
  • RequestChanges needs either a review body or >0 review comments

HACK

Disabling the GitHubActionLinks that are used for the approve/comment/request changes buttons had no visual effect. Seems that hyperlinks don't respect dependency property precedence, so the IsEnabled=False trigger for Hyperlink.Foreground doesn't override the foreground TemplateBinding. Not sure what we can do here other than set the control's opacity when disabled, so that's what I did for now.

Depends on #1581
Part of #1491

Split the `PullRequestReviewAuthoringViewModel.Submit` command into 2 separate commands and add a `CanExecute` observable for them:

- `Approve` can always be submitted
- `Comment` needs either a review body or >0 review comments
- `RequestChanges` always needs a review body
@grokys grokys changed the base branch from master to feature/pr-reviews-master Apr 4, 2018
@meaghanlewis
Copy link
Contributor

@meaghanlewis meaghanlewis commented Apr 5, 2018

This LGTM - except I thought that RequestChanges option needs to be either have a review body or > 0 review comments to submit.

@grokys
Copy link
Contributor Author

@grokys grokys commented Apr 5, 2018

@meaghanlewis oh, yes you're right! Fixed that.

Sigh. Seems that hyperlinks don't respect dependency property precedence, so the `IsEnabled=False` trigger for `Hyperlink.Foreground` doesn't override the foreground `TemplateBinding`. Not sure what we can do here other than set the control's opacity when disabled, so that's what we'll do for now.
@jcansdale jcansdale changed the base branch from feature/pr-reviews-master to feature/pr-reviews Apr 10, 2018
@jcansdale jcansdale changed the base branch from feature/pr-reviews to feature/pr-reviews-master Apr 10, 2018
@jcansdale jcansdale merged commit a96fd8f into feature/pr-reviews-master Apr 10, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jcansdale jcansdale deleted the fixes/pr-authoring-command-validatation branch Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.