Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit 265e2dc

Browse files
grokysgrokys
committed
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
1 parent 447b30a commit 265e2dc

File tree

8 files changed

+64
-95
lines changed

8 files changed

+64
-95
lines changed

‎src/GitHub.App/SampleData/CommentThreadViewModelDesigner.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ public class CommentThreadViewModelDesigner : ViewModelBase, ICommentThreadViewM
1414
public IActorViewModel CurrentUser { get; set; }
1515
= new ActorViewModel { Login = "shana" };
1616

17-
public Task DeleteComment(int pullRequestId, int commentId) => Task.CompletedTask;
18-
public Task EditComment(string id, string body) => Task.CompletedTask;
19-
public Task PostComment(string body) => Task.CompletedTask;
17+
public Task DeleteComment(ICommentViewModel comment) => Task.CompletedTask;
18+
public Task EditComment(ICommentViewModel comment) => Task.CompletedTask;
19+
public Task PostComment(ICommentViewModel comment) => Task.CompletedTask;
2020
}
2121
}

‎src/GitHub.App/ViewModels/CommentThreadViewModel.cs‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public CommentThreadViewModel(
4747
{
4848
Guard.ArgumentNotNull(draftStore, nameof(draftStore));
4949

50-
this.DraftStore = draftStore;
50+
DraftStore = draftStore;
5151
this.timerScheduler = timerScheduler;
5252
}
5353

@@ -63,13 +63,13 @@ public CommentThreadViewModel(
6363
protected IMessageDraftStore DraftStore { get; }
6464

6565
/// <inheritdoc/>
66-
public abstract Task PostComment(string body);
66+
public abstract Task PostComment(ICommentViewModel comment);
6767

6868
/// <inheritdoc/>
69-
public abstract Task EditComment(string id, string body);
69+
public abstract Task EditComment(ICommentViewModel comment);
7070

7171
/// <inheritdoc/>
72-
public abstract Task DeleteComment(int pullRequestId, int commentId);
72+
public abstract Task DeleteComment(ICommentViewModel comment);
7373

7474
/// <summary>
7575
/// Adds a placeholder comment that will allow the user to enter a reply, and wires up
@@ -106,6 +106,18 @@ protected virtual CommentDraft BuildDraft(ICommentViewModel comment)
106106
null;
107107
}
108108

109+
protected async Task DeleteDraft(ICommentViewModel comment)
110+
{
111+
if (draftThrottles.TryGetValue(comment, out var throttle))
112+
{
113+
throttle.OnCompleted();
114+
draftThrottles.Remove(comment);
115+
}
116+
117+
var (key, secondaryKey) = GetDraftKeys(comment);
118+
await DraftStore.DeleteDraft(key, secondaryKey).ConfigureAwait(false);
119+
}
120+
109121
protected abstract (string key, string secondaryKey) GetDraftKeys(ICommentViewModel comment);
110122

111123
void PlaceholderChanged(ICommentViewModel placeholder, CommentEditState state, string body)
@@ -124,14 +136,7 @@ void PlaceholderChanged(ICommentViewModel placeholder, CommentEditState state, s
124136
}
125137
else if (state != CommentEditState.Editing)
126138
{
127-
if (draftThrottles.TryGetValue(placeholder, out var throttle))
128-
{
129-
throttle.OnCompleted();
130-
draftThrottles.Remove(placeholder);
131-
}
132-
133-
var (key, secondaryKey) = GetDraftKeys(placeholder);
134-
DraftStore.DeleteDraft(key, secondaryKey).Forget();
139+
DeleteDraft(placeholder).Forget();
135140
}
136141
}
137142

‎src/GitHub.App/ViewModels/CommentViewModel.cs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ async Task DoDelete()
219219
ErrorMessage = null;
220220
IsSubmitting = true;
221221

