Merge "Add a standard `<gr-weblink>` component"
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 348649d..cf89982 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -1417,8 +1417,8 @@
Allow to link:cmd-set-account.html[modify accounts over the ssh prompt].
This capability allows the granted group members to modify any user account
-setting. In addition this capability is required to view secondary emails
-of other accounts.
+setting. In addition this capability allows to view secondary emails of other
+accounts.
[[capability_priority]]
=== Priority
@@ -1520,7 +1520,8 @@
This capability allows to view all accounts but not all account data.
E.g. secondary emails of all accounts can only be viewed with the
-link:#capability_modifyAccount[Modify Account] capability.
+link:#capability_viewSecondaryEmails[View Secondary Emails] capability
+or the link:#capability_modifyAccount[Modify Account] capability.
[[capability_viewCaches]]
@@ -1553,6 +1554,15 @@
link:cmd-show-queue.html[look at the Gerrit task queue via ssh].
+[[capability_viewSecondaryEmails]]
+=== View Secondary Emails
+
+Allows viewing secondary emails of other accounts.
+
+Users with the link:#capability_modifyAccount[Modify Account] capability have
+this capbility implicitly.
+
+
[[reference]]
== Permission evaluation reference
diff --git a/Documentation/config-project-config.txt b/Documentation/config-project-config.txt
index 31008f6..7b1baba 100644
--- a/Documentation/config-project-config.txt
+++ b/Documentation/config-project-config.txt
@@ -457,11 +457,6 @@
NOTE: Rebasing merge commits is not supported. If a change with a merge commit
is submitted the link:#merge_if_necessary[merge if necessary] submit action is
applied.
-+
-When rebasing the patchset, Gerrit automatically appends onto the end of the
-commit message a short summary of the change's approvals, and a URL link back
-to the change in the web UI (see link:#submit-footers[below]). If a fast-forward
-is done no footers are added.
[[rebase_always]]
* 'rebase always':
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index e6494e6..cf5da07 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -204,6 +204,8 @@
Whether the rebase was done on behalf of the uploader.
** `rebase_chain`:
Whether a chain was rebased.
+** `allow_conflicts`:
+ Whether the rebase was done with allowing conflicts.
* `change/submitted_with_rebaser_approval`: Number of rebased changes that were
submitted with a Code-Review approval of the rebaser that would not have been
submittable if the rebase was not done on behalf of the uploader.
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt
index 7e06e4a..7646777 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -2316,6 +2316,9 @@
link:access-control.html#capability_viewPlugins[View Plugins] capability.
|`viewQueue` |not set if `false`|Whether the user has the
link:access-control.html#capability_viewQueue[View Queue] capability.
+|`viewSecondaryEmails`|not set if `false`|Whether the user has the
+link:access-control.html#capability_viewSecondaryEmails[View Secondary
+Emails] capability.
|=================================
[[contributor-agreement-info]]
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index 4293fd6..67b8d75 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -647,7 +647,7 @@
last update (comment or patch set) from the change owner.
[[author]]
-author:'AUTHOR'::
+author:'AUTHOR', a:'AUTHOR'::
+
Changes where 'AUTHOR' is the author of the current patch set. 'AUTHOR' may be
the author's exact email address, or part of the name or email address. The
diff --git a/java/com/google/gerrit/common/data/GlobalCapability.java b/java/com/google/gerrit/common/data/GlobalCapability.java
index c7a9a63..23151c2 100644
--- a/java/com/google/gerrit/common/data/GlobalCapability.java
+++ b/java/com/google/gerrit/common/data/GlobalCapability.java
@@ -129,6 +129,9 @@
/** Can view all pending tasks in the queue (not just the filtered set). */
public static final String VIEW_QUEUE = "viewQueue";
+ /** Can view secondary emails of other accounts. */
+ public static final String VIEW_SECONDARY_EMAILS = "viewSecondaryEmails";
+
private static final List<String> NAMES_ALL;
private static final List<String> NAMES_LC;
private static final String[] RANGE_NAMES = {
@@ -160,6 +163,7 @@
NAMES_ALL.add(VIEW_CONNECTIONS);
NAMES_ALL.add(VIEW_PLUGINS);
NAMES_ALL.add(VIEW_QUEUE);
+ NAMES_ALL.add(VIEW_SECONDARY_EMAILS);
NAMES_LC = new ArrayList<>(NAMES_ALL.size());
for (String name : NAMES_ALL) {
diff --git a/java/com/google/gerrit/entities/BUILD b/java/com/google/gerrit/entities/BUILD
index c0f5de6..dbbcf71 100644
--- a/java/com/google/gerrit/entities/BUILD
+++ b/java/com/google/gerrit/entities/BUILD
@@ -10,6 +10,7 @@
deps = [
"//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/extensions:api",
+ "//java/com/google/gerrit/server:project_util",
"//lib:gson",
"//lib:guava",
"//lib:jgit",
diff --git a/java/com/google/gerrit/entities/Project.java b/java/com/google/gerrit/entities/Project.java
index b587b1d..b43650b 100644
--- a/java/com/google/gerrit/entities/Project.java
+++ b/java/com/google/gerrit/entities/Project.java
@@ -23,6 +23,7 @@
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.server.ProjectUtil;
import java.io.Serializable;
import java.util.Arrays;
import java.util.HashMap;
@@ -61,7 +62,7 @@
/** Parse a Project.NameKey out of a string representation. */
public static NameKey parse(String str) {
- return nameKey(KeyUtil.decode(str));
+ return nameKey(ProjectUtil.sanitizeProjectName(KeyUtil.decode(str)));
}
private final String name;
diff --git a/java/com/google/gerrit/server/BUILD b/java/com/google/gerrit/server/BUILD
index 2be3383..3338464 100644
--- a/java/com/google/gerrit/server/BUILD
+++ b/java/com/google/gerrit/server/BUILD
@@ -14,12 +14,22 @@
"account/externalids/testing/ExternalIdTestUtil.java",
]
+PROJECT_UTIL_SRC = [
+ "ProjectUtil.java",
+]
+
java_library(
name = "constants",
srcs = CONSTANTS_SRC,
visibility = ["//visibility:public"],
)
+java_library(
+ name = "project_util",
+ srcs = PROJECT_UTIL_SRC,
+ visibility = ["//visibility:public"],
+)
+
# Giant kitchen-sink target.
#
# The only reason this hasn't been split up further is because we have too many
@@ -30,13 +40,14 @@
name = "server",
srcs = glob(
["**/*.java"],
- exclude = CONSTANTS_SRC + GERRIT_GLOBAL_MODULE_SRC + TESTING_SRC,
+ exclude = CONSTANTS_SRC + GERRIT_GLOBAL_MODULE_SRC + TESTING_SRC + PROJECT_UTIL_SRC,
),
resource_strip_prefix = "resources",
resources = ["//resources/com/google/gerrit/server"],
visibility = ["//visibility:public"],
deps = [
":constants",
+ ":project_util",
"//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/common:server",
"//java/com/google/gerrit/entities",
diff --git a/java/com/google/gerrit/server/BranchUtil.java b/java/com/google/gerrit/server/BranchUtil.java
new file mode 100644
index 0000000..78f693d
--- /dev/null
+++ b/java/com/google/gerrit/server/BranchUtil.java
@@ -0,0 +1,47 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server;
+
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import java.io.IOException;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.Repository;
+
+public class BranchUtil {
+ /**
+ * Checks whether the specified branch exists.
+ *
+ * @param repoManager Git repository manager to open the git repository
+ * @param branch the branch for which it should be checked if it exists
+ * @return {@code true} if the specified branch exists or if {@code HEAD} points to this branch,
+ * otherwise {@code false}
+ * @throws RepositoryNotFoundException the repository of the branch's project does not exist.
+ * @throws IOException error while retrieving the branch from the repository.
+ */
+ public static boolean branchExists(final GitRepositoryManager repoManager, BranchNameKey branch)
+ throws RepositoryNotFoundException, IOException {
+ try (Repository repo = repoManager.openRepository(branch.project())) {
+ boolean exists = repo.getRefDatabase().exactRef(branch.branch()) != null;
+ if (!exists) {
+ exists =
+ repo.getFullBranch().equals(branch.branch())
+ || RefNames.REFS_CONFIG.equals(branch.branch());
+ }
+ return exists;
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/ProjectUtil.java b/java/com/google/gerrit/server/ProjectUtil.java
index fa056b3..e87c8bf 100644
--- a/java/com/google/gerrit/server/ProjectUtil.java
+++ b/java/com/google/gerrit/server/ProjectUtil.java
@@ -14,38 +14,7 @@
package com.google.gerrit.server;
-import com.google.gerrit.entities.BranchNameKey;
-import com.google.gerrit.entities.RefNames;
-import com.google.gerrit.server.git.GitRepositoryManager;
-import java.io.IOException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.Repository;
-
public class ProjectUtil {
-
- /**
- * Checks whether the specified branch exists.
- *
- * @param repoManager Git repository manager to open the git repository
- * @param branch the branch for which it should be checked if it exists
- * @return {@code true} if the specified branch exists or if {@code HEAD} points to this branch,
- * otherwise {@code false}
- * @throws RepositoryNotFoundException the repository of the branch's project does not exist.
- * @throws IOException error while retrieving the branch from the repository.
- */
- public static boolean branchExists(final GitRepositoryManager repoManager, BranchNameKey branch)
- throws RepositoryNotFoundException, IOException {
- try (Repository repo = repoManager.openRepository(branch.project())) {
- boolean exists = repo.getRefDatabase().exactRef(branch.branch()) != null;
- if (!exists) {
- exists =
- repo.getFullBranch().equals(branch.branch())
- || RefNames.REFS_CONFIG.equals(branch.branch());
- }
- return exists;
- }
- }
-
public static String sanitizeProjectName(String name) {
name = stripGitSuffix(name);
name = stripTrailingSlash(name);
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 1818b1b..2020d2f 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -434,16 +434,16 @@
@Override
public Stream<AccountState> search(String input, CurrentUser asUser) throws IOException {
- boolean canSeeSecondaryEmails = false;
+ boolean canViewSecondaryEmails = false;
try {
- if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
- canSeeSecondaryEmails = true;
+ if (permissionBackend.user(asUser).test(GlobalPermission.VIEW_SECONDARY_EMAILS)) {
+ canViewSecondaryEmails = true;
}
} catch (PermissionBackendException e) {
// remains false
}
- if (canSeeSecondaryEmails) {
+ if (canViewSecondaryEmails) {
return toAccountStates(emails.getAccountFor(input));
}
@@ -549,15 +549,15 @@
// up with a reasonable result list.
// TODO(dborowitz): This doesn't match the documentation; consider whether it's possible to be
// more strict here.
- boolean canSeeSecondaryEmails = false;
+ boolean canViewSecondaryEmails = false;
try {
- if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
- canSeeSecondaryEmails = true;
+ if (permissionBackend.user(asUser).test(GlobalPermission.VIEW_SECONDARY_EMAILS)) {
+ canViewSecondaryEmails = true;
}
} catch (PermissionBackendException e) {
// remains false
}
- return accountQueryProvider.get().byDefault(input, canSeeSecondaryEmails).stream();
+ return accountQueryProvider.get().byDefault(input, canViewSecondaryEmails).stream();
}
@Override
diff --git a/java/com/google/gerrit/server/account/InternalAccountDirectory.java b/java/com/google/gerrit/server/account/InternalAccountDirectory.java
index b895834..64b8ec0 100644
--- a/java/com/google/gerrit/server/account/InternalAccountDirectory.java
+++ b/java/com/google/gerrit/server/account/InternalAccountDirectory.java
@@ -96,12 +96,12 @@
return;
}
- boolean canModifyAccount = false;
+ boolean canViewSecondaryEmails = false;
Account.Id currentUserId = null;
if (self.get().isIdentifiedUser()) {
currentUserId = self.get().getAccountId();
- if (permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
- canModifyAccount = true;
+ if (permissionBackend.currentUser().test(GlobalPermission.VIEW_SECONDARY_EMAILS)) {
+ canViewSecondaryEmails = true;
}
}
@@ -115,7 +115,7 @@
if (state != null) {
if (!options.contains(FillOptions.SECONDARY_EMAILS)
|| Objects.equals(currentUserId, state.account().id())
- || canModifyAccount) {
+ || canViewSecondaryEmails) {
fill(info, accountStates.get(id), options);
} else {
// user is not allowed to see secondary emails
diff --git a/java/com/google/gerrit/server/config/CapabilityConstants.java b/java/com/google/gerrit/server/config/CapabilityConstants.java
index 59819bb..1f799c6 100644
--- a/java/com/google/gerrit/server/config/CapabilityConstants.java
+++ b/java/com/google/gerrit/server/config/CapabilityConstants.java
@@ -45,4 +45,5 @@
public String viewConnections;
public String viewPlugins;
public String viewQueue;
+ public String viewSecondaryEmails;
}
diff --git a/java/com/google/gerrit/server/index/account/AccountField.java b/java/com/google/gerrit/server/index/account/AccountField.java
index cc86ffc..e675003 100644
--- a/java/com/google/gerrit/server/index/account/AccountField.java
+++ b/java/com/google/gerrit/server/index/account/AccountField.java
@@ -66,7 +66,8 @@
* External IDs.
*
* <p>This field includes secondary emails. Use this field only if the current user is allowed to
- * see secondary emails (requires the {@link GlobalCapability#MODIFY_ACCOUNT} capability).
+ * see secondary emails (requires the {@link GlobalCapability#VIEW_SECONDARY_EMAILS} capability or
+ * the {@link GlobalCapability#MODIFY_ACCOUNT} capability).
*/
public static final IndexedField<AccountState, Iterable<String>> EXTERNAL_ID_FIELD =
IndexedField.<AccountState>iterableStringBuilder("ExternalId")
@@ -80,8 +81,9 @@
* Fuzzy prefix match on name and email parts.
*
* <p>This field includes parts from the secondary emails. Use this field only if the current user
- * is allowed to see secondary emails (requires the {@link GlobalCapability#MODIFY_ACCOUNT}
- * capability).
+ * is allowed to see secondary emails (requires requires the {@link
+ * GlobalCapability#VIEW_SECONDARY_EMAILS} capability or the {@link
+ * GlobalCapability#MODIFY_ACCOUNT} capability).
*
* <p>Use the {@link AccountField#NAME_PART_NO_SECONDARY_EMAIL_SPEC} if the current user can't see
* secondary emails.
diff --git a/java/com/google/gerrit/server/mail/send/BranchEmailUtils.java b/java/com/google/gerrit/server/mail/send/BranchEmailUtils.java
new file mode 100644
index 0000000..acba4ea
--- /dev/null
+++ b/java/com/google/gerrit/server/mail/send/BranchEmailUtils.java
@@ -0,0 +1,98 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.mail.send;
+
+import com.google.common.collect.Iterables;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.mail.MailHeader;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/** Contains utils for email notification related to the events on project+branch. */
+class BranchEmailUtils {
+
+ /** Set a reasonable list id so that filters can be used to sort messages. */
+ static void setListIdHeader(OutgoingEmail email, BranchNameKey branch) {
+ email.setHeader(
+ "List-Id",
+ "<gerrit-" + branch.project().get().replace('/', '-') + "." + email.getGerritHost() + ">");
+ if (email.getSettingsUrl() != null) {
+ email.setHeader("List-Unsubscribe", "<" + email.getSettingsUrl() + ">");
+ }
+ }
+
+ /** Add branch information to soy template params. */
+ static void addBranchData(OutgoingEmail email, EmailArguments args, BranchNameKey branch) {
+ Map<String, Object> soyContext = email.getSoyContext();
+ Map<String, Object> soyContextEmailData = email.getSoyContextEmailData();
+
+ String projectName = branch.project().get();
+ soyContext.put("projectName", projectName);
+ // shortProjectName is the project name with the path abbreviated.
+ soyContext.put("shortProjectName", getShortProjectName(projectName));
+
+ // instanceAndProjectName is the instance's name followed by the abbreviated project path
+ soyContext.put(
+ "instanceAndProjectName",
+ getInstanceAndProjectName(args.instanceNameProvider.get(), projectName));
+ soyContext.put("addInstanceNameInSubject", args.addInstanceNameInSubject);
+
+ soyContextEmailData.put("sshHost", getSshHost(email.getGerritHost(), args.sshAddresses));
+
+ Map<String, String> branchData = new HashMap<>();
+ branchData.put("shortName", branch.shortName());
+ soyContext.put("branch", branchData);
+
+ email.addFooter(MailHeader.PROJECT.withDelimiter() + branch.project().get());
+ email.addFooter(MailHeader.BRANCH.withDelimiter() + branch.shortName());
+ }
+
+ @Nullable
+ private static String getSshHost(String gerritHost, List<String> sshAddresses) {
+ String host = Iterables.getFirst(sshAddresses, null);
+ if (host == null) {
+ return null;
+ }
+ if (host.startsWith("*:")) {
+ return gerritHost + host.substring(1);
+ }
+ return host;
+ }
+
+ /** Shortens project/repo name to only show part after the last '/'. */
+ static String getShortProjectName(String projectName) {
+ int lastIndexSlash = projectName.lastIndexOf('/');
+ if (lastIndexSlash == 0) {
+ return projectName.substring(1); // Remove the first slash
+ }
+ if (lastIndexSlash == -1) { // No slash in the project name
+ return projectName;
+ }
+
+ return "..." + projectName.substring(lastIndexSlash + 1);
+ }
+
+ /** Returns a project/repo name that includes instance as prefix. */
+ static String getInstanceAndProjectName(String instanceName, String projectName) {
+ if (instanceName == null || instanceName.isEmpty()) {
+ return getShortProjectName(projectName);
+ }
+ // Extract the project name (everything after the last slash) and prepends it with gerrit's
+ // instance name
+ return instanceName + "/" + projectName.substring(projectName.lastIndexOf('/') + 1);
+ }
+}
diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
index ec81cf6..714ddda 100644
--- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java
+++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -26,6 +26,7 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Address;
import com.google.gerrit.entities.AttentionSetUpdate;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeSizeBucket;
import com.google.gerrit.entities.NotifyConfig.NotifyType;
@@ -43,6 +44,7 @@
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.mail.send.ProjectWatch.Watchers;
+import com.google.gerrit.server.mail.send.ProjectWatch.Watchers.WatcherList;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOptions;
@@ -78,7 +80,7 @@
import org.eclipse.jgit.util.TemporaryBuffer;
/** Sends an email to one or more interested parties. */
-public abstract class ChangeEmail extends NotificationEmail {
+public abstract class ChangeEmail extends OutgoingEmail {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -100,19 +102,23 @@
protected PatchSetInfo patchSetInfo;
protected String changeMessage;
protected Instant timestamp;
+ protected BranchNameKey branch;
protected ProjectState projectState;
- protected Set<Account.Id> authors;
- protected boolean emailOnlyAuthors;
+ private Set<Account.Id> authors;
+ private boolean emailOnlyAuthors;
protected boolean emailOnlyAttentionSetIfEnabled;
+ // Watchers ignore attention set rules.
+ protected Set<Account.Id> watchers = new HashSet<>();
protected ChangeEmail(EmailArguments args, String messageClass, ChangeData changeData) {
- super(args, messageClass, changeData.change().getDest());
+ super(args, messageClass);
this.changeData = changeData;
change = changeData.change();
emailOnlyAuthors = false;
emailOnlyAttentionSetIfEnabled = true;
currentAttentionSet = getAttentionSet();
+ branch = changeData.change().getDest();
}
@Override
@@ -193,7 +199,6 @@
}
}
}
- authors = getAuthors();
try {
stars = changeData.stars();
@@ -202,6 +207,7 @@
}
super.init();
+ BranchEmailUtils.setListIdHeader(this, branch);
if (timestamp != null) {
setHeader(FieldName.DATE, timestamp);
}
@@ -377,7 +383,7 @@
/** TO or CC all vested parties (change owner, patch set uploader, author). */
protected void addAuthors(RecipientType rt) {
- for (Account.Id id : authors) {
+ for (Account.Id id : getAuthors()) {
addByAccountId(rt, id);
}
}
@@ -395,8 +401,38 @@
}
}
- @Override
- protected final Watchers getWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig) {
+ /** Include users and groups that want notification of events. */
+ protected void includeWatchers(NotifyType type) {
+ includeWatchers(type, true);
+ }
+
+ /** Include users and groups that want notification of events. */
+ protected void includeWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig) {
+ try {
+ Watchers matching = getWatchers(type, includeWatchersFromNotifyConfig);
+ addWatchers(RecipientType.TO, matching.to);
+ addWatchers(RecipientType.CC, matching.cc);
+ addWatchers(RecipientType.BCC, matching.bcc);
+ } catch (StorageException err) {
+ // Just don't CC everyone. Better to send a partial message to those
+ // we already have queued up then to fail deliver entirely to people
+ // who have a lower interest in the change.
+ logger.atWarning().withCause(err).log("Cannot BCC watchers for %s", type);
+ }
+ }
+
+ /** Add users or email addresses to the TO, CC, or BCC list. */
+ private void addWatchers(RecipientType type, WatcherList watcherList) {
+ watchers.addAll(watcherList.accounts);
+ for (Account.Id user : watcherList.accounts) {
+ addByAccountId(type, user);
+ }
+ for (Address addr : watcherList.emails) {
+ addByEmail(type, addr);
+ }
+ }
+
+ private final Watchers getWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig) {
if (!NotifyHandling.ALL.equals(notify.handling())) {
return new Watchers();
}
@@ -438,38 +474,14 @@
}
@Override
- protected void addByAccountId(RecipientType rt, Account.Id to) {
- addRecipient(rt, to, /* isWatcher= */ false);
- }
-
- /** This bypasses the EmailStrategy.ATTENTION_SET_ONLY strategy when adding the recipient. */
- @Override
- protected void addWatcher(RecipientType rt, Account.Id to) {
- addRecipient(rt, to, /* isWatcher= */ true);
- }
-
- private void addRecipient(RecipientType rt, Account.Id to, boolean isWatcher) {
- if (!isWatcher) {
- Optional<AccountState> accountState = args.accountCache.get(to);
- if (emailOnlyAttentionSetIfEnabled
- && accountState.isPresent()
- && accountState.get().generalPreferences().getEmailStrategy()
- == EmailStrategy.ATTENTION_SET_ONLY
- && !currentAttentionSet.contains(to)) {
- return;
- }
- }
- if (emailOnlyAuthors && !authors.contains(to)) {
- return;
- }
- super.addByAccountId(rt, to);
- }
-
- @Override
protected boolean isRecipientAllowed(Address addr) throws PermissionBackendException {
if (!projectState.statePermitsRead()) {
return false;
}
+ if (emailOnlyAuthors) {
+ return false;
+ }
+
return args.permissionBackend
.user(args.anonymousUser.get())
.change(changeData)
@@ -481,11 +493,28 @@
if (!projectState.statePermitsRead()) {
return false;
}
+ if (emailOnlyAuthors && !authors.contains(to)) {
+ return false;
+ }
+ if (!watchers.contains(to)) {
+ Optional<AccountState> accountState = args.accountCache.get(to);
+ if (emailOnlyAttentionSetIfEnabled
+ && accountState.isPresent()
+ && accountState.get().generalPreferences().getEmailStrategy()
+ == EmailStrategy.ATTENTION_SET_ONLY
+ && !currentAttentionSet.contains(to)) {
+ return false;
+ }
+ }
+
return args.permissionBackend.absentUser(to).change(changeData).test(ChangePermission.READ);
}
- /** Find all users who are authors of any part of this change. */
+ /** Lazily finds all users who are authors of any part of this change. */
protected Set<Account.Id> getAuthors() {
+ if (this.authors != null) {
+ return this.authors;
+ }
Set<Account.Id> authors = new HashSet<>();
switch (notify.handling()) {
@@ -511,12 +540,13 @@
break;
}
- return authors;
+ return this.authors = authors;
}
@Override
protected void setupSoyContext() {
super.setupSoyContext();
+ BranchEmailUtils.addBranchData(this, args, branch);
soyContext.put("changeId", change.getKey().get());
soyContext.put("coverLetter", getCoverLetter());
diff --git a/java/com/google/gerrit/server/mail/send/MergedSender.java b/java/com/google/gerrit/server/mail/send/MergedSender.java
index 44b6efa..ce2e3dc 100644
--- a/java/com/google/gerrit/server/mail/send/MergedSender.java
+++ b/java/com/google/gerrit/server/mail/send/MergedSender.java
@@ -56,6 +56,8 @@
super(args, "merged", newChangeData(args, project, changeId));
labelTypes = changeData.getLabelTypes();
this.stickyApprovalDiff = stickyApprovalDiff;
+ // We want to send the submit email even if the "send only when in attention set" is enabled.
+ emailOnlyAttentionSetIfEnabled = false;
}
@Override
@@ -76,9 +78,6 @@
@Override
protected void init() throws EmailException {
- // We want to send the submit email even if the "send only when in attention set" is enabled.
- emailOnlyAttentionSetIfEnabled = false;
-
super.init();
ccAllApprovals();
diff --git a/java/com/google/gerrit/server/mail/send/NotificationEmail.java b/java/com/google/gerrit/server/mail/send/NotificationEmail.java
deleted file mode 100644
index c943724..0000000
--- a/java/com/google/gerrit/server/mail/send/NotificationEmail.java
+++ /dev/null
@@ -1,155 +0,0 @@
-// Copyright (C) 2012 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.mail.send;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.Iterables;
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.Address;
-import com.google.gerrit.entities.BranchNameKey;
-import com.google.gerrit.entities.NotifyConfig.NotifyType;
-import com.google.gerrit.exceptions.EmailException;
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.api.changes.RecipientType;
-import com.google.gerrit.mail.MailHeader;
-import com.google.gerrit.server.mail.send.ProjectWatch.Watchers;
-import com.google.gerrit.server.mail.send.ProjectWatch.Watchers.WatcherList;
-import java.util.HashMap;
-import java.util.Map;
-
-/** Common class for notifications that are related to a project and branch */
-public abstract class NotificationEmail extends OutgoingEmail {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- protected BranchNameKey branch;
-
- protected NotificationEmail(EmailArguments args, String messageClass, BranchNameKey branch) {
- super(args, messageClass);
- this.branch = branch;
- }
-
- @Override
- protected void init() throws EmailException {
- super.init();
- setListIdHeader();
- }
-
- private void setListIdHeader() {
- // Set a reasonable list id so that filters can be used to sort messages
- setHeader(
- "List-Id",
- "<gerrit-" + branch.project().get().replace('/', '-') + "." + getGerritHost() + ">");
- if (getSettingsUrl() != null) {
- setHeader("List-Unsubscribe", "<" + getSettingsUrl() + ">");
- }
- }
-
- /** Include users and groups that want notification of events. */
- protected void includeWatchers(NotifyType type) {
- includeWatchers(type, true);
- }
-
- /** Include users and groups that want notification of events. */
- protected void includeWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig) {
- try {
- Watchers matching = getWatchers(type, includeWatchersFromNotifyConfig);
- addWatchers(RecipientType.TO, matching.to);
- addWatchers(RecipientType.CC, matching.cc);
- addWatchers(RecipientType.BCC, matching.bcc);
- } catch (StorageException err) {
- // Just don't CC everyone. Better to send a partial message to those
- // we already have queued up then to fail deliver entirely to people
- // who have a lower interest in the change.
- logger.atWarning().withCause(err).log("Cannot BCC watchers for %s", type);
- }
- }
-
- /** Returns all watchers that are relevant */
- protected abstract Watchers getWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig);
-
- /** Add users or email addresses to the TO, CC, or BCC list. */
- protected void addWatchers(RecipientType type, WatcherList watcherList) {
- for (Account.Id user : watcherList.accounts) {
- addWatcher(type, user);
- }
- for (Address addr : watcherList.emails) {
- addByEmail(type, addr);
- }
- }
-
- protected abstract void addWatcher(RecipientType type, Account.Id to);
-
- @Nullable
- public String getSshHost() {
- String host = Iterables.getFirst(args.sshAddresses, null);
- if (host == null) {
- return null;
- }
- if (host.startsWith("*:")) {
- return getGerritHost() + host.substring(1);
- }
- return host;
- }
-
- @Override
- protected void setupSoyContext() {
- super.setupSoyContext();
-
- String projectName = branch.project().get();
- soyContext.put("projectName", projectName);
- // shortProjectName is the project name with the path abbreviated.
- soyContext.put("shortProjectName", getShortProjectName(projectName));
-
- // instanceAndProjectName is the instance's name followed by the abbreviated project path
- soyContext.put(
- "instanceAndProjectName",
- getInstanceAndProjectName(args.instanceNameProvider.get(), projectName));
- soyContext.put("addInstanceNameInSubject", args.addInstanceNameInSubject);
-
- soyContextEmailData.put("sshHost", getSshHost());
-
- Map<String, String> branchData = new HashMap<>();
- branchData.put("shortName", branch.shortName());
- soyContext.put("branch", branchData);
-
- footers.add(MailHeader.PROJECT.withDelimiter() + branch.project().get());
- footers.add(MailHeader.BRANCH.withDelimiter() + branch.shortName());
- }
-
- @VisibleForTesting
- protected static String getShortProjectName(String projectName) {
- int lastIndexSlash = projectName.lastIndexOf('/');
- if (lastIndexSlash == 0) {
- return projectName.substring(1); // Remove the first slash
- }
- if (lastIndexSlash == -1) { // No slash in the project name
- return projectName;
- }
-
- return "..." + projectName.substring(lastIndexSlash + 1);
- }
-
- @VisibleForTesting
- protected static String getInstanceAndProjectName(String instanceName, String projectName) {
- if (instanceName == null || instanceName.isEmpty()) {
- return getShortProjectName(projectName);
- }
- // Extract the project name (everything after the last slash) and prepends it with gerrit's
- // instance name
- return instanceName + "/" + projectName.substring(projectName.lastIndexOf('/') + 1);
- }
-}
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index d00c874..aba8f62 100644
--- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -661,6 +661,26 @@
soyContext.put("email", soyContextEmailData);
}
+ /** Mutable map of parameters passed into email templates when rendering. */
+ public Map<String, Object> getSoyContext() {
+ return this.soyContext;
+ }
+
+ // TODO: It's not clear why we need this explicit separation. Probably worth
+ // simplifying.
+ /** Mutable content of `email` parameter in the templates. */
+ public Map<String, Object> getSoyContextEmailData() {
+ return this.soyContextEmailData;
+ }
+
+ /**
+ * Add a line to email footer with additional information. Typically, in the form of {@literal
+ * <key>: <value>}.
+ */
+ public void addFooter(String footer) {
+ footers.add(footer);
+ }
+
private String getInstanceName() {
return args.instanceNameProvider.get();
}
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
index bf4d05a..a4ee052 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
@@ -195,12 +195,17 @@
case MODIFY_ACCOUNT:
case READ_AS:
case STREAM_EVENTS:
+ case VIEW_ACCESS:
case VIEW_ALL_ACCOUNTS:
case VIEW_CONNECTIONS:
case VIEW_PLUGINS:
- case VIEW_ACCESS:
return has(globalPermissionName(perm)) || isAdmin();
+ case VIEW_SECONDARY_EMAILS:
+ return has(globalPermissionName(perm))
+ || has(globalPermissionName(GlobalPermission.MODIFY_ACCOUNT))
+ || isAdmin();
+
case ACCESS_DATABASE:
case RUN_AS:
return has(globalPermissionName(perm));
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
index da24dcd..958de1b 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
@@ -61,6 +61,7 @@
.put(GlobalPermission.VIEW_CONNECTIONS, GlobalCapability.VIEW_CONNECTIONS)
.put(GlobalPermission.VIEW_PLUGINS, GlobalCapability.VIEW_PLUGINS)
.put(GlobalPermission.VIEW_QUEUE, GlobalCapability.VIEW_QUEUE)
+ .put(GlobalPermission.VIEW_SECONDARY_EMAILS, GlobalCapability.VIEW_SECONDARY_EMAILS)
.build();
static {
diff --git a/java/com/google/gerrit/server/permissions/GlobalPermission.java b/java/com/google/gerrit/server/permissions/GlobalPermission.java
index c0b44e5..3429978 100644
--- a/java/com/google/gerrit/server/permissions/GlobalPermission.java
+++ b/java/com/google/gerrit/server/permissions/GlobalPermission.java
@@ -58,7 +58,8 @@
VIEW_CACHES,
VIEW_CONNECTIONS,
VIEW_PLUGINS,
- VIEW_QUEUE;
+ VIEW_QUEUE,
+ VIEW_SECONDARY_EMAILS;
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
diff --git a/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java b/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
index d5c4a97..2f4a923 100644
--- a/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
@@ -151,7 +151,7 @@
@Operator
public Predicate<AccountState> email(String email)
throws PermissionBackendException, QueryParseException {
- if (canSeeSecondaryEmails()) {
+ if (canViewSecondaryEmails()) {
return AccountPredicates.emailIncludingSecondaryEmails(email);
}
@@ -185,7 +185,7 @@
@Operator
public Predicate<AccountState> name(String name)
throws PermissionBackendException, QueryParseException {
- if (canSeeSecondaryEmails()) {
+ if (canViewSecondaryEmails()) {
return AccountPredicates.equalsNameIncludingSecondaryEmails(name);
}
@@ -210,7 +210,7 @@
@Override
protected Predicate<AccountState> defaultField(String query) {
Predicate<AccountState> defaultPredicate =
- AccountPredicates.defaultPredicate(args.schema(), checkedCanSeeSecondaryEmails(), query);
+ AccountPredicates.defaultPredicate(args.schema(), checkedCanViewSecondaryEmails(), query);
if (query.startsWith("cansee:")) {
try {
return cansee(query.substring(7));
@@ -233,13 +233,13 @@
return args.getIdentifiedUser().getAccountId();
}
- private boolean canSeeSecondaryEmails() throws PermissionBackendException, QueryParseException {
- return args.permissionBackend.user(args.getUser()).test(GlobalPermission.MODIFY_ACCOUNT);
+ private boolean canViewSecondaryEmails() throws PermissionBackendException, QueryParseException {
+ return args.permissionBackend.user(args.getUser()).test(GlobalPermission.VIEW_SECONDARY_EMAILS);
}
- private boolean checkedCanSeeSecondaryEmails() {
+ private boolean checkedCanViewSecondaryEmails() {
try {
- return canSeeSecondaryEmails();
+ return canViewSecondaryEmails();
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("Permission check failed");
return false;
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index ef067a1..57b59ef 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -1539,6 +1539,11 @@
}
@Operator
+ public Predicate<ChangeData> a(String who) throws QueryParseException {
+ return author(who);
+ }
+
+ @Operator
public Predicate<ChangeData> author(String who) throws QueryParseException {
return getAuthorOrCommitterPredicate(
who.trim(), ChangePredicates::exactAuthor, ChangePredicates::author);
diff --git a/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java b/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java
index 82d8717..9ee4852 100644
--- a/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java
+++ b/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.server.query.change.EqualsLabelPredicates.type;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@@ -30,6 +31,8 @@
import java.util.Optional;
public class MagicLabelPredicates {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
public static class PostFilterMagicLabelPredicate extends PostFilterPredicate<ChangeData> {
private static class PostFilterMatcher extends Matcher {
public PostFilterMatcher(
@@ -177,6 +180,7 @@
}
public boolean ignoresUploaderApprovals() {
+ logger.atFine().log("account = %d", account.get());
return account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
|| account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID);
}
diff --git a/java/com/google/gerrit/server/restapi/BUILD b/java/com/google/gerrit/server/restapi/BUILD
index dd0ec78d..4ce8c42 100644
--- a/java/com/google/gerrit/server/restapi/BUILD
+++ b/java/com/google/gerrit/server/restapi/BUILD
@@ -21,6 +21,7 @@
"//java/com/google/gerrit/json",
"//java/com/google/gerrit/metrics",
"//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server:project_util",
"//java/com/google/gerrit/server/ioutil",
"//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/util/time",
diff --git a/java/com/google/gerrit/server/restapi/account/GetEmails.java b/java/com/google/gerrit/server/restapi/account/GetEmails.java
index 4d70eb9..a518532 100644
--- a/java/com/google/gerrit/server/restapi/account/GetEmails.java
+++ b/java/com/google/gerrit/server/restapi/account/GetEmails.java
@@ -52,7 +52,7 @@
public Response<List<EmailInfo>> apply(AccountResource rsrc)
throws AuthException, PermissionBackendException {
if (!self.get().hasSameAccountId(rsrc.getUser())) {
- permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
+ permissionBackend.currentUser().check(GlobalPermission.VIEW_SECONDARY_EMAILS);
}
return Response.ok(
rsrc.getUser().getEmailAddresses().stream()
diff --git a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
index 79737f3..9fc0c42 100644
--- a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
+++ b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
@@ -173,7 +173,7 @@
}
boolean modifyAccountCapabilityChecked = false;
if (options.contains(ListAccountsOption.ALL_EMAILS)) {
- permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
+ permissionBackend.currentUser().check(GlobalPermission.VIEW_SECONDARY_EMAILS);
modifyAccountCapabilityChecked = true;
fillOptions.add(FillOptions.EMAIL);
fillOptions.add(FillOptions.SECONDARY_EMAILS);
@@ -185,7 +185,7 @@
if (modifyAccountCapabilityChecked) {
fillOptions.add(FillOptions.SECONDARY_EMAILS);
} else {
- if (permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
+ if (permissionBackend.currentUser().test(GlobalPermission.VIEW_SECONDARY_EMAILS)) {
fillOptions.add(FillOptions.SECONDARY_EMAILS);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/ApplyPatchUtil.java b/java/com/google/gerrit/server/restapi/change/ApplyPatchUtil.java
index 74033c1..78c4fbe 100644
--- a/java/com/google/gerrit/server/restapi/change/ApplyPatchUtil.java
+++ b/java/com/google/gerrit/server/restapi/change/ApplyPatchUtil.java
@@ -31,8 +31,7 @@
import java.util.Optional;
import org.apache.commons.codec.binary.Base64;
import org.apache.commons.lang3.StringUtils;
-import org.eclipse.jgit.api.errors.PatchApplyException;
-import org.eclipse.jgit.api.errors.PatchFormatException;
+import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
@@ -67,10 +66,9 @@
PatchApplier applier = new PatchApplier(repo, tip, oi);
PatchApplier.Result applyResult = applier.applyPatch(patchStream);
return applyResult.getTreeId();
- } catch (PatchFormatException e) {
- throw new BadRequestException("Invalid patch format: " + input.patch, e);
- } catch (PatchApplyException e) {
- throw RestApiException.wrap("Cannot apply patch: " + input.patch, e);
+ } catch (GitAPIException e) {
+ // TODO(nitzan) - write better error handling here.
+ throw new BadRequestException("Cannot apply patch: " + input.patch, e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 3cb1870..e47515a 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -127,7 +127,7 @@
bu.addOp(change.getId(), rebaseOp);
bu.execute();
- rebaseMetrics.countRebase(input.onBehalfOfUploader);
+ rebaseMetrics.countRebase(input.onBehalfOfUploader, input.allowConflicts);
ChangeInfo changeInfo = json.create(OPTIONS).format(change.getProject(), change.getId());
changeInfo.containsGitConflicts =
@@ -142,9 +142,7 @@
UiAction.Description description =
new UiAction.Description()
.setLabel("Rebase")
- .setTitle(
- "Rebase onto tip of branch or parent change. Makes you the uploader of this "
- + "change which can affect validity of approvals.")
+ .setTitle("Rebase onto tip of branch or parent change.")
.setVisible(false);
Change change = rsrc.getChange();
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 32b5b11..bbac318 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -208,7 +208,7 @@
}
}
- rebaseMetrics.countRebaseChain(input.onBehalfOfUploader);
+ rebaseMetrics.countRebaseChain(input.onBehalfOfUploader, input.allowConflicts);
RebaseChainInfo res = new RebaseChainInfo();
res.rebasedChanges = new ArrayList<>();
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java b/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java
index 114a112..d6577ea 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server.restapi.change;
-import com.google.gerrit.metrics.Counter2;
+import com.google.gerrit.metrics.Counter3;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Field;
import com.google.gerrit.metrics.MetricMaker;
@@ -24,7 +24,7 @@
/** Metrics for the rebase REST endpoints ({@link Rebase} and {@link RebaseChain}). */
@Singleton
public class RebaseMetrics {
- private final Counter2<Boolean, Boolean> countRebases;
+ private final Counter3<Boolean, Boolean, Boolean> countRebases;
@Inject
public RebaseMetrics(MetricMaker metricMaker) {
@@ -37,18 +37,25 @@
.build(),
Field.ofBoolean("rebase_chain", (metadataBuilder, isRebaseChain) -> {})
.description("Whether a chain was rebased.")
+ .build(),
+ Field.ofBoolean("allow_conflicts", (metadataBuilder, allow_conflicts) -> {})
+ .description("Whether the rebase was done with allowing conflicts.")
.build());
}
- public void countRebase(boolean isOnBehalfOfUploader) {
- countRebase(isOnBehalfOfUploader, /* isRebaseChain= */ false);
+ public void countRebase(boolean isOnBehalfOfUploader, boolean allowConflicts) {
+ countRebase(isOnBehalfOfUploader, /* isRebaseChain= */ false, allowConflicts);
}
- public void countRebaseChain(boolean isOnBehalfOfUploader) {
- countRebase(isOnBehalfOfUploader, /* isRebaseChain= */ true);
+ public void countRebaseChain(boolean isOnBehalfOfUploader, boolean allowConflicts) {
+ countRebase(isOnBehalfOfUploader, /* isRebaseChain= */ true, allowConflicts);
}
- private void countRebase(boolean isOnBehalfOfUploader, boolean isRebaseChain) {
- countRebases.increment(/* field1= */ isOnBehalfOfUploader, /* field2= */ isRebaseChain);
+ private void countRebase(
+ boolean isOnBehalfOfUploader, boolean isRebaseChain, boolean allowConflicts) {
+ countRebases.increment(
+ /* field1= */ isOnBehalfOfUploader,
+ /* field2= */ isRebaseChain,
+ /* field3= */ allowConflicts);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index 5fc4f41..b1f1da5 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -43,11 +43,11 @@
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.webui.UiAction;
+import com.google.gerrit.server.BranchUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.ProjectUtil;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeResource;
@@ -198,7 +198,7 @@
Change change = rsrc.getChange();
if (!change.isNew()) {
throw new ResourceConflictException("change is " + ChangeUtil.status(change));
- } else if (!ProjectUtil.branchExists(repoManager, change.getDest())) {
+ } else if (!BranchUtil.branchExists(repoManager, change.getDest())) {
throw new ResourceConflictException(
String.format("destination branch \"%s\" not found.", change.getDest().branch()));
} else if (!rsrc.getPatchSet().id().equals(change.currentPatchSetId())) {
diff --git a/java/com/google/gerrit/server/submit/MergeMetrics.java b/java/com/google/gerrit/server/submit/MergeMetrics.java
index e9832fd..4c11925 100644
--- a/java/com/google/gerrit/server/submit/MergeMetrics.java
+++ b/java/com/google/gerrit/server/submit/MergeMetrics.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.submit;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.SubmitRequirement;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -28,6 +29,8 @@
/** Metrics are recorded when a change is merged (aka submitted). */
public class MergeMetrics {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private final Provider<SubmitRequirementChangeQueryBuilder> submitRequirementChangequeryBuilder;
// TODO: This metric is for measuring the impact of allowing users to rebase changes on behalf of
@@ -81,12 +84,19 @@
// impersonated. Impersonating the uploader is only allowed on rebase by rebasing on behalf of
// the uploader. Hence if the current patch set has different accounts as uploader and real
// uploader we can assume that it was created by rebase on behalf of the uploader.
- return !cd.currentPatchSet().uploader().equals(cd.currentPatchSet().realUploader());
+ boolean isRebaseOnBehalfOfUploader =
+ !cd.currentPatchSet().uploader().equals(cd.currentPatchSet().realUploader());
+ logger.atFine().log("isRebaseOnBehalfOfUploader = %s", isRebaseOnBehalfOfUploader);
+ return isRebaseOnBehalfOfUploader;
}
private boolean hasCodeReviewApprovalOfRealUploader(ChangeData cd) {
- return cd.currentApprovals().stream()
- .anyMatch(psa -> psa.accountId().equals(cd.currentPatchSet().realUploader()));
+ boolean hasCodeReviewApprovalOfRealUploader =
+ cd.currentApprovals().stream()
+ .anyMatch(psa -> psa.accountId().equals(cd.currentPatchSet().realUploader()));
+ logger.atFine().log(
+ "hasCodeReviewApprovalOfRealUploader = %s", hasCodeReviewApprovalOfRealUploader);
+ return hasCodeReviewApprovalOfRealUploader;
}
private boolean ignoresCodeReviewApprovalsOfUploader(ChangeData cd) {
@@ -96,15 +106,27 @@
submitRequirementChangequeryBuilder
.get()
.parse(submitRequirement.submittabilityExpression().expressionString());
- return ignoresCodeReviewApprovalsOfUploader(predicate);
+ boolean ignoresCodeReviewApprovalsOfUploader =
+ ignoresCodeReviewApprovalsOfUploader(predicate);
+ logger.atFine().log(
+ "ignoresCodeReviewApprovalsOfUploader = %s", ignoresCodeReviewApprovalsOfUploader);
+ if (ignoresCodeReviewApprovalsOfUploader) {
+ return true;
+ }
} catch (QueryParseException e) {
- return false;
+ logger.atFine().log(
+ "Failed to parse submit requirement expression %s: %s",
+ submitRequirement.submittabilityExpression().expressionString(), e.getMessage());
+ // ignore and inspect the next submit requirement
}
}
return false;
}
private boolean ignoresCodeReviewApprovalsOfUploader(Predicate<ChangeData> predicate) {
+ logger.atFine().log(
+ "predicate = (%s) %s (child count = %d)",
+ predicate.getClass().getName(), predicate, predicate.getChildCount());
if (predicate.getChildCount() == 0) {
// Submit requirements may require a Code-Review approval but ignore approvals by the
// uploader. This is done by using a label predicate with 'user=non_uploader' or
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index a9a0b70..dd04200 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -991,14 +991,15 @@
}
@Test
- public void cannotGetEmailsOfOtherAccountWithoutModifyAccount() throws Exception {
+ public void cannotGetEmailsOfOtherAccountWithoutViewSecondaryEmailsAndWithoutModifyAccount()
+ throws Exception {
String email = "preferred2@example.com";
TestAccount foo = accountCreator.create(name("foo"), email, "Foo", null);
requestScopeOperations.setApiUser(user.id());
AuthException thrown =
assertThrows(AuthException.class, () -> gApi.accounts().id(foo.id().get()).getEmails());
- assertThat(thrown).hasMessageThat().contains("modify account not permitted");
+ assertThat(thrown).hasMessageThat().contains("view secondary emails not permitted");
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java b/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
index 7b7e145a..fc95425 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
@@ -49,7 +49,6 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.inject.Inject;
-import org.eclipse.jgit.api.errors.PatchApplyException;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.util.Base64;
import org.junit.Test;
@@ -243,18 +242,6 @@
}
@Test
- public void applyPatchWithConflict_fails() throws Exception {
- initBaseWithFile(MODIFIED_FILE_NAME, "Unexpected base content");
- ApplyPatchPatchSetInput in = buildInput(MODIFIED_FILE_DIFF);
-
- Throwable error = assertThrows(RestApiException.class, () -> applyPatch(in));
-
- assertThat(error).hasMessageThat().contains("Cannot apply patch");
- assertThat(error).hasCauseThat().isInstanceOf(PatchApplyException.class);
- assertThat(error).hasCauseThat().hasMessageThat().contains("Cannot apply: HunkHeader");
- }
-
- @Test
public void applyPatchWithoutAddPatchSetPermissions_fails() throws Exception {
initDestBranch();
ApplyPatchPatchSetInput in = buildInput(ADDED_FILE_DIFF);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java
index de73c00..1790133 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java
@@ -54,6 +54,28 @@
}
@Test
+ public void projectChangeNumberReturnsChangeWhenProjectEndsWithSlash() throws Exception {
+ Project.NameKey p = projectOperations.newProject().create();
+ ChangeInfo ci = gApi.changes().create(new ChangeInput(p.get(), "master", "msg")).get();
+
+ ChangeInfo changeInfo = gApi.changes().id(p.get() + "/", ci._number).get();
+
+ assertThat(changeInfo.changeId).isEqualTo(ci.changeId);
+ assertThat(changeInfo.project).isEqualTo(p.get());
+ }
+
+ @Test
+ public void projectChangeNumberReturnsChangeWhenProjectEndsWithDotGit() throws Exception {
+ Project.NameKey p = projectOperations.newProject().create();
+ ChangeInfo ci = gApi.changes().create(new ChangeInput(p.get(), "master", "msg")).get();
+
+ ChangeInfo changeInfo = gApi.changes().id(p.get() + ".git", ci._number).get();
+
+ assertThat(changeInfo.changeId).isEqualTo(ci.changeId);
+ assertThat(changeInfo.project).isEqualTo(p.get());
+ }
+
+ @Test
public void wrongProjectInProjectChangeNumberReturnsNotFound() throws Exception {
ResourceNotFoundException thrown =
assertThrows(
diff --git a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
index 43c8684..3f3ad37 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
@@ -549,6 +549,19 @@
@Test
public void changesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability()
throws Exception {
+ testCangesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability(
+ GlobalCapability.MODIFY_ACCOUNT);
+ }
+
+ @Test
+ public void changesFoundWhenQueryingBySecondaryEmailWithViewSecondaryEmailsCapability()
+ throws Exception {
+ testCangesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability(
+ GlobalCapability.VIEW_SECONDARY_EMAILS);
+ }
+
+ private void testCangesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability(
+ String globalCapability) throws Exception {
String secondaryOwnerEmail = "owner-secondary@example.com";
Account.Id owner =
accountOperations
@@ -574,7 +587,7 @@
projectOperations
.allProjectsForUpdate()
- .add(allowCapability(GlobalCapability.MODIFY_ACCOUNT).group(REGISTERED_USERS))
+ .add(allowCapability(globalCapability).group(REGISTERED_USERS))
.update();
requestScopeOperations.setApiUser(user.id());
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseChainOnBehalfOfUploaderIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseChainOnBehalfOfUploaderIT.java
index 13f9904..e4a6247 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseChainOnBehalfOfUploaderIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseChainOnBehalfOfUploaderIT.java
@@ -1234,11 +1234,11 @@
RebaseInput rebaseInput = new RebaseInput();
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased2.get()).rebaseChain(rebaseInput);
- // field1 is on_behalf_of_uploader, field2 is rebase_chain
- assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(1);
- assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(0);
- assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(0);
- assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(0);
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain, field3 is allow_conflicts
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true, false)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true, false)).isEqualTo(0);
}
private void assertRebase(
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
index dee8d1f..4e95032 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
@@ -665,10 +665,15 @@
extensionRegistry.newRegistration().add(wipStateChangedListener)) {
RebaseInput rebaseInput = new RebaseInput();
rebaseInput.allowConflicts = true;
+ testMetricMaker.reset();
ChangeInfo changeInfo =
gApi.changes().id(changeId).revision(patchSet.name()).rebaseAsInfo(rebaseInput);
assertThat(changeInfo.containsGitConflicts).isTrue();
assertThat(changeInfo.workInProgress).isTrue();
+
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain, field3 is allow_conflicts
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false, true))
+ .isEqualTo(1);
}
assertThat(wipStateChangedListener.invoked).isTrue();
assertThat(wipStateChangedListener.wip).isTrue();
@@ -790,11 +795,12 @@
// Rebase the second change
testMetricMaker.reset();
rebaseCallWithInput.call(r2.getChangeId(), new RebaseInput());
- // field1 is on_behalf_of_uploader, field2 is rebase_chain
- assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(1);
- assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(0);
- assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
- assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(0);
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain, field3 is allow_conflicts
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false, false))
+ .isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true, false)).isEqualTo(0);
}
@Test
@@ -1249,11 +1255,12 @@
testMetricMaker.reset();
verifyRebaseChainResponse(
gApi.changes().id(r4.getChangeId()).rebaseChain(), false, r2, r3, r4);
- // field1 is on_behalf_of_uploader, field2 is rebase_chain
- assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(1);
- assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(0);
- assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
- assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(0);
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain, field3 is allow_conflicts
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true, false)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false, false))
+ .isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false, false)).isEqualTo(0);
}
private void verifyRebaseChainResponse(
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
index 8565ced..82d22b5 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
@@ -21,6 +21,8 @@
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.server.notedb.ChangeNoteFooters.FOOTER_REAL_USER;
+import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
+import static com.google.gerrit.server.project.testing.TestLabels.value;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.Iterables;
@@ -37,6 +39,8 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.RefNames;
@@ -1025,11 +1029,25 @@
@Test
public void testSubmittedWithRebaserApprovalMetric() throws Exception {
+ allowVotingOnCodeReviewToAllUsers();
+
+ createVerifiedLabel();
+ allowVotingOnVerifiedToAllUsers();
+
// Require a Code-Review approval from a non-uploader for submit.
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.upsertSubmitRequirement(
SubmitRequirement.builder()
+ .setName(TestLabels.verified().getName())
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create(
+ String.format("label:%s=MAX", TestLabels.verified().getName())))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ u.getConfig()
+ .upsertSubmitRequirement(
+ SubmitRequirement.builder()
.setName(TestLabels.codeReview().getName())
.setSubmittabilityExpression(
SubmitRequirementExpression.create(
@@ -1056,7 +1074,10 @@
// Approve and submit the change that will be the new base for the change that will be rebased.
requestScopeOperations.setApiUser(approver);
- gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes()
+ .id(changeToBeTheNewBase.get())
+ .current()
+ .review(ReviewInput.approve().label(TestLabels.verified().getName(), 1));
testMetricMaker.reset();
gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
assertThat(testMetricMaker.getCount("change/submitted_with_rebaser_approval")).isEqualTo(0);
@@ -1068,8 +1089,10 @@
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
// Approve the change as the rebaser.
- allowVotingOnCodeReviewToAllUsers();
- gApi.changes().id(changeToBeRebased.get()).current().review(ReviewInput.approve());
+ gApi.changes()
+ .id(changeToBeRebased.get())
+ .current()
+ .review(ReviewInput.approve().label(TestLabels.verified().getName(), 1));
// The change is submittable because the approval is from a user (the rebaser) that is not the
// uploader.
@@ -1105,11 +1128,11 @@
RebaseInput rebaseInput = new RebaseInput();
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- // field1 is on_behalf_of_uploader, field2 is rebase_chain
- assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(1);
- assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(0);
- assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
- assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(0);
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain, field3 is allow_conflicts
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false, false)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true, false)).isEqualTo(0);
}
private void allowPermissionToAllUsers(String permission) {
@@ -1136,6 +1159,29 @@
.update();
}
+ private void createVerifiedLabel() throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder verified =
+ labelBuilder(
+ LabelId.VERIFIED, value(1, "Passes"), value(0, "No score"), value(-1, "Failed"))
+ .setCopyCondition("is:MIN");
+ u.getConfig().upsertLabelType(verified.build());
+ u.save();
+ }
+ }
+
+ private void allowVotingOnVerifiedToAllUsers() {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel(TestLabels.verified().getName())
+ .ref("refs/*")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+ }
+
private void blockPermissionForAllUsers(String permission) {
blockPermission(permission, REGISTERED_USERS);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/CapabilityInfo.java b/javatests/com/google/gerrit/acceptance/rest/account/CapabilityInfo.java
index 7598062..3ce6d8d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/CapabilityInfo.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/CapabilityInfo.java
@@ -38,6 +38,7 @@
public boolean viewConnections;
public boolean viewPlugins;
public boolean viewQueue;
+ public boolean viewSecondaryEmails;
static class QueryLimit {
short min;
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index 0360622..a1ec41c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -95,7 +95,6 @@
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
-import org.eclipse.jgit.api.errors.PatchApplyException;
import org.eclipse.jgit.api.errors.PatchFormatException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
@@ -1003,15 +1002,6 @@
+ "@@ -0,0 +1,2 @@\n"
+ "+First added line\n"
+ "+Second added line\n";
- private static final String MODIFICATION_PATCH_INPUT =
- "diff --git a/a_file.txt b/a_file.txt\n"
- + "new file mode 100644\n"
- + "--- a/a_file.txt\n"
- + "+++ b/a_file.txt.txt\n"
- + "@@ -1,2 +1 @@\n"
- + "-First original line\n"
- + "-Second original line\n"
- + "+Modified line\n";
@Test
public void createPatchApplyingChange_success() throws Exception {
@@ -1130,24 +1120,6 @@
}
@Test
- public void createPatchApplyingChange_withInfeasiblePatch_fails() throws Exception {
- createBranch(BranchNameKey.create(project, "other"));
- PushOneCommit push =
- pushFactory.create(
- admin.newIdent(),
- testRepo,
- "Adding unexpected base content, which will cause the patch to fail",
- PATCH_FILE_NAME,
- "unexpected base content");
- Result conflictingChange = push.to("refs/heads/other");
- conflictingChange.assertOkStatus();
- ChangeInput input = newPatchApplyingChangeInput("other", MODIFICATION_PATCH_INPUT);
-
- assertCreateFailsWithCause(
- input, RestApiException.class, PatchApplyException.class, "Cannot apply: HunkHeader");
- }
-
- @Test
@UseSystemTime
public void sha1sOfTwoNewChangesDiffer() throws Exception {
ChangeInput changeInput = newChangeInput(ChangeStatus.NEW);
diff --git a/javatests/com/google/gerrit/server/mail/send/NotificationEmailTest.java b/javatests/com/google/gerrit/server/mail/send/BranchEmailUtilsTest.java
similarity index 88%
rename from javatests/com/google/gerrit/server/mail/send/NotificationEmailTest.java
rename to javatests/com/google/gerrit/server/mail/send/BranchEmailUtilsTest.java
index b87c4a1..3c60b79 100644
--- a/javatests/com/google/gerrit/server/mail/send/NotificationEmailTest.java
+++ b/javatests/com/google/gerrit/server/mail/send/BranchEmailUtilsTest.java
@@ -15,12 +15,12 @@
package com.google.gerrit.server.mail.send;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.server.mail.send.NotificationEmail.getInstanceAndProjectName;
-import static com.google.gerrit.server.mail.send.NotificationEmail.getShortProjectName;
+import static com.google.gerrit.server.mail.send.BranchEmailUtils.getInstanceAndProjectName;
+import static com.google.gerrit.server.mail.send.BranchEmailUtils.getShortProjectName;
import org.junit.Test;
-public class NotificationEmailTest {
+public class BranchEmailUtilsTest {
@Test
public void instanceAndProjectName() throws Exception {
assertThat(getInstanceAndProjectName("test", "/my/api")).isEqualTo("test/api");
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 0f6245f..a188251 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -676,11 +676,21 @@
}
@Test
+ public void byAuthorExact_byAlias() throws Exception {
+ byAuthorOrCommitterExact("a:");
+ }
+
+ @Test
public void byAuthorFullText() throws Exception {
byAuthorOrCommitterFullText("author:");
}
@Test
+ public void byAuthorFullText_byAlias() throws Exception {
+ byAuthorOrCommitterFullText("a:");
+ }
+
+ @Test
public void byCommitterExact() throws Exception {
byAuthorOrCommitterExact("committer:");
}
diff --git a/modules/jgit b/modules/jgit
index 0687c73..5166ded 160000
--- a/modules/jgit
+++ b/modules/jgit
@@ -1 +1 @@
-Subproject commit 0687c73a12b5157c350de430f62ea8243d813e19
+Subproject commit 5166ded0986df7a99bbc9ae6bc057a27a1e7d974
diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor
index 597573b..1a7835b 160000
--- a/plugins/codemirror-editor
+++ b/plugins/codemirror-editor
@@ -1 +1 @@
-Subproject commit 597573b5d1ed44c75844d2b8047aae08f7ef8619
+Subproject commit 1a7835bdfa55d60c717bad20b943e34499da0016
diff --git a/plugins/package.json b/plugins/package.json
index 2299a93..612062b 100644
--- a/plugins/package.json
+++ b/plugins/package.json
@@ -32,7 +32,7 @@
"@codemirror/theme-one-dark": "^6.1.1",
"@codemirror/view": "^6.9.1",
"lit": "^2.2.3",
- "rxjs": "^7.8.0",
+ "rxjs": "^6.6.7",
"sinon": "^13.0.0"
},
"license": "Apache-2.0",
diff --git a/plugins/yarn.lock b/plugins/yarn.lock
index bad5fcc..3156f4c 100644
--- a/plugins/yarn.lock
+++ b/plugins/yarn.lock
@@ -2480,12 +2480,12 @@
dependencies:
queue-microtask "^1.2.2"
-rxjs@^7.8.0:
- version "7.8.0"
- resolved "https://registry.yarnpkg.com/rxjs/-/rxjs-7.8.0.tgz#90a938862a82888ff4c7359811a595e14e1e09a4"
- integrity sha512-F2+gxDshqmIub1KdvZkaEfGDwLNpPvk9Fs6LD/MyQxNgMds/WH9OdDDXOmxUZpME+iSK3rQCctkL0DYyytUqMg==
+rxjs@^6.6.7:
+ version "6.6.7"
+ resolved "https://registry.yarnpkg.com/rxjs/-/rxjs-6.6.7.tgz#90ac018acabf491bf65044235d5863c4dab804c9"
+ integrity sha512-hTdwr+7yYNIT5n4AMYp85KA6yw2Va0FLa3Rguvbpa4W3I5xynaBZo41cM3XM+4Q6fRMj3sBYIR1VAmZMXYJvRQ==
dependencies:
- tslib "^2.1.0"
+ tslib "^1.9.0"
safe-buffer@5.2.1, safe-buffer@~5.2.0:
version "5.2.1"
@@ -2681,10 +2681,10 @@
resolved "https://registry.yarnpkg.com/tr46/-/tr46-0.0.3.tgz#8184fd347dac9cdc185992f3a6622e14b9d9ab6a"
integrity sha512-N3WMsuqV66lT30CrXNbEjx4GEwlow3v6rr4mCcv6prnfwhS01rkgyFdjPNBYd9br7LpXV1+Emh01fHnq2Gdgrw==
-tslib@^2.1.0:
- version "2.5.0"
- resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.5.0.tgz#42bfed86f5787aeb41d031866c8f402429e0fddf"
- integrity sha512-336iVw3rtn2BUK7ORdIAHTyxHGRIHVReokCR3XjbckJMK7ms8FysBfhLR8IXnAgy7T0PTPNBWKiH514FOW/WSg==
+tslib@^1.9.0:
+ version "1.14.1"
+ resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.14.1.tgz#cf2d38bdc34a134bcaf1091c41f6619e2f672d00"
+ integrity sha512-Xni35NKzjgMrwevysHTCArtLDpPvye8zV/0E4EyYn43P7/7qvQwPh9BGkHewbMulVntbigmcT7rdX3BNo9wRJg==
tsscmp@1.0.6:
version "1.0.6"
diff --git a/polygerrit-ui/app/api/diff.ts b/polygerrit-ui/app/api/diff.ts
index d8f4942..4bf253d 100644
--- a/polygerrit-ui/app/api/diff.ts
+++ b/polygerrit-ui/app/api/diff.ts
@@ -243,12 +243,6 @@
image_diff_prefs?: ImageDiffPreferences;
responsive_mode?: DiffResponsiveMode;
num_lines_rendered_at_once?: number;
- /**
- * If enabled, then a new (experimental) diff rendering is used that is
- * based on Lit components and multiple rendering passes. This is planned to
- * be a temporary setting until the experiment is concluded.
- */
- use_lit_components?: boolean;
show_sign_col?: boolean;
/**
* The default view mode is SIDE_BY_SIDE.
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-summary-chip.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-summary-chip.ts
index 146ab61..5588f40 100644
--- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-summary-chip.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-summary-chip.ts
@@ -66,7 +66,7 @@
border-color: var(--info-foreground);
background: var(--info-background);
}
- .summaryChip.info:hover {
+ button.summaryChip.info:hover {
background: var(--info-background-hover);
box-shadow: var(--elevation-level-1);
}
@@ -80,7 +80,7 @@
border-color: var(--warning-foreground);
background: var(--warning-background);
}
- .summaryChip.warning:hover {
+ button.summaryChip.warning:hover {
background: var(--warning-background-hover);
box-shadow: var(--elevation-level-1);
}
@@ -94,7 +94,7 @@
border-color: var(--gray-foreground);
background: var(--gray-background);
}
- .summaryChip.check:hover {
+ button.summaryChip.check:hover {
background: var(--gray-background-hover);
box-shadow: var(--elevation-level-1);
}
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
index 3982666..166d895 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
@@ -3,6 +3,7 @@
* Copyright 2016 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
+import '../../shared/gr-account-chip/gr-account-chip';
import {css, html, LitElement, PropertyValues} from 'lit';
import {customElement, property, query, state} from 'lit/decorators.js';
import {when} from 'lit/directives/when.js';
@@ -10,6 +11,8 @@
NumericChangeId,
BranchName,
ChangeActionDialog,
+ AccountDetailInfo,
+ AccountInfo,
} from '../../../types/common';
import '../../shared/gr-dialog/gr-dialog';
import '../../shared/gr-autocomplete/gr-autocomplete';
@@ -23,10 +26,10 @@
import {ValueChangedEvent} from '../../../types/events';
import {throwingErrorCallback} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
import {fireNoBubbleNoCompose} from '../../../utils/event-util';
-import {KnownExperimentId} from '../../../services/flags/flags';
import {resolve} from '../../../models/dependency';
import {changeModelToken} from '../../../models/change/change-model';
import {subscribe} from '../../lit/subscription-controller';
+import {userModelToken} from '../../../models/user/user-model';
export interface RebaseChange {
name: string;
@@ -87,9 +90,6 @@
@state()
allowConflicts = false;
- @state()
- isOwner = false;
-
@query('#rebaseOnParentInput')
private rebaseOnParentInput!: HTMLInputElement;
@@ -108,19 +108,30 @@
@query('#parentInput')
parentInput!: GrAutocomplete;
+ @state()
+ account?: AccountDetailInfo;
+
+ @state()
+ uploader?: AccountInfo;
+
private readonly restApiService = getAppContext().restApiService;
- private readonly flagsService = getAppContext().flagsService;
-
private readonly getChangeModel = resolve(this, changeModelToken);
+ private readonly getUserModel = resolve(this, userModelToken);
+
constructor() {
super();
this.query = input => this.getChangeSuggestions(input);
subscribe(
this,
- () => this.getChangeModel().isOwner$,
- x => (this.isOwner = x)
+ () => this.getUserModel().account$,
+ x => (this.account = x)
+ );
+ subscribe(
+ this,
+ () => this.getChangeModel().latestUploader$,
+ x => (this.uploader = x)
);
}
@@ -155,12 +166,15 @@
display: block;
width: 100%;
}
- .rebaseCheckbox:first-of-type {
+ .rebaseCheckbox {
margin-top: var(--spacing-m);
}
.rebaseOption {
margin: var(--spacing-m) 0;
}
+ .rebaseOnBehalfMsg {
+ margin-top: var(--spacing-m);
+ }
`,
];
@@ -258,11 +272,7 @@
>
</div>
${when(
- this.flagsService.isEnabled(
- KnownExperimentId.REBASE_ON_BEHALF_OF_UPLOADER
- ) &&
- !this.isOwner &&
- this.allowConflicts,
+ !this.isCurrentUserEqualToLatestUploader() && this.allowConflicts,
() =>
html`<span class="message"
>Rebase cannot be done on behalf of the uploader when allowing
@@ -283,6 +293,16 @@
<label for="rebaseChain">Rebase all ancestors</label>
</div>`
)}
+ ${when(
+ !this.isCurrentUserEqualToLatestUploader(),
+ () => html`<div class="rebaseOnBehalfMsg">Rebase will be done on behalf of${
+ !this.allowConflicts ? ' the uploader:' : ''
+ } <gr-account-chip
+ .account=${this.allowConflicts ? this.account : this.uploader}
+ .hideHovercard=${true}
+ ></gr-account-chip
+ ><span></div>`
+ )}
</div>
</gr-dialog>
`;
@@ -317,6 +337,11 @@
});
}
+ isCurrentUserEqualToLatestUploader() {
+ if (!this.account || !this.uploader) return true;
+ return this.account._account_id === this.uploader._account_id;
+ }
+
getRecentChanges() {
if (this.recentChanges) {
return Promise.resolve(this.recentChanges);
@@ -396,13 +421,6 @@
}
private rebaseOnBehalfOfUploader() {
- if (
- !this.flagsService.isEnabled(
- KnownExperimentId.REBASE_ON_BEHALF_OF_UPLOADER
- )
- ) {
- return false;
- }
if (this.allowConflicts) return false;
return true;
}
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.ts
index 3064014..8d907c9 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.ts
@@ -9,21 +9,34 @@
import {
pressKey,
queryAndAssert,
- stubFlags,
stubRestApi,
waitUntil,
} from '../../../test/test-utils';
-import {NumericChangeId, BranchName} from '../../../types/common';
-import {createChangeViewChange} from '../../../test/test-data-generators';
+import {NumericChangeId, BranchName, Timestamp} from '../../../types/common';
+import {
+ createAccountWithEmail,
+ createChangeViewChange,
+} from '../../../test/test-data-generators';
import {fixture, html, assert} from '@open-wc/testing';
import {Key} from '../../../utils/dom-util';
import {GrDialog} from '../../shared/gr-dialog/gr-dialog';
+import {testResolver} from '../../../test/common-test-setup';
+import {userModelToken} from '../../../models/user/user-model';
+import {
+ changeModelToken,
+ LoadingStatus,
+} from '../../../models/change/change-model';
+import {GrAccountChip} from '../../shared/gr-account-chip/gr-account-chip';
suite('gr-confirm-rebase-dialog tests', () => {
let element: GrConfirmRebaseDialog;
setup(async () => {
- stubFlags('isEnabled').returns(true);
+ const userModel = testResolver(userModelToken);
+ userModel.setAccount({
+ ...createAccountWithEmail('abc@def.com'),
+ registered_on: '2015-03-12 18:32:08.000000000' as Timestamp,
+ });
element = await fixture(
html`<gr-confirm-rebase-dialog></gr-confirm-rebase-dialog>`
);
@@ -91,6 +104,57 @@
);
});
+ suite('on behalf of uploader', () => {
+ let changeModel;
+ const change = {
+ ...createChangeViewChange(),
+ };
+ setup(async () => {
+ element.branch = 'test' as BranchName;
+ await element.updateComplete;
+ changeModel = testResolver(changeModelToken);
+ changeModel.setState({
+ loadingStatus: LoadingStatus.LOADED,
+ change,
+ });
+ });
+ test('for reviewer it shows message about on behalf', () => {
+ const rebaseOnBehalfMsg = queryAndAssert(element, '.rebaseOnBehalfMsg');
+ assert.dom.equal(
+ rebaseOnBehalfMsg,
+ /* HTML */ `<div class="rebaseOnBehalfMsg">
+ Rebase will be done on behalf of the uploader:
+ <gr-account-chip> </gr-account-chip> <span> </span>
+ </div>`
+ );
+ const accountChip: GrAccountChip = queryAndAssert(
+ rebaseOnBehalfMsg,
+ 'gr-account-chip'
+ );
+ assert.equal(
+ accountChip.account!,
+ change?.revisions[change.current_revision]?.uploader
+ );
+ });
+ test('allowConflicts', async () => {
+ element.allowConflicts = true;
+ await element.updateComplete;
+ const rebaseOnBehalfMsg = queryAndAssert(element, '.rebaseOnBehalfMsg');
+ assert.dom.equal(
+ rebaseOnBehalfMsg,
+ /* HTML */ `<div class="rebaseOnBehalfMsg">
+ Rebase will be done on behalf of
+ <gr-account-chip> </gr-account-chip> <span> </span>
+ </div>`
+ );
+ const accountChip: GrAccountChip = queryAndAssert(
+ rebaseOnBehalfMsg,
+ 'gr-account-chip'
+ );
+ assert.equal(accountChip.account, element.account);
+ });
+ });
+
test('disableActions property disables dialog confirm', async () => {
element.disableActions = false;
await element.updateComplete;
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 63a2ac3..345feb2 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
@@ -21,6 +21,8 @@
PatchSetNum,
VotingRangeInfo,
isRobot,
+ EDIT,
+ PARENT,
} from '../../../types/common';
import {GrMessage, MessageAnchorTapDetail} from '../gr-message/gr-message';
import {getVotingRange} from '../../../utils/label-util';
@@ -154,13 +156,25 @@
message: CombinedMessage,
allMessages: CombinedMessage[]
): PatchSetNum | undefined {
- if (message._revision_number && message._revision_number > 0)
+ if (
+ message._revision_number !== undefined &&
+ message._revision_number !== 0 &&
+ message._revision_number !== PARENT &&
+ message._revision_number !== EDIT
+ ) {
return message._revision_number;
+ }
let revision: PatchSetNum = 0 as PatchSetNum;
for (const m of allMessages) {
if (m.date > message.date) break;
- if (m._revision_number && m._revision_number > revision)
+ if (
+ m._revision_number !== undefined &&
+ m._revision_number !== 0 &&
+ m._revision_number !== PARENT &&
+ m._revision_number !== EDIT
+ ) {
revision = m._revision_number;
+ }
}
return revision > 0 ? revision : undefined;
}
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 9783306..9bead95 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
@@ -342,10 +342,6 @@
resolve(this, highlightServiceToken),
() => getAppContext().reportingService
);
- this.renderPrefs = {
- ...this.renderPrefs,
- use_lit_components: true,
- };
this.addEventListener(
// These are named inconsistently for a reason:
// The create-comment event is fired to indicate that we should
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 c38e788..de6a39c9 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -237,7 +237,7 @@
* This is triggered when the user types into the editing textarea. We then
* debounce it and call autoSave().
*/
- private autoSaveTrigger$ = new Subject<void>();
+ private autoSaveTrigger$ = new Subject();
/**
* Set to the content of DraftInfo when entering editing mode.
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
index 782c055..7779fff 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
@@ -475,14 +475,19 @@
}
private async computeSuggestions() {
+ this.suggestions = [];
if (this.currentSearchString === undefined) {
- this.suggestions = [];
return;
}
+ const searchString = this.currentSearchString;
+ let suggestions: (Item | EmojiSuggestion)[] = [];
if (this.isEmojiDropdownActive()) {
- this.computeEmojiSuggestions(this.currentSearchString);
+ suggestions = this.computeEmojiSuggestions(this.currentSearchString);
} else if (this.isMentionsDropdownActive()) {
- await this.computeReviewerSuggestions();
+ suggestions = await this.computeReviewerSuggestions();
+ }
+ if (searchString === this.currentSearchString) {
+ this.suggestions = suggestions;
}
}
@@ -570,7 +575,7 @@
}
// private but used in test
- formatSuggestions(matchedSuggestions: EmojiSuggestion[]) {
+ formatSuggestions(matchedSuggestions: EmojiSuggestion[]): EmojiSuggestion[] {
const suggestions = [];
for (const suggestion of matchedSuggestions) {
assert(isEmojiSuggestion(suggestion), 'malformed suggestion');
@@ -578,28 +583,27 @@
suggestion.text = `${suggestion.value} ${suggestion.match}`;
suggestions.push(suggestion);
}
- this.suggestions = suggestions;
+ return suggestions;
}
// private but used in test
- computeEmojiSuggestions(suggestionsText?: string) {
+ computeEmojiSuggestions(suggestionsText?: string): EmojiSuggestion[] {
if (suggestionsText === undefined) {
- this.suggestions = [];
- return;
+ return [];
}
if (!suggestionsText.length) {
- this.formatSuggestions(ALL_SUGGESTIONS);
+ return this.formatSuggestions(ALL_SUGGESTIONS);
} else {
const matches = ALL_SUGGESTIONS.filter(suggestion =>
suggestion.match.includes(suggestionsText)
).slice(0, MAX_ITEMS_DROPDOWN);
- this.formatSuggestions(matches);
+ return this.formatSuggestions(matches);
}
}
// TODO(dhruvsri): merge with getAccountSuggestions in account-util
- async computeReviewerSuggestions() {
- this.suggestions = (
+ async computeReviewerSuggestions(): Promise<Item[]> {
+ return (
(await this.restApiService.getSuggestedAccounts(
this.currentSearchString ?? '',
/* number= */ 15,
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
index 154ea0a..4dcaa80 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
@@ -6,8 +6,16 @@
import '../../../test/common-test-setup';
import './gr-textarea';
import {GrTextarea} from './gr-textarea';
-import {ItemSelectedEventDetail} from '../gr-autocomplete-dropdown/gr-autocomplete-dropdown';
-import {pressKey, stubRestApi, waitUntil} from '../../../test/test-utils';
+import {
+ Item,
+ ItemSelectedEventDetail,
+} from '../gr-autocomplete-dropdown/gr-autocomplete-dropdown';
+import {
+ mockPromise,
+ pressKey,
+ stubRestApi,
+ waitUntil,
+} from '../../../test/test-utils';
import {fixture, html, assert} from '@open-wc/testing';
import {createAccountWithEmail} from '../../../test/test-data-generators';
import {Key} from '../../../utils/dom-util';
@@ -120,6 +128,98 @@
assert.isFalse(element.mentionsSuggestions!.isHidden);
});
+ test('mention suggestions cleared before request returns', async () => {
+ const promise = mockPromise<Item[]>();
+ stubRestApi('getSuggestedAccounts').returns(promise);
+ element.textarea!.focus();
+ await waitUntil(() => element.textarea!.focused === true);
+
+ element.suggestions = [
+ {dataValue: 'prior@google.com', text: 'Prior suggestion'},
+ ];
+ element.textarea!.selectionStart = 1;
+ element.textarea!.selectionEnd = 1;
+ element.text = '@';
+
+ await element.updateComplete;
+ assert.equal(element.suggestions.length, 0);
+
+ promise.resolve([
+ createAccountWithEmail('abc@google.com'),
+ createAccountWithEmail('abcdef@google.com'),
+ ]);
+ await waitUntil(() => element.suggestions.length !== 0);
+ assert.deepEqual(element.suggestions, [
+ {
+ dataValue: 'abc@google.com',
+ text: 'abc@google.com <abc@google.com>',
+ },
+ {
+ dataValue: 'abcdef@google.com',
+ text: 'abcdef@google.com <abcdef@google.com>',
+ },
+ ]);
+ });
+
+ test('mention dropdown shows suggestion for latest text', async () => {
+ const promise1 = mockPromise<Item[]>();
+ const promise2 = mockPromise<Item[]>();
+ const suggestionStub = stubRestApi('getSuggestedAccounts');
+ suggestionStub.returns(promise1);
+ element.textarea!.focus();
+ await waitUntil(() => element.textarea!.focused === true);
+
+ element.textarea!.selectionStart = 1;
+ element.textarea!.selectionEnd = 1;
+ element.text = '@';
+ await element.updateComplete;
+ assert.equal(element.currentSearchString, '');
+
+ suggestionStub.returns(promise2);
+ element.text = '@abc@google.com';
+ // None of suggestions returned yet.
+ assert.equal(element.suggestions.length, 0);
+ await element.updateComplete;
+ assert.equal(element.currentSearchString, 'abc@google.com');
+
+ promise2.resolve([
+ createAccountWithEmail('abc@google.com'),
+ createAccountWithEmail('abcdef@google.com'),
+ ]);
+
+ await waitUntil(() => element.suggestions.length !== 0);
+ assert.deepEqual(element.suggestions, [
+ {
+ dataValue: 'abc@google.com',
+ text: 'abc@google.com <abc@google.com>',
+ },
+ {
+ dataValue: 'abcdef@google.com',
+ text: 'abcdef@google.com <abcdef@google.com>',
+ },
+ ]);
+
+ promise1.resolve([
+ createAccountWithEmail('dce@google.com'),
+ createAccountWithEmail('defcba@google.com'),
+ ]);
+ // Empty the event queue.
+ await new Promise<void>(resolve => {
+ setTimeout(() => resolve());
+ });
+ // Suggestions didn't change
+ assert.deepEqual(element.suggestions, [
+ {
+ dataValue: 'abc@google.com',
+ text: 'abc@google.com <abc@google.com>',
+ },
+ {
+ dataValue: 'abcdef@google.com',
+ text: 'abcdef@google.com <abcdef@google.com>',
+ },
+ ]);
+ });
+
test('emoji selector does not open when previous char is \n', async () => {
element.textarea!.focus();
await waitUntil(() => element.textarea!.focused === true);
@@ -195,21 +295,25 @@
assert.isFalse(element.mentionsSuggestions!.isHidden);
element.text = '@h';
+ await waitUntil(() => element.suggestions.length > 0);
await element.updateComplete;
assert.isTrue(element.emojiSuggestions!.isHidden);
assert.isFalse(element.mentionsSuggestions!.isHidden);
element.text = '@h ';
+ await waitUntil(() => element.suggestions.length > 0);
await element.updateComplete;
assert.isTrue(element.emojiSuggestions!.isHidden);
assert.isFalse(element.mentionsSuggestions!.isHidden);
element.text = '@h :';
+ await waitUntil(() => element.suggestions.length > 0);
await element.updateComplete;
assert.isTrue(element.emojiSuggestions!.isHidden);
assert.isFalse(element.mentionsSuggestions!.isHidden);
element.text = '@h :D';
+ await waitUntil(() => element.suggestions.length > 0);
await element.updateComplete;
assert.isTrue(element.emojiSuggestions!.isHidden);
assert.isFalse(element.mentionsSuggestions!.isHidden);
@@ -465,13 +569,13 @@
{value: '😢', match: 'tear'},
{value: '😂', match: 'tears'},
];
- element.formatSuggestions(matchedSuggestions);
+ const suggestions = element.formatSuggestions(matchedSuggestions);
assert.deepEqual(
[
{value: '😢', dataValue: '😢', match: 'tear', text: '😢 tear'},
{value: '😂', dataValue: '😂', match: 'tears', text: '😂 tears'},
],
- element.suggestions
+ suggestions
);
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index 71a0637..7fa6d72 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -13,9 +13,7 @@
DiffContextExpandedEventDetail,
isImageDiffBuilder,
} from './gr-diff-builder';
-import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
import {GrDiffBuilderImage} from './gr-diff-builder-image';
-import {GrDiffBuilderUnified} from './gr-diff-builder-unified';
import {GrDiffBuilderBinary} from './gr-diff-builder-binary';
import {GrDiffBuilderLit} from './gr-diff-builder-lit';
import {CancelablePromise, makeCancelable} from '../../../utils/async-util';
@@ -428,7 +426,6 @@
}
let builder = null;
- const useLit = this.renderPrefs?.use_lit_components ?? false;
if (this.isImageDiff) {
builder = new GrDiffBuilderImage(
this.diff,
@@ -447,45 +444,25 @@
...this.renderPrefs,
view_mode: DiffViewMode.SIDE_BY_SIDE,
};
- if (useLit) {
- builder = new GrDiffBuilderLit(
- this.diff,
- localPrefs,
- this.diffElement,
- this.layersInternal,
- this.renderPrefs
- );
- } else {
- builder = new GrDiffBuilderSideBySide(
- this.diff,
- localPrefs,
- this.diffElement,
- this.layersInternal,
- this.renderPrefs
- );
- }
+ builder = new GrDiffBuilderLit(
+ this.diff,
+ localPrefs,
+ this.diffElement,
+ this.layersInternal,
+ this.renderPrefs
+ );
} else if (this.viewMode === DiffViewMode.UNIFIED) {
this.renderPrefs = {
...this.renderPrefs,
view_mode: DiffViewMode.UNIFIED,
};
- if (useLit) {
- builder = new GrDiffBuilderLit(
- this.diff,
- localPrefs,
- this.diffElement,
- this.layersInternal,
- this.renderPrefs
- );
- } else {
- builder = new GrDiffBuilderUnified(
- this.diff,
- localPrefs,
- this.diffElement,
- this.layersInternal,
- this.renderPrefs
- );
- }
+ builder = new GrDiffBuilderLit(
+ this.diff,
+ localPrefs,
+ this.diffElement,
+ this.layersInternal,
+ this.renderPrefs
+ );
}
if (!builder) {
throw Error(`Unsupported diff view mode: ${this.viewMode}`);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
index 9cf9bae..ba6acfd 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
@@ -6,17 +6,15 @@
import '../../../test/common-test-setup';
import {
createConfig,
- createDiff,
createEmptyDiff,
} from '../../../test/test-data-generators';
import './gr-diff-builder-element';
-import {queryAndAssert, stubBaseUrl, waitUntil} from '../../../test/test-utils';
+import {stubBaseUrl, waitUntil} from '../../../test/test-utils';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
import {GrDiffLine, GrDiffLineType} from '../gr-diff/gr-diff-line';
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
import {
DiffContent,
- DiffInfo,
DiffLayer,
DiffPreferencesInfo,
DiffViewMode,
@@ -30,6 +28,7 @@
import {KeyLocations} from '../gr-diff-processor/gr-diff-processor';
import {BlameInfo} from '../../../types/common';
import {fixture, html, assert} from '@open-wc/testing';
+import {GrDiffRow} from './gr-diff-row';
const DEFAULT_PREFS = createDefaultDiffPrefs();
@@ -39,7 +38,6 @@
let diffTable: HTMLTableElement;
const LINE_BREAK_HTML = '<span class="gr-diff br"></span>';
- const WBR_HTML = '<wbr class="gr-diff">';
const setBuilderPrefs = (prefs: Partial<DiffPreferencesInfo>) => {
builder = new GrDiffBuilderSideBySide(
@@ -65,15 +63,6 @@
setBuilderPrefs({});
});
- test('line_length applied with <wbr> if line_wrapping is true', () => {
- setBuilderPrefs({line_wrapping: true, tab_size: 4, line_length: 50});
- const text = 'a'.repeat(51);
- const expected = 'a'.repeat(50) + WBR_HTML + 'a';
- const result = builder.createTextEl(null, line(text)).firstElementChild
- ?.firstElementChild?.innerHTML;
- assert.equal(result, expected);
- });
-
test('line_length applied with line break if line_wrapping is false', () => {
setBuilderPrefs({line_wrapping: false, tab_size: 4, line_length: 50});
const text = 'a'.repeat(51);
@@ -691,7 +680,7 @@
assert.include(diffRows[4].textContent, 'unchanged 11');
});
- test('clicking +x common lines expands those lines', () => {
+ test('clicking +x common lines expands those lines', async () => {
const contextControls = diffTable.querySelectorAll('gr-context-controls');
const topExpandCommonButton =
contextControls[0].shadowRoot?.querySelectorAll<HTMLElement>(
@@ -699,10 +688,19 @@
)[0];
assert.isOk(topExpandCommonButton);
assert.include(topExpandCommonButton!.textContent, '+9 common lines');
+ let diffRows = diffTable.querySelectorAll('.diff-row');
+ // 5 lines:
+ // FILE, LOST, the changed line plus one line of context in each direction
+ assert.equal(diffRows.length, 5);
+
topExpandCommonButton!.click();
- const diffRows = diffTable.querySelectorAll('.diff-row');
- // The first two are LOST and FILE line
- assert.equal(diffRows.length, 2 + 10 + 1 + 1);
+
+ await waitUntil(() => {
+ diffRows = diffTable.querySelectorAll<GrDiffRow>('.diff-row');
+ return diffRows.length === 14;
+ });
+ // 14 lines: The 5 above plus the 9 unchanged lines that were expanded
+ assert.equal(diffRows.length, 14);
assert.include(diffRows[2].textContent, 'unchanged 1');
assert.include(diffRows[3].textContent, 'unchanged 2');
assert.include(diffRows[4].textContent, 'unchanged 3');
@@ -722,6 +720,11 @@
dispatchStub.reset();
element.unhideLine(4, Side.LEFT);
+ await waitUntil(() => {
+ const rows = diffTable.querySelectorAll<GrDiffRow>('.diff-row');
+ return rows.length === 2 + 5 + 1 + 1 + 1;
+ });
+
const diffRows = diffTable.querySelectorAll('.diff-row');
// The first two are LOST and FILE line
// Lines 3-5 (Line 4 plus 1 context in each direction) will be expanded
@@ -745,332 +748,6 @@
});
});
- [DiffViewMode.UNIFIED, DiffViewMode.SIDE_BY_SIDE].forEach(mode => {
- suite(`mock-diff mode:${mode}`, () => {
- let builder: GrDiffBuilderSideBySide;
- let diff: DiffInfo;
- let keyLocations: KeyLocations;
-
- setup(() => {
- element.viewMode = mode;
- diff = createDiff();
- element.diff = diff;
-
- keyLocations = {left: {}, right: {}};
-
- element.prefs = {
- ...createDefaultDiffPrefs(),
- line_length: 80,
- show_tabs: true,
- tab_size: 4,
- };
- element.render(keyLocations);
- builder = element.builder as GrDiffBuilderSideBySide;
- });
-
- test('aria-labels on added line numbers', () => {
- const deltaLineNumberButton = diffTable.querySelectorAll(
- '.lineNumButton.right'
- )[5];
-
- assert.isOk(deltaLineNumberButton);
- assert.equal(
- deltaLineNumberButton.getAttribute('aria-label'),
- '5 added'
- );
- });
-
- test('aria-labels on removed line numbers', () => {
- const deltaLineNumberButton = diffTable.querySelectorAll(
- '.lineNumButton.left'
- )[10];
-
- assert.isOk(deltaLineNumberButton);
- assert.equal(
- deltaLineNumberButton.getAttribute('aria-label'),
- '10 removed'
- );
- });
-
- test('getContentByLine', () => {
- let actual: HTMLElement | null;
-
- actual = builder.getContentByLine(2, Side.LEFT);
- assert.equal(actual?.textContent, diff.content[0].ab?.[1]);
-
- actual = builder.getContentByLine(2, Side.RIGHT);
- assert.equal(actual?.textContent, diff.content[0].ab?.[1]);
-
- actual = builder.getContentByLine(5, Side.LEFT);
- assert.equal(actual?.textContent, diff.content[2].ab?.[0]);
-
- actual = builder.getContentByLine(5, Side.RIGHT);
- assert.equal(actual?.textContent, diff.content[1].b?.[0]);
- });
-
- test('getContentTdByLineEl works both with button and td', () => {
- const diffRow = diffTable.querySelectorAll('tr.diff-row')[2];
-
- const lineNumTdLeft = queryAndAssert(diffRow, 'td.lineNum.left');
- const lineNumButtonLeft = queryAndAssert(lineNumTdLeft, 'button');
- const contentTdLeft = diffRow.querySelectorAll('.content')[0];
-
- const lineNumTdRight = queryAndAssert(diffRow, 'td.lineNum.right');
- const lineNumButtonRight = queryAndAssert(lineNumTdRight, 'button');
- const contentTdRight =
- mode === DiffViewMode.SIDE_BY_SIDE
- ? diffRow.querySelectorAll('.content')[1]
- : contentTdLeft;
-
- assert.equal(
- element.getContentTdByLineEl(lineNumTdLeft),
- contentTdLeft
- );
- assert.equal(
- element.getContentTdByLineEl(lineNumButtonLeft),
- contentTdLeft
- );
- assert.equal(
- element.getContentTdByLineEl(lineNumTdRight),
- contentTdRight
- );
- assert.equal(
- element.getContentTdByLineEl(lineNumButtonRight),
- contentTdRight
- );
- });
-
- test('findLinesByRange LEFT', () => {
- const lines: GrDiffLine[] = [];
- const elems: HTMLElement[] = [];
- const start = 1;
- const end = 44;
-
- // lines 26-29 are collapsed, so minus 4
- let count = end - start + 1 - 4;
- // Lines 14+15 are part of a 'common' chunk. And we have a bug in
- // unified diff that results in not rendering these lines for the LEFT
- // side. TODO: Fix that bug!
- if (mode === DiffViewMode.UNIFIED) count -= 2;
-
- builder.findLinesByRange(start, end, Side.LEFT, lines, elems);
-
- assert.equal(lines.length, count);
- assert.equal(elems.length, count);
-
- for (let i = 0; i < count; i++) {
- assert.instanceOf(lines[i], GrDiffLine);
- assert.instanceOf(elems[i], HTMLElement);
- assert.equal(lines[i].text, elems[i].textContent);
- }
- });
-
- test('findLinesByRange RIGHT', () => {
- const lines: GrDiffLine[] = [];
- const elems: HTMLElement[] = [];
- const start = 1;
- const end = 48;
-
- // lines 26-29 are collapsed, so minus 4
- const count = end - start + 1 - 4;
-
- builder.findLinesByRange(start, end, Side.RIGHT, lines, elems);
-
- assert.equal(lines.length, count);
- assert.equal(elems.length, count);
-
- for (let i = 0; i < count; i++) {
- assert.instanceOf(lines[i], GrDiffLine);
- assert.instanceOf(elems[i], HTMLElement);
- assert.equal(lines[i].text, elems[i].textContent);
- }
- });
-
- test('renderContentByRange', () => {
- const spy = sinon.spy(builder, 'createTextEl');
- const start = 9;
- const end = 14;
- let count = end - start + 1;
- // Lines 14+15 are part of a 'common' chunk. And we have a bug in
- // unified diff that results in not rendering these lines for the LEFT
- // side. TODO: Fix that bug!
- if (mode === DiffViewMode.UNIFIED) count -= 1;
-
- builder.renderContentByRange(start, end, Side.LEFT);
-
- assert.equal(spy.callCount, count);
- spy.getCalls().forEach((call, i: number) => {
- assert.equal(call.args[1].beforeNumber, start + i);
- });
- });
-
- test('renderContentByRange non-existent elements', () => {
- const spy = sinon.spy(builder, 'createTextEl');
-
- sinon
- .stub(builder, 'getLineNumberEl')
- .returns(document.createElement('div'));
- sinon
- .stub(builder, 'findLinesByRange')
- .callsFake((_1, _2, _3, lines, elements) => {
- // Add a line and a corresponding element.
- lines?.push(new GrDiffLine(GrDiffLineType.BOTH));
- const tr = document.createElement('tr');
- const td = document.createElement('td');
- const el = document.createElement('div');
- tr.appendChild(td);
- td.appendChild(el);
- elements?.push(el);
-
- // Add 2 lines without corresponding elements.
- lines?.push(new GrDiffLine(GrDiffLineType.BOTH));
- lines?.push(new GrDiffLine(GrDiffLineType.BOTH));
- });
-
- builder.renderContentByRange(1, 10, Side.LEFT);
- // Should be called only once because only one line had a corresponding
- // element.
- assert.equal(spy.callCount, 1);
- });
-
- test('getLineNumberEl side-by-side left', () => {
- const contentEl = builder.getContentByLine(
- 5,
- Side.LEFT,
- element.diffElement as HTMLTableElement
- );
- assert.isOk(contentEl);
- const lineNumberEl = builder.getLineNumberEl(contentEl!, Side.LEFT);
- assert.isOk(lineNumberEl);
- assert.isTrue(lineNumberEl!.classList.contains('lineNum'));
- assert.isTrue(lineNumberEl!.classList.contains(Side.LEFT));
- });
-
- test('getLineNumberEl side-by-side right', () => {
- const contentEl = builder.getContentByLine(
- 5,
- Side.RIGHT,
- element.diffElement as HTMLTableElement
- );
- assert.isOk(contentEl);
- const lineNumberEl = builder.getLineNumberEl(contentEl!, Side.RIGHT);
- assert.isOk(lineNumberEl);
- assert.isTrue(lineNumberEl!.classList.contains('lineNum'));
- assert.isTrue(lineNumberEl!.classList.contains(Side.RIGHT));
- });
-
- test('getLineNumberEl unified left', async () => {
- // Re-render as unified:
- element.viewMode = 'UNIFIED_DIFF';
- element.render(keyLocations);
- builder = element.builder as GrDiffBuilderSideBySide;
-
- const contentEl = builder.getContentByLine(
- 5,
- Side.LEFT,
- element.diffElement as HTMLTableElement
- );
- assert.isOk(contentEl);
- const lineNumberEl = builder.getLineNumberEl(contentEl!, Side.LEFT);
- assert.isOk(lineNumberEl);
- assert.isTrue(lineNumberEl!.classList.contains('lineNum'));
- assert.isTrue(lineNumberEl!.classList.contains(Side.LEFT));
- });
-
- test('getLineNumberEl unified right', async () => {
- // Re-render as unified:
- element.viewMode = 'UNIFIED_DIFF';
- element.render(keyLocations);
- builder = element.builder as GrDiffBuilderSideBySide;
-
- const contentEl = builder.getContentByLine(
- 5,
- Side.RIGHT,
- element.diffElement as HTMLTableElement
- );
- assert.isOk(contentEl);
- const lineNumberEl = builder.getLineNumberEl(contentEl!, Side.RIGHT);
- assert.isOk(lineNumberEl);
- assert.isTrue(lineNumberEl!.classList.contains('lineNum'));
- assert.isTrue(lineNumberEl!.classList.contains(Side.RIGHT));
- });
-
- test('getNextContentOnSide side-by-side left', () => {
- const startElem = builder.getContentByLine(
- 5,
- Side.LEFT,
- element.diffElement as HTMLTableElement
- );
- assert.isOk(startElem);
- const expectedStartString = diff.content[2].ab?.[0];
- const expectedNextString = diff.content[2].ab?.[1];
- assert.equal(startElem!.textContent, expectedStartString);
-
- const nextElem = builder.getNextContentOnSide(startElem!, Side.LEFT);
- assert.isOk(nextElem);
- assert.equal(nextElem!.textContent, expectedNextString);
- });
-
- test('getNextContentOnSide side-by-side right', () => {
- const startElem = builder.getContentByLine(
- 5,
- Side.RIGHT,
- element.diffElement as HTMLTableElement
- );
- const expectedStartString = diff.content[1].b?.[0];
- const expectedNextString = diff.content[1].b?.[1];
- assert.isOk(startElem);
- assert.equal(startElem!.textContent, expectedStartString);
-
- const nextElem = builder.getNextContentOnSide(startElem!, Side.RIGHT);
- assert.isOk(nextElem);
- assert.equal(nextElem!.textContent, expectedNextString);
- });
-
- test('getNextContentOnSide unified left', async () => {
- // Re-render as unified:
- element.viewMode = 'UNIFIED_DIFF';
- element.render(keyLocations);
- builder = element.builder as GrDiffBuilderSideBySide;
-
- const startElem = builder.getContentByLine(
- 5,
- Side.LEFT,
- element.diffElement as HTMLTableElement
- );
- const expectedStartString = diff.content[2].ab?.[0];
- const expectedNextString = diff.content[2].ab?.[1];
- assert.isOk(startElem);
- assert.equal(startElem!.textContent, expectedStartString);
-
- const nextElem = builder.getNextContentOnSide(startElem!, Side.LEFT);
- assert.isOk(nextElem);
- assert.equal(nextElem!.textContent, expectedNextString);
- });
-
- test('getNextContentOnSide unified right', async () => {
- // Re-render as unified:
- element.viewMode = 'UNIFIED_DIFF';
- element.render(keyLocations);
- builder = element.builder as GrDiffBuilderSideBySide;
-
- const startElem = builder.getContentByLine(
- 5,
- Side.RIGHT,
- element.diffElement as HTMLTableElement
- );
- const expectedStartString = diff.content[1].b?.[0];
- const expectedNextString = diff.content[1].b?.[1];
- assert.isOk(startElem);
- assert.equal(startElem!.textContent, expectedStartString);
-
- const nextElem = builder.getNextContentOnSide(startElem!, Side.RIGHT);
- assert.isOk(nextElem);
- assert.equal(nextElem!.textContent, expectedNextString);
- });
- });
- });
-
suite('blame', () => {
let mockBlame: BlameInfo[];
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
index b3f3714..e786cf4 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-legacy.ts
@@ -313,7 +313,7 @@
// For unified diff, this method will be called with number set to 0 for
// the empty line number column for added/removed lines. This should not
// be announced to the screenreader.
- if (number > 0) {
+ if (number !== 'FILE' && number > 0) {
if (line.type === GrDiffLineType.REMOVE) {
button.setAttribute('aria-label', `${number} removed`);
} else if (line.type === GrDiffLineType.ADD) {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
index b97ae59..9acda81 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
@@ -309,7 +309,7 @@
// For unified diff, this method will be called with number set to 0 for
// the empty line number column for added/removed lines. This should not
// be announced to the screenreader.
- if (lineNumber <= 0) return undefined;
+ if (lineNumber === 'LOST' || lineNumber <= 0) return undefined;
switch (line.type) {
case GrDiffLineType.REMOVE:
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-text_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-text_test.ts
index a0e7840..3858bed 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-text_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-text_test.ts
@@ -10,6 +10,8 @@
const LINE_BREAK = '<span class="gr-diff br"></span>';
+const LINE_BREAK_WBR = '<wbr class="gr-diff"></wbr>';
+
const TAB = '<span class="" style=""></span>';
const TAB_IGNORE = ['class', 'style'];
@@ -39,6 +41,12 @@
await check('a'.repeat(20), `aaaaaaaaaa${LINE_BREAK}aaaaaaaaaa`);
});
+ test('renderText newlines 1 responsive', async () => {
+ element.isResponsive = true;
+ await check('abcdef', 'abcdef');
+ await check('a'.repeat(20), `aaaaaaaaaa${LINE_BREAK_WBR}aaaaaaaaaa`);
+ });
+
test('renderText newlines 2', async () => {
await check(
'<span class="thumbsup">👍</span>',
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor_test.ts
index b9db280..61f8551 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-cursor/gr-diff-cursor_test.ts
@@ -46,7 +46,6 @@
diff = createDiff();
diffElement.prefs = createDefaultDiffPrefs();
- diffElement.renderPrefs = {use_lit_components: true};
diffElement.diff = diff;
await promise;
});
@@ -661,7 +660,7 @@
// Goto second last line of the first diff
cursor.moveToLineNumber(lastLine - 1, Side.RIGHT);
assert.equal(
- cursor.getTargetLineElement()!.textContent,
+ cursor.getTargetLineElement()!.textContent?.trim(),
`${lastLine - 1}`
);
@@ -669,7 +668,7 @@
cursor.moveDown();
assert.equal(getTargetDiffIndex(), 0);
assert.equal(
- cursor.getTargetLineElement()!.textContent,
+ cursor.getTargetLineElement()!.textContent?.trim(),
lastLine.toString()
);
@@ -677,7 +676,7 @@
cursor.moveDown();
assert.equal(getTargetDiffIndex(), 0);
assert.equal(
- cursor.getTargetLineElement()!.textContent,
+ cursor.getTargetLineElement()!.textContent?.trim(),
lastLine.toString()
);
@@ -686,9 +685,10 @@
await waitForEventOnce(diffElements[1], 'render');
// Now we can go down
- cursor.moveDown();
+ cursor.moveDown(); // LOST
+ cursor.moveDown(); // FILE
assert.equal(getTargetDiffIndex(), 1);
- assert.equal(cursor.getTargetLineElement()!.textContent, 'File');
+ assert.equal(cursor.getTargetLineElement()!.textContent?.trim(), 'File');
});
});
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-selection/gr-diff-selection_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-selection/gr-diff-selection_test.ts
index 9e3d288..f216e04 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-selection/gr-diff-selection_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-selection/gr-diff-selection_test.ts
@@ -62,7 +62,6 @@
],
};
grDiff.prefs = createDefaultDiffPrefs();
- grDiff.renderPrefs = {use_lit_components: true};
grDiff.diff = diff;
await waitForEventOnce(grDiff, 'render');
assert.isOk(element.diffTable);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
index 59da20d..5a1ded2 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
@@ -132,8 +132,14 @@
const after = [];
for (const line of group.lines) {
if (
- (line.beforeNumber && line.beforeNumber < leftSplit) ||
- (line.afterNumber && line.afterNumber < rightSplit)
+ (line.beforeNumber &&
+ line.beforeNumber !== 'FILE' &&
+ line.beforeNumber !== 'LOST' &&
+ line.beforeNumber < leftSplit) ||
+ (line.afterNumber &&
+ line.afterNumber !== 'FILE' &&
+ line.afterNumber !== 'LOST' &&
+ line.afterNumber < rightSplit)
) {
before.push(line);
} else {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index 4adb1cf..227ac2d 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -68,18 +68,8 @@
);
});
- test('a unified diff legacy', async () => {
- element.viewMode = DiffViewMode.UNIFIED;
- await testUnified();
- });
-
test('a unified diff lit', async () => {
element.viewMode = DiffViewMode.UNIFIED;
- element.renderPrefs = {...element.renderPrefs, use_lit_components: true};
- await testUnified();
- });
-
- const testUnified = async () => {
element.prefs = {...MINIMAL_PREFS};
element.diff = createDiff();
await element.updateComplete;
@@ -1343,18 +1333,9 @@
],
}
);
- };
-
- test('a normal diff legacy', async () => {
- await testNormal();
});
test('a normal diff lit', async () => {
- element.renderPrefs = {...element.renderPrefs, use_lit_components: true};
- await testNormal();
- });
-
- const testNormal = async () => {
element.prefs = {...MINIMAL_PREFS};
element.diff = createDiff();
await element.updateComplete;
@@ -3008,7 +2989,7 @@
],
}
);
- };
+ });
});
suite('selectionchange event handling', () => {
@@ -3548,7 +3529,11 @@
await element.updateComplete;
const ROWS = 48;
const FILE_ROW = 1;
- assert.equal(element.getCursorStops().length, ROWS + FILE_ROW);
+ const LOST_ROW = 1;
+ assert.equal(
+ element.getCursorStops().length,
+ ROWS + FILE_ROW + LOST_ROW
+ );
});
test('returns an additional AbortStop when still loading', async () => {
@@ -3557,8 +3542,9 @@
await element.updateComplete;
const ROWS = 48;
const FILE_ROW = 1;
+ const LOST_ROW = 1;
const actual = element.getCursorStops();
- assert.equal(actual.length, ROWS + FILE_ROW + 1);
+ assert.equal(actual.length, ROWS + FILE_ROW + LOST_ROW + 1);
assert.isTrue(actual[actual.length - 1] instanceof AbortStop);
});
});
@@ -4050,13 +4036,13 @@
b: ['Non eram nescius, Brute, cum, quae summis ingeniis '],
},
];
- function assertDiffTableWithContent() {
+ function diffTableHasContent() {
assertIsDefined(element.diffTable);
const diffTable = element.diffTable;
- assert.isTrue(diffTable.innerText.includes(content[0].a?.[0] ?? ''));
+ return diffTable.innerText.includes(content[0].a?.[0] ?? '');
}
await setupSampleDiff({content});
- assertDiffTableWithContent();
+ await waitUntil(diffTableHasContent);
element.diff = {...element.diff!};
await element.updateComplete;
// immediately cleaned up
@@ -4066,7 +4052,7 @@
element.renderDiffTable();
await element.updateComplete;
// rendered again
- assertDiffTableWithContent();
+ await waitUntil(diffTableHasContent);
});
suite('selection test', () => {
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index ad5b217..b6b9de7 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -196,6 +196,11 @@
computeLatestPatchNumWithEdit(patchsets)
);
+ public readonly latestUploader$ = select(
+ this.change$,
+ change => change?.revisions[change.current_revision]?.uploader
+ );
+
/**
* Emits the current patchset number. If the route does not define the current
* patchset num, then this selector waits for the change to be defined and
diff --git a/polygerrit-ui/app/models/change/files-model.ts b/polygerrit-ui/app/models/change/files-model.ts
index 0683af0..b7ff16f 100644
--- a/polygerrit-ui/app/models/change/files-model.ts
+++ b/polygerrit-ui/app/models/change/files-model.ts
@@ -155,7 +155,8 @@
),
this.subscribeToFiles(
(psLeft, _) => {
- if (psLeft === PARENT || psLeft <= 0) return undefined;
+ if (psLeft === PARENT || (psLeft as PatchSetNumber) <= 0)
+ return undefined;
return {basePatchNum: PARENT, patchNum: psLeft as PatchSetNumber};
},
files => {
@@ -164,7 +165,8 @@
),
this.subscribeToFiles(
(psLeft, psRight) => {
- if (psLeft === PARENT || psLeft <= 0) return undefined;
+ if (psLeft === PARENT || (psLeft as PatchSetNumber) <= 0)
+ return undefined;
return {basePatchNum: PARENT, patchNum: psRight as PatchSetNumber};
},
files => {
diff --git a/polygerrit-ui/app/models/checks/checks-model_test.ts b/polygerrit-ui/app/models/checks/checks-model_test.ts
index 35a8465..da4e3f1 100644
--- a/polygerrit-ui/app/models/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/models/checks/checks-model_test.ts
@@ -139,9 +139,6 @@
// emits at 'trailing' of throttle interval
assert.equal(fetchSpy.callCount, 1);
- // 600 ms is greater than the 500 ms throttle time.
- clock.tick(600);
-
model.reload('test-plugin');
model.reload('test-plugin');
model.reload('test-plugin');
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index 8c642c1..e8d715e 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -472,7 +472,11 @@
);
}
value.isSingleAttempt = false;
- if (run.attempt > value.latestAttempt) {
+ if (
+ value.latestAttempt !== 'all' &&
+ value.latestAttempt !== 'latest' &&
+ run.attempt > value.latestAttempt
+ ) {
value.latestAttempt = run.attempt;
}
value.attempts.push(detail);
diff --git a/polygerrit-ui/app/package.json b/polygerrit-ui/app/package.json
index 32aff91..edf0206 100644
--- a/polygerrit-ui/app/package.json
+++ b/polygerrit-ui/app/package.json
@@ -42,7 +42,7 @@
"polymer-bridges": "file:../../polymer-bridges/",
"polymer-resin": "^2.0.1",
"resemblejs": "^4.0.0",
- "rxjs": "^7.8.0",
+ "rxjs": "^6.6.7",
"safevalues": "^0.3.1",
"web-vitals": "^3.0.0"
},
diff --git a/polygerrit-ui/app/services/flags/flags.ts b/polygerrit-ui/app/services/flags/flags.ts
index 53f589a..973960e 100644
--- a/polygerrit-ui/app/services/flags/flags.ts
+++ b/polygerrit-ui/app/services/flags/flags.ts
@@ -19,6 +19,5 @@
PUSH_NOTIFICATIONS_DEVELOPER = 'UiFeature__push_notifications_developer',
PUSH_NOTIFICATIONS = 'UiFeature__push_notifications',
SUGGEST_EDIT = 'UiFeature__suggest_edit',
- REBASE_ON_BEHALF_OF_UPLOADER = 'UiFeature__rebase_on_behalf_of_uploader',
COMMENTS_CHIPS_IN_FILE_LIST = 'UiFeature__comments_chips_in_file_list',
}
diff --git a/polygerrit-ui/app/yarn.lock b/polygerrit-ui/app/yarn.lock
index c021046..e056a35 100644
--- a/polygerrit-ui/app/yarn.lock
+++ b/polygerrit-ui/app/yarn.lock
@@ -846,12 +846,12 @@
dependencies:
glob "^7.1.3"
-rxjs@^7.8.0:
- version "7.8.0"
- resolved "https://registry.yarnpkg.com/rxjs/-/rxjs-7.8.0.tgz#90a938862a82888ff4c7359811a595e14e1e09a4"
- integrity sha512-F2+gxDshqmIub1KdvZkaEfGDwLNpPvk9Fs6LD/MyQxNgMds/WH9OdDDXOmxUZpME+iSK3rQCctkL0DYyytUqMg==
+rxjs@^6.6.7:
+ version "6.6.7"
+ resolved "https://registry.yarnpkg.com/rxjs/-/rxjs-6.6.7.tgz#90ac018acabf491bf65044235d5863c4dab804c9"
+ integrity sha512-hTdwr+7yYNIT5n4AMYp85KA6yw2Va0FLa3Rguvbpa4W3I5xynaBZo41cM3XM+4Q6fRMj3sBYIR1VAmZMXYJvRQ==
dependencies:
- tslib "^2.1.0"
+ tslib "^1.9.0"
safe-buffer@~5.1.0, safe-buffer@~5.1.1:
version "5.1.2"
@@ -954,10 +954,10 @@
resolved "https://registry.yarnpkg.com/tr46/-/tr46-0.0.3.tgz#8184fd347dac9cdc185992f3a6622e14b9d9ab6a"
integrity sha1-gYT9NH2snNwYWZLzpmIuFLnZq2o=
-tslib@^2.1.0:
- version "2.5.0"
- resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.5.0.tgz#42bfed86f5787aeb41d031866c8f402429e0fddf"
- integrity sha512-336iVw3rtn2BUK7ORdIAHTyxHGRIHVReokCR3XjbckJMK7ms8FysBfhLR8IXnAgy7T0PTPNBWKiH514FOW/WSg==
+tslib@^1.9.0:
+ version "1.14.1"
+ resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.14.1.tgz#cf2d38bdc34a134bcaf1091c41f6619e2f672d00"
+ integrity sha512-Xni35NKzjgMrwevysHTCArtLDpPvye8zV/0E4EyYn43P7/7qvQwPh9BGkHewbMulVntbigmcT7rdX3BNo9wRJg==
util-deprecate@~1.0.1:
version "1.0.2"
diff --git a/resources/com/google/gerrit/server/config/CapabilityConstants.properties b/resources/com/google/gerrit/server/config/CapabilityConstants.properties
index 1a355eb..b6aca6f 100644
--- a/resources/com/google/gerrit/server/config/CapabilityConstants.properties
+++ b/resources/com/google/gerrit/server/config/CapabilityConstants.properties
@@ -21,3 +21,4 @@
viewConnections = View Connections
viewPlugins = View Plugins
viewQueue = View Queue
+viewSecondaryEmails = View Secondary Emails
