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

Save drafts of comments #1994

Merged
merged 21 commits into from Oct 29, 2018

Conversation

Projects
None yet
4 participants
@grokys
Contributor

grokys commented Oct 17, 2018

Save drafts of inline comments, PR reviews and PRs.

Note: This feature required a refactoring of the comment view models because they now need async initialization and to be available from GitHub.App. This part of the PR has been submitted separately as #1993 to ease review. The two PRs can alternatively be reviewed as one if that's more convenient.

As described in #1905, it is easy to lose a comment that you're working on if you close the diff view accidentally. This PR saves drafts of comments as they are being written to an SQLite database.

In addition to saving drafts of inline comments, it also saves comments to PR reviews and PRs themselves.

The comments are written to an SQLite database directly instead of going through Akavache because in the case of inline reviews, there can be many drafts in progress on a separate file. When a diff is opened we need to look for any comments present on that file and show the most recent. That use-case didn't fit well with Akavache (being a pure key/value store).

Testing

Inline Comments

  • Open a PR
  • Open the diff of a file
  • Start adding a comment
  • Close the comment by closing the peek view, or the document tab
  • Reopen the diff
  • You should see the comment displayed in edit mode with the draft of the comment you were previously writing

PR reviews

  • Open a PR
  • Click "Add your review"
  • Start adding a review
  • Click the "Back" button and navigate to a different PR
  • Click the "Back" button and navigate to the original PR
  • Click "Add your review"
  • You should see the the draft of the review you were previously writing

PRs

  • Click "Create new" at the top of the PR list
  • Start adding a PR title/description
  • Close VS
  • Restart VS and click "Create new" again
  • You should see the the draft of the PR you were previously writing

Depends on #1993
Fixes #1905

grokys added some commits Oct 12, 2018

Refactor comment view models.
- Renamed `InlineCommentThreadViewModel` -> `PullRequestReviewCommentThreadViewModel`
- Moved `PullRequestReviewCommentThreadViewModel` to GitHub.App
- Make comment view models MEF creatable
- Add `InitializeAsync` methods to initialize them with data.
Move InlineCommentPeekService to GitHub.App.
So that it can be used everywhere. Had to refactor it slightly to remove some dependencies.
@codecov

This comment has been minimized.

codecov bot commented Oct 18, 2018

Codecov Report

Merging #1994 into master will decrease coverage by 1.17%.
The diff coverage is 56.56%.

@@            Coverage Diff             @@
##           master    #1994      +/-   ##
==========================================
- Coverage   40.51%   39.34%   -1.18%     
==========================================
  Files         372      410      +38     
  Lines       16099    17573    +1474     
  Branches     2216     2425     +209     
==========================================
+ Hits         6523     6914     +391     
- Misses       9054    10112    +1058     
- Partials      522      547      +25
Impacted Files Coverage Δ
...rc/GitHub.App/Services/InlineCommentPeekService.cs 0% <0%> (ø)
src/GitHub.App/Services/SqliteMessageDraftStore.cs 0% <0%> (ø)
...ub.InlineReviews/Peek/InlineCommentPeekableItem.cs 0% <0%> (ø) ⬆️
...Reviews/Commands/InlineCommentNavigationCommand.cs 0% <0%> (ø) ⬆️
...ub.InlineReviews/Tags/InlineCommentGlyphFactory.cs 0% <0%> (ø) ⬆️
...GitHub.App/Models/Drafts/PullRequestReviewDraft.cs 100% <100%> (ø)
...neReviews/ViewModels/InlineCommentPeekViewModel.cs 76.23% <100%> (-1.24%) ⬇️
...App/Models/Drafts/PullRequestReviewCommentDraft.cs 100% <100%> (ø)
...pp/ViewModels/PullRequestReviewCommentViewModel.cs 94.87% <100%> (+0.13%) ⬆️
src/GitHub.App/Models/Drafts/PullRequestDraft.cs 100% <100%> (ø)
... and 51 more
@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Oct 18, 2018

This is looking great! Really enjoying using this feature so far.

When leaving an inline comment, the comment posts successfully and then a new comment is drafted with the same text.
screen shot 2018-10-18 at 9 46 26 am

If I leave a review comment, I'm still given the option to start a review and selecting that option throws an error.
screen shot 2018-10-18 at 1 15 58 pm

grokys added some commits Oct 19, 2018

Fix drafts being retained after submission.
- Make posting a PR review comment delete the draft
- Remove the placeholder logic from `InlineCommentPeekViewModel` as this is now handled by the draft system
@grokys

This comment has been minimized.

Contributor

grokys commented Oct 19, 2018

Oops, sorry about that @meaghanlewis - I was sure I tested those things, but must have got messed up again at some point. Should be fixed now.

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Oct 19, 2018

Thanks @grokys. This looks great to me.

@jcansdale

This comment has been minimized.

Collaborator

jcansdale commented Oct 26, 2018

All of the inline comment stuff seems to be working great! I cloned and opened solutions and my comments are still there. 🎉

With the pull request saving functionality, I tried the following:

  1. Create a new pull request:
    image

  2. Pull request created
    image

  3. Click Create New again
    image

The details from the previously created pull request are restored.

Are you missing a clear when a pull request is created?

@jcansdale

This is looking great!

Just a few comments on the tests. Is WithScheduler still necessary? There are a bunch of tests that still use it. I few other minor commands inline on the tests.

[Test]
public async Task SavesDraftForEditingComment()
{
using (TestUtils.WithScheduler(Scheduler.CurrentThread))

This comment has been minimized.

@jcansdale

jcansdale Oct 26, 2018

Collaborator

Do we still need to use WithScheduler after your unit testing fix?

@@ -115,6 +171,13 @@ IEnumerable<InlineCommentModel> CreateComments(params string[] bodies)
}
}
IPullRequestSessionFile CreateFile(string relativePath = "file.cs")

This comment has been minimized.

@jcansdale

jcansdale Oct 26, 2018

Collaborator

This can be static.

result.RelativePath.Returns(relativePath);
return result;
}
IViewViewModelFactory CreateFactory()

This comment has been minimized.

@jcansdale

jcansdale Oct 26, 2018

Collaborator

This can be static.

@@ -327,12 +291,13 @@ InlineCommentModel CreateComment(string body)
IViewViewModelFactory CreateFactory()

This comment has been minimized.

@jcansdale

jcansdale Oct 26, 2018

Collaborator

This can be static.

@@ -0,0 +1,165 @@
////using System.Collections.Generic;

This comment has been minimized.

@jcansdale

jcansdale Oct 26, 2018

Collaborator

Looks like commented out test file has come back?

grokys and others added some commits Oct 29, 2018

@jcansdale

This is looking good. LGTM!

@jcansdale jcansdale merged commit 82a2ccd into master Oct 29, 2018

4 checks passed

codecov/patch 56.56% of diff hit (target 40.51%)
Details
codecov/project Absolute coverage decreased by -1.17% but relative coverage increased by +16.04% compared to adb4119
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jcansdale jcansdale deleted the feature/save-drafts branch Oct 29, 2018

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