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

Pull Requests w/ Statuses #1788

Merged
merged 63 commits into from Aug 9, 2018
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
d1d60ff
Attempting to query all Check Suites with Pull Requests
StanleyGoldman Jul 12, 2018
bfd6bfb
Trying to get a Check and Status summary
StanleyGoldman Jul 13, 2018
8622f21
Experimenting with ways to get the data via GraphQL
StanleyGoldman Jul 17, 2018
518c8c0
Updating nuget package
StanleyGoldman Jul 17, 2018
c3d3b1c
Adding models
StanleyGoldman Jul 17, 2018
047ba1a
Adding pull request summary state to the list
StanleyGoldman Jul 19, 2018
ae00f33
Merge branch 'master' into stanley/check-suites-pull-request-model-1
StanleyGoldman Jul 19, 2018
c6cd36f
We're three columns now
donokuda Jul 20, 2018
eb575e6
Move comment count and status into its own column
donokuda Jul 20, 2018
756e733
Set a minimum width
donokuda Jul 20, 2018
389d49a
Right align pull request status and comment count
donokuda Jul 20, 2018
9c60db1
Functionality to display Checks on PullRequestDetailView
StanleyGoldman Jul 23, 2018
2405755
Restrain the size of the CheckRun display until it gets proper designs
StanleyGoldman Jul 24, 2018
42bb263
Adding a Hyperlink
StanleyGoldman Jul 24, 2018
533506f
Merge branch 'master' into stanley/check-suites-pull-request-model-1
StanleyGoldman Jul 24, 2018
42fbab6
Separating models
StanleyGoldman Jul 24, 2018
b086078
Removing functionality to read CheckSuites
StanleyGoldman Jul 24, 2018
bade798
Code cleanup
StanleyGoldman Jul 24, 2018
6d4c4ca
Add a bit of spacing
donokuda Jul 25, 2018
e7e26d9
Attempting to open up a browser link
StanleyGoldman Jul 25, 2018
2f3e739
Merge pull request #1793 from github/donokuda/check-suites-polish
StanleyGoldman Jul 25, 2018
2236973
Changing column order
StanleyGoldman Jul 25, 2018
10b1fdd
Correcting functionality to open the browser
StanleyGoldman Jul 25, 2018
7107bdc
Merge branch 'master' into stanley/check-suites-pull-request-model-1
StanleyGoldman Jul 25, 2018
4074fea
Add missing argument
StanleyGoldman Jul 26, 2018
5a7775c
Adjust some row/column definitions
donokuda Jul 26, 2018
9c6c5da
make the avatar smaller
donokuda Jul 26, 2018
276549b
Hide this description
donokuda Jul 26, 2018
5c2c0f7
Use the primitive dot octicon for pending checks
donokuda Jul 26, 2018
5edce94
Add a lil' bit of right margin
donokuda Jul 26, 2018
af979f2
Merge branch 'master' into stanley/check-suites-pull-request-model-1
StanleyGoldman Jul 26, 2018
9db38a9
Try out a GridView
donokuda Jul 30, 2018
d279d06
Use IsSharedSizeScope so that Grid Rendering works
donokuda Jul 30, 2018
86b3942
Remove extra line
donokuda Jul 30, 2018
b0cdcf0
Fix text color for dark theme
donokuda Jul 30, 2018
7ced79b
Merge pull request #1813 from github/donokuda/polish-check-details
StanleyGoldman Jul 31, 2018
967e91d
Adding metrics to the UsageTracker
StanleyGoldman Jul 31, 2018
e890055
Hiding Status Avatars until we get a way to query them through the Gr…
StanleyGoldman Aug 1, 2018
0974944
Merge branch 'master' into stanley/check-suites-pull-request-model-1
StanleyGoldman Aug 1, 2018
b45bcc8
Attempting to use the view locator correctly
StanleyGoldman Jul 26, 2018
5c2b93c
Merge pull request #1808 from github/view-locator-tears
StanleyGoldman Aug 2, 2018
035915d
Merge branch 'master' into stanley/check-suites-pull-request-model-1
StanleyGoldman Aug 6, 2018
ee83908
Merge branch 'master' into stanley/check-suites-pull-request-model-1
grokys Aug 7, 2018
2b8cee3
Removing Checks from PullRequestModel
StanleyGoldman Aug 7, 2018
f964972
Making StatusSummaryModel an inner class
StanleyGoldman Aug 7, 2018
14d5eca
Merge branch 'master' into stanley/check-suites-pull-request-model-1
StanleyGoldman Aug 7, 2018
a026bab
Making properties of PullRequestCheckViewModel read only
StanleyGoldman Aug 7, 2018
6a6c565
Making member readonly
StanleyGoldman Aug 7, 2018
cfa7c20
Renaming PullRequestCheckStatusEnum
StanleyGoldman Aug 7, 2018
35ba4b2
Renaming PullRequestChecksEnum
StanleyGoldman Aug 7, 2018
e6bf1bf
Inlining variable
StanleyGoldman Aug 7, 2018
d5d633e
Renaming StatusStateEnum
StanleyGoldman Aug 7, 2018
f9f8d9a
Formatting code
StanleyGoldman Aug 7, 2018
3639807
Creating a PullRequestCheckViewModelDesigner
StanleyGoldman Aug 7, 2018
48f180d
Removing viewLocator
StanleyGoldman Aug 7, 2018
e55e47a
Removing designer usage
StanleyGoldman Aug 7, 2018
d6a95f7
Making several properties read only
StanleyGoldman Aug 7, 2018
1fe4822
Merge remote-tracking branch 'origin/master' into stanley/check-suite…
StanleyGoldman Aug 7, 2018
86df2bc
Final nitpicks
StanleyGoldman Aug 8, 2018
01efca2
Cleanup view imports
StanleyGoldman Aug 8, 2018
57b4f31
Adding some xmlcomments
StanleyGoldman Aug 8, 2018
923ad51
More xmldocs
StanleyGoldman Aug 8, 2018
f4d59bb
Formatting and xmldocs
StanleyGoldman Aug 8, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/GitHub.App/GitHub.App.csproj
Expand Up @@ -267,6 +267,7 @@
<Compile Include="ViewModels\GitHubPane\NotAGitRepositoryViewModel.cs" />
<Compile Include="ViewModels\GitHubPane\PullRequestReviewAuthoringViewModel.cs" />
<Compile Include="ViewModels\GitHubPane\PullRequestReviewCommentViewModel.cs" />
<Compile Include="ViewModels\GitHubPane\PullRequestCheckViewModel.cs" />
<Compile Include="ViewModels\GitHubPane\PullRequestReviewSummaryViewModel.cs" />
<Compile Include="ViewModels\GitHubPane\PullRequestReviewViewModel.cs" />
<Compile Include="ViewModels\GitHubPane\PullRequestUserReviewsViewModel.cs" />
Expand Down
14 changes: 12 additions & 2 deletions src/GitHub.App/Models/PullRequestModel.cs
Expand Up @@ -3,7 +3,6 @@
using GitHub.Primitives;
using GitHub.VisualStudio.Helpers;
using System.Diagnostics;
using System.Collections.Generic;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Would be nice to completely remove the changes to these unchanged files to give a cleaner diff ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😸

