From 1d3fe7ed9cce9999e1251a9eba5dd9fedbfa01be Mon Sep 17 00:00:00 2001 From: Tao Zhou Date: Mon, 6 Apr 2020 18:16:50 +0200 Subject: [PATCH] Hide 'ACK' and 'DONE' button for resolved thread It doesn't make sense to still show 'ACK' and 'DONE' button for resolved thread Change-Id: If19d5623a514058b461f58cdf8ac8013aac68a30 --- .../change/gr-thread-list/gr-thread-list.js | 16 ++-- .../gr-comment-thread_html.js | 6 +- .../gr-comment-thread_test.html | 79 ++++++++++++++++++- 3 files changed, 87 insertions(+), 14 deletions(-) diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js index e89be0f58f..7601e0f3be 100644 --- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js +++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js @@ -46,7 +46,7 @@ class GrThreadList extends GestureEventListeners( static get properties() { return { - /** @type {?} */ + /** @type {?} */ change: Object, threads: Array, changeNum: String, @@ -113,6 +113,8 @@ class GrThreadList extends GestureEventListeners( this._updateSortedThreads(threads); } + // TODO(taoalpha): should allow only sort once during initialization + // to avoid flickering _updateSortedThreads(threads) { this._sortedThreads = threads.map(this._getThreadWithSortInfo).sort((c1, c2) => { @@ -122,9 +124,6 @@ class GrThreadList extends GestureEventListeners( // - file path // - line // - updated time - const c1Date = c1.__date || util.parseDate(c1.updated); - const c2Date = c2.__date || util.parseDate(c2.updated); - const dateCompare = c2Date - c1Date; if (c2.unresolved || c1.unresolved) { if (!c1.unresolved) { return 1; } if (!c2.unresolved) { return -1; } @@ -151,6 +150,9 @@ class GrThreadList extends GestureEventListeners( return c1.thread.line - c2.thread.line; } + const c1Date = c1.__date || util.parseDate(c1.updated); + const c2Date = c2.__date || util.parseDate(c2.updated); + const dateCompare = c2Date - c1Date; if (dateCompare === 0 && (!c1.id || !c1.id.localeCompare)) { return 0; } @@ -209,7 +211,7 @@ class GrThreadList extends GestureEventListeners( // anybody other than the current user would see. unresolved: !!lastNonDraftComment.unresolved, hasDraft: !!lastComment.__draft, - updated: lastComment.updated, + updated: lastComment.updated || lastComment.__date, }; } @@ -229,10 +231,6 @@ class GrThreadList extends GestureEventListeners( } _handleCommentsChanged(e) { - // Reset threads so thread computations occur on deep array changes to - // threads comments that are not observed naturally. - this._updateSortedThreads(this.threads); - this.dispatchEvent(new CustomEvent('thread-list-modified', {detail: {rootId: e.detail.rootId, path: e.detail.path}})); } diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.js b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.js index 1d991cbe84..800e247f26 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.js +++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.js @@ -86,8 +86,10 @@ export const htmlTemplate = html`
Reply Quote - Ack - Done +
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.html b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.html index 895866b0a4..f9fcf78df2 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.html +++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.html @@ -228,7 +228,7 @@ suite('gr-comment-thread tests', () => { }); }); -suite('comment action tests', () => { +suite('comment action tests with unresolved thread', () => { let element; let sandbox; @@ -265,6 +265,7 @@ suite('comment action tests', () => { message: 'is this a crossover episode!?', updated: '2015-12-08 19:48:33.843000000', path: '/path/to/file.txt', + unresolved: true, }]; flushAsynchronousOperations(); }); @@ -350,7 +351,7 @@ suite('comment action tests', () => { .querySelector('gr-comment'); assert.ok(commentEl); - const ackBtn = element.$.ackBtn; + const ackBtn = element.shadowRoot.querySelector('#ackBtn'); MockInteractions.tap(ackBtn); flush(() => { const drafts = element.comments.filter(c => c.__draft == true); @@ -372,7 +373,7 @@ suite('comment action tests', () => { .querySelector('gr-comment'); assert.ok(commentEl); - const doneBtn = element.$.doneBtn; + const doneBtn = element.shadowRoot.querySelector('#doneBtn'); MockInteractions.tap(doneBtn); flush(() => { const drafts = element.comments.filter(c => c.__draft == true); @@ -767,4 +768,76 @@ suite('comment action tests', () => { assert.notOk(element.hasAttribute('range')); }); }); + +suite('comment action tests on resolved comments', () => { + let element; + let sandbox; + + setup(() => { + sandbox = sinon.sandbox.create(); + stub('gr-rest-api-interface', { + getLoggedIn() { return Promise.resolve(false); }, + saveDiffDraft() { + return Promise.resolve({ + ok: true, + text() { + return Promise.resolve(')]}\'\n' + + JSON.stringify({ + id: '7afa4931_de3d65bd', + path: '/path/to/file.txt', + line: 5, + in_reply_to: 'baf0414d_60047215', + updated: '2015-12-21 02:01:10.850000000', + message: 'Done', + })); + }, + }); + }, + deleteDiffDraft() { return Promise.resolve({ok: true}); }, + }); + element = fixture('withComment'); + 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', + path: '/path/to/file.txt', + unresolved: false, + }]; + flushAsynchronousOperations(); + }); + + teardown(() => { + sandbox.restore(); + }); + + test('ack and done should be hidden', () => { + element.changeNum = '42'; + element.patchNum = '1'; + + const commentEl = element.shadowRoot + .querySelector('gr-comment'); + assert.ok(commentEl); + + const ackBtn = element.shadowRoot.querySelector('#ackBtn'); + const doneBtn = element.shadowRoot.querySelector('#doneBtn'); + assert.equal(ackBtn, null); + assert.equal(doneBtn, null); + }); + + test('reply and quote button should be visible', () => { + const commentEl = element.shadowRoot + .querySelector('gr-comment'); + assert.ok(commentEl); + + const replyBtn = element.shadowRoot.querySelector('#replyBtn'); + const quoteBtn = element.shadowRoot.querySelector('#quoteBtn'); + assert.ok(replyBtn); + assert.ok(quoteBtn); + }); +});