Merge "Split up the api/revision acceptance tests"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 25d6518..3bb6564 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -7427,24 +7427,24 @@
[options="header",cols="1,^1,5"]
|============================
-|Field Name ||Description
-|`message` |optional|
+|Field Name ||Description
+|`message` |optional|
The message to be added as review comment.
-|`tag` |optional|
+|`tag` |optional|
Apply this tag to the review comment message, votes, and inline
comments. Tags may be used by CI or other automated systems to
distinguish them from human reviews. Votes/comments that contain `tag` with
'autogenerated:' prefix can be filtered out in the web UI.
-|`labels` |optional|
+|`labels` |optional|
The votes that should be added to the revision as a map that maps the
label names to the voting values.
-|`comments` |optional|
+|`comments` |optional|
The comments that should be added as a map that maps a file path to a
list of link:#comment-input[CommentInput] entities.
-|`robot_comments` |optional|
+|`robot_comments` |optional|
The robot comments that should be added as a map that maps a file path
to a list of link:#robot-comment-input[RobotCommentInput] entities.
-|`drafts` |optional|
+|`drafts` |optional|
Draft handling that defines how draft comments are handled that are
already in the database but that were not also described in this
input. +
@@ -7453,36 +7453,36 @@
Only `KEEP` is allowed when used in conjunction with `on_behalf_of`. +
If not set, the default is `KEEP`. If `on_behalf_of` is set, then no other value
besides `KEEP` is allowed.
-|`notify` |optional|
+|`notify` |optional|
Notify handling that defines to whom email notifications should be sent
after the review is stored. +
Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. +
If not set, the default is `ALL`.
-|`notify_details` |optional|
+|`notify_details` |optional|
Additional information about whom to notify about the update as a map
of recipient type to link:#notify-info[NotifyInfo] entity.
-|`omit_duplicate_comments` |optional|
+|`omit_duplicate_comments` |optional|
If `true`, comments with the same content at the same place will be omitted.
-|`on_behalf_of` |optional|
+|`on_behalf_of` |optional|
link:rest-api-accounts.html#account-id[\{account-id\}] the review
should be posted on behalf of. To use this option the caller must
have been granted `labelAs-NAME` permission for all keys of labels.
-|`reviewers` |optional|
+|`reviewers` |optional|
A list of link:rest-api-changes.html#reviewer-input[ReviewerInput]
representing reviewers that should be added to the change.
-|`ready` |optional|
+|`ready` |optional|
If true, and if the change is work in progress, then start review.
It is an error for both `ready` and `work_in_progress` to be true.
-|`work_in_progress` |optional|
+|`work_in_progress` |optional|
If true, mark the change as work in progress. It is an error for both
`ready` and `work_in_progress` to be true.
-|`add_to_attention_set` |optional|
+|`add_to_attention_set` |optional|
list of link:#attention-set-input[AttentionSetInput] entities to add
to the link:#attention-set[attention set].
-remove_from_attention_set` |optional|
+|`remove_from_attention_set` |optional|
list of link:#attention-set-input[AttentionSetInput] entities to remove
from the link:#attention-set[attention set].
-ignore_default_attention_set_rules`|optional|
+|`ignore_default_attention_set_rules`|optional|
If set to true, ignore all default attention set rules described in the
link:#attention-set[attention set]. Updates in add_to_attention_set
and remove_from_attention_set are not ignored.
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
index de83cff..89269b6 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
@@ -166,36 +166,39 @@
private void addCapabilities(
ProjectConfig projectConfig, ImmutableList<TestCapability> addedCapabilities) {
for (TestCapability c : addedCapabilities) {
- PermissionRule rule = newRule(projectConfig, c.group());
+ PermissionRule.Builder rule = newRule(projectConfig, c.group());
rule.setRange(c.min(), c.max());
projectConfig
.getAccessSection(AccessSection.GLOBAL_CAPABILITIES, true)
.getPermission(c.name(), true)
- .add(rule);
+ .add(rule.build());
}
}
private void addPermissions(
ProjectConfig projectConfig, ImmutableList<TestPermission> addedPermissions) {
for (TestPermission p : addedPermissions) {
- PermissionRule rule = newRule(projectConfig, p.group());
+ PermissionRule.Builder rule = newRule(projectConfig, p.group());
rule.setAction(p.action());
rule.setForce(p.force());
- projectConfig.getAccessSection(p.ref(), true).getPermission(p.name(), true).add(rule);
+ projectConfig
+ .getAccessSection(p.ref(), true)
+ .getPermission(p.name(), true)
+ .add(rule.build());
}
}
private void addLabelPermissions(
ProjectConfig projectConfig, ImmutableList<TestLabelPermission> addedLabelPermissions) {
for (TestLabelPermission p : addedLabelPermissions) {
- PermissionRule rule = newRule(projectConfig, p.group());
+ PermissionRule.Builder rule = newRule(projectConfig, p.group());
rule.setAction(p.action());
rule.setRange(p.min(), p.max());
String permissionName =
p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name());
Permission permission =
projectConfig.getAccessSection(p.ref(), true).getPermission(permissionName, true);
- permission.add(rule);
+ permission.add(rule.build());
}
}
@@ -324,9 +327,10 @@
}
}
- private static PermissionRule newRule(ProjectConfig project, AccountGroup.UUID groupUUID) {
+ private static PermissionRule.Builder newRule(
+ ProjectConfig project, AccountGroup.UUID groupUUID) {
GroupReference group = GroupReference.create(groupUUID, groupUUID.get());
group = project.resolve(group);
- return new PermissionRule(group);
+ return PermissionRule.builder(group);
}
}
diff --git a/java/com/google/gerrit/common/data/ContributorAgreement.java b/java/com/google/gerrit/common/data/ContributorAgreement.java
index bc106f0..ed05203 100644
--- a/java/com/google/gerrit/common/data/ContributorAgreement.java
+++ b/java/com/google/gerrit/common/data/ContributorAgreement.java
@@ -14,98 +14,67 @@
package com.google.gerrit.common.data;
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Project;
-import java.util.ArrayList;
-import java.util.List;
/** Portion of a {@link Project} describing a single contributor agreement. */
-public class ContributorAgreement implements Comparable<ContributorAgreement> {
- protected String name;
- protected String description;
- protected List<PermissionRule> accepted;
- protected GroupReference autoVerify;
- protected String agreementUrl;
- protected List<String> excludeProjectsRegexes;
- protected List<String> matchProjectsRegexes;
+@AutoValue
+public abstract class ContributorAgreement implements Comparable<ContributorAgreement> {
+ public abstract String getName();
- protected ContributorAgreement() {}
+ @Nullable
+ public abstract String getDescription();
- public ContributorAgreement(String name) {
- setName(name);
- }
+ public abstract ImmutableList<PermissionRule> getAccepted();
- public String getName() {
- return name;
- }
+ @Nullable
+ public abstract GroupReference getAutoVerify();
- public void setName(String name) {
- this.name = name;
- }
+ @Nullable
+ public abstract String getAgreementUrl();
- public String getDescription() {
- return description;
- }
+ public abstract ImmutableList<String> getExcludeProjectsRegexes();
- public void setDescription(String description) {
- this.description = description;
- }
+ public abstract ImmutableList<String> getMatchProjectsRegexes();
- public List<PermissionRule> getAccepted() {
- if (accepted == null) {
- accepted = new ArrayList<>();
- }
- return accepted;
- }
-
- public void setAccepted(List<PermissionRule> accepted) {
- this.accepted = accepted;
- }
-
- public GroupReference getAutoVerify() {
- return autoVerify;
- }
-
- public void setAutoVerify(GroupReference autoVerify) {
- this.autoVerify = autoVerify;
- }
-
- public String getAgreementUrl() {
- return agreementUrl;
- }
-
- public void setAgreementUrl(String agreementUrl) {
- this.agreementUrl = agreementUrl;
- }
-
- public List<String> getExcludeProjectsRegexes() {
- if (excludeProjectsRegexes == null) {
- excludeProjectsRegexes = new ArrayList<>();
- }
- return excludeProjectsRegexes;
- }
-
- public void setExcludeProjectsRegexes(List<String> excludeProjectsRegexes) {
- this.excludeProjectsRegexes = excludeProjectsRegexes;
- }
-
- public List<String> getMatchProjectsRegexes() {
- if (matchProjectsRegexes == null) {
- matchProjectsRegexes = new ArrayList<>();
- }
- return matchProjectsRegexes;
- }
-
- public void setMatchProjectsRegexes(List<String> matchProjectsRegexes) {
- this.matchProjectsRegexes = matchProjectsRegexes;
+ public static ContributorAgreement.Builder builder(String name) {
+ return new AutoValue_ContributorAgreement.Builder()
+ .setName(name)
+ .setAccepted(ImmutableList.of())
+ .setExcludeProjectsRegexes(ImmutableList.of())
+ .setMatchProjectsRegexes(ImmutableList.of());
}
@Override
- public int compareTo(ContributorAgreement o) {
+ public final int compareTo(ContributorAgreement o) {
return getName().compareTo(o.getName());
}
@Override
- public String toString() {
+ public final String toString() {
return "ContributorAgreement[" + getName() + "]";
}
+
+ public abstract Builder toBuilder();
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder setName(String name);
+
+ public abstract Builder setDescription(@Nullable String description);
+
+ public abstract Builder setAccepted(ImmutableList<PermissionRule> accepted);
+
+ public abstract Builder setAutoVerify(@Nullable GroupReference autoVerify);
+
+ public abstract Builder setAgreementUrl(@Nullable String agreementUrl);
+
+ public abstract Builder setExcludeProjectsRegexes(ImmutableList<String> excludeProjectsRegexes);
+
+ public abstract Builder setMatchProjectsRegexes(ImmutableList<String> matchProjectsRegexes);
+
+ public abstract ContributorAgreement build();
+ }
}
diff --git a/java/com/google/gerrit/common/data/Permission.java b/java/com/google/gerrit/common/data/Permission.java
index 9b86b7e..5aaffba 100644
--- a/java/com/google/gerrit/common/data/Permission.java
+++ b/java/com/google/gerrit/common/data/Permission.java
@@ -207,7 +207,7 @@
}
if (create) {
- PermissionRule r = new PermissionRule(group);
+ PermissionRule r = PermissionRule.create(group);
rules.add(r);
return r;
}
@@ -218,7 +218,8 @@
for (PermissionRule srcRule : src.getRules()) {
PermissionRule dstRule = getRule(srcRule.getGroup());
if (dstRule != null) {
- dstRule.mergeFrom(srcRule);
+ rules.remove(dstRule);
+ rules.add(PermissionRule.merge(srcRule, dstRule));
} else {
add(srcRule);
}
diff --git a/java/com/google/gerrit/common/data/PermissionRule.java b/java/com/google/gerrit/common/data/PermissionRule.java
index ce94695..c543769 100644
--- a/java/com/google/gerrit/common/data/PermissionRule.java
+++ b/java/com/google/gerrit/common/data/PermissionRule.java
@@ -14,10 +14,10 @@
package com.google.gerrit.common.data;
-public class PermissionRule implements Comparable<PermissionRule> {
- public static final String FORCE_PUSH = "Force Push";
- public static final String FORCE_EDIT = "Force Edit";
+import com.google.auto.value.AutoValue;
+@AutoValue
+public abstract class PermissionRule implements Comparable<PermissionRule> {
public enum Action {
ALLOW,
DENY,
@@ -27,102 +27,57 @@
BATCH
}
- protected Action action = Action.ALLOW;
- protected boolean force;
- protected int min;
- protected int max;
- protected GroupReference group;
+ public abstract Action getAction();
- public PermissionRule() {}
+ public abstract boolean getForce();
- public PermissionRule(GroupReference group) {
- this.group = group;
+ public abstract int getMin();
+
+ public abstract int getMax();
+
+ public abstract GroupReference getGroup();
+
+ public static PermissionRule.Builder builder(GroupReference group) {
+ return builder().setGroup(group);
}
- public Action getAction() {
- return action;
+ public static PermissionRule create(GroupReference group) {
+ return builder().setGroup(group).build();
}
- public void setAction(Action action) {
- if (action == null) {
- throw new NullPointerException("action");
- }
- this.action = action;
+ protected static Builder builder() {
+ return new AutoValue_PermissionRule.Builder()
+ .setMin(0)
+ .setMax(0)
+ .setAction(Action.ALLOW)
+ .setForce(false);
}
- public boolean isDeny() {
- return action == Action.DENY;
- }
+ static PermissionRule merge(PermissionRule src, PermissionRule dest) {
+ PermissionRule.Builder result = dest.toBuilder();
+ if (dest.getAction() != src.getAction()) {
+ if (dest.getAction() == Action.BLOCK || src.getAction() == Action.BLOCK) {
+ result.setAction(Action.BLOCK);
- public void setDeny() {
- action = Action.DENY;
- }
+ } else if (dest.getAction() == Action.DENY || src.getAction() == Action.DENY) {
+ result.setAction(Action.DENY);
- public boolean isBlock() {
- return action == Action.BLOCK;
- }
-
- public void setBlock() {
- action = Action.BLOCK;
- }
-
- public boolean getForce() {
- return force;
- }
-
- public void setForce(boolean newForce) {
- force = newForce;
- }
-
- public int getMin() {
- return min;
- }
-
- public void setMin(int min) {
- this.min = min;
- }
-
- public void setMax(int max) {
- this.max = max;
- }
-
- public int getMax() {
- return max;
- }
-
- public void setRange(int newMin, int newMax) {
- if (newMax < newMin) {
- min = newMax;
- max = newMin;
- } else {
- min = newMin;
- max = newMax;
- }
- }
-
- public GroupReference getGroup() {
- return group;
- }
-
- public void setGroup(GroupReference newGroup) {
- group = newGroup;
- }
-
- void mergeFrom(PermissionRule src) {
- if (getAction() != src.getAction()) {
- if (getAction() == Action.BLOCK || src.getAction() == Action.BLOCK) {
- setAction(Action.BLOCK);
-
- } else if (getAction() == Action.DENY || src.getAction() == Action.DENY) {
- setAction(Action.DENY);
-
- } else if (getAction() == Action.BATCH || src.getAction() == Action.BATCH) {
- setAction(Action.BATCH);
+ } else if (dest.getAction() == Action.BATCH || src.getAction() == Action.BATCH) {
+ result.setAction(Action.BATCH);
}
}
- setForce(getForce() || src.getForce());
- setRange(Math.min(getMin(), src.getMin()), Math.max(getMax(), src.getMax()));
+ result.setForce(dest.getForce() || src.getForce());
+ result.setRange(Math.min(dest.getMin(), src.getMin()), Math.max(dest.getMax(), src.getMax()));
+ return result.build();
+ }
+
+ public boolean isDeny() {
+ return getAction() == Action.DENY;
+ }
+
+ public boolean isBlock() {
+ return getAction() == Action.BLOCK;
}
@Override
@@ -159,7 +114,7 @@
}
@Override
- public String toString() {
+ public final String toString() {
return asString(true);
}
@@ -211,7 +166,7 @@
public static PermissionRule fromString(String src, boolean mightUseRange) {
final String orig = src;
- final PermissionRule rule = new PermissionRule();
+ final PermissionRule.Builder rule = PermissionRule.builder();
src = src.trim();
@@ -261,7 +216,7 @@
throw new IllegalArgumentException("Rule must include group: " + orig);
}
- return rule;
+ return rule.build();
}
public boolean hasRange() {
@@ -275,21 +230,39 @@
return Integer.parseInt(value);
}
- @Override
- public boolean equals(Object obj) {
- if (!(obj instanceof PermissionRule)) {
- return false;
- }
- final PermissionRule other = (PermissionRule) obj;
- return action.equals(other.action)
- && force == other.force
- && min == other.min
- && max == other.max
- && group.equals(other.group);
- }
+ public abstract Builder toBuilder();
- @Override
- public int hashCode() {
- return group.hashCode();
+ @AutoValue.Builder
+ public abstract static class Builder {
+ public Builder setDeny() {
+ return setAction(Action.DENY);
+ }
+
+ public Builder setBlock() {
+ return setAction(Action.BLOCK);
+ }
+
+ public Builder setRange(int newMin, int newMax) {
+ if (newMax < newMin) {
+ setMin(newMax);
+ setMax(newMin);
+ } else {
+ setMin(newMin);
+ setMax(newMax);
+ }
+ return this;
+ }
+
+ public abstract Builder setAction(Action action);
+
+ public abstract Builder setGroup(GroupReference groupReference);
+
+ public abstract Builder setForce(boolean newForce);
+
+ public abstract Builder setMin(int min);
+
+ public abstract Builder setMax(int max);
+
+ public abstract PermissionRule build();
}
}
diff --git a/java/com/google/gerrit/server/account/CapabilityCollection.java b/java/com/google/gerrit/server/account/CapabilityCollection.java
index b52d616..157f946 100644
--- a/java/com/google/gerrit/server/account/CapabilityCollection.java
+++ b/java/com/google/gerrit/server/account/CapabilityCollection.java
@@ -111,7 +111,7 @@
List<PermissionRule> r = new ArrayList<>(admins.size() + rules.size());
for (GroupReference g : admins) {
- r.add(new PermissionRule(g));
+ r.add(PermissionRule.create(g));
}
for (PermissionRule rule : rules) {
if (!admins.contains(rule.getGroup())) {
@@ -142,9 +142,9 @@
if (doesNotDeclare(section, capName)) {
PermissionRange.WithDefaults range = GlobalCapability.getRange(capName);
if (range != null) {
- PermissionRule rule = new PermissionRule(group);
+ PermissionRule.Builder rule = PermissionRule.builder(group);
rule.setRange(range.getDefaultMin(), range.getDefaultMax());
- out.put(capName, Collections.singletonList(rule));
+ out.put(capName, Collections.singletonList(rule.build()));
}
}
}
diff --git a/java/com/google/gerrit/server/project/GroupList.java b/java/com/google/gerrit/server/project/GroupList.java
index 7237bb6..9b65413 100644
--- a/java/com/google/gerrit/server/project/GroupList.java
+++ b/java/com/google/gerrit/server/project/GroupList.java
@@ -74,7 +74,7 @@
public GroupReference byName(String name) {
return byUUID.entrySet().stream()
.map(Map.Entry::getValue)
- .filter(groupReference -> name.equals(groupReference.getName()))
+ .filter(groupReference -> groupReference.getName().equals(name))
.findAny()
.orElse(null);
}
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 35257ef..cc13606 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -444,25 +444,18 @@
public void replace(AccessSection section) {
for (Permission permission : section.getPermissions()) {
+ ImmutableList.Builder<PermissionRule> newRules = ImmutableList.builder();
for (PermissionRule rule : permission.getRules()) {
- rule.setGroup(resolve(rule.getGroup()));
+ newRules.add(rule.toBuilder().setGroup(resolve(rule.getGroup())).build());
}
+ permission.setRules(newRules.build());
}
accessSections.put(section.getName(), section);
}
public ContributorAgreement getContributorAgreement(String name) {
- return getContributorAgreement(name, false);
- }
-
- public ContributorAgreement getContributorAgreement(String name, boolean create) {
- ContributorAgreement ca = contributorAgreements.get(name);
- if (ca == null && create) {
- ca = new ContributorAgreement(name);
- contributorAgreements.put(name, ca);
- }
- return ca;
+ return contributorAgreements.get(name);
}
public Collection<ContributorAgreement> getContributorAgreements() {
@@ -476,12 +469,15 @@
}
public void replace(ContributorAgreement section) {
- section.setAutoVerify(resolve(section.getAutoVerify()));
+ ContributorAgreement.Builder ca = section.toBuilder();
+ ca.setAutoVerify(resolve(section.getAutoVerify()));
+ ImmutableList.Builder<PermissionRule> newRules = ImmutableList.builder();
for (PermissionRule rule : section.getAccepted()) {
- rule.setGroup(resolve(rule.getGroup()));
+ newRules.add(rule.toBuilder().setGroup(resolve(rule.getGroup())).build());
}
+ ca.setAccepted(newRules.build());
- contributorAgreements.put(section.getName(), section);
+ contributorAgreements.put(section.getName(), ca.build());
}
public Collection<NotifyConfig> getNotifyConfigs() {
@@ -510,6 +506,12 @@
upsertLabelType(builder.build());
}
+ /** Adds or replaces the given {@link ContributorAgreement} in this config. */
+ public void upsertContributorAgreement(ContributorAgreement ca) {
+ contributorAgreements.remove(ca.getName());
+ contributorAgreements.put(ca.getName(), ca);
+ }
+
public Collection<StoredCommentLinkInfo> getCommentLinkSections() {
return commentLinkSections.values();
}
@@ -680,7 +682,7 @@
private void loadContributorAgreements(Config rc) {
contributorAgreements = new HashMap<>();
for (String name : rc.getSubsections(CONTRIBUTOR_AGREEMENT)) {
- ContributorAgreement ca = getContributorAgreement(name, true);
+ ContributorAgreement.Builder ca = ContributorAgreement.builder(name);
ca.setDescription(rc.getString(CONTRIBUTOR_AGREEMENT, name, KEY_DESCRIPTION));
ca.setAgreementUrl(rc.getString(CONTRIBUTOR_AGREEMENT, name, KEY_AGREEMENT_URL));
ca.setAccepted(loadPermissionRules(rc, CONTRIBUTOR_AGREEMENT, name, KEY_ACCEPTED, false));
@@ -717,6 +719,7 @@
} else {
ca.setAutoVerify(rules.get(0).getGroup());
}
+ contributorAgreements.put(name, ca.build());
}
}
@@ -913,8 +916,7 @@
PROJECT_CONFIG, "group \"" + ref.getName() + "\" not in " + GroupList.FILE_NAME));
}
- rule.setGroup(ref);
- perm.add(rule);
+ perm.add(rule.toBuilder().setGroup(ref).build());
}
}
@@ -1288,7 +1290,7 @@
if (ca.getAutoVerify().getUUID() != null) {
keepGroups.add(ca.getAutoVerify().getUUID());
}
- String autoVerify = new PermissionRule(ca.getAutoVerify()).asString(false);
+ String autoVerify = PermissionRule.create(ca.getAutoVerify()).asString(false);
set(rc, CONTRIBUTOR_AGREEMENT, ca.getName(), KEY_AUTO_VERIFY, autoVerify);
} else {
rc.unset(CONTRIBUTOR_AGREEMENT, ca.getName(), KEY_AUTO_VERIFY);
@@ -1321,7 +1323,7 @@
.forEach(keepGroups::add);
List<String> email =
nc.getGroups().stream()
- .map(gr -> new PermissionRule(gr).asString(false))
+ .map(gr -> PermissionRule.create(gr).asString(false))
.sorted()
.collect(toList());
diff --git a/java/com/google/gerrit/server/project/ProjectCreator.java b/java/com/google/gerrit/server/project/ProjectCreator.java
index 6ffbdef..5ec4a58 100644
--- a/java/com/google/gerrit/server/project/ProjectCreator.java
+++ b/java/com/google/gerrit/server/project/ProjectCreator.java
@@ -184,7 +184,7 @@
GroupDescription.Basic g = groupBackend.get(ownerId);
if (g != null) {
GroupReference group = config.resolve(GroupReference.forGroup(g));
- all.getPermission(Permission.OWNER, true).add(new PermissionRule(group));
+ all.getPermission(Permission.OWNER, true).add(PermissionRule.create(group));
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java b/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
index 390dea9..0f7bc3d 100644
--- a/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
+++ b/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
@@ -99,7 +99,7 @@
}
PermissionRuleInfo pri = permissionRuleInfoEntry.getValue();
- PermissionRule r = new PermissionRule(GroupReference.forGroup(group));
+ PermissionRule.Builder r = PermissionRule.builder(GroupReference.forGroup(group));
if (pri != null) {
if (pri.max != null) {
r.setMax(pri.max);
@@ -114,7 +114,7 @@
r.setForce(pri.force);
}
}
- p.add(r);
+ p.add(r.build());
}
accessSection.addPermission(p);
}
diff --git a/java/com/google/gerrit/server/schema/AclUtil.java b/java/com/google/gerrit/server/schema/AclUtil.java
index f6c3aad..ed4a6ec 100644
--- a/java/com/google/gerrit/server/schema/AclUtil.java
+++ b/java/com/google/gerrit/server/schema/AclUtil.java
@@ -53,9 +53,7 @@
}
for (GroupReference group : groupList) {
if (group != null) {
- PermissionRule r = rule(config, group);
- r.setForce(force);
- p.add(r);
+ p.add(rule(config, group).setForce(force).build());
}
}
}
@@ -65,9 +63,7 @@
Permission p = section.getPermission(permission, true);
for (GroupReference group : groupList) {
if (group != null) {
- PermissionRule r = rule(config, group);
- r.setBlock();
- p.add(r);
+ p.add(rule(config, group).setBlock().build());
}
}
}
@@ -95,15 +91,13 @@
p.setExclusiveGroup(exclusive);
for (GroupReference group : groupList) {
if (group != null) {
- PermissionRule r = rule(config, group);
- r.setRange(min, max);
- p.add(r);
+ p.add(rule(config, group).setRange(min, max).build());
}
}
}
- public static PermissionRule rule(ProjectConfig config, GroupReference group) {
- return new PermissionRule(config.resolve(group));
+ public static PermissionRule.Builder rule(ProjectConfig config, GroupReference group) {
+ return PermissionRule.builder(config.resolve(group));
}
public static void remove(
@@ -111,8 +105,7 @@
Permission p = section.getPermission(permission, true);
for (GroupReference group : groupList) {
if (group != null) {
- PermissionRule r = rule(config, group);
- p.remove(r);
+ p.remove(rule(config, group).build());
}
}
}
diff --git a/java/com/google/gerrit/server/schema/AllProjectsCreator.java b/java/com/google/gerrit/server/schema/AllProjectsCreator.java
index 018a96a..908ce0a 100644
--- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java
+++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java
@@ -28,7 +28,6 @@
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
-import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.GerritPersonIdent;
@@ -183,12 +182,10 @@
private void initDefaultAclsForBatchUsers(
AccessSection capabilities, ProjectConfig config, GroupReference batchUsersGroup) {
Permission priority = capabilities.getPermission(GlobalCapability.PRIORITY, true);
- PermissionRule r = rule(config, batchUsersGroup);
- r.setAction(Action.BATCH);
- priority.add(r);
+ priority.add(rule(config, batchUsersGroup).setAction(Action.BATCH).build());
Permission stream = capabilities.getPermission(GlobalCapability.STREAM_EVENTS, true);
- stream.add(rule(config, batchUsersGroup));
+ stream.add(rule(config, batchUsersGroup).build());
}
private void initDefaultAclsForAdmins(
diff --git a/java/com/google/gerrit/server/schema/ProjectConfigSchemaUpdate.java b/java/com/google/gerrit/server/schema/ProjectConfigSchemaUpdate.java
index 5e7dbf0..21ce1d1 100644
--- a/java/com/google/gerrit/server/schema/ProjectConfigSchemaUpdate.java
+++ b/java/com/google/gerrit/server/schema/ProjectConfigSchemaUpdate.java
@@ -98,7 +98,7 @@
r -> {
PermissionRule rule = PermissionRule.fromString(r, false);
if (rule.getForce()) {
- rule.setForce(false);
+ rule = rule.toBuilder().setForce(false).build();
updated = true;
}
return rule.asString(false);
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java
index c0a9da6..23fdc0b 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java
@@ -76,21 +76,21 @@
protected ContributorAgreement configureContributorAgreement(boolean autoVerify)
throws Exception {
- ContributorAgreement ca;
+ ContributorAgreement.Builder ca;
String name = autoVerify ? "cla-test-group" : "cla-test-no-auto-verify-group";
AccountGroup.UUID g = groupOperations.newGroup().name(name).create();
GroupApi groupApi = gApi.groups().id(g.get());
groupApi.description("CLA test group");
InternalGroup caGroup = group(AccountGroup.uuid(groupApi.detail().id));
GroupReference groupRef = GroupReference.create(caGroup.getGroupUUID(), caGroup.getName());
- PermissionRule rule = new PermissionRule(groupRef);
- rule.setAction(PermissionRule.Action.ALLOW);
+ PermissionRule rule =
+ PermissionRule.builder(groupRef).setAction(PermissionRule.Action.ALLOW).build();
if (autoVerify) {
- ca = new ContributorAgreement("cla-test");
+ ca = ContributorAgreement.builder("cla-test");
ca.setAutoVerify(groupRef);
ca.setAccepted(ImmutableList.of(rule));
} else {
- ca = new ContributorAgreement("cla-test-no-auto-verify");
+ ca = ContributorAgreement.builder("cla-test-no-auto-verify");
}
ca.setDescription("description");
ca.setAgreementUrl("agreement-url");
@@ -98,9 +98,10 @@
ca.setExcludeProjectsRegexes(ImmutableList.of("ExcludedProject"));
try (ProjectConfigUpdate u = updateProject(allProjects)) {
- u.getConfig().replace(ca);
+ ContributorAgreement contributorAgreement = ca.build();
+ u.getConfig().replace(contributorAgreement);
u.save();
- return ca;
+ return contributorAgreement;
}
}
diff --git a/javatests/com/google/gerrit/common/data/PermissionRuleTest.java b/javatests/com/google/gerrit/common/data/PermissionRuleTest.java
index 6dc357c..d193b09 100644
--- a/javatests/com/google/gerrit/common/data/PermissionRuleTest.java
+++ b/javatests/com/google/gerrit/common/data/PermissionRuleTest.java
@@ -15,7 +15,6 @@
package com.google.gerrit.common.data;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.gerrit.common.data.PermissionRule.Action;
import com.google.gerrit.entities.AccountGroup;
@@ -29,140 +28,33 @@
@Before
public void setup() {
this.groupReference = GroupReference.create(AccountGroup.uuid("uuid"), "group");
- this.permissionRule = new PermissionRule(groupReference);
- }
-
- @Test
- public void getAndSetAction() {
- assertThat(permissionRule.getAction()).isEqualTo(Action.ALLOW);
-
- permissionRule.setAction(Action.DENY);
- assertThat(permissionRule.getAction()).isEqualTo(Action.DENY);
- }
-
- @Test
- public void cannotSetActionToNull() {
- assertThrows(NullPointerException.class, () -> permissionRule.setAction(null));
- }
-
- @Test
- public void setDeny() {
- assertThat(permissionRule.isDeny()).isFalse();
-
- permissionRule.setDeny();
- assertThat(permissionRule.isDeny()).isTrue();
- }
-
- @Test
- public void setBlock() {
- assertThat(permissionRule.isBlock()).isFalse();
-
- permissionRule.setBlock();
- assertThat(permissionRule.isBlock()).isTrue();
- }
-
- @Test
- public void setForce() {
- assertThat(permissionRule.getForce()).isFalse();
-
- permissionRule.setForce(true);
- assertThat(permissionRule.getForce()).isTrue();
-
- permissionRule.setForce(false);
- assertThat(permissionRule.getForce()).isFalse();
- }
-
- @Test
- public void setMin() {
- assertThat(permissionRule.getMin()).isEqualTo(0);
-
- permissionRule.setMin(-2);
- assertThat(permissionRule.getMin()).isEqualTo(-2);
-
- permissionRule.setMin(2);
- assertThat(permissionRule.getMin()).isEqualTo(2);
- }
-
- @Test
- public void setMax() {
- assertThat(permissionRule.getMax()).isEqualTo(0);
-
- permissionRule.setMax(2);
- assertThat(permissionRule.getMax()).isEqualTo(2);
-
- permissionRule.setMax(-2);
- assertThat(permissionRule.getMax()).isEqualTo(-2);
- }
-
- @Test
- public void setRange() {
- assertThat(permissionRule.getMin()).isEqualTo(0);
- assertThat(permissionRule.getMax()).isEqualTo(0);
-
- permissionRule.setRange(-2, 2);
- assertThat(permissionRule.getMin()).isEqualTo(-2);
- assertThat(permissionRule.getMax()).isEqualTo(2);
-
- permissionRule.setRange(2, -2);
- assertThat(permissionRule.getMin()).isEqualTo(-2);
- assertThat(permissionRule.getMax()).isEqualTo(2);
-
- permissionRule.setRange(1, 1);
- assertThat(permissionRule.getMin()).isEqualTo(1);
- assertThat(permissionRule.getMax()).isEqualTo(1);
- }
-
- @Test
- public void hasRange() {
- assertThat(permissionRule.hasRange()).isFalse();
-
- permissionRule.setMin(-1);
- assertThat(permissionRule.hasRange()).isTrue();
-
- permissionRule.setMax(1);
- assertThat(permissionRule.hasRange()).isTrue();
- }
-
- @Test
- public void getGroup() {
- assertThat(permissionRule.getGroup()).isEqualTo(groupReference);
- }
-
- @Test
- public void setGroup() {
- GroupReference groupReference2 = GroupReference.create(AccountGroup.uuid("uuid2"), "group2");
- assertThat(groupReference2).isNotEqualTo(groupReference);
-
- assertThat(permissionRule.getGroup()).isEqualTo(groupReference);
-
- permissionRule.setGroup(groupReference2);
- assertThat(permissionRule.getGroup()).isEqualTo(groupReference2);
+ this.permissionRule = PermissionRule.create(groupReference);
}
@Test
public void mergeFromAnyBlock() {
GroupReference groupReference1 = GroupReference.create(AccountGroup.uuid("uuid1"), "group1");
- PermissionRule permissionRule1 = new PermissionRule(groupReference1);
+ PermissionRule permissionRule1 = PermissionRule.builder(groupReference1).build();
GroupReference groupReference2 = GroupReference.create(AccountGroup.uuid("uuid2"), "group2");
- PermissionRule permissionRule2 = new PermissionRule(groupReference2);
+ PermissionRule permissionRule2 = PermissionRule.builder(groupReference2).build();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.isBlock()).isFalse();
assertThat(permissionRule2.isBlock()).isFalse();
- permissionRule2.setBlock();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setBlock().build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.isBlock()).isTrue();
assertThat(permissionRule2.isBlock()).isTrue();
- permissionRule2.setDeny();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setDeny().build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.isBlock()).isTrue();
assertThat(permissionRule2.isBlock()).isFalse();
- permissionRule2.setAction(Action.BATCH);
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setAction(Action.BATCH).build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.isBlock()).isTrue();
assertThat(permissionRule2.isBlock()).isFalse();
}
@@ -170,22 +62,22 @@
@Test
public void mergeFromAnyDeny() {
GroupReference groupReference1 = GroupReference.create(AccountGroup.uuid("uuid1"), "group1");
- PermissionRule permissionRule1 = new PermissionRule(groupReference1);
+ PermissionRule permissionRule1 = PermissionRule.builder(groupReference1).build();
GroupReference groupReference2 = GroupReference.create(AccountGroup.uuid("uuid2"), "group2");
- PermissionRule permissionRule2 = new PermissionRule(groupReference2);
+ PermissionRule permissionRule2 = PermissionRule.builder(groupReference2).build();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.isDeny()).isFalse();
assertThat(permissionRule2.isDeny()).isFalse();
- permissionRule2.setDeny();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setDeny().build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.isDeny()).isTrue();
assertThat(permissionRule2.isDeny()).isTrue();
- permissionRule2.setAction(Action.BATCH);
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setAction(Action.BATCH).build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.isDeny()).isTrue();
assertThat(permissionRule2.isDeny()).isFalse();
}
@@ -193,22 +85,22 @@
@Test
public void mergeFromAnyBatch() {
GroupReference groupReference1 = GroupReference.create(AccountGroup.uuid("uuid1"), "group1");
- PermissionRule permissionRule1 = new PermissionRule(groupReference1);
+ PermissionRule permissionRule1 = PermissionRule.builder(groupReference1).build();
GroupReference groupReference2 = GroupReference.create(AccountGroup.uuid("uuid2"), "group2");
- PermissionRule permissionRule2 = new PermissionRule(groupReference2);
+ PermissionRule permissionRule2 = PermissionRule.builder(groupReference2).build();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.getAction()).isNotEqualTo(Action.BATCH);
assertThat(permissionRule2.getAction()).isNotEqualTo(Action.BATCH);
- permissionRule2.setAction(Action.BATCH);
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setAction(Action.BATCH).build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.getAction()).isEqualTo(Action.BATCH);
assertThat(permissionRule2.getAction()).isEqualTo(Action.BATCH);
- permissionRule2.setAction(Action.ALLOW);
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setAction(Action.ALLOW).build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.getAction()).isEqualTo(Action.BATCH);
assertThat(permissionRule2.getAction()).isNotEqualTo(Action.BATCH);
}
@@ -216,22 +108,22 @@
@Test
public void mergeFromAnyForce() {
GroupReference groupReference1 = GroupReference.create(AccountGroup.uuid("uuid1"), "group1");
- PermissionRule permissionRule1 = new PermissionRule(groupReference1);
+ PermissionRule permissionRule1 = PermissionRule.builder(groupReference1).build();
GroupReference groupReference2 = GroupReference.create(AccountGroup.uuid("uuid2"), "group2");
- PermissionRule permissionRule2 = new PermissionRule(groupReference2);
+ PermissionRule permissionRule2 = PermissionRule.builder(groupReference2).build();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.getForce()).isFalse();
assertThat(permissionRule2.getForce()).isFalse();
- permissionRule2.setForce(true);
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setForce(true).build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.getForce()).isTrue();
assertThat(permissionRule2.getForce()).isTrue();
- permissionRule2.setForce(false);
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule2 = permissionRule2.toBuilder().setForce(false).build();
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.getForce()).isTrue();
assertThat(permissionRule2.getForce()).isFalse();
}
@@ -239,14 +131,14 @@
@Test
public void mergeFromMergeRange() {
GroupReference groupReference1 = GroupReference.create(AccountGroup.uuid("uuid1"), "group1");
- PermissionRule permissionRule1 = new PermissionRule(groupReference1);
- permissionRule1.setRange(-1, 2);
+ PermissionRule permissionRule1 =
+ PermissionRule.builder(groupReference1).setRange(-1, 2).build();
GroupReference groupReference2 = GroupReference.create(AccountGroup.uuid("uuid2"), "group2");
- PermissionRule permissionRule2 = new PermissionRule(groupReference2);
- permissionRule2.setRange(-2, 1);
+ PermissionRule permissionRule2 =
+ PermissionRule.builder(groupReference2).setRange(-2, 1).build();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.getMin()).isEqualTo(-2);
assertThat(permissionRule1.getMax()).isEqualTo(2);
assertThat(permissionRule2.getMin()).isEqualTo(-2);
@@ -256,49 +148,56 @@
@Test
public void mergeFromGroupNotChanged() {
GroupReference groupReference1 = GroupReference.create(AccountGroup.uuid("uuid1"), "group1");
- PermissionRule permissionRule1 = new PermissionRule(groupReference1);
+ PermissionRule permissionRule1 = PermissionRule.builder(groupReference1).build();
GroupReference groupReference2 = GroupReference.create(AccountGroup.uuid("uuid2"), "group2");
- PermissionRule permissionRule2 = new PermissionRule(groupReference2);
+ PermissionRule permissionRule2 = PermissionRule.builder(groupReference2).build();
- permissionRule1.mergeFrom(permissionRule2);
+ permissionRule1 = PermissionRule.merge(permissionRule2, permissionRule1);
assertThat(permissionRule1.getGroup()).isEqualTo(groupReference1);
assertThat(permissionRule2.getGroup()).isEqualTo(groupReference2);
}
@Test
public void asString() {
- assertThat(permissionRule.asString(true)).isEqualTo("group " + groupReference.getName());
+ PermissionRule.Builder permissionRule = this.permissionRule.toBuilder();
+
+ assertThat(permissionRule.build().asString(true))
+ .isEqualTo("group " + groupReference.getName());
permissionRule.setDeny();
- assertThat(permissionRule.asString(true)).isEqualTo("deny group " + groupReference.getName());
+ assertThat(permissionRule.build().asString(true))
+ .isEqualTo("deny group " + groupReference.getName());
permissionRule.setBlock();
- assertThat(permissionRule.asString(true)).isEqualTo("block group " + groupReference.getName());
+ assertThat(permissionRule.build().asString(true))
+ .isEqualTo("block group " + groupReference.getName());
permissionRule.setAction(Action.BATCH);
- assertThat(permissionRule.asString(true)).isEqualTo("batch group " + groupReference.getName());
+ assertThat(permissionRule.build().asString(true))
+ .isEqualTo("batch group " + groupReference.getName());
permissionRule.setAction(Action.INTERACTIVE);
- assertThat(permissionRule.asString(true))
+ assertThat(permissionRule.build().asString(true))
.isEqualTo("interactive group " + groupReference.getName());
permissionRule.setForce(true);
- assertThat(permissionRule.asString(true))
+ assertThat(permissionRule.build().asString(true))
.isEqualTo("interactive +force group " + groupReference.getName());
permissionRule.setAction(Action.ALLOW);
- assertThat(permissionRule.asString(true)).isEqualTo("+force group " + groupReference.getName());
+ assertThat(permissionRule.build().asString(true))
+ .isEqualTo("+force group " + groupReference.getName());
permissionRule.setMax(1);
- assertThat(permissionRule.asString(true))
+ assertThat(permissionRule.build().asString(true))
.isEqualTo("+force +0..+1 group " + groupReference.getName());
permissionRule.setMin(-1);
- assertThat(permissionRule.asString(true))
+ assertThat(permissionRule.build().asString(true))
.isEqualTo("+force -1..+1 group " + groupReference.getName());
- assertThat(permissionRule.asString(false))
+ assertThat(permissionRule.build().asString(false))
.isEqualTo("+force group " + groupReference.getName());
}
@@ -348,35 +247,42 @@
@Test
public void testEquals() {
GroupReference groupReference2 = GroupReference.create(AccountGroup.uuid("uuid2"), "group2");
- PermissionRule permissionRuleOther = new PermissionRule(groupReference2);
+ PermissionRule.Builder permissionRuleOther = PermissionRule.builder(groupReference2);
+ PermissionRule.Builder permissionRule = this.permissionRule.toBuilder();
+
assertThat(permissionRule.equals(permissionRuleOther)).isFalse();
permissionRuleOther.setGroup(groupReference);
- assertThat(permissionRule.equals(permissionRuleOther)).isTrue();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isTrue();
permissionRule.setDeny();
- assertThat(permissionRule.equals(permissionRuleOther)).isFalse();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isFalse();
permissionRuleOther.setDeny();
- assertThat(permissionRule.equals(permissionRuleOther)).isTrue();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isTrue();
permissionRule.setForce(true);
- assertThat(permissionRule.equals(permissionRuleOther)).isFalse();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isFalse();
permissionRuleOther.setForce(true);
- assertThat(permissionRule.equals(permissionRuleOther)).isTrue();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isTrue();
permissionRule.setMin(-1);
- assertThat(permissionRule.equals(permissionRuleOther)).isFalse();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isFalse();
permissionRuleOther.setMin(-1);
- assertThat(permissionRule.equals(permissionRuleOther)).isTrue();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isTrue();
permissionRule.setMax(1);
- assertThat(permissionRule.equals(permissionRuleOther)).isFalse();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isFalse();
permissionRuleOther.setMax(1);
- assertThat(permissionRule.equals(permissionRuleOther)).isTrue();
+ assertThat(permissionRuleEquals(permissionRule, permissionRuleOther)).isTrue();
+ }
+
+ private static boolean permissionRuleEquals(
+ PermissionRule.Builder r1, PermissionRule.Builder r2) {
+ return r1.build().equals(r2.build());
}
private void assertPermissionRule(
diff --git a/javatests/com/google/gerrit/common/data/PermissionTest.java b/javatests/com/google/gerrit/common/data/PermissionTest.java
index ef36ad9..a363129 100644
--- a/javatests/com/google/gerrit/common/data/PermissionTest.java
+++ b/javatests/com/google/gerrit/common/data/PermissionTest.java
@@ -154,14 +154,14 @@
@Test
public void setAndGetRules() {
PermissionRule permissionRule1 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
PermissionRule permissionRule2 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
permission.setRules(ImmutableList.of(permissionRule1, permissionRule2));
assertThat(permission.getRules()).containsExactly(permissionRule1, permissionRule2).inOrder();
PermissionRule permissionRule3 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"));
permission.setRules(ImmutableList.of(permissionRule3));
assertThat(permission.getRules()).containsExactly(permissionRule3);
}
@@ -169,9 +169,9 @@
@Test
public void cannotAddPermissionByModifyingListThatWasProvidedToAccessSection() {
PermissionRule permissionRule1 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
PermissionRule permissionRule2 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3");
List<PermissionRule> rules = new ArrayList<>();
@@ -180,7 +180,7 @@
permission.setRules(rules);
assertThat(permission.getRule(groupReference3)).isNull();
- PermissionRule permissionRule3 = new PermissionRule(groupReference3);
+ PermissionRule permissionRule3 = PermissionRule.create(groupReference3);
rules.add(permissionRule3);
assertThat(permission.getRule(groupReference3)).isNull();
}
@@ -195,7 +195,7 @@
@Test
public void getRule() {
GroupReference groupReference = GroupReference.create(AccountGroup.uuid("uuid-1"), "group1");
- PermissionRule permissionRule = new PermissionRule(groupReference);
+ PermissionRule permissionRule = PermissionRule.create(groupReference);
permission.setRules(ImmutableList.of(permissionRule));
assertThat(permission.getRule(groupReference)).isEqualTo(permissionRule);
}
@@ -206,20 +206,20 @@
assertThat(permission.getRule(groupReference)).isNull();
assertThat(permission.getRule(groupReference, true))
- .isEqualTo(new PermissionRule(groupReference));
+ .isEqualTo(PermissionRule.create(groupReference));
}
@Test
public void addRule() {
PermissionRule permissionRule1 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
PermissionRule permissionRule2 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
permission.setRules(ImmutableList.of(permissionRule1, permissionRule2));
GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3");
assertThat(permission.getRule(groupReference3)).isNull();
- PermissionRule permissionRule3 = new PermissionRule(groupReference3);
+ PermissionRule permissionRule3 = PermissionRule.create(groupReference3);
permission.add(permissionRule3);
assertThat(permission.getRule(groupReference3)).isEqualTo(permissionRule3);
assertThat(permission.getRules())
@@ -230,11 +230,11 @@
@Test
public void removeRule() {
PermissionRule permissionRule1 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
PermissionRule permissionRule2 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3");
- PermissionRule permissionRule3 = new PermissionRule(groupReference3);
+ PermissionRule permissionRule3 = PermissionRule.create(groupReference3);
permission.setRules(ImmutableList.of(permissionRule1, permissionRule2, permissionRule3));
assertThat(permission.getRule(groupReference3)).isNotNull();
@@ -247,11 +247,11 @@
@Test
public void removeRuleByGroupReference() {
PermissionRule permissionRule1 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
PermissionRule permissionRule2 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3");
- PermissionRule permissionRule3 = new PermissionRule(groupReference3);
+ PermissionRule permissionRule3 = PermissionRule.create(groupReference3);
permission.setRules(ImmutableList.of(permissionRule1, permissionRule2, permissionRule3));
assertThat(permission.getRule(groupReference3)).isNotNull();
@@ -264,9 +264,9 @@
@Test
public void clearRules() {
PermissionRule permissionRule1 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
PermissionRule permissionRule2 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
permission.setRules(ImmutableList.of(permissionRule1, permissionRule2));
assertThat(permission.getRules()).isNotEmpty();
@@ -278,11 +278,11 @@
@Test
public void mergePermissions() {
PermissionRule permissionRule1 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
PermissionRule permissionRule2 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
PermissionRule permissionRule3 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"));
Permission permission1 = new Permission("foo");
permission1.setRules(ImmutableList.of(permissionRule1, permissionRule2));
@@ -299,9 +299,9 @@
@Test
public void testEquals() {
PermissionRule permissionRule1 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"));
PermissionRule permissionRule2 =
- new PermissionRule(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
+ PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2"));
permission.setRules(ImmutableList.of(permissionRule1, permissionRule2));
diff --git a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
index 214aae7..b7125c3 100644
--- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
+++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
@@ -305,15 +305,17 @@
ProjectConfig cfg = read(rev);
AccessSection section = cfg.getAccessSection("refs/heads/*");
cfg.getAccountsSection()
- .setSameGroupVisibility(Collections.singletonList(new PermissionRule(cfg.resolve(staff))));
+ .setSameGroupVisibility(
+ Collections.singletonList(PermissionRule.create(cfg.resolve(staff))));
Permission submit = section.getPermission(Permission.SUBMIT);
- submit.add(new PermissionRule(cfg.resolve(staff)));
- ContributorAgreement ca = cfg.getContributorAgreement("Individual");
- ca.setAccepted(Collections.singletonList(new PermissionRule(cfg.resolve(staff))));
+ submit.add(PermissionRule.create(cfg.resolve(staff)));
+ ContributorAgreement.Builder ca = cfg.getContributorAgreement("Individual").toBuilder();
+ ca.setAccepted(ImmutableList.of(PermissionRule.create(cfg.resolve(staff))));
ca.setAutoVerify(null);
- ca.setMatchProjectsRegexes(null);
- ca.setExcludeProjectsRegexes(Collections.singletonList("^/theirproject"));
+ ca.setMatchProjectsRegexes(ImmutableList.of());
+ ca.setExcludeProjectsRegexes(ImmutableList.of("^/theirproject"));
ca.setDescription("A new description");
+ cfg.upsertContributorAgreement(ca.build());
rev = commit(cfg);
assertThat(text(rev, "project.config"))
.isEqualTo(
@@ -423,7 +425,7 @@
ProjectConfig cfg = read(rev);
AccessSection section = cfg.getAccessSection("refs/heads/*");
Permission submit = section.getPermission(Permission.SUBMIT);
- submit.add(new PermissionRule(cfg.resolve(staff)));
+ submit.add(PermissionRule.create(cfg.resolve(staff)));
rev = commit(cfg);
assertThat(text(rev, "project.config"))
.isEqualTo(
@@ -713,8 +715,9 @@
update(rev);
ProjectConfig cfg = read(rev);
- ContributorAgreement section = cfg.getContributorAgreement("Individual");
+ ContributorAgreement.Builder section = cfg.getContributorAgreement("Individual").toBuilder();
section.setAccepted(ImmutableList.of());
+ cfg.upsertContributorAgreement(section.build());
rev = commit(cfg);
assertThat(text(rev, "project.config"))
.isEqualTo(
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 4104017..968d4f7 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1861,8 +1861,10 @@
ProjectConfig config = projectConfigFactory.read(md);
AccessSection s = config.getAccessSection(ref, true);
Permission p = s.getPermission(permission, true);
- PermissionRule rule = new PermissionRule(GroupReference.create(groupUUID, groupUUID.get()));
- rule.setForce(force);
+ PermissionRule rule =
+ PermissionRule.builder(GroupReference.create(groupUUID, groupUUID.get()))
+ .setForce(force)
+ .build();
p.add(rule);
config.commit(md);
projectCache.evict(config.getProject());
diff --git a/package.json b/package.json
index d051c41..329e3cb 100644
--- a/package.json
+++ b/package.json
@@ -22,6 +22,8 @@
},
"scripts": {
"clean": "git clean -fdx && bazel clean --expunge",
+ "compile:local": "tsc --project ./polygerrit-ui/app/tsconfig.json",
+ "compile:watch": "npm run compile:local -- --preserveWatchOutput --watch",
"start": "polygerrit-ui/run-server.sh",
"test": "./polygerrit-ui/app/run_test.sh",
"safe_bazelisk": "if which bazelisk >/dev/null; then bazel_bin=bazelisk; else bazel_bin=bazel; fi && $bazel_bin",
diff --git a/plugins/download-commands b/plugins/download-commands
index 47b783e..c4ef993 160000
--- a/plugins/download-commands
+++ b/plugins/download-commands
@@ -1 +1 @@
-Subproject commit 47b783ea75036664dd591d2d3f1bcd06b68cdd5e
+Subproject commit c4ef993fa5e8578641d6447c831ace13743dd5de
diff --git a/polygerrit-ui/README.md b/polygerrit-ui/README.md
index ce274f2..3a66e97 100644
--- a/polygerrit-ui/README.md
+++ b/polygerrit-ui/README.md
@@ -4,7 +4,7 @@
contains several typescript files and uses typescript compiler. This is a
preparation for the upcoming migration to typescript and we actively working on
it. We want to avoid massive typescript-related changes until the preparation
-work is done. Thanks for your understanding!
+work is done. Thanks for your understanding!
Follow the
@@ -93,7 +93,7 @@
manually. For example, if IntelliJ IDEA shows
`Cannot find parent 'tsconfig.json'` error, you can try to setup typescript
options `--project polygerrit-ui/app/tsconfig.json` in the IDE settings.
-
+
## Serving files locally
@@ -171,29 +171,58 @@
For daily development you typically only want to run and debug individual tests.
There are several ways to run tests.
-* Run all tests in headless mode:
+* Run all tests in headless mode (exactly like CI does):
```sh
npm run test
```
+This command uses bazel rules for running frontend tests. Bazel fetches
+all nessecary dependencies and runs all required rules.
* Run all tests in debug mode (the command opens Chrome browser with
the default Karma page; you should click the "Debug" button to start testing):
```sh
+# The following command doesn't compile code before tests
npm run test:debug
```
* Run a single test file:
```
-# Headless mode
+# Headless mode (doesn't compile code before run)
npm run test:single async-foreach-behavior_test.js
-# Debug mode
+
+# Debug mode (doesn't compile code before run)
+npm run test:debug async-foreach-behavior_test.js
+```
+
+Commands `test:debug` and `test:single` assumes that compiled code is located
+in the `./ts-out/polygerrit-ui/app` directory. It's up to you how to achieve it.
+For example, the following options are possible:
+* You can configure IDE for recompiling source code on changes
+* You can use `compile:local` command for running compiler once and
+`compile:watch` for running compiler in watch mode (`compile:...` places
+compile code exactly in the `./ts-out/polygerrit-ui/app` directory)
+
+```sh
+# Compile frontend once and run tests from a file:
+npm run compile:local && npm run test:single async-foreach-behavior_test.js
+
+# Watch mode:
+## Terminal 1:
+npm run compile:watch
+## Terminal 2:
npm run test:debug async-foreach-behavior_test.js
```
* You can run tests in IDE. :
- [IntelliJ: running unit tests on Karma](https://www.jetbrains.com/help/idea/running-unit-tests-on-karma.html#ws_karma_running)
- You should configure IDE to compile typescript before running tests.
-
+
+**NOTE**: Bazel plugin for IntelliJ has a bug - it recompiles typescript
+project only if .ts and/or .d.ts files have been changed. If only .js files
+were changed, the plugin doesn't run compiler. As a workaround, setup
+"Run npm script 'compile:local" action instead of the "Compile Typescript" in
+the "Before launch" section for IntelliJ. This is a temporary problem until
+typescript migration is complete.
## Style guide
diff --git a/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior.js b/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior.js
deleted file mode 100644
index 1d384bc..0000000
--- a/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior.js
+++ /dev/null
@@ -1,48 +0,0 @@
-/**
- * @license
- * Copyright (C) 2017 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.
- */
-
-/** @polymerBehavior AsyncForeachBehavior */
-export const AsyncForeachBehavior = {
- /**
- * @template T
- * @param {!Array<T>} array
- * @param {!Function} fn An iteratee function to be passed each element of
- * the array in order. Must return a promise, and the following
- * iteration will not begin until resolution of the promise returned by
- * the previous iteration.
- *
- * An optional second argument to fn is a callback that will halt the
- * loop if called.
- * @return {!Promise<undefined>}
- */
- asyncForeach(array, fn) {
- if (!array.length) { return Promise.resolve(); }
- let stop = false;
- const stopCallback = () => { stop = true; };
- return fn(array[0], stopCallback).then(exit => {
- if (stop) { return Promise.resolve(); }
- return this.asyncForeach(array.slice(1), fn);
- });
- },
-};
-
-// TODO(dmfilippov) Remove the following lines with assignments
-// Plugins can use the behavior because it was accessible with
-// the global Gerrit... variable. To avoid breaking changes in plugins
-// temporary assign global variables.
-window.Gerrit = window.Gerrit || {};
-window.Gerrit.AsyncForeachBehavior = AsyncForeachBehavior;
diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js
index 2db1c09..b525a82 100644
--- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js
+++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js
@@ -577,6 +577,7 @@
detail: {
event: e,
goKey: this._inGoKeyMode(),
+ vKey: this._inVKeyMode(),
},
composed: true, bubbles: true,
}));
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index af542e0..8f27730 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -1354,15 +1354,12 @@
if ([changeRecord, canStartReview].includes(undefined)) {
return 'Reply';
}
- if (canStartReview) {
- return 'Start Review';
- }
const drafts = (changeRecord && changeRecord.base) || {};
const draftCount = Object.keys(drafts)
.reduce((count, file) => count + drafts[file].length, 0);
- let label = 'Reply';
+ let label = canStartReview ? 'Start Review' : 'Reply';
if (draftCount > 0) {
label += ' (' + draftCount + ')';
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
index d2e8ea7..c8b13b9 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
@@ -1136,6 +1136,7 @@
'file2.txt': [{}, {}],
};
assert.equal(getLabel(changeRecord, false), 'Reply (3)');
+ assert.equal(getLabel(changeRecord, true), 'Start Review (3)');
});
test('comment events properly update diff drafts', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index 44700e9..df6490d 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -33,7 +33,7 @@
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin.js';
import {PolymerElement} from '@polymer/polymer/polymer-element.js';
import {htmlTemplate} from './gr-file-list_html.js';
-import {AsyncForeachBehavior} from '../../../behaviors/async-foreach-behavior/async-foreach-behavior.js';
+import {asyncForeach} from '../../../utils/async-util.js';
import {DomUtilBehavior} from '../../../behaviors/dom-util-behavior/dom-util-behavior.js';
import {PatchSetBehavior} from '../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.js';
import {PathListBehavior} from '../../../behaviors/gr-path-list-behavior/gr-path-list-behavior.js';
@@ -91,7 +91,6 @@
* @extends PolymerElement
*/
class GrFileList extends mixinBehaviors( [
- AsyncForeachBehavior,
DomUtilBehavior,
KeyboardShortcutBehavior,
PatchSetBehavior,
@@ -1292,7 +1291,7 @@
detail: {resolve},
composed: true, bubbles: true,
}));
- })).then(() => this.asyncForeach(files, (file, cancel) => {
+ })).then(() => asyncForeach(files, (file, cancel) => {
const path = file.path;
this._cancelForEachDiff = cancel;
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
index 3fb8442..0690a87 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
@@ -281,7 +281,7 @@
type: Boolean,
computed: '_computeSendButtonDisabled(canBeStarted, ' +
'draftCommentThreads, draft, _reviewersMutated, _labelsChanged, ' +
- '_includeComments, disabled, _commentEditing)',
+ '_includeComments, disabled, _commentEditing, _attentionModified)',
observer: '_sendDisabledChanged',
},
draftCommentThreads: {
@@ -1028,7 +1028,8 @@
_computeSendButtonDisabled(
canBeStarted, draftCommentThreads, text, reviewersMutated,
- labelsChanged, includeComments, disabled, commentEditing) {
+ labelsChanged, includeComments, disabled, commentEditing,
+ attentionModified) {
// Polymer 2: check for undefined
if ([
canBeStarted,
@@ -1039,14 +1040,15 @@
includeComments,
disabled,
commentEditing,
+ attentionModified,
].includes(undefined)) {
return undefined;
}
-
if (commentEditing || disabled) { return true; }
if (canBeStarted === true) { return false; }
const hasDrafts = includeComments && draftCommentThreads.length;
- return !hasDrafts && !text.length && !reviewersMutated && !labelsChanged;
+ return !hasDrafts && !text.length && !reviewersMutated && !labelsChanged &&
+ !attentionModified;
}
_computePatchSetWarning(patchNum, labelsChanged) {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
index 456adb5..29f1e8a 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
@@ -1235,7 +1235,8 @@
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
- /* commentEditing= */ false
+ /* commentEditing= */ false,
+ /* attentionModified= */ false
));
});
@@ -1250,7 +1251,24 @@
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
- /* commentEditing= */ false
+ /* commentEditing= */ false,
+ /* attentionModified= */ false
+ ));
+ });
+
+ test('_computeSendButtonDisabled_attentionModified true', () => {
+ const fn = element._computeSendButtonDisabled.bind(element);
+ // Mock everything false
+ assert.isFalse(fn(
+ /* canBeStarted= */ false,
+ /* draftCommentThreads= */ [],
+ /* text= */ '',
+ /* reviewersMutated= */ false,
+ /* labelsChanged= */ false,
+ /* includeComments= */ false,
+ /* disabled= */ false,
+ /* commentEditing= */ false,
+ /* attentionModified= */ true
));
});
@@ -1265,7 +1283,8 @@
/* labelsChanged= */ false,
/* includeComments= */ true,
/* disabled= */ false,
- /* commentEditing= */ false
+ /* commentEditing= */ false,
+ /* attentionModified= */ false
));
});
@@ -1280,7 +1299,8 @@
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
- /* commentEditing= */ false
+ /* commentEditing= */ false,
+ /* attentionModified= */ false
));
});
@@ -1295,7 +1315,8 @@
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
- /* commentEditing= */ false
+ /* commentEditing= */ false,
+ /* attentionModified= */ false
));
});
@@ -1310,7 +1331,8 @@
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
- /* commentEditing= */ false
+ /* commentEditing= */ false,
+ /* attentionModified= */ false
));
});
@@ -1325,7 +1347,8 @@
/* labelsChanged= */ true,
/* includeComments= */ false,
/* disabled= */ false,
- /* commentEditing= */ false
+ /* commentEditing= */ false,
+ /* attentionModified= */ false
));
});
@@ -1340,7 +1363,8 @@
/* labelsChanged= */ true,
/* includeComments= */ false,
/* disabled= */ true,
- /* commentEditing= */ false
+ /* commentEditing= */ false,
+ /* attentionModified= */ false
));
assert.isTrue(fn(
/* buttonLabel= */ 'Send',
@@ -1350,7 +1374,8 @@
/* labelsChanged= */ true,
/* includeComments= */ false,
/* disabled= */ false,
- /* commentEditing= */ true
+ /* commentEditing= */ true,
+ /* attentionModified= */ false
));
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
index 665e4a6..b82d4b8 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
@@ -23,6 +23,7 @@
import {htmlTemplate} from './gr-diff-highlight_html.js';
import {GrAnnotation} from './gr-annotation.js';
import {GrRangeNormalizer} from './gr-range-normalizer.js';
+import {strToClassName} from '../../../utils/dom-util.js';
/**
* @extends PolymerElement
@@ -125,20 +126,27 @@
// As gr-ranged-comment-layer now does not notify the layer re-render and
// lack of access to the thread or the lineEl from the ranged-comment-layer,
// need to update range class for styles here.
- const currentLine = threadEl.assignedSlot.parentElement.previousSibling;
- if (currentLine && currentLine.querySelector) {
+ let curNode = threadEl.assignedSlot;
+ while (curNode) {
+ if (curNode.nodeName === 'TABLE') break;
+ curNode = curNode.parentElement;
+ }
+ if (curNode && curNode.querySelectorAll) {
if (highlightRange) {
- const rangeNode = currentLine.querySelector('.range');
- if (rangeNode) {
+ const rangeNodes = curNode
+ .querySelectorAll(`.range.${strToClassName(threadEl.rootId)}`);
+ rangeNodes.forEach(rangeNode => {
rangeNode.classList.add('rangeHighlight');
rangeNode.classList.remove('range');
- }
+ });
} else {
- const rangeNode = currentLine.querySelector('.rangeHighlight');
- if (rangeNode) {
+ const rangeNodes = curNode.querySelectorAll(
+ `.rangeHighlight.${strToClassName(threadEl.rootId)}`
+ );
+ rangeNodes.forEach(rangeNode => {
rangeNode.classList.remove('rangeHighlight');
rangeNode.classList.add('range');
- }
+ });
}
}
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index ad72ee1..d3cdfbe 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -563,8 +563,9 @@
this.$.cursor.moveToNextCommentThread();
} else {
if (this.modifierPressed(e)) { return; }
+ // navigate to next file if key is not being held down
this.$.cursor.moveToNextChunk(/* opt_clipToTop = */false,
- /* opt_navigateToNextFile = */true);
+ /* opt_navigateToNextFile = */!e.detail.keyboardEvent.repeat);
}
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index 252b9bc..4d96d53 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -352,7 +352,7 @@
function commentRangeFromThreadEl(threadEl) {
const side = threadEl.getAttribute('comment-side');
const range = JSON.parse(threadEl.getAttribute('range'));
- return {side, range, hovering: false};
+ return {side, range, hovering: false, rootId: threadEl.rootId};
}
const addedCommentRanges = addedThreadEls
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
index 806e147..6a9b0bf 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
@@ -297,12 +297,19 @@
previous: detail.patchNum,
current: e.detail.value,
latest: latestPatchNum,
+ commentCount: this.changeComments.computeCommentCount(
+ {patchNum: e.detail.value}),
});
detail.patchNum = e.detail.value;
} else {
if (this.patchNumEquals(detail.basePatchNum, e.detail.value)) return;
this.reporting.reportInteraction('left-patchset-changed',
- {previous: detail.basePatchNum, current: e.detail.value});
+ {
+ previous: detail.basePatchNum,
+ current: e.detail.value,
+ commentCount: this.changeComments.computeCommentCount(
+ {patchNum: e.detail.value}),
+ });
detail.basePatchNum = e.detail.value;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js
index c774f1a..231e4b5 100644
--- a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js
+++ b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js
@@ -20,6 +20,7 @@
import {PolymerElement} from '@polymer/polymer/polymer-element.js';
import {htmlTemplate} from './gr-ranged-comment-layer_html.js';
import {GrDiffLine} from '../gr-diff/gr-diff-line.js';
+import {strToClassName} from '../../../utils/dom-util.js';
// Polymer 1 adds # before array's key, while Polymer 2 doesn't
const HOVER_PATH_PATTERN = /^(commentRanges\.#?\d+)\.hovering$/;
@@ -92,7 +93,8 @@
for (const range of ranges) {
GrAnnotation.annotateElement(el, range.start,
range.end - range.start,
- range.hovering ? HOVER_HIGHLIGHT : RANGE_HIGHLIGHT);
+ (range.hovering ? HOVER_HIGHLIGHT : RANGE_HIGHLIGHT) +
+ ` ${strToClassName(range.rootId)}`);
}
}
@@ -136,11 +138,11 @@
// If the entire set of comments was changed.
if (record.path === 'commentRanges') {
this._rangesMap = {left: {}, right: {}};
- for (const {side, range, hovering} of record.value) {
+ for (const {side, range, rootId, hovering} of record.value) {
this._updateRangesMap({
side, range, hovering,
operation: (forLine, start, end, hovering) => {
- forLine.push({start, end, hovering});
+ forLine.push({start, end, hovering, rootId});
}});
}
}
@@ -150,7 +152,7 @@
if (match) {
// The #number indicates the key of that item in the array
// not the index, especially in polymer 1.
- const {side, range, hovering} = this.get(match[1]);
+ const {side, range, hovering, rootId} = this.get(match[1]);
this._updateRangesMap({
side, range, hovering, skipLayerUpdate: true,
@@ -158,6 +160,7 @@
const index = forLine.findIndex(lineRange =>
lineRange.start === start && lineRange.end === end);
forLine[index].hovering = hovering;
+ forLine[index].rootId = rootId;
}});
}
@@ -165,21 +168,22 @@
if (record.path === 'commentRanges.splices') {
for (const indexSplice of record.value.indexSplices) {
const removed = indexSplice.removed;
- for (const {side, range, hovering} of removed) {
+ for (const {side, range, hovering, rootId} of removed) {
this._updateRangesMap({
side, range, hovering, operation: (forLine, start, end) => {
const index = forLine.findIndex(lineRange =>
- lineRange.start === start && lineRange.end === end);
+ lineRange.start === start && lineRange.end === end &&
+ rootId === lineRange.rootId);
forLine.splice(index, 1);
}});
}
const added = indexSplice.object.slice(
indexSplice.index, indexSplice.index + indexSplice.addedCount);
- for (const {side, range, hovering} of added) {
+ for (const {side, range, hovering, rootId} of added) {
this._updateRangesMap({
side, range, hovering,
operation: (forLine, start, end, hovering) => {
- forLine.push({start, end, hovering});
+ forLine.push({start, end, hovering, rootId});
}});
}
}
diff --git a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.js b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.js
index 2ce0afa..5f32677 100644
--- a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.js
@@ -36,6 +36,7 @@
start_character: 6,
start_line: 36,
},
+ rootId: 'a',
},
{
side: 'right',
@@ -45,6 +46,7 @@
start_character: 10,
start_line: 10,
},
+ rootId: 'b',
},
{
side: 'right',
@@ -54,6 +56,7 @@
start_character: 5,
start_line: 100,
},
+ rootId: 'c',
},
{
side: 'right',
@@ -63,6 +66,7 @@
start_character: 32,
start_line: 55,
},
+ rootId: 'd',
},
];
@@ -106,7 +110,7 @@
assert.equal(lastCall.args[0], el);
assert.equal(lastCall.args[1], expectedStart);
assert.equal(lastCall.args[2], expectedLength);
- assert.equal(lastCall.args[3], 'style-scope gr-diff range');
+ assert.equal(lastCall.args[3], 'style-scope gr-diff range generated_a');
});
test('type=Remove has-comment hovering', () => {
@@ -124,7 +128,9 @@
assert.equal(lastCall.args[0], el);
assert.equal(lastCall.args[1], expectedStart);
assert.equal(lastCall.args[2], expectedLength);
- assert.equal(lastCall.args[3], 'style-scope gr-diff rangeHighlight');
+ assert.equal(
+ lastCall.args[3], 'style-scope gr-diff rangeHighlight generated_a'
+ );
});
test('type=Both has-comment', () => {
@@ -141,7 +147,7 @@
assert.equal(lastCall.args[0], el);
assert.equal(lastCall.args[1], expectedStart);
assert.equal(lastCall.args[2], expectedLength);
- assert.equal(lastCall.args[3], 'style-scope gr-diff range');
+ assert.equal(lastCall.args[3], 'style-scope gr-diff range generated_a');
});
test('type=Both has-comment off side', () => {
@@ -169,7 +175,7 @@
assert.equal(lastCall.args[0], el);
assert.equal(lastCall.args[1], expectedStart);
assert.equal(lastCall.args[2], expectedLength);
- assert.equal(lastCall.args[3], 'style-scope gr-diff range');
+ assert.equal(lastCall.args[3], 'style-scope gr-diff range generated_b');
});
});
diff --git a/polygerrit-ui/app/elements/gr-app-element.js b/polygerrit-ui/app/elements/gr-app-element.js
index a01e8a4..db098c5 100644
--- a/polygerrit-ui/app/elements/gr-app-element.js
+++ b/polygerrit-ui/app/elements/gr-app-element.js
@@ -421,10 +421,11 @@
}
_handleShortcutTriggered(event) {
- const {event: e, goKey} = event.detail;
+ const {event: e, goKey, vKey} = event.detail;
// eg: {key: "k:keydown", ..., from: "gr-diff-view"}
let key = `${e.key}:${e.type}`;
if (goKey) key = 'g+' + key;
+ if (vKey) key = 'v+' + key;
if (e.shiftKey) key = 'shift+' + key;
if (e.ctrlKey) key = 'ctrl+' + key;
if (e.metaKey) key = 'meta+' + key;
diff --git a/polygerrit-ui/app/utils/async-util.js b/polygerrit-ui/app/utils/async-util.js
new file mode 100644
index 0000000..14d0288
--- /dev/null
+++ b/polygerrit-ui/app/utils/async-util.js
@@ -0,0 +1,38 @@
+/**
+ * @license
+ * Copyright (C) 2017 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.
+ */
+
+/**
+ * @template T
+ * @param {!Array<T>} array
+ * @param {!Function} fn An iteratee function to be passed each element of
+ * the array in order. Must return a promise, and the following
+ * iteration will not begin until resolution of the promise returned by
+ * the previous iteration.
+ *
+ * An optional second argument to fn is a callback that will halt the
+ * loop if called.
+ * @return {!Promise<undefined>}
+ */
+export function asyncForeach(array, fn) {
+ if (!array.length) { return Promise.resolve(); }
+ let stop = false;
+ const stopCallback = () => { stop = true; };
+ return fn(array[0], stopCallback).then(exit => {
+ if (stop) { return Promise.resolve(); }
+ return asyncForeach(array.slice(1), fn);
+ });
+}
diff --git a/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior_test.js b/polygerrit-ui/app/utils/async-util_test.js
similarity index 81%
rename from polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior_test.js
rename to polygerrit-ui/app/utils/async-util_test.js
index 0a44884..df29e97 100644
--- a/polygerrit-ui/app/behaviors/async-foreach-behavior/async-foreach-behavior_test.js
+++ b/polygerrit-ui/app/utils/async-util_test.js
@@ -15,12 +15,13 @@
* limitations under the License.
*/
-import '../../test/common-test-setup-karma.js';
-import {AsyncForeachBehavior} from './async-foreach-behavior.js';
-suite('async-foreach-behavior tests', () => {
+import '../test/common-test-setup-karma.js';
+import {asyncForeach} from './async-util.js';
+
+suite('async-util tests', () => {
test('loops over each item', () => {
const fn = sinon.stub().returns(Promise.resolve());
- return AsyncForeachBehavior.asyncForeach([1, 2, 3], fn)
+ return asyncForeach([1, 2, 3], fn)
.then(() => {
assert.isTrue(fn.calledThrice);
assert.equal(fn.getCall(0).args[0], 1);
@@ -36,7 +37,7 @@
stop();
return Promise.resolve();
};
- return AsyncForeachBehavior.asyncForeach([1, 2, 3], fn)
+ return asyncForeach([1, 2, 3], fn)
.then(() => {
assert.isTrue(stub.calledOnce);
assert.equal(stub.lastCall.args[0], 1);
diff --git a/polygerrit-ui/app/utils/dom-util.js b/polygerrit-ui/app/utils/dom-util.js
index a9f080f..e26bf74 100644
--- a/polygerrit-ui/app/utils/dom-util.js
+++ b/polygerrit-ui/app/utils/dom-util.js
@@ -175,4 +175,18 @@
element = element.parentElement;
}
return isDescendant;
+}
+
+/**
+ * Convert any string into a valid class name.
+ *
+ * For class names, naming rules:
+ * Must begin with a letter A-Z or a-z
+ * Can be followed by: letters (A-Za-z), digits (0-9), hyphens ("-"), and underscores ("_")
+ *
+ * @param {string} str
+ * @param {string} prefix
+ */
+export function strToClassName(str = '', prefix = 'generated_') {
+ return `${prefix}${str.replace(/[^a-zA-Z0-9-_]/g, '_')}`;
}
\ No newline at end of file
diff --git a/polygerrit-ui/app/utils/dom-util_test.js b/polygerrit-ui/app/utils/dom-util_test.js
index c317578..4306cd2 100644
--- a/polygerrit-ui/app/utils/dom-util_test.js
+++ b/polygerrit-ui/app/utils/dom-util_test.js
@@ -15,7 +15,7 @@
* limitations under the License.
*/
import '../test/common-test-setup-karma.js';
-import {getComputedStyleValue, querySelector, querySelectorAll, descendedFromClass, getEventPath} from './dom-util.js';
+import {strToClassName, getComputedStyleValue, querySelector, querySelectorAll, descendedFromClass, getEventPath} from './dom-util.js';
import {PolymerElement} from '@polymer/polymer/polymer-element.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
@@ -136,4 +136,16 @@
querySelector(testEl, '.b')));
});
});
+
+ suite('strToClassName', () => {
+ test('basic tests', () => {
+ assert.equal(strToClassName(''), 'generated_');
+ assert.equal(strToClassName('11'), 'generated_11');
+ assert.equal(strToClassName('0.123'), 'generated_0_123');
+ assert.equal(strToClassName('0.123', 'prefix_'), 'prefix_0_123');
+ assert.equal(strToClassName('0>123', 'prefix_'), 'prefix_0_123');
+ assert.equal(strToClassName('0<123', 'prefix_'), 'prefix_0_123');
+ assert.equal(strToClassName('0+1+23', 'prefix_'), 'prefix_0_1_23');
+ });
+ });
});
\ No newline at end of file
diff --git a/resources/com/google/gerrit/server/mail/Comment.soy b/resources/com/google/gerrit/server/mail/Comment.soy
index 1eb016b..fc92b31 100644
--- a/resources/com/google/gerrit/server/mail/Comment.soy
+++ b/resources/com/google/gerrit/server/mail/Comment.soy
@@ -41,7 +41,7 @@
{for $group in $commentFiles}
// Insert a space before the newline so that Gmail does not mistakenly link
// the following line with the file link. See issue 9201.
- {$group.link}{sp}{\n}
+ {if $group.link}{$group.link}{sp}{/if}{\n}
{$group.title}:{\n}
{\n}
diff --git a/tools/eclipse/project.py b/tools/eclipse/project.py
index f360fa5..b1d5242 100755
--- a/tools/eclipse/project.py
+++ b/tools/eclipse/project.py
@@ -240,7 +240,8 @@
# Exceptions: both source and lib
if p.endswith('libquery_parser.jar') or \
p.endswith('libgerrit-prolog-common.jar') or \
- p.endswith('com_google_protobuf/libprotobuf_java.jar') or \
+ p.endswith('external/com_google_protobuf/java/core/libcore.jar') or \
+ p.endswith('external/com_google_protobuf/java/core/liblite.jar') or \
p.endswith('lucene-core-and-backward-codecs-merged_deploy.jar'):
lib.add(p)
if proto_library.match(p) :