Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit 1a867f8

Browse files
grokysgrokys
committed
Validate PR review state before submit.
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
1 parent 9c36a14 commit 1a867f8

File tree

5 files changed

+158
-23
lines changed

5 files changed

+158
-23
lines changed

‎src/GitHub.App/SampleData/PullRequestReviewAuthoringViewModelDesigner.cs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@ public PullRequestReviewAuthoringViewModelDesigner()
4747
public string OperationError { get; set; }
4848
public IPullRequestModel PullRequestModel { get; set; }
4949
public string RemoteRepositoryOwner { get; set; }
50-
public ReactiveCommand<Unit> Submit { get; }
50+
public ReactiveCommand<Unit> Approve { get; }
51+
public ReactiveCommand<Unit> Comment { get; }
52+
public ReactiveCommand<Unit> RequestChanges { get; }
5153
public ReactiveCommand<Unit> Cancel { get; }
5254

5355
public Task InitializeAsync(

‎src/GitHub.App/ViewModels/GitHubPane/PullRequestReviewAuthoringViewModel.cs‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,16 @@ public PullRequestReviewAuthoringViewModel(
5757
.ToProperty(this, x => x.CanApproveRequestChanges);
5858

5959
Files = files;
60-
Submit = ReactiveCommand.CreateAsyncTask(DoSubmit);
60+
Approve = ReactiveCommand.CreateAsyncTask(_ => DoSubmit(Octokit.PullRequestReviewEvent.Approve));
61+
Comment = ReactiveCommand.CreateAsyncTask(
62+
this.WhenAnyValue(
63+
x => x.Body,
64+
x => x.FileComments.Count,
65+
(body, comments) => !string.IsNullOrWhiteSpace(body) || comments > 0),
66+
_ => DoSubmit(Octokit.PullRequestReviewEvent.Comment));
67+
RequestChanges = ReactiveCommand.CreateAsyncTask(
68+
this.WhenAnyValue(x => x.Body, body => !string.IsNullOrWhiteSpace(body)),
69+
_ => DoSubmit(Octokit.PullRequestReviewEvent.RequestChanges));
6170
Cancel = ReactiveCommand.CreateAsyncTask(DoCancel);
6271
}
6372

@@ -112,7 +121,9 @@ public IReadOnlyList<IPullRequestReviewFileCommentViewModel> FileComments
112121
}
113122

114123
public ReactiveCommand<object> NavigateToPullRequest { get; }
115-
public ReactiveCommand<Unit> Submit { get; }
124+
public ReactiveCommand<Unit> Approve { get; }
125+
public ReactiveCommand<Unit> Comment { get; }
126+
public ReactiveCommand<Unit> RequestChanges { get; }
116127
public ReactiveCommand<Unit> Cancel { get; }
117128

118129
public async Task InitializeAsync(
@@ -237,20 +248,15 @@ async Task UpdateFileComments()
237248
await Files.InitializeAsync(session, FilterComments);
238249
}
239250