using GitHub.Extensions;

namespace GitHub.Models
Expand Down Expand Up @@ -125,6 +124,17 @@ public PullRequestStateEnum State
}
}

PullRequestChecksEnum checks;
public PullRequestChecksEnum Checks
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this actually needs to be added to PullRequestModel - this class is now obsolete (I should really have added an [Obsolete] attribute to it) and is only used in the PR creation view, from where I hope to soon remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see...

{
get { return checks; }
set
{
checks = value;
this.RaisePropertyChange();
}
}

// TODO: Remove these property once maintainer workflow has been merged to master.
public bool IsOpen => State == PullRequestStateEnum.Open;
public bool Merged => State == PullRequestStateEnum.Merged;
Expand Down Expand Up @@ -183,4 +193,4 @@ internal string DebuggerDisplay
}
}
}
}
}
32 changes: 32 additions & 0 deletions src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs
Expand Up @@ -95,6 +95,36 @@ public PullRequestDetailViewModelDesigner()
};

Files = new PullRequestFilesViewModelDesigner();

var defaultAvatar = "pack://application:,,,/GitHub.App;component/Images/default_user_avatar.png";

Checks = new[]
{
new PullRequestCheckViewModel(null)
{
Title = "continuous-integration/appveyor/branch",
Description = "AppVeyor build succeeded",
Status = PullRequestCheckStatusEnum.Success,
AvatarUrl = defaultAvatar,
Avatar = AvatarProvider.CreateBitmapImage(defaultAvatar),
},
new PullRequestCheckViewModel(null)
{
Title = "continuous-integration/appveyor/pr",
Description = "AppVeyor building",
Status = PullRequestCheckStatusEnum.Pending,
AvatarUrl = defaultAvatar,
Avatar = AvatarProvider.CreateBitmapImage(defaultAvatar),
},
new PullRequestCheckViewModel(null)
{
Title = "continuous-integration/appveyor/other",
Description = "AppVeyor build failed",
Status = PullRequestCheckStatusEnum.Failure,
AvatarUrl = defaultAvatar,
Avatar = AvatarProvider.CreateBitmapImage(defaultAvatar),
},
};
}

