opendev/gerrit
Code Issues Proposed changes

Merge "Toggle default to show unresolved comments"

Browse Source
This commit is contained in:
Ben Rohlfs
2020-10-13 20:42:00 +00:00
committed by Gerrit Code Review
parent 25b2925ba7 548f0f4250
commit 85168b43ba
5 changed files with 155 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
polygerrit-ui/app/constants/messages.ts
View File

@@ -15,10 +15,6 @@
* limitations under the License.
*/
/** @desc Default message shown when no threads in gr-thread-list */
export const NO_THREADS_MSG =
'There are no inline comment threads on any diff for this change.';
/** @desc Message shown when no threads in gr-thread-list for robot comments */
export const NO_ROBOT_COMMENTS_THREADS_MSG =
'There are no findings for this patchset.';

1
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
View File

@@ -637,6 +637,7 @@ export const htmlTemplate = html`
logged-in="[[_loggedIn]]"
only-show-robot-comments-with-human-reply=""
on-thread-list-modified="_handleReloadDiffComments"
unresolved-only
></gr-thread-list>
</template>
<template

98
polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
View File

@@ -24,12 +24,14 @@ import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-thread-list_html';
import {parseDate} from '../../../utils/date-util';
import {NO_THREADS_MSG} from '../../../constants/messages';
import {CommentSide, SpecialFilePath} from '../../../constants/constants';
import {customElement, observe, property} from '@polymer/decorators';
import {CommentThread, isDraft, UIRobot} from '../../../utils/comment-util';
import {PolymerSpliceChange} from '@polymer/polymer/interfaces';
import {
PolymerSpliceChange,
PolymerDeepPropertyChange,
} from '@polymer/polymer/interfaces';
import {ChangeInfo} from '../../../types/common';
import {CommentThread, isDraft, UIRobot} from '../../../utils/comment-util';
interface CommentThreadWithInfo {
thread: CommentThread;
@@ -64,8 +66,19 @@ export class GrThreadList extends GestureEventListeners(
@property({type: Array})
_sortedThreads: CommentThread[] = [];
@property({
computed:
'_computeDisplayedThreads(_sortedThreads.*, unresolvedOnly, ' +
'_draftsOnly, onlyShowRobotCommentsWithHumanReply)',
type: Array,
})
_displayedThreads: CommentThread[] = [];
// thread-list is used in multiple places like the change log, hence
// keeping the default to be false. When used in comments tab, it's
// set as true.
@property({type: Boolean})
_unresolvedOnly = false;
unresolvedOnly = false;
@property({type: Boolean})
_draftsOnly = false;
@@ -76,13 +89,53 @@ export class GrThreadList extends GestureEventListeners(
@property({type: Boolean})
hideToggleButtons = false;
@property({type: String})
emptyThreadMsg = NO_THREADS_MSG;
_computeShowDraftToggle(loggedIn?: boolean) {
return loggedIn ? 'show' : '';
}
_showEmptyThreadsMessage(
threads: CommentThread[],
displayedThreads: CommentThread[],
unresolvedOnly: boolean
) {
if (!threads || !displayedThreads) return false;
return !threads.length || (unresolvedOnly && !displayedThreads.length);
}
_computeEmptyThreadsMessage(threads: CommentThread[]) {
return !threads.length ? 'No comments.' : 'No unresolved comments';
}
_showPartyPopper(threads: CommentThread[]) {
return !!threads.length;
}
_computeResolvedCommentsMessage(
threads: CommentThread[],
displayedThreads: CommentThread[],
unresolvedOnly: boolean
) {
if (unresolvedOnly && threads.length && !displayedThreads.length) {
return (
`Show ${threads.length} resolved comment` +
(threads.length > 1 ? 's' : '')
);
}
return '';
}
_showResolvedCommentsButton(
threads: CommentThread[],
displayedThreads: CommentThread[],
unresolvedOnly: boolean
) {
return unresolvedOnly && threads.length && !displayedThreads.length;
}
_handleResolvedCommentsMessageClick() {
this.unresolvedOnly = !this.unresolvedOnly;
}
_compareThreads(c1: CommentThreadWithInfo, c2: CommentThreadWithInfo) {
if (c1.thread.path !== c2.thread.path) {
// '/PATCHSET' will not come before '/COMMIT' when sorting
@@ -166,6 +219,7 @@ export class GrThreadList extends GestureEventListeners(
) {
if (!threads || threads.length === 0) {
this._sortedThreads = [];
this._displayedThreads = [];
return;
}
// We only want to sort on thread additions / removals to avoid
@@ -196,14 +250,34 @@ export class GrThreadList extends GestureEventListeners(
.map(threadInfo => threadInfo.thread);
}
_computeDisplayedThreads(
sortedThreadsRecord?: PolymerDeepPropertyChange<
CommentThread[],
CommentThread[]
>,
unresolvedOnly?: boolean,
draftsOnly?: boolean,
onlyShowRobotCommentsWithHumanReply?: boolean
) {
if (!sortedThreadsRecord || !sortedThreadsRecord.base) return [];
return sortedThreadsRecord.base.filter(t =>
this._shouldShowThread(
t,
unresolvedOnly,
draftsOnly,
onlyShowRobotCommentsWithHumanReply
)
);
}
_isFirstThreadWithFileName(
sortedThreads: CommentThread[],
displayedThreads: CommentThread[],
thread: CommentThread,
unresolvedOnly?: boolean,
draftsOnly?: boolean,
onlyShowRobotCommentsWithHumanReply?: boolean
) {
const threads = sortedThreads.filter(t =>
const threads = displayedThreads.filter(t =>
this._shouldShowThread(
t,
unresolvedOnly,
@@ -219,13 +293,13 @@ export class GrThreadList extends GestureEventListeners(
}
_shouldRenderSeparator(
sortedThreads: CommentThread[],
displayedThreads: CommentThread[],
thread: CommentThread,
unresolvedOnly?: boolean,
draftsOnly?: boolean,
onlyShowRobotCommentsWithHumanReply?: boolean
) {
const threads = sortedThreads.filter(t =>
const threads = displayedThreads.filter(t =>
this._shouldShowThread(
t,
unresolvedOnly,
@@ -240,7 +314,7 @@ export class GrThreadList extends GestureEventListeners(
return (
index > 0 &&
this._isFirstThreadWithFileName(
sortedThreads,
displayedThreads,
thread,
unresolvedOnly,
draftsOnly,

79
polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
View File

@@ -57,15 +57,23 @@ export const htmlTemplate = html`
border-top: 1px solid var(--border-color);
margin-top: var(--spacing-xl);
}
.resolved-comments-message {
color: var(--link-color);
cursor: pointer;
}
.show-resolved-comments {
box-shadow: none;
padding-left: var(--spacing-m);
}
</style>
<template is="dom-if" if="[[!hideToggleButtons]]">
<div class="header">
<div class="toggleItem">
<paper-toggle-button
id="unresolvedToggle"
checked="{{_unresolvedOnly}}"
checked="{{!unresolvedOnly}}"
on-tap="_onTapUnresolvedToggle"
>Only unresolved threads</paper-toggle-button
>All comments</paper-toggle-button
>
</div>
<div
@@ -75,48 +83,63 @@ export const htmlTemplate = html`
id="draftToggle"
checked="{{_draftsOnly}}"
on-tap="_onTapUnresolvedToggle"
>Only threads with drafts</paper-toggle-button
>Comments with drafts</paper-toggle-button
>
</div>
</div>
</template>
<div id="threads">
<template is="dom-if" if="[[!threads.length]]">
[[emptyThreadMsg]]
<template
is="dom-if"
if="[[_showEmptyThreadsMessage(threads, _displayedThreads, unresolvedOnly)]]"
>
<div>
<span>
<template is="dom-if" if="[[_showPartyPopper(threads)]]">
<span> \&#x1F389 </span>
</template>
[[_computeEmptyThreadsMessage(threads, _displayedThreads,
unresolvedOnly)]]
<template is="dom-if" if="[[_showResolvedCommentsButton(threads, _displayedThreads, unresolvedOnly)]]">
<gr-button
class="show-resolved-comments"
link
on-click="_handleResolvedCommentsMessageClick">
[[_computeResolvedCommentsMessage(threads, _displayedThreads,
unresolvedOnly)]]
</gr-button>
</template>
</span>
</div>
</template>
<template
is="dom-repeat"
items="[[_sortedThreads]]"
items="[[_displayedThreads]]"
as="thread"
initial-count="10"
target-framerate="60"
>
<template
is="dom-if"
if="[[_shouldShowThread(thread, _unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply)]]"
if="[[_shouldRenderSeparator(_displayedThreads, thread, unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply)]]"
>
<template
is="dom-if"
if="[[_shouldRenderSeparator(_sortedThreads, thread, _unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply)]]"
>
<div class="thread-separator"></div>
</template>
<gr-comment-thread
show-file-path=""
change-num="[[changeNum]]"
comments="[[thread.comments]]"
comment-side="[[thread.commentSide]]"
show-file-name="[[_isFirstThreadWithFileName(_sortedThreads, thread, _unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply)]]"
project-name="[[change.project]]"
is-on-parent="[[_isOnParent(thread.commentSide)]]"
line-num="[[thread.line]]"
patch-num="[[thread.patchNum]]"
path="[[thread.path]]"
root-id="{{thread.rootId}}"
on-thread-changed="_handleCommentsChanged"
on-thread-discard="_handleThreadDiscard"
></gr-comment-thread>
<div class="thread-separator"></div>
</template>
<gr-comment-thread
show-file-path=""
change-num="[[changeNum]]"
comments="[[thread.comments]]"
comment-side="[[thread.commentSide]]"
show-file-name="[[_isFirstThreadWithFileName(_displayedThreads, thread, unresolvedOnly, _draftsOnly, onlyShowRobotCommentsWithHumanReply)]]"
project-name="[[change.project]]"
is-on-parent="[[_isOnParent(thread.commentSide)]]"
line-num="[[thread.line]]"
patch-num="[[thread.patchNum]]"
path="[[thread.path]]"
root-id="{{thread.rootId}}"
on-thread-changed="_handleCommentsChanged"
on-thread-discard="_handleThreadDiscard"
></gr-comment-thread>
</template>
</div>
`;

