Display comment resolve state This change tracks and exposes the resolved state of a comment thread without exposing the UI for modifying that state. This enables features to be built out while the API request does not exist in the backend. Feature: Issue 4879 Change-Id: If002035024920a7762519cedf5a869221bbbc3c8
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html index ab6854f..1ed7ac7 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
@@ -23,13 +23,18 @@ <template> <style> :host { - background-color: #ffd; border: 1px solid #bbb; display: block; white-space: normal; } + #container { + background-color: #fcfad6; + } + #container.unresolved { + background-color: #fcfaa6; + } </style> - <div id="container"> + <div id="container" class$="[[_computeHostClass(_unresolved)]]"> <template id="commentList" is="dom-repeat" items="[[_orderedComments]]" as="comment"> <gr-diff-comment comment="{{comment}}"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js index 8ef395f..d043a68 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
@@ -45,6 +45,10 @@ _showActions: Boolean, _orderedComments: Array, + _unresolved: { + type: Boolean, + notify: true, + }, }, behaviors: [ @@ -56,7 +60,7 @@ }, observers: [ - '_commentsChanged(comments.splices)', + '_commentsChanged(comments.*)', ], keyBindings: { @@ -83,6 +87,7 @@ addDraft: function(opt_lineNum, opt_range) { var draft = this._newDraft(opt_lineNum, opt_range); draft.__editing = true; + draft.unresolved = true; this.push('comments', draft); }, @@ -92,6 +97,11 @@ _commentsChanged: function(changeRecord) { this._orderedComments = this._sortedComments(this.comments); + this._unresolved = this._getLastComment().unresolved; + }, + + _getLastComment: function() { + return this._orderedComments[this._orderedComments.length - 1] || {}; }, _handleEKey: function(e) { @@ -125,11 +135,13 @@ }); }, - _createReplyComment: function(parent, content, opt_isEditing) { + _createReplyComment: function(parent, content, opt_isEditing, + opt_unresolved) { var reply = this._newReply( this._orderedComments[this._orderedComments.length - 1].id, parent.line, - content); + content, + opt_unresolved); // 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. @@ -162,17 +174,17 @@ var msg = comment.message; quoteStr = '> ' + msg.replace(NEWLINE_PATTERN, '\n> ') + '\n\n'; } - this._createReplyComment(comment, quoteStr, true); + this._createReplyComment(comment, quoteStr, true, comment.unresolved); }, _handleCommentAck: function(e) { var comment = e.detail.comment; - this._createReplyComment(comment, 'Ack'); + this._createReplyComment(comment, 'Ack', false, comment.unresolved); }, _handleCommentDone: function(e) { var comment = e.detail.comment; - this._createReplyComment(comment, 'Done'); + this._createReplyComment(comment, 'Done', false, false); }, _handleCommentFix: function(e) { @@ -180,7 +192,7 @@ var msg = comment.message; var quoteStr = '> ' + msg.replace(NEWLINE_PATTERN, '\n> ') + '\n\n'; var response = quoteStr + 'Please Fix'; - this._createReplyComment(comment, response); + this._createReplyComment(comment, response, false, true); }, _commentElWithDraftID: function(id) { @@ -193,12 +205,15 @@ return null; }, - _newReply: function(inReplyTo, opt_lineNum, opt_message) { + _newReply: function(inReplyTo, opt_lineNum, opt_message, opt_unresolved) { var d = this._newDraft(opt_lineNum); d.in_reply_to = inReplyTo; if (opt_message != null) { d.message = opt_message; } + if (opt_unresolved !== undefined) { + d.unresolved = opt_unresolved; + } return d; }, @@ -261,7 +276,7 @@ console.error('Comment update for another comment thread.'); return; } - this.comments[index] = comment; + this.set(['comments', index], comment); }, _indexOf: function(comment, arr) { @@ -274,5 +289,9 @@ } return -1; }, + + _computeHostClass: function(unresolved) { + return unresolved ? 'unresolved' : ''; + }, }); })();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html index 1189f32..af28a26 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
@@ -240,6 +240,7 @@ assert.equal(drafts.length, 1); assert.equal(drafts[0].message, 'Done'); assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215'); + assert.isFalse(drafts[0].unresolved); done(); }); commentEl.fire('create-done-comment', {comment: commentEl.comment}, @@ -259,6 +260,7 @@ assert.equal( drafts[0].message, '> is this a crossover episode!?\n\nPlease Fix'); assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215'); + assert.isTrue(drafts[0].unresolved); done(); }); commentEl.fire('create-fix-comment', {comment: commentEl.comment}, @@ -462,13 +464,15 @@ assert.strictEqual(element.comments[0], updatedComment); }); - test('orphan replies', function() { - var comments = [ + suite('jack and sally comment data test consolidation', function() { + setup(function() { + element.comments = [ { id: 'jacks_reply', message: 'i like you, too', in_reply_to: 'sallys_confession', updated: '2015-12-25 15:00:20.396000000', + unresolved: false, }, { id: 'sallys_confession', in_reply_to: 'nonexistent_comment', @@ -484,70 +488,41 @@ message: 'i will poison you so i can get away', updated: '2015-10-31 15:00:20.396000000', }]; - element.comments = comments; - assert.equal(4, element._orderedComments.length); + }); + + test('orphan replies', function() { + assert.equal(4, element._orderedComments.length); + }); + + test('keyboard shortcuts', function() { + var expandCollapseStub = sinon.stub(element, '_expandCollapseComments'); + MockInteractions.pressAndReleaseKeyOn(element, 69, null, 'e'); + assert.isTrue(expandCollapseStub.lastCall.calledWith(false)); + + MockInteractions.pressAndReleaseKeyOn(element, 69, 'shift', 'e'); + assert.isTrue(expandCollapseStub.lastCall.calledWith(true)); + expandCollapseStub.restore(); + }); + + test('comment in_reply_to is either null or most recent comment id', + function() { + element._createReplyComment(element.comments[3], 'dummy', true); + flushAsynchronousOperations(); + assert.equal(element._orderedComments.length, 5); + assert.equal(element._orderedComments[4].in_reply_to, 'jacks_reply'); + }); + + test('resolvable comments', function() { + assert.isFalse(element._unresolved); + element._createReplyComment(element.comments[3], 'dummy', true, true); + flushAsynchronousOperations(); + assert.isTrue(element._unresolved); + }); }); - test('keyboard shortcuts', function() { - var comments = [ - { - id: 'jacks_reply', - message: 'i like you, too', - in_reply_to: 'sallys_confession', - updated: '2015-12-25 15:00:20.396000000', - }, { - id: 'sallys_confession', - in_reply_to: 'nonexistent_comment', - message: 'i like you, jack', - updated: '2015-12-24 15:00:20.396000000', - }, { - id: 'sally_to_dr_finklestein', - in_reply_to: 'nonexistent_comment', - message: 'i’m running away', - updated: '2015-10-31 09:00:20.396000000', - }, { - id: 'sallys_defiance', - message: 'i will poison you so i can get away', - updated: '2015-10-31 15:00:20.396000000', - }]; - element.comments = comments; - var expandCollapseStub = sinon.stub(element, '_expandCollapseComments'); - MockInteractions.pressAndReleaseKeyOn(element, 69, null, 'e'); - assert.isTrue(expandCollapseStub.lastCall.calledWith(false)); - - MockInteractions.pressAndReleaseKeyOn(element, 69, 'shift', 'e'); - assert.isTrue(expandCollapseStub.lastCall.calledWith(true)); - expandCollapseStub.restore(); - }); - - test('comment in_reply_to is either null or most recent comment id', - function() { - var comments = [ - { - id: 'jacks_reply', - message: 'i like you, too', - in_reply_to: 'sallys_confession', - updated: '2015-12-25 15:00:20.396000000', - }, { - id: 'sallys_confession', - in_reply_to: 'nonexistent_comment', - message: 'i like you, jack', - updated: '2015-12-24 15:00:20.396000000', - }, { - id: 'sally_to_dr_finklestein', - in_reply_to: 'nonexistent_comment', - message: 'i’m running away', - updated: '2015-10-31 09:00:20.396000000', - }, { - id: 'sallys_defiance', - message: 'i will poison you so i can get away', - updated: '2015-10-31 15:00:20.396000000', - }]; - element.set('comments', comments); - element._createReplyComment(comments[3], 'dummy', true); - flushAsynchronousOperations(); - assert.equal(element._orderedComments.length, 5); - assert.equal(element._orderedComments[4].in_reply_to, 'jacks_reply'); + test('_computeHostClass', function() { + assert.equal(element._computeHostClass(true), 'unresolved'); + assert.equal(element._computeHostClass(false), ''); }); }); </script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html index 2bde4e7..5d2c860 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
@@ -109,7 +109,8 @@ display: inline; } .draft:not(.editing) .save, - .draft:not(.editing) .cancel { + .draft:not(.editing) .cancel, + .draft:not(.editing) .resolve { display: none; } .editing .message, @@ -170,6 +171,13 @@ #container.collapsed iron-autogrow-textarea { display: none; } + .resolve { + margin: auto; + } + .resolve label { + color: #333; + font-size: 12px; + } </style> <div id="container" class="container"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js index b0311dd..218890d8 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -114,6 +114,11 @@ value: '', observer: '_messageTextChanged', }, + + resolved: { + type: Boolean, + observer: '_toggleResolved', + }, }, observers: [ @@ -186,6 +191,7 @@ _commentChanged: function(comment) { this.editing = !!comment.__editing; + this.resolved = !comment.unresolved; if (this.editing) { // It's a new draft/reply, notify. this._fireUpdate(); } @@ -354,6 +360,7 @@ _handleSave: function(e) { e.preventDefault(); + this.set('comment.__editing', false); this.save(); }, @@ -439,5 +446,14 @@ _handleMouseLeave: function(e) { this.fire('comment-mouse-out', this._getEventPayload()); }, + + _handleToggleResolved: function() { + this.resolved = !this.resolved; + }, + + _toggleResolved: function(resolved) { + this.comment.unresolved = !resolved; + this.fire('comment-update', this._getEventPayload()); + }, }); })();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html index 59e5874..a1e36c5 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -129,13 +129,6 @@ MockInteractions.tap(element.$$('.done')); }); - test('proper event fires on fix', function(done) { - element.addEventListener('create-fix-comment', function(e) { - done(); - }); - MockInteractions.tap(element.$$('.fix')); - }); - test('clicking on date link does not trigger nav', function() { var showStub = sinon.stub(page, 'show'); var dateEl = element.$$('.date'); @@ -480,6 +473,7 @@ line: 5, path: '/path/to/file', message: 'good news, everyone!', + unresolved: false, }, patchNum: 1, },