222-
await Thread.DeleteComment(PullRequestId, DatabaseId).ConfigureAwait(true);
222+
await Thread.DeleteComment(this).ConfigureAwait(true);
223223
}
224224
catch (Exception e)
225225
{
@@ -264,12 +264,14 @@ async Task DoCommitEdit()
264264

265265
if (Id == null)
266266
{
267-
await Thread.PostComment(Body).ConfigureAwait(true);
267+
await Thread.PostComment(this).ConfigureAwait(true);
268268
}
269269
else
270270
{
271-
await Thread.EditComment(Id, Body).ConfigureAwait(true);
271+
await Thread.EditComment(this).ConfigureAwait(true);
272272
}
273+
274+
EditState = CommentEditState.None;
273275
}
274276
catch (Exception e)
275277
{

‎src/GitHub.App/ViewModels/PullRequestReviewCommentThreadViewModel.cs‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,9 @@ public async Task InitializeNewAsync(
153153
AddPlaceholder(vm);
154154
}
155155

156-
public override async Task PostComment(string body)
156+
public override async Task PostComment(ICommentViewModel comment)
157157
{
158-
Guard.ArgumentNotNull(body, nameof(body));
158+
Guard.ArgumentNotNull(comment, nameof(comment));
159159

160160
if (IsNewThread)
161161
{
@@ -173,7 +173,7 @@ public override async Task PostComment(string body)
173173
}
174174

175175
await Session.PostReviewComment(
176-
body,
176+
comment.Body,
177177
File.CommitSha,
178178
File.RelativePath.Replace("\\", "/"),
179179
File.Diff,
@@ -182,21 +182,24 @@ await Session.PostReviewComment(
182182
else
183183
{
184184
var replyId = Comments[0].Id;
185-
await Session.PostReviewComment(body, replyId).ConfigureAwait(false);
185+
await Session.PostReviewComment(comment.Body, replyId).ConfigureAwait(false);
186186
}
187+
188+
await DeleteDraft(comment).ConfigureAwait(false);
187189
}
188190

189-
public override async Task EditComment(string id, string body)
191+
public override async Task EditComment(ICommentViewModel comment)
190192
{
191-
Guard.ArgumentNotNull(id, nameof(id));
192-
Guard.ArgumentNotNull(body, nameof(body));
193+
Guard.ArgumentNotNull(comment, nameof(comment));
193194

194-
await Session.EditComment(id, body).ConfigureAwait(false);
195+
await Session.EditComment(comment.Id, comment.Body).ConfigureAwait(false);
195196
}
196197

197-
public override async Task DeleteComment(int pullRequestId, int commentId)
198+
public override async Task DeleteComment(ICommentViewModel comment)
198199
{
199-
await Session.DeleteComment(pullRequestId, commentId).ConfigureAwait(false);
200+
Guard.ArgumentNotNull(comment, nameof(comment));
201+
202+
await Session.DeleteComment(comment.PullRequestId, comment.DatabaseId).ConfigureAwait(false);
200203
}
201204

202205
public static (string key, string secondaryKey) GetDraftKeys(

‎src/GitHub.Exports.Reactive/ViewModels/ICommentThreadViewModel.cs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,16 +22,19 @@ public interface ICommentThreadViewModel : IViewModel
2222
/// <summary>
2323
/// Called by a comment in the thread to post itself as a new comment to the API.
2424
/// </summary>
25-
Task PostComment(string body);
25+
/// <param name="comment">The comment to post.</param>
26+
Task PostComment(ICommentViewModel comment);
2627

2728
/// <summary>
2829
/// Called by a comment in the thread to post itself as an edit to a comment to the API.
2930
/// </summary>
30-
Task EditComment(string id, string body);
31+
/// <param name="comment">The comment to edit.</param>
32+
Task EditComment(ICommentViewModel comment);
3133

3234
/// <summary>
3335
/// Called by a comment in the thread to delete the comment on the API.
3436
/// </summary>
35-
Task DeleteComment(int pullRequestId, int commentId);
37+
/// <param name="comment">The comment to delete.</param>
38+
Task DeleteComment(ICommentViewModel comment);
3639
}
3740
}

‎src/GitHub.InlineReviews/ViewModels/InlineCommentPeekViewModel.cs‎

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,6 @@ async void LinesChanged(IReadOnlyList<Tuple<int, DiffSide>> lines)
165165

166166
async Task UpdateThread()
167167
{
168-
var placeholderBody = GetPlaceholderBodyToPreserve();
169-
170168
Thread = null;
171169
threadSubscription?.Dispose();
172170

@@ -190,17 +188,6 @@ async Task UpdateThread()
190188
await vm.InitializeNewAsync(session, file, lineNumber, side, true);
191189
}
192190

193-
if (!string.IsNullOrWhiteSpace(placeholderBody))
194-
{
195-
var placeholder = vm.Comments.LastOrDefault();
196-
197-
if (placeholder?.EditState == CommentEditState.Placeholder)
198-
{
199-
await placeholder.BeginEdit.Execute();
200-
placeholder.Body = placeholderBody;
201-
}
202-
}
203-
204191
Thread = vm;
205192
}
206193

@@ -220,17 +207,5 @@ async Task SessionChanged(IPullRequestSession pullRequestSession)
220207
await UpdateThread();
221208
}
222209
}
223-
224-
string GetPlaceholderBodyToPreserve()
225-
{
226-
var lastComment = Thread?.Comments.LastOrDefault();
227-
228-
if (lastComment?.EditState == CommentEditState.Editing)
229-
{
230-
if (!lastComment.IsSubmitting) return lastComment.Body;
231-
}
232-
233-
return null;
234-
}
235210
}
236211
}

