Merge "Revert accidental change to test output level"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 2810d1e..aceb38e 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -3799,6 +3799,10 @@
If another user removed a user's vote, the user with the deleted vote will be
added to the attention set.
+The request returns:
+ * '204 No Content' if the vote is deleted successfully;
+ * '404 Not Found' when the vote to be deleted is zero or not present.
+
.Request
----
DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/reviewers/John%20Doe/votes/Code-Review HTTP/1.0
diff --git a/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java
index 1dcd662..5efcfc6 100644
--- a/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java
@@ -150,6 +150,7 @@
String refName = RefNames.fullName(changeCreation.branch());
ChangeInserter inserter = getChangeInserter(changeId, refName, commitId);
+ inserter.setGroups(getGroups(changeCreation));
changeCreation.topic().ifPresent(t -> inserter.setTopic(t));
inserter.setApprovals(changeCreation.approvals());
@@ -243,9 +244,73 @@
return createCommit(objectInserter, tree, parentCommits, author, committer, commitMessage);
}
+ private ImmutableList<String> getGroups(TestChangeCreation changeCreation) {
+ return changeCreation
+ .parents()
+ .map(parents -> getGroups(parents))
+ .orElseGet(() -> ImmutableList.of());
+ }
+
+ private ImmutableList<String> getGroups(ImmutableList<TestCommitIdentifier> parents) {
+ return parents.stream()
+ .map(parent -> getGroups(parent))
+ .flatMap(groups -> groups.stream())
+ .collect(toImmutableList());
+ }
+
+ private ImmutableList<String> getGroups(TestCommitIdentifier parentCommit) {
+ switch (parentCommit.getKind()) {
+ case BRANCH:
+ return ImmutableList.of();
+ case CHANGE_ID:
+ return getGroupsFromChange(parentCommit.changeId());
+ case COMMIT_SHA_1:
+ return ImmutableList.of();
+ case PATCHSET_ID:
+ return getGroupsFromPatchset(parentCommit.patchsetId());
+ default:
+ throw new IllegalStateException(
+ String.format("No parent behavior implemented for %s.", parentCommit.getKind()));
+ }
+ }
+
+ private ImmutableList<String> getGroupsFromChange(Change.Id changeId) {
+ Optional<ChangeNotes> changeNotes = changeFinder.findOne(changeId);
+
+ if (changeNotes.isPresent() && changeNotes.get().getChange().isClosed()) {
+ return ImmutableList.of();
+ }
+
+ return changeNotes
+ .map(ChangeNotes::getCurrentPatchSet)
+ .map(PatchSet::groups)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format(
+ "Change %s not found and hence can't be used as parent.", changeId)));
+ }
+
+ private ImmutableList<String> getGroupsFromPatchset(PatchSet.Id patchsetId) {
+ Optional<ChangeNotes> changeNotes = changeFinder.findOne(patchsetId.changeId());
+
+ if (changeNotes.isPresent() && changeNotes.get().getChange().isClosed()) {
+ return ImmutableList.of();
+ }
+
+ return changeNotes
+ .map(ChangeNotes::getPatchSets)
+ .map(patchsets -> patchsets.get(patchsetId))
+ .map(PatchSet::groups)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format(
+ "Patchset %s not found and hence can't be used as parent.", patchsetId)));
+ }
+
private ImmutableList<ObjectId> getParentCommits(
Repository repository, RevWalk revWalk, TestChangeCreation changeCreation) {
-
return changeCreation
.parents()
.map(parents -> resolveParents(repository, revWalk, parents))
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index b40e100..dc9bc32 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -174,4 +174,14 @@
submitted = Timestamp.from(when);
submitter = who;
}
+
+ public RevisionInfo getCurrentRevision() {
+ RevisionInfo currentRevisionInfo = revisions.get(currentRevision);
+ if (currentRevisionInfo.commit != null) {
+ // If all revisions are requested the commit.commit field is not populated because the commit
+ // SHA1 is already present as the key in the revisions map.
+ currentRevisionInfo.commit.commit = currentRevision;
+ }
+ return currentRevisionInfo;
+ }
}
diff --git a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
index 92788b7..fb28d30 100644
--- a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
+++ b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
@@ -101,6 +101,7 @@
IndexPreloadingUtil.computeChangeRequestsPath(requestedPath, page).get());
data.put("changeNum", IndexPreloadingUtil.computeChangeNum(requestedPath, page).get());
break;
+ case PROFILE:
case DASHBOARD:
// Dashboard is preloaded queries are added later when we check user is authenticated.
case PAGE_WITHOUT_PRELOADING:
diff --git a/java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java b/java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java
index afaeaf6..402e48a 100644
--- a/java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java
+++ b/java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java
@@ -40,6 +40,7 @@
CHANGE,
DIFF,
DASHBOARD,
+ PROFILE,
PAGE_WITHOUT_PRELOADING,
}
@@ -50,6 +51,7 @@
public static final Pattern DIFF_URL_PATTERN =
Pattern.compile(CHANGE_CANONICAL_PATH + BASE_PATCH_NUM_PATH_PART + "(/(.+))" + "/?$");
public static final Pattern DASHBOARD_PATTERN = Pattern.compile("/dashboard/self$");
+ public static final Pattern PROFILE_PATTERN = Pattern.compile("/profile/self$");
public static final String ROOT_PATH = "/";
// These queries should be kept in sync with PolyGerrit:
@@ -130,6 +132,11 @@
return RequestedPage.DASHBOARD;
}
+ Matcher profileMatcher = IndexPreloadingUtil.PROFILE_PATTERN.matcher(requestedPath);
+ if (profileMatcher.matches()) {
+ return RequestedPage.PROFILE;
+ }
+
if (ROOT_PATH.equals(requestedPath)) {
return RequestedPage.DASHBOARD;
}
@@ -148,6 +155,7 @@
matcher = DIFF_URL_PATTERN.matcher(requestedURL);
break;
case DASHBOARD:
+ case PROFILE:
case PAGE_WITHOUT_PRELOADING:
default:
return Optional.empty();
@@ -172,6 +180,7 @@
matcher = DIFF_URL_PATTERN.matcher(requestedURL);
break;
case DASHBOARD:
+ case PROFILE:
case PAGE_WITHOUT_PRELOADING:
default:
return Optional.empty();
diff --git a/java/com/google/gerrit/httpd/raw/StaticModule.java b/java/com/google/gerrit/httpd/raw/StaticModule.java
index 961bf9b..8319d9d 100644
--- a/java/com/google/gerrit/httpd/raw/StaticModule.java
+++ b/java/com/google/gerrit/httpd/raw/StaticModule.java
@@ -77,6 +77,7 @@
"/x/*",
"/admin/*",
"/dashboard/*",
+ "/profile/*",
"/groups/self",
"/settings/*",
"/Documentation/q/*");
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 703693d..1818b1b 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -25,6 +25,7 @@
import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -43,6 +44,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TreeSet;
@@ -421,13 +423,70 @@
private class ByEmail extends StringSearcher {
@Override
+ public boolean requiresContextUser() {
+ return true;
+ }
+
+ @Override
protected boolean matches(String input) {
return input.contains("@");
}
@Override
- public Stream<AccountState> search(String input) throws IOException {
- return toAccountStates(emails.getAccountFor(input));
+ public Stream<AccountState> search(String input, CurrentUser asUser) throws IOException {
+ boolean canSeeSecondaryEmails = false;
+ try {
+ if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
+ canSeeSecondaryEmails = true;
+ }
+ } catch (PermissionBackendException e) {
+ // remains false
+ }
+
+ if (canSeeSecondaryEmails) {
+ return toAccountStates(emails.getAccountFor(input));
+ }
+
+ // User cannot see secondary emails, hence search by preferred email only.
+ List<AccountState> accountStates = accountQueryProvider.get().byPreferredEmail(input);
+
+ if (accountStates.size() == 1) {
+ return Stream.of(Iterables.getOnlyElement(accountStates));
+ }
+
+ if (accountStates.size() > 1) {
+ // An email can only belong to a single account. If multiple accounts are found it means
+ // there is an inconsistency, i.e. some of the found accounts have a preferred email set
+ // that they do not own via an external ID. Hence in this case we return only the one
+ // account that actually owns the email via an external ID.
+ for (AccountState accountState : accountStates) {
+ if (accountState.externalIds().stream()
+ .map(ExternalId::email)
+ .filter(Objects::nonNull)
+ .anyMatch(email -> email.equals(input))) {
+ return Stream.of(accountState);
+ }
+ }
+
+ // None of the matched accounts owns the email, return all matches to be consistent with
+ // the behavior of Emails.getAccountFor(String) that is used above if the user can see
+ // secondary emails.
+ return accountStates.stream();
+ }
+
+ // No match by preferred email. Since users can always see their own secondary emails, check
+ // if the input matches a secondary email of the user and if yes, return the account of the
+ // user.
+ if (asUser.isIdentifiedUser()
+ && asUser.asIdentifiedUser().state().externalIds().stream()
+ .map(ExternalId::email)
+ .filter(Objects::nonNull)
+ .anyMatch(email -> email.equals(input))) {
+ return Stream.of(asUser.asIdentifiedUser().state());
+ }
+
+ // No match.
+ return Stream.empty();
}
@Override
@@ -671,6 +730,18 @@
input, searchers, self.get(), this::allVisiblePredicate, AccountResolver::isActive);
}
+ public Result resolveAsUserIgnoreVisibility(CurrentUser asUser, String input)
+ throws ConfigInvalidException, IOException {
+ return resolveAsUserIgnoreVisibility(asUser, input, AccountResolver::isActive);
+ }
+
+ public Result resolveAsUserIgnoreVisibility(
+ CurrentUser asUser, String input, Predicate<AccountState> accountActivityPredicate)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(
+ input, searchers, asUser, this::allVisiblePredicate, accountActivityPredicate);
+ }
+
/**
* Resolves all accounts matching the input string by name or email.
*
diff --git a/java/com/google/gerrit/server/account/Emails.java b/java/com/google/gerrit/server/account/Emails.java
index 8c3f033..13385d0 100644
--- a/java/com/google/gerrit/server/account/Emails.java
+++ b/java/com/google/gerrit/server/account/Emails.java
@@ -57,12 +57,11 @@
* are needed it is more efficient to use {@link #getAccountsFor(String...)} as this method reads
* the SHA1 of the refs/meta/external-ids branch only once (and not once per email).
*
- * <p>In addition accounts are included that have the given email as preferred email even if they
- * have no external ID for the preferred email. Having accounts with a preferred email that does
- * not exist as external ID is an inconsistency, but existing functionality relies on still
- * getting those accounts, which is why they are included. Accounts by preferred email are fetched
- * from the account index as a fallback for email addresses that could not be resolved using
- * {@link ExternalIds}.
+ * <p>If there is no account that owns the email via an external ID all accounts that have the
+ * email set as a preferred email are returned. Having accounts with a preferred email that does
+ * not exist as external ID is an inconsistency, but existing functionality relies on getting
+ * those accounts, which is why they are returned as a fall-back by fetching them from the account
+ * index.
*
* @see #getAccountsFor(String...)
*/
diff --git a/java/com/google/gerrit/server/change/RebaseUtil.java b/java/com/google/gerrit/server/change/RebaseUtil.java
index 8acc925..65fef05 100644
--- a/java/com/google/gerrit/server/change/RebaseUtil.java
+++ b/java/com/google/gerrit/server/change/RebaseUtil.java
@@ -18,29 +18,38 @@
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gerrit.server.update.BatchUpdate;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -50,6 +59,11 @@
public class RebaseUtil {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final Provider<PersonIdent> serverIdent;
+ private final IdentifiedUser.GenericFactory userFactory;
+ private final PermissionBackend permissionBackend;
+ private final ChangeResource.Factory changeResourceFactory;
+ private final GitRepositoryManager repoManager;
private final Provider<InternalChangeQuery> queryProvider;
private final ChangeNotes.Factory notesFactory;
private final PatchSetUtil psUtil;
@@ -57,10 +71,20 @@
@Inject
RebaseUtil(
+ @GerritPersonIdent Provider<PersonIdent> serverIdent,
+ IdentifiedUser.GenericFactory userFactory,
+ PermissionBackend permissionBackend,
+ ChangeResource.Factory changeResourceFactory,
+ GitRepositoryManager repoManager,
Provider<InternalChangeQuery> queryProvider,
ChangeNotes.Factory notesFactory,
PatchSetUtil psUtil,
RebaseChangeOp.Factory rebaseFactory) {
+ this.serverIdent = serverIdent;
+ this.userFactory = userFactory;
+ this.permissionBackend = permissionBackend;
+ this.changeResourceFactory = changeResourceFactory;
+ this.repoManager = repoManager;
this.queryProvider = queryProvider;
this.notesFactory = notesFactory;
this.psUtil = psUtil;
@@ -68,6 +92,143 @@
}
/**
+ * Checks that the uploader has permissions to create a new patch set and creates a new {@link
+ * RevisionResource} that contains the uploader (aka the impersonated user) as the current user
+ * which can be used for {@link BatchUpdate} to do the rebase on behalf of the uploader.
+ *
+ * <p>The following permissions are required for the uploader:
+ *
+ * <ul>
+ * <li>The {@code Read} permission that allows to see the change.
+ * <li>The {@code Push} permission that allows upload.
+ * <li>The {@code Add Patch Set} permission, required if the change is owned by another user
+ * (change owners implicitly have this permission).
+ * <li>The {@code Forge Author} permission if the patch set that is rebased has a forged author
+ * (author != uploader).
+ * <li>The {@code Forge Server} permission if the patch set that is rebased has the server
+ * identity as the author.
+ * </ul>
+ *
+ * <p>Usually the uploader should have all these permission since they were already required for
+ * the original upload, but there is the edge case that the uploader had the permission when doing
+ * the original upload and then the permission was revoked.
+ *
+ * <p>Note that patch sets with a forged committer (committer != uploader) can be rebased on
+ * behalf of the uploader, even if the uploader doesn't have the {@code Forge Committer}
+ * permission. This is because on rebase on behalf of the uploader the uploader will become the
+ * committer of the new rebased patch set, hence for the rebased patch set the committer is no
+ * longer forged (committer == uploader) and hence the {@code Forge Committer} permission is not
+ * required.
+ *
+ * <p>Note that the {@code Rebase} permission is not required for the uploader since the {@code
+ * Rebase} permission is specifically about allowing a user to do a rebase via the web UI by
+ * clicking on the {@code REBASE} button and the uploader is not clicking on this button.
+ *
+ * <p>The permissions of the uploader are checked explicitly here so that we can return a {@code
+ * 409 Conflict} response with a proper error message if they are missing (the error message says
+ * that the permission is missing for the uploader). The normal code path also checks these
+ * permission but the exception thrown there would result in a {@code 403 Forbidden} response and
+ * the error message would wrongly look like the caller (i.e. the rebaser) is missing the
+ * permission.
+ *
+ * <p>Note that this method doesn't check permissions for the rebaser (aka the impersonating user
+ * aka the calling user). Callers should check the permissions for the rebaser before calling this
+ * method.
+ *
+ * @param rsrc the revision resource that should be rebased
+ * @param rebaseInput the request input containing options for the rebase
+ * @return revision resource that contains the uploader (aka the impersonated user) as the current
+ * user which can be used for {@link BatchUpdate} to do the rebase on behalf of the uploader
+ */
+ public RevisionResource onBehalfOf(RevisionResource rsrc, RebaseInput rebaseInput)
+ throws IOException, PermissionBackendException, BadRequestException,
+ ResourceConflictException {
+ if (rsrc.getPatchSet().id().get() != rsrc.getChange().currentPatchSetId().get()) {
+ throw new BadRequestException(
+ "non-current patch set cannot be rebased on behalf of the uploader");
+ }
+ if (rebaseInput.allowConflicts) {
+ throw new BadRequestException(
+ "allow_conflicts and on_behalf_of_uploader are mutually exclusive");
+ }
+
+ CurrentUser caller = rsrc.getUser();
+ Account.Id uploaderId = rsrc.getPatchSet().uploader();
+ IdentifiedUser uploader = userFactory.runAs(/*remotePeer= */ null, uploaderId, caller);
+ logger.atFine().log(
+ "%s is rebasing patch set %s of project %s on behalf of uploader %s",
+ caller.getLoggableName(),
+ rsrc.getPatchSet().id(),
+ rsrc.getProject(),
+ uploader.getLoggableName());
+
+ checkPermissionForUploader(
+ uploader,
+ rsrc.getNotes(),
+ ChangePermission.READ,
+ String.format("uploader %s cannot read change", uploader.getLoggableName()));
+ checkPermissionForUploader(
+ uploader,
+ rsrc.getNotes(),
+ ChangePermission.ADD_PATCH_SET,
+ String.format("uploader %s cannot add patch set", uploader.getLoggableName()));
+
+ try (Repository repo = repoManager.openRepository(rsrc.getProject())) {
+ RevCommit commit = repo.parseCommit(rsrc.getPatchSet().commitId());
+
+ if (!uploader.hasEmailAddress(commit.getAuthorIdent().getEmailAddress())) {
+ checkPermissionForUploader(
+ uploader,
+ rsrc.getNotes(),
+ RefPermission.FORGE_AUTHOR,
+ String.format(
+ "author of patch set %d is forged and the uploader %s cannot forge author",
+ rsrc.getPatchSet().id().get(), uploader.getLoggableName()));
+
+ if (serverIdent.get().getEmailAddress().equals(commit.getAuthorIdent().getEmailAddress())) {
+ checkPermissionForUploader(
+ uploader,
+ rsrc.getNotes(),
+ RefPermission.FORGE_SERVER,
+ String.format(
+ "author of patch set %d is the server identity and the uploader %s cannot forge"
+ + " the server identity",
+ rsrc.getPatchSet().id().get(), uploader.getLoggableName()));
+ }
+ }
+ }
+
+ return new RevisionResource(
+ changeResourceFactory.create(rsrc.getNotes(), uploader), rsrc.getPatchSet());
+ }
+
+ private void checkPermissionForUploader(
+ IdentifiedUser uploader,
+ ChangeNotes changeNotes,
+ ChangePermission changePermission,
+ String errorMessage)
+ throws PermissionBackendException, ResourceConflictException {
+ try {
+ permissionBackend.user(uploader).change(changeNotes).check(changePermission);
+ } catch (AuthException e) {
+ throw new ResourceConflictException(errorMessage, e);
+ }
+ }
+
+ private void checkPermissionForUploader(
+ IdentifiedUser uploader,
+ ChangeNotes changeNotes,
+ RefPermission refPermission,
+ String errorMessage)
+ throws PermissionBackendException, ResourceConflictException {
+ try {
+ permissionBackend.user(uploader).ref(changeNotes.getChange().getDest()).check(refPermission);
+ } catch (AuthException e) {
+ throw new ResourceConflictException(errorMessage, e);
+ }
+ }
+
+ /**
* Checks whether the given change fulfills all preconditions to be rebased.
*
* <p>This method does not check whether the calling user is allowed to rebase the change.
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index f6fc8db..56a0bbd 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -24,6 +24,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Enums;
import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
@@ -230,6 +231,7 @@
public static final Account.Id OWNER_ACCOUNT_ID = Account.id(0);
public static final Account.Id NON_UPLOADER_ACCOUNT_ID = Account.id(-1);
public static final Account.Id NON_CONTRIBUTOR_ACCOUNT_ID = Account.id(-2);
+ public static final Account.Id NON_EXISTING_ACCOUNT_ID = Account.id(-1000);
public static final String OPERATOR_MERGED_BEFORE = "mergedbefore";
public static final String OPERATOR_MERGED_AFTER = "mergedafter";
@@ -1043,7 +1045,7 @@
} else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
} else {
- accounts = parseAccount(value);
+ accounts = parseAccountIgnoreVisibility(value);
}
} else if (key.equalsIgnoreCase(ARG_ID_GROUP)) {
group = parseGroup(value).getUUID();
@@ -1071,17 +1073,17 @@
if (accounts != null || group != null) {
throw new QueryParseException("more than one user/group specified (" + value + ")");
}
- try {
- if (value.equals(ARG_ID_OWNER)) {
- accounts = Collections.singleton(OWNER_ACCOUNT_ID);
- } else if (value.equals(ARG_ID_NON_UPLOADER)) {
- accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
- } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
- accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
- } else {
- accounts = parseAccount(value);
- }
- } catch (QueryParseException qpex) {
+ if (value.equals(ARG_ID_OWNER)) {
+ accounts = Collections.singleton(OWNER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_UPLOADER)) {
+ accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
+ accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
+ } else {
+ accounts = parseAccountIgnoreVisibility(value);
+ }
+
+ if (accounts.contains(NON_EXISTING_ACCOUNT_ID)) {
// If it doesn't match an account, see if it matches a group
// (accounts get precedence)
try {
@@ -1227,7 +1229,7 @@
@Operator
public Predicate<ChangeData> owner(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return owner(parseAccount(who, (AccountState s) -> true));
+ return owner(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> owner(Set<Account.Id> who) {
@@ -1240,7 +1242,7 @@
private Predicate<ChangeData> ownerDefaultField(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- Set<Account.Id> accounts = parseAccount(who);
+ Set<Account.Id> accounts = parseAccountIgnoreVisibility(who);
if (accounts.size() > MAX_ACCOUNTS_PER_DEFAULT_FIELD) {
return Predicate.any();
}
@@ -1251,7 +1253,7 @@
public Predicate<ChangeData> uploader(String who)
throws QueryParseException, IOException, ConfigInvalidException {
checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploader");
- return uploader(parseAccount(who, (AccountState s) -> true));
+ return uploader(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> uploader(Set<Account.Id> who) {
@@ -1266,7 +1268,7 @@
public Predicate<ChangeData> attention(String who)
throws QueryParseException, IOException, ConfigInvalidException {
checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
- return attention(parseAccount(who, (AccountState s) -> true));
+ return attention(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> attention(Set<Account.Id> who) {
@@ -1403,7 +1405,7 @@
@Operator
public Predicate<ChangeData> commentby(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return commentby(parseAccount(who));
+ return commentby(parseAccountIgnoreVisibility(who));
}
private Predicate<ChangeData> commentby(Set<Account.Id> who) {
@@ -1417,7 +1419,7 @@
@Operator
public Predicate<ChangeData> from(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- Set<Account.Id> ownerIds = parseAccount(who);
+ Set<Account.Id> ownerIds = parseAccountIgnoreVisibility(who);
return Predicate.or(owner(ownerIds), commentby(ownerIds));
}
@@ -1469,7 +1471,7 @@
@Operator
public Predicate<ChangeData> reviewedby(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return ChangePredicates.reviewedBy(parseAccount(who));
+ return ChangePredicates.reviewedBy(parseAccountIgnoreVisibility(who));
}
@Operator
@@ -1722,18 +1724,42 @@
}
}
- private Set<Account.Id> parseAccount(
- String who, java.util.function.Predicate<AccountState> activityFilter)
- throws QueryParseException, IOException, ConfigInvalidException {
+ private Set<Account.Id> parseAccountIgnoreVisibility(String who)
+ throws QueryRequiresAuthException, IOException, ConfigInvalidException {
try {
return args.accountResolver
- .resolveAsUser(args.getUser(), who, activityFilter)
+ .resolveAsUserIgnoreVisibility(args.getUser(), who)
.asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
}
- throw new QueryParseException(e.getMessage(), e);
+ return ImmutableSet.of(NON_EXISTING_ACCOUNT_ID);
+ }
+ }
+
+ private Set<Account.Id> parseAccountIgnoreVisibility(
+ String who, java.util.function.Predicate<AccountState> activityFilter)
+ throws QueryRequiresAuthException, IOException, ConfigInvalidException {
+ try {
+ return args.accountResolver
+ .resolveAsUserIgnoreVisibility(args.getUser(), who, activityFilter)
+ .asNonEmptyIdSet();
+ } catch (UnresolvableAccountException e) {
+ // Thrown if no account was found.
+
+ // Users can always see their own account. This means if self was being resolved and there was
+ // no match the user wasn't logged it and the request was done anonymously.
+ if (e.isSelf()) {
+ throw new QueryRequiresAuthException(e.getMessage(), e);
+ }
+
+ // If no account is found, we don't want to fail with an error as this would allow users to
+ // probe the existence of accounts (error -> account doesn't exist, empty result -> account
+ // exists but didn't take part in any visible changes). Hence, we return a special account ID
+ // (NON_EXISTING_ACCOUNT_ID) that doesn't match any account so the query can be normally
+ // executed
+ return ImmutableSet.of(NON_EXISTING_ACCOUNT_ID);
}
}
@@ -1790,7 +1816,7 @@
Predicate<ChangeData> reviewerPredicate = null;
try {
- Set<Account.Id> accounts = parseAccount(who);
+ Set<Account.Id> accounts = parseAccountIgnoreVisibility(who);
if (!forDefaultField || accounts.size() <= MAX_ACCOUNTS_PER_DEFAULT_FIELD) {
reviewerPredicate =
Predicate.or(
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
index 0e1a218..3ac4d22 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
@@ -153,10 +153,12 @@
}
// Set the approval to 0 if vote is being removed.
newApprovals.put(a.label(), (short) 0);
- found = true;
-
- // Set old value, as required by VoteDeleted.
- oldApprovals.put(a.label(), a.value());
+ // If the value is 0, we treat it as already deleted, so no additional actions is required
+ if (a.value() != 0) {
+ found = true;
+ // Set old value, as required by VoteDeleted.
+ oldApprovals.put(a.label(), a.value());
+ }
break;
}
if (!found) {
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 5368c75..3cb1870 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -19,23 +19,16 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiAction;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.GerritPersonIdent;
-import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeResource;
@@ -45,36 +38,28 @@
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
-import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
public class Rebase
implements RestModifyView<RevisionResource, RebaseInput>, UiAction<RevisionResource> {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private static final ImmutableSet<ListChangesOption> OPTIONS =
Sets.immutableEnumSet(ListChangesOption.CURRENT_REVISION, ListChangesOption.CURRENT_COMMIT);
- private final Provider<PersonIdent> serverIdent;
private final BatchUpdate.Factory updateFactory;
private final GitRepositoryManager repoManager;
private final RebaseUtil rebaseUtil;
@@ -82,13 +67,10 @@
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
private final PatchSetUtil patchSetUtil;
- private final IdentifiedUser.GenericFactory userFactory;
- private final ChangeResource.Factory changeResourceFactory;
private final RebaseMetrics rebaseMetrics;
@Inject
public Rebase(
- @GerritPersonIdent Provider<PersonIdent> serverIdent,
BatchUpdate.Factory updateFactory,
GitRepositoryManager repoManager,
RebaseUtil rebaseUtil,
@@ -96,10 +78,7 @@
PermissionBackend permissionBackend,
ProjectCache projectCache,
PatchSetUtil patchSetUtil,
- IdentifiedUser.GenericFactory userFactory,
- ChangeResource.Factory changeResourceFactory,
RebaseMetrics rebaseMetrics) {
- this.serverIdent = serverIdent;
this.updateFactory = updateFactory;
this.repoManager = repoManager;
this.rebaseUtil = rebaseUtil;
@@ -107,8 +86,6 @@
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.patchSetUtil = patchSetUtil;
- this.userFactory = userFactory;
- this.changeResourceFactory = changeResourceFactory;
this.rebaseMetrics = rebaseMetrics;
}
@@ -118,7 +95,7 @@
if (input.onBehalfOfUploader && !rsrc.getPatchSet().uploader().equals(rsrc.getAccountId())) {
rsrc.permissions().check(ChangePermission.REBASE_ON_BEHALF_OF_UPLOADER);
- rsrc = onBehalfOf(rsrc, input);
+ rsrc = rebaseUtil.onBehalfOf(rsrc, input);
} else {
rsrc.permissions().check(ChangePermission.REBASE);
}
@@ -160,143 +137,6 @@
}
}
- /**
- * Checks that the uploader has permissions to create a new patch set and creates a new {@link
- * RevisionResource} that contains the uploader (aka the impersonated user) as the current user
- * which can be used for {@link BatchUpdate} to do the rebase on behalf of the uploader.
- *
- * <p>The following permissions are required for the uploader:
- *
- * <ul>
- * <li>The {@code Read} permission that allows to see the change.
- * <li>The {@code Push} permission that allows upload.
- * <li>The {@code Add Patch Set} permission, required if the change is owned by another user
- * (change owners implicitly have this permission).
- * <li>The {@code Forge Author} permission if the patch set that is rebased has a forged author
- * (author != uploader).
- * <li>The {@code Forge Server} permission if the patch set that is rebased has the server
- * identity as the author.
- * </ul>
- *
- * <p>Usually the uploader should have all these permission since they were already required for
- * the original upload, but there is the edge case that the uploader had the permission when doing
- * the original upload and then the permission was revoked.
- *
- * <p>Note that patch sets with a forged committer (committer != uploader) can be rebased on
- * behalf of the uploader, even if the uploader doesn't have the {@code Forge Committer}
- * permission. This is because on rebase on behalf of the uploader the uploader will become the
- * committer of the new rebased patch set, hence for the rebased patch set the committer is no
- * longer forged (committer == uploader) and hence the {@code Forge Committer} permission is not
- * required.
- *
- * <p>Note that the {@code Rebase} permission is not required for the uploader since the {@code
- * Rebase} permission is specifically about allowing a user to do a rebase via the web UI by
- * clicking on the {@code REBASE} button and the uploader is not clicking on this button.
- *
- * <p>The permissions of the uploader are checked explicitly here so that we can return a {@code
- * 409 Conflict} response with a proper error message if they are missing (the error message says
- * that the permission is missing for the uploader). The normal code path also checks these
- * permission but the exception thrown there would result in a {@code 403 Forbidden} response and
- * the error message would wrongly look like the caller (i.e. the rebaser) is missing the
- * permission.
- *
- * <p>Note that this method doesn't check permissions for the rebaser (aka the impersonating user
- * aka the calling user). Callers should check the permissions for the rebaser before calling this
- * method.
- *
- * @param rsrc the revision resource that should be rebased
- * @param rebaseInput the request input containing options for the rebase
- * @return revision resource that contains the uploader (aka the impersonated user) as the current
- * user which can be used for {@link BatchUpdate} to do the rebase on behalf of the uploader
- */
- private RevisionResource onBehalfOf(RevisionResource rsrc, RebaseInput rebaseInput)
- throws IOException, PermissionBackendException, BadRequestException,
- ResourceConflictException {
- if (rsrc.getPatchSet().id().get() != rsrc.getChange().currentPatchSetId().get()) {
- throw new BadRequestException(
- "non-current patch set cannot be rebased on behalf of the uploader");
- }
- if (rebaseInput.allowConflicts) {
- throw new BadRequestException(
- "allow_conflicts and on_behalf_of_uploader are mutually exclusive");
- }
-
- CurrentUser caller = rsrc.getUser();
- Account.Id uploaderId = rsrc.getPatchSet().uploader();
- IdentifiedUser uploader = userFactory.runAs(/*remotePeer= */ null, uploaderId, caller);
- logger.atFine().log(
- "%s is rebasing patch set %s of project %s on behalf of uploader %s",
- caller.getLoggableName(),
- rsrc.getPatchSet().id(),
- rsrc.getProject(),
- uploader.getLoggableName());
-
- checkPermissionForUploader(
- uploader,
- rsrc.getNotes(),
- ChangePermission.READ,
- String.format("uploader %s cannot read change", uploader.getLoggableName()));
- checkPermissionForUploader(
- uploader,
- rsrc.getNotes(),
- ChangePermission.ADD_PATCH_SET,
- String.format("uploader %s cannot add patch set", uploader.getLoggableName()));
-
- try (Repository repo = repoManager.openRepository(rsrc.getProject())) {
- RevCommit commit = repo.parseCommit(rsrc.getPatchSet().commitId());
-
- if (!uploader.hasEmailAddress(commit.getAuthorIdent().getEmailAddress())) {
- checkPermissionForUploader(
- uploader,
- rsrc.getNotes(),
- RefPermission.FORGE_AUTHOR,
- String.format(
- "author of patch set %d is forged and the uploader %s cannot forge author",
- rsrc.getPatchSet().id().get(), uploader.getLoggableName()));
-
- if (serverIdent.get().getEmailAddress().equals(commit.getAuthorIdent().getEmailAddress())) {
- checkPermissionForUploader(
- uploader,
- rsrc.getNotes(),
- RefPermission.FORGE_SERVER,
- String.format(
- "author of patch set %d is the server identity and the uploader %s cannot forge"
- + " the server identity",
- rsrc.getPatchSet().id().get(), uploader.getLoggableName()));
- }
- }
- }
-
- return new RevisionResource(
- changeResourceFactory.create(rsrc.getNotes(), uploader), rsrc.getPatchSet());
- }
-
- private void checkPermissionForUploader(
- IdentifiedUser uploader,
- ChangeNotes changeNotes,
- ChangePermission changePermission,
- String errorMessage)
- throws PermissionBackendException, ResourceConflictException {
- try {
- permissionBackend.user(uploader).change(changeNotes).check(changePermission);
- } catch (AuthException e) {
- throw new ResourceConflictException(errorMessage, e);
- }
- }
-
- private void checkPermissionForUploader(
- IdentifiedUser uploader,
- ChangeNotes changeNotes,
- RefPermission refPermission,
- String errorMessage)
- throws PermissionBackendException, ResourceConflictException {
- try {
- permissionBackend.user(uploader).ref(changeNotes.getChange().getDest()).check(refPermission);
- } catch (AuthException e) {
- throw new ResourceConflictException(errorMessage, e);
- }
- }
-
@Override
public UiAction.Description getDescription(RevisionResource rsrc) throws IOException {
UiAction.Description description =
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 412a8ee..745c918 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -23,6 +23,7 @@
import static java.util.stream.Collectors.toMap;
import static java.util.stream.Collectors.toSet;
+import com.google.auto.value.AutoValue;
import com.google.common.base.Throwables;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
@@ -50,6 +51,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.extensions.events.AttentionSetObserver;
@@ -243,6 +245,12 @@
}
class ContextImpl implements Context {
+ private final CurrentUser contextUser;
+
+ ContextImpl(@Nullable CurrentUser contextUser) {
+ this.contextUser = contextUser != null ? contextUser : user;
+ }
+
@Override
public RepoView getRepoView() throws IOException {
return BatchUpdate.this.getRepoView();
@@ -270,7 +278,7 @@
@Override
public CurrentUser getUser() {
- return user;
+ return contextUser;
}
@Override
@@ -281,6 +289,10 @@
}
private class RepoContextImpl extends ContextImpl implements RepoContext {
+ RepoContextImpl(@Nullable CurrentUser contextUser) {
+ super(contextUser);
+ }
+
@Override
public ObjectInserter getInserter() throws IOException {
return getRepoView().getInserterWrapper();
@@ -296,21 +308,22 @@
private final ChangeNotes notes;
/**
- * Updates where the caller instructed us to create one NoteDb commit per update. Keyed by
- * PatchSet.Id only for convenience.
+ * Updates where the caller allowed us to combine potentially multiple adjustments into a single
+ * commit in NoteDb by re-using the same ChangeUpdate instance. Will still be one commit per
+ * patch set.
*/
private final Map<PatchSet.Id, ChangeUpdate> defaultUpdates;
/**
- * Updates where the caller allowed us to combine potentially multiple adjustments into a single
- * commit in NoteDb by re-using the same ChangeUpdate instance. Will still be one commit per
- * patch set.
+ * Updates where the caller instructed us to create one NoteDb commit per update. Keyed by
+ * PatchSet.Id only for convenience.
*/
private final ListMultimap<PatchSet.Id, ChangeUpdate> distinctUpdates;
private boolean deleted;
- ChangeContextImpl(ChangeNotes notes) {
+ ChangeContextImpl(@Nullable CurrentUser contextUser, ChangeNotes notes) {
+ super(contextUser);
this.notes = requireNonNull(notes);
defaultUpdates = new TreeMap<>(comparing(PatchSet.Id::get));
distinctUpdates = ArrayListMultimap.create();
@@ -334,7 +347,7 @@
}
private ChangeUpdate getNewChangeUpdate(PatchSet.Id psId) {
- ChangeUpdate u = changeUpdateFactory.create(notes, user, when);
+ ChangeUpdate u = changeUpdateFactory.create(notes, getUser(), getWhen());
if (newChanges.containsKey(notes.getChangeId())) {
u.setAllowWriteToNewRef(true);
}
@@ -356,7 +369,9 @@
private class PostUpdateContextImpl extends ContextImpl implements PostUpdateContext {
private final Map<Change.Id, ChangeData> changeDatas;
- PostUpdateContextImpl(Map<Change.Id, ChangeData> changeDatas) {
+ PostUpdateContextImpl(
+ @Nullable CurrentUser contextUser, Map<Change.Id, ChangeData> changeDatas) {
+ super(contextUser);
this.changeDatas = changeDatas;
}
@@ -385,6 +400,7 @@
}
private final GitRepositoryManager repoManager;
+ private final AccountCache accountCache;
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory changeNotesFactory;
private final ChangeUpdate.Factory changeUpdateFactory;
@@ -397,10 +413,10 @@
private final Instant when;
private final ZoneId zoneId;
- private final ListMultimap<Change.Id, BatchUpdateOp> ops =
+ private final ListMultimap<Change.Id, OpData<BatchUpdateOp>> ops =
MultimapBuilder.linkedHashKeys().arrayListValues().build();
private final Map<Change.Id, Change> newChanges = new HashMap<>();
- private final List<RepoOnlyOp> repoOnlyOps = new ArrayList<>();
+ private final List<OpData<RepoOnlyOp>> repoOnlyOps = new ArrayList<>();
private final Map<Change.Id, NotifyHandling> perChangeNotifyHandling = new HashMap<>();
private RepoView repoView;
@@ -419,6 +435,7 @@
BatchUpdate(
GitRepositoryManager repoManager,
@GerritPersonIdent PersonIdent serverIdent,
+ AccountCache accountCache,
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory changeNotesFactory,
ChangeUpdate.Factory changeUpdateFactory,
@@ -430,6 +447,7 @@
@Assisted CurrentUser user,
@Assisted Instant when) {
this.repoManager = repoManager;
+ this.accountCache = accountCache;
this.changeDataFactory = changeDataFactory;
this.changeNotesFactory = changeNotesFactory;
this.changeUpdateFactory = changeUpdateFactory;
@@ -549,48 +567,72 @@
toMap(entry -> BranchNameKey.create(project, entry.getKey()), Map.Entry::getValue));
}
+ /**
+ * Adds a {@link BatchUpdate} for a change.
+ *
+ * <p>The op is executed by the user for which the {@link BatchUpdate} has been created.
+ */
public BatchUpdate addOp(Change.Id id, BatchUpdateOp op) {
checkArgument(!(op instanceof InsertChangeOp), "use insertChange");
requireNonNull(op);
- ops.put(id, op);
+ ops.put(id, OpData.create(op, user));
return this;
}
+ /** Adds a {@link BatchUpdate} for a change that should be executed by the given context user. */
+ public BatchUpdate addOp(Change.Id id, CurrentUser contextUser, BatchUpdateOp op) {
+ checkArgument(!(op instanceof InsertChangeOp), "use insertChange");
+ requireNonNull(op);
+ ops.put(id, OpData.create(op, contextUser));
+ return this;
+ }
+
+ /**
+ * Adds a {@link RepoOnlyOp}.
+ *
+ * <p>The op is executed by the user for which the {@link BatchUpdate} has been created.
+ */
public BatchUpdate addRepoOnlyOp(RepoOnlyOp op) {
checkArgument(!(op instanceof BatchUpdateOp), "use addOp()");
- repoOnlyOps.add(op);
+ repoOnlyOps.add(OpData.create(op, user));
+ return this;
+ }
+
+ /** Adds a {@link RepoOnlyOp} that should be executed by the given context user. */
+ public BatchUpdate addRepoOnlyOp(CurrentUser contextUser, RepoOnlyOp op) {
+ checkArgument(!(op instanceof BatchUpdateOp), "use addOp()");
+ repoOnlyOps.add(OpData.create(op, contextUser));
return this;
}
public BatchUpdate insertChange(InsertChangeOp op) throws IOException {
- Context ctx = new ContextImpl();
+ Context ctx = new ContextImpl(user);
Change c = op.createChange(ctx);
checkArgument(
!newChanges.containsKey(c.getId()), "only one op allowed to create change %s", c.getId());
newChanges.put(c.getId(), c);
- ops.get(c.getId()).add(0, op);
+ ops.get(c.getId()).add(0, OpData.create(op, user));
return this;
}
private void executeUpdateRepo() throws UpdateException, RestApiException {
try {
logDebug("Executing updateRepo on %d ops", ops.size());
- RepoContextImpl ctx = new RepoContextImpl();
- for (Map.Entry<Change.Id, BatchUpdateOp> op : ops.entries()) {
+ for (Map.Entry<Change.Id, OpData<BatchUpdateOp>> e : ops.entries()) {
+ BatchUpdateOp op = e.getValue().op();
+ RepoContextImpl ctx = new RepoContextImpl(e.getValue().user());
try (TraceContext.TraceTimer ignored =
TraceContext.newTimer(
op.getClass().getSimpleName() + "#updateRepo",
- Metadata.builder()
- .projectName(project.get())
- .changeId(op.getKey().get())
- .build())) {
- op.getValue().updateRepo(ctx);
+ Metadata.builder().projectName(project.get()).changeId(e.getKey().get()).build())) {
+ op.updateRepo(ctx);
}
}
logDebug("Executing updateRepo on %d RepoOnlyOps", repoOnlyOps.size());
- for (RepoOnlyOp op : repoOnlyOps) {
- op.updateRepo(ctx);
+ for (OpData<RepoOnlyOp> opData : repoOnlyOps) {
+ RepoContextImpl ctx = new RepoContextImpl(opData.user());
+ opData.op().updateRepo(ctx);
}
if (onSubmitValidators != null && !getRefUpdates().isEmpty()) {
@@ -599,7 +641,7 @@
// first update's executeRefUpdates has finished, hence after first repo's refs have been
// updated, which is too late.
onSubmitValidators.validate(
- project, ctx.getRevWalk().getObjectReader(), repoView.getCommands());
+ project, getRepoView().getRevWalk().getObjectReader(), repoView.getCommands());
}
} catch (Exception e) {
Throwables.throwIfInstanceOf(e, RestApiException.class);
@@ -613,12 +655,14 @@
}
}
- private void fireAttentionSetUpdateEvents(PostUpdateContext ctx) {
+ private void fireAttentionSetUpdateEvents(Map<Change.Id, ChangeData> changeDatas) {
for (ProjectChangeKey key : attentionSetUpdates.keySet()) {
- ChangeData change = ctx.getChangeData(key.projectName(), key.changeId());
- AccountState account = ctx.getAccount();
+ ChangeData change =
+ changeDatas.computeIfAbsent(
+ key.changeId(), id -> changeDataFactory.create(key.projectName(), key.changeId()));
for (AttentionSetUpdate update : attentionSetUpdates.get(key)) {
- attentionSetObserver.fire(change, account, update, ctx.getWhen());
+ attentionSetObserver.fire(
+ change, accountCache.getEvenIfMissing(update.account()), update, when);
}
}
}
@@ -709,31 +753,45 @@
}
handle.manager.setRefLogMessage(refLogMessage);
handle.manager.setPushCertificate(pushCert);
- for (Map.Entry<Change.Id, Collection<BatchUpdateOp>> e : ops.asMap().entrySet()) {
+ for (Map.Entry<Change.Id, Collection<OpData<BatchUpdateOp>>> e : ops.asMap().entrySet()) {
Change.Id id = e.getKey();
- ChangeContextImpl ctx = newChangeContext(id);
boolean dirty = false;
+ boolean deleted = false;
+ List<ChangeUpdate> changeUpdates = new ArrayList<>();
+ ChangeContextImpl ctx = null;
logDebug(
"Applying %d ops for change %s: %s",
e.getValue().size(),
id,
lazy(() -> e.getValue().stream().map(op -> op.getClass().getName()).collect(toSet())));
- for (BatchUpdateOp op : e.getValue()) {
+ for (OpData<BatchUpdateOp> opData : e.getValue()) {
+ if (ctx == null) {
+ ctx = newChangeContext(opData.user(), id);
+ } else if (!ctx.getUser().equals(opData.user())) {
+ ctx.defaultUpdates.values().forEach(changeUpdates::add);
+ ctx.distinctUpdates.values().forEach(changeUpdates::add);
+ ctx = newChangeContext(opData.user(), id);
+ }
try (TraceContext.TraceTimer ignored =
TraceContext.newTimer(
- op.getClass().getSimpleName() + "#updateChange",
+ opData.getClass().getSimpleName() + "#updateChange",
Metadata.builder().projectName(project.get()).changeId(id.get()).build())) {
- dirty |= op.updateChange(ctx);
+ dirty |= opData.op().updateChange(ctx);
+ deleted |= ctx.deleted;
}
}
+ if (ctx != null) {
+ ctx.defaultUpdates.values().forEach(changeUpdates::add);
+ ctx.distinctUpdates.values().forEach(changeUpdates::add);
+ }
+
if (!dirty) {
logDebug("No ops reported dirty, short-circuiting");
handle.setResult(id, ChangeResult.SKIPPED);
continue;
}
- ctx.defaultUpdates.values().forEach(handle.manager::add);
- ctx.distinctUpdates.values().forEach(handle.manager::add);
- if (ctx.deleted) {
+ changeUpdates.forEach(handle.manager::add);
+ if (deleted) {
logDebug("Change %s was deleted", id);
handle.manager.deleteChange(id);
handle.setResult(id, ChangeResult.DELETED);
@@ -744,7 +802,7 @@
return handle;
}
- private ChangeContextImpl newChangeContext(Change.Id id) {
+ private ChangeContextImpl newChangeContext(@Nullable CurrentUser contextUser, Change.Id id) {
logDebug("Opening change %s for update", id);
Change c = newChanges.get(id);
boolean isNew = c != null;
@@ -757,27 +815,30 @@
logDebug("Change %s is new", id);
}
ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew);
- return new ChangeContextImpl(notes);
+ return new ChangeContextImpl(contextUser, notes);
}
private void executePostOps(Map<Change.Id, ChangeData> changeDatas) throws Exception {
- PostUpdateContextImpl ctx = new PostUpdateContextImpl(changeDatas);
- for (BatchUpdateOp op : ops.values()) {
+ for (OpData<BatchUpdateOp> opData : ops.values()) {
+ PostUpdateContextImpl ctx = new PostUpdateContextImpl(opData.user(), changeDatas);
try (TraceContext.TraceTimer ignored =
- TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
- op.postUpdate(ctx);
+ TraceContext.newTimer(
+ opData.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
+ opData.op().postUpdate(ctx);
}
}
- for (RepoOnlyOp op : repoOnlyOps) {
+ for (OpData<RepoOnlyOp> opData : repoOnlyOps) {
+ PostUpdateContextImpl ctx = new PostUpdateContextImpl(opData.user(), changeDatas);
try (TraceContext.TraceTimer ignored =
- TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
- op.postUpdate(ctx);
+ TraceContext.newTimer(
+ opData.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
+ opData.op().postUpdate(ctx);
}
}
try (TraceContext.TraceTimer ignored =
TraceContext.newTimer("fireAttentionSetUpdates#postUpdate", Metadata.empty())) {
- fireAttentionSetUpdateEvents(ctx);
+ fireAttentionSetUpdateEvents(changeDatas);
}
}
@@ -808,4 +869,18 @@
logger.atFine().log(msg, arg1, arg2, arg3);
}
}
+
+ /** Data needed to execute a {@link RepoOnlyOp} or a {@link BatchUpdateOp}. */
+ @AutoValue
+ abstract static class OpData<T extends RepoOnlyOp> {
+ /** Op that should be executed. */
+ abstract T op();
+
+ /** User that should be used to execute the {@link #op}. */
+ abstract CurrentUser user();
+
+ static <T extends RepoOnlyOp> OpData<T> create(T op, CurrentUser user) {
+ return new AutoValue_BatchUpdate_OpData<>(op, user);
+ }
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 2c1739d..a9a0b70 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -2899,8 +2899,12 @@
requestScopeOperations.setApiUser(user.id());
assertThrows(ResourceNotFoundException.class, () -> gApi.accounts().id("secondary"));
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id("secondary@example.com"));
requestScopeOperations.setApiUser(admin.id());
assertThat(gApi.accounts().id("secondary").get()._accountId).isEqualTo(foo.id().get());
+ assertThat(gApi.accounts().id("secondary@example.com").get()._accountId)
+ .isEqualTo(foo.id().get());
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
index 3bfb573..43c8684 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
@@ -23,16 +24,20 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
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.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.entities.AccessSection;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
@@ -40,9 +45,11 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.restapi.change.QueryChanges;
+import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.Arrays;
@@ -53,6 +60,7 @@
public class QueryChangesIT extends AbstractDaemonTest {
@Inject private AccountOperations accountOperations;
+ @Inject private ChangeOperations changeOperations;
@Inject private ProjectOperations projectOperations;
@Inject private Provider<QueryChanges> queryChangesProvider;
@Inject private RequestScopeOperations requestScopeOperations;
@@ -364,6 +372,282 @@
assertThat(result3).hasSize(1);
}
+ /**
+ * This test verifies that querying by a non-visible account doesn't fail.
+ *
+ * <p>Change queries only return changes that are visible to the calling user. If a non-visible
+ * account participated in such a change the existence of this account is known to everyone who
+ * can see the change. Hence it's OK to that the account visibility check is skipped when querying
+ * changes by non-visible accounts. If the account is visible through any visible change these
+ * changes are returned, otherwise the result is empty (see
+ * emptyResultWhenQueryingByNonVisibleAccountAndMatchingChangesAreNotVisible()), same as for
+ * non-existing accounts (see test emptyResultWhenQueryingByNonExistingAccount()).
+ */
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void changesCanBeQueriesByNonVisibleAccounts() throws Exception {
+ String ownerEmail = "owner@example.com";
+ Account.Id nonVisibleOwner = accountOperations.newAccount().preferredEmail(ownerEmail).create();
+
+ String reviewerEmail = "reviewer@example.com";
+ Account.Id nonVisibleReviewer =
+ accountOperations.newAccount().preferredEmail(reviewerEmail).create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(nonVisibleOwner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(nonVisibleReviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that user cannot see the other accounts.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleOwner.get()).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleReviewer.get()).get());
+
+ // Verify that the change is also found if user queries for changes owned/uploaded by
+ // nonVisibleOwner.
+ assertThat(gApi.changes().query("owner:" + ownerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + ownerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is also found if user queries for changes reviewed by
+ // nonVisibleReviewer.
+ assertThat(gApi.changes().query("reviewer:" + reviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + reviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
+ /**
+ * This test verifies that an empty result is returned for a query by a non-existing account.
+ *
+ * <p>Such queries must not return an error so that users cannot probe whether an account exists.
+ * Since we return an empty result for non-visible accounts if there are no matched changes or non
+ * of the matched changes is visible, users could conclude the existence of a account if we would
+ * return an error for non-existing accounts.
+ */
+ @Test
+ public void emptyResultWhenQueryingByNonExistingAccount() throws Exception {
+ assertThat(gApi.changes().query("owner:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("reviewer:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=non-existing@example.com").get())
+ .isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void emptyResultWhenQueryingByNonVisibleAccountAndMatchingChangesAreNotVisible()
+ throws Exception {
+ String ownerEmail = "owner@example.com";
+ Account.Id nonVisibleOwner = accountOperations.newAccount().preferredEmail(ownerEmail).create();
+
+ String reviewerEmail = "reviewer@example.com";
+ Account.Id nonVisibleReviewer =
+ accountOperations.newAccount().preferredEmail(reviewerEmail).create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(nonVisibleOwner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(nonVisibleReviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ // Block read permission so that the change is not visible.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user cannot see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get()).isEmpty();
+
+ // Verify that user cannot see the other accounts.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleOwner.get()).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleReviewer.get()).get());
+
+ // Verify that the change is also found if user queries for changes owned/uploaded by
+ // nonVisibleOwner.
+ assertThat(gApi.changes().query("owner:" + ownerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:" + ownerEmail).get()).isEmpty();
+
+ // Verify that the change is also found if user queries for changes reviewed by
+ // nonVisibleReviewer.
+ assertThat(gApi.changes().query("reviewer:" + reviewerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + reviewerEmail).get()).isEmpty();
+ }
+
+ @Test
+ public void emptyResultWhenQueryingByNonVisibleSecondaryEmail() throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that user cannot see the other accounts by their secondary email.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(secondaryOwnerEmail).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(secondaryReviewerEmail).get());
+
+ // Verify that the change is not found if user queries for changes owned/uploaded by the
+ // secondary email of the owner that is not visible to user.
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get()).isEmpty();
+
+ // Verify that the change is not found if user queries for changes reviewed by the secondary
+ // email of the reviewer that is not visible to user.
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .isEmpty();
+ }
+
+ @Test
+ public void changesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability()
+ throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability(GlobalCapability.MODIFY_ACCOUNT).group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the other accounts by their secondary email.
+ assertThat(gApi.accounts().id(secondaryOwnerEmail).get()._accountId).isEqualTo(owner.get());
+ assertThat(gApi.accounts().id(secondaryReviewerEmail).get()._accountId)
+ .isEqualTo(reviewer.get());
+
+ // Verify that the change is found if user queries for changes owned/uploaded by the secondary
+ // email.
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is found if user queries for changes reviewed by the secondary email.
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
+ @Test
+ public void changesFoundWhenQueryingByOwnSecondaryEmail() throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ // Verify that the change is found if owner queries for changes owned/uploaded by their
+ // secondary email.
+ requestScopeOperations.setApiUser(owner);
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is found if reviewer queries for changes reviewed by their secondary
+ // email.
+ requestScopeOperations.setApiUser(reviewer);
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
private static void assertNoChangeHasMoreChangesSet(List<ChangeInfo> results) {
for (ChangeInfo info : results) {
assertThat(info._moreChanges).isNull();
@@ -383,4 +667,9 @@
}
}
}
+
+ private static Correspondence<ChangeInfo, Change.Id> hasChangeId() {
+ return NullAwareCorrespondence.transforming(
+ changeInfo -> Change.id(changeInfo._number), "hasChangeId");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
index 3531234..b7b55ee 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
@@ -322,8 +322,8 @@
testRepo.reset("HEAD~1");
PushOneCommit.Result r2 = createChange();
- ChangeInfo ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- RevisionInfo ri2 = ci2.revisions.get(ci2.currentRevision);
+ RevisionInfo ri2 =
+ get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(branchTip);
Change.Id id1 = r1.getChange().getId();
@@ -332,8 +332,7 @@
rebaseCallWithInput.call(r2.getChangeId(), in);
Change.Id id2 = r2.getChange().getId();
- ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- ri2 = ci2.revisions.get(ci2.currentRevision);
+ ri2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
List<RelatedChangeAndCommitInfo> related =
@@ -352,8 +351,8 @@
testRepo.reset("HEAD~1");
PushOneCommit.Result r2 = createChange();
- ChangeInfo ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- RevisionInfo ri2 = ci2.revisions.get(ci2.currentRevision);
+ RevisionInfo ri2 =
+ get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(branchTip);
// Submit first change.
@@ -367,8 +366,7 @@
rebaseCallWithInput.call(r2.getChangeId(), in);
Change.Id id2 = r2.getChange().getId();
- ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- ri2 = ci2.revisions.get(ci2.currentRevision);
+ ri2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
assertThat(gApi.changes().id(id2.get()).revision(ri2._number).related().changes).isEmpty();
@@ -539,7 +537,7 @@
ChangeInfo info =
gApi.changes().id(changeId.get()).get(CURRENT_REVISION, CURRENT_COMMIT, DETAILED_LABELS);
- RevisionInfo r = info.revisions.get(info.currentRevision);
+ RevisionInfo r = info.getCurrentRevision();
assertThat(r._number).isEqualTo(expectedNumRevisions);
assertThat(r.realUploader).isNull();
@@ -550,10 +548,10 @@
.isEqualTo(baseCommit);
// ...and the committer and description should be correct
- GitPerson committer = info.revisions.get(info.currentRevision).commit.committer;
+ GitPerson committer = r.commit.committer;
assertThat(committer.name).isEqualTo(admin.fullName());
assertThat(committer.email).isEqualTo(admin.email());
- String description = info.revisions.get(info.currentRevision).description;
+ String description = r.description;
assertThat(description).isEqualTo("Rebase");
if (shouldHaveApproval) {
@@ -676,7 +674,7 @@
ChangeInfo changeInfo =
gApi.changes().id(changeId).get(ALL_REVISIONS, CURRENT_COMMIT, CURRENT_REVISION);
assertThat(changeInfo.revisions).hasSize(2);
- assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.parents.get(0).commit)
+ assertThat(changeInfo.getCurrentRevision().commit.parents.get(0).commit)
.isEqualTo(base.name());
// Verify that the file content in the created patch set is correct.
@@ -767,8 +765,8 @@
rebaseCallWithInput.call(r3.getChangeId(), in);
Change.Id id3 = r3.getChange().getId();
- ChangeInfo ci3 = get(r3.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- RevisionInfo ri3 = ci3.revisions.get(ci3.currentRevision);
+ RevisionInfo ri3 =
+ get(r3.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri3.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
assertThat(gApi.changes().id(id3.get()).revision(ri3._number).related().changes).isEmpty();
@@ -843,22 +841,20 @@
fileName2,
fileContent2Ps2)
.assertOkStatus();
- ChangeInfo changeInfo2 = gApi.changes().id(changeId2).get();
- assertThat(changeInfo2.revisions.get(changeInfo2.currentRevision)._number).isEqualTo(2);
+ assertThat(gApi.changes().id(changeId2).get().getCurrentRevision()._number).isEqualTo(2);
// Rebase the first patch set of the second change
gApi.changes().id(changeId2).revision(1).rebase();
// Second change should have 3 patch sets
- changeInfo2 = gApi.changes().id(changeId2).get();
- assertThat(changeInfo2.revisions.get(changeInfo2.currentRevision)._number).isEqualTo(3);
+ assertThat(gApi.changes().id(changeId2).get().getCurrentRevision()._number).isEqualTo(3);
// ... and the committer and description should be correct
ChangeInfo info = gApi.changes().id(changeId2).get(CURRENT_REVISION, CURRENT_COMMIT);
- GitPerson committer = info.revisions.get(info.currentRevision).commit.committer;
+ GitPerson committer = info.getCurrentRevision().commit.committer;
assertThat(committer.name).isEqualTo(admin.fullName());
assertThat(committer.email).isEqualTo(admin.email());
- String description = info.revisions.get(info.currentRevision).description;
+ String description = info.getCurrentRevision().description;
assertThat(description).isEqualTo("Rebase");
// ... and the file contents should match with patch set 1 based on change1
@@ -936,8 +932,13 @@
verifyChangeIsUpToDate(r4);
// r5 wasn't rebased.
- ChangeInfo r5info = gApi.changes().id(r5.getChangeId()).get(CURRENT_REVISION);
- assertThat(r5info.revisions.get(r5info.currentRevision)._number).isEqualTo(1);
+ assertThat(
+ gApi.changes()
+ .id(r5.getChangeId())
+ .get(CURRENT_REVISION)
+ .getCurrentRevision()
+ ._number)
+ .isEqualTo(1);
// Rebasing r5
verifyRebaseChainResponse(
@@ -1081,7 +1082,7 @@
.id(changeWithConflictId)
.get(ALL_REVISIONS, CURRENT_COMMIT, CURRENT_REVISION);
assertThat(changeInfo.revisions).hasSize(2);
- assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.parents.get(0).commit)
+ assertThat(changeInfo.getCurrentRevision().commit.parents.get(0).commit)
.isEqualTo(base.name());
// Verify that the file content in the created patch set is correct.
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
index 5a5402b..9219ede 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
@@ -215,7 +215,7 @@
}
ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo = changeInfo2.getCurrentRevision();
// The change had 2 patch sets before the rebase, now it should be 3
assertThat(currentRevisionInfo._number).isEqualTo(3);
assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
@@ -278,7 +278,7 @@
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo = changeInfo2.getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
@@ -297,7 +297,7 @@
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ currentRevisionInfo = changeInfo2.getCurrentRevision();
// The change had 2 patch sets before the rebase, now it should be 3
assertThat(currentRevisionInfo._number).isEqualTo(3);
assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
@@ -317,7 +317,7 @@
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ currentRevisionInfo = changeInfo2.getCurrentRevision();
// The change had 3 patch sets before the rebase, now it should be 4
assertThat(currentRevisionInfo._number).isEqualTo(4);
assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
@@ -432,8 +432,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
@@ -531,8 +531,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 2 patch set before the rebase, now it should be 3
assertThat(currentRevisionInfo._number).isEqualTo(3);
assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
@@ -632,8 +632,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.commit.author.email).isEqualTo(authorEmail);
@@ -720,8 +720,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
@@ -773,8 +773,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.commit.author.email)
@@ -871,8 +871,8 @@
gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
requestScopeOperations.setApiUser(rebaser);
- ChangeInfo changeInfo = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo revisionInfo = changeInfo.revisions.get(changeInfo.currentRevision);
+ RevisionInfo revisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
assertThat(revisionInfo.actions).containsKey("rebase");
ActionInfo rebaseActionInfo = revisionInfo.actions.get("rebase");
assertThat(rebaseActionInfo.enabled).isTrue();
@@ -900,8 +900,8 @@
gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
requestScopeOperations.setApiUser(changeOwner);
- ChangeInfo changeInfo = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo revisionInfo = changeInfo.revisions.get(changeInfo.currentRevision);
+ RevisionInfo revisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
assertThat(revisionInfo.actions).containsKey("rebase");
ActionInfo rebaseActionInfo = revisionInfo.actions.get("rebase");
assertThat(rebaseActionInfo.enabled).isTrue();
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java b/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
index c89e11a..b21f97d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
@@ -91,16 +91,26 @@
Account.Id id =
accountOperations
.newAccount()
- .preferredEmail("preferred@email")
- .addSecondaryEmail("secondary@email")
+ .preferredEmail("preferred@eexample.com")
+ .addSecondaryEmail("secondary@example.com")
.create();
+
RestResponse r = userRestSession.get("/accounts/secondary/detail/");
r.assertStatus(404);
+
+ r = userRestSession.get("/accounts/secondary@example.com/detail/");
+ r.assertStatus(404);
+
// The admin has MODIFY_ACCOUNT permission and can see the user.
r = adminRestSession.get("/accounts/secondary/detail/");
r.assertStatus(200);
AccountDetailInfo info = newGson().fromJson(r.getReader(), AccountDetailInfo.class);
assertThat(info._accountId).isEqualTo(id.get());
+
+ r = adminRestSession.get("/accounts/secondary@example.com/detail/");
+ r.assertStatus(200);
+ info = newGson().fromJson(r.getReader(), AccountDetailInfo.class);
+ assertThat(info._accountId).isEqualTo(id.get());
}
private static class CustomAccountTagProvider implements AccountTagProvider {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
index 016b1e6..6491202 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
@@ -140,6 +140,33 @@
verifyCannotDeleteVote(true);
}
+ @Test
+ public void deleteAlreadyDeletedVote_returnsNotFoundAndWithoutEmails() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+ String deleteAdminVoteEndPoint =
+ "/changes/"
+ + r.getChangeId()
+ + "/reviewers/"
+ + admin.id().toString()
+ + "/votes/Code-Review";
+
+ sender.clear();
+ RestResponse response = userRestSession.delete(deleteAdminVoteEndPoint);
+ response.assertNoContent();
+ assertThat(sender.getMessages()).hasSize(1);
+
+ sender.clear();
+ response = userRestSession.delete(deleteAdminVoteEndPoint);
+ response.assertNotFound();
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
private void verifyDeleteVote(boolean onRevisionLevel) throws Exception {
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
diff --git a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
index cf1eee0..432a6c6 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
@@ -450,8 +450,18 @@
.to("refs/for/master");
r.assertOkStatus();
- // assert no email notifications for user
- assertThat(sender.getMessages()).isEmpty();
+ // Assert email notification for user.
+ // The non-visible account participated in a change that is visible to user, hence through this
+ // change user can see the non-visible account.
+ // Even if watching by the non-visible account was not possible, user could just watch all
+ // changes that are visible to them and then filter them by the non-visible account locally.
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ Message m = messages.get(0);
+ assertThat(m.rcpt()).containsExactly(user.getNameEmail());
+ assertThat(m.body()).contains("Change subject: subject\n");
+ assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+ sender.clear();
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java
index 3b97372..5bdf91f 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java
@@ -24,6 +24,7 @@
import static com.google.gerrit.truth.MapSubject.assertThatMap;
import static com.google.gerrit.truth.OptionalSubject.assertThat;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.truth.Correspondence;
@@ -37,6 +38,7 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeType;
@@ -147,6 +149,124 @@
}
@Test
+ public void createdChangeHasDefaultGroupsByDefault() throws Exception {
+ Project.NameKey project = projectOperations.newProject().branches("test-branch").create();
+ Change.Id changeId =
+ changeOperations.newChange().project(project).branch("test-branch").create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
+ public void createdChangeHasDefaultGroupsIfBranchTipIsSpecifiedAsParent() throws Exception {
+ Project.NameKey project = projectOperations.newProject().branches("test-branch").create();
+
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .childOf()
+ .tipOfBranch("refs/heads/test-branch")
+ .create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
+ public void createdChangeHasSameGroupsAsOpenParentChange() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ Change.Id parentChangeId = changeOperations.newChange().project(project).create();
+
+ ChangeInfo parentChange = getChangeFromServer(parentChangeId);
+ ImmutableList<String> parentGroups = getGroups(project, parentChangeId);
+ assertThat(parentGroups).containsExactly(parentChange.currentRevision);
+
+ Change.Id changeId =
+ changeOperations.newChange().project(project).childOf().change(parentChangeId).create();
+
+ assertThat(getGroups(project, changeId)).isEqualTo(parentGroups);
+ }
+
+ @Test
+ public void createdChangeHasDefaultGroupsIfClosedChangeIsSpecifiedAsParent() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ Change.Id parentChangeId = changeOperations.newChange().project(project).create();
+ gApi.changes().id(parentChangeId.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(parentChangeId.get()).current().submit();
+
+ Change.Id changeId =
+ changeOperations.newChange().project(project).childOf().change(parentChangeId).create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
+ public void createdChangeHasSameGroupsAsPatchSetOfOpenParentChange() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ Change.Id parentChangeId = changeOperations.newChange().project(project).create();
+ TestPatchset parentPatchset = changeOperations.change(parentChangeId).currentPatchset().get();
+ changeOperations.change(parentChangeId).newPatchset().create();
+
+ ImmutableList<String> parentGroups = getGroups(project, parentPatchset.patchsetId());
+ assertThat(parentGroups).containsExactly(parentPatchset.commitId().name());
+
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .childOf()
+ .patchset(parentPatchset.patchsetId())
+ .create();
+
+ assertThat(getGroups(project, changeId)).isEqualTo(parentGroups);
+ }
+
+ @Test
+ public void createdChangeHasDefaultGroupsIfPatchSetOfClosedChangeIsSpecifiedAsParent()
+ throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ Change.Id parentChangeId = changeOperations.newChange().project(project).create();
+ TestPatchset parentPatchset = changeOperations.change(parentChangeId).currentPatchset().get();
+ changeOperations.change(parentChangeId).newPatchset().create();
+ gApi.changes().id(parentChangeId.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(parentChangeId.get()).current().submit();
+
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .childOf()
+ .patchset(parentPatchset.patchsetId())
+ .create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
+ public void createdChangeHasDefaultGroupsIfCommitIsSpecifiedAsParent() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ // Currently, the easiest way to create a commit is by creating another change.
+ Change.Id anotherChangeId = changeOperations.newChange().project(project).create();
+ ObjectId parentCommitId =
+ changeOperations.change(anotherChangeId).currentPatchset().get().commitId();
+
+ Change.Id changeId =
+ changeOperations.newChange().project(project).childOf().commit(parentCommitId).create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
public void createdChangeUsesTipOfTargetBranchAsParentByDefault() throws Exception {
Project.NameKey project = projectOperations.newProject().branches("test-branch").create();
ObjectId parentCommitId = projectOperations.project(project).getHead("test-branch").getId();
@@ -1634,6 +1754,17 @@
return gApi.changes().id(changeId.get()).revision(patchsetId.get()).file(filePath).content();
}
+ private ImmutableList<String> getGroups(Project.NameKey projectName, Change.Id changeId) {
+ return changeDataFactory.create(projectName, changeId).currentPatchSet().groups();
+ }
+
+ private ImmutableList<String> getGroups(Project.NameKey projectName, PatchSet.Id patchSetId) {
+ return changeDataFactory
+ .create(projectName, patchSetId.changeId())
+ .patchSet(patchSetId)
+ .groups();
+ }
+
private Correspondence<CommitInfo, String> hasSha1() {
return NullAwareCorrespondence.transforming(commitInfo -> commitInfo.commit, "hasSha1");
}
diff --git a/javatests/com/google/gerrit/httpd/raw/IndexPreloadingUtilTest.java b/javatests/com/google/gerrit/httpd/raw/IndexPreloadingUtilTest.java
index e1cccf8..9f8f494 100644
--- a/javatests/com/google/gerrit/httpd/raw/IndexPreloadingUtilTest.java
+++ b/javatests/com/google/gerrit/httpd/raw/IndexPreloadingUtilTest.java
@@ -52,6 +52,7 @@
@Test
public void preloadOnlyForSelfDashboard() throws Exception {
assertThat(parseRequestedPage("/dashboard/self")).isEqualTo(RequestedPage.DASHBOARD);
+ assertThat(parseRequestedPage("/profile/self")).isEqualTo(RequestedPage.PROFILE);
assertThat(parseRequestedPage("/dashboard/1085901"))
.isEqualTo(RequestedPage.PAGE_WITHOUT_PRELOADING);
assertThat(parseRequestedPage("/dashboard/gerrit"))
diff --git a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
index 07159b7..3066d9e 100644
--- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -38,6 +38,7 @@
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountManager;
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.change.AddReviewersOp;
@@ -46,6 +47,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.notedb.Sequences;
import com.google.gerrit.server.patch.DiffSummary;
import com.google.gerrit.server.patch.DiffSummaryKey;
@@ -55,6 +57,8 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.name.Named;
+import java.util.ArrayList;
+import java.util.List;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -97,6 +101,7 @@
@Inject private DynamicSet<AttentionSetListener> attentionSetListeners;
@Inject private AccountManager accountManager;
@Inject private AuthRequest.Factory authRequestFactory;
+ @Inject private IdentifiedUser.GenericFactory userFactory;
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
@@ -374,6 +379,157 @@
assertThat(diffSummaryCache.asMap()).hasSize(cacheSizeBefore + 1);
}
+ @Test
+ public void executeOpsWithDifferentUsers() throws Exception {
+ Change.Id changeId = createChange();
+
+ ObjectId oldHead =
+ repo.getRepository().exactRef(RefNames.changeMetaRef(changeId)).getObjectId();
+
+ CurrentUser defaultUser = user.get();
+ IdentifiedUser user1 =
+ userFactory.create(
+ accountManager.authenticate(authRequestFactory.createForUser("user1")).getAccountId());
+ IdentifiedUser user2 =
+ userFactory.create(
+ accountManager.authenticate(authRequestFactory.createForUser("user2")).getAccountId());
+
+ TestOp testOp1 = new TestOp().addReviewer(defaultUser.getAccountId());
+ TestOp testOp2 = new TestOp().addReviewer(user1.getAccountId());
+ TestOp testOp3 = new TestOp().addReviewer(user2.getAccountId());
+
+ try (BatchUpdate bu = batchUpdateFactory.create(project, defaultUser, TimeUtil.now())) {
+ bu.addOp(changeId, user1, testOp1);
+ bu.addOp(changeId, user2, testOp2);
+ bu.addOp(changeId, testOp3);
+ bu.execute();
+ }
+
+ assertThat(testOp1.updateRepoUser).isEqualTo(user1);
+ assertThat(testOp1.updateChangeUser).isEqualTo(user1);
+ assertThat(testOp1.postUpdateUser).isEqualTo(user1);
+
+ assertThat(testOp2.updateRepoUser).isEqualTo(user2);
+ assertThat(testOp2.updateChangeUser).isEqualTo(user2);
+ assertThat(testOp2.postUpdateUser).isEqualTo(user2);
+
+ assertThat(testOp3.updateRepoUser).isEqualTo(defaultUser);
+ assertThat(testOp3.updateChangeUser).isEqualTo(defaultUser);
+ assertThat(testOp3.postUpdateUser).isEqualTo(defaultUser);
+
+ // Verify that we got one meta commit per op.
+ RevCommit metaCommitForTestOp3 =
+ repo.getRepository()
+ .parseCommit(
+ repo.getRepository().exactRef(RefNames.changeMetaRef(changeId)).getObjectId());
+ assertThat(metaCommitForTestOp3.getAuthorIdent().getEmailAddress())
+ .isEqualTo(String.format("%s@gerrit", defaultUser.getAccountId()));
+ assertThat(metaCommitForTestOp3.getFullMessage())
+ .startsWith(
+ "Update patch set 1\n"
+ + "\n"
+ + "Patch-set: 1\n"
+ + String.format(
+ "Reviewer: Gerrit User %s <%s@gerrit>\n",
+ user2.getAccountId(), user2.getAccountId())
+ + "Attention:");
+
+ RevCommit metaCommitForTestOp2 =
+ repo.getRepository().parseCommit(metaCommitForTestOp3.getParent(0));
+ assertThat(metaCommitForTestOp2.getAuthorIdent().getEmailAddress())
+ .isEqualTo(String.format("%s@gerrit", user2.getAccountId()));
+ assertThat(metaCommitForTestOp2.getFullMessage())
+ .startsWith(
+ "Update patch set 1\n"
+ + "\n"
+ + "Patch-set: 1\n"
+ + String.format(
+ "Reviewer: Gerrit User %s <%s@gerrit>\n",
+ user1.getAccountId(), user1.getAccountId())
+ + "Attention:");
+
+ RevCommit metaCommitForTestOp1 =
+ repo.getRepository().parseCommit(metaCommitForTestOp2.getParent(0));
+ assertThat(metaCommitForTestOp1.getAuthorIdent().getEmailAddress())
+ .isEqualTo(String.format("%s@gerrit", user1.getAccountId()));
+ assertThat(metaCommitForTestOp1.getFullMessage())
+ .isEqualTo(
+ "Update patch set 1\n"
+ + "\n"
+ + "Patch-set: 1\n"
+ + String.format(
+ "Reviewer: Gerrit User %s <%s@gerrit>\n",
+ defaultUser.getAccountId(), defaultUser.getAccountId()));
+
+ assertThat(metaCommitForTestOp1.getParent(0)).isEqualTo(oldHead);
+ }
+
+ @Test
+ public void executeOpsWithSameUser() throws Exception {
+ Change.Id changeId = createChange();
+
+ ObjectId oldHead =
+ repo.getRepository().exactRef(RefNames.changeMetaRef(changeId)).getObjectId();
+
+ CurrentUser defaultUser = user.get();
+ IdentifiedUser user1 =
+ userFactory.create(
+ accountManager.authenticate(authRequestFactory.createForUser("user1")).getAccountId());
+ IdentifiedUser user2 =
+ userFactory.create(
+ accountManager.authenticate(authRequestFactory.createForUser("user2")).getAccountId());
+
+ TestOp testOp1 = new TestOp().addReviewer(user1.getAccountId());
+ TestOp testOp2 = new TestOp().addReviewer(user2.getAccountId());
+
+ try (BatchUpdate bu = batchUpdateFactory.create(project, defaultUser, TimeUtil.now())) {
+ bu.addOp(changeId, defaultUser, testOp1);
+ bu.addOp(changeId, testOp2);
+ bu.execute();
+ }
+
+ assertThat(testOp1.updateRepoUser).isEqualTo(defaultUser);
+ assertThat(testOp1.updateChangeUser).isEqualTo(defaultUser);
+ assertThat(testOp1.postUpdateUser).isEqualTo(defaultUser);
+
+ assertThat(testOp2.updateRepoUser).isEqualTo(defaultUser);
+ assertThat(testOp2.updateChangeUser).isEqualTo(defaultUser);
+ assertThat(testOp2.postUpdateUser).isEqualTo(defaultUser);
+
+ // Verify that we got a single meta commit (updates of both ops squashed into one commit).
+ RevCommit metaCommit =
+ repo.getRepository()
+ .parseCommit(
+ repo.getRepository().exactRef(RefNames.changeMetaRef(changeId)).getObjectId());
+ assertThat(metaCommit.getAuthorIdent().getEmailAddress())
+ .isEqualTo(String.format("%s@gerrit", defaultUser.getAccountId()));
+ assertThat(metaCommit.getFullMessage())
+ .startsWith(
+ "Update patch set 1\n"
+ + "\n"
+ + "Patch-set: 1\n"
+ + String.format(
+ "Reviewer: Gerrit User %s <%s@gerrit>\n",
+ user1.getAccountId(), user1.getAccountId())
+ + String.format(
+ "Reviewer: Gerrit User %s <%s@gerrit>\n",
+ user2.getAccountId(), user2.getAccountId())
+ + "Attention:");
+
+ assertThat(metaCommit.getParent(0)).isEqualTo(oldHead);
+ }
+
+ private Change.Id createChange() throws Exception {
+ Change.Id id = Change.id(sequences.nextChangeId());
+ try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.now())) {
+ bu.insertChange(
+ changeInserterFactory.create(
+ id, repo.commit().message("Change").insertChangeId().create(), "refs/heads/master"));
+ bu.execute();
+ }
+ return id;
+ }
+
private Change.Id createChangeWithUpdates(int totalUpdates) throws Exception {
checkArgument(totalUpdates > 0);
checkArgument(totalUpdates <= MAX_UPDATES);
@@ -468,4 +624,38 @@
return true;
}
}
+
+ private static class TestOp implements BatchUpdateOp {
+ CurrentUser updateRepoUser;
+ CurrentUser updateChangeUser;
+ CurrentUser postUpdateUser;
+
+ private List<Account.Id> reviewersToAdd = new ArrayList<>();
+
+ TestOp addReviewer(Account.Id accountId) {
+ reviewersToAdd.add(accountId);
+ return this;
+ }
+
+ @Override
+ public void updateRepo(RepoContext ctx) {
+ updateRepoUser = ctx.getUser();
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx) {
+ updateChangeUser = ctx.getUser();
+
+ reviewersToAdd.forEach(
+ accountId ->
+ ctx.getUpdate(ctx.getChange().currentPatchSetId())
+ .putReviewer(accountId, ReviewerStateInternal.REVIEWER));
+ return true;
+ }
+
+ @Override
+ public void postUpdate(PostUpdateContext ctx) {
+ postUpdateUser = ctx.getUser();
+ }
+ }
}
diff --git a/polygerrit-ui/app/constants/reporting.ts b/polygerrit-ui/app/constants/reporting.ts
index be86863..ad59edd 100644
--- a/polygerrit-ui/app/constants/reporting.ts
+++ b/polygerrit-ui/app/constants/reporting.ts
@@ -124,32 +124,7 @@
CHECKS_RUNS_PANEL_TOGGLE = 'checks-runs-panel-toggle',
CHECKS_RUNS_SELECTED_TRIGGERED = 'checks-runs-selected-triggered',
CHECKS_STATS = 'checks-stats',
- // The following interactions are logged for investigating a spurious bug of
- // auto-closing draft comments.
- COMMENTS_AUTOCLOSE_FIRST_UPDATE = 'comments-autoclose-first-update',
- COMMENTS_AUTOCLOSE_EDITING_FALSE_SAVE = 'comments-autoclose-editing-false-save',
- COMMENTS_AUTOCLOSE_EDITING_DISCONNECTED = 'comments-autoclose-editing-disconnected',
- COMMENTS_AUTOCLOSE_EDITING_THREAD_DISCONNECTED = 'comments-autoclose-editing-thread-disconnected',
- COMMENTS_AUTOCLOSE_CHECKS_UPDATED = 'comments-autoclose-checks-updated',
- COMMENTS_AUTOCLOSE_THREADS_UPDATED = 'comments-autoclose-threads-updated',
- COMMENTS_AUTOCLOSE_COMMENT_REMOVED = 'comments-autoclose-comment-removed',
- COMMENTS_AUTOCLOSE_MESSAGES_LIST_CREATED = 'comments-autoclose-messages-list-created',
- COMMENTS_AUTOCLOSE_MESSAGES_LIST_UPDATED = 'comments-autoclose-messages-list-updated',
- COMMENTS_AUTOCLOSE_THREAD_LIST_CREATED = 'comments-autoclose-thread-list-created',
- COMMENTS_AUTOCLOSE_THREAD_LIST_UPDATED = 'comments-autoclose-thread-list-updated',
- // The following interactions are logged for investigating a spurious bug of
- // auto-closing diffs.
- DIFF_AUTOCLOSE_DIFF_UNDEFINED = 'diff-autoclose-diff-undefined',
- DIFF_AUTOCLOSE_DIFF_ONGOING = 'diff-autoclose-diff-ongoing',
- DIFF_AUTOCLOSE_RELOAD_ON_WHITESPACE = 'diff-autoclose-reload-on-whitespace',
- DIFF_AUTOCLOSE_RELOAD_ON_SYNTAX = 'diff-autoclose-reload-on-syntax',
- DIFF_AUTOCLOSE_RELOAD_FILELIST_PREFS = 'diff-autoclose-reload-filelist-prefs',
- DIFF_AUTOCLOSE_DIFF_HOST_CREATED = 'diff-autoclose-diff-host-created',
- DIFF_AUTOCLOSE_DIFF_HOST_NOT_RENDERING = 'diff-autoclose-diff-not-rendering',
- DIFF_AUTOCLOSE_FILE_LIST_UPDATED = 'diff-autoclose-file-list-updated',
- DIFF_AUTOCLOSE_SHOWN_FILES_CHANGED = 'diff-autoclose-shown-files-changed',
- // The following interaction is logged for reporting and counting a suspected
- // Chrome bug that leads to html`` misbehavior.
- AUTOCLOSE_HTML_PATCHED = 'autoclose-html-patched',
CHANGE_ACTION_FIRED = 'change-action-fired',
+ BUTTON_CLICK = 'button-click',
+ LINK_CLICK = 'link-click',
}
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
index 41ad3f8..a11951d 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
@@ -11,7 +11,6 @@
import '../../shared/gr-list-view/gr-list-view';
import '../gr-create-pointer-dialog/gr-create-pointer-dialog';
import '../gr-confirm-delete-item-dialog/gr-confirm-delete-item-dialog';
-import {encodeURL} from '../../../utils/url-util';
import {GrCreatePointerDialog} from '../gr-create-pointer-dialog/gr-create-pointer-dialog';
import {
BranchInfo,
@@ -28,12 +27,16 @@
import {formStyles} from '../../../styles/gr-form-styles';
import {tableStyles} from '../../../styles/gr-table-styles';
import {sharedStyles} from '../../../styles/shared-styles';
-import {LitElement, PropertyValues, css, html} from 'lit';
+import {LitElement, PropertyValues, css, html, nothing} from 'lit';
import {customElement, query, property, state} from 'lit/decorators.js';
import {BindValueChangeEvent} from '../../../types/events';
import {assertIsDefined} from '../../../utils/common-util';
import {ifDefined} from 'lit/directives/if-defined.js';
-import {RepoDetailView, RepoViewState} from '../../../models/views/repo';
+import {
+ createRepoUrl,
+ RepoDetailView,
+ RepoViewState,
+} from '../../../models/views/repo';
import {modalStyles} from '../../../styles/gr-modal-styles';
const PGP_START = '-----BEGIN PGP SIGNATURE-----';
@@ -132,6 +135,8 @@
}
override render() {
+ if (!this.repo) return nothing;
+ if (!this.detailType) return nothing;
return html`
<gr-list-view
.createNew=${this.loggedIn}
@@ -140,7 +145,7 @@
.items=${this.items}
.loading=${this.loading}
.offset=${this.offset}
- .path=${this.getPath(this.repo, this.detailType)}
+ .path=${this.getPath()}
@create-clicked=${() => {
this.handleCreateClicked();
}}
@@ -434,11 +439,10 @@
return Promise.reject(new Error('unknown detail type'));
}
- private getPath(repo?: RepoName, detailType?: RepoDetailView) {
- // TODO: Replace with `createRepoUrl()`, but be aware that `encodeURL()`
- // gets `false` as a second parameter here. The router pattern in gr-router
- // does not handle the filter URLs, if the repo is not encoded!
- return `/admin/repos/${encodeURL(repo ?? '')},${detailType}`;
+ private getPath() {
+ if (!this.repo) return '';
+ if (!this.detailType) return '';
+ return createRepoUrl({repo: this.repo, detail: this.detailType});
}
private computeWeblink(repo: ProjectInfo | BranchInfo | TagInfo) {
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 36ac307..ccb750d 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
@@ -47,7 +47,7 @@
isQuickLabelInfo,
LabelInfo,
NumericChangeId,
- PatchSetNum,
+ PatchSetNumber,
RequestPayload,
RevertSubmissionInfo,
ReviewInput,
@@ -110,6 +110,7 @@
import {whenVisible} from '../../../utils/dom-util';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {modalStyles} from '../../../styles/gr-modal-styles';
+import {subscribe} from '../../lit/subscription-controller';
const ERR_BRANCH_EMPTY = 'The destination branch can’t be empty.';
const ERR_COMMIT_EMPTY = 'The commit message can’t be empty.';
@@ -410,8 +411,7 @@
@property({type: Boolean})
hasParent?: boolean;
- @property({type: String})
- latestPatchNum?: PatchSetNum;
+ @state() latestPatchNum?: PatchSetNumber;
@property({type: String})
commitMessage = '';
@@ -533,6 +533,11 @@
constructor() {
super();
+ subscribe(
+ this,
+ () => this.getChangeModel().latestPatchNum$,
+ x => (this.latestPatchNum = x)
+ );
}
override connectedCallback() {
@@ -1327,7 +1332,7 @@
}
// private but used in test
- getRevision(change: ChangeViewChangeInfo, patchNum?: PatchSetNum) {
+ getRevision(change: ChangeViewChangeInfo, patchNum?: PatchSetNumber) {
for (const rev of Object.values(change.revisions)) {
if (rev._number === patchNum) {
return rev;
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 82900a3..9c01f2f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -34,7 +34,6 @@
ChangeSubmissionId,
CommitId,
NumericChangeId,
- PatchSetNum,
PatchSetNumber,
RepoName,
ReviewInput,
@@ -135,7 +134,7 @@
},
};
element.changeNum = 42 as NumericChangeId;
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
element.account = {
_account_id: 123 as AccountId,
};
@@ -514,7 +513,7 @@
rev2: {...createRevision(), _number: 2 as PatchSetNumber},
},
};
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
queryAndAssert<GrButton>(
element,
@@ -1808,7 +1807,7 @@
element.change!.is_private = false;
element.changeNum = 2 as NumericChangeId;
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
await element.updateComplete;
await element.reload();
@@ -1862,7 +1861,7 @@
element.change!.is_private = true;
element.changeNum = 2 as NumericChangeId;
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
await element.updateComplete;
await element.reload();
@@ -2331,7 +2330,7 @@
const reloadStub = sinon.stub(element, 'reload');
element.changeNum = 123 as NumericChangeId;
assert.isFalse(reloadStub.called);
- element.latestPatchNum = 456 as PatchSetNum;
+ element.latestPatchNum = 456 as PatchSetNumber;
assert.isFalse(reloadStub.called);
});
@@ -2427,7 +2426,7 @@
setup(async () => {
cleanup = sinon.stub();
element.changeNum = 42 as NumericChangeId;
- element.latestPatchNum = 12 as PatchSetNum;
+ element.latestPatchNum = 12 as PatchSetNumber;
element.change = {
...createChangeViewChange(),
revisions: createRevisions(element.latestPatchNum as number),
@@ -2695,7 +2694,7 @@
// set the following properties
element.change = createChangeViewChange();
element.changeNum = 42 as NumericChangeId;
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
stubRestApi('getRepoBranches').returns(Promise.resolve([]));
await element.updateComplete;
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 2246671..d07a6ac 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
@@ -1345,7 +1345,6 @@
.changeNum=${this.changeNum}
.changeStatus=${this.change?.status}
.commitNum=${this.commitInfo?.commit}
- .latestPatchNum=${computeLatestPatchNum(this.allPatchSets)}
.commitMessage=${this.latestCommitMessage}
.editPatchsetLoaded=${this.patchRange
? hasEditPatchsetLoaded(this.patchRange)
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 ad75253..999190e 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
@@ -51,7 +51,7 @@
import {GrCursorManager} from '../../shared/gr-cursor-manager/gr-cursor-manager';
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
import {ParsedChangeInfo, PatchSetFile} from '../../../types/types';
-import {Interaction, Timing} from '../../../constants/reporting';
+import {Timing} from '../../../constants/reporting';
import {RevisionInfo} from '../../shared/revision-info/revision-info';
import {select} from '../../../utils/observable-util';
import {resolve} from '../../../models/dependency';
@@ -78,7 +78,6 @@
import {classMap} from 'lit/directives/class-map.js';
import {incrementalRepeat} from '../../lit/incremental-repeat';
import {ifDefined} from 'lit/directives/if-defined.js';
-import {HtmlPatched} from '../../../utils/lit-util';
import {
createDiffUrl,
createEditUrl,
@@ -309,13 +308,6 @@
private readonly getBrowserModel = resolve(this, browserModelToken);
- private readonly patched = new HtmlPatched(key => {
- this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, {
- component: this.tagName,
- key: key.substring(0, 300),
- });
- });
-
shortcutsController = new ShortcutController(this);
private readonly getNavigation = resolve(this, navigationToken);
@@ -1015,25 +1007,12 @@
);
}
- // for DIFF_AUTOCLOSE logging purposes only
- private shownFilesOld: NormalizedFileInfo[] = this.shownFiles;
-
private renderShownFiles() {
const showDynamicColumns = this.computeShowDynamicColumns();
const showPrependedDynamicColumns =
this.computeShowPrependedDynamicColumns();
const sizeBarLayout = this.computeSizeBarLayout();
- // for DIFF_AUTOCLOSE logging purposes only
- if (
- this.shownFilesOld.length > 0 &&
- this.shownFiles !== this.shownFilesOld
- ) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_SHOWN_FILES_CHANGED
- );
- }
- this.shownFilesOld = this.shownFiles;
return incrementalRepeat({
values: this.shownFiles,
mapFn: (f, i) =>
@@ -1084,7 +1063,7 @@
</div>
${when(
this.isFileExpanded(file.__path),
- () => this.patched.html`
+ () => html`
<gr-diff-host
?noAutoRender=${true}
?showLoadFailure=${true}
@@ -1658,17 +1637,6 @@
this.reporting.fileListDisplayed();
}
- protected override updated(): void {
- // for DIFF_AUTOCLOSE logging purposes only
- const ids = this.diffs.map(d => d.uid);
- if (ids.length > 0) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_FILE_LIST_UPDATED,
- {l: ids.length, ids: ids.slice(0, 10)}
- );
- }
- }
-
// TODO: Move into files-model.
// visible for testing
async updateCleanlyMergedPaths() {
@@ -1787,10 +1755,6 @@
if (!this.diffs.length) {
return;
}
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_RELOAD_FILELIST_PREFS
- );
-
// Re-render all expanded diffs sequentially.
this.renderInOrder(this.expandedFiles, this.diffs);
}
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
index 72969a7..45f24d7 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
@@ -128,9 +128,6 @@
private readonly getNavigation = resolve(this, navigationToken);
- // for COMMENTS_AUTOCLOSE logging purposes only
- readonly uid = performance.now().toString(36) + Math.random().toString(36);
-
constructor() {
super();
this.addEventListener('click', e => this.handleClick(e));
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
index c46f4fc..972b871 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
@@ -43,7 +43,6 @@
shortcutsServiceToken,
} from '../../../services/shortcuts/shortcuts-service';
import {GrFormattedText} from '../../shared/gr-formatted-text/gr-formatted-text';
-import {Interaction} from '../../../constants/reporting';
/**
* The content of the enum is also used in the UI for the button text.
@@ -353,21 +352,6 @@
this.changeNum = x;
}
);
- // for COMMENTS_AUTOCLOSE logging purposes only
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_MESSAGES_LIST_CREATED
- );
- }
-
- override updated(): void {
- // for COMMENTS_AUTOCLOSE logging purposes only
- const messages = this.shadowRoot!.querySelectorAll('gr-message');
- if (messages.length > 0) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_MESSAGES_LIST_UPDATED,
- {uid: messages[0].uid}
- );
- }
}
override willUpdate(changedProperties: PropertyValues): void {
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 599e38b..7015900 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
@@ -42,11 +42,8 @@
import {ParsedChangeInfo} from '../../../types/types';
import {repeat} from 'lit/directives/repeat.js';
import {GrCommentThread} from '../../shared/gr-comment-thread/gr-comment-thread';
-import {getAppContext} from '../../../services/app-context';
import {resolve} from '../../../models/dependency';
import {changeModelToken} from '../../../models/change/change-model';
-import {Interaction} from '../../../constants/reporting';
-import {HtmlPatched} from '../../../utils/lit-util';
import {userModelToken} from '../../../models/user/user-model';
import {specialFilePathCompare} from '../../../utils/path-list-util';
@@ -206,17 +203,8 @@
private readonly getChangeModel = resolve(this, changeModelToken);
- private readonly reporting = getAppContext().reportingService;
-
private readonly getUserModel = resolve(this, userModelToken);
- private readonly patched = new HtmlPatched(key => {
- this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, {
- component: this.tagName,
- key: key.substring(0, 300),
- });
- });
-
constructor() {
super();
subscribe(
@@ -234,10 +222,6 @@
() => this.getUserModel().account$,
x => (this.account = x)
);
- // for COMMENTS_AUTOCLOSE logging purposes only
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_THREAD_LIST_CREATED
- );
}
override willUpdate(changed: PropertyValues) {
@@ -341,17 +325,6 @@
];
}
- override updated(): void {
- // for COMMENTS_AUTOCLOSE logging purposes only
- const threads = this.shadowRoot!.querySelectorAll('gr-comment-thread');
- if (threads.length > 0) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_THREAD_LIST_UPDATED,
- {uid: threads[0].uid}
- );
- }
- }
-
override render() {
return html`
${this.renderDropdown()}
@@ -425,16 +398,16 @@
index === 0 || threads[index - 1].path !== threads[index].path;
const separator =
index !== 0 && isFirst
- ? this.patched.html`<div class="thread-separator"></div>`
+ ? html`<div class="thread-separator"></div>`
: undefined;
const commentThread = this.renderCommentThread(thread, isFirst);
- return this.patched.html`${separator}${commentThread}`;
+ return html`${separator}${commentThread}`;
}
);
}
private renderCommentThread(thread: CommentThread, isFirst: boolean) {
- return this.patched.html`
+ return html`
<gr-comment-thread
.thread=${thread}
show-file-path
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index f3be5c4..5071e4f 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -72,7 +72,6 @@
import {changeModelToken} from '../../models/change/change-model';
import {getAppContext} from '../../services/app-context';
import {when} from 'lit/directives/when.js';
-import {HtmlPatched} from '../../utils/lit-util';
import {DropdownItem} from '../shared/gr-dropdown-list/gr-dropdown-list';
import './gr-checks-attempt';
import {createDiffUrl, changeViewModelToken} from '../../models/views/change';
@@ -818,13 +817,6 @@
private readonly reporting = getAppContext().reportingService;
- private readonly patched = new HtmlPatched(key => {
- this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, {
- component: this.tagName,
- key: key.substring(0, 300),
- });
- });
-
constructor() {
super();
subscribe(
@@ -1502,7 +1494,7 @@
${repeat(
filtered,
result => result.internalResultId,
- (result?: RunResult) => this.patched.html`
+ (result?: RunResult) => html`
<gr-result-row
class=${charsOnly(result!.checkName)}
.result=${result}
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 43a6819..0fa5503 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
@@ -64,7 +64,7 @@
import {assertIsDefined} from '../../../utils/common-util';
import {DiffContextExpandedEventDetail} from '../../../embed/diff/gr-diff-builder/gr-diff-builder';
import {TokenHighlightLayer} from '../../../embed/diff/gr-diff-builder/token-highlight-layer';
-import {Timing, Interaction} from '../../../constants/reporting';
+import {Timing} from '../../../constants/reporting';
import {ChangeComments} from '../gr-comment-api/gr-comment-api';
import {Subscription} from 'rxjs';
import {
@@ -336,9 +336,6 @@
private checksSubscription?: Subscription;
- // for DIFF_AUTOCLOSE logging purposes only
- readonly uid = performance.now().toString(36) + Math.random().toString(36);
-
constructor() {
super();
this.syntaxLayer = new GrSyntaxLayerWorker(
@@ -386,23 +383,6 @@
this.prefs = diffPreferences;
}
);
- this.logForDiffAutoClose();
- }
-
- // for DIFF_AUTOCLOSE logging purposes only
- private logForDiffAutoClose() {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_DIFF_HOST_CREATED,
- {uid: this.uid}
- );
- setTimeout(() => {
- if (!this.hasReloadBeenCalledOnce) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_DIFF_HOST_NOT_RENDERING,
- {uid: this.uid}
- );
- }
- }, /* 10 seconds */ 10000);
}
override connectedCallback() {
@@ -586,14 +566,7 @@
return this.reloadPromise;
}
- // for DIFF_AUTOCLOSE logging purposes only
- private reloadOngoing = false;
-
- // for DIFF_AUTOCLOSE logging purposes only
- private hasReloadBeenCalledOnce = false;
-
async reloadInternal(shouldReportMetric?: boolean) {
- this.hasReloadBeenCalledOnce = true;
this.reporting.time(Timing.DIFF_TOTAL);
this.reporting.time(Timing.DIFF_LOAD);
// TODO: Find better names for these 3 clear/cancel methods. Ideally the
@@ -606,10 +579,6 @@
this.diff = undefined;
this.errorMessage = null;
const whitespaceLevel = this.getIgnoreWhitespace();
- if (this.reloadOngoing) {
- this.reporting.reportInteraction(Interaction.DIFF_AUTOCLOSE_DIFF_ONGOING);
- }
- this.reloadOngoing = true;
try {
// We are carefully orchestrating operations that have to wait for another
@@ -619,11 +588,6 @@
// assets in parallel.
const layerPromise = this.initLayers();
const diff = await this.getDiff();
- if (diff === undefined) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_DIFF_UNDEFINED
- );
- }
this.loadedWhitespaceLevel = whitespaceLevel;
this.reportDiff(diff);
@@ -670,7 +634,6 @@
}
} finally {
this.reporting.timeEnd(Timing.DIFF_TOTAL, this.timingDetails());
- this.reloadOngoing = false;
}
}
@@ -758,9 +721,6 @@
const idToEl = new Map<string, GrDiffCheckResult>();
const checkEls = this.getCheckEls();
const dontRemove = new Set<GrDiffCheckResult>();
- let createdCount = 0;
- let updatedCount = 0;
- let removedCount = 0;
const checksCount = checks.length;
const checkElsCount = checkEls.length;
if (checksCount === 0 && checkElsCount === 0) return;
@@ -775,23 +735,16 @@
if (existingEl) {
existingEl.result = check;
dontRemove.add(existingEl);
- updatedCount++;
} else {
const newEl = this.createCheckEl(check);
dontRemove.add(newEl);
- createdCount++;
}
}
// Remove all check els that don't have a matching check anymore.
for (const el of checkEls) {
if (dontRemove.has(el)) continue;
el.remove();
- removedCount++;
}
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_CHECKS_UPDATED,
- {createdCount, updatedCount, removedCount, checksCount, checkElsCount}
- );
}
/**
@@ -1106,9 +1059,6 @@
}
}
const dontRemove = new Set<GrCommentThread>();
- let createdCount = 0;
- let updatedCount = 0;
- let removedCount = 0;
const threadCount = threads.length;
const threadElCount = threadEls.length;
if (threadCount === 0 && threadElCount === 0) return;
@@ -1150,12 +1100,10 @@
) {
existingThreadEl.thread = thread;
dontRemove.add(existingThreadEl);
- updatedCount++;
} else {
const threadEl = this.createThreadElement(thread);
this.attachThreadElement(threadEl);
dontRemove.add(threadEl);
- createdCount++;
}
}
// Remove all threads that are no longer existing.
@@ -1165,13 +1113,8 @@
// might be unsaved and thus not be reflected in `threads` yet, so let's
// keep them open.
if (threadEl.editing && threadEl.thread?.comments.length === 0) continue;
- removedCount++;
threadEl.remove();
}
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_THREADS_UPDATED,
- {createdCount, updatedCount, removedCount, threadCount, threadElCount}
- );
const portedThreadsCount = threads.filter(thread => thread.ported).length;
const portedThreadsWithoutRange = threads.filter(
thread => thread.ported && thread.rangeInfoLost
@@ -1371,9 +1314,6 @@
preferredWhitespaceLevel !== loadedWhitespaceLevel &&
!noRenderOnPrefsChange
) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_RELOAD_ON_WHITESPACE
- );
return this.reload();
}
}
@@ -1392,9 +1332,6 @@
if (oldPrefs?.syntax_highlighting === prefs.syntax_highlighting) return;
if (!noRenderOnPrefsChange) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_RELOAD_ON_SYNTAX
- );
return this.reload();
}
}
diff --git a/polygerrit-ui/app/elements/gr-app-global-var-init.ts b/polygerrit-ui/app/elements/gr-app-global-var-init.ts
index d66fec0..d6a14ed 100644
--- a/polygerrit-ui/app/elements/gr-app-global-var-init.ts
+++ b/polygerrit-ui/app/elements/gr-app-global-var-init.ts
@@ -20,6 +20,7 @@
initPerformanceReporter,
initErrorReporter,
initWebVitals,
+ initClickReporter,
} from '../services/gr-reporting/gr-reporting_impl';
import {Finalizable} from '../services/registry';
@@ -34,6 +35,7 @@
initPerformanceReporter(reportingService);
initWebVitals(reportingService);
initErrorReporter(reportingService);
+ initClickReporter(reportingService);
}
window.GrAnnotation = GrAnnotation;
window.GrPluginActionContext = GrPluginActionContext;
diff --git a/polygerrit-ui/app/elements/shared/gr-button/gr-button.ts b/polygerrit-ui/app/elements/shared/gr-button/gr-button.ts
index 75974e5..b44a16b 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button.ts
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button.ts
@@ -11,6 +11,7 @@
import {addShortcut, getEventPath, Key} from '../../../utils/dom-util';
import {getAppContext} from '../../../services/app-context';
import {classMap} from 'lit/directives/class-map.js';
+import {Interaction} from '../../../constants/reporting';
declare global {
interface HTMLElementTagNameMap {
@@ -228,6 +229,8 @@
return;
}
- this.reporting.reportInteraction('button-click', {path: getEventPath(e)});
+ this.reporting.reportInteraction(Interaction.BUTTON_CLICK, {
+ path: getEventPath(e),
+ });
}
}
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 062aa54..aba0aea 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
@@ -70,8 +70,6 @@
import {commentsModelToken} from '../../../models/comments/comments-model';
import {changeModelToken} from '../../../models/change/change-model';
import {whenRendered} from '../../../utils/dom-util';
-import {Interaction} from '../../../constants/reporting';
-import {HtmlPatched} from '../../../utils/lit-util';
import {createChangeUrl, createDiffUrl} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
@@ -254,8 +252,6 @@
private readonly getUserModel = resolve(this, userModelToken);
- private readonly reporting = getAppContext().reportingService;
-
private readonly shortcuts = new ShortcutController(this);
private readonly syntaxLayer = new GrSyntaxLayerWorker(
@@ -263,16 +259,6 @@
() => getAppContext().reportingService
);
- // for COMMENTS_AUTOCLOSE logging purposes only
- readonly uid = performance.now().toString(36) + Math.random().toString(36);
-
- private readonly patched = new HtmlPatched(key => {
- this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, {
- component: this.tagName,
- key: key.substring(0, 300),
- });
- });
-
constructor() {
super();
this.shortcuts.addGlobal({key: 'e'}, () => this.handleExpandShortcut());
@@ -322,15 +308,6 @@
);
}
- override disconnectedCallback() {
- if (this.editing) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_EDITING_THREAD_DISCONNECTED
- );
- }
- super.disconnectedCallback();
- }
-
static override get styles() {
return [
a11yStyles,
@@ -519,16 +496,15 @@
(this.messageId
? comment.change_message_id !== this.messageId
: !this.unresolved);
- return this.patched.html`
+ return html`
<gr-comment
.comment=${comment}
.comments=${this.thread!.comments}
?initially-collapsed=${initiallyCollapsed}
?robot-button-disabled=${robotButtonDisabled}
?show-patchset=${this.showPatchset}
- ?show-ported-comment=${
- this.showPortedComment && comment.id === this.rootId
- }
+ ?show-ported-comment=${this.showPortedComment &&
+ comment.id === this.rootId}
@reply-to-comment=${this.handleReplyToComment}
@copy-comment-link=${this.handleCopyLink}
@comment-editing-changed=${(
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 0edfe03..f7e8edd 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -57,7 +57,6 @@
import {Subject} from 'rxjs';
import {debounceTime} from 'rxjs/operators';
import {changeModelToken} from '../../../models/change/change-model';
-import {Interaction} from '../../../constants/reporting';
import {KnownExperimentId} from '../../../services/flags/flags';
import {isBase64FileContent} from '../../../api/rest-api';
import {createDiffUrl} from '../../../models/views/change';
@@ -318,11 +317,6 @@
override disconnectedCallback() {
// Clean up emoji dropdown.
if (this.textarea) this.textarea.closeDropdown();
- if (this.editing) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_EDITING_DISCONNECTED
- );
- }
super.disconnectedCallback();
}
@@ -938,10 +932,6 @@
this.unresolved = this.comment.unresolved ?? true;
if (isUnsaved(this.comment)) this.editing = true;
if (isDraftOrUnsaved(this.comment)) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_FIRST_UPDATE,
- {editing: this.editing, unsaved: isUnsaved(this.comment)}
- );
this.collapsed = false;
} else {
this.collapsed = !!this.initiallyCollapsed;
@@ -1210,9 +1200,6 @@
await this.rawSave(messageToSave, {showToast: true});
}
}
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_EDITING_FALSE_SAVE
- );
if (!this.permanentEditingMode) {
this.editing = false;
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-image-viewer/gr-image-viewer.ts b/polygerrit-ui/app/embed/diff/gr-diff-image-viewer/gr-image-viewer.ts
index cb466c3..e8575ff 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-image-viewer/gr-image-viewer.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-image-viewer/gr-image-viewer.ts
@@ -26,7 +26,7 @@
import {Dimensions, fitToFrame, FrameConstrainer, Point, Rect} from './util';
import {ValueChangedEvent} from '../../../types/events';
-import {fireNoBubbleNoCompose} from '../../../utils/event-util';
+import {fire} from '../../../utils/event-util';
import {ImageDiffAction} from '../../../api/diff';
const DRAG_DEAD_ZONE_PIXELS = 5;
@@ -683,7 +683,7 @@
}
fireAction(detail: ImageDiffAction) {
- fireNoBubbleNoCompose(this, 'image-diff-action', detail);
+ fire(this, 'image-diff-action', detail);
}
selectBase() {
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
index 06e8204..5b6e852 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
@@ -17,6 +17,7 @@
Timing,
} from '../../constants/reporting';
import {onCLS, onFID, onLCP, Metric, onINP} from 'web-vitals';
+import {getEventPath, isElementTarget} from '../../utils/dom-util';
// Latency reporting constants.
@@ -186,6 +187,22 @@
});
}
+export function initClickReporter(reportingService: ReportingService) {
+ document.addEventListener('click', (e: MouseEvent) => {
+ const anchorEl = e
+ .composedPath()
+ .find(el => isElementTarget(el) && el.tagName.toUpperCase() === 'A') as
+ | HTMLAnchorElement
+ | undefined;
+ if (!anchorEl) return;
+ reportingService.reportInteraction(Interaction.LINK_CLICK, {
+ path: getEventPath(e),
+ link: anchorEl.href,
+ text: anchorEl.innerText,
+ });
+ });
+}
+
export function initWebVitals(reportingService: ReportingService) {
function reportWebVitalMetric(name: Timing, metric: Metric) {
let score = metric.value;
diff --git a/polygerrit-ui/app/utils/lit-util.ts b/polygerrit-ui/app/utils/lit-util.ts
deleted file mode 100644
index 7ffab89..0000000
--- a/polygerrit-ui/app/utils/lit-util.ts
+++ /dev/null
@@ -1,69 +0,0 @@
-/**
- * @license
- * Copyright 2022 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {html} from 'lit';
-
-/**
- * This is a patched version of html`` to work around this Chrome bug:
- * https://bugs.chromium.org/p/v8/issues/detail?id=13190.
- *
- * The problem is that Chrome should guarantee that the TemplateStringsArray
- * is always the same instance, if the strings themselves are equal, but that
- * guarantee seems to be broken. So we are maintaining a map from
- * "concatenated strings" to TemplateStringsArray. If "concatenated strings"
- * are equal, then return the already known instance of TemplateStringsArray,
- * so html`` can use its strict equality check on it.
- */
-export class HtmlPatched {
- constructor(private readonly reporter?: (key: string) => void) {}
-
- /**
- * If `strings` are in this set, then we are sure that they are also in the
- * map, and that we will not run into the issue of "same key, but different
- * strings array". So this set allows us to optimize performance a bit, and
- * call the native html`` function early.
- */
- private readonly lookupSet = new Set<TemplateStringsArray>();
-
- private readonly lookupMap = new Map<string, TemplateStringsArray>();
-
- /**
- * Proxies lit's html`` tagges template literal. See
- * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
- * https://lit.dev/docs/libraries/standalone-templates/
- *
- * Example: If you call html`a${1}b${2}c`, then
- * ['a', 'b', 'c'] are the "strings", and 1, 2 are the "values".
- */
- html(strings: TemplateStringsArray, ...values: unknown[]) {
- if (this.lookupSet.has(strings)) {
- return this.nativeHtml(strings, ...values);
- }
-
- const key = strings.join('\0');
- const oldStrings = this.lookupMap.get(key);
-
- if (oldStrings === undefined) {
- this.lookupSet.add(strings);
- this.lookupMap.set(key, strings);
- return this.nativeHtml(strings, ...values);
- }
-
- if (oldStrings === strings) {
- return this.nativeHtml(strings, ...values);
- }
-
- // Without using HtmlPatcher html`` would be called with `strings`,
- // which will be considered different, although actually being equal.
- console.warn(`HtmlPatcher was required for '${key.substring(0, 100)}'.`);
- this.reporter?.(key);
- return this.nativeHtml(oldStrings, ...values);
- }
-
- // Allows spying on calls in tests.
- nativeHtml(strings: TemplateStringsArray, ...values: unknown[]) {
- return html(strings, ...values);
- }
-}
diff --git a/polygerrit-ui/app/utils/lit-util_test.ts b/polygerrit-ui/app/utils/lit-util_test.ts
deleted file mode 100644
index 17197f0..0000000
--- a/polygerrit-ui/app/utils/lit-util_test.ts
+++ /dev/null
@@ -1,118 +0,0 @@
-/**
- * @license
- * Copyright 2020 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {assert} from '@open-wc/testing';
-import '../test/common-test-setup';
-import {HtmlPatched} from './lit-util';
-
-function tsa(strings: string[]): TemplateStringsArray {
- return strings as unknown as TemplateStringsArray;
-}
-
-suite('lit-util HtmlPatched tests', () => {
- let patched: HtmlPatched;
- let nativeHtmlSpy: sinon.SinonSpy;
- let reporterSpy: sinon.SinonSpy;
-
- setup(async () => {
- reporterSpy = sinon.spy();
- patched = new HtmlPatched(reporterSpy);
- nativeHtmlSpy = sinon.spy(patched, 'nativeHtml');
- });
-
- test('simple call', () => {
- const instance1 = tsa(['1']);
- patched.html(instance1, 'a value');
- assert.equal(nativeHtmlSpy.callCount, 1);
- assert.equal(reporterSpy.callCount, 0);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].args[0], instance1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].args[1], 'a value');
- });
-
- test('two calls, same instance', () => {
- const instance1 = tsa(['1']);
- patched.html(instance1, 'a value');
- patched.html(instance1, 'a value');
- assert.equal(nativeHtmlSpy.callCount, 2);
- assert.equal(reporterSpy.callCount, 0);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1);
- });
-
- test('two calls, different strings', () => {
- const instance1 = tsa(['1']);
- const instance2 = tsa(['2']);
- patched.html(instance1, 'a value');
- patched.html(instance2, 'a value');
- assert.equal(nativeHtmlSpy.callCount, 2);
- assert.equal(reporterSpy.callCount, 0);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance2);
- });
-
- test('two calls, same strings, different instances', () => {
- const instance1 = tsa(['1']);
- const instance2 = tsa(['1']);
- patched.html(instance1, 'a value');
- patched.html(instance2, 'a value');
- assert.equal(nativeHtmlSpy.callCount, 2);
- assert.equal(reporterSpy.callCount, 1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1);
- });
-
- test('many calls', () => {
- const instance1a = tsa(['1']);
- const instance1b = tsa(['1']);
- const instance1c = tsa(['1']);
- const instance2a = tsa(['asdf', 'qwer']);
- const instance2b = tsa(['asdf', 'qwer']);
- const instance2c = tsa(['asdf', 'qwer']);
- const instance3a = tsa(['asd', 'fqwer']);
- const instance3b = tsa(['asd', 'fqwer']);
- const instance3c = tsa(['asd', 'fqwer']);
-
- patched.html(instance1a, 'a value');
- patched.html(instance1a, 'a value');
- patched.html(instance1b, 'a value');
- patched.html(instance1b, 'a value');
- patched.html(instance1c, 'a value');
- patched.html(instance1c, 'a value');
- patched.html(instance2a, 'a value');
- patched.html(instance2a, 'a value');
- patched.html(instance2b, 'a value');
- patched.html(instance2b, 'a value');
- patched.html(instance2c, 'a value');
- patched.html(instance2c, 'a value');
- patched.html(instance3a, 'a value');
- patched.html(instance3a, 'a value');
- patched.html(instance3b, 'a value');
- patched.html(instance3b, 'a value');
- patched.html(instance3c, 'a value');
- patched.html(instance3c, 'a value');
-
- assert.equal(nativeHtmlSpy.callCount, 18);
- assert.equal(reporterSpy.callCount, 12);
-
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[2].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[3].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[4].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[5].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[6].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[7].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[8].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[9].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[10].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[11].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[12].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[13].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[14].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[15].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[16].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[17].firstArg, instance3a);
- });
-});
