Use GraphQL for Pull Request Models. #1712

grokys · 2018-05-31T16:47:21Z

Description

Previously we were using a mix of REST and GraphQL for Pull Requests and Pull Request Reviews, and our data model didn't fit either very well. This PR:

Adds a PullRequestDetailModel which is modeled after what we're hoping will be the future shape of the GraphQL pull request/reviews API
Moves reading pull request details out of ModelService and into PullRequestSession - this is now the only place to get hold of a PR details model. Doing this means that all parts of the extension always agree about the state of a pull request, and prevents some unnecessary API reads that we were carrying out before - our UI should feel a lot snappier now!
Reads PullRequestDetailModel using GraphQL. Because the GraphQL API for reviews and comments isn't complete yet, after reading this we massage the model into what should be the final shape after reading (see BuildPullRequestThreads

The old PullRequestModel is still there (although in a cut-down form) and still read using REST. It is now used just for the PR list. This will be moved to GraphQL in a later PR.

Testing

Basically, everything around pull requests/reviews/comments should work as before!

TODO

Make adding comments work - Octokit.GraphQL is currently trying to use the Mutation object for auto-paging and failing
- Worked around this by re-reading the PR details after submitting a comment. This might not be ideal, so might need to be revisited. The Octokit.GraphQL issue (Auto-paging doesn't work in mutations. octokit/octokit.graphql.net#93) also needs to be fixed.
Update with changes from Adding Edit/Delete functions to inline comments #1701 once merged
Add missing XML doc comments

Now using a data model based on GraphQL (or at least a hypothetical future version of the GraphQL API based on conversations with that team).

Should really be using `ActorAvatarView` here but our mess of assemblies means that it'd have to be moved. Fix later.

Don't do this. It makes all PRs get returned with the same number.

If the basic logic doesn't work, likely the data is bad.

...`PullRequestUserReviewsViewModelTests`. The view now loads instantly :D

Mainly merging #1701 Conflicts: src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs src/GitHub.InlineReviews/SampleData/CommentThreadViewModelDesigner.cs src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs src/GitHub.InlineReviews/Services/PullRequestSession.cs src/GitHub.InlineReviews/Services/PullRequestSessionService.cs src/GitHub.InlineReviews/ViewModels/CommentThreadViewModel.cs src/GitHub.InlineReviews/ViewModels/CommentViewModel.cs src/GitHub.InlineReviews/ViewModels/ICommentThreadViewModel.cs test/GitHub.InlineReviews.UnitTests/ViewModels/PullRequestReviewCommentViewModelTests.cs

Because adding e.g. a PR review comment affects both the `PullRequestReviewModel` _and_ the `PullRequestDetailModel`, re-read the PR when a comment mutation is executed.

The placeholder should be in `Editing` state when it is shown.

jcansdale · 2018-06-12T16:22:50Z

test/GitHub.InlineReviews.UnitTests/ViewModels/NewInlineCommentThreadViewModelTests.cs

@@ -10,6 +10,11 @@ namespace GitHub.InlineReviews.UnitTests.ViewModels
 {
    public class NewInlineCommentThreadViewModelTests
    {
+        public NewInlineCommentThreadViewModelTests()
+        {
+            Splat.ModeDetector.Current.SetInUnitTestRunner(true);


We should probably move this into an assembly wide setup for the whole test assembly. This is possible with NUnit in a way it wasn't with xUnit.

For example we could move it into AssemblyInfo.cs like this:

VisualStudio/test/GitHub.App.UnitTests/Properties/AssemblyInfo.cs

Line 45 in ebb900e

Splat.ModeDetector.Current.SetInUnitTestRunner(true);

Otherwise it's too easy to forget and end up with hanging or not depending on the order they're executed. 😨

Alternatively we could do this as part of #1721, which I think would make more sense. 😉

jcansdale

I like how you've cleaned up the models. :-) Just a few questions, no blockers.

jcansdale · 2018-06-12T18:56:18Z

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

@@ -105,6 +105,7 @@ public void Dispose()
                { string.Empty, new PullRequestDirectoryNode(string.Empty) }
            };

+            await Task.Delay(0);


What is the Task.Delay(0) for? Would this behave the same with Task.Yield()?

Oops, that was left in from when I commented out the whole of the method. Putting a Task.Delay(0) in was my way of preventing the "method doesn't use await" error. Will remove.

jcansdale · 2018-06-12T19:33:56Z

src/GitHub.InlineReviews/Services/PullRequestSessionService.cs

+            if (readPullRequest == null)
+            {
+                readPullRequest = new Query()
+                    .Repository(Var(nameof(owner)), Var(nameof(name)))


I'm curious why you're caching the compiled query here but not other queries? I've found an easy way to automatically handle the named arguments and caching if this is something users of GraphQL should be doing.

Basically just because this was written from scratch and the other queries were just modified. We should probably be doing the same with the other queries, not sure if that should be in this PR or another?

Can I show you my GraphQL prototype before we do the the same with the other queries? I'd like to find out if you think it's a good idea in principle before developing it any further. So maybe push this to another PR.

I wanted to get your thoughts on this:
octokit/octokit.graphql.net#104

jcansdale · 2018-06-12T19:42:54Z

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

@@ -15,9 +16,9 @@

    <d:DesignData.DataContext>
        <vm:PullRequestReviewSummaryViewModel Id="1" State="Pending" FileCommentCount="2">
-            <vm:PullRequestReviewSummaryViewModel.User>
+            <!--<vm:PullRequestReviewSummaryViewModel.User>


Is this commented out for a reason?

jcansdale

There seems to be an issue when attempting to leave comments on some files.

Above I'm attempting to leave a comment on the following line of this PR:
https://github.com/jcansdale/VisualStudio/blob/df6aa658a185620d61e3398656871cf27c3ae02f/src/GitHub.App/ViewModels/GitHubPane/PullRequestReviewCommentViewModel.cs#L66

grokys · 2018-06-13T12:19:55Z

@jcansdale are you sure this was caused by this PR? This doesn't look like anything that this PR should have changed. Can you leave a comment in the same position on master?

meaghanlewis · 2018-06-13T16:24:16Z

@grokys @jcansdale I just tried to leave a comment in the same position on master and I do see this same error happening.

jcansdale

Sorry about the position is invalid false alarm!

This PR looks good. 👍

Conflicts: src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs src/GitHub.InlineReviews/Services/PullRequestSession.cs src/GitHub.InlineReviews/Services/PullRequestSessionService.cs src/GitHub.InlineReviews/ViewModels/InlineCommentThreadViewModel.cs test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionTests.cs test/GitHub.InlineReviews.UnitTests/ViewModels/InlineCommentThreadViewModelTests.cs

grokys added 7 commits May 31, 2018 15:32

Initial refactoring of PR-related models.

71c438d

Now using a data model based on GraphQL (or at least a hypothetical future version of the GraphQL API based on conversations with that team).

Fix display of avatar in CommentView.

4d939c3

Should really be using `ActorAvatarView` here but our mess of assemblies means that it'd have to be moved. Fix later.

Factor out creating threads to separate method.

7e04855

D'oh.

5b2b013

Don't do this. It makes all PRs get returned with the same number.

Don't try to be clever about creating threads.

3b5526b

If the basic logic doesn't work, likely the data is bad.

Remove ModelService dependency from...

a265a1c

...`PullRequestUserReviewsViewModelTests`. The view now loads instantly :D

Reinstated "Add your review".

3c4b204

grokys added the WIP label May 31, 2018

grokys and others added 3 commits June 1, 2018 11:40

Re-read the PR on mutation.

62a7bce

Because adding e.g. a PR review comment affects both the `PullRequestReviewModel` _and_ the `PullRequestDetailModel`, re-read the PR when a comment mutation is executed.

Added some XML documentation.

128315e

grokys mentioned this pull request Jun 11, 2018

Missing buttons and errors when deleting or editing a comment #1723

Merged

StanleyGoldman added a commit that referenced this pull request Jun 11, 2018

Grafting code from #1712

27b2ed5

grokys added 2 commits June 12, 2018 10:52

Set IsPending state on comments.

1ef5767

Make comment links work again.

104fa72

grokys changed the title ~~WIP: Refactoring Pull Request Models.~~ Refactoring Pull Request Models. Jun 12, 2018

grokys added 3 commits June 12, 2018 15:59

Fix CA errors.

d4de540

Merge branch 'master' into refactor/pr-models

41338fc

Fix failing tests.

e849c1b

grokys force-pushed the refactor/pr-models branch from 4bdfc55 to e849c1b Compare June 12, 2018 15:21

Fix incorrect test.

df6aa65

The placeholder should be in `Editing` state when it is shown.

jcansdale reviewed Jun 12, 2018

View reviewed changes

Removed leftovers from the refactoring.

43877a7

This was referenced Jun 13, 2018

Nest review comments under reviews. #1613

Closed

WIP: Read PR details using GraphQL #1648

Closed

jcansdale suggested changes Jun 13, 2018

View reviewed changes

meaghanlewis mentioned this pull request Jun 14, 2018

Unable to leave inline comment on some lines #1731

Open

jcansdale approved these changes Jun 14, 2018

View reviewed changes

meaghanlewis added this to the 2.5.4 milestone Jun 14, 2018

grokys mentioned this pull request Jun 19, 2018

Speed up PR list and other improvements #1737

Merged

Merge branch 'master' into refactor/pr-models

d1bc28c

grokys merged commit d5ddf0d into master Jun 21, 2018

grokys changed the title ~~Refactoring Pull Request Models.~~ Use GraphQL for Pull Request Models. Jul 10, 2018

grokys mentioned this pull request Jul 10, 2018

Fix standalone comment replies #1772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use GraphQL for Pull Request Models. #1712

Use GraphQL for Pull Request Models. #1712

grokys commented May 31, 2018 •

edited

Loading

jcansdale Jun 12, 2018

jcansdale Jun 12, 2018

jcansdale left a comment

jcansdale Jun 12, 2018

grokys Jun 13, 2018

jcansdale Jun 12, 2018

grokys Jun 13, 2018

jcansdale Jun 13, 2018

jcansdale Jun 20, 2018

jcansdale Jun 12, 2018

grokys Jun 13, 2018

jcansdale left a comment

grokys commented Jun 13, 2018

meaghanlewis commented Jun 13, 2018

jcansdale left a comment

Use GraphQL for Pull Request Models. #1712

Use GraphQL for Pull Request Models. #1712

Conversation

grokys commented May 31, 2018 • edited Loading

Description

Testing

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcansdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcansdale left a comment

Choose a reason for hiding this comment

grokys commented Jun 13, 2018

meaghanlewis commented Jun 13, 2018

jcansdale left a comment

Choose a reason for hiding this comment

grokys commented May 31, 2018 •

edited

Loading