34
polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js
View File

@@ -18,7 +18,6 @@
import '../../../test/common-test-setup-karma.js';
import './gr-thread-list.js';
import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js';
import {NO_THREADS_MSG} from '../../../constants/messages.js';
import {SpecialFilePath} from '../../../constants/constants.js';
const basicFixture = fixtureFromElement('gr-thread-list');
@@ -287,13 +286,23 @@ suite('gr-thread-list tests', () => {
assert.equal(getVisibleThreads().length, element.threads.length);
});
test('show unresolved threads if unresolvedOnly is set', done => {
element.unresolvedOnly = true;
flush();
const unresolvedThreads = element.threads.filter(t => t.comments.some(
c => c.unresolved
));
assert.equal(getVisibleThreads().length, unresolvedThreads.length);
done();
});
test('showing file name takes visible threads into account', () => {
assert.equal(element._isFirstThreadWithFileName(element._sortedThreads,
element._sortedThreads[2], element._unresolvedOnly, element._draftsOnly,
element._sortedThreads[2], element.unresolvedOnly, element._draftsOnly,
element.onlyShowRobotCommentsWithHumanReply), true);
element._unresolvedOnly = true;
element.unresolvedOnly = true;
assert.equal(element._isFirstThreadWithFileName(element._sortedThreads,
element._sortedThreads[2], element._unresolvedOnly, element._draftsOnly,
element._sortedThreads[2], element.unresolvedOnly, element._draftsOnly,
element.onlyShowRobotCommentsWithHumanReply), false);
});
@@ -539,7 +548,7 @@ suite('gr-thread-list tests', () => {
});
});
test('toggle unresolved only shows unresolved comments', () => {
test('toggle unresolved shows all comments', () => {
MockInteractions.tap(element.shadowRoot.querySelector(
'#unresolvedToggle'));
flush();
@@ -617,18 +626,9 @@ suite('gr-thread-list tests', () => {
});
test('default empty message should show', () => {
assert.equal(
element.shadowRoot.querySelector('#threads').textContent.trim(),
NO_THREADS_MSG
);
});
test('can override empty message', () => {
element.emptyThreadMsg = 'test';
assert.equal(
element.shadowRoot.querySelector('#threads').textContent.trim(),
'test'
);
assert.isTrue(
element.shadowRoot.querySelector('#threads').textContent.trim()
.includes('No comments.'));
});
});
});