Merge "Fix the gap between commit message and summaries"
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index a360510..562bdf8 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -1272,28 +1272,27 @@
del.add(c);
update.putApproval(normName, (short) 0);
}
- } else if (c != null) {
- // Check if the label exists in the request input (the user voted again). If the user
- // hadn't voted again, there is no need to re-apply the vote.
- if (inLabels.keySet().contains(c.label())) {
- PatchSetApproval.Builder b =
- c.toBuilder()
- .value(ent.getValue())
- .granted(ctx.getWhen())
- .tag(Optional.ofNullable(in.tag));
- ctx.getUser().updateRealAccountId(b::realAccountId);
- c = b.build();
- ups.add(c);
- addLabelDelta(normName, c.value());
- oldApprovals.put(normName, previous.get(normName));
- approvals.put(normName, c.value());
- update.putApproval(normName, ent.getValue());
- } else {
- current.put(normName, c);
- oldApprovals.put(normName, null);
- approvals.put(normName, c.value());
- }
- } else {
+ // Only allow voting again if the vote is copied over from a past patch-set, or the
+ // values are different.
+ } else if (c != null
+ && (c.value() != ent.getValue() || isApprovalCopiedOver(c, ctx.getNotes()))) {
+ PatchSetApproval.Builder b =
+ c.toBuilder()
+ .value(ent.getValue())
+ .granted(ctx.getWhen())
+ .tag(Optional.ofNullable(in.tag));
+ ctx.getUser().updateRealAccountId(b::realAccountId);
+ c = b.build();
+ ups.add(c);
+ addLabelDelta(normName, c.value());
+ oldApprovals.put(normName, previous.get(normName));
+ approvals.put(normName, c.value());
+ update.putApproval(normName, ent.getValue());
+ } else if (c != null && c.value() == ent.getValue()) {
+ current.put(normName, c);
+ oldApprovals.put(normName, null);
+ approvals.put(normName, c.value());
+ } else if (c == null) {
c =
ApprovalsUtil.newApproval(psId, user, lt.getLabelId(), ent.getValue(), ctx.getWhen())
.tag(Optional.ofNullable(in.tag))
@@ -1319,6 +1318,17 @@
return !del.isEmpty() || !ups.isEmpty();
}
+ /**
+ * Approval is copied over if it doesn't exist in the approvals of the current patch-set
+ * according to change notes (which means it was computed in {@link
+ * com.google.gerrit.server.ApprovalInference})
+ */
+ private boolean isApprovalCopiedOver(
+ PatchSetApproval patchSetApproval, ChangeNotes changeNotes) {
+ return !changeNotes.getApprovals().get(changeNotes.getChange().currentPatchSetId()).stream()
+ .anyMatch(p -> p.equals(patchSetApproval));
+ }
+
private void validatePostSubmitLabels(
ChangeContext ctx,
LabelTypes labelTypes,
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 0b2e0f7..c8b1715 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -63,7 +63,6 @@
import com.google.gerrit.server.restapi.change.OnPostReview;
import com.google.gerrit.server.restapi.change.PostReview;
import com.google.gerrit.server.update.CommentsRejectedException;
-import com.google.gerrit.testing.FakeEmailSender;
import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -597,7 +596,7 @@
input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
gApi.changes().id(r.getChangeId()).current().review(input);
testOnPostReview.assertApproval(
- LabelId.CODE_REVIEW, /* expectedOldValue= */ 2, /* expectedNewValue= */ 2);
+ LabelId.CODE_REVIEW, /* expectedOldValue= */ null, /* expectedNewValue= */ 2);
// Delete the vote.
input = new ReviewInput().label(LabelId.CODE_REVIEW, 0);
@@ -627,21 +626,19 @@
assertThat(r.getChange().approvals().values()).hasSize(1);
List<ChangeMessageInfo> changeMessages = gApi.changes().id(r.getChangeId()).messages();
- // The two latest change messages are both about Code-Review+2
+ // Only the last change message is about Code-Review+2
assertThat(Iterables.getLast(changeMessages).message).isEqualTo("Patch Set 1: Code-Review+2");
changeMessages.remove(changeMessages.size() - 1);
- assertThat(Iterables.getLast(changeMessages).message).isEqualTo("Patch Set 1: Code-Review+2");
+ assertThat(Iterables.getLast(changeMessages).message)
+ .isNotEqualTo("Patch Set 1: Code-Review+2");
- // The two latest emails are about Code-Review +2.
- List<FakeEmailSender.Message> messages = sender.getMessages();
- assertThat(messages).hasSize(2);
- for (FakeEmailSender.Message message : messages) {
- assertThat(message.body()).contains("Patch Set 1: Code-Review+2");
- }
+ // Only one email is about Code-Review +2 was sent.
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .contains("Patch Set 1: Code-Review+2");
}
@Test
- public void votingTheSameVoteSecondTimeExtendsOnPostReview() throws Exception {
+ public void votingTheSameVoteSecondTimeExtendsOnPostReviewWithOldNullValue() throws Exception {
PushOneCommit.Result r = createChange();
// Add a new vote.
@@ -656,12 +653,12 @@
gApi.changes().id(r.getChangeId()).current().review(input);
testOnPostReview.assertApproval(
- LabelId.CODE_REVIEW, /* expectedOldValue= */ 2, /* expectedNewValue= */ 2);
+ LabelId.CODE_REVIEW, /* expectedOldValue= */ null, /* expectedNewValue= */ 2);
}
}
@Test
- public void votingTheSameVoteSecondTimeFiresOnCommentAdded() throws Exception {
+ public void votingTheSameVoteSecondTimeDoesNotFireOnCommentAdded() throws Exception {
PushOneCommit.Result r = createChange();
// Add a new vote.
@@ -675,8 +672,8 @@
input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
gApi.changes().id(r.getChangeId()).current().review(input);
- assertThat(testListener.lastCommentAddedEvent.getComment())
- .isEqualTo("Patch Set 1: Code-Review+2");
+ // Event not fired.
+ assertThat(testListener.lastCommentAddedEvent).isNull();
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 67e62dd..abfd7896 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -176,12 +176,12 @@
assertThat(approval.postSubmit).isNull();
assertPermitted(gApi.changes().id(changeId).get(DETAILED_LABELS), LabelId.CODE_REVIEW, 1, 2);
- // Repeating the current label is allowed. Flips the postSubmit since technically this is a
- // new vote.
+ // Repeating the current label is allowed. Does not flip the postSubmit bit due to
+ // deduplication codepath.
gApi.changes().id(changeId).current().review(ReviewInput.recommend());
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
- assertThat(approval.postSubmit).isTrue();
+ assertThat(approval.postSubmit).isNull();
// Reducing vote is not allowed.
ResourceConflictException thrown =
@@ -193,7 +193,7 @@
.isEqualTo("Cannot reduce vote on labels for closed change: Code-Review");
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
- assertThat(approval.postSubmit).isTrue();
+ assertThat(approval.postSubmit).isNull();
// Increasing vote is allowed.
gApi.changes().id(changeId).current().review(ReviewInput.approve());
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index c33eb26..8f74f76 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -118,7 +118,7 @@
GrCommentApi,
ChangeComments,
} from '../../diff/gr-comment-api/gr-comment-api';
-import {hasOwnProperty} from '../../../utils/common-util';
+import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util';
import {GrEditControls} from '../../edit/gr-edit-controls/gr-edit-controls';
import {
CommentThread,
@@ -883,7 +883,7 @@
}
_handleCommitMessageSave(e: EditableContentSaveEvent) {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._changeNum)
throw new Error('missing required changeNum property');
// Trim trailing whitespace from each line.
@@ -1424,7 +1424,7 @@
}
_handleMessageAnchorTap(e: CustomEvent<{id: string}>) {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const hash = MSG_PREFIX + e.detail.id;
@@ -1706,7 +1706,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
if (this._patchRange.basePatchNum === ParentPatchSetNum) {
@@ -1720,7 +1720,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
if (this._patchRange.basePatchNum === ParentPatchSetNum) {
@@ -1734,7 +1734,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
@@ -1753,7 +1753,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1772,7 +1772,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
@@ -1918,7 +1918,7 @@
}
_getProjectConfig() {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
return this.restApiService
.getProjectConfig(this._change.project)
.then(config => {
@@ -2314,7 +2314,7 @@
* (`this._patchRange`) being defined.
*/
_reloadPatchNumDependentResources(rightPatchNumChanged?: boolean) {
- if (!this._changeNum) throw new Error('missing changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
if (!this._patchRange?.patchNum) throw new Error('missing patchNum');
const promises = [this._getCommitInfo(), this.$.fileList.reload()];
if (rightPatchNumChanged)
@@ -2554,7 +2554,7 @@
}
this._updateCheckTimerHandle = this.async(() => {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
const change = this._change;
fetchChangeUpdates(change, this.restApiService).then(result => {
let toastMessage = null;
@@ -2662,7 +2662,7 @@
GrEditControls
>('#editControls');
if (!controls) throw new Error('Missing edit controls');
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const path = e.detail.path;
@@ -2697,7 +2697,7 @@
if (!this._selectedRevision) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
let patchNum: PatchSetNum;
if (patchNumStr === 'edit') {
@@ -2745,7 +2745,7 @@
}
_handleStopEditTap() {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
GerritNav.navigateToChange(this._change, this._patchRange.patchNum);
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 36764f3..705a402 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
@@ -92,6 +92,7 @@
} from '@polymer/polymer/interfaces';
import {
areSetsEqual,
+ assertIsDefined,
assertNever,
containsAll,
} from '../../../utils/common-util';
@@ -433,7 +434,7 @@
}
open(focusTarget?: FocusTarget) {
- if (!this.change) throw new Error('missing required change property');
+ assertIsDefined(this.change, 'change');
this.knownLatestState = LatestPatchState.CHECKING;
fetchChangeUpdates(this.change, this.restApiService).then(result => {
this.knownLatestState = result.isLatest
@@ -605,7 +606,7 @@
account: AccountInfoInput | GroupInfoInput,
type: ReviewerType
) {
- if (!this.change) throw new Error('missing required change property');
+ assertIsDefined(this.change, 'change');
if (account._pendingAdd || !isAccount(account)) {
return;
}
@@ -1213,7 +1214,7 @@
}
cancel() {
- if (!this.change) throw new Error('missing required change property');
+ assertIsDefined(this.change, 'change');
if (!this._owner) throw new Error('missing required _owner property');
this.dispatchEvent(
new CustomEvent('cancel', {
@@ -1269,8 +1270,8 @@
}
_saveReview(review: ReviewInput, errFn?: ErrorCallback) {
- if (!this.change) throw new Error('missing required change property');
- if (!this.patchNum) throw new Error('missing required patchNum property');
+ assertIsDefined(this.change, 'change');
+ assertIsDefined(this.patchNum, 'patchNum');
return this.restApiService.saveChangeReview(
this.change._number,
this.patchNum,
@@ -1316,7 +1317,7 @@
}
_getStorageLocation(): StorageLocation {
- if (!this.change) throw new Error('missing required change property');
+ assertIsDefined(this.change, 'change');
return {
changeNum: this.change._number,
patchNum: '@change',
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 ff60531..47b4a1f 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
@@ -79,6 +79,7 @@
fireEvent,
} from '../../../utils/event-util';
import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader';
+import {assertIsDefined} from '../../../utils/common-util';
const MSG_EMPTY_BLAME = 'No blame information for this diff.';
@@ -324,8 +325,8 @@
return getPluginLoader()
.awaitPluginsLoaded()
.then(() => {
- if (!this.path) throw new Error('Missing required "path" property.');
- if (!this.changeNum) throw new Error('Missing required "changeNum".');
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.changeNum, 'changeNum');
this._layers = this._getLayers(this.path, this.changeNum);
this._coverageRanges = [];
// We kick off fetching the data here, but we don't return the promise,
@@ -341,8 +342,8 @@
*/
async reload(shouldReportMetric?: boolean) {
this.clear();
- if (!this.path) throw new Error('Missing required "path" property.');
- if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.changeNum, 'changeNum');
this.diff = undefined;
this._errorMessage = null;
const whitespaceLevel = this._getIgnoreWhitespace();
@@ -420,10 +421,10 @@
}
_getCoverageData() {
- if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
- if (!this.change) throw new Error('Missing required "change" prop.');
- if (!this.path) throw new Error('Missing required "path" prop.');
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.change, 'change');
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.patchRange, 'patchRange');
const changeNum = this.changeNum;
const change = this.change;
const path = this.path;
@@ -442,7 +443,7 @@
if (!provider) return;
provider(changeNum, path, basePatchNum, patchNum, change)
.then(coverageRanges => {
- if (!this.patchRange) throw new Error('Missing "patchRange".');
+ assertIsDefined(this.patchRange, 'patchRange');
if (
!coverageRanges ||
changeNum !== this.changeNum ||
@@ -532,9 +533,9 @@
* Load and display blame information for the base of the diff.
*/
loadBlame(): Promise<BlameInfo[]> {
- if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
- if (!this.path) throw new Error('Missing required "path" property.');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.patchRange, 'patchRange');
+ assertIsDefined(this.path, 'path');
return this.restApiService
.getBlame(this.changeNum, this.patchRange.patchNum, this.path, true)
.then(blame => {
@@ -599,9 +600,9 @@
// Wrap the diff request in a new promise so that the error handler
// rejects the promise, allowing the error to be handled in the .catch.
return new Promise((resolve, reject) => {
- if (!this.changeNum) throw new Error('Missing required "changeNum".');
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
- if (!this.path) throw new Error('Missing required "path" property.');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.patchRange, 'patchRange');
+ assertIsDefined(this.path, 'path');
this.restApiService
.getDiff(
this.changeNum,
@@ -669,7 +670,7 @@
// Report the due_to_rebase percentage in the "diff" category when
// applicable.
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
+ assertIsDefined(this.patchRange, 'patchRange');
if (this.patchRange.basePatchNum === 'PARENT') {
this.reporting.reportInteraction(EVENT_AGAINST_PARENT);
} else if (percentRebaseDelta === 0) {
@@ -726,8 +727,8 @@
}
_getImages(diff: DiffInfo) {
- if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.patchRange, 'patchRange');
return this.restApiService.getImagesForDiff(
this.changeNum,
diff,
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index b31333c..088d9cf 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -97,6 +97,7 @@
import {CustomKeyboardEvent, OpenFixPreviewEvent} from '../../../types/events';
import {fireAlert, fireTitleChange} from '../../../utils/event-util';
import {GerritView} from '../../../services/router/router-model';
+import {assertIsDefined} from '../../../utils/common-util';
const ERR_REVIEW_STATUS = 'Couldn’t change file review status.';
const MSG_LOADING_BLAME = 'Loading blame...';
const MSG_LOADED_BLAME = 'Blame loaded';
@@ -370,7 +371,7 @@
}
_getChangeEdit() {
- if (!this._changeNum) throw new Error('Missing this._changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
return this.restApiService.getChangeEdit(this._changeNum);
}
@@ -980,7 +981,7 @@
leftSide = !!this.params.leftSide;
}
}
- if (!this._patchRange) throw new Error('Failed to initialize patchRange.');
+ assertIsDefined(this._patchRange, '_patchRange');
this._initLineOfInterestAndCursor(leftSide);
if (this.params?.commentId) {
@@ -1052,10 +1053,10 @@
this._initPatchRange();
this._initCommitRange();
- if (!this._path) throw new Error('path must be defined');
+ assertIsDefined(this._path, '_path');
if (!this._changeComments)
throw new Error('change comments must be defined');
- if (!this._patchRange) throw new Error('patch range must be defined');
+ assertIsDefined(this._patchRange, '_patchRange');
// TODO(dhruvsri): check if basePath should be set here
this.$.diffHost.threads = this._changeComments.getThreadsBySideForFile(
@@ -1082,9 +1083,9 @@
if (!this._diff) throw new Error('Missing this._diff');
const fileUnchanged = this._isFileUnchanged(this._diff);
if (fileUnchanged && value.commentLink) {
- if (!this._change) throw new Error('Missing this._change');
- if (!this._path) throw new Error('Missing this._path');
- if (!this._patchRange) throw new Error('Missing this._patchRange');
+ assertIsDefined(this._change, '_change');
+ assertIsDefined(this._path, '_path');
+ assertIsDefined(this._patchRange, '_patchRange');
if (this._patchRange.basePatchNum === ParentPatchSetNum) {
// file is unchanged between Base vs X
@@ -1493,7 +1494,7 @@
}
_loadComments(patchSet?: PatchSetNum) {
- if (!this._changeNum) throw new Error('Missing this._changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
return this.$.commentAPI
.loadAll(this._changeNum, patchSet)
.then(comments => {
@@ -1529,7 +1530,7 @@
}
_getDiffDrafts() {
- if (!this._changeNum) throw new Error('Missing this._changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
return this.restApiService.getDiffDrafts(this._changeNum);
}
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 189bac7..6974a76 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -75,6 +75,7 @@
RenderPreferences,
} from '../../../api/diff';
import {isSafari} from '../../../utils/dom-util';
+import {assertIsDefined} from '../../../utils/common-util';
const NO_NEWLINE_BASE = 'No newline at end of base file.';
const NO_NEWLINE_REVISION = 'No newline at end of revision file.';
@@ -629,7 +630,7 @@
const contentEl = this.$.diffBuilder.getContentTdByLineEl(lineEl);
if (!contentEl) throw new Error('content el not found for line el');
side = side ?? this._getCommentSideByLineAndContent(lineEl, contentEl);
- if (!this.path) throw new Error('must have a path to create comments');
+ assertIsDefined(this.path, 'path');
this.dispatchEvent(
new CustomEvent<CreateCommentEventDetail>('create-comment', {
bubbles: true,
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
index f6f4395..1864598 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
@@ -45,6 +45,7 @@
import {fireAlert, fireTitleChange} from '../../../utils/event-util';
import {appContext} from '../../../services/app-context';
import {ErrorCallback} from '../../../api/rest';
+import {assertIsDefined} from '../../../utils/common-util';
const RESTORED_MESSAGE = 'Content restored from a previous edit.';
const SAVING_MESSAGE = 'Saving changes...';
@@ -326,7 +327,7 @@
}
_handlePublishTap() {
- if (!this._changeNum) throw new Error('missing changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
const changeNum = this._changeNum;
this._saveEdit().then(() => {
@@ -347,7 +348,7 @@
handleError
)
.then(() => {
- if (!this._change) throw new Error('missing change');
+ assertIsDefined(this._change, '_change');
GerritNav.navigateToChange(this._change);
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index a9b910d..a5b7df7 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -60,7 +60,7 @@
import {KnownExperimentId} from '../../../services/flags/flags';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
import {RenderPreferences} from '../../../api/diff';
-import {check, checkProperty} from '../../../utils/common-util';
+import {check, assertIsDefined} from '../../../utils/common-util';
const UNRESOLVED_EXPAND_COUNT = 5;
const NEWLINE_PATTERN = /\n/g;
@@ -334,8 +334,8 @@
}
_getUrlForViewDiff(comments: UIComment[]) {
- checkProperty(!!this.changeNum, 'changeNum');
- checkProperty(!!this.projectName, 'projectName');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.projectName, 'projectName');
check(comments.length > 0, 'comment not found');
return GerritNav.getUrlForComment(
this.changeNum,
@@ -633,8 +633,8 @@
}
_handleCommentDiscard(e: Event) {
- if (!this.changeNum) throw new Error('changeNum is missing');
- if (!this.patchNum) throw new Error('patchNum is missing');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.patchNum, 'patchNum');
const diffCommentEl = (dom(e) as EventApi).rootTarget as GrComment;
const comment = diffCommentEl.comment;
const idx = this._indexOf(comment, this.comments);
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 898aff3..bf376f6 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -61,6 +61,7 @@
import {OpenFixPreviewEventDetail} from '../../../types/events';
import {fireAlert} from '../../../utils/event-util';
import {pluralize} from '../../../utils/string-util';
+import {assertIsDefined} from '../../../utils/common-util';
const STORAGE_DEBOUNCE_INTERVAL = 400;
const TOAST_DEBOUNCE_INTERVAL = 200;
@@ -322,7 +323,7 @@
}
_handlePortedMessageClick() {
- if (!this.comment) throw new Error('comment not set');
+ assertIsDefined(this.comment, 'comment');
this.reporting.reportInteraction('navigate-to-original-comment', {
line: this.comment.line,
range: this.comment.range,
@@ -496,10 +497,8 @@
// prior to it being saved.
this.cancelDebouncer(DEBOUNCER_STORE);
- if (!this.comment?.path) throw new Error('Cannot erase Draft Comment');
- if (this.changeNum === undefined) {
- throw new Error('undefined changeNum');
- }
+ assertIsDefined(this.comment?.path, 'comment.path');
+ assertIsDefined(this.changeNum, 'changeNum');
this.storage.eraseDraftComment({
changeNum: this.changeNum,
patchNum: this._getPatchNum(),
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 592f7903..ddae8ea 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
@@ -45,6 +45,7 @@
import {ReviewerState} from '../../../constants/constants';
import {CURRENT} from '../../../utils/patch-set-util';
import {isInvolved, isRemovableReviewer} from '../../../utils/change-util';
+import {assertIsDefined} from '../../../utils/common-util';
@customElement('gr-hovercard-account')
export class GrHovercardAccount extends GestureEventListeners(
@@ -163,7 +164,7 @@
}
_handleChangeReviewerOrCCStatus() {
- if (!this.change) throw new Error('expected change object to be present');
+ assertIsDefined(this.change, 'change');
// accountKey() throws an error if _account_id & email is not found, which
// we want to check before showing reloading toast
const _accountKey = accountKey(this.account);
diff --git a/polygerrit-ui/app/utils/common-util.ts b/polygerrit-ui/app/utils/common-util.ts
index ad76b79..f95105d 100644
--- a/polygerrit-ui/app/utils/common-util.ts
+++ b/polygerrit-ui/app/utils/common-util.ts
@@ -67,6 +67,18 @@
}
/**
+ * Throws an error if the property is not defined.
+ */
+export function assertIsDefined<T>(
+ val: T,
+ variableName = 'variable'
+): asserts val is NonNullable<T> {
+ if (val === undefined || val === null) {
+ throw new Error(`${variableName} is not defined`);
+ }
+}
+
+/**
* Returns true, if both sets contain the same members.
*/
export function areSetsEqual<T>(a: Set<T>, b: Set<T>): boolean {
