Merge changes from topics "gr-file-list-to-ts", "gr-smart-search-to-ts"
* changes:
Convert files to typescript
Rename files to preserve history
Convert files to typescript
Rename files to preserve history
Convert gr-smart-search to typescript
Rename files to preserve history
diff --git a/java/com/google/gerrit/pgm/init/SitePathInitializer.java b/java/com/google/gerrit/pgm/init/SitePathInitializer.java
index 8a71c1c..ddc4f79 100644
--- a/java/com/google/gerrit/pgm/init/SitePathInitializer.java
+++ b/java/com/google/gerrit/pgm/init/SitePathInitializer.java
@@ -125,7 +125,8 @@
extractMailExample("DeleteVoteHtml.soy");
extractMailExample("Footer.soy");
extractMailExample("FooterHtml.soy");
- extractMailExample("HeaderHtml.soy");
+ extractMailExample("ChangeHeader.soy");
+ extractMailExample("ChangeHeaderHtml.soy");
extractMailExample("HttpPasswordUpdate.soy");
extractMailExample("HttpPasswordUpdateHtml.soy");
extractMailExample("InboundEmailRejection.soy");
diff --git a/java/com/google/gerrit/server/mail/EmailSettings.java b/java/com/google/gerrit/server/mail/EmailSettings.java
index c411af5..1ac6eb6 100644
--- a/java/com/google/gerrit/server/mail/EmailSettings.java
+++ b/java/com/google/gerrit/server/mail/EmailSettings.java
@@ -38,6 +38,7 @@
public final Encryption encryption;
public final long fetchInterval; // in milliseconds
public final boolean sendNewPatchsetEmails;
+ public final boolean isAttentionSetEnabled;
@Inject
EmailSettings(@GerritServerConfig Config cfg) {
@@ -60,5 +61,6 @@
TimeUnit.MILLISECONDS.convert(60, TimeUnit.SECONDS),
TimeUnit.MILLISECONDS);
sendNewPatchsetEmails = cfg.getBoolean("change", null, "sendNewPatchsetEmails", true);
+ isAttentionSetEnabled = cfg.getBoolean("change", null, "enableAttentionSet", false);
}
}
diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
index 6637f99..8d76e23 100644
--- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java
+++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.mail.send;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.server.util.AttentionSetUtil.additionsOnly;
import com.google.common.base.Splitter;
@@ -128,6 +129,10 @@
/** Format the message body by calling {@link #appendText(String)}. */
@Override
protected void format() throws EmailException {
+ if (useHtml()) {
+ appendHtml(soyHtmlTemplate("ChangeHeaderHtml"));
+ }
+ appendText(textTemplate("ChangeHeader"));
formatChange();
appendText(textTemplate("ChangeFooter"));
if (useHtml()) {
@@ -505,6 +510,13 @@
for (Account.Id attentionUser : currentAttentionSet) {
footers.add(MailHeader.ATTENTION.withDelimiter() + getNameEmailFor(attentionUser));
}
+ // Since this would be user visible, only show it if attention set is enabled
+ if (args.settings.isAttentionSetEnabled && !currentAttentionSet.isEmpty()) {
+ // We need names rather than account ids / emails to make it user readable.
+ soyContext.put(
+ "attentionSet",
+ currentAttentionSet.stream().map(this::getNameFor).collect(toImmutableSet()));
+ }
}
/**
diff --git a/java/com/google/gerrit/server/mail/send/MailSoySauceProvider.java b/java/com/google/gerrit/server/mail/send/MailSoySauceProvider.java
index 623bdc2..1b58057 100644
--- a/java/com/google/gerrit/server/mail/send/MailSoySauceProvider.java
+++ b/java/com/google/gerrit/server/mail/send/MailSoySauceProvider.java
@@ -45,6 +45,8 @@
"AddToAttentionSetHtml.soy",
"ChangeFooter.soy",
"ChangeFooterHtml.soy",
+ "ChangeHeader.soy",
+ "ChangeHeaderHtml.soy",
"ChangeSubject.soy",
"Comment.soy",
"CommentHtml.soy",
@@ -60,7 +62,6 @@
"InboundEmailRejectionHtml.soy",
"Footer.soy",
"FooterHtml.soy",
- "HeaderHtml.soy",
"HttpPasswordUpdate.soy",
"HttpPasswordUpdateHtml.soy",
"Merged.soy",
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index 1eb274b..bef5317 100644
--- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -116,9 +116,6 @@
if (messageId == null) {
throw new IllegalStateException("All emails must have a messageId");
}
- if (useHtml()) {
- appendHtml(soyHtmlTemplate("HeaderHtml"));
- }
format();
appendText(textTemplate("Footer"));
if (useHtml()) {
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 3b77dd9..3430047 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -461,9 +461,12 @@
// If we naively execute postUpdate even if the change is already merged when updateChange
// being, then we are subject to a race where postUpdate steps are run twice if two submit
// processes run at the same time.
- logger.atFine().log("Skipping post-update steps for change %s", getId());
+ logger.atFine().log(
+ "Skipping post-update steps for change %s; submitter is %s", getId(), submitter);
return;
}
+ logger.atFine().log(
+ "Begin post-update steps for change %s; submitter is %s", getId(), submitter);
postUpdateImpl(ctx);
if (command != null) {
@@ -483,6 +486,9 @@
}
}
+ logger.atFine().log(
+ "Begin sending emails for submitting change %s; submitter is %s", getId(), submitter);
+
// Assume the change must have been merged at this point, otherwise we would
// have failed fast in one of the other steps.
try {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 08e70fb..61eef63 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -28,6 +28,7 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.UseClockStep;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
@@ -1363,6 +1364,75 @@
}
@Test
+ @GerritConfig(name = "change.enableAttentionSet", value = "true")
+ public void attentionSetEmailHeader() throws Exception {
+ PushOneCommit.Result r = createChange();
+ TestAccount user2 = accountCreator.user2();
+ // Add user and user2 to the attention set.
+ change(r)
+ .current()
+ .review(
+ ReviewInput.create().reviewer(user.email()).reviewer(accountCreator.user2().email()));
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .contains(
+ "Attention is currently required from: "
+ + user2.fullName()
+ + ", "
+ + user.fullName()
+ + ".");
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).htmlBody())
+ .contains(
+ "Attention is currently required from: "
+ + user2.fullName()
+ + ", "
+ + user.fullName()
+ + ".");
+ sender.clear();
+
+ // Irrelevant reply, User and User2 are still in the attention set.
+ change(r).current().review(ReviewInput.approve());
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .contains(
+ "Attention is currently required from: "
+ + user2.fullName()
+ + ", "
+ + user.fullName()
+ + ".");
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).htmlBody())
+ .contains(
+ "Attention is currently required from: "
+ + user2.fullName()
+ + ", "
+ + user.fullName()
+ + ".");
+ sender.clear();
+
+ // Abandon the change which removes user from attention set; there is an email but without the
+ // attention footer.
+ change(r).abandon();
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .doesNotContain("Attention is currently required");
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).htmlBody())
+ .doesNotContain("Attention is currently required");
+ sender.clear();
+ }
+
+ @Test
+ @GerritConfig(name = "change.enableAttentionSet", value = "false")
+ public void noReferenceToAttentionSetInEmailsWhenDisabled() throws Exception {
+ PushOneCommit.Result r = createChange();
+ // Add user and to the attention set.
+ change(r).addReviewer(user.id().toString());
+
+ // Attention set is not referenced.
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .doesNotContain("Attention is currently required");
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).htmlBody())
+ .doesNotContain("Attention is currently required");
+ sender.clear();
+ }
+
+ @Test
public void attentionSetWithEmailFilter() throws Exception {
PushOneCommit.Result r = createChange();
diff --git a/polygerrit-ui/app/constants/messages.ts b/polygerrit-ui/app/constants/messages.ts
index 15692bd..5b4a534 100644
--- a/polygerrit-ui/app/constants/messages.ts
+++ b/polygerrit-ui/app/constants/messages.ts
@@ -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.';
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
index f0b8372..6af8960 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
@@ -637,6 +637,7 @@
logged-in="[[_loggedIn]]"
only-show-robot-comments-with-human-reply=""
on-thread-list-modified="_handleReloadDiffComments"
+ unresolved-only
></gr-thread-list>
</template>
<template
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
index 245c65f..2e9189e 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.ts
@@ -61,7 +61,7 @@
@property({type: Object})
_labelValues?: LabelValuesMap;
- getLabelValues(): LabelNameToValuesMap {
+ getLabelValues(includeDefaults = true): LabelNameToValuesMap {
const labels: LabelNameToValuesMap = {};
if (this.shadowRoot === null || !this.change) {
return labels;
@@ -106,8 +106,12 @@
prevValNum = prevVal;
}
+ const defValNum = this._getDefaultValue(this.change.labels, label);
+
if (selectedVal !== prevValNum) {
- labels[label] = selectedVal;
+ if (includeDefaults || !!prevValNum || selectedVal !== defValNum) {
+ labels[label] = selectedVal;
+ }
}
}
return labels;
@@ -126,6 +130,12 @@
return numberValue;
}
+ _getDefaultValue(labels?: LabelNameToInfoMap, labelName?: string) {
+ if (!labelName || !labels?.[labelName]) return undefined;
+ const labelInfo = labels[labelName] as DetailedLabelInfo;
+ return labelInfo.default_value;
+ }
+
_getVoteForAccount(
labels: LabelNameToInfoMap | undefined,
labelName: string,
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_test.js b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_test.js
index ffc17cd..ae639e1 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_test.js
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_test.js
@@ -99,6 +99,22 @@
});
});
+ test('getLabelValues includeDefaults', async () => {
+ element.change = {
+ _number: '123',
+ labels: {
+ 'Code-Review': {
+ values: {'0': 'meh', '+1': 'good', '-1': 'bad'},
+ default_value: 0,
+ },
+ },
+ };
+ await flush();
+
+ assert.deepEqual(element.getLabelValues(true), {'Code-Review': 0});
+ assert.deepEqual(element.getLabelValues(false), {});
+ });
+
test('_getVoteForAccount', () => {
const labelName = 'Code-Review';
assert.strictEqual(element._getVoteForAccount(
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 50bf411..b3478b1 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -321,6 +321,11 @@
@property({type: Boolean})
_reviewersMutated = false;
+ /**
+ * Signifies that the user has changed their vote on a label or (if they have
+ * not yet voted on a label) if a selected vote is different from the default
+ * vote.
+ */
@property({type: Boolean})
_labelsChanged = false;
@@ -950,7 +955,9 @@
'_ccs.*',
'change',
'draftCommentThreads',
- '_includeComments'
+ '_includeComments',
+ '_labelsChanged',
+ 'draft'
)
_computeNewAttention(
currentUser?: AccountInfo,
@@ -961,7 +968,9 @@
ccs?: PolymerDeepPropertyChange<AccountInfoInput[], AccountInfoInput[]>,
change?: ChangeInfo,
draftCommentThreads?: CommentThread[],
- includeComments?: boolean
+ includeComments?: boolean,
+ _labelsChanged?: boolean,
+ draft?: boolean
) {
if (
currentUser === undefined ||
@@ -977,6 +986,10 @@
// The draft comments are only relevant for the attention set as long as the
// user actually plans to publish their drafts.
draftCommentThreads = includeComments ? draftCommentThreads : [];
+ const hasDraft = draftCommentThreads.length > 0 || !!draft;
+ const hasVote = !!_labelsChanged;
+ const isOwner = this._isOwner(currentUser, change);
+ const isUploader = this._uploader?._account_id === currentUser._account_id;
this._attentionCcsCount = removeServiceUsers(ccs.base).length;
this._currentAttentionSet = new Set(
Object.keys(change.attention_set || {}).map(
@@ -991,24 +1004,21 @@
);
// Remove the current user.
newAttention.delete(currentUser._account_id);
- // Add all new reviewers.
+ // Add all new reviewers, but not the current reviewer, if they are also
+ // sending a draft or a label vote.
+ const notIsReviewerAndHasDraftOrLabel = (r: AccountInfo) =>
+ !(r._account_id === currentUser._account_id && (hasDraft || hasVote));
reviewers.base
.filter(r => r._pendingAdd && r._account_id)
+ .filter(notIsReviewerAndHasDraftOrLabel)
.forEach(r => newAttention.add(r._account_id!));
- // Add the uploader, if someone else replies.
- if (
- this._uploader &&
- this._uploader._account_id !== currentUser._account_id
- ) {
- // An uploader must have an _account_id.
- newAttention.add(this._uploader._account_id!);
- }
- // Add the owner, if someone else replies. Also add the owner, if the
- // attention set would otherwise be empty.
- if (change.owner) {
- if (!this._isOwner(currentUser, change)) {
- // A change owner must have an _account_id.
- newAttention.add(change.owner._account_id!);
+ // Add owner and uploader, if someone else replies.
+ if (hasDraft || hasVote) {
+ if (this._uploader?._account_id && !isUploader) {
+ newAttention.add(this._uploader._account_id);
+ }
+ if (change.owner?._account_id && !isOwner) {
+ newAttention.add(change.owner._account_id);
}
}
} else {
@@ -1368,7 +1378,7 @@
_handleLabelsChanged() {
this._labelsChanged =
- Object.keys(this.$.labelScores.getLabelValues()).length !== 0;
+ Object.keys(this.$.labelScores.getLabelValues(false)).length !== 0;
}
_isState(knownLatestState?: LatestPatchState, value?: LatestPatchState) {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
index 4b0bc24..c56a5c9 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
@@ -412,6 +412,7 @@
data-label="Edit"
data-action-type="change"
data-action-key="edit"
+ has-tooltip=""
title="[[_computeAttentionButtonTitle(_sendDisabled)]]"
role="button"
tabindex="0"
@@ -552,14 +553,14 @@
</template>
<template
is="dom-if"
- if="[[_computeShowAttentionTip(account, _owner, _currentAttentionSet, _newAttentionSet)]]"
+ if="[[_computeShowAttentionTip(_account, _owner, _currentAttentionSet, _newAttentionSet)]]"
>
<div class="attentionTip">
<iron-icon
class="pointer"
icon="gr-icons:lightbulb-outline"
></iron-icon>
- Be mindful of requiring attention from too many reviewers.
+ Be mindful of requiring attention from too many users.
</div>
</template>
</section>
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
index 7af3792..7a89f3d 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
@@ -283,6 +283,33 @@
checkComputeAttention('MERGED', 1, [22, 33], 1, [22, 33], [], [22, 33]);
});
+ test('computeNewAttention when adding reviewers', () => {
+ const user = {_account_id: 1};
+ const reviewers = {base: [
+ {_account_id: 1, _pendingAdd: true},
+ {_account_id: 2, _pendingAdd: true},
+ ]};
+ const change = {
+ owner: {_account_id: 5},
+ status: 'NEW',
+ attention_set: {},
+ };
+ element.change = change;
+ element._reviewers = reviewers.base;
+ flush();
+
+ element._computeNewAttention(user, reviewers, [], change, [], true);
+ assert.sameMembers([...element._newAttentionSet], [1, 2]);
+
+ // If the user votes on the change, then they should not be added to the
+ // attention set, even if they have just added themselves as reviewer.
+ // But voting should also add the owner (5).
+ const labelsChanged = true;
+ element._computeNewAttention(
+ user, reviewers, [], change, [], true, labelsChanged);
+ assert.sameMembers([...element._newAttentionSet], [2, 5]);
+ });
+
test('computeNewAttentionAccounts', () => {
element._reviewers = [
{_account_id: 123, display_name: 'Ernie'},
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
index 7585d16..6a32834 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
@@ -24,12 +24,14 @@
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 @@
@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 @@
@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 @@
) {
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 @@
.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 @@
}
_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 @@
return (
index > 0 &&
this._isFirstThreadWithFileName(
- sortedThreads,
+ displayedThreads,
thread,
unresolvedOnly,
draftsOnly,
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
index d74c985..e55f98a 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
@@ -57,15 +57,23 @@
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 @@
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> \🎉 </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>
`;
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js
index bad3a99..efc072f 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.js
@@ -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 @@
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 @@
});
});
- test('toggle unresolved only shows unresolved comments', () => {
+ test('toggle unresolved shows all comments', () => {
MockInteractions.tap(element.shadowRoot.querySelector(
'#unresolvedToggle'));
flush();
@@ -617,18 +626,9 @@
});
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.'));
});
});
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 61db314..e7c9a4a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -231,9 +231,6 @@
@property({type: Boolean})
_loggedIn = false;
- @property({type: Boolean})
- _loading = false;
-
@property({type: String})
_errorMessage: string | null = null;
@@ -331,7 +328,7 @@
this.clear();
if (!this.path) throw new Error('Missing required "path" property.');
if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
- this._loading = true;
+ this.diff = undefined;
this._errorMessage = null;
const whitespaceLevel = this._getIgnoreWhitespace();
@@ -381,7 +378,6 @@
} finally {
this.reporting.timeEnd(TimingLabel.TOTAL);
}
- this._loading = false;
}
private _getLayers(path: string, changeNum: NumericChangeId): DiffLayer[] {
@@ -411,12 +407,12 @@
const changeNum = this.changeNum;
const path = this.path;
// Coverage providers do not provide data for EDIT and PARENT patch sets.
- const basePatchNum = isNumber(this.patchRange.basePatchNum)
- ? this.patchRange.basePatchNum
- : undefined;
- const patchNum = isNumber(this.patchRange.patchNum)
- ? this.patchRange.patchNum
- : undefined;
+
+ const toNumberOnly = (patchNum: PatchSetNum) =>
+ isNumber(patchNum) ? patchNum : undefined;
+
+ const basePatchNum = toNumberOnly(this.patchRange.basePatchNum);
+ const patchNum = toNumberOnly(this.patchRange.patchNum);
this.$.jsAPI
.getCoverageAnnotationApi()
.then(coverageAnnotationApi => {
@@ -430,8 +426,8 @@
!coverageRanges ||
changeNum !== this.changeNum ||
path !== this.path ||
- basePatchNum !== this.patchRange.basePatchNum ||
- patchNum !== this.patchRange.patchNum
+ basePatchNum !== toNumberOnly(this.patchRange.basePatchNum) ||
+ patchNum !== toNumberOnly(this.patchRange.patchNum)
) {
return;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_html.ts
index c92fd5c..d1564b0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_html.ts
@@ -32,7 +32,6 @@
view-mode="[[viewMode]]"
line-of-interest="[[lineOfInterest]]"
logged-in="[[_loggedIn]]"
- loading="[[_loading]]"
error-message="[[_errorMessage]]"
base-image="[[_baseImage]]"
revision-image="[[_revisionImage]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index e3bda36..cd2c860 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -388,6 +388,13 @@
sinon.stub(element, '_getDiff').callsFake(() => new Promise(() => {}));
element.patchRange = {};
+ // Needs to be set to something first for it to cancel.
+ element.diff = {
+ content: [{
+ a: ['foo'],
+ }],
+ };
+
element.reload();
assert.isTrue(cancelStub.called);
});
@@ -824,7 +831,7 @@
test('delegates cancel()', () => {
const stub = sinon.stub(element.$.diff, 'cancel');
element.patchRange = {};
- element.reload();
+ element.cancel();
assert.isTrue(stub.calledOnce);
assert.equal(stub.lastCall.args.length, 0);
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index 022ac80..7c317bb 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -191,8 +191,9 @@
@property({type: Object})
lineOfInterest?: LineOfInterest;
- @property({type: Boolean, observer: '_loadingChanged'})
- loading = false;
+ /** True when diff is changed, until the content is done rendering. */
+ @property({type: Boolean})
+ _loading = false;
@property({type: Boolean})
loggedIn = false;
@@ -787,12 +788,6 @@
this.clearDiffContent();
}
- _loadingChanged(newValue?: boolean) {
- if (newValue) {
- this._cleanup();
- }
- }
-
_lineWrappingObserver() {
this._prefsChanged(this.prefs);
}
@@ -831,8 +826,9 @@
}
_diffChanged(newValue?: DiffInfo) {
+ this._loading = true;
+ this._cleanup();
if (newValue) {
- this._cleanup();
this._diffLength = this.getDiffLength(newValue);
this._debounceRenderDiffTable();
}
@@ -890,6 +886,7 @@
}
_handleRenderContent() {
+ this._loading = false;
this._unobserveIncrementalNodes();
this._incrementalNodeObserver = (dom(
this
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
index 3c4642c..e9de9e7 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
@@ -446,7 +446,7 @@
<template
is="dom-if"
- if="[[showNoChangeMessage(loading, prefs, _diffLength, diff)]]"
+ if="[[showNoChangeMessage(_loading, prefs, _diffLength, diff)]]"
>
<div class="whitespace-change-only-message">
This file only contains whitespace changes. Modify the whitespace
@@ -457,7 +457,7 @@
</gr-diff-highlight>
</gr-diff-selection>
</div>
- <div class$="[[_computeNewlineWarningClass(_newlineWarning, loading)]]">
+ <div class$="[[_computeNewlineWarningClass(_newlineWarning, _loading)]]">
[[_newlineWarning]]
</div>
<div id="loadingError" class$="[[_computeErrorClass(errorMessage)]]">
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
index 0eddbc0..da2881e 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
@@ -172,9 +172,7 @@
// we want to check before showing reloading toast
const _accountKey = accountKey(this.account);
this.dispatchEventThroughTarget('show-alert', {
- detail: {
- message: 'Reloading page...',
- },
+ message: 'Reloading page...',
});
const reviewInput: Partial<ReviewInput> = {};
reviewInput.reviewers = [
@@ -204,9 +202,7 @@
if (!this.change || !(this.account?._account_id || this.account?.email))
throw new Error('Missing change or account.');
this.dispatchEventThroughTarget('show-alert', {
- detail: {
- message: 'Reloading page...',
- },
+ message: 'Reloading page...',
});
this.$.restAPI
.removeChangeReviewer(
@@ -241,10 +237,8 @@
_handleClickAddToAttentionSet() {
if (!this.change || !this.account._account_id) return;
this.dispatchEventThroughTarget('show-alert', {
- detail: {
- message: 'Saving attention set update ...',
- dismissOnNavigation: true,
- },
+ message: 'Saving attention set update ...',
+ dismissOnNavigation: true,
});
// We are deliberately updating the UI before making the API call. It is a
@@ -273,10 +267,8 @@
_handleClickRemoveFromAttentionSet() {
if (!this.change || !this.account._account_id) return;
this.dispatchEventThroughTarget('show-alert', {
- detail: {
- message: 'Saving attention set update ...',
- dismissOnNavigation: true,
- },
+ message: 'Saving attention set update ...',
+ dismissOnNavigation: true,
});
// We are deliberately updating the UI before making the API call. It is a
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index dca8115..5bcf0b8 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -477,6 +477,9 @@
revert_submission?: ActionInfo;
abandon?: ActionInfo;
submit?: ActionInfo;
+ topic?: ActionInfo;
+ hashtags?: ActionInfo;
+ assignee?: ActionInfo;
}
/**
diff --git a/resources/com/google/gerrit/server/mail/HeaderHtml.soy b/resources/com/google/gerrit/server/mail/ChangeHeader.soy
similarity index 62%
copy from resources/com/google/gerrit/server/mail/HeaderHtml.soy
copy to resources/com/google/gerrit/server/mail/ChangeHeader.soy
index 4710d8c..fde69f1 100644
--- a/resources/com/google/gerrit/server/mail/HeaderHtml.soy
+++ b/resources/com/google/gerrit/server/mail/ChangeHeader.soy
@@ -1,5 +1,5 @@
/**
- * Copyright (C) 2016 The Android Open Source Project
+ * Copyright (C) 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -16,5 +16,17 @@
{namespace com.google.gerrit.server.mail.template}
-{template .HeaderHtml}
+{template .ChangeHeader kind="text"}
+ {@param attentionSet: ?}
+ {if $attentionSet}
+ Attention is currently required from:{sp}
+ {for $attentionSetUser in $attentionSet}
+ {$attentionSetUser}
+ // add commas or dot.
+ {if isLast($attentionSetUser)}.
+ {else},{sp}
+ {/if}
+ {/for}
+ {\n}
+ {/if}
{/template}
diff --git a/resources/com/google/gerrit/server/mail/HeaderHtml.soy b/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy
similarity index 62%
rename from resources/com/google/gerrit/server/mail/HeaderHtml.soy
rename to resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy
index 4710d8c..ea12455 100644
--- a/resources/com/google/gerrit/server/mail/HeaderHtml.soy
+++ b/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy
@@ -1,5 +1,5 @@
/**
- * Copyright (C) 2016 The Android Open Source Project
+ * Copyright (C) 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -14,7 +14,20 @@
* limitations under the License.
*/
+
{namespace com.google.gerrit.server.mail.template}
-{template .HeaderHtml}
+{template .ChangeHeaderHtml}
+ {@param attentionSet: ?}
+ {if $attentionSet}
+ <p> Attention is currently required from:{sp}
+ {for $attentionSetUser in $attentionSet}
+ {$attentionSetUser}
+ //add commas or dot.
+ {if isLast($attentionSetUser)}.
+ {else},{sp}
+ {/if}
+ {/for} </p>
+ {\n}
+ {/if}
{/template}
