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

Master PR for Pull Request Reviews #1491

Merged
merged 109 commits into from Apr 16, 2018

Conversation

Projects
5 participants
@grokys
Contributor

grokys commented Feb 15, 2018

This is the master PR for the Pull Request Reviews feature (spiked out over at #1415).

  • #1500 Removed unused InlineReviews code
  • #1492 Moved changed files tree into its own view
  • #1501 Use GraphQL in ModelService to read PR reviews/comments
  • #1521 Update octokit to 0.29.0
  • #1523 Add "Reviewers" section to PR details
  • #1545 Display Pull Request Reviews for a User
  • #1549 Display PR review comments inline
  • #1562 Start PR Reviews from Inline Comments
  • #1567 Add PR reviews metrics
  • #1572 Add the ability to cancel a Pull Request review
  • #1578 Use correct GraphQL endpoint for enterprise
  • #1580 Fix navigate to editor
  • #1581 Fix Octokit.GraphQL escaping issue
  • #1582 Validate PR review state before allowing submission
  • #1585 Reuse existing tabs when opening files from changes tree
  • #1586 Make avatar in PR review summary clickable
  • #1552 Make Navigate to Editor more discoverable
Whitespace change.
As GitHub won't let us create empty PRs.

grokys and others added some commits Mar 16, 2018

Removed unused InlineReviews code.
There was code left over from the now punted "conversation" view. Remove this code because by the time we get back to it, it will be out of date anyway.
Removed unused IInlineCommentModel.
And removed missing files.
Merge pull request #1500 from github/refactor/remove-unused-inlinerev…
…iews-code

Removed unused InlineReviews code.
Moved changed files tree into its own view.
Ported from #1415. Moves the PR details changed files tree into its own view so that it can be shared by the PR reviews view.
Port changes from #1407.
Enable navigation from diff view to editor
Added GraphQL to GitHub.Api.
And associated factory/keychain classes.
Use GraphQL to read PR reviews/comments.
This is integrated a bit hackily into `ModelService`; `ModelService` doesn't really mesh well with GraphQL but without a lot of refactoring this was the best way to get things up and running.
Need to force-add the .nupkg.
As it's ignored by default.
Fix PR review comments cursor bug.
The cursor was not being used for the first page, or ever in fact because the capitalization was wrong.
Add binding redirect for Newtonsoft.Json...
...in UnitTests. Akavache uses Newtonsoft.Json 6.0.8 while we use 10.0.3. The earlier version needs to be redirected to the later version for tests to work.
Use GraphQL to read PR reviews/comments.
This is integrated a bit hackily into `ModelService`; `ModelService` doesn't really mesh well with GraphQL but without a lot of refactoring this was the best way to get things up and running.
Moved changed files tree into its own view.
Ported from #1415. Moves the PR details changed files tree into its own view so that it can be shared by the PR reviews view.
Port changes from #1407.
Enable navigation from diff view to editor
Use GraphQL to read PR reviews/comments.
This is integrated a bit hackily into `ModelService`; `ModelService` doesn't really mesh well with GraphQL but without a lot of refactoring this was the best way to get things up and running.

jcansdale added some commits Apr 10, 2018

Show status bar hint when using View File
Factor out `EnableNavigateStatusBarMessage` method.
<HintPath>..\..\packages\Newtonsoft.Json.10.0.3\lib\net45\Newtonsoft.Json.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Microsoft.VisualStudio.Utilities, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">

This comment has been minimized.

@jcansdale

jcansdale Apr 10, 2018

Contributor

We have a duplicate Microsoft.VisualStudio.Utilities reference.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Apr 10, 2018

Should I be able to click on the 💬 on the reviewers section? I thought that was one of the PRs that got merged. 😕 I don't seem to be able to in this version.

jcansdale and others added some commits Apr 11, 2018

Merge branch 'master' into feature/pr-reviews-master
 Conflicts:
	src/GitHub.Exports/Models/UsageModel.cs
@jcansdale

I haven't scrutinized every line, but I've been dogfooding it quite a bit and it has been working well!

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Apr 13, 2018

Should I be able to click on the 💬 on the reviewers section? I thought that was one of the PRs that got merged. 😕 I don't seem to be able to in this version.

No, I misremembered. There was one about making the avatar clickable not the 💬 on the reviewers section.

@sguthals

This comment has been minimized.

Contributor

sguthals commented Apr 13, 2018

This might just be because I re-sized my VM and then re-sized the experimental version of VS, but I just notice that now things are being cut off on the left side within our pane, but I can scroll over to be able to see everything:
screen shot 2018-04-13 at 7 41 49 am

@sguthals

This comment has been minimized.

Contributor

sguthals commented Apr 13, 2018

Looks like the horizontal scroll is stuck at the bottom:
screen shot 2018-04-13 at 7 49 27 am

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Apr 13, 2018

This PR LGTM tested on VS2015 and 2017.

I ran through these test scenarios and did some exploratory testing as well.

@sguthals

This comment has been minimized.

Contributor

sguthals commented Apr 13, 2018

I double clicked on a folder instead of a file and it caused an exception:
screen shot 2018-04-13 at 9 08 11 am

@sguthals

LGTM! Left some comments/questions, but they aren't blockers. Reviewed this in experimental version!

public int Id { get; set; }
public string NodeId { get; set; }

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

What is the difference between Id and NodeId?

This comment has been minimized.

@grokys

grokys Apr 16, 2018

Contributor

As you noticed later, NodeId is the GraphQL ID which is different from the "database" Id. I didn't add doc comments to these fields because this will all change as we move move stuff to GraphQL, but I probably should have.

public IReadOnlyCollection<IPullRequestFileModel> ChangedFiles { get; set; } = new IPullRequestFileModel[0];
public IReadOnlyCollection<ICommentModel> Comments { get; set; } = new ICommentModel[0];
public IReadOnlyList<IPullRequestFileModel> ChangedFiles { get; set; } = new IPullRequestFileModel[0];
public IReadOnlyList<ICommentModel> Comments { get; set; } = new ICommentModel[0];

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

Why switch from IReadOnlyColletion to IReadOnlyList?

This comment has been minimized.

@grokys

grokys Apr 16, 2018

Contributor

IReadOnlyCollection doesn't allow indexers (e.g. collection[0]) which is useful in unit tests. There was no reason to use collection over list so I moved over to that.

@@ -69,8 +67,35 @@ public PullRequestDetailViewModelDesigner()
modelsDir.Files.Add(oldBranchModel);
gitHubDir.Directories.Add(modelsDir);
changedFilesTree = new List<IPullRequestChangeNode>();
changedFilesTree.Add(gitHubDir);
Reviews = new[]

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

So this is just a model of the type of data that would be included in a real PR Review that you model here?

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

Since this is all in sample data. I get it now.

This comment has been minimized.

@grokys

grokys Apr 16, 2018

Contributor

Yep, sample data for the designer.

namespace GitHub.SampleData
{
public class PullRequestReviewFileCommentViewModelDesigner : IPullRequestReviewFileCommentViewModel

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

Why dont' we have sample data here?

This comment has been minimized.

@grokys

grokys Apr 16, 2018

Contributor

Because this class isn't used directly by the designer, it's used by other sample data view models, so they set the sample data. See https://github.com/github/VisualStudio/pull/1491/files/692beca0cb8b2139246cfa8ce4b71e9e88ba037e#diff-b62ccfc33b879b821bb668fd4cdbee75R37 for example.

// HACK: We need to wait here for the diff view to set itself up and move its cursor
// to the first changed line. There must be a better way of doing this.
await Task.Delay(1500);

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

Should we make a new issue to address this later?

This comment has been minimized.

@grokys

grokys Apr 16, 2018

Contributor

Yes, probably, we've had this hack in there since inline comments was first introduced.

statusBar.ShowMessage(message + ": " + e.Message);
}
void AddBufferTag(

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

What is the buffer tag? Is it the "PR 1491" that is to the right of the file name on the right diff?

This comment has been minimized.

@grokys

grokys Apr 16, 2018

Contributor

It's a way of attaching our own data to a text buffer describing what PR/commit/file a diff view represents. It adds an instance of PullRequestTextBufferInfo as a property of the text buffer. That class is documented (though probably not well enough) instead of this method to avoid duplicating documentation. Might be worth documenting both though if it's confusing.

This comment has been minimized.

@sguthals

sguthals Apr 16, 2018

Contributor

Maybe worth just putting:

It adds an instance of PullRequestTextBufferInfo as a property of the text buffer.

}
catch (Exception)
{
// TODO: Show error.

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

Does this need to be added before the merge?

This comment has been minimized.

@grokys

grokys Apr 16, 2018

Contributor

Ah, yes, I will address this together with the double-click exceptions in a separate PR.

/// <summary>
/// Gets the GraphQL ID for the review.
/// </summary>
string NodeId { get; set; }

This comment has been minimized.

@sguthals

sguthals Apr 13, 2018

Contributor

Aha! So Node Id is for GraphQL. Why is that different from the Id of the review though still?

This comment has been minimized.

@grokys

grokys Apr 16, 2018

Contributor

Well... That's probably quite a long explanation ;) The short answer is that the REST API exposed database IDs whereas GraphQL uses global relay IDs. At the moment we're using both APIs so for certain models we need both IDs in order to interoperate. This will change hopefully very soon as we move more stuff to GraphQL.

This comment has been minimized.

@sguthals

sguthals Apr 16, 2018

Contributor

Oh got it - that makes sense for sure :) Thanks!

Merge pull request #1552 from github/fixes/1546-make-navigate-to-edit…
…or-discoverable

Make Navigate to Editor more discoverable

@jcansdale jcansdale dismissed stale reviews from sguthals and themself via c2b105e Apr 16, 2018

@grokys grokys merged commit c0dc9cb into master Apr 16, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

2.4.4 automation moved this from To do to Done Apr 16, 2018

@grokys grokys deleted the feature/pr-reviews-master branch Apr 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment