opendev/gerrit
Code Issues Proposed changes

Fixes comment issues with multiple editing comments

Browse Source 
This change addresses a few issues that existed due to multiple comments
in an editing state at the same time.

1) Fixes issue where if you create two replies and add text to the
second reply, then delete the first one, the text in the second textarea
gets removed.

2) Fixes issue where if you reply and add text, then reply again with
the first draft still editing, the second draft gets populated with the
message from the first comment.

3) Fixes issue where if you have multiple replies and delete one of
them, local storage gets erased. This change sets local storage to the
value of the first editing message found after deleting the other one
(if it exists).

Bug: Issue 4409
Change-Id: Ib5913a34a79783a4a87b4a298e25b02fc587b8dd
This commit is contained in:
Becky Siegel 2016-12-22 15:04:11 -08:00
parent 883ec22964
commit 43c5ed8045
5 changed files with 244 additions and 2 deletions

2
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html

@ -16,6 +16,7 @@ limitations under the License.
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../shared/gr-storage/gr-storage.html">
<link rel="import" href="../gr-diff-comment/gr-diff-comment.html">
<dom-module id="gr-diff-comment-thread">
@ -45,6 +46,7 @@ limitations under the License.
</template>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="storage"></gr-storage>
</template>
<script src="gr-diff-comment-thread.js"></script>
</dom-module>

24
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js