public PullRequestDetailModel Model { get; }
Expand Down Expand Up @@ -123,6 +153,8 @@ public PullRequestDetailViewModelDesigner()
public ReactiveCommand<object> OpenOnGitHub { get; }
public ReactiveCommand<object> ShowReview { get; }

public IReadOnlyList<IPullRequestCheckViewModel> Checks { get; }

public Task InitializeAsync(ILocalRepositoryModel localRepository, IConnection connection, string owner, string repo, int number) => Task.CompletedTask;

public string GetLocalFilePath(IPullRequestFileNode file)
Expand Down
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using GitHub.Models;
using GitHub.ViewModels;
using GitHub.ViewModels.GitHubPane;

Expand All @@ -16,5 +17,6 @@ public class PullRequestListItemViewModelDesigner : ViewModelBase, IPullRequestL
public int Number { get; set; }
public string Title { get; set; }
public DateTimeOffset UpdatedAt { get; set; }
public PullRequestChecksEnum Checks { get; set; }
}
}
2 changes: 2 additions & 0 deletions src/GitHub.App/Services/ModelService.cs
Expand Up @@ -440,6 +440,7 @@ IPullRequestModel Create(PullRequestCacheItem prCacheItem)
CommitCount = prCacheItem.CommitCount,
CreatedAt = prCacheItem.CreatedAt,
Head = Create(prCacheItem.Head),
Checks = prCacheItem.Checks ?? PullRequestChecksEnum.None,
State = prCacheItem.State.HasValue ?
prCacheItem.State.Value :
prCacheItem.IsOpen.Value ? PullRequestStateEnum.Open : PullRequestStateEnum.Closed,
Expand Down Expand Up @@ -577,6 +578,7 @@ public PullRequestCacheItem(PullRequest pr)

// Nullable for compatibility with old caches.
public PullRequestStateEnum? State { get; set; }
public PullRequestChecksEnum? Checks { get; set; }

// This fields exists only for compatibility with old caches. The State property should be used.
public bool? IsOpen { get; set; }
Expand Down
61 changes: 60 additions & 1 deletion src/GitHub.App/Services/PullRequestService.cs
Expand Up @@ -93,6 +93,17 @@ public class PullRequestService : IPullRequestService
Items = page.Nodes.Select(pr => new ListItemAdapter
{
Id = pr.Id.Value,
LastCommit = pr.Commits(null, null, 1, null).Nodes.Select(commit =>
new LastCommitSummaryModel
{
Statuses = commit.Commit.Status
.Select(context =>
context.Contexts.Select(statusContext => new StatusSummaryModel
{
State = (StatusStateEnum)statusContext.State,
}).ToList()
).SingleOrDefault()
}).ToList().FirstOrDefault(),
Author = new ActorModel
{
Login = pr.Author.Login,
Expand Down Expand Up @@ -123,10 +134,46 @@ public class PullRequestService : IPullRequestService

var result = await graphql.Run(readPullRequests, vars);

foreach (ListItemAdapter item in result.Items)
foreach (var item in result.Items.Cast<ListItemAdapter>())
{
item.CommentCount += item.Reviews.Sum(x => x.Count);
item.Reviews = null;

var hasStatuses = item.LastCommit.Statuses != null
&& item.LastCommit.Statuses.Any();

if (!hasStatuses)
{
item.Checks = PullRequestChecksEnum.None;
}
else
{
var statusHasFailure = item.LastCommit
.Statuses
.Any(status => status.State == StatusStateEnum.Failure);

var statusHasCompleteSuccess = true;
if (!statusHasFailure)
{
statusHasCompleteSuccess =
item.LastCommit.Statuses.All(status => status.State == StatusStateEnum.Success);
}

if (statusHasFailure)
{
item.Checks = PullRequestChecksEnum.Failure;
}
else if (statusHasCompleteSuccess)
{
item.Checks = PullRequestChecksEnum.Success;
}
else
{
item.Checks = PullRequestChecksEnum.Pending;
}
}

item.LastCommit = null;
}

return result;
Expand Down Expand Up @@ -840,6 +887,8 @@ static string BuildGHfVSConfigKeyValue(string owner, int number)
class ListItemAdapter : PullRequestListItemModel
{
public IList<ReviewAdapter> Reviews { get; set; }

public LastCommitSummaryModel LastCommit { get; set; }
}

class ReviewAdapter
Expand All @@ -848,5 +897,15 @@ class ReviewAdapter
public int CommentCount { get; set; }
public int Count => CommentCount + (!string.IsNullOrWhiteSpace(Body) ? 1 : 0);
}

class LastCommitSummaryModel
{
public List<StatusSummaryModel> Statuses { get; set; }
}
}

public class StatusSummaryModel
Copy link
Contributor

Choose a reason for hiding this comment

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

If this model class is only used within PullRequestService then it should be a private inner class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

{
public StatusStateEnum State { get; set; }
}
}
119 changes: 119 additions & 0 deletions src/GitHub.App/ViewModels/GitHubPane/PullRequestCheckViewModel.cs
@@ -0,0 +1,119 @@
using System;
using System.Collections.Generic;
using System.ComponentModel.Composition;
using System.Linq;
using System.Reactive;
using System.Reactive.Linq;
using System.Windows.Media.Imaging;
using GitHub.Extensions;
using GitHub.Factories;
using GitHub.Models;
using GitHub.Services;
using ReactiveUI;