‎test/GitHub.App.UnitTests/ViewModels/CommentThreadViewModelTests.cs‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,23 @@ public async Task DoesntSaveDraftForNonEditingComment()
5555
}
5656
}
5757

58+
[Test]
59+
public async Task CommitEditDeletesDraft()
60+
{
61+
using (TestUtils.WithScheduler(Scheduler.CurrentThread))
62+
{
63+
var drafts = Substitute.For<IMessageDraftStore>();
64+
var target = CreateTarget(drafts: drafts);
65+
66+
await target.AddPlaceholder(false);
67+
68+
drafts.ClearReceivedCalls();
69+
await target.Comments[0].CommitEdit.Execute();
70+
71+
await drafts.Received().DeleteDraft("file.cs", "10");
72+
}
73+
}
74+
5875
[Test]
5976
public async Task CancelEditDeletesDraft()
6077
{
@@ -94,9 +111,9 @@ public async Task AddPlaceholder(bool isEditing)
94111
AddPlaceholder(c);
95112
}
96113

97-
public override Task DeleteComment(int pullRequestId, int commentId) => throw new NotImplementedException();
98-
public override Task EditComment(string id, string body) => throw new NotImplementedException();
99-
public override Task PostComment(string body) => throw new NotImplementedException();
114+
public override Task DeleteComment(ICommentViewModel comment) => Task.CompletedTask;
115+
public override Task EditComment(ICommentViewModel comment) => Task.CompletedTask;
116+
public override Task PostComment(ICommentViewModel comment) => Task.CompletedTask;
100117
protected override (string key, string secondaryKey) GetDraftKeys(ICommentViewModel comment) => ("file.cs", "10");
101118
}
102119

‎test/GitHub.InlineReviews.UnitTests/ViewModels/InlineCommentPeekViewModelTests.cs‎

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -173,42 +173,6 @@ public async Task RefreshesWhenSessionInlineCommentThreadsChanges()
173173
Assert.That(target.Thread.Comments.Count, Is.EqualTo(3));
174174
}
175175

176-
[Test]
177-
public async Task RetainsCommentBeingEditedWhenSessionRefreshed()
178-
{
179-
using (TestUtils.WithScheduler(Scheduler.CurrentThread))
180-
{
181-
var sessionManager = CreateSessionManager();
182-
var peekSession = CreatePeekSession();
183-
var target = new InlineCommentPeekViewModel(
184-
CreatePeekService(lineNumber: 10),
185-
CreatePeekSession(),
186-
sessionManager,
187-
Substitute.For<INextInlineCommentCommand>(),
188-
Substitute.For<IPreviousInlineCommentCommand>(),
189-
CreateFactory());
190-
191-
await target.Initialize();
192-
193-
Assert.That(target.Thread.Comments.Count, Is.EqualTo(2));
194-
195-
var placeholder = target.Thread.Comments.Last();
196-
placeholder.BeginEdit.Execute().Subscribe();
197-
placeholder.Body = "Comment being edited";
198-
199-
var file = await sessionManager.GetLiveFile(
200-
RelativePath,
201-
peekSession.TextView,
202-
peekSession.TextView.TextBuffer);
203-
AddCommentToExistingThread(file);
204-
205-
placeholder = target.Thread.Comments.Last();
206-
Assert.That(target.Thread.Comments.Count, Is.EqualTo(3));
207-
Assert.That(placeholder.EditState, Is.EqualTo(CommentEditState.Editing));
208-
Assert.That(placeholder.Body, Is.EqualTo("Comment being edited"));
209-
}
210-
}
211-
212176
[Test]
213177
public async Task CommittingEditDoesntRetainSubmittedCommentInPlaceholderAfterPost()
214178
{

0 commit comments

Comments
 (0)