@ -128,6 +128,15 @@
_createReplyComment: function(parent, content, opt_isEditing) {
var reply = this._newReply(parent.id, parent.line, content);
// If there is currently a comment in an editing state, add an attribute
// so that the gr-diff-comment knows not to populate the draft text.
for (var i = 0; i < this.comments.length; i++) {
if (this.comments[i].__editing) {
reply.__otherEditing = true;
break;
}
}
if (opt_isEditing) {
reply.__editing = true;
}
@ -224,6 +233,21 @@
if (this.comments.length == 0) {
this.fire('thread-discard', {lastComment: comment});
}
// Check to see if there are any other open comments getting edited and
// set the local storage value to its message value.
for (var i = 0; i < this.comments.length; i++) {
if (this.comments[i].__editing) {
var commentLocation = {
changeNum: this.changeNum,
patchNum: this.patchNum,
path: this.comments[i].path,
line: this.comments[i].line,
};
return this.$.storage.setDraftComment(commentLocation,
this.comments[i].message);
}
}
},
_handleCommentUpdate: function(e) {

164
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html

@ -288,6 +288,170 @@ limitations under the License.
draftEl.fire('comment-discard', null, {bubbles: false});
});
test('first editing comment does not add __otherEditing attribute',
function(done) {
var commentEl = element.$$('gr-diff-comment');
element.comments = [{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
id: 'baf0414d_60047215',
line: 5,
message: 'is this a crossover episode!?',
updated: '2015-12-08 19:48:33.843000000',
__draft: true,
}];
flushAsynchronousOperations();
commentEl.addEventListener('create-reply-comment', function() {
var editing = element._orderedComments.filter(function(c) {
return c.__editing == true;
});
assert.equal(editing.length, 1);
assert.equal(!!editing[0].__otherEditing, false);
done();
});
commentEl.fire('create-reply-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('two editing comments adds __otherEditing attribute', function(done) {
var commentEl = element.$$('gr-diff-comment');
element.comments = [{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
id: 'baf0414d_60047215',
line: 5,
message: 'is this a crossover episode!?',
updated: '2015-12-08 19:48:33.843000000',
__editing: true,
__draft: true,
}];
flushAsynchronousOperations();
commentEl.addEventListener('create-reply-comment', function() {
var editing = element._orderedComments.filter(function(c) {
return c.__editing == true;
});
assert.equal(editing.length, 2);
assert.equal(editing[1].__otherEditing, true);
done();
});
commentEl.fire('create-reply-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('When editing other comments, local storage set after discard',
function(done) {
element.changeNum = '42';
element.patchNum = '1';
element.comments = [{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
id: 'baf0414d_60047215',
in_reply_to: 'baf0414d_60047215',
line: 5,
message: 'is this a crossover episode!?',
updated: '2015-12-08 19:48:31.843000000',
},
{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
__draftID: '1',
in_reply_to: 'baf0414d_60047215',
line: 5,
message: 'yes',
updated: '2015-12-08 19:48:32.843000000',
__draft: true,
__editing: true,
},
{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
__draftID: '2',
in_reply_to: 'baf0414d_60047215',
line: 5,
message: 'no',
updated: '2015-12-08 19:48:33.843000000',
__draft: true,
__editing: true,
}];
var storageStub = sinon.stub(element.$.storage, 'setDraftComment');
flushAsynchronousOperations();
var draftEl =
Polymer.dom(element.root).querySelectorAll('gr-diff-comment')[1];
assert.ok(draftEl);
draftEl.addEventListener('comment-discard', function() {
assert.isTrue(storageStub.called);
storageStub.restore();
done();
});
draftEl.fire('comment-discard', null, {bubbles: false});
});
test('When not editing other comments, local storage not set after discard',
function(done) {
element.changeNum = '42';
element.patchNum = '1';
element.comments = [{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
id: 'baf0414d_60047215',
line: 5,
message: 'is this a crossover episode!?',
updated: '2015-12-08 19:48:31.843000000',
},
{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
__draftID: '1',
in_reply_to: 'baf0414d_60047215',
line: 5,
message: 'yes',
updated: '2015-12-08 19:48:32.843000000',
__draft: true,
__editing: true,
},
{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
__draftID: '2',
in_reply_to: 'baf0414d_60047215',
line: 5,
message: 'no',
updated: '2015-12-08 19:48:33.843000000',
__draft: true,
}];
var storageStub = sinon.stub(element.$.storage, 'setDraftComment');
flushAsynchronousOperations();
var draftEl =
Polymer.dom(element.root).querySelectorAll('gr-diff-comment')[1];
assert.ok(draftEl);
draftEl.addEventListener('comment-discard', function() {
assert.isFalse(storageStub.called);
storageStub.restore();
done();
});
draftEl.fire('comment-discard', null, {bubbles: false});
});
test('comment-update', function() {
var commentEl = element.$$('gr-diff-comment');
var updatedComment = {

12
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js

@ -281,6 +281,9 @@
_messageTextChanged: function(newValue, oldValue) {
if (!this.comment || (this.comment && this.comment.id)) { return; }
// Keep comment.message in sync so that gr-diff-comment-thread is aware
// of the current message in the case that another comment is deleted.
this.comment.message = this._messageText || '';
this.debounce('store', function() {
var message = this._messageText;
@ -356,7 +359,8 @@
_handleCancel: function(e) {
e.preventDefault();
if (this.comment.message == null || this.comment.message.length == 0) {
if (this.comment.message === null ||
this.comment.message.trim().length === 0) {
this._fireDiscard();
return;
}
@ -408,7 +412,11 @@
_loadLocalDraft: function(changeNum, patchNum, comment) {
// Only apply local drafts to comments that haven't been saved
// remotely, and haven't been given a default message already.
if (!comment || comment.id || comment.message) {
//
// Don't get local draft if there is another comment that is currently
// in an editing state.
if (!comment || comment.id || comment.message || comment.__otherEditing) {
delete comment.__otherEditing;
return;
}

44
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html

@ -147,6 +147,49 @@ limitations under the License.
showStub.restore();
});
test('message is not retrieved from storage when other editing is true',
function(done) {
var storageStub = sandbox.stub(element.$.storage, 'getDraftComment');
var loadSpy = sandbox.spy(element, '_loadLocalDraft');
element.changeNum = 1;
element.patchNum = 1;
element.comment = {
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
line: 5,
__otherEditing: true,
};
flush(function() {
assert.isTrue(loadSpy.called);
assert.isFalse(storageStub.called);
done();
});
});
test('message is retrieved from storage when there is no other editing',
function(done) {
var storageStub = sandbox.stub(element.$.storage, 'getDraftComment');
var loadSpy = sandbox.spy(element, '_loadLocalDraft');
element.changeNum = 1;
element.patchNum = 1;
element.comment = {
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
line: 5,
};
flush(function() {
assert.isTrue(loadSpy.called);
assert.isTrue(storageStub.called);
done();
});
});
test('comment expand and collapse', function() {
element.collapsed = true;
assert.isFalse(isVisible(element.$$('gr-formatted-text')),
@ -436,6 +479,7 @@ limitations under the License.
__editing: true,
line: 5,
path: '/path/to/file',
message: 'good news, everyone!',
},
patchNum: 1,
},