Pull Requests w/ Statuses #1788

StanleyGoldman · 2018-07-19T21:55:47Z

Initially we set out to implement both the Statuses API and the Checks API, but OAuth applications do not currently have access to the Checks API and there are no major projects making use of the Statuses API.

Since Statuses are simpler Check Runs, we can implement Statuses with the anticipation of adding Check Runs.

https://developer.github.com/v3/repos/statuses/

Known issues

The icon will display the icon of the user that integrated the app, not the icon of the app itself
- There is a bug in the GraphQL API where it is not possible to get the correct Application icon for Statuses. I thought I did everything for MEF to like me, but VisualStudioBrowser remains null.
- We are waiting for API changes to make this data available

Trouble opening the Hyperlink from PullRequestCheckRunView

VisualStudio/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestCheckView.xaml.cs

Lines 27 to 34 in 2f3e739

    
           [Import] 
        
           IVisualStudioBrowser VisualStudioBrowser { get; set; } 
        
           void DoOpenDetailsUrl() 
        
           { 
        
               var browser = VisualStudioBrowser; 
        
               browser.OpenUrl(ViewModel.DetailsUrl); 
        
           }

New Metrics

NumberOfPRDetailsOpenInGitHub: The number of times the "View on GitHub" link is clicked from a pull request detail view
NumberOfPRCheckStatusesOpenInGitHub: The number of times a Status API "Details" link is clicked

TODO

Need designs for the Checks dropdown on the PullRequestDetailsView (@donokuda)

User Interfaces

# Conflicts: # src/GitHub.VisualStudio/Views/GitHubPane/PullRequestListItemView.xaml

Attempting to use the view locator correctly

grokys

Looking great so far! A few comments.

grokys · 2018-08-07T07:51:32Z

src/GitHub.App/Models/PullRequestModel.cs

        }

+        PullRequestChecksEnum checks;
+        public PullRequestChecksEnum Checks


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.

grokys · 2018-08-07T07:54:23Z

src/GitHub.Exports/Models/IPullRequestModel.cs

        Merged,
    }

+    public enum PullRequestChecksEnum


This should probably be called PullRequestCheckState - the Enum suffix on PullRequestStateEnum was a hack and is only still present because I'm in the process of removing PullRequestModel.

grokys · 2018-08-07T07:56:58Z

src/GitHub.App/Services/PullRequestService.cs

+        }
+    }
+
+    public class StatusSummaryModel


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

grokys · 2018-08-07T07:58:10Z

src/GitHub.App/ViewModels/GitHubPane/PullRequestCheckViewModel.cs

