Merge "Add unit tests for the coverage provider."
diff --git a/java/com/google/gerrit/server/comment/CommentContextKey.java b/java/com/google/gerrit/server/comment/CommentContextKey.java
index e4a927a..ccd50b7 100644
--- a/java/com/google/gerrit/server/comment/CommentContextKey.java
+++ b/java/com/google/gerrit/server/comment/CommentContextKey.java
@@ -6,11 +6,10 @@
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.cache.proto.Cache;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
-import java.util.Collection;
/**
* An identifier of a comment that should be used to load the comment context using {@link
- * CommentContextCache#get(CommentContextKey)}, or {@link CommentContextCache#getAll(Collection)}.
+ * CommentContextCache#get(CommentContextKey)}, or {@link CommentContextCache#getAll(Iterable)}.
*
* <p>The {@link CommentContextCacheImpl} implementation uses this class as the cache key, while
* replacing the {@link #path()} field with the hashed path.
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index b816264..e8f1fe1 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -138,8 +138,9 @@
}
/**
- * Create change notes based on a {@link Change.Id}. This requires using the Change index and
- * should only be used when {@link Project.NameKey} and the numeric change ID are not available.
+ * Create change notes based on a {@link com.google.gerrit.entities.Change.Id}. This requires
+ * using the Change index and should only be used when {@link
+ * com.google.gerrit.entities.Project.NameKey} and the numeric change ID are not available.
*/
public ChangeNotes createCheckedUsingIndexLookup(Change.Id changeId) {
InternalChangeQuery query = queryProvider.get().noFields();
@@ -155,9 +156,9 @@
}
/**
- * Create change notes based on a list of {@link Change.Id}s. This requires using the Change
- * index and should only be used when {@link Project.NameKey} and the numeric change ID are not
- * available.
+ * Create change notes based on a list of {@link com.google.gerrit.entities.Change.Id}s. This
+ * requires using the Change index and should only be used when {@link
+ * com.google.gerrit.entities.Project.NameKey} and the numeric change ID are not available.
*/
public List<ChangeNotes> createUsingIndexLookup(Collection<Change.Id> changeIds) {
List<ChangeNotes> notes = new ArrayList<>();
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/FileHeaderUtil.java b/java/com/google/gerrit/server/patch/gitfilediff/FileHeaderUtil.java
index 9827a69..8f53751 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/FileHeaderUtil.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/FileHeaderUtil.java
@@ -56,7 +56,8 @@
/**
* Returns the old file path associated with the {@link FileHeader}, or empty if the file is
- * {@link Patch.ChangeType#ADDED} or {@link Patch.ChangeType#REWRITE}.
+ * {@link com.google.gerrit.entities.Patch.ChangeType#ADDED} or {@link
+ * com.google.gerrit.entities.Patch.ChangeType#REWRITE}.
*/
static Optional<String> getOldPath(FileHeader header) {
Patch.ChangeType changeType = getChangeType(header);
@@ -76,7 +77,7 @@
/**
* Returns the new file path associated with the {@link FileHeader}, or empty if the file is
- * {@link Patch.ChangeType#DELETED}.
+ * {@link com.google.gerrit.entities.Patch.ChangeType#DELETED}.
*/
static Optional<String> getNewPath(FileHeader header) {
Patch.ChangeType changeType = getChangeType(header);
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
index 41db9ee..66299a8 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
@@ -99,8 +99,8 @@
}
/**
- * Returns the {@link Account.Id} of the current user if a user is signed in. Catches exceptions
- * so that background jobs don't get impacted.
+ * Returns the {@link com.google.gerrit.entities.Account.Id} of the current user if a user is
+ * signed in. Catches exceptions so that background jobs don't get impacted.
*/
private Optional<Account.Id> getAccountIdOfIdentifiedUser() {
try {
diff --git a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
index cb34bdb..7f8add8 100644
--- a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
@@ -18,6 +18,13 @@
import static org.apache.http.HttpStatus.SC_CREATED;
import static org.apache.http.HttpStatus.SC_INTERNAL_SERVER_ERROR;
import static org.apache.http.HttpStatus.SC_OK;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.timeout;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyZeroInteractions;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
@@ -52,7 +59,6 @@
import com.google.gerrit.server.validators.ProjectCreationValidationListener;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
-import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
@@ -370,37 +376,31 @@
@Test
public void performanceLoggingForRestCall() throws Exception {
- TestPerformanceLogger testPerformanceLogger = new TestPerformanceLogger();
+ PerformanceLogger testPerformanceLogger = mock(PerformanceLogger.class);
try (Registration registration =
extensionRegistry.newRegistration().add(testPerformanceLogger)) {
RestResponse response = adminRestSession.put("/projects/new10");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
-
- // This assertion assumes that the server invokes the PerformanceLogger plugins before it
- // sends
- // the response to the client. If this assertion gets flaky it's likely that this got changed
- // on
- // server-side.
- assertThat(testPerformanceLogger.logEntries()).isNotEmpty();
+ verify(testPerformanceLogger, timeout(5000).atLeastOnce()).log(anyString(), anyLong(), any());
}
}
@Test
public void performanceLoggingForPush() throws Exception {
- TestPerformanceLogger testPerformanceLogger = new TestPerformanceLogger();
+ PerformanceLogger testPerformanceLogger = mock(PerformanceLogger.class);
try (Registration registration =
extensionRegistry.newRegistration().add(testPerformanceLogger)) {
PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
PushOneCommit.Result r = push.to("refs/heads/master");
r.assertOkStatus();
- assertThat(testPerformanceLogger.logEntries()).isNotEmpty();
+ verify(testPerformanceLogger, timeout(5000).atLeastOnce()).log(anyString(), anyLong(), any());
}
}
@Test
@GerritConfig(name = "tracing.performanceLogging", value = "false")
public void noPerformanceLoggingIfDisabled() throws Exception {
- TestPerformanceLogger testPerformanceLogger = new TestPerformanceLogger();
+ PerformanceLogger testPerformanceLogger = mock(PerformanceLogger.class);
try (Registration registration =
extensionRegistry.newRegistration().add(testPerformanceLogger)) {
RestResponse response = adminRestSession.put("/projects/new11");
@@ -410,7 +410,7 @@
PushOneCommit.Result r = push.to("refs/heads/master");
r.assertOkStatus();
- assertThat(testPerformanceLogger.logEntries()).isEmpty();
+ verifyZeroInteractions(testPerformanceLogger);
}
}
@@ -844,19 +844,6 @@
}
}
- private static class TestPerformanceLogger implements PerformanceLogger {
- private List<PerformanceLogEntry> logEntries = new ArrayList<>();
-
- @Override
- public void log(String operation, long durationMs, Metadata metadata) {
- logEntries.add(PerformanceLogEntry.create(operation, metadata));
- }
-
- ImmutableList<PerformanceLogEntry> logEntries() {
- return ImmutableList.copyOf(logEntries);
- }
- }
-
@AutoValue
abstract static class PerformanceLogEntry {
static PerformanceLogEntry create(String operation, Metadata metadata) {
diff --git a/plugins/gitiles b/plugins/gitiles
index a33c8b8..0cd08bb 160000
--- a/plugins/gitiles
+++ b/plugins/gitiles
@@ -1 +1 @@
-Subproject commit a33c8b8d61b778f8ca84196b1a7cc1fd4fe24946
+Subproject commit 0cd08bbfac1a938079be8aaf6a6eef0e82ff9a41
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 471f638..069d4a9 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -45,6 +45,7 @@
} from '../../../utils/patch-set-util';
import {
changeIsOpen,
+ isOwner,
ListChangesOption,
listChangesOptionsToHex,
} from '../../../utils/change-util';
@@ -66,6 +67,7 @@
ErrorCallback,
} from '../../../services/services/gr-rest-api/gr-rest-api';
import {
+ AccountInfo,
ActionInfo,
ActionNameToActionInfoMap,
BranchName,
@@ -113,7 +115,11 @@
UIActionInfo,
} from '../../shared/gr-js-api-interface/gr-change-actions-js-api';
import {fireAlert} from '../../../utils/event-util';
-import {CODE_REVIEW} from '../../../utils/label-util';
+import {
+ CODE_REVIEW,
+ getApprovalInfo,
+ getVotingRange,
+} from '../../../utils/label-util';
const ERR_BRANCH_EMPTY = 'The destination branch can’t be empty.';
const ERR_COMMIT_EMPTY = 'The commit message can’t be empty.';
@@ -402,6 +408,9 @@
@property({type: Boolean})
_hideQuickApproveAction = false;
+ @property({type: Object})
+ account?: AccountInfo;
+
@property({type: String})
changeNum?: NumericChangeId;
@@ -949,29 +958,38 @@
}
}
// Allow the user to use quick approve to vote the max score on code review
- // even if it is already granted.
+ // even if it is already granted by someone else. Does not apply if the
+ // user owns the change or has already granted the max score themselves.
+ const codeReviewLabel = this.change.labels[CODE_REVIEW];
+ const codeReviewPermittedValues = this.change.permitted_labels[CODE_REVIEW];
if (
!result &&
- this.change.labels[CODE_REVIEW] &&
- this._getLabelStatus(this.change.labels[CODE_REVIEW]) ===
- LabelStatus.OK &&
- this.change.permitted_labels[CODE_REVIEW]
+ codeReviewLabel &&
+ codeReviewPermittedValues &&
+ this.account?._account_id &&
+ isDetailedLabelInfo(codeReviewLabel) &&
+ this._getLabelStatus(codeReviewLabel) === LabelStatus.OK &&
+ !isOwner(this.change, this.account) &&
+ getApprovalInfo(codeReviewLabel, this.account)?.value !==
+ getVotingRange(codeReviewLabel)?.max
) {
result = CODE_REVIEW;
}
if (result) {
- const score = this.change.permitted_labels[result].slice(-1)[0];
const labelInfo = this.change.labels[result];
if (!isDetailedLabelInfo(labelInfo)) {
return null;
}
- const maxScore = Object.keys(labelInfo.values).slice(-1)[0];
- if (score === maxScore) {
+ const permittedValues = this.change.permitted_labels[result];
+ const usersMaxPermittedScore =
+ permittedValues[permittedValues.length - 1];
+ const maxScoreForLabel = getVotingRange(labelInfo)?.max;
+ if (Number(usersMaxPermittedScore) === maxScoreForLabel) {
// Allow quick approve only for maximal score.
return {
label: result,
- score,
+ score: usersMaxPermittedScore,
};
}
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
index 3b6a2bf..5a06ceb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
@@ -21,6 +21,8 @@
import {GerritNav} from '../../core/gr-navigation/gr-navigation.js';
import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader.js';
import {
+ createAccountWithId,
+ createApproval,
createChange,
createChangeMessages,
createRevisions,
@@ -105,6 +107,9 @@
enabled: true,
},
};
+ element.account = {
+ _account_id: 123,
+ };
sinon.stub(appContext.restApiService, 'getRepoBranches').returns(
Promise.resolve([]));
@@ -1525,9 +1530,6 @@
setup(() => {
element.change = {
current_revision: 'abc1234',
- };
- element.change = {
- current_revision: 'abc1234',
labels: {
foo: {
values: {
@@ -1734,10 +1736,66 @@
};
flush();
const approveButton =
- element.shadowRoot
- .querySelector('gr-button[data-action-key=\'review\']');
+ element.shadowRoot
+ .querySelector('gr-button[data-action-key=\'review\']');
assert.isNotNull(approveButton);
});
+
+ test('not added when the user has already approved', () => {
+ const vote = {
+ ...createApproval(),
+ _account_id: 123,
+ name: 'name',
+ value: 2,
+ };
+ element.change = {
+ current_revision: 'abc1234',
+ labels: {
+ 'Code-Review': {
+ approved: {},
+ values: {
+ ' 0': '',
+ '+1': '',
+ '+2': '',
+ },
+ all: [vote],
+ },
+ },
+ permitted_labels: {
+ 'Code-Review': [' 0', '+1', '+2'],
+ },
+ };
+ flush();
+ const approveButton =
+ element.shadowRoot
+ .querySelector('gr-button[data-action-key=\'review\']');
+ assert.isNull(approveButton);
+ });
+
+ test('not added when user owns the change', () => {
+ element.change = {
+ current_revision: 'abc1234',
+ owner: createAccountWithId(123),
+ labels: {
+ 'Code-Review': {
+ approved: {},
+ values: {
+ ' 0': '',
+ '+1': '',
+ '+2': '',
+ },
+ },
+ },
+ permitted_labels: {
+ 'Code-Review': [' 0', '+1', '+2'],
+ },
+ };
+ flush();
+ const approveButton =
+ element.shadowRoot
+ .querySelector('gr-button[data-action-key=\'review\']');
+ assert.isNull(approveButton);
+ });
});
test('adds download revision action', () => {
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 7225bb9..cb8ed6a 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
@@ -405,6 +405,7 @@
has-parent="[[hasParent]]"
actions="[[_change.actions]]"
revision-actions="{{_currentRevisionActions}}"
+ account="[[_account]]"
change-num="[[_changeNum]]"
change-status="[[_change.status]]"
commit-num="[[_commitInfo.commit]]"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index f3ccd61..8fce39a 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -1633,14 +1633,9 @@
return;
}
- // Comments are not returned with the commentSide attribute from
- // the api, but it's necessary to be stored on the diff's
- // comments due to use in the _handleCommentUpdate function.
- // The comment thread already has a side associated with it, so
- // set the comment's side to match.
- threadEl.comments = newComments.map(c =>
- Object.assign(c, {diffSide: threadEl.diffSide})
- );
+ threadEl.comments = newComments.map(c => {
+ return {...c};
+ });
flush();
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-utils.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-utils.ts
index eebcb15..038153a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-utils.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-utils.ts
@@ -83,7 +83,7 @@
// For Gerrit these are instances of GrCommentThread, but other gr-diff users
// have different HTML elements in use for comment threads.
// TODO: Also document the required HTML attritbutes that thread elements must
-// have, e.g. 'comment-side', 'range', 'line-num', 'data-value'.
+// have, e.g. 'diff-side', 'range', 'line-num', 'data-value'.
export interface GrDiffThreadElement extends HTMLElement {
rootId: string;
}
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 2c8b704..557d1eb 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
@@ -552,9 +552,9 @@
/** Make comments selectable when selected */
.selected-left.selected-comment
- ::slotted(gr-comment-thread[comment-side='left']),
+ ::slotted(gr-comment-thread[diff-side='left']),
.selected-right.selected-comment
- ::slotted(gr-comment-thread[comment-side='right']) {
+ ::slotted(gr-comment-thread[diff-side='right']) {
-webkit-user-select: text;
-moz-user-select: text;
-ms-user-select: text;
diff --git a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.ts b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.ts
index cd701fc..68a73933 100644
--- a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.ts
+++ b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.ts
@@ -20,7 +20,7 @@
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-overlay_html';
import {IronOverlayMixin} from '../../../mixins/iron-overlay-mixin/iron-overlay-mixin';
-import {customElement, property} from '@polymer/decorators';
+import {customElement} from '@polymer/decorators';
import {IronOverlayBehavior} from '@polymer/iron-overlay-behavior/iron-overlay-behavior';
import {findActiveElement} from '../../../utils/dom-util';
import {fireEvent} from '../../../utils/event-util';
@@ -56,7 +56,6 @@
* @event fullscreen-overlay-opened
*/
- @property({type: Boolean})
private _fullScreenOpen = false;
private _boundHandleClose: () => void = () => super.close();
diff --git a/polygerrit-ui/app/utils/label-util.ts b/polygerrit-ui/app/utils/label-util.ts
index 4313745..fc83d77 100644
--- a/polygerrit-ui/app/utils/label-util.ts
+++ b/polygerrit-ui/app/utils/label-util.ts
@@ -15,7 +15,9 @@
* limitations under the License.
*/
import {
+ AccountInfo,
ApprovalInfo,
+ DetailedLabelInfo,
isDetailedLabelInfo,
LabelInfo,
VotingRangeInfo,
@@ -42,3 +44,10 @@
const votingRange = getVotingRangeOrDefault(label);
return label.all.filter(account => account.value === votingRange.max);
}
+
+export function getApprovalInfo(
+ label: DetailedLabelInfo,
+ account: AccountInfo
+): ApprovalInfo | undefined {
+ return label.all?.filter(x => x._account_id === account._account_id)[0];
+}
diff --git a/polygerrit-ui/app/utils/label-util_test.js b/polygerrit-ui/app/utils/label-util_test.js
index d6f7b3e..6a2f768 100644
--- a/polygerrit-ui/app/utils/label-util_test.js
+++ b/polygerrit-ui/app/utils/label-util_test.js
@@ -20,6 +20,7 @@
getVotingRange,
getVotingRangeOrDefault,
getMaxAccounts,
+ getApprovalInfo,
} from './label-util.js';
const VALUES_1 = {
@@ -87,4 +88,29 @@
assert.isEmpty(getMaxAccounts({}));
assert.isEmpty(getMaxAccounts({values: VALUES_2}));
});
+
+ test('getApprovalInfo', () => {
+ const myAccountInfo = {_account_id: 314};
+ const myApprovalInfo = {value: 2, _account_id: 314};
+ const label = {
+ values: VALUES_2,
+ all: [myApprovalInfo, {value: 1, _account_id: 777}],
+ };
+ assert.equal(
+ getApprovalInfo(label, myAccountInfo),
+ myApprovalInfo
+ );
+ });
+
+ test('getApprovalInfo no approval for user', () => {
+ const myAccountInfo = {_account_id: 123};
+ const label = {
+ values: VALUES_2,
+ all: [
+ {value: 2, _account_id: 314},
+ {value: 1, _account_id: 777},
+ ],
+ };
+ assert.isUndefined(getApprovalInfo(label, myAccountInfo));
+ });
});