namespace GitHub.ViewModels.GitHubPane
{
[Export(typeof(IPullRequestCheckViewModel))]
[PartCreationPolicy(CreationPolicy.NonShared)]
public class PullRequestCheckViewModel: ViewModelBase, IPullRequestCheckViewModel
{
private readonly IUsageTracker usageTracker;
const string DefaultAvatar = "pack://application:,,,/GitHub.App;component/Images/default_user_avatar.png";

private string title;
private string description;
private PullRequestCheckStatusEnum status;
private Uri detailsUrl;
private string avatarUrl;
private BitmapImage avatar;

public static IEnumerable<IPullRequestCheckViewModel> Build(IViewViewModelFactory viewViewModelFactory, PullRequestDetailModel pullRequest)
{
return pullRequest.Statuses?.Select(model =>
{
var statusStateEnum = model.State;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the additional variable here? It's only used once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool


PullRequestCheckStatusEnum checkStatus;
switch (statusStateEnum)
{
case StatusStateEnum.Expected:
case StatusStateEnum.Error:
case StatusStateEnum.Failure:
checkStatus = PullRequestCheckStatusEnum.Failure;
break;
case StatusStateEnum.Pending:
checkStatus = PullRequestCheckStatusEnum.Pending;
break;
case StatusStateEnum.Success:
checkStatus = PullRequestCheckStatusEnum.Success;
break;
default:
throw new InvalidOperationException("Unkown PullRequestCheckStatusEnum");
}

var pullRequestCheckViewModel = viewViewModelFactory.CreateViewModel<IPullRequestCheckViewModel>();
pullRequestCheckViewModel.Title = model.Context;
pullRequestCheckViewModel.Description = model.Description;
pullRequestCheckViewModel.Status = checkStatus;
pullRequestCheckViewModel.DetailsUrl = new Uri(model.TargetUrl);
pullRequestCheckViewModel.AvatarUrl = model.AvatarUrl ?? DefaultAvatar;
pullRequestCheckViewModel.Avatar = model.AvatarUrl != null
? new BitmapImage(new Uri(model.AvatarUrl))
: AvatarProvider.CreateBitmapImage(DefaultAvatar);

return pullRequestCheckViewModel;

}) ?? new PullRequestCheckViewModel[0];
}

[ImportingConstructor]
public PullRequestCheckViewModel(IUsageTracker usageTracker)
{
this.usageTracker = usageTracker;
OpenDetailsUrl = ReactiveCommand.Create().OnExecuteCompleted(DoOpenDetailsUrl);
}

private void DoOpenDetailsUrl(object obj)
{
usageTracker.IncrementCounter(x => x.NumberOfPRCheckStatusesOpenInGitHub).Forget();
}

public string Title
{
get { return title; }
set { this.RaiseAndSetIfChanged(ref title, value); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these properties change? If not then they shouldn't be settable and don't need to raise property changed events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't change, but they are being set here...

var pullRequestCheckViewModel = viewViewModelFactory.CreateViewModel<IPullRequestCheckViewModel>();
pullRequestCheckViewModel.Title = model.Context;
pullRequestCheckViewModel.Description = model.Description;
pullRequestCheckViewModel.Status = checkStatus;
pullRequestCheckViewModel.DetailsUrl = new Uri(model.TargetUrl);
pullRequestCheckViewModel.AvatarUrl = model.AvatarUrl ?? DefaultAvatar;
pullRequestCheckViewModel.Avatar = model.AvatarUrl != null
? new BitmapImage(new Uri(model.AvatarUrl))
: AvatarProvider.CreateBitmapImage(DefaultAvatar);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, but I see the point..

}

public string Description
{
get { return description; }
set { this.RaiseAndSetIfChanged(ref description, value); }
}

public PullRequestCheckStatusEnum Status
{
get { return status; }
set { this.RaiseAndSetIfChanged(ref status, value); }
}

public Uri DetailsUrl
{
get { return detailsUrl; }
set { this.RaiseAndSetIfChanged(ref detailsUrl, value); }
}

public string AvatarUrl
{
get { return avatarUrl; }
set { this.RaiseAndSetIfChanged(ref avatarUrl, value); }
}

public BitmapImage Avatar
{
get { return avatar; }
set { this.RaiseAndSetIfChanged(ref avatar, value); }
}

public ReactiveCommand<object> OpenDetailsUrl { get; }
}
}
Expand Up @@ -55,6 +55,8 @@ public sealed class PullRequestDetailViewModel : PanePageViewModelBase, IPullReq
bool refreshOnActivate;
Uri webUrl;
IDisposable sessionSubscription;
IViewViewModelFactory viewViewModelFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be readonly and placed at the top with the other services.

IReadOnlyList<IPullRequestCheckViewModel> checks;

/// <summary>
/// Initializes a new instance of the <see cref="PullRequestDetailViewModel"/> class.
Expand All @@ -73,21 +75,24 @@ public sealed class PullRequestDetailViewModel : PanePageViewModelBase, IPullReq
IUsageTracker usageTracker,
ITeamExplorerContext teamExplorerContext,
IPullRequestFilesViewModel files,
ISyncSubmodulesCommand syncSubmodulesCommand)
ISyncSubmodulesCommand syncSubmodulesCommand,
IViewViewModelFactory viewViewModelFactory)
{
Guard.ArgumentNotNull(pullRequestsService, nameof(pullRequestsService));
Guard.ArgumentNotNull(sessionManager, nameof(sessionManager));
Guard.ArgumentNotNull(modelServiceFactory, nameof(modelServiceFactory));
Guard.ArgumentNotNull(usageTracker, nameof(usageTracker));
Guard.ArgumentNotNull(teamExplorerContext, nameof(teamExplorerContext));
Guard.ArgumentNotNull(syncSubmodulesCommand, nameof(syncSubmodulesCommand));
Guard.ArgumentNotNull(viewViewModelFactory, nameof(viewViewModelFactory));

this.pullRequestsService = pullRequestsService;
this.sessionManager = sessionManager;
this.modelServiceFactory = modelServiceFactory;
this.usageTracker = usageTracker;
this.teamExplorerContext = teamExplorerContext;
this.syncSubmodulesCommand = syncSubmodulesCommand;
this.viewViewModelFactory = viewViewModelFactory;
Files = files;

Checkout = ReactiveCommand.CreateAsyncObservable(
Expand Down Expand Up @@ -302,6 +307,12 @@ public Uri WebUrl
/// </summary>
public ReactiveCommand<object> ShowReview { get; }

public IReadOnlyList<IPullRequestCheckViewModel> Checks
{
get { return checks; }
private set { this.RaiseAndSetIfChanged(ref checks, value); }
}

/// <summary>
/// Initializes the view model.
/// </summary>
Expand Down Expand Up @@ -377,6 +388,8 @@ public async Task Load(PullRequestDetailModel pullRequest)
Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : Resources.NoDescriptionProvidedMarkdown;
Reviews = PullRequestReviewSummaryViewModel.BuildByUser(Session.User, pullRequest).ToList();

Checks = PullRequestCheckViewModel.Build(viewViewModelFactory, pullRequest)?.ToList();

await Files.InitializeAsync(Session);

var localBranches = await pullRequestsService.GetLocalBranches(LocalRepository, pullRequest).ToList();
Expand Down