+        {
+            return pullRequest.Statuses?.Select(model =>
+            {
+                var statusStateEnum = model.State;


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

grokys · 2018-08-07T08:02:04Z

src/GitHub.VisualStudio/Views/GitHubPane/PullRequestCheckView.xaml

+             mc:Ignorable="d" d:DesignWidth="600">
+
+    <d:DesignData.DataContext>
+        <vm:PullRequestCheckViewModel Title="continuous-integration/appveyor/branch"


Getting an error here:

The type "PullRequestCheckViewModel" does not include any accessible constructors.

Probably need a PullRequestCheckViewModelDesigner class.

grokys · 2018-08-07T08:07:08Z

src/GitHub.Exports/Models/StatusStateEnum.cs

@@ -0,0 +1,11 @@
+namespace GitHub.Models
+{
+    public enum StatusStateEnum


Again, the Enum suffix isn't needed.

grokys · 2018-08-07T08:08:17Z

src/GitHub.Exports.Reactive/ViewModels/GitHubPane/IPullRequestCheckViewModel.cs

+        ReactiveCommand<object> OpenDetailsUrl { get; }
+    }
+
+    public enum PullRequestCheckStatusEnum


Again, the Enum suffix isn't necessary.

grokys · 2018-08-07T08:09:34Z

src/GitHub.App/ViewModels/GitHubPane/PullRequestCheckViewModel.cs

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


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

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

VisualStudio/src/GitHub.App/ViewModels/GitHubPane/PullRequestCheckViewModel.cs

Lines 54 to 62 in 14d5eca

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);

Oh, but I see the point..

grokys · 2018-08-07T08:12:09Z

src/GitHub.App/ViewModels/GitHubPane/PullRequestListItemViewModel.cs

        /// <inheritdoc/>
        public IActorViewModel Author { get; protected set; }

+        /// <inheritdoc/>


Actually, I'm not sure why I made these properties protected set - I think that was a mistake... I think I initially wrote this as a base class for both issues and PRs and then decided not to do that.

grokys · 2018-08-07T08:13:11Z

src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs

        bool refreshOnActivate;
        Uri webUrl;
        IDisposable sessionSubscription;
+        IViewViewModelFactory viewViewModelFactory;


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

…s-pull-request-model-1

grokys

Pretty much there! I'm mainly nitpicking at this point ;) Also, some XML documentation might be nice for the new classes?

grokys · 2018-08-08T06:20:35Z

src/GitHub.App/Models/PullRequestModel.cs

 using GitHub.Primitives;
 using GitHub.VisualStudio.Helpers;
 using System.Diagnostics;
-using System.Collections.Generic;


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

grokys · 2018-08-08T06:23:11Z

src/GitHub.App/SampleData/PullRequestCheckViewModelDesigner.cs

+using GitHub.ViewModels.GitHubPane;
+using ReactiveUI;
+
+namespace GitHub.App.SampleData


Namespace for this should simply be GitHub.SampleData.

grokys · 2018-08-08T06:23:56Z

src/GitHub.VisualStudio/Views/GitHubPane/PullRequestCheckView.xaml

+             mc:Ignorable="d" d:DesignWidth="600">
+
+    <d:DesignData.DataContext>
+        <sampleData1:PullRequestCheckViewModelDesigner />


If namespace for this class is changed as per previous comment you can use ghfvs: XML namespace.

grokys · 2018-08-08T06:24:34Z

src/GitHub.VisualStudio/Views/GitHubPane/PullRequestCheckView.xaml

+             xmlns:ui="clr-namespace:GitHub.UI;assembly=GitHub.UI"
+             xmlns:vm="clr-namespace:GitHub.ViewModels.GitHubPane;assembly=GitHub.App"
+             xmlns:cache="clr-namespace:GitHub.UI.Helpers;assembly=GitHub.UI"
+             xmlns:sampleData="clr-namespace:GitHub.SampleData;assembly=GitHub.App"


And this namespace isn't used.

grokys · 2018-08-08T06:26:20Z

src/GitHub.App/Services/ModelService.cs


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


This isn't used any more.

grokys · 2018-08-08T06:28:23Z

src/GitHub.Exports/Models/StatusStateEnum.cs

@@ -0,0 +1,11 @@
+namespace GitHub.Models
+{
+    public enum StatusState


You renamed the enum but the filename is still StatusStateEnum.

grokys · 2018-08-08T06:29:26Z

src/GitHub.App/ViewModels/GitHubPane/PullRequestListItemViewModel.cs


        /// <inheritdoc/>
-        public int Number { get; protected set; }
+        public int Number { get; }


Thanks for fixing these :D

grokys · 2018-08-08T14:07:16Z

src/GitHub.VisualStudio/Views/GitHubPane/PullRequestCheckView.xaml

+             mc:Ignorable="d" d:DesignWidth="600">
+
+    <d:DesignData.DataContext>
+        <ghvs:PullRequestCheckViewModelDesigner />


I think you misunderstood me here: I meant you should be able to use the existing ghfvs namespace for this.

StanleyGoldman added 7 commits July 12, 2018 16:32

Attempting to query all Check Suites with Pull Requests

d1d60ff

Trying to get a Check and Status summary

bfd6bfb

Experimenting with ways to get the data via GraphQL

8622f21

Updating nuget package

518c8c0

Adding models

c3d3b1c

Adding pull request summary state to the list

047ba1a

Merge branch 'master' into stanley/check-suites-pull-request-model-1

ae00f33

# Conflicts: # src/GitHub.VisualStudio/Views/GitHubPane/PullRequestListItemView.xaml

StanleyGoldman added the WIP label Jul 19, 2018

StanleyGoldman force-pushed the stanley/check-suites-pull-request-model-1 branch from c883707 to ae00f33 Compare July 19, 2018 23:40

StanleyGoldman changed the title ~~Pull Requests w/ Checks & Suites~~ WIP: Pull Requests w/ Checks & Suites Jul 20, 2018

donokuda and others added 7 commits July 20, 2018 14:33

We're three columns now

c6cd36f

Move comment count and status into its own column

eb575e6

Set a minimum width

756e733

Right align pull request status and comment count

389d49a

Functionality to display Checks on PullRequestDetailView

9c60db1

Restrain the size of the CheckRun display until it gets proper designs

2405755

Adding a Hyperlink

42bb263

StanleyGoldman changed the title ~~WIP: Pull Requests w/ Checks & Suites~~ Pull Requests w/ Statuses Jul 24, 2018

StanleyGoldman removed the WIP label Jul 24, 2018

StanleyGoldman added 2 commits July 24, 2018 13:54

Merge branch 'master' into stanley/check-suites-pull-request-model-1

533506f

Separating models

42fbab6

StanleyGoldman force-pushed the stanley/check-suites-pull-request-model-1 branch from 3ca835c to 68c0064 Compare July 24, 2018 18:27

Removing functionality to read CheckSuites

b086078

StanleyGoldman force-pushed the stanley/check-suites-pull-request-model-1 branch from 68c0064 to b086078 Compare July 24, 2018 18:29

Code cleanup

bade798

StanleyGoldman force-pushed the stanley/check-suites-pull-request-model-1 branch from 722abbf to bade798 Compare July 24, 2018 18:35

StanleyGoldman requested review from grokys, jcansdale and shana July 24, 2018 18:40

StanleyGoldman added the Ready for Review label Jul 24, 2018

StanleyGoldman and others added 3 commits August 1, 2018 11:59

Attempting to use the view locator correctly

b45bcc8

Merge pull request #1808 from github/view-locator-tears

5c2b93c

Attempting to use the view locator correctly

Merge branch 'master' into stanley/check-suites-pull-request-model-1

035915d

grokys suggested changes Aug 7, 2018

View reviewed changes

grokys and others added 16 commits August 7, 2018 10:19

Merge branch 'master' into stanley/check-suites-pull-request-model-1

ee83908

Removing Checks from PullRequestModel

2b8cee3

Making StatusSummaryModel an inner class

f964972

Merge branch 'master' into stanley/check-suites-pull-request-model-1

14d5eca

Making properties of PullRequestCheckViewModel read only

a026bab

Making member readonly

6a6c565

Renaming PullRequestCheckStatusEnum

cfa7c20

Renaming PullRequestChecksEnum

35ba4b2

Inlining variable

e6bf1bf

Renaming StatusStateEnum

d5d633e

Formatting code

f9f8d9a

Creating a PullRequestCheckViewModelDesigner

3639807

Removing viewLocator

48f180d

Removing designer usage

e55e47a

Making several properties read only

d6a95f7

Merge remote-tracking branch 'origin/master' into stanley/check-suite…

1fe4822

…s-pull-request-model-1

grokys suggested changes Aug 8, 2018

View reviewed changes

Final nitpicks

86df2bc

grokys suggested changes Aug 8, 2018

View reviewed changes

StanleyGoldman added 4 commits August 8, 2018 10:55

Cleanup view imports

01efca2

Adding some xmlcomments

57b4f31

More xmldocs

923ad51

Formatting and xmldocs

f4d59bb

grokys approved these changes Aug 8, 2018

View reviewed changes

grokys merged commit c3acc5d into master Aug 9, 2018

StanleyGoldman deleted the stanley/check-suites-pull-request-model-1 branch December 13, 2018 14:59

	[Import]
	IVisualStudioBrowser VisualStudioBrowser { get; set; }

	void DoOpenDetailsUrl()
	{
	var browser = VisualStudioBrowser;
	browser.OpenUrl(ViewModel.DetailsUrl);
	}

	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);

Pull Requests w/ Statuses #1788

Pull Requests w/ Statuses #1788

Uh oh!

Conversation

StanleyGoldman commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Known issues

New Metrics

TODO

User Interfaces

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StanleyGoldman Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

StanleyGoldman commented Jul 19, 2018 •

edited

Loading

StanleyGoldman Aug 7, 2018 •

edited

Loading