Merge "Highlight gr-change-list-item on hover"
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt
index 2c42d74..9aa0a3b 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -773,6 +773,12 @@
and `Edit Config` buttons on the project screen, and the `Follow-Up`
button on the change screen).
+- [[publish-comments-on-push]]`Publish Draft Comments When a Change Is Updated by Push`:
++
+Whether to publish any outstanding draft comments by default when pushing
+updates to open changes. This preference just sets the default; the behavior can
+still be overridden using a link:user-upload.html#publish-comments[push option].
+
- [[use-flash]]`Use Flash Clipboard Widget`:
+
Whether the Flash clipboard widget should be used. If enabled and the Flash
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt
index 13fca66..fbfd5e4 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -1248,6 +1248,7 @@
"size_bar_in_change_table": true,
"review_category_strategy": "ABBREV",
"mute_common_path_prefixes": true,
+ "publish_comments_on_push": true,
"default_base_for_merges": "FIRST_PARENT",
"my": [
{
@@ -1361,6 +1362,7 @@
"size_bar_in_change_table": true,
"review_category_strategy": "NAME",
"diff_view": "SIDE_BY_SIDE",
+ "publish_comments_on_push": true,
"mute_common_path_prefixes": true,
"my": [
{
@@ -2643,6 +2645,9 @@
The base which should be pre-selected in the 'Diff Against' drop-down
list when the change screen is opened for a merge commit.
Allowed values are `AUTO_MERGE` and `FIRST_PARENT`.
+|`publish_comments_on_push` |not set if `false`|
+Whether to link:user-upload.html#publish-comments[publish draft comments] on
+push by default.
|============================================
[[preferences-input]]
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index fd35a29..49dbf30 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -1065,6 +1065,7 @@
"size_bar_in_change_table": true,
"review_category_strategy": "NONE",
"mute_common_path_prefixes": true,
+ "publish_comments_on_push": true,
"my": [
{
"url": "#/dashboard/self",
@@ -1147,6 +1148,7 @@
"size_bar_in_change_table": true,
"review_category_strategy": "NONE",
"mute_common_path_prefixes": true,
+ "publish_comments_on_push": true,
"my": [
{
"url": "#/dashboard/self",
diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt
index 259540e..deec660 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -262,6 +262,21 @@
git push refs parameter does not allow spaces. Use the '_' character instead,
it will then be applied as "This is a rebase on master".
+[[publish-comments]]
+==== Publish Draft Comments
+
+If you have draft comments on the change(s) that are updated by the push, the
+`publish-comments` option will cause them to be published:
+
+----
+ git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%publish-comments
+----
+
+The default for this option can be set as a
+link:intro-user.html#publish-comments-on-push[user preference]. If the
+preference is set so the default behavior is to publish, this can be overridden
+with the `no-publish-comments` (or `np`) option.
+
[[review_labels]]
==== Review Labels
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 4f4b6c6..2ce0b3b 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -15,21 +15,25 @@
package com.google.gerrit.acceptance.git;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
import static com.google.gerrit.acceptance.GitUtil.assertPushRejected;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
import static com.google.gerrit.extensions.common.EditInfoSubject.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.project.Util.category;
import static com.google.gerrit.server.project.Util.value;
import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.toList;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.common.collect.Streams;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -37,16 +41,20 @@
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ProjectWatchInfo;
import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
+import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
@@ -61,12 +69,14 @@
import com.google.gerrit.testutil.TestTimeUtil;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.regex.Pattern;
import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
@@ -75,6 +85,7 @@
import org.eclipse.jgit.transport.PushResult;
import org.eclipse.jgit.transport.RefSpec;
import org.eclipse.jgit.transport.RemoteRefUpdate;
+import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
@@ -111,6 +122,14 @@
grant(Permission.LABEL + "Patch-Set-Lock", project, "refs/heads/*");
}
+ @After
+ public void tearDown() throws Exception {
+ setApiUser(admin);
+ GeneralPreferencesInfo prefs = gApi.accounts().id(admin.id.get()).getPreferences();
+ prefs.publishCommentsOnPush = false;
+ gApi.accounts().id(admin.id.get()).setPreferences(prefs);
+ }
+
protected void selectProtocol(Protocol p) throws Exception {
String url;
switch (p) {
@@ -1469,6 +1488,193 @@
assertThat(info.status).isEqualTo(ChangeStatus.NEW);
}
+ @Test
+ public void publishCommentsOnPushPublishesDraftsOnAllRevisions() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String rev1 = r.getCommit().name();
+ CommentInfo c1 = addDraft(r.getChangeId(), rev1, newDraft(FILE_NAME, 1, "comment1"));
+ CommentInfo c2 = addDraft(r.getChangeId(), rev1, newDraft(FILE_NAME, 1, "comment2"));
+
+ r = amendChange(r.getChangeId());
+ String rev2 = r.getCommit().name();
+ CommentInfo c3 = addDraft(r.getChangeId(), rev2, newDraft(FILE_NAME, 1, "comment3"));
+
+ assertThat(getPublishedComments(r.getChangeId())).isEmpty();
+
+ gApi.changes().id(r.getChangeId()).addReviewer(user.email);
+ sender.clear();
+ amendChange(r.getChangeId(), "refs/for/master%publish-comments");
+
+ Collection<CommentInfo> comments = getPublishedComments(r.getChangeId());
+ assertThat(comments.stream().map(c -> c.id)).containsExactly(c1.id, c2.id, c3.id);
+ assertThat(comments.stream().map(c -> c.message))
+ .containsExactly("comment1", "comment2", "comment3");
+ assertThat(getLastMessage(r.getChangeId())).isEqualTo("Uploaded patch set 3.\n\n(3 comments)");
+
+ List<String> messages =
+ sender
+ .getMessages()
+ .stream()
+ .map(m -> m.body())
+ .sorted(Comparator.comparingInt(m -> m.contains("reexamine") ? 0 : 1))
+ .collect(toList());
+ assertThat(messages).hasSize(2);
+
+ assertThat(messages.get(0)).contains("Gerrit-MessageType: newpatchset");
+ assertThat(messages.get(0)).contains("I'd like you to reexamine a change");
+ assertThat(messages.get(0)).doesNotContain("Uploaded patch set 3");
+
+ assertThat(messages.get(1)).contains("Gerrit-MessageType: comment");
+ assertThat(messages.get(1))
+ .containsMatch(
+ Pattern.compile(
+ // A little weird that the comment email contains this text, but it's actually
+ // what's in the ChangeMessage. Really we should fuse the emails into one, but until
+ // then, this test documents the current behavior.
+ "Uploaded patch set 3\\.\n"
+ + "\n"
+ + "\\(3 comments\\)\\n.*"
+ + "PS1, Line 1:.*"
+ + "comment1\\n.*"
+ + "PS1, Line 1:.*"
+ + "comment2\\n.*"
+ + "PS2, Line 1:.*"
+ + "comment3\\n",
+ Pattern.DOTALL));
+ }
+
+ @Test
+ public void publishCommentsOnPushWithMessage() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String rev = r.getCommit().name();
+ addDraft(r.getChangeId(), rev, newDraft(FILE_NAME, 1, "comment1"));
+
+ r = amendChange(r.getChangeId(), "refs/for/master%publish-comments,m=The_message");
+
+ Collection<CommentInfo> comments = getPublishedComments(r.getChangeId());
+ assertThat(comments.stream().map(c -> c.message)).containsExactly("comment1");
+ assertThat(getLastMessage(r.getChangeId()))
+ .isEqualTo("Uploaded patch set 2.\n\n(1 comment)\n\nThe message");
+ }
+
+ @Test
+ public void publishCommentsOnPushPublishesDraftsOnMultipleChanges() throws Exception {
+ ObjectId initialHead = testRepo.getRepository().resolve("HEAD");
+ List<RevCommit> commits = createChanges(2, "refs/for/master");
+ String id1 = byCommit(commits.get(0)).change().getKey().get();
+ String id2 = byCommit(commits.get(1)).change().getKey().get();
+ CommentInfo c1 = addDraft(id1, commits.get(0).name(), newDraft(FILE_NAME, 1, "comment1"));
+ CommentInfo c2 = addDraft(id2, commits.get(1).name(), newDraft(FILE_NAME, 1, "comment2"));
+
+ assertThat(getPublishedComments(id1)).isEmpty();
+ assertThat(getPublishedComments(id2)).isEmpty();
+
+ amendChanges(initialHead, commits, "refs/for/master%publish-comments");
+
+ Collection<CommentInfo> cs1 = getPublishedComments(id1);
+ assertThat(cs1.stream().map(c -> c.message)).containsExactly("comment1");
+ assertThat(cs1.stream().map(c -> c.id)).containsExactly(c1.id);
+ assertThat(getLastMessage(id1))
+ .isEqualTo("Uploaded patch set 2: Commit message was updated.\n\n(1 comment)");
+
+ Collection<CommentInfo> cs2 = getPublishedComments(id2);
+ assertThat(cs2.stream().map(c -> c.message)).containsExactly("comment2");
+ assertThat(cs2.stream().map(c -> c.id)).containsExactly(c2.id);
+ assertThat(getLastMessage(id2))
+ .isEqualTo("Uploaded patch set 2: Commit message was updated.\n\n(1 comment)");
+ }
+
+ @Test
+ public void publishCommentsOnPushOnlyPublishesDraftsOnUpdatedChanges() throws Exception {
+ PushOneCommit.Result r1 = createChange();
+ PushOneCommit.Result r2 = createChange();
+ String id1 = r1.getChangeId();
+ String id2 = r2.getChangeId();
+ addDraft(id1, r1.getCommit().name(), newDraft(FILE_NAME, 1, "comment1"));
+ CommentInfo c2 = addDraft(id2, r2.getCommit().name(), newDraft(FILE_NAME, 1, "comment2"));
+
+ assertThat(getPublishedComments(id1)).isEmpty();
+ assertThat(getPublishedComments(id2)).isEmpty();
+
+ r2 = amendChange(id2, "refs/for/master%publish-comments");
+
+ assertThat(getPublishedComments(id1)).isEmpty();
+ assertThat(gApi.changes().id(id1).drafts()).hasSize(1);
+
+ Collection<CommentInfo> cs2 = getPublishedComments(id2);
+ assertThat(cs2.stream().map(c -> c.message)).containsExactly("comment2");
+ assertThat(cs2.stream().map(c -> c.id)).containsExactly(c2.id);
+
+ assertThat(getLastMessage(id1)).doesNotMatch("[Cc]omment");
+ assertThat(getLastMessage(id2)).isEqualTo("Uploaded patch set 2.\n\n(1 comment)");
+ }
+
+ @Test
+ public void publishCommentsOnPushWithPreference() throws Exception {
+ PushOneCommit.Result r = createChange();
+ addDraft(r.getChangeId(), r.getCommit().name(), newDraft(FILE_NAME, 1, "comment1"));
+ r = amendChange(r.getChangeId());
+
+ assertThat(getPublishedComments(r.getChangeId())).isEmpty();
+
+ GeneralPreferencesInfo prefs = gApi.accounts().id(admin.id.get()).getPreferences();
+ prefs.publishCommentsOnPush = true;
+ gApi.accounts().id(admin.id.get()).setPreferences(prefs);
+
+ r = amendChange(r.getChangeId());
+ assertThat(getPublishedComments(r.getChangeId()).stream().map(c -> c.message))
+ .containsExactly("comment1");
+ }
+
+ @Test
+ public void publishCommentsOnPushOverridingPreference() throws Exception {
+ PushOneCommit.Result r = createChange();
+ addDraft(r.getChangeId(), r.getCommit().name(), newDraft(FILE_NAME, 1, "comment1"));
+
+ GeneralPreferencesInfo prefs = gApi.accounts().id(admin.id.get()).getPreferences();
+ prefs.publishCommentsOnPush = true;
+ gApi.accounts().id(admin.id.get()).setPreferences(prefs);
+
+ r = amendChange(r.getChangeId(), "refs/for/master%no-publish-comments");
+
+ assertThat(getPublishedComments(r.getChangeId())).isEmpty();
+ }
+
+ private DraftInput newDraft(String path, int line, String message) {
+ DraftInput d = new DraftInput();
+ d.path = path;
+ d.side = Side.REVISION;
+ d.line = line;
+ d.message = message;
+ d.unresolved = true;
+ return d;
+ }
+
+ private CommentInfo addDraft(String changeId, String revId, DraftInput in) throws Exception {
+ return gApi.changes().id(changeId).revision(revId).createDraft(in).get();
+ }
+
+ private Collection<CommentInfo> getPublishedComments(String changeId) throws Exception {
+ return gApi.changes()
+ .id(changeId)
+ .comments()
+ .values()
+ .stream()
+ .flatMap(cs -> cs.stream())
+ .collect(toList());
+ }
+
+ private String getLastMessage(String changeId) throws Exception {
+ return Streams.findLast(
+ gApi.changes()
+ .id(changeId)
+ .get(EnumSet.of(ListChangesOption.MESSAGES))
+ .messages
+ .stream()
+ .map(m -> m.message))
+ .get();
+ }
+
private void assertThatUserIsOnlyReviewer(ChangeInfo ci, TestAccount reviewer) {
assertThat(ci.reviewers).isNotNull();
assertThat(ci.reviewers.keySet()).containsExactly(ReviewerState.REVIEWER);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
index 36f8452..552d7a4 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
@@ -163,7 +163,7 @@
systemGroupBackend.getGroup(REGISTERED_USERS).getUUID(),
"refs/for/" + newBranch.get());
exception.expect(AuthException.class);
- exception.expectMessage("Move not permitted");
+ exception.expectMessage("move not permitted");
move(r.getChangeId(), newBranch.get());
}
@@ -179,7 +179,7 @@
r.getChange().change().getDest().get());
setApiUser(user);
exception.expect(AuthException.class);
- exception.expectMessage("Move not permitted");
+ exception.expectMessage("move not permitted");
move(r.getChangeId(), newBranch.get());
}
@@ -223,7 +223,7 @@
revision(r).review(new ReviewInput().label("Patch-Set-Lock", 1));
exception.expect(AuthException.class);
- exception.expectMessage("Move not permitted");
+ exception.expectMessage("move not permitted");
move(r.getChangeId(), newBranch.get());
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java
index bf08ac9..067c80a 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java
@@ -117,7 +117,7 @@
private void assertDeleteForbidden() throws Exception {
exception.expect(AuthException.class);
- exception.expectMessage("Cannot delete branch");
+ exception.expectMessage("delete not permitted");
branch().delete();
}
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java
index c9d7446..c0c2e93 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java
@@ -112,7 +112,7 @@
private void assertDeleteForbidden() throws Exception {
exception.expect(AuthException.class);
- exception.expectMessage("Cannot delete tag");
+ exception.expectMessage("delete not permitted");
tag().delete();
}
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/TagsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/TagsIT.java
index f191681..6d2bf16 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/TagsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/TagsIT.java
@@ -243,7 +243,7 @@
TagInput input = new TagInput();
input.ref = "test";
exception.expect(AuthException.class);
- exception.expectMessage("Cannot create tag \"" + R_TAGS + "test\"");
+ exception.expectMessage("create not permitted");
tag(input.ref).create(input);
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java
index d628268..2b29054 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java
@@ -32,6 +32,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
@@ -83,6 +84,7 @@
import org.junit.Before;
import org.junit.Test;
+@NoHttpd
public class NoteDbPrimaryIT extends AbstractDaemonTest {
@ConfigSuite.Default
public static Config defaultConfig() {
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
index 7192ff9..9dcba5e 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
@@ -157,6 +157,7 @@
public EmailStrategy emailStrategy;
public EmailFormat emailFormat;
public DefaultBase defaultBaseForMerges;
+ public Boolean publishCommentsOnPush;
public boolean isShowInfoInReviewCategory() {
return getReviewCategoryStrategy() != ReviewCategoryStrategy.NONE;
@@ -225,6 +226,7 @@
p.muteCommonPathPrefixes = true;
p.signedOffBy = false;
p.defaultBaseForMerges = DefaultBase.FIRST_PARENT;
+ p.publishCommentsOnPush = false;
return p;
}
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/AuthException.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/AuthException.java
index 1d4cda7..0b4f459 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/AuthException.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/AuthException.java
@@ -22,4 +22,12 @@
public AuthException(String msg) {
super(msg);
}
+
+ /**
+ * @param msg message to return to the client.
+ * @param cause cause of this exception.
+ */
+ public AuthException(String msg, Throwable cause) {
+ super(msg, cause);
+ }
}
diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/GeneralPreferences.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/GeneralPreferences.java
index 23e1a93..1dcb284 100644
--- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/GeneralPreferences.java
+++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/GeneralPreferences.java
@@ -148,6 +148,9 @@
private native String defaultBaseForMergesRaw() /*-{ return this.default_base_for_merges }-*/;
+ public final native boolean
+ publishCommentsOnPush() /*-{ return this.publish_comments_on_push || false }-*/;
+
public final native JsArray<TopMenuItem> my() /*-{ return this.my; }-*/;
public final native void changesPerPage(int n) /*-{ this.changes_per_page = n }-*/;
@@ -224,6 +227,9 @@
private native void defaultBaseForMergesRaw(String b) /*-{ this.default_base_for_merges = b }-*/;
+ public final native void publishCommentsOnPush(
+ boolean p) /*-{ this.publish_comments_on_push = p }-*/;
+
public final void setMyMenus(List<TopMenuItem> myMenus) {
initMy();
for (TopMenuItem n : myMenus) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java
index 4a3a5f8..314871e 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.java
@@ -67,6 +67,8 @@
String signedOffBy();
+ String publishCommentsOnPush();
+
String myMenu();
String myMenuInfo();
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties
index 9d87365..d31abdb 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties
@@ -38,6 +38,7 @@
showLegacycidInChangeTable = Show Change Number In Changes Table
muteCommonPathPrefixes = Mute Common Path Prefixes In File List
signedOffBy = Insert Signed-off-by Footer For Inline Edit Changes
+publishCommentsOnPush = Publish Draft Comments When a Change Is Updated by Push
myMenu = My Menu
myMenuInfo = \
Menu items for the 'My' top level menu. \
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPreferencesScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPreferencesScreen.java
index 2edc137..9be15ff 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPreferencesScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/MyPreferencesScreen.java
@@ -55,6 +55,7 @@
private CheckBox legacycidInChangeTable;
private CheckBox muteCommonPathPrefixes;
private CheckBox signedOffBy;
+ private CheckBox publishCommentsOnPush;
private ListBox maximumPageSize;
private ListBox dateFormat;
private ListBox timeFormat;
@@ -161,9 +162,10 @@
legacycidInChangeTable = new CheckBox(Util.C.showLegacycidInChangeTable());
muteCommonPathPrefixes = new CheckBox(Util.C.muteCommonPathPrefixes());
signedOffBy = new CheckBox(Util.C.signedOffBy());
+ publishCommentsOnPush = new CheckBox(Util.C.publishCommentsOnPush());
boolean flashClippy = !UserAgent.hasJavaScriptClipboard() && UserAgent.Flash.isInstalled();
- final Grid formGrid = new Grid(14 + (flashClippy ? 1 : 0), 2);
+ final Grid formGrid = new Grid(15 + (flashClippy ? 1 : 0), 2);
int row = 0;
@@ -223,6 +225,10 @@
formGrid.setWidget(row, fieldIdx, signedOffBy);
row++;
+ formGrid.setText(row, labelIdx, "");
+ formGrid.setWidget(row, fieldIdx, publishCommentsOnPush);
+ row++;
+
if (flashClippy) {
formGrid.setText(row, labelIdx, "");
formGrid.setWidget(row, fieldIdx, useFlashClipboard);
@@ -257,6 +263,7 @@
e.listenTo(legacycidInChangeTable);
e.listenTo(muteCommonPathPrefixes);
e.listenTo(signedOffBy);
+ e.listenTo(publishCommentsOnPush);
e.listenTo(diffView);
e.listenTo(reviewCategoryStrategy);
e.listenTo(emailStrategy);
@@ -295,6 +302,7 @@
legacycidInChangeTable.setEnabled(on);
muteCommonPathPrefixes.setEnabled(on);
signedOffBy.setEnabled(on);
+ publishCommentsOnPush.setEnabled(on);
reviewCategoryStrategy.setEnabled(on);
diffView.setEnabled(on);
emailStrategy.setEnabled(on);
@@ -320,6 +328,7 @@
legacycidInChangeTable.setValue(p.legacycidInChangeTable());
muteCommonPathPrefixes.setValue(p.muteCommonPathPrefixes());
signedOffBy.setValue(p.signedOffBy());
+ publishCommentsOnPush.setValue(p.publishCommentsOnPush());
setListBox(
reviewCategoryStrategy,
GeneralPreferencesInfo.ReviewCategoryStrategy.NONE,
@@ -412,6 +421,7 @@
p.legacycidInChangeTable(legacycidInChangeTable.getValue());
p.muteCommonPathPrefixes(muteCommonPathPrefixes.getValue());
p.signedOffBy(signedOffBy.getValue());
+ p.publishCommentsOnPush(publishCommentsOnPush.getValue());
p.reviewCategoryStrategy(
getListBox(
reviewCategoryStrategy, ReviewCategoryStrategy.NONE, ReviewCategoryStrategy.values()));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java
index 77eff90..aee97fc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java
@@ -28,6 +28,7 @@
import com.google.gerrit.server.edit.tree.TreeModification;
import com.google.gerrit.server.fixes.FixReplacementInterpreter;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectState;
import com.google.gwtorm.server.OrmException;
@@ -61,7 +62,7 @@
@Override
public Response<EditInfo> apply(FixResource fixResource, Void nothing)
throws AuthException, OrmException, ResourceConflictException, IOException,
- ResourceNotFoundException {
+ ResourceNotFoundException, PermissionBackendException {
RevisionResource revisionResource = fixResource.getRevisionResource();
Project.NameKey project = revisionResource.getProject();
ProjectState projectState = revisionResource.getControl().getProjectControl().getProjectState();
@@ -75,8 +76,7 @@
ChangeEdit changeEdit =
changeEditModifier.combineWithModifiedPatchSetTree(
repository, revisionResource.getControl(), patchSet, treeModifications);
- EditInfo editInfo = changeEditJson.toEditInfo(changeEdit, false);
- return Response.ok(editInfo);
+ return Response.ok(changeEditJson.toEditInfo(changeEdit, false));
} catch (InvalidChangeOperationException e) {
throw new ResourceConflictException(e.getMessage());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeEdits.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeEdits.java
index c1db891..929268b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeEdits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeEdits.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.edit.UnchangedCommitMessageException;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gwtorm.server.OrmException;
@@ -154,7 +155,8 @@
@Override
public Response<?> apply(ChangeResource resource, Put.Input input)
- throws AuthException, ResourceConflictException, IOException, OrmException {
+ throws AuthException, ResourceConflictException, IOException, OrmException,
+ PermissionBackendException {
putEdit.apply(resource.getControl(), path, input.content);
return Response.none();
}
@@ -178,7 +180,8 @@
@Override
public Response<?> apply(ChangeResource rsrc, DeleteFile.Input in)
- throws IOException, AuthException, ResourceConflictException, OrmException {
+ throws IOException, AuthException, ResourceConflictException, OrmException,
+ PermissionBackendException {
return deleteContent.apply(rsrc.getControl(), path);
}
}
@@ -268,7 +271,8 @@
@Override
public Response<?> apply(ChangeResource resource, Post.Input input)
- throws AuthException, IOException, ResourceConflictException, OrmException {
+ throws AuthException, IOException, ResourceConflictException, OrmException,
+ PermissionBackendException {
Project.NameKey project = resource.getProject();
try (Repository repository = repositoryManager.openRepository(project)) {
ChangeControl changeControl = resource.getControl();
@@ -314,12 +318,14 @@
@Override
public Response<?> apply(ChangeEditResource rsrc, Input input)
- throws AuthException, ResourceConflictException, IOException, OrmException {
+ throws AuthException, ResourceConflictException, IOException, OrmException,
+ PermissionBackendException {
return apply(rsrc.getControl(), rsrc.getPath(), input.content);
}
public Response<?> apply(ChangeControl changeControl, String path, RawInput newContent)
- throws ResourceConflictException, AuthException, IOException, OrmException {
+ throws ResourceConflictException, AuthException, IOException, OrmException,
+ PermissionBackendException {
if (Strings.isNullOrEmpty(path) || path.charAt(0) == '/') {
throw new ResourceConflictException("Invalid path: " + path);
}
@@ -356,12 +362,14 @@
@Override
public Response<?> apply(ChangeEditResource rsrc, DeleteContent.Input input)
- throws AuthException, ResourceConflictException, OrmException, IOException {
+ throws AuthException, ResourceConflictException, OrmException, IOException,
+ PermissionBackendException {
return apply(rsrc.getControl(), rsrc.getPath());
}
public Response<?> apply(ChangeControl changeControl, String filePath)
- throws AuthException, IOException, OrmException, ResourceConflictException {
+ throws AuthException, IOException, OrmException, ResourceConflictException,
+ PermissionBackendException {
Project.NameKey project = changeControl.getChange().getProject();
try (Repository repository = repositoryManager.openRepository(project)) {
editModifier.deleteFile(repository, changeControl, filePath);
@@ -456,7 +464,7 @@
@Override
public Object apply(ChangeResource rsrc, Input input)
throws AuthException, IOException, BadRequestException, ResourceConflictException,
- OrmException {
+ OrmException, PermissionBackendException {
if (input == null || Strings.isNullOrEmpty(input.message)) {
throw new BadRequestException("commit message must be provided");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateMergePatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateMergePatchSet.java
index 02c91ac..b53bdd9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateMergePatchSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateMergePatchSet.java
@@ -22,7 +22,6 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.MergeInput;
import com.google.gerrit.extensions.common.MergePatchSetInput;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -41,6 +40,8 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeIdenticalTreeException;
import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectControl;
@@ -102,27 +103,18 @@
@Override
protected Response<ChangeInfo> applyImpl(
- BatchUpdate.Factory updateFactory, ChangeResource req, MergePatchSetInput in)
+ BatchUpdate.Factory updateFactory, ChangeResource rsrc, MergePatchSetInput in)
throws OrmException, IOException, InvalidChangeOperationException, RestApiException,
- UpdateException {
- if (in.merge == null) {
- throw new BadRequestException("merge field is required");
- }
+ UpdateException, PermissionBackendException {
+ rsrc.permissions().database(db).check(ChangePermission.ADD_PATCH_SET);
MergeInput merge = in.merge;
- if (Strings.isNullOrEmpty(merge.source)) {
+ if (merge == null || Strings.isNullOrEmpty(merge.source)) {
throw new BadRequestException("merge.source must be non-empty");
}
- ChangeControl ctl = req.getControl();
- if (!ctl.isVisible(db.get())) {
- throw new InvalidChangeOperationException("Base change not found: " + req.getId());
- }
+ ChangeControl ctl = rsrc.getControl();
PatchSet ps = psUtil.current(db.get(), ctl.getNotes());
- if (!ctl.canAddPatchSet(db.get())) {
- throw new AuthException("cannot add patch set");
- }
-
ProjectControl projectControl = ctl.getProjectControl();
Change change = ctl.getChange();
Project.NameKey project = change.getProject();
@@ -139,11 +131,9 @@
}
RevCommit currentPsCommit = rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));
-
Timestamp now = TimeUtil.nowTs();
IdentifiedUser me = user.get().asIdentifiedUser();
PersonIdent author = me.newCommitterIdent(now, serverTimeZone);
-
RevCommit newCommit =
createMergeCommit(
in,
@@ -166,7 +156,8 @@
psInserter
.setMessage("Uploaded patch set " + nextPsId.get() + ".")
.setDraft(ps.isDraft())
- .setNotify(NotifyHandling.NONE));
+ .setNotify(NotifyHandling.NONE)
+ .setCheckAddPatchSetPermission(false));
bu.execute();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteComment.java
index b0b222b..d358184 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteComment.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteComment.java
@@ -27,9 +27,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -54,9 +52,6 @@
private final PermissionBackend permissionBackend;
private final BatchUpdate.Factory batchUpdateFactory;
private final CommentsUtil commentsUtil;
- private final PatchSetUtil psUtil;
- private final BatchUpdate.Factory updateFactory;
- private final PatchListCache patchListCache;
private final Provider<CommentJson> commentJson;
private final ChangeNotes.Factory notesFactory;
@@ -67,9 +62,6 @@
PermissionBackend permissionBackend,
BatchUpdate.Factory batchUpdateFactory,
CommentsUtil commentsUtil,
- PatchSetUtil psUtil,
- BatchUpdate.Factory updateFactory,
- PatchListCache patchListCache,
Provider<CommentJson> commentJson,
ChangeNotes.Factory notesFactory) {
this.userProvider = userProvider;
@@ -77,9 +69,6 @@
this.permissionBackend = permissionBackend;
this.batchUpdateFactory = batchUpdateFactory;
this.commentsUtil = commentsUtil;
- this.psUtil = psUtil;
- this.updateFactory = updateFactory;
- this.patchListCache = patchListCache;
this.commentJson = commentJson;
this.notesFactory = notesFactory;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
index 1a0d118..040b6de 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
@@ -258,8 +258,7 @@
if (patchSet == null) {
throw new PatchListNotAvailableException(
String.format(
- "patch set %s of change %s not found",
- old.get(), change.getId().get()));
+ "patch set %s of change %s not found", old.get(), change.getId().get()));
}
PatchList oldList = patchListCache.get(change, patchSet);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
index 98b79d1..ffc0dc4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
@@ -33,10 +33,14 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -57,6 +61,7 @@
@Singleton
public class Move extends RetryingRestModifyView<ChangeResource, MoveInput, ChangeInfo> {
+ private final PermissionBackend permissionBackend;
private final Provider<ReviewDb> dbProvider;
private final ChangeJson.Factory json;
private final GitRepositoryManager repoManager;
@@ -66,6 +71,7 @@
@Inject
Move(
+ PermissionBackend permissionBackend,
Provider<ReviewDb> dbProvider,
ChangeJson.Factory json,
GitRepositoryManager repoManager,
@@ -74,6 +80,7 @@
RetryHelper retryHelper,
PatchSetUtil psUtil) {
super(retryHelper);
+ this.permissionBackend = permissionBackend;
this.dbProvider = dbProvider;
this.json = json;
this.repoManager = repoManager;
@@ -84,22 +91,40 @@
@Override
protected ChangeInfo applyImpl(
- BatchUpdate.Factory updateFactory, ChangeResource req, MoveInput input)
- throws RestApiException, OrmException, UpdateException {
- ChangeControl control = req.getControl();
+ BatchUpdate.Factory updateFactory, ChangeResource rsrc, MoveInput input)
+ throws RestApiException, OrmException, UpdateException, PermissionBackendException {
+ Change change = rsrc.getChange();
+ Project.NameKey project = rsrc.getProject();
+ IdentifiedUser caller = rsrc.getUser();
input.destinationBranch = RefNames.fullName(input.destinationBranch);
- if (!control.canMoveTo(input.destinationBranch, dbProvider.get())) {
- throw new AuthException("Move not permitted");
+
+ if (change.getStatus().isClosed()) {
+ throw new ResourceConflictException("Change is " + ChangeUtil.status(change));
+ }
+
+ Branch.NameKey newDest = new Branch.NameKey(project, input.destinationBranch);
+ if (change.getDest().equals(newDest)) {
+ throw new ResourceConflictException("Change is already destined for the specified branch");
+ }
+
+ // Move requires abandoning this change, and creating a new change.
+ try {
+ rsrc.permissions().database(dbProvider).check(ChangePermission.ABANDON);
+ permissionBackend
+ .user(caller)
+ .database(dbProvider)
+ .ref(newDest)
+ .check(RefPermission.CREATE_CHANGE);
+ } catch (AuthException denied) {
+ throw new AuthException("move not permitted", denied);
}
try (BatchUpdate u =
- updateFactory.create(
- dbProvider.get(), req.getChange().getProject(), control.getUser(), TimeUtil.nowTs())) {
- u.addOp(req.getChange().getId(), new Op(input));
+ updateFactory.create(dbProvider.get(), project, caller, TimeUtil.nowTs())) {
+ u.addOp(change.getId(), new Op(input));
u.execute();
}
-
- return json.noOptions().format(req.getChange());
+ return json.noOptions().format(project, rsrc.getId());
}
private class Op implements BatchUpdateOp {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index 5933b19..581f2ba 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -44,6 +44,9 @@
import com.google.gerrit.server.mail.send.ReplacePatchSetSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -69,6 +72,7 @@
}
// Injected fields.
+ private final PermissionBackend permissionBackend;
private final PatchSetInfoFactory patchSetInfoFactory;
private final CommitValidators.Factory commitValidatorsFactory;
private final ReplacePatchSetSender.Factory replacePatchSetFactory;
@@ -108,6 +112,7 @@
@Inject
public PatchSetInserter(
+ PermissionBackend permissionBackend,
ApprovalsUtil approvalsUtil,
ApprovalCopier approvalCopier,
ChangeMessagesUtil cmUtil,
@@ -119,6 +124,7 @@
@Assisted ChangeControl ctl,
@Assisted PatchSet.Id psId,
@Assisted ObjectId commitId) {
+ this.permissionBackend = permissionBackend;
this.approvalsUtil = approvalsUtil;
this.approvalCopier = approvalCopier;
this.cmUtil = cmUtil;
@@ -206,7 +212,8 @@
@Override
public void updateRepo(RepoContext ctx)
- throws AuthException, ResourceConflictException, IOException, OrmException {
+ throws AuthException, ResourceConflictException, IOException, OrmException,
+ PermissionBackendException {
validate(ctx);
ctx.addRefUpdate(ObjectId.zeroId(), commitId, getPatchSetId().toRefName());
}
@@ -301,9 +308,13 @@
}
private void validate(RepoContext ctx)
- throws AuthException, ResourceConflictException, IOException, OrmException {
- if (checkAddPatchSetPermission && !origCtl.canAddPatchSet(ctx.getDb())) {
- throw new AuthException("cannot add patch set");
+ throws AuthException, ResourceConflictException, IOException, PermissionBackendException {
+ if (checkAddPatchSetPermission) {
+ permissionBackend
+ .user(ctx.getUser())
+ .database(ctx.getDb())
+ .change(origCtl.getNotes())
+ .check(ChangePermission.ADD_PATCH_SET);
}
if (!validate) {
return;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeEdit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeEdit.java
index 7b673dd..9e840c3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeEdit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeEdit.java
@@ -28,6 +28,7 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.edit.ChangeEditModifier;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -82,7 +83,8 @@
@Override
public Response<?> apply(ChangeResource rsrc, Rebase.Input in)
- throws AuthException, ResourceConflictException, IOException, OrmException {
+ throws AuthException, ResourceConflictException, IOException, OrmException,
+ PermissionBackendException {
Project.NameKey project = rsrc.getProject();
try (Repository repository = repositoryManager.openRepository(project)) {
editModifier.rebaseEdit(repository, rsrc.getControl());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 16e3a08..50c03fe 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -25,6 +25,7 @@
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.change.RebaseUtil.Base;
import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
@@ -133,7 +134,7 @@
@Override
public void updateRepo(RepoContext ctx)
throws MergeConflictException, InvalidChangeOperationException, RestApiException, IOException,
- OrmException, NoSuchChangeException {
+ OrmException, NoSuchChangeException, PermissionBackendException {
// Ok that originalPatchSet was not read in a transaction, since we just
// need its revision.
RevId oldRev = originalPatchSet.getRevision();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
index 75da89d..8d3cbae 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -37,6 +37,9 @@
import com.google.gerrit.server.edit.tree.TreeCreator;
import com.google.gerrit.server.edit.tree.TreeModification;
import com.google.gerrit.server.index.change.ChangeIndexer;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gwtorm.server.OrmException;
@@ -78,6 +81,7 @@
private final ChangeIndexer indexer;
private final Provider<ReviewDb> reviewDb;
private final Provider<CurrentUser> currentUser;
+ private final PermissionBackend permissionBackend;
private final ChangeEditUtil changeEditUtil;
private final PatchSetUtil patchSetUtil;
@@ -87,11 +91,13 @@
ChangeIndexer indexer,
Provider<ReviewDb> reviewDb,
Provider<CurrentUser> currentUser,
+ PermissionBackend permissionBackend,
ChangeEditUtil changeEditUtil,
PatchSetUtil patchSetUtil) {
this.indexer = indexer;
this.reviewDb = reviewDb;
this.currentUser = currentUser;
+ this.permissionBackend = permissionBackend;
this.tz = gerritIdent.getTimeZone();
this.changeEditUtil = changeEditUtil;
this.patchSetUtil = patchSetUtil;
@@ -105,10 +111,12 @@
* be created
* @throws AuthException if the user isn't authenticated or not allowed to use change edits
* @throws InvalidChangeOperationException if a change edit already existed for the change
+ * @throws PermissionBackendException
*/
public void createEdit(Repository repository, ChangeControl changeControl)
- throws AuthException, IOException, InvalidChangeOperationException, OrmException {
- ensureAuthenticatedAndPermitted(changeControl);
+ throws AuthException, IOException, InvalidChangeOperationException, OrmException,
+ PermissionBackendException {
+ assertCanEdit(changeControl);
Optional<ChangeEdit> changeEdit = lookupChangeEdit(changeControl);
if (changeEdit.isPresent()) {
@@ -132,11 +140,12 @@
* change, the change edit is already based on the latest patch set, or the change represents
* the root commit
* @throws MergeConflictException if rebase fails due to merge conflicts
+ * @throws PermissionBackendException
*/
public void rebaseEdit(Repository repository, ChangeControl changeControl)
throws AuthException, InvalidChangeOperationException, IOException, OrmException,
- MergeConflictException {
- ensureAuthenticatedAndPermitted(changeControl);
+ MergeConflictException, PermissionBackendException {
+ assertCanEdit(changeControl);
Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(changeControl);
if (!optionalChangeEdit.isPresent()) {
@@ -195,11 +204,13 @@
* @param newCommitMessage the new commit message
* @throws AuthException if the user isn't authenticated or not allowed to use change edits
* @throws UnchangedCommitMessageException if the commit message is the same as before
+ * @throws PermissionBackendException
*/
public void modifyMessage(
Repository repository, ChangeControl changeControl, String newCommitMessage)
- throws AuthException, IOException, UnchangedCommitMessageException, OrmException {
- ensureAuthenticatedAndPermitted(changeControl);
+ throws AuthException, IOException, UnchangedCommitMessageException, OrmException,
+ PermissionBackendException {
+ assertCanEdit(changeControl);
newCommitMessage = getWellFormedCommitMessage(newCommitMessage);
Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(changeControl);
@@ -236,10 +247,12 @@
* @param newContent the new file content
* @throws AuthException if the user isn't authenticated or not allowed to use change edits
* @throws InvalidChangeOperationException if the file already had the specified content
+ * @throws PermissionBackendException
*/
public void modifyFile(
Repository repository, ChangeControl changeControl, String filePath, RawInput newContent)
- throws AuthException, InvalidChangeOperationException, IOException, OrmException {
+ throws AuthException, InvalidChangeOperationException, IOException, OrmException,
+ PermissionBackendException {
modifyTree(repository, changeControl, new ChangeFileContentModification(filePath, newContent));
}
@@ -253,9 +266,11 @@
* @param file path of the file which should be deleted
* @throws AuthException if the user isn't authenticated or not allowed to use change edits
* @throws InvalidChangeOperationException if the file does not exist
+ * @throws PermissionBackendException
*/
public void deleteFile(Repository repository, ChangeControl changeControl, String file)
- throws AuthException, InvalidChangeOperationException, IOException, OrmException {
+ throws AuthException, InvalidChangeOperationException, IOException, OrmException,
+ PermissionBackendException {
modifyTree(repository, changeControl, new DeleteFileModification(file));
}
@@ -271,13 +286,15 @@
* @throws AuthException if the user isn't authenticated or not allowed to use change edits
* @throws InvalidChangeOperationException if the file was already renamed to the specified new
* name
+ * @throws PermissionBackendException
*/
public void renameFile(
Repository repository,
ChangeControl changeControl,
String currentFilePath,
String newFilePath)
- throws AuthException, InvalidChangeOperationException, IOException, OrmException {
+ throws AuthException, InvalidChangeOperationException, IOException, OrmException,
+ PermissionBackendException {
modifyTree(repository, changeControl, new RenameFileModification(currentFilePath, newFilePath));
}
@@ -292,16 +309,19 @@
* @param file the path of the file which should be restored
* @throws AuthException if the user isn't authenticated or not allowed to use change edits
* @throws InvalidChangeOperationException if the file was already restored
+ * @throws PermissionBackendException
*/
public void restoreFile(Repository repository, ChangeControl changeControl, String file)
- throws AuthException, InvalidChangeOperationException, IOException, OrmException {
+ throws AuthException, InvalidChangeOperationException, IOException, OrmException,
+ PermissionBackendException {
modifyTree(repository, changeControl, new RestoreFileModification(file));
}
private void modifyTree(
Repository repository, ChangeControl changeControl, TreeModification treeModification)
- throws AuthException, IOException, OrmException, InvalidChangeOperationException {
- ensureAuthenticatedAndPermitted(changeControl);
+ throws AuthException, IOException, OrmException, InvalidChangeOperationException,
+ PermissionBackendException {
+ assertCanEdit(changeControl);
Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(changeControl);
PatchSet basePatchSet = getBasePatchSet(optionalChangeEdit, changeControl);
@@ -345,8 +365,8 @@
PatchSet patchSet,
List<TreeModification> treeModifications)
throws AuthException, IOException, InvalidChangeOperationException, MergeConflictException,
- OrmException {
- ensureAuthenticatedAndPermitted(changeControl);
+ OrmException, PermissionBackendException {
+ assertCanEdit(changeControl);
Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(changeControl);
ensureAllowedPatchSet(changeControl, optionalChangeEdit, patchSet);
@@ -375,21 +395,19 @@
return createEdit(repository, changeControl, patchSet, newEditCommit, nowTimestamp);
}
- private void ensureAuthenticatedAndPermitted(ChangeControl changeControl)
- throws AuthException, OrmException {
- ensureAuthenticated();
- ensurePermitted(changeControl);
- }
-
- private void ensureAuthenticated() throws AuthException {
+ private void assertCanEdit(ChangeControl changeControl)
+ throws AuthException, PermissionBackendException {
if (!currentUser.get().isIdentifiedUser()) {
throw new AuthException("Authentication required");
}
- }
-
- private void ensurePermitted(ChangeControl changeControl) throws OrmException, AuthException {
- if (!changeControl.canAddPatchSet(reviewDb.get())) {
- throw new AuthException("Not allowed to edit a change.");
+ try {
+ permissionBackend
+ .user(currentUser)
+ .database(reviewDb)
+ .change(changeControl.getNotes())
+ .check(ChangePermission.ADD_PATCH_SET);
+ } catch (AuthException denied) {
+ throw new AuthException("edit not permitted", denied);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index bb07884..ac504c4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.git;
+import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
@@ -104,6 +105,7 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
+import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -1119,24 +1121,30 @@
return false;
}
- private void parseDelete(ReceiveCommand cmd) {
+ private void parseDelete(ReceiveCommand cmd) throws PermissionBackendException {
logDebug("Deleting {}", cmd);
- RefControl ctl = projectControl.controlForRef(cmd.getRefName());
- if (ctl.getRefName().startsWith(REFS_CHANGES)) {
- errors.put(Error.DELETE_CHANGES, ctl.getRefName());
+ if (cmd.getRefName().startsWith(REFS_CHANGES)) {
+ errors.put(Error.DELETE_CHANGES, cmd.getRefName());
reject(cmd, "cannot delete changes");
- } else if (ctl.canDelete()) {
+ } else if (canDelete(cmd)) {
if (!validRefOperation(cmd)) {
return;
}
actualCommands.add(cmd);
+ } else if (RefNames.REFS_CONFIG.equals(cmd.getRefName())) {
+ reject(cmd, "cannot delete project configuration");
} else {
- if (RefNames.REFS_CONFIG.equals(ctl.getRefName())) {
- reject(cmd, "cannot delete project configuration");
- } else {
- errors.put(Error.DELETE, ctl.getRefName());
- reject(cmd, "cannot delete references");
- }
+ errors.put(Error.DELETE, cmd.getRefName());
+ reject(cmd, "cannot delete references");
+ }
+ }
+
+ private boolean canDelete(ReceiveCommand cmd) throws PermissionBackendException {
+ try {
+ permissions.ref(cmd.getRefName()).check(RefPermission.DELETE);
+ return true;
+ } catch (AuthException e) {
+ return false;
}
}
@@ -1187,6 +1195,7 @@
final ReceiveCommand cmd;
final LabelTypes labelTypes;
final NotesMigration notesMigration;
+ private final boolean defaultPublishComments;
Branch.NameKey dest;
RefControl ctl;
Set<Account.Id> reviewer = Sets.newLinkedHashSet();
@@ -1235,6 +1244,16 @@
@Option(name = "--merged", usage = "create single change for a merged commit")
boolean merged;
+ @Option(name = "--publish-comments", usage = "publish all draft comments on updated changes")
+ private boolean publishComments;
+
+ @Option(
+ name = "--no-publish-comments",
+ aliases = {"--np"},
+ usage = "do not publish draft comments"
+ )
+ private boolean noPublishComments;
+
@Option(
name = "--notify",
usage =
@@ -1318,11 +1337,17 @@
//TODO(dpursehouse): validate hashtags
}
- MagicBranchInput(ReceiveCommand cmd, LabelTypes labelTypes, NotesMigration notesMigration) {
+ MagicBranchInput(
+ IdentifiedUser user,
+ ReceiveCommand cmd,
+ LabelTypes labelTypes,
+ NotesMigration notesMigration) {
this.cmd = cmd;
this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE);
this.labelTypes = labelTypes;
this.notesMigration = notesMigration;
+ this.defaultPublishComments =
+ firstNonNull(user.getAccount().getGeneralPreferencesInfo().publishCommentsOnPush, false);
}
MailRecipients getMailRecipients() {
@@ -1338,6 +1363,15 @@
return accountsToNotify;
}
+ boolean shouldPublishComments() {
+ if (publishComments) {
+ return true;
+ } else if (noPublishComments) {
+ return false;
+ }
+ return defaultPublishComments;
+ }
+
String parse(
CmdLineParser clp,
Repository repo,
@@ -1407,7 +1441,7 @@
}
logDebug("Found magic branch {}", cmd.getRefName());
- magicBranch = new MagicBranchInput(cmd, labelTypes, notesMigration);
+ magicBranch = new MagicBranchInput(user, cmd, labelTypes, notesMigration);
magicBranch.reviewer.addAll(reviewersFromCommandLine);
magicBranch.cc.addAll(ccFromCommandLine);
@@ -1450,7 +1484,8 @@
magicBranch.dest = new Branch.NameKey(project.getNameKey(), ref);
magicBranch.ctl = projectControl.controlForRef(ref);
- if (!magicBranch.ctl.canWrite()) {
+ if (projectControl.getProject().getState()
+ != com.google.gerrit.extensions.client.ProjectState.ACTIVE) {
reject(cmd, "project is read only");
return;
}
@@ -1484,6 +1519,11 @@
reject(cmd, "the options 'wip' and 'ready' are mutually exclusive");
return;
}
+ if (magicBranch.publishComments && magicBranch.noPublishComments) {
+ reject(
+ cmd, "the options 'publish-comments' and 'no-publish-comments' are mutually exclusive");
+ return;
+ }
if (magicBranch.draft && magicBranch.submit) {
reject(cmd, "cannot submit draft");
@@ -2219,7 +2259,7 @@
req.inputCommand.setResult(REJECTED_OTHER_REASON, "internal server error");
}
}
- } catch (IOException err) {
+ } catch (IOException | PermissionBackendException err) {
logError(
String.format(
"Cannot read repository before replacement for project %s", project.getName()),
@@ -2260,7 +2300,6 @@
final ReceiveCommand inputCommand;
final boolean checkMergedInto;
ChangeNotes notes;
- ChangeControl changeCtl;
BiMap<RevCommit, PatchSet.Id> revisions;
PatchSet.Id psId;
ReceiveCommand prev;
@@ -2308,8 +2347,10 @@
* @return whether the new commit is valid
* @throws IOException
* @throws OrmException
+ * @throws PermissionBackendException
*/
- boolean validate(boolean autoClose) throws IOException, OrmException {
+ boolean validate(boolean autoClose)
+ throws IOException, OrmException, PermissionBackendException {
if (!autoClose && inputCommand.getResult() != NOT_ATTEMPTED) {
return false;
} else if (notes == null) {
@@ -2317,7 +2358,8 @@
return false;
}
- priorPatchSet = notes.getChange().currentPatchSetId();
+ Change change = notes.getChange();
+ priorPatchSet = change.currentPatchSetId();
if (!revisions.containsValue(priorPatchSet)) {
reject(inputCommand, "change " + ontoChange + " missing revisions");
return false;
@@ -2325,16 +2367,18 @@
RevCommit newCommit = rp.getRevWalk().parseCommit(newCommitId);
RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
-
- changeCtl = projectControl.controlFor(notes);
- if (!changeCtl.canAddPatchSet(db)) {
+ try {
+ permissions.change(notes).database(db).check(ChangePermission.ADD_PATCH_SET);
+ } catch (AuthException no) {
String locked = ".";
- if (changeCtl.isPatchSetLocked(db)) {
+ if (projectControl.controlFor(notes).isPatchSetLocked(db)) {
locked = ". Change is patch set locked.";
}
reject(inputCommand, "cannot add patch set to " + ontoChange + locked);
return false;
- } else if (notes.getChange().getStatus().isClosed()) {
+ }
+
+ if (change.getStatus().isClosed()) {
reject(inputCommand, "change " + ontoChange + " closed");
return false;
} else if (revisions.containsKey(newCommit)) {
@@ -2359,6 +2403,7 @@
}
}
+ ChangeControl changeCtl = projectControl.controlFor(notes);
if (!validCommit(rp.getRevWalk(), changeCtl.getRefControl(), inputCommand, newCommit)) {
return false;
}
@@ -2410,7 +2455,7 @@
Optional<ChangeEdit> edit = null;
try {
- edit = editUtil.byChange(changeCtl);
+ edit = editUtil.byChange(projectControl.controlFor(notes));
} catch (AuthException | IOException e) {
logError("Cannot retrieve edit", e);
return false;
@@ -2837,7 +2882,7 @@
bu.execute();
} catch (RestApiException e) {
logError("Can't insert patchset", e);
- } catch (IOException | OrmException | UpdateException e) {
+ } catch (IOException | OrmException | UpdateException | PermissionBackendException e) {
logError("Can't scan for changes to close", e);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
index 0cee090..382af51 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.git;
import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
+import static com.google.gerrit.server.ChangeMessagesUtil.TAG_UPLOADED_PATCH_SET;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
@@ -22,6 +23,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -30,20 +32,24 @@
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.server.ApprovalCopier;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
+import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.change.ChangeKindCache;
+import com.google.gerrit.server.change.EmailReviewComments;
import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.extensions.events.RevisionCreated;
import com.google.gerrit.server.git.ReceiveCommits.MagicBranchInput;
import com.google.gerrit.server.mail.MailUtil.MailRecipients;
import com.google.gerrit.server.mail.send.ReplacePatchSetSender;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectControl;
@@ -100,6 +106,8 @@
private final ChangeData.Factory changeDataFactory;
private final ChangeKindCache changeKindCache;
private final ChangeMessagesUtil cmUtil;
+ private final CommentsUtil commentsUtil;
+ private final EmailReviewComments.Factory emailCommentsFactory;
private final ExecutorService sendEmailExecutor;
private final RevisionCreated revisionCreated;
private final CommentAdded commentAdded;
@@ -123,10 +131,11 @@
private final MailRecipients recipients = new MailRecipients();
private RevCommit commit;
private ReceiveCommand cmd;
- private Change change;
+ private ChangeNotes notes;
private PatchSet newPatchSet;
private ChangeKind changeKind;
private ChangeMessage msg;
+ private List<Comment> comments = ImmutableList.of();
private String rejectMessage;
private MergedByPushOp mergedByPushOp;
private RequestScopePropagator requestScopePropagator;
@@ -140,6 +149,8 @@
ChangeData.Factory changeDataFactory,
ChangeKindCache changeKindCache,
ChangeMessagesUtil cmUtil,
+ CommentsUtil commentsUtil,
+ EmailReviewComments.Factory emailCommentsFactory,
RevisionCreated revisionCreated,
CommentAdded commentAdded,
MergedByPushOp.Factory mergedByPushOpFactory,
@@ -164,6 +175,8 @@
this.changeDataFactory = changeDataFactory;
this.changeKindCache = changeKindCache;
this.cmUtil = cmUtil;
+ this.commentsUtil = commentsUtil;
+ this.emailCommentsFactory = emailCommentsFactory;
this.revisionCreated = revisionCreated;
this.commentAdded = commentAdded;
this.mergedByPushOpFactory = mergedByPushOpFactory;
@@ -211,13 +224,14 @@
@Override
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
- change = ctx.getChange();
+ notes = ctx.getNotes();
+ Change change = notes.getChange();
if (change == null || change.getStatus().isClosed()) {
rejectMessage = CHANGE_IS_CLOSED;
return false;
}
if (groups.isEmpty()) {
- PatchSet prevPs = psUtil.current(ctx.getDb(), ctx.getNotes());
+ PatchSet prevPs = psUtil.current(ctx.getDb(), notes);
groups = prevPs != null ? prevPs.getGroups() : ImmutableList.<String>of();
}
@@ -233,7 +247,7 @@
approvals.putAll(magicBranch.labels);
Set<String> hashtags = magicBranch.hashtags;
if (hashtags != null && !hashtags.isEmpty()) {
- hashtags.addAll(ctx.getNotes().getHashtags());
+ hashtags.addAll(notes.getHashtags());
update.setHashtags(hashtags);
}
if (magicBranch.topic != null && !magicBranch.topic.equals(ctx.getChange().getTopic())) {
@@ -253,6 +267,9 @@
change.setWorkInProgress(true);
update.setWorkInProgress(true);
}
+ if (shouldPublishComments()) {
+ comments = publishComments(ctx);
+ }
}
boolean draft = magicBranch != null && magicBranch.draft;
@@ -305,6 +322,20 @@
recipients.add(oldRecipients);
+ msg = createChangeMessage(ctx, reviewMessage);
+ cmUtil.addChangeMessage(ctx.getDb(), update, msg);
+
+ if (mergedByPushOp == null) {
+ resetChange(ctx);
+ } else {
+ mergedByPushOp.setPatchSetProvider(Providers.of(newPatchSet)).updateChange(ctx);
+ }
+
+ return true;
+ }
+
+ private ChangeMessage createChangeMessage(ChangeContext ctx, String reviewMessage)
+ throws OrmException {
String approvalMessage =
ApprovalsUtil.renderMessageWithApprovals(
patchSetId.get(), approvals, scanLabels(ctx, approvals));
@@ -315,26 +346,21 @@
} else {
message.append('.');
}
+ if (comments.size() == 1) {
+ message.append("\n\n(1 comment)");
+ } else if (comments.size() > 1) {
+ message.append(String.format("\n\n(%d comments)", comments.size()));
+ }
if (!Strings.isNullOrEmpty(reviewMessage)) {
- message.append("\n").append(reviewMessage);
+ message.append("\n\n").append(reviewMessage);
}
boolean workInProgress = magicBranch != null && magicBranch.workInProgress;
- msg =
- ChangeMessagesUtil.newMessage(
- patchSetId,
- ctx.getUser(),
- ctx.getWhen(),
- message.toString(),
- ChangeMessagesUtil.uploadedPatchSetTag(workInProgress));
- cmUtil.addChangeMessage(ctx.getDb(), update, msg);
-
- if (mergedByPushOp == null) {
- resetChange(ctx);
- } else {
- mergedByPushOp.setPatchSetProvider(Providers.of(newPatchSet)).updateChange(ctx);
- }
-
- return true;
+ return ChangeMessagesUtil.newMessage(
+ patchSetId,
+ ctx.getUser(),
+ ctx.getWhen(),
+ message.toString(),
+ ChangeMessagesUtil.uploadedPatchSetTag(workInProgress));
}
private String changeKindMessage(ChangeKind changeKind) {
@@ -396,50 +422,45 @@
}
}
+ private List<Comment> publishComments(ChangeContext ctx) throws OrmException {
+ List<Comment> comments =
+ commentsUtil.draftByChangeAuthor(ctx.getDb(), ctx.getNotes(), ctx.getUser().getAccountId());
+ commentsUtil.publish(ctx, patchSetId, comments, TAG_UPLOADED_PATCH_SET);
+ return comments;
+ }
+
@Override
public void postUpdate(Context ctx) throws Exception {
if (changeKind != ChangeKind.TRIVIAL_REBASE) {
- Runnable sender =
- new Runnable() {
- @Override
- public void run() {
- try {
- ReplacePatchSetSender cm =
- replacePatchSetFactory.create(
- projectControl.getProject().getNameKey(), change.getId());
- cm.setFrom(ctx.getAccountId());
- cm.setPatchSet(newPatchSet, info);
- cm.setChangeMessage(msg.getMessage(), ctx.getWhen());
- if (magicBranch != null) {
- cm.setNotify(magicBranch.notify);
- cm.setAccountsToNotify(magicBranch.getAccountsToNotify());
- }
- cm.addReviewers(recipients.getReviewers());
- cm.addExtraCC(recipients.getCcOnly());
- cm.send();
- } catch (Exception e) {
- log.error("Cannot send email for new patch set " + newPatchSet.getId(), e);
- }
- }
-
- @Override
- public String toString() {
- return "send-email newpatchset";
- }
- };
-
+ // TODO(dborowitz): Merge email templates so we only have to send one.
+ Runnable e = new ReplaceEmailTask(ctx);
if (requestScopePropagator != null) {
@SuppressWarnings("unused")
- Future<?> possiblyIgnoredError =
- sendEmailExecutor.submit(requestScopePropagator.wrap(sender));
+ Future<?> possiblyIgnoredError = sendEmailExecutor.submit(requestScopePropagator.wrap(e));
} else {
- sender.run();
+ e.run();
}
}
NotifyHandling notify =
magicBranch != null && magicBranch.notify != null ? magicBranch.notify : NotifyHandling.ALL;
- revisionCreated.fire(change, newPatchSet, ctx.getAccount(), ctx.getWhen(), notify);
+
+ if (shouldPublishComments()) {
+ emailCommentsFactory
+ .create(
+ notify,
+ magicBranch != null ? magicBranch.getAccountsToNotify() : ImmutableListMultimap.of(),
+ notes,
+ newPatchSet,
+ ctx.getUser().asIdentifiedUser(),
+ msg,
+ comments,
+ msg.getMessage(),
+ ImmutableList.of()) // TODO(dborowitz): Include labels.
+ .sendAsync();
+ }
+
+ revisionCreated.fire(notes.getChange(), newPatchSet, ctx.getAccount(), ctx.getWhen(), notify);
try {
fireCommentAddedEvent(ctx);
} catch (Exception e) {
@@ -450,6 +471,40 @@
}
}
+ private class ReplaceEmailTask implements Runnable {
+ private final Context ctx;
+
+ private ReplaceEmailTask(Context ctx) {
+ this.ctx = ctx;
+ }
+
+ @Override
+ public void run() {
+ try {
+ ReplacePatchSetSender cm =
+ replacePatchSetFactory.create(
+ projectControl.getProject().getNameKey(), notes.getChangeId());
+ cm.setFrom(ctx.getAccount().getId());
+ cm.setPatchSet(newPatchSet, info);
+ cm.setChangeMessage(msg.getMessage(), ctx.getWhen());
+ if (magicBranch != null) {
+ cm.setNotify(magicBranch.notify);
+ cm.setAccountsToNotify(magicBranch.getAccountsToNotify());
+ }
+ cm.addReviewers(recipients.getReviewers());
+ cm.addExtraCC(recipients.getCcOnly());
+ cm.send();
+ } catch (Exception e) {
+ log.error("Cannot send email for new patch set " + newPatchSet.getId(), e);
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "send-email newpatchset";
+ }
+ }
+
private void fireCommentAddedEvent(Context ctx) throws OrmException {
if (approvals.isEmpty()) {
return;
@@ -461,7 +516,7 @@
* show a transition from an oldValue of 0 to the new value.
*/
ChangeControl changeControl =
- changeControlFactory.controlFor(ctx.getDb(), change, ctx.getUser());
+ changeControlFactory.controlFor(ctx.getDb(), notes.getChange(), ctx.getUser());
List<LabelType> labels = changeControl.getLabelTypes().getLabelTypes();
Map<String, Short> allApprovals = new HashMap<>();
Map<String, Short> oldApprovals = new HashMap<>();
@@ -477,7 +532,13 @@
}
commentAdded.fire(
- change, newPatchSet, ctx.getAccount(), null, allApprovals, oldApprovals, ctx.getWhen());
+ notes.getChange(),
+ newPatchSet,
+ ctx.getAccount(),
+ null,
+ allApprovals,
+ oldApprovals,
+ ctx.getWhen());
}
public PatchSet getPatchSet() {
@@ -485,7 +546,7 @@
}
public Change getChange() {
- return change;
+ return notes.getChange();
}
public String getRejectMessage() {
@@ -520,4 +581,8 @@
return null;
}
}
+
+ private boolean shouldPublishComments() {
+ return magicBranch != null && magicBranch.shouldPublishComments();
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
index a156a90..40bb6c1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
@@ -29,6 +29,7 @@
import com.google.gerrit.server.git.MergeIdenticalTreeException;
import com.google.gerrit.server.git.MergeTip;
import com.google.gerrit.server.git.RebaseSorter;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.update.ChangeContext;
@@ -114,7 +115,7 @@
@Override
public void updateRepoImpl(RepoContext ctx)
throws IntegrationException, InvalidChangeOperationException, RestApiException, IOException,
- OrmException {
+ OrmException, PermissionBackendException {
if (args.mergeUtil.canFastForward(
args.mergeSorter, args.mergeTip.getCurrentTip(), args.rw, toMerge)) {
if (!rebaseAlways) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
index 156f106..9aa8d27 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -219,6 +219,20 @@
/** @return instance scoped for {@code ref} in this project. */
public abstract ForRef ref(String ref);
+ /** @return instance scoped for the change, and its destination ref and project. */
+ public ForChange change(ChangeData cd) {
+ try {
+ return ref(cd.change().getDest().get()).change(cd);
+ } catch (OrmException e) {
+ return FailedPermissionBackend.change("unavailable", e);
+ }
+ }
+
+ /** @return instance scoped for the change, and its destination ref and project. */
+ public ForChange change(ChangeNotes notes) {
+ return ref(notes.getChange().getDest().get()).change(notes);
+ }
+
/** Verify scoped user can {@code perm}, throwing if denied. */
public abstract void check(ProjectPermission perm)
throws AuthException, PermissionBackendException;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index 16c63c8..2cb8d96 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -252,11 +252,6 @@
&& !isPatchSetLocked(db);
}
- /** Can this user change the destination branch of this change to the new ref? */
- public boolean canMoveTo(String ref, ReviewDb db) throws OrmException {
- return getProjectControl().controlForRef(ref).canUpload() && canAbandon(db);
- }
-
/** Can this user publish this draft change or any draft patch set of this change? */
public boolean canPublish(final ReviewDb db) throws OrmException {
return (isOwner() || getRefControl().canPublishDrafts()) && isVisible(db);
@@ -330,7 +325,7 @@
}
/** Can this user add a patch set to this change? */
- public boolean canAddPatchSet(ReviewDb db) throws OrmException {
+ private boolean canAddPatchSet(ReviewDb db) throws OrmException {
if (!getRefControl().canUpload()
|| isPatchSetLocked(db)
|| !isPatchVisible(patchSetUtil.current(db, notes), db)) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java
index 5919ba1..0598d75 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java
@@ -26,6 +26,8 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.util.MagicBranch;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -50,6 +52,7 @@
}
private final Provider<IdentifiedUser> identifiedUser;
+ private final PermissionBackend permissionBackend;
private final GitRepositoryManager repoManager;
private final Provider<ReviewDb> db;
private final GitReferenceUpdated referenceUpdated;
@@ -59,12 +62,14 @@
@Inject
CreateBranch(
Provider<IdentifiedUser> identifiedUser,
+ PermissionBackend permissionBackend,
GitRepositoryManager repoManager,
Provider<ReviewDb> db,
GitReferenceUpdated referenceUpdated,
RefValidationHelper.Factory refHelperFactory,
@Assisted String ref) {
this.identifiedUser = identifiedUser;
+ this.permissionBackend = permissionBackend;
this.repoManager = repoManager;
this.db = db;
this.referenceUpdated = referenceUpdated;
@@ -170,7 +175,10 @@
BranchInfo info = new BranchInfo();
info.ref = ref;
info.revision = revid.getName();
- info.canDelete = refControl.canDelete() ? true : null;
+ info.canDelete =
+ permissionBackend.user(identifiedUser).ref(name).testOrFalse(RefPermission.DELETE)
+ ? true
+ : null;
return info;
} catch (IOException err) {
log.error("Cannot create branch \"" + name + "\"", err);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateTag.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateTag.java
index f674d17..6b1c0e9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateTag.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateTag.java
@@ -31,6 +31,9 @@
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.TagCache;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.RefUtil.InvalidRevisionException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -56,6 +59,7 @@
CreateTag create(String ref);
}
+ private final PermissionBackend permissionBackend;
private final Provider<IdentifiedUser> identifiedUser;
private final GitRepositoryManager repoManager;
private final TagCache tagCache;
@@ -64,11 +68,13 @@
@Inject
CreateTag(
+ PermissionBackend permissionBackend,
Provider<IdentifiedUser> identifiedUser,
GitRepositoryManager repoManager,
TagCache tagCache,
GitReferenceUpdated referenceUpdated,
@Assisted String ref) {
+ this.permissionBackend = permissionBackend;
this.identifiedUser = identifiedUser;
this.repoManager = repoManager;
this.tagCache = tagCache;
@@ -78,7 +84,7 @@
@Override
public TagInfo apply(ProjectResource resource, TagInput input)
- throws RestApiException, IOException {
+ throws RestApiException, IOException, PermissionBackendException {
if (input == null) {
input = new TagInput();
}
@@ -92,6 +98,9 @@
ref = RefUtil.normalizeTagRef(ref);
RefControl refControl = resource.getControl().controlForRef(ref);
+ PermissionBackend.ForRef perm =
+ permissionBackend.user(identifiedUser).project(resource.getNameKey()).ref(ref);
+
try (Repository repo = repoManager.openRepository(resource.getNameKey())) {
ObjectId revid = RefUtil.parseBaseRevision(repo, resource.getNameKey(), input.revision);
RevWalk rw = RefUtil.verifyConnected(repo, revid);
@@ -103,8 +112,8 @@
throw new MethodNotAllowedException("Cannot create signed tag \"" + ref + "\"");
} else if (isAnnotated && !refControl.canPerform(Permission.CREATE_TAG)) {
throw new AuthException("Cannot create annotated tag \"" + ref + "\"");
- } else if (!refControl.canPerform(Permission.CREATE)) {
- throw new AuthException("Cannot create tag \"" + ref + "\"");
+ } else {
+ perm.check(RefPermission.CREATE);
}
if (repo.getRefDatabase().exactRef(ref) != null) {
throw new ResourceConflictException("tag \"" + ref + "\" already exists");
@@ -134,7 +143,7 @@
result.getObjectId(),
identifiedUser.get().getAccount());
try (RevWalk w = new RevWalk(repo)) {
- return ListTags.createTagInfo(result, w, refControl);
+ return ListTags.createTagInfo(perm, result, w);
}
}
} catch (InvalidRevisionException e) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java
index 4601bfe..2fad19b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java
@@ -14,11 +14,14 @@
package com.google.gerrit.server.project;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.DeleteBranch.Input;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -33,19 +36,25 @@
private final Provider<InternalChangeQuery> queryProvider;
private final DeleteRef.Factory deleteRefFactory;
+ private final Provider<CurrentUser> user;
+ private final PermissionBackend permissionBackend;
@Inject
- DeleteBranch(Provider<InternalChangeQuery> queryProvider, DeleteRef.Factory deleteRefFactory) {
+ DeleteBranch(
+ Provider<InternalChangeQuery> queryProvider,
+ DeleteRef.Factory deleteRefFactory,
+ Provider<CurrentUser> user,
+ PermissionBackend permissionBackend) {
this.queryProvider = queryProvider;
this.deleteRefFactory = deleteRefFactory;
+ this.user = user;
+ this.permissionBackend = permissionBackend;
}
@Override
public Response<?> apply(BranchResource rsrc, Input input)
- throws RestApiException, OrmException, IOException {
- if (!rsrc.getControl().controlForRef(rsrc.getBranchKey()).canDelete()) {
- throw new AuthException("Cannot delete branch");
- }
+ throws RestApiException, OrmException, IOException, PermissionBackendException {
+ permissionBackend.user(user).ref(rsrc.getBranchKey()).check(RefPermission.DELETE);
if (!queryProvider.get().setLimit(1).byBranchOpen(rsrc.getBranchKey()).isEmpty()) {
throw new ResourceConflictException("branch " + rsrc.getBranchKey() + " has open changes");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java
index f5e55b1..4b45a41 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java
@@ -19,6 +19,7 @@
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -35,12 +36,10 @@
@Override
public Response<?> apply(ProjectResource project, DeleteBranchesInput input)
- throws OrmException, IOException, RestApiException {
-
+ throws OrmException, IOException, RestApiException, PermissionBackendException {
if (input == null || input.branches == null || input.branches.isEmpty()) {
throw new BadRequestException("branches must be specified");
}
-
deleteRefFactory.create(project).refs(input.branches).delete();
return Response.none();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java
index 78a78c2..9b6538c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java
@@ -19,11 +19,15 @@
import static org.eclipse.jgit.lib.Constants.R_TAGS;
import static org.eclipse.jgit.transport.ReceiveCommand.Type.DELETE;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -52,6 +56,7 @@
private static final long SLEEP_ON_LOCK_FAILURE_MS = 15;
private final Provider<IdentifiedUser> identifiedUser;
+ private final PermissionBackend permissionBackend;
private final GitRepositoryManager repoManager;
private final GitReferenceUpdated referenceUpdated;
private final RefValidationHelper refDeletionValidator;
@@ -67,12 +72,14 @@
@Inject
DeleteRef(
Provider<IdentifiedUser> identifiedUser,
+ PermissionBackend permissionBackend,
GitRepositoryManager repoManager,
GitReferenceUpdated referenceUpdated,
RefValidationHelper.Factory refDeletionValidatorFactory,
Provider<InternalChangeQuery> queryProvider,
@Assisted ProjectResource resource) {
this.identifiedUser = identifiedUser;
+ this.permissionBackend = permissionBackend;
this.repoManager = repoManager;
this.referenceUpdated = referenceUpdated;
this.refDeletionValidator = refDeletionValidatorFactory.create(DELETE);
@@ -96,7 +103,8 @@
return this;
}
- public void delete() throws OrmException, IOException, ResourceConflictException {
+ public void delete()
+ throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
if (!refsToDelete.isEmpty()) {
try (Repository r = repoManager.openRepository(resource.getNameKey())) {
if (refsToDelete.size() == 1) {
@@ -168,7 +176,7 @@
}
private void deleteMultipleRefs(Repository r)
- throws OrmException, IOException, ResourceConflictException {
+ throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
BatchRefUpdate batchUpdate = r.getRefDatabase().newBatchUpdate();
batchUpdate.setAtomic(false);
List<String> refs =
@@ -198,7 +206,7 @@
}
private ReceiveCommand createDeleteCommand(ProjectResource project, Repository r, String refName)
- throws OrmException, IOException, ResourceConflictException {
+ throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
Ref ref = r.getRefDatabase().getRef(refName);
ReceiveCommand command;
if (ref == null) {
@@ -210,7 +218,13 @@
}
command = new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), ref.getName());
- if (!project.getControl().controlForRef(refName).canDelete()) {
+ try {
+ permissionBackend
+ .user(identifiedUser)
+ .project(project.getNameKey())
+ .ref(refName)
+ .check(RefPermission.DELETE);
+ } catch (AuthException denied) {
command.setResult(
Result.REJECTED_OTHER_REASON,
"it doesn't exist or you do not have permission to delete it");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteTag.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteTag.java
index f26d40f..a05fa2e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteTag.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteTag.java
@@ -14,36 +14,46 @@
package com.google.gerrit.server.project;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
@Singleton
public class DeleteTag implements RestModifyView<TagResource, DeleteTag.Input> {
- private final DeleteRef.Factory deleteRefFactory;
-
public static class Input {}
+ private final PermissionBackend permissionBackend;
+ private final Provider<CurrentUser> user;
+ private final DeleteRef.Factory deleteRefFactory;
+
@Inject
- DeleteTag(DeleteRef.Factory deleteRefFactory) {
+ DeleteTag(
+ PermissionBackend permissionBackend,
+ Provider<CurrentUser> user,
+ DeleteRef.Factory deleteRefFactory) {
+ this.permissionBackend = permissionBackend;
+ this.user = user;
this.deleteRefFactory = deleteRefFactory;
}
@Override
public Response<?> apply(TagResource resource, Input input)
- throws OrmException, RestApiException, IOException {
+ throws OrmException, RestApiException, IOException, PermissionBackendException {
String tag = RefUtil.normalizeTagRef(resource.getTagInfo().ref);
- RefControl refControl = resource.getControl().controlForRef(tag);
-
- if (!refControl.canDelete()) {
- throw new AuthException("Cannot delete tag");
- }
-
+ permissionBackend
+ .user(user)
+ .project(resource.getNameKey())
+ .ref(tag)
+ .check(RefPermission.DELETE);
deleteRefFactory.create(resource).ref(tag).delete();
return Response.none();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteTags.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteTags.java
index 75cf03f..c020351 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteTags.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteTags.java
@@ -21,6 +21,7 @@
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -37,12 +38,10 @@
@Override
public Response<?> apply(ProjectResource project, DeleteTagsInput input)
- throws OrmException, RestApiException, IOException {
-
+ throws OrmException, RestApiException, IOException, PermissionBackendException {
if (input == null || input.tags == null || input.tags.isEmpty()) {
throw new BadRequestException("tags must be specified");
}
-
deleteRefFactory.create(project).refs(input.tags).prefix(R_TAGS).delete();
return Response.none();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java
index 09a6b86..ecb3ea6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java
@@ -26,10 +26,14 @@
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.WebLinks;
import com.google.gerrit.server.extensions.webui.UiActions;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
@@ -46,6 +50,8 @@
public class ListBranches implements RestReadView<ProjectResource> {
private final GitRepositoryManager repoManager;
+ private final PermissionBackend permissionBackend;
+ private final Provider<CurrentUser> user;
private final DynamicMap<RestView<BranchResource>> branchViews;
private final UiActions uiActions;
private final WebLinks webLinks;
@@ -98,10 +104,14 @@
@Inject
public ListBranches(
GitRepositoryManager repoManager,
+ PermissionBackend permissionBackend,
+ Provider<CurrentUser> user,
DynamicMap<RestView<BranchResource>> branchViews,
UiActions uiActions,
WebLinks webLinks) {
this.repoManager = repoManager;
+ this.permissionBackend = permissionBackend;
+ this.user = user;
this.branchViews = branchViews;
this.uiActions = uiActions;
this.webLinks = webLinks;
@@ -140,6 +150,8 @@
}
}
+ ProjectControl pctl = rsrc.getControl();
+ PermissionBackend.ForProject perm = permissionBackend.user(user).project(rsrc.getNameKey());
List<BranchInfo> branches = new ArrayList<>(refs.size());
for (Ref ref : refs) {
if (ref.isSymbolic()) {
@@ -147,7 +159,7 @@
// showing the resolved value, show the name it references.
//
String target = ref.getTarget().getName();
- RefControl targetRefControl = rsrc.getControl().controlForRef(target);
+ RefControl targetRefControl = pctl.controlForRef(target);
if (!targetRefControl.isVisible()) {
continue;
}
@@ -161,14 +173,13 @@
branches.add(b);
if (!Constants.HEAD.equals(ref.getName())) {
- b.canDelete = targetRefControl.canDelete() ? true : null;
+ b.canDelete = perm.ref(ref.getName()).testOrFalse(RefPermission.DELETE) ? true : null;
}
continue;
}
- RefControl refControl = rsrc.getControl().controlForRef(ref.getName());
- if (refControl.isVisible()) {
- branches.add(createBranchInfo(ref, refControl, targets));
+ if (pctl.controlForRef(ref.getName()).isVisible()) {
+ branches.add(createBranchInfo(perm.ref(ref.getName()), ref, pctl, targets));
}
}
Collections.sort(branches, new BranchComparator());
@@ -194,13 +205,15 @@
}
}
- private BranchInfo createBranchInfo(Ref ref, RefControl refControl, Set<String> targets) {
+ private BranchInfo createBranchInfo(
+ PermissionBackend.ForRef perm, Ref ref, ProjectControl pctl, Set<String> targets) {
BranchInfo info = new BranchInfo();
info.ref = ref.getName();
info.revision = ref.getObjectId() != null ? ref.getObjectId().name() : null;
- info.canDelete = !targets.contains(ref.getName()) && refControl.canDelete() ? true : null;
+ info.canDelete =
+ !targets.contains(ref.getName()) && perm.testOrFalse(RefPermission.DELETE) ? true : null;
- BranchResource rsrc = new BranchResource(refControl.getProjectControl(), info);
+ BranchResource rsrc = new BranchResource(pctl, info);
for (UiAction.Description d : uiActions.from(branchViews, rsrc)) {
if (info.actions == null) {
info.actions = new TreeMap<>();
@@ -208,9 +221,7 @@
info.actions.put(d.getId(), new ActionInfo(d));
}
- List<WebLinkInfo> links =
- webLinks.getBranchLinks(
- refControl.getProjectControl().getProject().getName(), ref.getName());
+ List<WebLinkInfo> links = webLinks.getBranchLinks(pctl.getProject().getName(), ref.getName());
info.webLinks = links.isEmpty() ? null : links;
return info;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListTags.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListTags.java
index 7f1ee60..7cbea47 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListTags.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListTags.java
@@ -24,11 +24,14 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CommonConverters;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.SearchingChangeCacheImpl;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.VisibleRefFilter;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.RefPermission;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
@@ -50,6 +53,8 @@
public class ListTags implements RestReadView<ProjectResource> {
private final GitRepositoryManager repoManager;
+ private final PermissionBackend permissionBackend;
+ private final Provider<CurrentUser> user;
private final Provider<ReviewDb> dbProvider;
private final TagCache tagCache;
private final ChangeNotes.Factory changeNotesFactory;
@@ -103,11 +108,15 @@
@Inject
public ListTags(
GitRepositoryManager repoManager,
+ PermissionBackend permissionBackend,
+ Provider<CurrentUser> user,
Provider<ReviewDb> dbProvider,
TagCache tagCache,
ChangeNotes.Factory changeNotesFactory,
@Nullable SearchingChangeCacheImpl changeCache) {
this.repoManager = repoManager;
+ this.permissionBackend = permissionBackend;
+ this.user = user;
this.dbProvider = dbProvider;
this.tagCache = tagCache;
this.changeNotesFactory = changeNotesFactory;
@@ -119,13 +128,14 @@
throws IOException, ResourceNotFoundException, BadRequestException {
List<TagInfo> tags = new ArrayList<>();
+ PermissionBackend.ForProject perm = permissionBackend.user(user).project(resource.getNameKey());
try (Repository repo = getRepository(resource.getNameKey());
RevWalk rw = new RevWalk(repo)) {
- ProjectControl control = resource.getControl();
+ ProjectControl pctl = resource.getControl();
Map<String, Ref> all =
- visibleTags(control, repo, repo.getRefDatabase().getRefs(Constants.R_TAGS));
+ visibleTags(pctl, repo, repo.getRefDatabase().getRefs(Constants.R_TAGS));
for (Ref ref : all.values()) {
- tags.add(createTagInfo(ref, rw, control.controlForRef(ref.getName())));
+ tags.add(createTagInfo(perm.ref(ref.getName()), ref, rw));
}
}
@@ -158,15 +168,22 @@
ProjectControl control = resource.getControl();
if (ref != null
&& !visibleTags(control, repo, ImmutableMap.of(ref.getName(), ref)).isEmpty()) {
- return createTagInfo(ref, rw, control.controlForRef(ref.getName()));
+ return createTagInfo(
+ permissionBackend
+ .user(control.getUser())
+ .project(resource.getNameKey())
+ .ref(ref.getName()),
+ ref,
+ rw);
}
}
throw new ResourceNotFoundException(id);
}
- public static TagInfo createTagInfo(Ref ref, RevWalk rw, RefControl control)
+ public static TagInfo createTagInfo(PermissionBackend.ForRef perm, Ref ref, RevWalk rw)
throws MissingObjectException, IOException {
RevObject object = rw.parseAny(ref.getObjectId());
+ boolean canDelete = perm.testOrFalse(RefPermission.DELETE);
if (object instanceof RevTag) {
// Annotated or signed tag
RevTag tag = (RevTag) object;
@@ -177,10 +194,10 @@
tag.getObject().getName(),
tag.getFullMessage().trim(),
tagger != null ? CommonConverters.toGitPerson(tag.getTaggerIdent()) : null,
- control.canDelete());
+ canDelete);
}
// Lightweight tag
- return new TagInfo(ref.getName(), ref.getObjectId().getName(), control.canDelete());
+ return new TagInfo(ref.getName(), ref.getObjectId().getName(), canDelete);
}
private Repository getRepository(Project.NameKey project)
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index 6291fb0..8773bad 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.project;
+import static com.google.common.base.Preconditions.checkArgument;
+
import com.google.common.collect.Maps;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.PageLinks;
@@ -46,6 +48,8 @@
import com.google.gerrit.server.git.VisibleRefFilter;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.FailedPermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
import com.google.gerrit.server.permissions.PermissionBackend.ForProject;
import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -533,6 +537,31 @@
}
@Override
+ public ForChange change(ChangeData cd) {
+ try {
+ checkProject(cd.change());
+ return super.change(cd);
+ } catch (OrmException e) {
+ return FailedPermissionBackend.change("unavailable", e);
+ }
+ }
+
+ @Override
+ public ForChange change(ChangeNotes notes) {
+ checkProject(notes.getChange());
+ return super.change(notes);
+ }
+
+ private void checkProject(Change change) {
+ Project.NameKey project = getProject().getNameKey();
+ checkArgument(
+ project.equals(change.getProject()),
+ "expected change in project %s, not %s",
+ project,
+ change.getProject());
+ }
+
+ @Override
public void check(ProjectPermission perm) throws AuthException, PermissionBackendException {
if (!can(perm)) {
throw new AuthException(perm.describeForException() + " not permitted");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
index 812872c..bf53ab1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
@@ -22,6 +22,7 @@
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
@@ -116,7 +117,9 @@
/** Can this user see this reference exists? */
public boolean isVisible() {
if (isVisible == null) {
- isVisible = (getUser().isInternalUser() || canPerform(Permission.READ)) && canRead();
+ isVisible =
+ (getUser().isInternalUser() || canPerform(Permission.READ))
+ && isProjectStatePermittingRead();
}
return isVisible;
}
@@ -155,7 +158,7 @@
*/
public boolean canUpload() {
return projectControl.controlForRef("refs/for/" + getRefName()).canPerform(Permission.PUSH)
- && canWrite();
+ && isProjectStatePermittingWrite();
}
/** @return true if this user can add a new patch set to this ref */
@@ -163,7 +166,7 @@
return projectControl
.controlForRef("refs/for/" + getRefName())
.canPerform(Permission.ADD_PATCH_SET)
- && canWrite();
+ && isProjectStatePermittingWrite();
}
/** @return true if this user can submit merge patch sets to this ref */
@@ -171,12 +174,12 @@
return projectControl
.controlForRef("refs/for/" + getRefName())
.canPerform(Permission.PUSH_MERGE)
- && canWrite();
+ && isProjectStatePermittingWrite();
}
/** @return true if this user can rebase changes on this ref */
boolean canRebase() {
- return canPerform(Permission.REBASE) && canWrite();
+ return canPerform(Permission.REBASE) && isProjectStatePermittingWrite();
}
/** @return true if this user can submit patch sets to this ref */
@@ -189,7 +192,7 @@
// granting of powers beyond submitting to the configuration.
return projectControl.isOwner();
}
- return canPerform(Permission.SUBMIT, isChangeOwner) && canWrite();
+ return canPerform(Permission.SUBMIT, isChangeOwner) && isProjectStatePermittingWrite();
}
/** @return true if the user can update the reference as a fast-forward. */
@@ -209,12 +212,12 @@
return false;
}
}
- return canPerform(Permission.PUSH) && canWrite();
+ return canPerform(Permission.PUSH) && isProjectStatePermittingWrite();
}
/** @return true if the user can rewind (force push) the reference. */
private boolean canForceUpdate() {
- if (!canWrite()) {
+ if (!isProjectStatePermittingWrite()) {
return false;
}
@@ -237,16 +240,18 @@
}
}
- public boolean canWrite() {
+ private boolean isProjectStatePermittingWrite() {
return getProjectControl().getProject().getState().equals(ProjectState.ACTIVE);
}
- public boolean canRead() {
- return getProjectControl().getProject().getState().equals(ProjectState.READ_ONLY) || canWrite();
+ private boolean isProjectStatePermittingRead() {
+ return getProjectControl().getProject().getState().equals(ProjectState.READ_ONLY)
+ || isProjectStatePermittingWrite();
}
private boolean canPushWithForce() {
- if (!canWrite() || (RefNames.REFS_CONFIG.equals(refName) && !projectControl.isOwner())) {
+ if (!isProjectStatePermittingWrite()
+ || (RefNames.REFS_CONFIG.equals(refName) && !projectControl.isOwner())) {
// Pushing requires being at least project owner, in addition to push.
// Pushing configuration changes modifies the access control
// rules. Allowing this to be done by a non-project-owner opens
@@ -266,7 +271,7 @@
* @return {@code true} if the user specified can create a new Git ref
*/
public boolean canCreate(ReviewDb db, Repository repo, RevObject object) {
- if (!canWrite()) {
+ if (!isProjectStatePermittingWrite()) {
return false;
}
@@ -358,8 +363,8 @@
*
* @return {@code true} if the user specified can delete a Git ref.
*/
- public boolean canDelete() {
- if (!canWrite() || (RefNames.REFS_CONFIG.equals(refName))) {
+ private boolean canDelete() {
+ if (!isProjectStatePermittingWrite() || (RefNames.REFS_CONFIG.equals(refName))) {
// Never allow removal of the refs/meta/config branch.
// Deleting the branch would destroy all Gerrit specific
// metadata about the project, including its access rules.
@@ -683,10 +688,13 @@
@Override
public ForChange change(ChangeNotes notes) {
+ Project.NameKey project = getProjectControl().getProject().getNameKey();
Change change = notes.getChange();
checkArgument(
- getProjectControl().getProject().getNameKey().equals(change.getProject()),
- "mismatched project");
+ project.equals(change.getProject()),
+ "expected change in project %s, not %s",
+ project,
+ change.getProject());
return getProjectControl().controlFor(notes).asForChange(null, db);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 611a1a4..39e9241 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -118,7 +118,7 @@
private static final Pattern DEF_CHANGE =
Pattern.compile("^(?:[1-9][0-9]*|(?:[^~]+~[^~]+~)?[iI][0-9a-f]{4,}.*)$");
- private static final int MAX_ACCOUNTS_PER_DEFAULT_FIELD = 10;
+ static final int MAX_ACCOUNTS_PER_DEFAULT_FIELD = 10;
// NOTE: As new search operations are added, please keep the
// SearchSuggestOracle up to date.
@@ -993,12 +993,16 @@
private Predicate<ChangeData> reviewer(String who, boolean forDefaultField)
throws QueryParseException, OrmException {
+ Predicate<ChangeData> byState =
+ reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField);
+ if (byState == Predicate.<ChangeData>any()) {
+ return Predicate.any();
+ }
if (args.getSchema().hasField(ChangeField.WIP)) {
return Predicate.and(
- Predicate.not(new BooleanPredicate(ChangeField.WIP, args.fillArgs)),
- reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField));
+ Predicate.not(new BooleanPredicate(ChangeField.WIP, args.fillArgs)), byState);
}
- return reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField);
+ return byState;
}
@Operator
@@ -1162,12 +1166,18 @@
// Adapt the capacity of this list when adding more default predicates.
List<Predicate<ChangeData>> predicates = Lists.newArrayListWithCapacity(11);
try {
- predicates.add(ownerDefaultField(query));
+ Predicate<ChangeData> p = ownerDefaultField(query);
+ if (p != Predicate.<ChangeData>any()) {
+ predicates.add(p);
+ }
} catch (OrmException | QueryParseException e) {
// Skip.
}
try {
- predicates.add(reviewerDefaultField(query));
+ Predicate<ChangeData> p = reviewerDefaultField(query);
+ if (p != Predicate.<ChangeData>any()) {
+ predicates.add(p);
+ }
} catch (OrmException | QueryParseException e) {
// Skip.
}
@@ -1266,7 +1276,10 @@
return Predicate.or(reviewerPredicate, reviewerByEmailPredicate);
} else if (reviewerPredicate != null) {
return reviewerPredicate;
+ } else if (reviewerByEmailPredicate != null) {
+ return reviewerByEmailPredicate;
+ } else {
+ return Predicate.any();
}
- return reviewerByEmailPredicate;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_150.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_150.java
index 4830fe1..456a01a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_150.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_150.java
@@ -23,4 +23,4 @@
Schema_150(Provider<Schema_149> prior) {
super(prior);
}
-}
\ No newline at end of file
+}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 0b01362..ac421f0 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -67,6 +67,7 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountManager;
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.change.ChangeInserter;
@@ -88,6 +89,8 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.schema.SchemaCreator;
import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gerrit.server.util.RequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.gerrit.testutil.ConfigSuite;
@@ -138,6 +141,7 @@
return cfg;
}
+ @Inject protected AccountCache accountCache;
@Inject protected AccountManager accountManager;
@Inject protected AllUsersName allUsersName;
@Inject protected BatchUpdate.Factory updateFactory;
@@ -151,6 +155,7 @@
@Inject protected InMemoryRepositoryManager repoManager;
@Inject protected InternalChangeQuery internalChangeQuery;
@Inject protected ChangeNotes.Factory notesFactory;
+ @Inject protected OneOffRequestContext oneOffRequestContext;
@Inject protected PatchSetInserter.Factory patchSetFactory;
@Inject protected PatchSetUtil psUtil;
@Inject protected ChangeControl.GenericFactory changeControlFactory;
@@ -2001,6 +2006,14 @@
assertQuery("starredby:me", change2, change1);
}
+ @Test
+ public void defaultFieldWithManyUsers() throws Exception {
+ for (int i = 0; i < ChangeQueryBuilder.MAX_ACCOUNTS_PER_DEFAULT_FIELD * 2; i++) {
+ createAccount("user" + i, "User " + i, "user" + i + "@example.com", true);
+ }
+ assertQuery("us");
+ }
+
protected ChangeInserter newChange(TestRepository<Repo> repo) throws Exception {
return newChange(repo, null, null, null, null);
}
@@ -2204,4 +2217,21 @@
Patch.COMMIT_MSG, ImmutableList.<ReviewInput.CommentInput>of(comment));
gApi.changes().id(changeId).current().review(input);
}
+
+ private Account.Id createAccount(String username, String fullName, String email, boolean active)
+ throws Exception {
+ try (ManualRequestContext ctx = oneOffRequestContext.open()) {
+ Account.Id id = accountManager.authenticate(AuthRequest.forUser(username)).getAccountId();
+ if (email != null) {
+ accountManager.link(id, AuthRequest.forEmail(email));
+ }
+ Account a = db.accounts().get(id);
+ a.setFullName(fullName);
+ a.setPreferredEmail(email);
+ a.setActive(active);
+ db.accounts().update(ImmutableList.of(a));
+ accountCache.evict(id);
+ return id;
+ }
+ }
}
diff --git a/polygerrit-ui/README.md b/polygerrit-ui/README.md
index 3fd774d..ce444c5 100644
--- a/polygerrit-ui/README.md
+++ b/polygerrit-ui/README.md
@@ -137,3 +137,12 @@
simple style issues.
If you modify JS inside of `<script>` tags, like for test suites, you may have
to supply the `--ext .html` flag.
+
+Some useful commands:
+
+* To run ESLint on the whole app, less some dependency code:
+`eslint --ignore-pattern 'bower_components/' --ignore-pattern 'gr-linked-text' --ignore-pattern 'scripts/vendor' --ext .html,.js polygerrit-ui/app`
+* To run ESLint on just the subdirectory you modified:
+`eslint --ext .html,.js polygerrit-ui/app/$YOUR_DIR_HERE`
+* To run the linter on all of your local changes:
+`git diff --name-only master | xargs eslint --ext .html,.js`
diff --git a/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.html b/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.html
index 6b35328..3e9e19e 100644
--- a/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.html
+++ b/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.html
@@ -15,4 +15,6 @@
-->
<link rel="import" href="../../bower_components/polymer/polymer.html">
<link rel="import" href="../../elements/shared/gr-tooltip/gr-tooltip.html">
+<script src="../../scripts/rootElement.js"></script>
+
<script src="gr-tooltip-behavior.js"></script>
diff --git a/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.js b/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.js
index 8eb63a4..ad3f2c2 100644
--- a/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.js
+++ b/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.js
@@ -75,7 +75,7 @@
// Set visibility to hidden before appending to the DOM so that
// calculations can be made based on the element’s size.
tooltip.style.visibility = 'hidden';
- Polymer.dom(document.body).appendChild(tooltip);
+ Gerrit.getRootElement().appendChild(tooltip);
this._positionTooltip(tooltip);
tooltip.style.visibility = null;
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js
index 425ea76..a18cfaa 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js
@@ -99,6 +99,7 @@
'n ]': '_handleNKey',
'o enter': '_handleEnterKey',
'p [': '_handlePKey',
+ 'shift+r': '_handleRKey',
},
attached: function() {
@@ -241,6 +242,15 @@
this.fire('previous-page');
},
+ _handleRKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) {
+ return;
+ }
+
+ e.preventDefault();
+ window.location.reload();
+ },
+
_changeURLForIndex: function(index) {
var changeEls = this._getListItems();
if (index < changeEls.length && changeEls[index]) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
index 7408e04..4918b03 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
@@ -205,7 +205,7 @@
<template is="dom-if" if="[[change.topic]]">
<gr-linked-chip
text="[[change.topic]]"
- href="[[_computeTopicHref(change.topic)]]"
+ href="[[_computeTopicURL(change.topic)]]"
removable="[[!_topicReadOnly]]"
on-remove="_handleTopicRemoved"></gr-linked-chip>
</template>
@@ -255,7 +255,7 @@
<section class="labelStatus">
<span class="title">Label Status</span>
<span class="value">
- <div hidden$="[[!change.work_in_progress]]">
+ <div hidden$="[[!_isWip]]">
Work in progress
</div>
<div hidden$="[[!_showMissingLabels(change.labels)]]">
@@ -268,7 +268,7 @@
</template>
</ul>
</div>
- <div hidden$="[[_showMissingRequirements(change.labels, change.work_in_progress)]]">
+ <div hidden$="[[_showMissingRequirements(change.labels, _isWip)]]">
Ready to submit
</div>
</span>
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js
index 9046d1b..4527104 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js
@@ -44,6 +44,10 @@
},
_assignee: Array,
+ _isWip: {
+ type: Boolean,
+ computed: '_computeIsWip(change)',
+ },
},
behaviors: [
@@ -264,15 +268,19 @@
' status:' + this.encodeURL(status, false);
},
- _computeTopicHref: function(topic) {
- var encodedTopic = encodeURIComponent('\"' + topic + '\"');
+ _computeTopicURL: function(topic) {
return this.getBaseUrl() + '/q/topic:' +
- encodeURIComponent(encodedTopic) + '+(status:open OR status:merged)';
+ this.encodeURL('"' + topic + '"', false) +
+ '+(status:open OR status:merged)';
},
_handleTopicRemoved: function() {
this.set(['change', 'topic'], '');
this.$.restAPI.setChangeTopic(this.change._number, null);
},
+
+ _computeIsWip(change) {
+ return !!change.work_in_progress;
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html
index 0ac8531..2840c1f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html
@@ -153,6 +153,19 @@
assert.equal(element._computeWebLinks(element.commitInfo).length, 1);
});
+ test('determines whether to show "Ready to Submit" label', function() {
+ var showMissingSpy = sandbox.spy(element, '_showMissingRequirements');
+ element.change = {status: 'NEW', submit_type: 'CHERRY_PICK', labels: {
+ test: {
+ all: [{_account_id: 1, name: 'bojack', value: 1}],
+ default_value: 0,
+ values: [],
+ },
+ }};
+ flushAsynchronousOperations();
+ assert.isTrue(showMissingSpy.called);
+ });
+
suite('Topic removal', function() {
var change;
setup(function() {
@@ -282,7 +295,7 @@
});
test('topic href has quotes', function() {
- var hrefArr = element._computeTopicHref('test')
+ var hrefArr = element._computeTopicURL('test')
.split('%2522'); // Double-escaped quote.
assert.equal(hrefArr[1], 'test');
});
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 776ace2..2d57392 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -19,6 +19,7 @@
<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
<link rel="import" href="../../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../diff/gr-diff-preferences/gr-diff-preferences.html">
<link rel="import" href="../../shared/gr-account-link/gr-account-link.html">
<link rel="import" href="../../shared/gr-select/gr-select.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
@@ -78,6 +79,9 @@
font-size: 1.2em;
font-weight: bold;
}
+ .prefsButton {
+ float: right;
+ }
gr-change-star {
margin-right: .25em;
vertical-align: -.425em;
@@ -193,6 +197,9 @@
height: 0;
margin-bottom: 1em;
}
+ #diffPrefsContainer {
+ margin: auto 0 auto auto;
+ }
.patchInfo-header-wrapper {
width: 100%;
}
@@ -461,9 +468,17 @@
read-only="[[_descriptionReadOnly]]"
on-changed="_handleDescriptionChanged"></gr-editable-label>
</span>
+ <span id="diffPrefsContainer"
+ hidden$="[[_computePrefsButtonHidden(_diffPrefs, _loggedIn)]]"
+ hidden>
+ <gr-button link
+ class="prefsButton desktop"
+ on-tap="_handlePrefsTap">Diff Preferences</gr-button>
+ </span>
</div>
</div>
<gr-file-list id="fileList"
+ diff-prefs="{{_diffPrefs}}"
change="[[_change]]"
change-num="[[_changeNum]]"
patch-range="{{_patchRange}}"
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 f140296..ea835e7 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
@@ -73,6 +73,7 @@
type: Object,
value: function() { return document.body; },
},
+ _diffPrefs: Object,
_numFilesShown: {
type: Number,
observer: '_numFilesShownChanged',
@@ -190,6 +191,7 @@
'u': '_handleUKey',
'x': '_handleXKey',
'z': '_handleZKey',
+ ',': '_handleCommaKey',
},
attached: function() {
@@ -225,6 +227,10 @@
}
},
+ _computePrefsButtonHidden: function(prefs, loggedIn) {
+ return !loggedIn || !prefs;
+ },
+
_handleEditCommitMessage: function(e) {
this._editingCommitMessage = true;
this.$.commitMessageEditor.focusTextarea();
@@ -273,6 +279,11 @@
return false;
},
+ _handlePrefsTap: function(e) {
+ e.preventDefault();
+ this.$.fileList.openDiffPrefs();
+ },
+
_handleCommentSave: function(e) {
if (!e.target.comment.__draft) { return; }
@@ -800,6 +811,14 @@
this.$.messageList.handleExpandCollapse(false);
},
+ _handleCommaKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e) ||
+ this.modifierPressed(e)) { return; }
+
+ e.preventDefault();
+ this.$.fileList.openDiffPrefs();
+ },
+
_determinePageBack: function() {
// Default backPage to '/' if user came to change view page
// via an email link, etc.
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index 06fb387..9e42408 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -98,6 +98,8 @@
test('A toggles overlay when logged in', function(done) {
sandbox.stub(element, '_getLoggedIn').returns(Promise.resolve(true));
+ sandbox.stub(element.$.replyDialog, 'fetchIsLatestKnown')
+ .returns(Promise.resolve(true));
element._change = {labels: {}};
MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
flush(function() {
@@ -157,6 +159,45 @@
MockInteractions.pressAndReleaseKeyOn(element, 68, null, 'd');
assert.isTrue(stub.called);
});
+
+ test(', should open diff preferences', function() {
+ var stub = sandbox.stub(element.$.fileList.$.diffPreferences, 'open');
+ MockInteractions.pressAndReleaseKeyOn(element, 188, null, ',');
+ assert.isTrue(stub.called);
+ });
+ });
+
+ test('Diff preferences hidden when no prefs or logged out',
+ function() {
+ element._loggedIn = false;
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = true;
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = false;
+ element._diffPrefs = {'font_size': '12'};
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = true;
+ flushAsynchronousOperations();
+ assert.isFalse(element.$.diffPrefsContainer.hidden);
+ });
+
+ test('prefsButton opens gr-diff-preferences', function() {
+ var handlePrefsTapSpy = sandbox.spy(element, '_handlePrefsTap');
+ var overlayOpenStub = sandbox.stub(element.$.fileList,
+ 'openDiffPrefs');
+ var prefsButton = Polymer.dom(element.root).querySelectorAll(
+ '.prefsButton')[0];
+
+ MockInteractions.tap(prefsButton);
+
+ assert.isTrue(handlePrefsTapSpy.called);
+ assert.isTrue(overlayOpenStub.called);
});
test('_computeDescriptionReadOnly', function() {
@@ -987,6 +1028,8 @@
suite('reply dialog tests', function() {
setup(function() {
sandbox.stub(element.$.replyDialog, '_draftChanged');
+ sandbox.stub(element.$.replyDialog, 'fetchIsLatestKnown',
+ function() { return Promise.resolve(true); });
element._change = {labels: {}};
});
@@ -1035,6 +1078,11 @@
});
suite('commit message expand/collapse', function() {
+ setup(function() {
+ sandbox.stub(element, 'fetchIsLatestKnown',
+ function() { return Promise.resolve(false); });
+ });
+
test('commitCollapseToggle hidden for short commit message', function() {
element._latestCommitMessage = '';
assert.isTrue(element.$.commitCollapseToggle.hasAttribute('hidden'));
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index 6f8568c..af412db 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -323,14 +323,13 @@
no-auto-render
inline-index=[[index]]
hidden="[[!_isFileExpanded(file.__path, _expandedFilePaths.*)]]"
- project="[[change.project]]"
- commit="[[change.current_revision]]"
change-num="[[changeNum]]"
patch-range="[[patchRange]]"
path="[[file.__path]]"
- prefs="[[_diffPrefs]]"
+ prefs="[[diffPrefs]]"
project-config="[[projectConfig]]"
on-line-selected="_onLineSelected"
+ no-render-on-prefs-change
view-mode="[[_getDiffViewMode(diffViewMode, _userPrefs)]]"></gr-diff>
</template>
</template>
@@ -384,6 +383,10 @@
[[_computeShowAllText(_files)]]
</gr-button><!--
--></gr-tooltip-content>
+ <gr-diff-preferences
+ id="diffPreferences"
+ prefs="{{diffPrefs}}"
+ local-prefs="{{_localPrefs}}"></gr-diff-preferences>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="storage"></gr-storage>
<gr-diff-cursor id="diffCursor"></gr-diff-cursor>
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 705a8f1..4647d81 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
@@ -54,6 +54,7 @@
diffViewMode: {
type: String,
notify: true,
+ observer: '_updateDiffPreferences',
},
_files: {
type: Array,
@@ -69,7 +70,11 @@
value: function() { return []; },
},
_diffAgainst: String,
- _diffPrefs: Object,
+ diffPrefs: {
+ type: Object,
+ notify: true,
+ observer: '_updateDiffPreferences',
+ },
_userPrefs: Object,
_localPrefs: Object,
_showInlineDiffs: Boolean,
@@ -158,7 +163,7 @@
this._localPrefs = this.$.storage.getPreferences();
promises.push(this._getDiffPreferences().then(function(prefs) {
- this._diffPrefs = prefs;
+ this.diffPrefs = prefs;
}.bind(this)));
promises.push(this._getPreferences().then(function(prefs) {
@@ -173,6 +178,10 @@
return Polymer.dom(this.root).querySelectorAll('gr-diff');
},
+ openDiffPrefs: function() {
+ this.$.diffPreferences.open();
+ },
+
_calculatePatchChange: function(files) {
var filesNoCommitMsg = files.filter(function(files) {
return files.__path !== '/COMMIT_MSG';
@@ -232,6 +241,19 @@
this._patchRangeStr(patchRange), true));
},
+ _updateDiffPreferences: function() {
+ if (!this.diffs.length) { return; }
+ // Re-render all expanded diffs sequentially.
+ var timerName = 'Update ' + this._expandedFilePaths.length +
+ ' diffs with new prefs';
+ this._renderInOrder(this._expandedFilePaths, this.diffs,
+ this._expandedFilePaths.length)
+ .then(function() {
+ this.$.reporting.timeEnd(timerName);
+ this.$.diffCursor.handleDiffUpdate();
+ }.bind(this));
+ },
+
_forEachDiff: function(fn) {
var diffs = this.diffs;
for (var i = 0; i < diffs.length; i++) {
@@ -519,6 +541,9 @@
e.preventDefault();
if (this._showInlineDiffs) {
this._openCursorFile();
+ } else if (this._userPrefs && this._userPrefs.expand_inline_diffs) {
+ if (this.$.fileCursor.index === -1) { return; }
+ this._togglePathExpandedByIndex(this.$.fileCursor.index);
} else {
this._openSelectedFile();
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
index a34463b..0fbd367 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -381,42 +381,70 @@
}
});
- test('_handleEnterKey navigates', function() {
- sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false);
- sandbox.stub(element, 'modifierPressed').returns(false);
- var expandStub = sandbox.stub(element, '_openCursorFile');
- var navStub = sandbox.stub(element, '_openSelectedFile');
- var e = new CustomEvent('fake-keyboard-event');
- sinon.stub(e, 'preventDefault');
- element._showInlineDiffs = false;
- element._handleEnterKey(e);
- assert.isTrue(e.preventDefault.called);
- assert.isTrue(navStub.called);
- assert.isFalse(expandStub.called);
- });
+ suite('_handleEnterKey', function() {
+ var interact;
- test('_handleEnterKey expands', function() {
- sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false);
- sandbox.stub(element, 'modifierPressed').returns(false);
- var expandStub = sandbox.stub(element, '_openCursorFile');
- var navStub = sandbox.stub(element, '_openSelectedFile');
- var e = new CustomEvent('fake-keyboard-event');
- sinon.stub(e, 'preventDefault');
- element._showInlineDiffs = true;
- element._handleEnterKey(e);
- assert.isTrue(e.preventDefault.called);
- assert.isFalse(navStub.called);
- assert.isTrue(expandStub.called);
- });
+ setup(function() {
+ sandbox.stub(element, 'shouldSuppressKeyboardShortcut')
+ .returns(false);
+ sandbox.stub(element, 'modifierPressed').returns(false);
+ var openCursorStub = sandbox.stub(element, '_openCursorFile');
+ var openSelectedStub = sandbox.stub(element, '_openSelectedFile');
+ var expandStub = sandbox.stub(element, '_togglePathExpanded');
- test('_handleEnterKey noop when anchor focused', function() {
- sandbox.stub(element, 'shouldSuppressKeyboardShortcut').returns(false);
- sandbox.stub(element, 'modifierPressed').returns(false);
- var e = new CustomEvent('fake-keyboard-event',
- {detail: {keyboardEvent: {target: document.createElement('a')}}});
- sinon.stub(e, 'preventDefault');
- element._handleEnterKey(e);
- assert.isFalse(e.preventDefault.called);
+ interact = function(opt_payload) {
+ openCursorStub.reset();
+ openSelectedStub.reset();
+ expandStub.reset();
+
+ var e = new CustomEvent('fake-keyboard-event', opt_payload);
+ sinon.stub(e, 'preventDefault');
+ element._handleEnterKey(e);
+ assert.isTrue(e.preventDefault.called);
+ var result = {};
+ if (openCursorStub.called) {
+ result.opened_cursor = true;
+ }
+ if (openSelectedStub.called) {
+ result.opened_selected = true;
+ }
+ if (expandStub.called) {
+ result.expanded = true;
+ }
+ return result;
+ };
+ });
+
+ test('open from selected file', function() {
+ element._showInlineDiffs = false;
+ assert.deepEqual(interact(), {opened_selected: true});
+ });
+
+ test('open from diff cursor', function() {
+ element._showInlineDiffs = true;
+ assert.deepEqual(interact(), {opened_cursor: true});
+
+ // "Show diffs" mode overrides userPrefs.expand_inline_diffs
+ element._userPrefs = {expand_inline_diffs: true};
+ assert.deepEqual(interact(), {opened_cursor: true});
+ });
+
+ test('expand when user prefers', function() {
+ element._showInlineDiffs = false;
+ assert.deepEqual(interact(), {opened_selected: true});
+ element._userPrefs = {};
+ assert.deepEqual(interact(), {opened_selected: true});
+ element._userPrefs.expand_inline_diffs = true;
+ assert.deepEqual(interact(), {expanded: true});
+ });
+
+ test('noop when anchor focused', function() {
+ var e = new CustomEvent('fake-keyboard-event',
+ {detail: {keyboardEvent: {target: document.createElement('a')}}});
+ sinon.stub(e, 'preventDefault');
+ element._handleEnterKey(e);
+ assert.isFalse(e.preventDefault.called);
+ });
});
});
@@ -699,6 +727,7 @@
basePatchNum: 'PARENT',
patchNum: '2',
};
+ sandbox.spy(element, '_updateDiffPreferences');
element.$.fileCursor.setCursorAtIndex(0);
flushAsynchronousOperations();
@@ -714,6 +743,7 @@
assert.equal(diffDisplay.viewMode, 'SIDE_BY_SIDE');
element.set('diffViewMode', 'UNIFIED_DIFF');
assert.equal(diffDisplay.viewMode, 'UNIFIED_DIFF');
+ assert.isTrue(element._updateDiffPreferences.called);
});
test('diff mode selector initializes from preferences', function() {
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 79a1f34..dc40fdd 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
@@ -14,21 +14,21 @@
(function() {
'use strict';
- var STORAGE_DEBOUNCE_INTERVAL_MS = 400;
+ const STORAGE_DEBOUNCE_INTERVAL_MS = 400;
- var FocusTarget = {
+ const FocusTarget = {
ANY: 'any',
BODY: 'body',
CCS: 'cc',
REVIEWERS: 'reviewers',
};
- var ReviewerTypes = {
+ const ReviewerTypes = {
REVIEWER: 'REVIEWER',
CC: 'CC',
};
- var LatestPatchState = {
+ const LatestPatchState = {
LATEST: 'latest',
CHECKING: 'checking',
NOT_LATEST: 'not-latest',
@@ -86,7 +86,7 @@
diffDrafts: Object,
filterReviewerSuggestion: {
type: Function,
- value: function() {
+ value() {
return this._filterReviewerSuggestion.bind(this);
},
},
@@ -138,7 +138,7 @@
},
},
- FocusTarget: FocusTarget,
+ FocusTarget,
behaviors: [
Gerrit.BaseUrlBehavior,
@@ -148,7 +148,7 @@
],
keyBindings: {
- 'esc': '_handleEscKey',
+ esc: '_handleEscKey',
},
observers: [
@@ -157,23 +157,23 @@
'_reviewersChanged(_reviewers.splices)',
],
- attached: function() {
- this._getAccount().then(function(account) {
+ attached() {
+ this._getAccount().then(account => {
this._account = account || {};
- }.bind(this));
+ });
},
- ready: function() {
+ ready() {
this.$.jsAPI.addElement(this.$.jsAPI.Element.REPLY_DIALOG, this);
},
- open: function(opt_focusTarget) {
+ open(opt_focusTarget) {
this.knownLatestState = LatestPatchState.CHECKING;
this.fetchIsLatestKnown(this.change, this.$.restAPI)
- .then(function(isUpToDate) {
+ .then(isUpToDate => {
this.knownLatestState = isUpToDate ?
LatestPatchState.LATEST : LatestPatchState.NOT_LATEST;
- }.bind(this));
+ });
this._focusOn(opt_focusTarget);
if (!this.draft || !this.draft.length) {
@@ -181,54 +181,54 @@
}
},
- focus: function() {
+ focus() {
this._focusOn(FocusTarget.ANY);
},
- getFocusStops: function() {
+ getFocusStops() {
return {
start: this.$.reviewers.focusStart,
end: this.$.cancelButton,
};
},
- setLabelValue: function(label, value) {
- var selectorEl =
+ setLabelValue(label, value) {
+ const selectorEl =
this.$.labelScores.$$('iron-selector[data-label="' + label + '"]');
// The selector may not be present if it’s not at the latest patch set.
if (!selectorEl) { return; }
- var item = selectorEl.$$('gr-button[data-value="' + value + '"]');
+ const item = selectorEl.$$('gr-button[data-value="' + value + '"]');
if (!item) { return; }
selectorEl.selectIndex(selectorEl.indexOf(item));
},
- _handleEscKey: function(e) {
+ _handleEscKey(e) {
this.cancel();
},
- _ccsChanged: function(splices) {
+ _ccsChanged(splices) {
if (splices && splices.indexSplices) {
this._processReviewerChange(splices.indexSplices, ReviewerTypes.CC);
}
},
- _reviewersChanged: function(splices) {
+ _reviewersChanged(splices) {
if (splices && splices.indexSplices) {
this._processReviewerChange(splices.indexSplices,
ReviewerTypes.REVIEWER);
}
},
- _processReviewerChange: function(indexSplices, type) {
- indexSplices.forEach(function(splice) {
- splice.removed.forEach(function(account) {
+ _processReviewerChange(indexSplices, type) {
+ for (const splice of indexSplices) {
+ for (const account of splice.removed) {
if (!this._reviewersPendingRemove[type]) {
console.err('Invalid type ' + type + ' for reviewer.');
return;
}
this._reviewersPendingRemove[type].push(account);
- }.bind(this));
- }.bind(this));
+ }
+ }
},
/**
@@ -239,15 +239,14 @@
* @param {Object} opt_accountIdsTransferred map of account IDs that must
* not be removed, because they have been readded in another state.
*/
- _purgeReviewersPendingRemove: function(isCancel,
- opt_accountIdsTransferred) {
- var reviewerArr;
- var keep = opt_accountIdsTransferred || {};
- for (var type in this._reviewersPendingRemove) {
+ _purgeReviewersPendingRemove(isCancel, opt_accountIdsTransferred) {
+ let reviewerArr;
+ const keep = opt_accountIdsTransferred || {};
+ for (const type in this._reviewersPendingRemove) {
if (this._reviewersPendingRemove.hasOwnProperty(type)) {
if (!isCancel) {
reviewerArr = this._reviewersPendingRemove[type];
- for (var i = 0; i < reviewerArr.length; i++) {
+ for (let i = 0; i < reviewerArr.length; i++) {
if (!keep[reviewerArr[i]._account_id]) {
this._removeAccount(reviewerArr[i], type);
}
@@ -265,76 +264,76 @@
* @param {Object} account
* @param {ReviewerTypes} type
*/
- _removeAccount: function(account, type) {
+ _removeAccount(account, type) {
if (account._pendingAdd) { return; }
return this.$.restAPI.removeChangeReviewer(this.change._number,
- account._account_id).then(function(response) {
- if (!response.ok) { return response; }
+ account._account_id).then(response => {
+ if (!response.ok) { return response; }
- var reviewers = this.change.reviewers[type] || [];
- for (var i = 0; i < reviewers.length; i++) {
- if (reviewers[i]._account_id == account._account_id) {
- this.splice(['change', 'reviewers', type], i, 1);
- break;
- }
- }
- }.bind(this));
+ const reviewers = this.change.reviewers[type] || [];
+ for (let i = 0; i < reviewers.length; i++) {
+ if (reviewers[i]._account_id == account._account_id) {
+ this.splice(['change', 'reviewers', type], i, 1);
+ break;
+ }
+ }
+ });
},
- _mapReviewer: function(reviewer) {
- var reviewerId;
- var confirmed;
+ _mapReviewer(reviewer) {
+ let reviewerId;
+ let confirmed;
if (reviewer.account) {
reviewerId = reviewer.account._account_id || reviewer.account.email;
} else if (reviewer.group) {
reviewerId = reviewer.group.id;
confirmed = reviewer.group.confirmed;
}
- return {reviewer: reviewerId, confirmed: confirmed};
+ return {reviewer: reviewerId, confirmed};
},
- send: function(includeComments) {
+ send(includeComments) {
if (this.knownLatestState === 'not-latest') {
this.fire('show-alert',
{message: 'Cannot reply to non-latest patch.'});
return;
}
- var labels = this.$.labelScores.getLabelValues();
+ const labels = this.$.labelScores.getLabelValues();
- var obj = {
+ const obj = {
drafts: includeComments ? 'PUBLISH_ALL_REVISIONS' : 'KEEP',
- labels: labels,
+ labels,
};
if (this.draft != null) {
obj.message = this.draft;
}
- var accountAdditions = {};
- obj.reviewers = this.$.reviewers.additions().map(function(reviewer) {
+ const accountAdditions = {};
+ obj.reviewers = this.$.reviewers.additions().map(reviewer => {
if (reviewer.account) {
accountAdditions[reviewer.account._account_id] = true;
}
return this._mapReviewer(reviewer);
- }.bind(this));
- var ccsEl = this.$$('#ccs');
+ });
+ const ccsEl = this.$$('#ccs');
if (ccsEl) {
- ccsEl.additions().forEach(function(reviewer) {
+ for (let reviewer of ccsEl.additions()) {
if (reviewer.account) {
accountAdditions[reviewer.account._account_id] = true;
}
reviewer = this._mapReviewer(reviewer);
reviewer.state = 'CC';
obj.reviewers.push(reviewer);
- }.bind(this));
+ }
}
this.disabled = true;
- var errFn = this._handle400Error.bind(this);
- return this._saveReview(obj, errFn).then(function(response) {
+ const errFn = this._handle400Error.bind(this);
+ return this._saveReview(obj, errFn).then(response => {
if (!response || !response.ok) {
return response;
}
@@ -343,29 +342,29 @@
this._includeComments = true;
this.fire('send', null, {bubbles: false});
return accountAdditions;
- }.bind(this)).catch(function(err) {
+ }).catch(err => {
this.disabled = false;
throw err;
- }.bind(this));
+ });
},
- _focusOn: function(section) {
+ _focusOn(section) {
if (section === FocusTarget.ANY) {
section = this._chooseFocusTarget();
}
if (section === FocusTarget.BODY) {
- var textarea = this.$.textarea;
+ const textarea = this.$.textarea;
textarea.async(textarea.textarea.focus.bind(textarea.textarea));
} else if (section === FocusTarget.REVIEWERS) {
- var reviewerEntry = this.$.reviewers.focusStart;
+ const reviewerEntry = this.$.reviewers.focusStart;
reviewerEntry.async(reviewerEntry.focus);
} else if (section === FocusTarget.CCS) {
- var ccEntry = this.$$('#ccs').focusStart;
+ const ccEntry = this.$$('#ccs').focusStart;
ccEntry.async(ccEntry.focus);
}
},
- _chooseFocusTarget: function() {
+ _chooseFocusTarget() {
// If we are the owner and the reviewers field is empty, focus on that.
if (this._account && this.change && this.change.owner &&
this._account._account_id === this.change.owner._account_id &&
@@ -377,7 +376,7 @@
return FocusTarget.BODY;
},
- _handle400Error: function(response) {
+ _handle400Error(response) {
// A call to _saveReview could fail with a server error if erroneous
// reviewers were requested. This is signalled with a 400 Bad Request
// status. The default gr-rest-api-interface error handling would
@@ -393,80 +392,83 @@
if (response.status !== 400) {
// This is all restAPI does when there is no custom error handling.
- this.fire('server-error', {response: response});
+ this.fire('server-error', {response});
return response;
}
// Process the response body, format a better error message, and fire
// an event for gr-event-manager to display.
- var jsonPromise = this.$.restAPI.getResponseObject(response);
- return jsonPromise.then(function(result) {
- var errors = [];
- ['reviewers', 'ccs'].forEach(function(state) {
- for (var input in result[state]) {
- var reviewer = result[state][input];
- if (!!reviewer.error) {
+ const jsonPromise = this.$.restAPI.getResponseObject(response);
+ return jsonPromise.then(result => {
+ const errors = [];
+ for (const state of ['reviewers', 'ccs']) {
+ for (const reviewer of result[state]) {
+ if (reviewer.error) {
errors.push(reviewer.error);
}
}
- });
+ }
response = {
ok: false,
status: response.status,
- text: function() { return Promise.resolve(errors.join(', ')); },
+ text() { return Promise.resolve(errors.join(', ')); },
};
- this.fire('server-error', {response: response});
- }.bind(this));
+ this.fire('server-error', {response});
+ });
},
- _computeHideDraftList: function(drafts) {
+ _computeHideDraftList(drafts) {
return Object.keys(drafts || {}).length == 0;
},
- _computeDraftsTitle: function(drafts) {
- var total = 0;
- for (var file in drafts) {
- total += drafts[file].length;
+ _computeDraftsTitle(drafts) {
+ let total = 0;
+ for (const file in drafts) {
+ if (drafts.hasOwnProperty(file)) {
+ total += drafts[file].length;
+ }
}
if (total == 0) { return ''; }
if (total == 1) { return '1 Draft'; }
if (total > 1) { return total + ' Drafts'; }
},
- _computeMessagePlaceholder: function(canBeStarted) {
+ _computeMessagePlaceholder(canBeStarted) {
return canBeStarted ?
'Add a note for your reviewers...' :
'Say something nice...';
},
- _changeUpdated: function(changeRecord, owner, serverConfig) {
+ _changeUpdated(changeRecord, owner, serverConfig) {
this._rebuildReviewerArrays(changeRecord.base, owner, serverConfig);
},
- _rebuildReviewerArrays: function(change, owner, serverConfig) {
+ _rebuildReviewerArrays(change, owner, serverConfig) {
this._owner = owner;
- var reviewers = [];
- var ccs = [];
+ let reviewers = [];
+ const ccs = [];
- for (var key in change) {
- if (key !== 'REVIEWER' && key !== 'CC') {
- console.warn('unexpected reviewer state:', key);
- continue;
+ for (const key in change) {
+ if (change.hasOwnProperty(key)) {
+ if (key !== 'REVIEWER' && key !== 'CC') {
+ console.warn('unexpected reviewer state:', key);
+ continue;
+ }
+ for (const entry of change[key]) {
+ if (entry._account_id === owner._account_id) {
+ return;
+ }
+ switch (key) {
+ case 'REVIEWER':
+ reviewers.push(entry);
+ break;
+ case 'CC':
+ ccs.push(entry);
+ break;
+ }
+ }
}
- change[key].forEach(function(entry) {
- if (entry._account_id === owner._account_id) {
- return;
- }
- switch (key) {
- case 'REVIEWER':
- reviewers.push(entry);
- break;
- case 'CC':
- ccs.push(entry);
- break;
- }
- });
}
if (serverConfig.note_db_enabled) {
@@ -478,12 +480,12 @@
this._reviewers = reviewers;
},
- _accountOrGroupKey: function(entry) {
+ _accountOrGroupKey(entry) {
return entry.id || entry._account_id;
},
- _filterReviewerSuggestion: function(suggestion) {
- var entry;
+ _filterReviewerSuggestion(suggestion) {
+ let entry;
if (suggestion.account) {
entry = suggestion.account;
} else if (suggestion.group) {
@@ -496,8 +498,8 @@
return false;
}
- var key = this._accountOrGroupKey(entry);
- var finder = function(entry) {
+ const key = this._accountOrGroupKey(entry);
+ const finder = function(entry) {
return this._accountOrGroupKey(entry) === key;
}.bind(this);
@@ -505,56 +507,56 @@
this._ccs.find(finder) === undefined;
},
- _getAccount: function() {
+ _getAccount() {
return this.$.restAPI.getAccount();
},
- _cancelTapHandler: function(e) {
+ _cancelTapHandler(e) {
e.preventDefault();
this.cancel();
},
- cancel: function() {
+ cancel() {
this.fire('cancel', null, {bubbles: false});
this._purgeReviewersPendingRemove(true);
this._rebuildReviewerArrays(this.change.reviewers, this._owner,
this.serverConfig);
},
- _saveTapHandler: function(e) {
+ _saveTapHandler(e) {
e.preventDefault();
- this.send(this._includeComments).then(function(keepReviewers) {
+ this.send(this._includeComments).then(keepReviewers => {
this._purgeReviewersPendingRemove(false, keepReviewers);
- }.bind(this));
+ });
},
- _sendTapHandler: function(e) {
+ _sendTapHandler(e) {
e.preventDefault();
if (this.canBeStarted) {
this._startReview()
- .then(function() {
- return this.send(this._includeComments);
- }.bind(this))
- .then(this._purgeReviewersPendingRemove.bind(this));
+ .then(() => {
+ return this.send(this._includeComments);
+ })
+ .then(this._purgeReviewersPendingRemove.bind(this));
return;
}
this.send(this._includeComments)
- .then(this._purgeReviewersPendingRemove.bind(this));
+ .then(this._purgeReviewersPendingRemove.bind(this));
},
- _saveReview: function(review, opt_errFn) {
+ _saveReview(review, opt_errFn) {
return this.$.restAPI.saveChangeReview(this.change._number, this.patchNum,
review, opt_errFn);
},
- _startReview: function() {
+ _startReview() {
if (!this.canBeStarted) {
return Promise.resolve();
}
return this.$.restAPI.startReview(this.change._number);
},
- _reviewerPendingConfirmationUpdated: function(reviewer) {
+ _reviewerPendingConfirmationUpdated(reviewer) {
if (reviewer === null) {
this.$.reviewerConfirmationOverlay.close();
} else {
@@ -564,7 +566,7 @@
}
},
- _confirmPendingReviewer: function() {
+ _confirmPendingReviewer() {
if (this._ccPendingConfirmation) {
this.$$('#ccs').confirmGroup(this._ccPendingConfirmation.group);
this._focusOn(FocusTarget.CCS);
@@ -574,16 +576,16 @@
}
},
- _cancelPendingReviewer: function() {
+ _cancelPendingReviewer() {
this._ccPendingConfirmation = null;
this._reviewerPendingConfirmation = null;
- var target =
+ const target =
this._ccPendingConfirmation ? FocusTarget.CCS : FocusTarget.REVIEWERS;
this._focusOn(target);
},
- _getStorageLocation: function() {
+ _getStorageLocation() {
// Tests trigger this method without setting change.
if (!this.change) { return {}; }
return {
@@ -593,13 +595,13 @@
};
},
- _loadStoredDraft: function() {
- var draft = this.$.storage.getDraftComment(this._getStorageLocation());
+ _loadStoredDraft() {
+ const draft = this.$.storage.getDraftComment(this._getStorageLocation());
return draft ? draft.message : '';
},
- _draftChanged: function(newDraft, oldDraft) {
- this.debounce('store', function() {
+ _draftChanged(newDraft, oldDraft) {
+ this.debounce('store', () => {
if (!newDraft.length && oldDraft) {
// If the draft has been modified to be empty, then erase the storage
// entry.
@@ -611,24 +613,24 @@
}, STORAGE_DEBOUNCE_INTERVAL_MS);
},
- _handleHeightChanged: function(e) {
+ _handleHeightChanged(e) {
// If the textarea resizes, we need to re-fit the overlay.
- this.debounce('autogrow', function() {
+ this.debounce('autogrow', () => {
this.fire('autogrow');
});
},
- _isState: function(knownLatestState, value) {
+ _isState(knownLatestState, value) {
return knownLatestState === value;
},
- _reload: function() {
+ _reload() {
// Load the current change without any patch range.
location.href = this.getBaseUrl() + '/c/' + this.change._number;
},
- _computeSendButtonLabel: function(canBeStarted) {
- return canBeStarted ? "Start review" : "Send";
+ _computeSendButtonLabel(canBeStarted) {
+ return canBeStarted ? 'Start review' : 'Send';
},
});
})();
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
index 55a2c9c..ef0d9dc 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
@@ -33,36 +33,36 @@
</test-fixture>
<script>
- suite('gr-reply-dialog tests', function() {
- var element;
- var changeNum;
- var patchNum;
+ suite('gr-reply-dialog tests', () => {
+ let element;
+ let changeNum;
+ let patchNum;
- var sandbox;
- var getDraftCommentStub;
- var setDraftCommentStub;
- var eraseDraftCommentStub;
+ let sandbox;
+ let getDraftCommentStub;
+ let setDraftCommentStub;
+ let eraseDraftCommentStub;
- var lastId = 0;
- var makeAccount = function() { return {_account_id: lastId++}; };
- var makeGroup = function() { return {id: lastId++}; };
+ let lastId = 0;
+ const makeAccount = function() { return {_account_id: lastId++}; };
+ const makeGroup = function() { return {id: lastId++}; };
- setup(function() {
+ setup(() => {
sandbox = sinon.sandbox.create();
changeNum = 42;
patchNum = 1;
stub('gr-rest-api-interface', {
- getConfig: function() { return Promise.resolve({}); },
- getAccount: function() { return Promise.resolve({}); },
+ getConfig() { return Promise.resolve({}); },
+ getAccount() { return Promise.resolve({}); },
});
element = fixture('basic');
element.change = {
_number: changeNum,
labels: {
- Verified: {
+ 'Verified': {
values: {
'-1': 'Fails',
' 0': 'No score',
@@ -89,7 +89,7 @@
' 0',
'+1',
],
- Verified: [
+ 'Verified': [
'-1',
' 0',
'+1',
@@ -103,26 +103,25 @@
'eraseDraftComment');
sandbox.stub(element, 'fetchIsLatestKnown',
- function() { return Promise.resolve(true); });
+ () => { return Promise.resolve(true); });
// Allow the elements created by dom-repeat to be stamped.
flushAsynchronousOperations();
});
- teardown(function() {
+ teardown(() => {
sandbox.restore();
});
- test('default to publishing drafts with reply', function(done) {
+ test('default to publishing drafts with reply', done => {
// Async tick is needed because iron-selector content is distributed and
// distributed content requires an observer to be set up.
// Note: Double flush seems to be needed in Safari. {@see Issue 4963}.
- flush(function() {
- flush(function() {
+ flush(() => {
+ flush(() => {
element.draft = 'I wholeheartedly disapprove';
- var saveReviewStub = sandbox.stub(element, '_saveReview',
- function(review) {
+ sandbox.stub(element, '_saveReview', review => {
assert.deepEqual(review, {
drafts: 'PUBLISH_ALL_REVISIONS',
labels: {},
@@ -136,26 +135,25 @@
// This is needed on non-Blink engines most likely due to the ways in
// which the dom-repeat elements are stamped.
- flush(function() {
+ flush(() => {
MockInteractions.tap(element.$$('.send'));
});
});
});
});
- test('keep drafts with reply', function(done) {
+ test('keep drafts with reply', done => {
MockInteractions.tap(element.$$('#includeComments'));
assert.equal(element._includeComments, false);
// Async tick is needed because iron-selector content is distributed and
// distributed content requires an observer to be set up.
// Note: Double flush seems to be needed in Safari. {@see Issue 4963}.
- flush(function() {
- flush(function() {
+ flush(() => {
+ flush(() => {
element.draft = 'I wholeheartedly disapprove';
- var saveReviewStub = sandbox.stub(element, '_saveReview',
- function(review) {
+ sandbox.stub(element, '_saveReview', review => {
assert.deepEqual(review, {
drafts: 'KEEP',
labels: {},
@@ -169,17 +167,16 @@
// This is needed on non-Blink engines most likely due to the ways in
// which the dom-repeat elements are stamped.
- flush(function() {
+ flush(() => {
MockInteractions.tap(element.$$('.send'));
});
});
});
});
- test('label picker', function(done) {
+ test('label picker', done => {
element.draft = 'I wholeheartedly disapprove';
- var saveReviewStub = sandbox.stub(element, '_saveReview',
- function(review) {
+ sandbox.stub(element, '_saveReview', review => {
assert.deepEqual(review, {
drafts: 'PUBLISH_ALL_REVISIONS',
labels: {
@@ -192,14 +189,14 @@
return Promise.resolve({ok: true});
});
- sandbox.stub(element.$.labelScores, 'getLabelValues', function() {
+ sandbox.stub(element.$.labelScores, 'getLabelValues', () => {
return {
'Code-Review': -1,
'Verified': -1,
};
});
- element.addEventListener('send', function() {
+ element.addEventListener('send', () => {
assert.isFalse(element.disabled,
'Element should be enabled when done sending reply.');
assert.equal(element.draft.length, 0);
@@ -208,21 +205,21 @@
// This is needed on non-Blink engines most likely due to the ways in
// which the dom-repeat elements are stamped.
- flush(function() {
+ flush(() => {
MockInteractions.tap(element.$$('.send'));
assert.isTrue(element.disabled);
});
});
- test('setlabelValue', function() {
+ test('setlabelValue', () => {
element._account = {_account_id: 1};
flushAsynchronousOperations();
- var label = 'Verified';
- var value = '+1';
+ const label = 'Verified';
+ const value = '+1';
element.setLabelValue(label, value);
flushAsynchronousOperations();
- var labels = element.$.labelScores.getLabelValues();
- assert.deepEqual(labels, {'Verified': 1});
+ const labels = element.$.labelScores.getLabelValues();
+ assert.deepEqual(labels, {Verified: 1});
});
function getActiveElement() {
@@ -235,7 +232,7 @@
}
function overlayObserver(mode) {
- return new Promise(function(resolve) {
+ return new Promise(resolve => {
function listener() {
element.removeEventListener('iron-overlay-' + mode, listener);
resolve();
@@ -245,9 +242,9 @@
}
function testConfirmationDialog(done, cc) {
- var yesButton =
+ const yesButton =
element.$$('.reviewerConfirmationButtons gr-button:first-child');
- var noButton =
+ const noButton =
element.$$('.reviewerConfirmationButtons gr-button:last-child');
element.serverConfig = {note_db_enabled: true};
@@ -257,19 +254,19 @@
assert.isFalse(isVisible(element.$.reviewerConfirmationOverlay));
// Cause the confirmation dialog to display.
- var observer = overlayObserver('opened');
- var group = {
+ let observer = overlayObserver('opened');
+ const group = {
id: 'id',
name: 'name',
};
if (cc) {
element._ccPendingConfirmation = {
- group: group,
+ group,
count: 10,
};
} else {
element._reviewerPendingConfirmation = {
- group: group,
+ group,
count: 10,
};
}
@@ -285,16 +282,16 @@
element._pendingConfirmationDetails);
}
- observer.then(function() {
+ observer.then(() => {
assert.isTrue(isVisible(element.$.reviewerConfirmationOverlay));
observer = overlayObserver('closed');
- var expected = 'Group name has 10 members';
+ const expected = 'Group name has 10 members';
assert.notEqual(
element.$.reviewerConfirmationOverlay.innerText.indexOf(expected),
-1);
MockInteractions.tap(noButton); // close the overlay
return observer;
- }).then(function() {
+ }).then(() => {
assert.isFalse(isVisible(element.$.reviewerConfirmationOverlay));
// We should be focused on account entry input.
@@ -308,24 +305,24 @@
observer = overlayObserver('opened');
if (cc) {
element._ccPendingConfirmation = {
- group: group,
+ group,
count: 10,
};
} else {
element._reviewerPendingConfirmation = {
- group: group,
+ group,
count: 10,
};
}
return observer;
- }).then(function() {
+ }).then(() => {
assert.isTrue(isVisible(element.$.reviewerConfirmationOverlay));
observer = overlayObserver('closed');
MockInteractions.tap(yesButton); // Confirm the group.
return observer;
- }).then(function() {
+ }).then(() => {
assert.isFalse(isVisible(element.$.reviewerConfirmationOverlay));
- var additions = cc ?
+ const additions = cc ?
element.$$('#ccs').additions() :
element.$.reviewers.additions();
assert.deepEqual(
@@ -345,41 +342,41 @@
// We should be focused on account entry input.
assert.equal(getActiveElement().id, 'input');
}).then(done);
- };
+ }
- test('cc confirmation', function(done) {
+ test('cc confirmation', done => {
testConfirmationDialog(done, true);
});
- test('reviewer confirmation', function(done) {
+ test('reviewer confirmation', done => {
testConfirmationDialog(done, false);
});
- test('_getStorageLocation', function() {
- var actual = element._getStorageLocation();
+ test('_getStorageLocation', () => {
+ const actual = element._getStorageLocation();
assert.equal(actual.changeNum, changeNum);
assert.equal(actual.patchNum, patchNum);
assert.equal(actual.path, '@change');
});
- test('gets draft from storage on open', function() {
- var storedDraft = 'hello world';
+ test('gets draft from storage on open', () => {
+ const storedDraft = 'hello world';
getDraftCommentStub.returns({message: storedDraft});
element.open();
assert.isTrue(getDraftCommentStub.called);
assert.equal(element.draft, storedDraft);
});
- test('blank if no stored draft', function() {
+ test('blank if no stored draft', () => {
getDraftCommentStub.returns(null);
element.open();
assert.isTrue(getDraftCommentStub.called);
assert.equal(element.draft, '');
});
- test('updates stored draft on edits', function() {
- var firstEdit = 'hello';
- var location = element._getStorageLocation();
+ test('updates stored draft on edits', () => {
+ const firstEdit = 'hello';
+ const location = element._getStorageLocation();
element.draft = firstEdit;
element.flushDebouncer('store');
@@ -392,32 +389,32 @@
assert.isTrue(eraseDraftCommentStub.calledWith(location));
});
- test('400 converts to human-readable server-error', function(done) {
- sandbox.stub(window, 'fetch', function() {
- var text = '....{"reviewers":{"id1":{"error":"first error"}},' +
+ test('400 converts to human-readable server-error', done => {
+ sandbox.stub(window, 'fetch', () => {
+ const text = '....{"reviewers":{"id1":{"error":"first error"}},' +
'"ccs":{"id2":{"error":"second error"}}}';
return Promise.resolve({
ok: false,
status: 400,
- text: function() { return Promise.resolve(text); },
+ text() { return Promise.resolve(text); },
});
});
- element.addEventListener('server-error', function(event) {
+ element.addEventListener('server-error', event => {
if (event.target !== element) {
return;
}
- event.detail.response.text().then(function(body) {
+ event.detail.response.text().then(body => {
assert.equal(body, 'first error, second error');
});
});
// Async tick is needed because iron-selector content is distributed and
// distributed content requires an observer to be set up.
- flush(function() { element.send().then(done); });
+ flush(() => { element.send().then(done); });
});
- test('ccs are displayed if NoteDb is enabled', function() {
+ test('ccs are displayed if NoteDb is enabled', () => {
function hasCc() {
flushAsynchronousOperations();
return !!element.$$('#ccs');
@@ -430,12 +427,12 @@
assert.isTrue(hasCc());
});
- test('filterReviewerSuggestion', function() {
- var owner = makeAccount();
- var reviewer1 = makeAccount();
- var reviewer2 = makeGroup();
- var cc1 = makeAccount();
- var cc2 = makeGroup();
+ test('filterReviewerSuggestion', () => {
+ const owner = makeAccount();
+ const reviewer1 = makeAccount();
+ const reviewer2 = makeGroup();
+ const cc1 = makeAccount();
+ const cc2 = makeGroup();
element._owner = owner;
element._reviewers = [reviewer1, reviewer2];
@@ -457,7 +454,7 @@
assert.isFalse(element._filterReviewerSuggestion({group: cc2}));
});
- test('_chooseFocusTarget', function() {
+ test('_chooseFocusTarget', () => {
element._account = null;
assert.strictEqual(
element._chooseFocusTarget(), element.FocusTarget.BODY);
@@ -484,15 +481,14 @@
element._chooseFocusTarget(), element.FocusTarget.BODY);
});
- test('only send labels that have changed', function(done) {
- flush(function() {
- var saveReviewStub = sandbox.stub(element, '_saveReview',
- function(review) {
+ test('only send labels that have changed', done => {
+ flush(() => {
+ sandbox.stub(element, '_saveReview', review => {
assert.deepEqual(review.labels, {Verified: -1});
return Promise.resolve({ok: true});
});
- element.addEventListener('send', function() {
+ element.addEventListener('send', () => {
done();
});
// Without wrapping this test in flush(), the below two calls to
@@ -507,16 +503,16 @@
});
});
- test('do not display tooltips on touch devices', function() {
+ test('do not display tooltips on touch devices', () => {
element._account = {_account_id: 1};
element.set(['change', 'labels', 'Verified', 'all'],
[{_account_id: 1, value: -1}]);
element.labels = {
- Verified: {
+ 'Verified': {
values: {
'-1': 'Fails',
' 0': 'No score',
- '+1': 'Verified'
+ '+1': 'Verified',
},
default_value: 0,
},
@@ -526,7 +522,7 @@
'-1': 'I would prefer that you didn\'t submit this',
' 0': 'No score',
'+1': 'Looks good to me, but someone else must approve',
- '+2': 'Looks good to me, approved'
+ '+2': 'Looks good to me, approved',
},
default_value: 0,
},
@@ -534,7 +530,7 @@
flushAsynchronousOperations();
- var verifiedBtn = element.$$('gr-label-scores').$$(
+ const verifiedBtn = element.$$('gr-label-scores').$$(
'iron-selector[data-label="Verified"] > ' +
'gr-button[data-value="-1"]');
@@ -553,8 +549,8 @@
assert.isNotOk(verifiedBtn._tooltip);
});
- test('_processReviewerChange', function() {
- var mockIndexSplices = function(toRemove) {
+ test('_processReviewerChange', () => {
+ const mockIndexSplices = function(toRemove) {
return [{
removed: [toRemove],
}];
@@ -565,16 +561,16 @@
assert.equal(element._reviewersPendingRemove.REVIEWER.length, 1);
});
- test('_purgeReviewersPendingRemove', function() {
- var removeStub = sandbox.stub(element, '_removeAccount');
- var mock = function() {
+ test('_purgeReviewersPendingRemove', () => {
+ const removeStub = sandbox.stub(element, '_removeAccount');
+ const mock = function() {
element._reviewersPendingRemove = {
test: [makeAccount()],
test2: [makeAccount(), makeAccount()],
};
};
- var checkObjEmpty = function(obj) {
- for (var prop in obj) {
+ const checkObjEmpty = function(obj) {
+ for (const prop in obj) {
if (obj.hasOwnProperty(prop) && obj[prop].length) { return false; }
}
return true;
@@ -590,48 +586,46 @@
assert.isTrue(checkObjEmpty(element._reviewersPendingRemove));
});
- test('_removeAccount', function(done) {
+ test('_removeAccount', done => {
sandbox.stub(element.$.restAPI, 'removeChangeReviewer')
.returns(Promise.resolve({ok: true}));
- var arr = [makeAccount(), makeAccount()];
+ const arr = [makeAccount(), makeAccount()];
element.change.reviewers = {
REVIEWER: arr.slice(),
};
- element._removeAccount(arr[1], 'REVIEWER').then(function() {
+ element._removeAccount(arr[1], 'REVIEWER').then(() => {
assert.equal(element.change.reviewers.REVIEWER.length, 1);
assert.deepEqual(element.change.reviewers.REVIEWER, arr.slice(0, 1));
done();
});
});
- test('migrate reviewers between states', function(done) {
+ test('migrate reviewers between states', done => {
element.serverConfig = {note_db_enabled: true};
element._reviewersPendingRemove = {
CC: [],
REVIEWER: [],
};
flushAsynchronousOperations();
- var reviewers = element.$.reviewers;
- var ccs = element.$$('#ccs');
- var reviewer1 = makeAccount();
- var reviewer2 = makeAccount();
- var cc1 = makeAccount();
- var cc2 = makeAccount();
+ const reviewers = element.$.reviewers;
+ const ccs = element.$$('#ccs');
+ const reviewer1 = makeAccount();
+ const reviewer2 = makeAccount();
+ const cc1 = makeAccount();
+ const cc2 = makeAccount();
element._reviewers = [reviewer1, reviewer2];
element._ccs = [cc1, cc2];
- var mutations = [];
+ const mutations = [];
- var saveReviewStub = sandbox.stub(element, '_saveReview',
- function(review) {
- mutations.push.apply(mutations, review.reviewers);
+ sandbox.stub(element, '_saveReview', review => {
+ mutations.push(...review.reviewers);
return Promise.resolve({ok: true});
});
- var removeAccountStub = sandbox.stub(element, '_removeAccount',
- function(account, type) {
- mutations.push({state: 'REMOVED', account: account});
+ sandbox.stub(element, '_removeAccount', (account, type) => {
+ mutations.push({state: 'REMOVED', account});
return Promise.resolve();
});
@@ -645,8 +639,8 @@
// (Currently not possible in UI, but this is a good consistency check).
reviewers.$.entry.fire('add', {value: {account: cc2}});
ccs.$.entry.fire('add', {value: {account: reviewer2}});
- var mapReviewer = function(reviewer, opt_state) {
- var result = {reviewer: reviewer._account_id, confirmed: undefined};
+ const mapReviewer = function(reviewer, opt_state) {
+ const result = {reviewer: reviewer._account_id, confirmed: undefined};
if (opt_state) {
result.state = opt_state;
}
@@ -656,20 +650,20 @@
// Send and purge and verify moves without deletions.
element.send()
.then(element._purgeReviewersPendingRemove.bind(element))
- .then(function() {
- assert.deepEqual(
- mutations, [
- mapReviewer(cc1),
- mapReviewer(cc2),
- mapReviewer(reviewer1, 'CC'),
- mapReviewer(reviewer2, 'CC'),
- ]);
- done();
- });
+ .then(() => {
+ assert.deepEqual(
+ mutations, [
+ mapReviewer(cc1),
+ mapReviewer(cc2),
+ mapReviewer(reviewer1, 'CC'),
+ mapReviewer(reviewer2, 'CC'),
+ ]);
+ done();
+ });
});
- test('emits cancel on esc key', function() {
- var cancelHandler = sandbox.spy();
+ test('emits cancel on esc key', () => {
+ const cancelHandler = sandbox.spy();
element.addEventListener('cancel', cancelHandler);
MockInteractions.pressAndReleaseKeyOn(element, 27, null, 'esc');
flushAsynchronousOperations();
@@ -677,7 +671,7 @@
assert.isTrue(cancelHandler.called);
});
- test('_computeMessagePlaceholder', function() {
+ test('_computeMessagePlaceholder', () => {
assert.equal(
element._computeMessagePlaceholder(false),
'Say something nice...');
@@ -686,7 +680,7 @@
'Add a note for your reviewers...');
});
- test('_computeSendButtonLabel', function() {
+ test('_computeSendButtonLabel', () => {
assert.equal(
element._computeSendButtonLabel(false),
'Send');
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js
index ab5ecc2..1ccb30b 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js
@@ -14,13 +14,13 @@
(function() {
'use strict';
- var HIDE_ALERT_TIMEOUT_MS = 5000;
- var CHECK_SIGN_IN_INTERVAL_MS = 60 * 1000;
- var STALE_CREDENTIAL_THRESHOLD_MS = 10 * 60 * 1000;
- var SIGN_IN_WIDTH_PX = 690;
- var SIGN_IN_HEIGHT_PX = 500;
- var TOO_MANY_FILES = 'too many files to find conflicts';
- var AUTHENTICATION_REQUIRED = 'Authentication required\n';
+ const HIDE_ALERT_TIMEOUT_MS = 5000;
+ const CHECK_SIGN_IN_INTERVAL_MS = 60 * 1000;
+ const STALE_CREDENTIAL_THRESHOLD_MS = 10 * 60 * 1000;
+ const SIGN_IN_WIDTH_PX = 690;
+ const SIGN_IN_HEIGHT_PX = 500;
+ const TOO_MANY_FILES = 'too many files to find conflicts';
+ const AUTHENTICATION_REQUIRED = 'Authentication required\n';
Polymer({
is: 'gr-error-manager',
@@ -48,11 +48,11 @@
*/
_lastCredentialCheck: {
type: Number,
- value: function() { return Date.now(); },
- }
+ value() { return Date.now(); },
+ },
},
- attached: function() {
+ attached() {
this.listen(document, 'server-error', '_handleServerError');
this.listen(document, 'network-error', '_handleNetworkError');
this.listen(document, 'show-alert', '_handleShowAlert');
@@ -60,7 +60,7 @@
this.listen(document, 'show-auth-required', '_handleAuthRequired');
},
- detached: function() {
+ detached() {
this._clearHideAlertHandle();
this.unlisten(document, 'server-error', '_handleServerError');
this.unlisten(document, 'network-error', '_handleNetworkError');
@@ -68,21 +68,21 @@
this.unlisten(document, 'visibilitychange', '_handleVisibilityChange');
},
- _shouldSuppressError: function(msg) {
- return msg.indexOf(TOO_MANY_FILES) > -1;
+ _shouldSuppressError(msg) {
+ return msg.includes(TOO_MANY_FILES);
},
- _handleAuthRequired: function() {
+ _handleAuthRequired() {
this._showAuthErrorAlert(
'Log in is required to perform that action.', 'Log in.');
},
- _handleServerError: function(e) {
+ _handleServerError(e) {
Promise.all([
- e.detail.response.text(), this._getLoggedIn()
- ]).then(function(values) {
- var text = values[0];
- var loggedIn = values[1];
+ e.detail.response.text(), this._getLoggedIn(),
+ ]).then(values => {
+ const text = values[0];
+ const loggedIn = values[1];
if (e.detail.response.status === 403 &&
loggedIn &&
text === AUTHENTICATION_REQUIRED) {
@@ -92,24 +92,24 @@
} else if (!this._shouldSuppressError(text)) {
this._showAlert('Server error: ' + text);
}
- }.bind(this));
+ });
},
- _handleShowAlert: function(e) {
+ _handleShowAlert(e) {
this._showAlert(e.detail.message, e.detail.action, e.detail.callback,
e.detail.dismissOnNavigation);
},
- _handleNetworkError: function(e) {
+ _handleNetworkError(e) {
this._showAlert('Server unavailable');
console.error(e.detail.error.message);
},
- _getLoggedIn: function() {
+ _getLoggedIn() {
return this.$.restAPI.getLoggedIn();
},
- _showAlert: function(text, opt_actionText, opt_actionCallback,
+ _showAlert(text, opt_actionText, opt_actionCallback,
dismissOnNavigation) {
if (this._alertElement) { return; }
@@ -121,12 +121,12 @@
this._hideAlertHandle =
this.async(this._hideAlert, HIDE_ALERT_TIMEOUT_MS);
}
- var el = this._createToastAlert();
+ const el = this._createToastAlert();
el.show(text, opt_actionText, opt_actionCallback);
this._alertElement = el;
},
- _hideAlert: function() {
+ _hideAlert() {
if (!this._alertElement) { return; }
this._alertElement.hide();
@@ -136,14 +136,14 @@
this.unlisten(document, 'location-change', '_hideAlert');
},
- _clearHideAlertHandle: function() {
+ _clearHideAlertHandle() {
if (this._hideAlertHandle != null) {
this.cancelAsync(this._hideAlertHandle);
this._hideAlertHandle = null;
}
},
- _showAuthErrorAlert: function(errorText, actionText) {
+ _showAuthErrorAlert(errorText, actionText) {
// TODO(viktard): close alert if it's not for auth error.
if (this._alertElement) { return; }
@@ -158,13 +158,13 @@
}
},
- _createToastAlert: function() {
- var el = document.createElement('gr-alert');
+ _createToastAlert() {
+ const el = document.createElement('gr-alert');
el.toast = true;
return el;
},
- _handleVisibilityChange: function() {
+ _handleVisibilityChange() {
// Ignore when the page is transitioning to hidden (or hidden is
// undefined).
if (document.hidden !== false) { return; }
@@ -172,7 +172,7 @@
// If not currently refreshing credentials and the credentials are old,
// request them to confirm their validity or (display an auth toast if it
// fails).
- var timeSinceLastCheck = Date.now() - this._lastCredentialCheck;
+ const timeSinceLastCheck = Date.now() - this._lastCredentialCheck;
if (!this._refreshingCredentials &&
this.knownAccountId !== undefined &&
timeSinceLastCheck > STALE_CREDENTIAL_THRESHOLD_MS) {
@@ -181,18 +181,17 @@
}
},
- _requestCheckLoggedIn: function() {
+ _requestCheckLoggedIn() {
this.debounce(
- 'checkLoggedIn', this._checkSignedIn, CHECK_SIGN_IN_INTERVAL_MS);
+ 'checkLoggedIn', this._checkSignedIn, CHECK_SIGN_IN_INTERVAL_MS);
},
- _checkSignedIn: function() {
- this.$.restAPI.checkCredentials().then(function(account) {
- var isLoggedIn = !!account;
+ _checkSignedIn() {
+ this.$.restAPI.checkCredentials().then(account => {
+ const isLoggedIn = !!account;
this._lastCredentialCheck = Date.now();
if (this._refreshingCredentials) {
if (isLoggedIn) {
-
// If the credentials were refreshed but the account is different
// then reload the page completely.
if (account._account_id !== this.knownAccountId) {
@@ -205,17 +204,19 @@
this._requestCheckLoggedIn();
}
}
- }.bind(this));
+ });
},
- _reloadPage: function() {
+ _reloadPage() {
window.location.reload();
},
- _createLoginPopup: function() {
- var left = window.screenLeft + (window.outerWidth - SIGN_IN_WIDTH_PX) / 2;
- var top = window.screenTop + (window.outerHeight - SIGN_IN_HEIGHT_PX) / 2;
- var options = [
+ _createLoginPopup() {
+ const left = window.screenLeft +
+ (window.outerWidth - SIGN_IN_WIDTH_PX) / 2;
+ const top = window.screenTop +
+ (window.outerHeight - SIGN_IN_HEIGHT_PX) / 2;
+ const options = [
'width=' + SIGN_IN_WIDTH_PX,
'height=' + SIGN_IN_HEIGHT_PX,
'left=' + left,
@@ -226,14 +227,14 @@
this.listen(window, 'focus', '_handleWindowFocus');
},
- _handleCredentialRefreshed: function() {
+ _handleCredentialRefreshed() {
this.unlisten(window, 'focus', '_handleWindowFocus');
this._refreshingCredentials = false;
this._hideAlert();
this._showAlert('Credentials refreshed.');
},
- _handleWindowFocus: function() {
+ _handleWindowFocus() {
this.flushDebouncer('checkLoggedIn');
},
});
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
index 83d4a7b..ba10f09 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
@@ -33,69 +33,69 @@
</test-fixture>
<script>
- suite('gr-error-manager tests', function() {
- var element;
- var sandbox;
+ suite('gr-error-manager tests', () => {
+ let element;
+ let sandbox;
- setup(function() {
+ setup(() => {
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
- getLoggedIn: function() { return Promise.resolve(true); },
+ getLoggedIn() { return Promise.resolve(true); },
});
element = fixture('basic');
});
- teardown(function() {
+ teardown(() => {
sandbox.restore();
});
- test('does not show auth error on 403 by default', function(done) {
- var showAuthErrorStub = sandbox.stub(element, '_showAuthErrorAlert');
- var responseText = Promise.resolve('server says no.');
+ test('does not show auth error on 403 by default', done => {
+ const showAuthErrorStub = sandbox.stub(element, '_showAuthErrorAlert');
+ const responseText = Promise.resolve('server says no.');
element.fire('server-error',
- {response: {status: 403, text: function() { return responseText; }}}
+ {response: {status: 403, text() { return responseText; }}}
);
Promise.all([
element.$.restAPI.getLoggedIn.lastCall.returnValue,
responseText,
- ]).then(function() {
- assert.isFalse(showAuthErrorStub.calledOnce);
- done();
+ ]).then(() => {
+ assert.isFalse(showAuthErrorStub.calledOnce);
+ done();
});
});
- test('shows auth error on 403 and Authentication required', function(done) {
- var showAuthErrorStub = sandbox.stub(element, '_showAuthErrorAlert');
- var responseText = Promise.resolve('Authentication required\n');
+ test('shows auth error on 403 and Authentication required', done => {
+ const showAuthErrorStub = sandbox.stub(element, '_showAuthErrorAlert');
+ const responseText = Promise.resolve('Authentication required\n');
element.fire('server-error',
- {response: {status: 403, text: function() { return responseText; }}}
+ {response: {status: 403, text() { return responseText; }}}
);
Promise.all([
element.$.restAPI.getLoggedIn.lastCall.returnValue,
responseText,
- ]).then(function() {
+ ]).then(() => {
assert.isTrue(showAuthErrorStub.calledOnce);
done();
});
});
- test('show logged in error', function() {
+ test('show logged in error', () => {
sandbox.stub(element, '_showAuthErrorAlert');
element.fire('show-auth-required');
assert.isTrue(element._showAuthErrorAlert.calledWithExactly(
'Log in is required to perform that action.', 'Log in.'));
});
- test('show normal server error', function(done) {
- var showAlertStub = sandbox.stub(element, '_showAlert');
- var textSpy = sandbox.spy(function() { return Promise.resolve('ZOMG'); });
+ test('show normal server error', done => {
+ const showAlertStub = sandbox.stub(element, '_showAlert');
+ const textSpy = sandbox.spy(() => { return Promise.resolve('ZOMG'); });
element.fire('server-error', {response: {status: 500, text: textSpy}});
assert.isTrue(textSpy.called);
Promise.all([
element.$.restAPI.getLoggedIn.lastCall.returnValue,
textSpy.lastCall.returnValue,
- ]).then(function() {
+ ]).then(() => {
assert.isTrue(showAlertStub.calledOnce);
assert.isTrue(showAlertStub.lastCall.calledWithExactly(
'Server error: ZOMG'));
@@ -103,9 +103,9 @@
});
});
- test('suppress TOO_MANY_FILES error', function(done) {
- var showAlertStub = sandbox.stub(element, '_showAlert');
- var textSpy = sandbox.spy(function() {
+ test('suppress TOO_MANY_FILES error', done => {
+ const showAlertStub = sandbox.stub(element, '_showAlert');
+ const textSpy = sandbox.spy(() => {
return Promise.resolve('too many files to find conflicts');
});
element.fire('server-error', {response: {status: 500, text: textSpy}});
@@ -114,17 +114,17 @@
Promise.all([
element.$.restAPI.getLoggedIn.lastCall.returnValue,
textSpy.lastCall.returnValue,
- ]).then(function() {
+ ]).then(() => {
assert.isFalse(showAlertStub.called);
done();
});
});
- test('show network error', function(done) {
- var consoleErrorStub = sandbox.stub(console, 'error');
- var showAlertStub = sandbox.stub(element, '_showAlert');
+ test('show network error', done => {
+ const consoleErrorStub = sandbox.stub(console, 'error');
+ const showAlertStub = sandbox.stub(element, '_showAlert');
element.fire('network-error', {error: new Error('ZOMG')});
- flush(function() {
+ flush(() => {
assert.isTrue(showAlertStub.calledOnce);
assert.isTrue(showAlertStub.lastCall.calledWithExactly(
'Server unavailable'));
@@ -134,21 +134,21 @@
});
});
- test('show auth refresh toast', function(done) {
- var refreshStub = sandbox.stub(element.$.restAPI, 'checkCredentials',
- function() { return Promise.resolve(true); });
- var toastSpy = sandbox.spy(element, '_createToastAlert');
- var windowOpen = sandbox.stub(window, 'open');
- var responseText = Promise.resolve('Authentication required\n');
+ test('show auth refresh toast', done => {
+ const refreshStub = sandbox.stub(element.$.restAPI, 'checkCredentials',
+ () => { return Promise.resolve(true); });
+ const toastSpy = sandbox.spy(element, '_createToastAlert');
+ const windowOpen = sandbox.stub(window, 'open');
+ const responseText = Promise.resolve('Authentication required\n');
element.fire('server-error',
- {response: {status: 403, text: function() { return responseText; }}}
+ {response: {status: 403, text() { return responseText; }}}
);
Promise.all([
element.$.restAPI.getLoggedIn.lastCall.returnValue,
responseText,
- ]).then(function() {
+ ]).then(() => {
assert.isTrue(toastSpy.called);
- var toast = toastSpy.lastCall.returnValue;
+ let toast = toastSpy.lastCall.returnValue;
assert.isOk(toast);
assert.include(
Polymer.dom(toast.root).textContent, 'Auth error');
@@ -163,12 +163,12 @@
assert.equal(windowOpen.lastCall.args[2].indexOf('noopener=yes'),
-1);
- var hideToastSpy = sandbox.spy(toast, 'hide');
+ const hideToastSpy = sandbox.spy(toast, 'hide');
element._handleWindowFocus();
assert.isTrue(refreshStub.called);
element.flushDebouncer('checkLoggedIn');
- flush(function() {
+ flush(() => {
assert.isTrue(refreshStub.called);
assert.isTrue(hideToastSpy.called);
@@ -182,18 +182,18 @@
});
});
- test('show alert', function() {
- var alertObj = {message: 'foo'}
+ test('show alert', () => {
+ const alertObj = {message: 'foo'};
sandbox.stub(element, '_showAlert');
- element.fire('show-alert', {message: 'foo'});
+ element.fire('show-alert', alertObj);
assert.isTrue(element._showAlert.calledOnce);
assert.equal(element._showAlert.lastCall.args[0], 'foo');
assert.isNotOk(element._showAlert.lastCall.args[1]);
assert.isNotOk(element._showAlert.lastCall.args[2]);
});
- test('checks stale credentials on visibility change', function() {
- var refreshStub = sandbox.stub(element.$.restAPI,
+ test('checks stale credentials on visibility change', () => {
+ const refreshStub = sandbox.stub(element.$.restAPI,
'checkCredentials');
sandbox.stub(Date, 'now').returns(999999);
element._lastCredentialCheck = 0;
@@ -211,19 +211,19 @@
assert.equal(element._lastCredentialCheck, 999999);
});
- test('refresh loop continues on credential fail', function(done) {
- var accountPromise = Promise.resolve(null);
+ test('refresh loop continues on credential fail', done => {
+ const accountPromise = Promise.resolve(null);
sandbox.stub(element.$.restAPI, 'checkCredentials')
.returns(accountPromise);
- var requestCheckStub = sandbox.stub(element, '_requestCheckLoggedIn');
- var handleRefreshStub = sandbox.stub(element,
+ const requestCheckStub = sandbox.stub(element, '_requestCheckLoggedIn');
+ const handleRefreshStub = sandbox.stub(element,
'_handleCredentialRefreshed');
- var reloadStub = sandbox.stub(element, '_reloadPage');
+ const reloadStub = sandbox.stub(element, '_reloadPage');
element._refreshingCredentials = true;
element._checkSignedIn();
- accountPromise.then(function() {
+ accountPromise.then(() => {
assert.isTrue(requestCheckStub.called);
assert.isFalse(handleRefreshStub.called);
assert.isFalse(reloadStub.called);
@@ -231,20 +231,20 @@
});
});
- test('refreshes with same credentials', function(done) {
- var accountPromise = Promise.resolve({_account_id: 1234});
+ test('refreshes with same credentials', done => {
+ const accountPromise = Promise.resolve({_account_id: 1234});
sandbox.stub(element.$.restAPI, 'checkCredentials')
.returns(accountPromise);
- var requestCheckStub = sandbox.stub(element, '_requestCheckLoggedIn');
- var handleRefreshStub = sandbox.stub(element,
+ const requestCheckStub = sandbox.stub(element, '_requestCheckLoggedIn');
+ const handleRefreshStub = sandbox.stub(element,
'_handleCredentialRefreshed');
- var reloadStub = sandbox.stub(element, '_reloadPage');
+ const reloadStub = sandbox.stub(element, '_reloadPage');
element.knownAccountId = 1234;
element._refreshingCredentials = true;
element._checkSignedIn();
- accountPromise.then(function() {
+ accountPromise.then(() => {
assert.isFalse(requestCheckStub.called);
assert.isTrue(handleRefreshStub.called);
assert.isFalse(reloadStub.called);
@@ -252,20 +252,20 @@
});
});
- test('reloads when refreshed credentials differ', function(done) {
- var accountPromise = Promise.resolve({_account_id: 1234});
+ test('reloads when refreshed credentials differ', done => {
+ const accountPromise = Promise.resolve({_account_id: 1234});
sandbox.stub(element.$.restAPI, 'checkCredentials')
.returns(accountPromise);
- var requestCheckStub = sandbox.stub(element, '_requestCheckLoggedIn');
- var handleRefreshStub = sandbox.stub(element,
+ const requestCheckStub = sandbox.stub(element, '_requestCheckLoggedIn');
+ const handleRefreshStub = sandbox.stub(element,
'_handleCredentialRefreshed');
- var reloadStub = sandbox.stub(element, '_reloadPage');
+ const reloadStub = sandbox.stub(element, '_reloadPage');
element.knownAccountId = 4321; // Different from 1234
element._refreshingCredentials = true;
element._checkSignedIn();
- accountPromise.then(function() {
+ accountPromise.then(() => {
assert.isFalse(requestCheckStub.called);
assert.isFalse(handleRefreshStub.called);
assert.isTrue(reloadStub.called);
@@ -273,9 +273,9 @@
});
});
- test('dismissOnNavigation respected', function() {
- var asyncStub = sandbox.stub(element, 'async');
- var hideSpy = sandbox.spy(element, '_hideAlert');
+ test('dismissOnNavigation respected', () => {
+ const asyncStub = sandbox.stub(element, 'async');
+ const hideSpy = sandbox.spy(element, '_hideAlert');
// No async call when dismissOnNavigation supplied.
element._showAlert('test', null, null, true);
assert.isFalse(asyncStub.called);
diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
index 9a3a267..b3180e9 100644
--- a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
+++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
@@ -162,6 +162,13 @@
</td>
<td>Show selected change</td>
</tr>
+ <tr>
+ <td>
+ <span class="key modifier">Shift</span>
+ <span class="key">r</span>
+ </td>
+ <td>Refresh list of changes</td>
+ </tr>
</tbody>
<!-- Dashboard -->
<tbody hidden$="[[!_computeInView(view, 'gr-dashboard-view')]]" hidden>
@@ -183,6 +190,13 @@
</td>
<td>Show selected change</td>
</tr>
+ <tr>
+ <td>
+ <span class="key modifier">Shift</span>
+ <span class="key">r</span>
+ </td>
+ <td>Refresh list of changes</td>
+ </tr>
</tbody>
<!-- Change View -->
<tbody hidden$="[[!_computeInView(view, 'gr-change-view')]]" hidden>
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
index 4edf1be..ee08e75 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
@@ -15,7 +15,7 @@
'use strict';
// Possible static search options for auto complete.
- var SEARCH_OPERATORS = [
+ const SEARCH_OPERATORS = [
'added',
'age',
'age:1week', // Give an example age
@@ -81,12 +81,12 @@
'tr',
];
- var SELF_EXPRESSION = 'self';
- var ME_EXPRESSION = 'me';
+ const SELF_EXPRESSION = 'self';
+ const ME_EXPRESSION = 'me';
- var MAX_AUTOCOMPLETE_RESULTS = 10;
+ const MAX_AUTOCOMPLETE_RESULTS = 10;
- var TOKENIZE_REGEX = /(?:[^\s"]+|"[^"]*")+\s*/g;
+ const TOKENIZE_REGEX = /(?:[^\s"]+|"[^"]*")+\s*/g;
Polymer({
is: 'gr-search-bar',
@@ -113,22 +113,22 @@
},
keyEventTarget: {
type: Object,
- value: function() { return document.body; },
+ value() { return document.body; },
},
query: {
type: Function,
- value: function() {
+ value() {
return this._getSearchSuggestions.bind(this);
},
},
_inputVal: String,
},
- _valueChanged: function(value) {
+ _valueChanged(value) {
this._inputVal = value;
},
- _handleInputCommit: function(e) {
+ _handleInputCommit(e) {
this._preventDefaultAndNavigateToInputVal(e);
},
@@ -140,9 +140,9 @@
*
* @param {!Event} e
*/
- _preventDefaultAndNavigateToInputVal: function(e) {
+ _preventDefaultAndNavigateToInputVal(e) {
e.preventDefault();
- var target = Polymer.dom(e).rootTarget;
+ const target = Polymer.dom(e).rootTarget;
// If the target is the #searchInput or has a sub-input component, that
// is what holds the focus as opposed to the target from the DOM event.
if (target.$.input) {
@@ -164,22 +164,21 @@
* @return {!Promise} This returns a promise that resolves to an array of
* strings.
*/
- _fetchAccounts: function(predicate, expression) {
+ _fetchAccounts(predicate, expression) {
if (expression.length === 0) { return Promise.resolve([]); }
return this.$.restAPI.getSuggestedAccounts(
expression,
MAX_AUTOCOMPLETE_RESULTS)
- .then(function(accounts) {
+ .then(accounts => {
if (!accounts) { return []; }
- return accounts.map(function(acct) {
- return predicate + ':"' + acct.name + ' <' + acct.email + '>"';
- });
- }).then(function(accounts) {
+ return accounts.map(acct =>
+ predicate + ':"' + acct.name + ' <' + acct.email + '>"');
+ }).then(accounts => {
// When the expression supplied is a beginning substring of 'self',
// add it as an autocomplete option.
- if (SELF_EXPRESSION.indexOf(expression) === 0) {
+ if (SELF_EXPRESSION.startsWith(expression)) {
return accounts.concat([predicate + ':' + SELF_EXPRESSION]);
- } else if (ME_EXPRESSION.indexOf(expression) === 0) {
+ } else if (ME_EXPRESSION.startsWith(expression)) {
return accounts.concat([predicate + ':' + ME_EXPRESSION]);
} else {
return accounts;
@@ -196,15 +195,15 @@
* @return {!Promise} This returns a promise that resolves to an array of
* strings.
*/
- _fetchGroups: function(predicate, expression) {
+ _fetchGroups(predicate, expression) {
if (expression.length === 0) { return Promise.resolve([]); }
return this.$.restAPI.getSuggestedGroups(
expression,
MAX_AUTOCOMPLETE_RESULTS)
- .then(function(groups) {
+ .then(groups => {
if (!groups) { return []; }
- var keys = Object.keys(groups);
- return keys.map(function(key) { return predicate + ':' + key; });
+ const keys = Object.keys(groups);
+ return keys.map(key => predicate + ':' + key);
});
},
@@ -217,14 +216,14 @@
* @return {!Promise} This returns a promise that resolves to an array of
* strings.
*/
- _fetchProjects: function(predicate, expression) {
+ _fetchProjects(predicate, expression) {
return this.$.restAPI.getSuggestedProjects(
expression,
MAX_AUTOCOMPLETE_RESULTS)
- .then(function(projects) {
+ .then(projects => {
if (!projects) { return []; }
- var keys = Object.keys(projects);
- return keys.map(function(key) { return predicate + ':' + key; });
+ const keys = Object.keys(projects);
+ return keys.map(key => predicate + ':' + key);
});
},
@@ -235,11 +234,11 @@
* @return {!Promise} This returns a promise that resolves to an array of
* strings.
*/
- _fetchSuggestions: function(input) {
+ _fetchSuggestions(input) {
// Split the input on colon to get a two part predicate/expression.
- var splitInput = input.split(':');
- var predicate = splitInput[0];
- var expression = splitInput[1] || '';
+ const splitInput = input.split(':');
+ const predicate = splitInput[0];
+ const expression = splitInput[1] || '';
// Switch on the predicate to determine what to autocomplete.
switch (predicate) {
case 'ownerin':
@@ -265,9 +264,7 @@
default:
return Promise.resolve(SEARCH_OPERATORS
- .filter(function(operator) {
- return operator.indexOf(input) !== -1;
- }));
+ .filter(operator => operator.includes(input)));
}
},
@@ -277,19 +274,19 @@
* @return {!Promise} This returns a promise that resolves to an array of
* strings.
*/
- _getSearchSuggestions: function(input) {
+ _getSearchSuggestions(input) {
// Allow spaces within quoted terms.
- var tokens = input.match(TOKENIZE_REGEX);
- var trimmedInput = tokens[tokens.length - 1].toLowerCase();
+ const tokens = input.match(TOKENIZE_REGEX);
+ const trimmedInput = tokens[tokens.length - 1].toLowerCase();
return this._fetchSuggestions(trimmedInput)
- .then(function(operators) {
+ .then(operators => {
if (!operators || !operators.length) { return []; }
return operators
// Prioritize results that start with the input.
- .sort(function(a, b) {
- var aContains = a.toLowerCase().indexOf(trimmedInput);
- var bContains = b.toLowerCase().indexOf(trimmedInput);
+ .sort((a, b) => {
+ const aContains = a.toLowerCase().indexOf(trimmedInput);
+ const bContains = b.toLowerCase().indexOf(trimmedInput);
if (aContains === bContains) {
return a.localeCompare(b);
}
@@ -304,7 +301,7 @@
// Return only the first {MAX_AUTOCOMPLETE_RESULTS} results.
.slice(0, MAX_AUTOCOMPLETE_RESULTS - 1)
// Map to an object to play nice with gr-autocomplete.
- .map(function(operator) {
+ .map(operator => {
return {
name: operator,
value: operator,
@@ -313,7 +310,7 @@
});
},
- _handleForwardSlashKey: function(e) {
+ _handleForwardSlashKey(e) {
if (this.shouldSuppressKeyboardShortcut(e) ||
this.modifierPressed(e)) { return; }
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.html b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.html
index 05ceb34..96240cd 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.html
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.html
@@ -35,26 +35,26 @@
</test-fixture>
<script>
- suite('gr-search-bar tests', function() {
- var element;
+ suite('gr-search-bar tests', () => {
+ let element;
- setup(function() {
+ setup(() => {
element = fixture('basic');
});
- test('value is propagated to _inputVal', function() {
+ test('value is propagated to _inputVal', () => {
element.value = 'foo';
assert.equal(element._inputVal, 'foo');
});
- function getActiveElement() {
+ getActiveElement = () => {
return document.activeElement.shadowRoot ?
document.activeElement.shadowRoot.activeElement :
document.activeElement;
}
- test('tap on search button triggers nav', function(done) {
- sinon.stub(page, 'show', function() {
+ test('tap on search button triggers nav', done => {
+ sinon.stub(page, 'show', () => {
page.show.restore();
assert.notEqual(getActiveElement(), element.$.searchInput);
assert.notEqual(getActiveElement(), element.$.searchButton);
@@ -64,8 +64,8 @@
MockInteractions.tap(element.$.searchButton);
});
- test('enter in search input triggers nav', function(done) {
- sinon.stub(page, 'show', function() {
+ test('enter in search input triggers nav', done => {
+ sinon.stub(page, 'show', () => {
page.show.restore();
assert.notEqual(getActiveElement(), element.$.searchInput);
assert.notEqual(getActiveElement(), element.$.searchButton);
@@ -76,8 +76,8 @@
null, 'enter');
});
- test('search query should be double-escaped', function() {
- var showStub = sinon.stub(page, 'show');
+ test('search query should be double-escaped', () => {
+ const showStub = sinon.stub(page, 'show');
element.$.searchInput.text = 'fate/stay';
MockInteractions.pressAndReleaseKeyOn(element.$.searchInput.$.input, 13,
null, 'enter');
@@ -85,9 +85,9 @@
showStub.restore();
});
- test('input blurred after commit', function() {
- var showStub = sinon.stub(page, 'show');
- var blurSpy = sinon.spy(element.$.searchInput.$.input, 'blur');
+ test('input blurred after commit', () => {
+ const showStub = sinon.stub(page, 'show');
+ const blurSpy = sinon.spy(element.$.searchInput.$.input, 'blur');
element.$.searchInput.text = 'fate/stay';
MockInteractions.pressAndReleaseKeyOn(element.$.searchInput.$.input, 13,
null, 'enter');
@@ -96,97 +96,97 @@
blurSpy.restore();
});
- test('empty search query does not trigger nav', function() {
- var showSpy = sinon.spy(page, 'show');
+ test('empty search query does not trigger nav', () => {
+ const showSpy = sinon.spy(page, 'show');
element.value = '';
MockInteractions.pressAndReleaseKeyOn(element.$.searchInput.$.input, 13,
null, 'enter');
assert.isFalse(showSpy.called);
});
- test('keyboard shortcuts', function() {
- var focusSpy = sinon.spy(element.$.searchInput, 'focus');
- var selectAllSpy = sinon.spy(element.$.searchInput, 'selectAll');
+ test('keyboard shortcuts', () => {
+ const focusSpy = sinon.spy(element.$.searchInput, 'focus');
+ const selectAllSpy = sinon.spy(element.$.searchInput, 'selectAll');
MockInteractions.pressAndReleaseKeyOn(document.body, 191, null, '/');
assert.isTrue(focusSpy.called);
assert.isTrue(selectAllSpy.called);
});
- suite('_getSearchSuggestions', function() {
- setup(function() {
- sinon.stub(element.$.restAPI, 'getSuggestedAccounts', function() {
- return Promise.resolve([
+ suite('_getSearchSuggestions', () => {
+ setup(() => {
+ sinon.stub(element.$.restAPI, 'getSuggestedAccounts', () =>
+ Promise.resolve([
{
name: 'fred',
email: 'fred@goog.co',
},
- ]);
- });
- sinon.stub(element.$.restAPI, 'getSuggestedGroups', function() {
- return Promise.resolve({
+ ])
+ );
+ sinon.stub(element.$.restAPI, 'getSuggestedGroups', () =>
+ Promise.resolve({
Polygerrit: 0,
gerrit: 0,
gerrittest: 0,
- });
- });
- sinon.stub(element.$.restAPI, 'getSuggestedProjects', function() {
- return Promise.resolve({
+ })
+ );
+ sinon.stub(element.$.restAPI, 'getSuggestedProjects', () =>
+ Promise.resolve({
Polygerrit: 0,
- });
- });
+ })
+ );
});
- teardown(function() {
+ teardown(() => {
element.$.restAPI.getSuggestedAccounts.restore();
element.$.restAPI.getSuggestedGroups.restore();
element.$.restAPI.getSuggestedProjects.restore();
});
- test('Autocompletes accounts', function(done) {
- element._getSearchSuggestions('owner:fr').then(function(s) {
+ test('Autocompletes accounts', done => {
+ element._getSearchSuggestions('owner:fr').then(s => {
assert.equal(s[0].value, 'owner:"fred <fred@goog.co>"');
done();
});
});
- test('Inserts self as option when valid', function(done) {
- element._getSearchSuggestions('owner:s').then(function(s) {
+ test('Inserts self as option when valid', done => {
+ element._getSearchSuggestions('owner:s').then(s => {
assert.equal(s[0].value, 'owner:self');
- }).then(function() {
- element._getSearchSuggestions('owner:selfs').then(function(s) {
+ }).then(() => {
+ element._getSearchSuggestions('owner:selfs').then(s => {
assert.notEqual(s[0].value, 'owner:self');
done();
});
});
});
- test('Inserts me as option when valid', function(done) {
- element._getSearchSuggestions('owner:m').then(function(s) {
+ test('Inserts me as option when valid', done => {
+ element._getSearchSuggestions('owner:m').then(s => {
assert.equal(s[0].value, 'owner:me');
- }).then(function() {
- element._getSearchSuggestions('owner:meme').then(function(s) {
+ }).then(() => {
+ element._getSearchSuggestions('owner:meme').then(s => {
assert.notEqual(s[0].value, 'owner:me');
done();
});
});
});
- test('Autocompletes groups', function(done) {
- element._getSearchSuggestions('ownerin:pol').then(function(s) {
+ test('Autocompletes groups', done => {
+ element._getSearchSuggestions('ownerin:pol').then(s => {
assert.equal(s[0].value, 'ownerin:Polygerrit');
done();
});
});
- test('Autocompletes projects', function(done) {
- element._getSearchSuggestions('project:pol').then(function(s) {
+ test('Autocompletes projects', done => {
+ element._getSearchSuggestions('project:pol').then(s => {
assert.equal(s[0].value, 'project:Polygerrit');
done();
});
});
- test('Autocompletes simple searches', function(done) {
- element._getSearchSuggestions('is:o').then(function(s) {
+ test('Autocompletes simple searches', done => {
+ element._getSearchSuggestions('is:o').then(s => {
assert.equal(s[0].name, 'is:open');
assert.equal(s[0].value, 'is:open');
assert.equal(s[1].name, 'is:owner');
@@ -195,26 +195,25 @@
});
});
- test('Does not autocomplete with no match', function(done) {
- element._getSearchSuggestions('asdasdasdasd').then(function(s) {
+ test('Does not autocomplete with no match', done => {
+ element._getSearchSuggestions('asdasdasdasd').then(s => {
assert.equal(s.length, 0);
done();
});
});
- test('Autocomplete doesnt override exact matches to input',
- function(done) {
- element._getSearchSuggestions('ownerin:gerrit').then(function(s) {
- assert.equal(s[0].value, 'ownerin:gerrit');
- done();
- });
- });
+ test('Autocomplete doesnt override exact matches to input', done => {
+ element._getSearchSuggestions('ownerin:gerrit').then(s => {
+ assert.equal(s[0].value, 'ownerin:gerrit');
+ done();
+ });
+ });
- test('Autocomplete respects spaces', function(done) {
- element._getSearchSuggestions('is:ope').then(function(s) {
+ test('Autocomplete respects spaces', done => {
+ element._getSearchSuggestions('is:ope').then(s => {
assert.equal(s[0].name, 'is:open');
assert.equal(s[0].value, 'is:open');
- element._getSearchSuggestions('is:ope ').then(function(s) {
+ element._getSearchSuggestions('is:ope ').then(s => {
assert.equal(s.length, 0);
done();
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 214454a..c407829 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -405,7 +405,6 @@
td.classList.add(line.type);
var html = this._escapeHTML(text);
html = this._addTabWrappers(html, this._prefs.tab_size);
-
if (!this._prefs.line_wrapping &&
this._textLength(text, this._prefs.tab_size) >
this._prefs.line_length) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
index a3ff3d1..76d2a69 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -264,7 +264,8 @@
<option value="SIDE_BY_SIDE">Side By Side</option>
<option value="UNIFIED_DIFF">Unified</option>
</select>
- <span hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]">
+ <span id="diffPrefsContainer"
+ hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]" hidden>
<span class="preferences desktop">
<span
hidden$="[[_computeModeSelectHidden(_isImageDiff)]]">/</span>
@@ -289,8 +290,6 @@
</div>
<gr-diff
id="diff"
- project="[[_change.project]]"
- commit="[[_change.current_revision]]"
is-image-diff="{{_isImageDiff}}"
files-weblinks="{{_filesWeblinks}}"
change-num="[[_changeNum]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index 166a3d3..663cd3b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -252,6 +252,38 @@
'Should navigate to /c/42/1');
});
+ test('Diff preferences hidden when no prefs or logged out',
+ function() {
+ element._loggedIn = false;
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = true;
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = false;
+ element._prefs = {'font_size': '12'};
+ flushAsynchronousOperations();
+ assert.isTrue(element.$.diffPrefsContainer.hidden);
+
+ element._loggedIn = true;
+ flushAsynchronousOperations();
+ assert.isFalse(element.$.diffPrefsContainer.hidden);
+ });
+
+ test('prefsButton opens gr-diff-preferences', function() {
+ var handlePrefsTapSpy = sandbox.spy(element, '_handlePrefsTap');
+ var overlayOpenStub = sandbox.stub(element.$.diffPreferences,
+ 'open');
+ var prefsButton = Polymer.dom(element.root).querySelector('.prefsButton');
+
+ MockInteractions.tap(prefsButton);
+
+ assert.isTrue(handlePrefsTapSpy.called);
+ assert.isTrue(overlayOpenStub.called);
+ });
+
test('go up to change via kb without change loaded', function() {
element._changeNum = '42';
element._patchRange = {
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 cd1931a..ba402f2 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -54,8 +54,6 @@
type: Object,
observer: '_projectConfigChanged',
},
- project: String,
- commit: String,
displayLine: {
type: Boolean,
value: false,
@@ -74,6 +72,7 @@
type: Boolean,
reflectToAttribute: true,
},
+ noRenderOnPrefsChange: Boolean,
_loggedIn: {
type: Boolean,
value: false,
@@ -438,7 +437,7 @@
this.updateStyles();
- if (this._diff && this._comments) {
+ if (this._diff && this._comments && !this.noRenderOnPrefsChange) {
this._renderDiffTable();
}
},
@@ -572,8 +571,8 @@
},
_getImages: function() {
- return this.$.restAPI.getImagesForDiff(this.project, this.commit,
- this.changeNum, this._diff, this.patchRange);
+ return this.$.restAPI.getImagesForDiff(this.changeNum, this._diff,
+ this.patchRange);
},
_projectConfigChanged: function(projectConfig) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 5868f77..f765b26 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -327,9 +327,6 @@
stubs.push(sandbox.stub(element.$.restAPI, 'getCommitInfo',
function() { return Promise.resolve(mockCommit); }));
stubs.push(sandbox.stub(element.$.restAPI,
- 'getCommitFileContents',
- function() { return Promise.resolve(mockFile1); }));
- stubs.push(sandbox.stub(element.$.restAPI,
'getChangeFileContents',
function(changeId, patchNum, path, opt_parentIndex) {
return Promise.resolve(opt_parentIndex === 1 ? mockFile1 :
@@ -840,6 +837,59 @@
});
});
+ suite('change in preferences', function() {
+ setup(function() {
+ element._diff = {
+ meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
+ meta_b: {name: 'carrot.jpg', content_type: 'image/jpeg',
+ lines: 560},
+ diff_header: [],
+ intraline_status: 'OK',
+ change_type: 'MODIFIED',
+ content: [{skip: 66}],
+ };
+ element._comments = {
+ meta: {
+ changeNum: '42',
+ patchRange: {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ },
+ path: '/path/to/foo',
+ projectConfig: {foo: 'bar'},
+ },
+ left: [
+ {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ ],
+ right: [
+ {id: 'c1', __commentSide: 'right'},
+ {id: 'c2', __commentSide: 'right'},
+ {id: 'd1', __draft: true, __commentSide: 'right'},
+ {id: 'd2', __draft: true, __commentSide: 'right'},
+ ],
+ };
+ });
+
+ test('change in preferences re-renders diff', function() {
+ sandbox.stub(element, '_renderDiffTable');
+ element.prefs = {};
+ element.prefs = {time_format: 'HHMM_12'};
+ assert.isTrue(element._renderDiffTable.called);
+ });
+
+ test('change in preferences does not re-renders diff with ' +
+ 'noRenderOnPrefsChange', function() {
+ sandbox.stub(element, '_renderDiffTable');
+ element.noRenderOnPrefsChange = true;
+ element.prefs = {};
+ element.prefs = {time_format: 'HHMM_12'};
+ assert.isFalse(element._renderDiffTable.called);
+ });
+ });
+
suite('handle comment-update', function() {
setup(function() {
diff --git a/polygerrit-ui/app/elements/gr-app.html b/polygerrit-ui/app/elements/gr-app.html
index 6f09701..87a4687 100644
--- a/polygerrit-ui/app/elements/gr-app.html
+++ b/polygerrit-ui/app/elements/gr-app.html
@@ -43,7 +43,7 @@
<style>
:host {
display: flex;
- min-height: 100vh;
+ min-height: 100%;
flex-direction: column;
}
gr-main-header,
diff --git a/polygerrit-ui/app/elements/gr-app_test.html b/polygerrit-ui/app/elements/gr-app_test.html
index 35b5ab1..dbe3423 100644
--- a/polygerrit-ui/app/elements/gr-app_test.html
+++ b/polygerrit-ui/app/elements/gr-app_test.html
@@ -53,7 +53,9 @@
plugin: {js_resource_paths: []},
});
},
+ getPreferences: function() { return Promise.resolve({my: []}); },
getVersion: function() { return Promise.resolve(42); },
+ probePath: function() { return Promise.resolve(42); },
});
element = fixture('basic');
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
index f485dd6..de3134b 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
@@ -205,6 +205,16 @@
on-change="_handleExpandInlineDiffsChanged">
</span>
</section>
+ <section>
+ <span class="title">Publish comments on push</span>
+ <span class="value">
+ <input
+ id="publishCommentsOnPush"
+ type="checkbox"
+ checked$="[[_localPrefs.publish_comments_on_push]]"
+ on-change="_handlePublishCommentsOnPushChanged">
+ </span>
+ </section>
<gr-button
id="savePrefs"
on-tap="_handleSavePreferences"
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
index 4647a2d..6c26550 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
@@ -21,6 +21,7 @@
'email_strategy',
'diff_view',
'expand_inline_diffs',
+ 'publish_comments_on_push',
'email_format',
];
@@ -256,6 +257,11 @@
this.$.expandInlineDiffs.checked);
},
+ _handlePublishCommentsOnPushChanged: function() {
+ this.set('_localPrefs.publish_comments_on_push',
+ this.$.publishCommentsOnPush.checked);
+ },
+
_handleMenuChanged: function() {
if (this._isLoading()) { return; }
this._menuChanged = true;
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
index cb471d4..33e6bd5 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
@@ -171,6 +171,8 @@
.firstElementChild.bindValue, preferences.diff_view);
assert.equal(valueOf('Expand inline diffs', 'preferences')
.firstElementChild.checked, false);
+ assert.equal(valueOf('Publish comments on push', 'preferences')
+ .firstElementChild.checked, false);
assert.isFalse(element._prefsChanged);
assert.isFalse(element._menuChanged);
@@ -205,6 +207,29 @@
});
});
+ test('publish comments on push', function(done) {
+ var publishCommentsOnPush =
+ valueOf('Publish comments on push', 'preferences').firstElementChild;
+ MockInteractions.tap(publishCommentsOnPush);
+
+ assert.isFalse(element._menuChanged);
+ assert.isTrue(element._prefsChanged);
+
+ stub('gr-rest-api-interface', {
+ savePreferences: function(prefs) {
+ assert.equal(prefs.publish_comments_on_push, true);
+ return Promise.resolve();
+ }
+ });
+
+ // Save the change.
+ element._handleSavePreferences().then(function() {
+ assert.isFalse(element._prefsChanged);
+ assert.isFalse(element._menuChanged);
+ done();
+ });
+ });
+
test('diff preferences', function(done) {
// Rendered with the expected preferences selected.
assert.equal(valueOf('Context', 'diffPreferences')
diff --git a/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html b/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
index 10eadf9..71cdec4 100644
--- a/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
+++ b/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
@@ -17,6 +17,8 @@
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../gr-button/gr-button.html">
+<script src="../../../scripts/rootElement.js"></script>
+
<dom-module id="gr-alert">
<template>
<style>
diff --git a/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.js b/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.js
index ee7fe97..a2daeb3 100644
--- a/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.js
+++ b/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.js
@@ -60,14 +60,14 @@
this.actionText = opt_actionText;
this._hideActionButton = !opt_actionText;
this._actionCallback = opt_actionCallback;
- document.body.appendChild(this);
+ Gerrit.getRootElement().appendChild(this);
this._setShown(true);
},
hide: function() {
this._setShown(false);
if (this._hasZeroTransitionDuration()) {
- document.body.removeChild(this);
+ Gerrit.getRootElement().removeChild(this);
}
},
@@ -81,7 +81,7 @@
_handleTransitionEnd: function(e) {
if (this.shown) { return; }
- document.body.removeChild(this);
+ Gerrit.getRootElement().removeChild(this);
},
_handleActionTap: function(e) {
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index ad79a2c..ae42c2a 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -984,15 +984,7 @@
'/content' + parent);
},
- getCommitFileContents: function(projectName, commit, path) {
- return this._fetchB64File(
- '/projects/' + encodeURIComponent(projectName) +
- '/commits/' + encodeURIComponent(commit) +
- '/files/' + encodeURIComponent(path) +
- '/content');
- },
-
- getImagesForDiff: function(project, commit, changeNum, diff, patchRange) {
+ getImagesForDiff: function(changeNum, diff, patchRange) {
var promiseA;
var promiseB;
@@ -1000,15 +992,7 @@
if (patchRange.basePatchNum === 'PARENT') {
// Note: we only attempt to get the image from the first parent.
promiseA = this.getChangeFileContents(changeNum, patchRange.patchNum,
- diff.meta_a.name, 1)
- .catch(function(result) {
- // If getting the parent-indexed version of the image fails, it
- // may be because the API has not been rolled out. Fall back to
- // getting the file from the commit using the slow API.
- // NOTE(wyatta): Remove this when the rollout is complete.
- return this._getImageFromCommit(project, commit,
- diff.meta_a.name);
- }.bind(this));
+ diff.meta_a.name, 1);
} else {
promiseA = this.getChangeFileContents(changeNum,
patchRange.basePatchNum, diff.meta_a.name);
@@ -1043,19 +1027,6 @@
}.bind(this));
},
- /**
- * Remove when parent-indexed file requests are completely rolled out.
- */
- _getImageFromCommit: function(project, commit, path) {
- return this.getCommitInfo(project, commit).then(function(info) {
- if (info.parents.length !== 1) {
- return Promise.reject('Change commit has multiple parents.');
- }
- var parent = info.parents[0].commit;
- return this.getCommitFileContents(project, parent, path);
- }.bind(this));
- },
-
setChangeTopic: function(changeNum, topic) {
return this.send('PUT', '/changes/' + encodeURIComponent(changeNum) +
'/topic', {topic: topic});
diff --git a/polygerrit-ui/app/scripts/rootElement.js b/polygerrit-ui/app/scripts/rootElement.js
new file mode 100644
index 0000000..1a07edf
--- /dev/null
+++ b/polygerrit-ui/app/scripts/rootElement.js
@@ -0,0 +1,20 @@
+// 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.
+
+(function(window) {
+ window.Gerrit = window.Gerrit || {};
+ if (window.Gerrit.hasOwnProperty('getRootElement')) { return; }
+
+ window.Gerrit.getRootElement = () => document.body;
+})(window);
\ No newline at end of file
