Skip to content
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

Use GraphQL for Pull Request Models. #1712

Merged
merged 19 commits into from Jun 21, 2018

Conversation

Projects
None yet
3 participants
@grokys
Copy link
Contributor

grokys commented May 31, 2018

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 (octokit/octokit.graphql.net#93) also needs to be fixed.
  • Update with changes from #1701 once merged
  • Add missing XML doc comments

grokys added some commits May 31, 2018

Initial refactoring of PR-related models.
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.
Should really be using `ActorAvatarView` here but our mess of assemblies means that it'd have to be moved. Fix later.
D'oh.
Don't do this. It makes all PRs get returned with the same number.
Don't try to be clever about creating threads.
If the basic logic doesn't work, likely the data is bad.
Remove ModelService dependency from...
...`PullRequestUserReviewsViewModelTests`. The view now loads instantly :D

@grokys grokys added the WIP label May 31, 2018

grokys added some commits Jun 1, 2018

Merge branch 'master' into refactor/pr-models
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
Re-read the PR on mutation.
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.

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

grokys added some commits Jun 12, 2018

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

grokys added some commits Jun 12, 2018

@grokys grokys force-pushed the refactor/pr-models branch from 4bdfc55 to e849c1b Jun 12, 2018

Fix incorrect test.
The placeholder should be in `Editing` state when it is shown.
@@ -10,6 +10,11 @@ namespace GitHub.InlineReviews.UnitTests.ViewModels
{
public class NewInlineCommentThreadViewModelTests
{
public NewInlineCommentThreadViewModelTests()
{
Splat.ModeDetector.Current.SetInUnitTestRunner(true);

This comment has been minimized.

@jcansdale

jcansdale Jun 12, 2018

Collaborator

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:

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. 😨

This comment has been minimized.

@jcansdale

jcansdale Jun 12, 2018

Collaborator

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

@jcansdale
Copy link
Collaborator

jcansdale left a comment

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

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

await Task.Delay(0);

This comment has been minimized.

@jcansdale

jcansdale Jun 12, 2018

Collaborator

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

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Author Contributor

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.

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

This comment has been minimized.

@jcansdale

jcansdale Jun 12, 2018

Collaborator

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.

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Author Contributor

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?

This comment has been minimized.

@jcansdale

jcansdale Jun 13, 2018

Collaborator

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.

This comment has been minimized.

@jcansdale

jcansdale Jun 20, 2018

Collaborator

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

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

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

This comment has been minimized.

@jcansdale

jcansdale Jun 12, 2018

Collaborator

Is this commented out for a reason?

This comment has been minimized.

@grokys

grokys Jun 13, 2018

Author Contributor

Fixed.

@jcansdale
Copy link
Collaborator

jcansdale left a comment

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

image

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

This comment has been minimized.

Copy link
Contributor Author

grokys commented Jun 13, 2018

@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

This comment has been minimized.

Copy link
Contributor

meaghanlewis commented Jun 13, 2018

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

@jcansdale
Copy link
Collaborator

jcansdale left a comment

Sorry about the position is invalid false alarm!

This PR looks good. 👍

@meaghanlewis meaghanlewis added this to the 2.5.4 milestone Jun 14, 2018

Merge branch 'master' into refactor/pr-models
 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 grokys merged commit d5ddf0d into master Jun 21, 2018

2 checks passed

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.