240-
async Task DoSubmit(object arg)
251+
async Task DoSubmit(Octokit.PullRequestReviewEvent e)
241252
{
242253
OperationError = null;
243254
IsBusy = true;
244255

245256
try
246257
{
247-
Octokit.PullRequestReviewEvent e;
248-
249-
if (Enum.TryParse(arg.ToString(), out e))
250-
{
251-
await session.PostReview(Body, e);
252-
Close();
253-
}
258+
await session.PostReview(Body, e);
259+
Close();
254260
}
255261
catch (Exception ex)
256262
{

‎src/GitHub.Exports.Reactive/ViewModels/GitHubPane/IPullRequestReviewAuthoringViewModel.cs‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,19 @@ public interface IPullRequestReviewAuthoringViewModel : IPanePageViewModel, IDis
6868
ReactiveCommand<object> NavigateToPullRequest { get; }
6969

7070
/// <summary>
71-
/// Gets a command which submits the review.
71+
/// Gets a command which submits the review as an approval.
7272
/// </summary>
73-
ReactiveCommand<Unit> Submit { get; }
73+
ReactiveCommand<Unit> Approve { get; }
74+
75+
/// <summary>
76+
/// Gets a command which submits the review as a comment.
77+
/// </summary>
78+
ReactiveCommand<Unit> Comment { get; }
79+
80+
/// <summary>
81+
/// Gets a command which submits the review requesting changes.
82+
/// </summary>
83+
ReactiveCommand<Unit> RequestChanges { get; }
7484

7585
/// <summary>
7686
/// Gets a command which cancels the review.

‎src/GitHub.VisualStudio/Views/GitHubPane/PullRequestReviewAuthoringView.xaml‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,13 @@
5656
BorderThickness="1"
5757
Padding="4">
5858
<StackPanel Background="{DynamicResource GitHubVsToolWindowBackground}">
59-
<ui:GitHubActionLink Command="{Binding Submit}" CommandParameter="Comment" Margin="4">Comment only</ui:GitHubActionLink>
60-
<ui:GitHubActionLink Command="{Binding Submit}"
61-
CommandParameter="Approve"
59+
<ui:GitHubActionLink Command="{Binding Comment}" Margin="4">Comment only</ui:GitHubActionLink>
60+
<ui:GitHubActionLink Command="{Binding Approve}"
6261
Visibility="{Binding CanApproveRequestChanges, Converter={ui:BooleanToVisibilityConverter}}"
6362
Margin="4">
6463
Approve
6564
</ui:GitHubActionLink>
66-
<ui:GitHubActionLink Command="{Binding Submit}"
67-
CommandParameter="RequestChanges"
65+
<ui:GitHubActionLink Command="{Binding RequestChanges}"
6866
Visibility="{Binding CanApproveRequestChanges, Converter={ui:BooleanToVisibilityConverter}}"
6967
Margin="4">
7068
Request changes

‎test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestReviewAuthoringViewModelTests.cs‎

Lines changed: 124 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -209,24 +209,71 @@ public async Task Updates_Model_Id_From_PendingReviewId_When_Session_PullRequest
209209
}
210210

211211
[Test]
212-
public async Task Submit_Calls_Session_PostReview()
212+
public async Task Approve_Calls_Session_PostReview_And_Closes()
213213
{
214214
var review = CreateReview(12, "grokys", state: PullRequestReviewState.Pending);
215215
var model = CreatePullRequest("shana", review);
216216
var session = CreateSession();
217+
var closed = false;
217218

218219
var target = CreateTarget(model, session);
219220

220221
await Initialize(target);
221-
222222
target.Body = "Post review";
223-
target.Submit.Execute(Octokit.PullRequestReviewEvent.Approve);
223+
target.CloseRequested.Subscribe(_ => closed = true);
224+
target.Approve.Execute(null);
224225

225226
await session.Received(1).PostReview("Post review", Octokit.PullRequestReviewEvent.Approve);
227+
Assert.True(closed);
228+
}
229+
230+
[Test]
231+
public async Task Comment_Is_Disabled_When_Has_Empty_Body_And_No_File_Comments()
232+
{
233+
var review = CreateReview(12, "grokys", body: "", state: PullRequestReviewState.Pending);
234+
var model = CreatePullRequest("shana", review);
235+
var session = CreateSession();
236+
237+
var target = CreateTarget(model, session);
238+
await Initialize(target);
239+
240+
Assert.IsFalse(target.Comment.CanExecute(null));
241+
}
242+
243+
[Test]
244+
public async Task Comment_Is_Enabled_When_Has_Body()
245+
{
246+
var review = CreateReview(12, "grokys", body: "", state: PullRequestReviewState.Pending);
247+
var model = CreatePullRequest("shana", review);
248+
var session = CreateSession();
249+
250+
var target = CreateTarget(model, session);
251+
await Initialize(target);
252+
target.Body = "Review body";
253+
254+
Assert.IsTrue(target.Comment.CanExecute(null));
226255
}
227256

228257
[Test]
229-
public async Task Submit_Closes_Page()
258+
public async Task Comment_Is_Enabled_When_Has_File_Comments()
259+
{
260+
var comment = CreateReviewComment(12);
261+
var review = CreateReview(12, "grokys", body: "", state: PullRequestReviewState.Pending);
262+
var model = CreatePullRequest(
263+
authorLogin: "shana",
264+
reviews: new[] { review },
265+
reviewComments: new[] { comment });
266+
var session = CreateSession();
267+
268+
var target = CreateTarget(model, session);
269+
await Initialize(target);
270+
target.Body = "Review body";
271+
272+
Assert.IsTrue(target.Comment.CanExecute(null));
273+
}
274+
275+
[Test]
276+
public async Task Comment_Calls_Session_PostReview_And_Closes()
230277
{
231278
var review = CreateReview(12, "grokys", state: PullRequestReviewState.Pending);
232279
var model = CreatePullRequest("shana", review);
@@ -237,10 +284,61 @@ public async Task Submit_Closes_Page()
237284

238285
await Initialize(target);
239286
target.Body = "Post review";
287+
target.CloseRequested.Subscribe(_ => closed = true);
288+
target.Comment.Execute(null);
289+
290+
await session.Received(1).PostReview("Post review", Octokit.PullRequestReviewEvent.Comment);
291+
Assert.True(closed);
292+
}
240293

294+
[Test]
295+
public async Task RequestChanges_Is_Disabled_When_Has_Empty_Body()
296+
{
297+
var comment = CreateReviewComment(12);
298+
var review = CreateReview(12, "grokys", body: "", state: PullRequestReviewState.Pending);
299+
var model = CreatePullRequest(
300+
authorLogin: "shana",
301+
reviews: new[] { review },
302+
reviewComments: new[] { comment });
303+
var session = CreateSession();
304+
305+
var target = CreateTarget(model, session);
306+
await Initialize(target);
307+
target.Body = "";
308+
309+
Assert.IsFalse(target.RequestChanges.CanExecute(null));
310+
}
311+
312+
[Test]
313+
public async Task RequestChanges_Is_Enabled_When_Has_Body()
314+
{
315+
var review = CreateReview(12, "grokys", body: "", state: PullRequestReviewState.Pending);
316+
var model = CreatePullRequest("shana", review);
317+
var session = CreateSession();
318+
319+
var target = CreateTarget(model, session);
320+
await Initialize(target);
321+
target.Body = "Request Changes";
322+
323+
Assert.IsTrue(target.RequestChanges.CanExecute(null));
324+
}
325+
326+
[Test]
327+
public async Task RequestChanges_Calls_Session_PostReview_And_Closes()
328+
{
329+
var review = CreateReview(12, "grokys", state: PullRequestReviewState.Pending);
330+
var model = CreatePullRequest("shana", review);
331+
var session = CreateSession();
332+
var closed = false;
333+
334+
var target = CreateTarget(model, session);
335+
336+
await Initialize(target);
337+
target.Body = "Post review";
241338
target.CloseRequested.Subscribe(_ => closed = true);
242-
target.Submit.Execute(Octokit.PullRequestReviewEvent.Approve);
339+
target.RequestChanges.Execute(null);
243340

341+
await session.Received(1).PostReview("Post review", Octokit.PullRequestReviewEvent.RequestChanges);
244342
Assert.True(closed);
245343
}
246344

@@ -349,6 +447,27 @@ static PullRequestModel CreatePullRequest(
349447
return result;
350448
}
351449

450+
static PullRequestModel CreatePullRequest(
451+
string authorLogin = "grokys",
452+
IEnumerable<IPullRequestReviewModel> reviews = null,
453+
IEnumerable<IPullRequestReviewCommentModel> reviewComments = null)
454+
{
455+
reviews = reviews ?? new IPullRequestReviewModel[0];
456+
reviewComments = reviewComments ?? new IPullRequestReviewCommentModel[0];
457+
458+
var author = Substitute.For<IAccount>();
459+
author.Login.Returns(authorLogin);
460+
461+
var result = new PullRequestModel(
462+
5,
463+
"Pull Request",
464+
author,
465+
DateTimeOffset.Now);
466+
result.Reviews = reviews.ToList();
467+
result.ReviewComments = reviewComments.ToList();
468+
return result;
469+
}
470+
352471
static IPullRequestSession CreateSession(
353472
string userLogin = "grokys",
354473
params IPullRequestSessionFile[] files)

0 commit comments

Comments
 (0)