Merge "Make sure user logged in before auto opening revert popup"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 34c72a6..ae02475 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5712,6 +5712,8 @@
|`robot_id` ||The ID of the robot that generated this comment.
|`robot_run_id` ||An ID of the run of the robot.
|`url` |optional|URL to more information.
+|`properties` |optional|
+Robot specific properties as map that maps arbitrary keys to values.
|===========================
[[robot-comment-input]]
diff --git a/contrib/populate-fixture-data.py b/contrib/populate-fixture-data.py
index c35f82c..b77c41a 100644
--- a/contrib/populate-fixture-data.py
+++ b/contrib/populate-fixture-data.py
@@ -182,14 +182,15 @@
def get_random_users(num_users):
- users = [(f, l) for f in FIRST_NAMES for l in LAST_NAMES][:num_users]
+ users = random.sample([(f, l) for f in FIRST_NAMES for l in LAST_NAMES],
+ num_users)
names = []
for u in users:
names.append({"firstname": u[0],
"lastname": u[1],
"name": u[0] + " " + u[1],
"username": u[0] + u[1],
- "email": u[0] + "." + u[1] + "@gmail.com",
+ "email": u[0] + "." + u[1] + "@gerritcodereview.com",
"http_password": "secret",
"groups": []})
return names
@@ -293,6 +294,7 @@
project_names = create_gerrit_projects(group_names)
for idx, u in enumerate(gerrit_users):
- create_change(u, project_names[4 * idx / len(gerrit_users)])
+ for _ in xrange(random.randint(1, 5)):
+ create_change(u, project_names[4 * idx / len(gerrit_users)])
main()
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/HttpSession.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/HttpSession.java
index 669b991..e5182df 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/HttpSession.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/HttpSession.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance;
import com.google.common.base.CharMatcher;
+import com.google.gerrit.common.Nullable;
import org.apache.http.HttpHost;
import org.apache.http.client.fluent.Executor;
@@ -24,17 +25,20 @@
import java.net.URI;
public class HttpSession {
-
+ protected TestAccount account;
protected final String url;
private final Executor executor;
- public HttpSession(GerritServer server, TestAccount account) {
+ public HttpSession(GerritServer server, @Nullable TestAccount account) {
this.url = CharMatcher.is('/').trimTrailingFrom(server.getUrl());
URI uri = URI.create(url);
- this.executor = Executor
- .newInstance()
- .auth(new HttpHost(uri.getHost(), uri.getPort()),
+ this.executor = Executor.newInstance();
+ this.account = account;
+ if (account != null) {
+ executor.auth(
+ new HttpHost(uri.getHost(), uri.getPort()),
account.username, account.httpPassword);
+ }
}
public String url() {
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/RestSession.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/RestSession.java
index 90ece46..689b2d0 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/RestSession.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/RestSession.java
@@ -18,6 +18,7 @@
import com.google.common.base.Preconditions;
import com.google.common.net.HttpHeaders;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.RawInput;
import com.google.gerrit.server.OutputFormat;
@@ -32,7 +33,7 @@
public class RestSession extends HttpSession {
- public RestSession(GerritServer server, TestAccount account) {
+ public RestSession(GerritServer server, @Nullable TestAccount account) {
super(server, account);
}
@@ -47,7 +48,7 @@
public RestResponse getWithHeader(String endPoint, Header header)
throws IOException {
- Request get = Request.Get(url + "/a" + endPoint);
+ Request get = Request.Get(getUrl(endPoint));
if (header != null) {
get.addHeader(header);
}
@@ -55,7 +56,7 @@
}
public RestResponse head(String endPoint) throws IOException {
- return execute(Request.Head(url + "/a" + endPoint));
+ return execute(Request.Head(getUrl(endPoint)));
}
public RestResponse put(String endPoint) throws IOException {
@@ -73,7 +74,7 @@
public RestResponse putWithHeader(String endPoint, Header header,
Object content) throws IOException {
- Request put = Request.Put(url + "/a" + endPoint);
+ Request put = Request.Put(getUrl(endPoint));
if (header != null) {
put.addHeader(header);
}
@@ -88,7 +89,7 @@
public RestResponse putRaw(String endPoint, RawInput stream) throws IOException {
Preconditions.checkNotNull(stream);
- Request put = Request.Put(url + "/a" + endPoint);
+ Request put = Request.Put(getUrl(endPoint));
put.addHeader(new BasicHeader("Content-Type", stream.getContentType()));
put.body(new BufferedHttpEntity(
new InputStreamEntity(
@@ -102,7 +103,15 @@
}
public RestResponse post(String endPoint, Object content) throws IOException {
- Request post = Request.Post(url + "/a" + endPoint);
+ return postWithHeader(endPoint, content, null);
+ }
+
+ public RestResponse postWithHeader(String endPoint, Object content,
+ Header header) throws IOException {
+ Request post = Request.Post(getUrl(endPoint));
+ if (header != null) {
+ post.addHeader(header);
+ }
if (content != null) {
post.addHeader(new BasicHeader("Content-Type", "application/json"));
post.body(new StringEntity(
@@ -113,6 +122,10 @@
}
public RestResponse delete(String endPoint) throws IOException {
- return execute(Request.Delete(url + "/a" + endPoint));
+ return execute(Request.Delete(getUrl(endPoint)));
+ }
+
+ private String getUrl(String endPoint) {
+ return url + (account != null ? "/a" : "") + endPoint;
}
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
index 064b206..7aa5876 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
@@ -76,6 +76,31 @@
}
@Test
+ public void noOptionalFields() throws Exception {
+ assume().that(notesMigration.enabled()).isTrue();
+
+ PushOneCommit.Result r = createChange();
+ RobotCommentInput in = createRobotCommentInputWithMandatoryFields();
+ ReviewInput reviewInput = new ReviewInput();
+ Map<String, List<RobotCommentInput>> robotComments = new HashMap<>();
+ robotComments.put(in.path, Collections.singletonList(in));
+ reviewInput.robotComments = robotComments;
+ reviewInput.message = "comment test";
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(reviewInput);
+
+ Map<String, List<RobotCommentInfo>> out = gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .robotComments();
+ assertThat(out).hasSize(1);
+ RobotCommentInfo comment = Iterables.getOnlyElement(out.get(in.path));
+ assertRobotComment(comment, in, false);
+ }
+
+ @Test
public void robotCommentsNotSupported() throws Exception {
assume().that(notesMigration.enabled()).isFalse();
@@ -95,17 +120,25 @@
.review(reviewInput);
}
- private RobotCommentInput createRobotCommentInput() {
+ private RobotCommentInput createRobotCommentInputWithMandatoryFields() {
RobotCommentInput in = new RobotCommentInput();
in.robotId = "happyRobot";
in.robotRunId = "1";
- in.url = "http://www.happy-robot.com";
in.line = 1;
in.message = "nit: trailing whitespace";
in.path = FILE_NAME;
return in;
}
+ private RobotCommentInput createRobotCommentInput() {
+ RobotCommentInput in = createRobotCommentInputWithMandatoryFields();
+ in.url = "http://www.happy-robot.com";
+ in.properties = new HashMap<>();
+ in.properties.put("key1", "value1");
+ in.properties.put("key2", "value2");
+ return in;
+ }
+
private void assertRobotComment(RobotCommentInfo c,
RobotCommentInput expected) {
assertRobotComment(c, expected, true);
@@ -116,6 +149,7 @@
assertThat(c.robotId).isEqualTo(expected.robotId);
assertThat(c.robotRunId).isEqualTo(expected.robotRunId);
assertThat(c.url).isEqualTo(expected.url);
+ assertThat(c.properties).isEqualTo(expected.properties);
assertThat(c.line).isEqualTo(expected.line);
assertThat(c.message).isEqualTo(expected.message);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index c990c8e..bdb6d2e 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.rest.account;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import com.google.common.collect.ImmutableList;
@@ -23,7 +24,10 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.acceptance.RestSession;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.DraftInput;
@@ -35,6 +39,7 @@
import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
@@ -52,6 +57,9 @@
import com.google.gerrit.server.project.Util;
import com.google.inject.Inject;
+import org.apache.http.Header;
+import org.apache.http.message.BasicHeader;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -59,11 +67,13 @@
@Inject
private AccountControl.Factory accountControlFactory;
+ private RestSession anonRestSession;
private TestAccount admin2;
private GroupInfo newGroup;
@Before
public void setUp() throws Exception {
+ anonRestSession = new RestSession(server, null);
admin2 = accounts.admin2();
GroupInput gi = new GroupInput();
gi.name = name("New-Group");
@@ -71,6 +81,11 @@
newGroup = gApi.groups().create(gi).get();
}
+ @After
+ public void tearDown() throws Exception {
+ removeRunAs();
+ }
+
@Test
public void voteOnBehalfOf() throws Exception {
allowCodeReviewOnBehalfOf();
@@ -386,6 +401,96 @@
.submit(in);
}
+ @Test
+ public void runAsValidUser() throws Exception {
+ allowRunAs();
+ RestResponse res =
+ adminRestSession.getWithHeader("/accounts/self", runAsHeader(user.id));
+ res.assertOK();
+ AccountInfo account =
+ newGson().fromJson(res.getEntityContent(), AccountInfo.class);
+ assertThat(account._accountId).isEqualTo(user.id.get());
+ }
+
+ @GerritConfig(name = "auth.enableRunAs", value = "false")
+ @Test
+ public void runAsDisabledByConfig() throws Exception {
+ allowRunAs();
+ RestResponse res =
+ adminRestSession.getWithHeader("/changes/", runAsHeader(user.id));
+ res.assertForbidden();
+ assertThat(res.getEntityContent())
+ .isEqualTo("X-Gerrit-RunAs disabled by auth.enableRunAs = false");
+ }
+
+ @Test
+ public void runAsNotPermitted() throws Exception {
+ RestResponse res =
+ adminRestSession.getWithHeader("/changes/", runAsHeader(user.id));
+ res.assertForbidden();
+ assertThat(res.getEntityContent())
+ .isEqualTo("not permitted to use X-Gerrit-RunAs");
+ }
+
+ @Test
+ public void runAsNeverPermittedForAnonymousUsers() throws Exception {
+ allowRunAs();
+ RestResponse res =
+ anonRestSession.getWithHeader("/changes/", runAsHeader(user.id));
+ res.assertForbidden();
+ assertThat(res.getEntityContent())
+ .isEqualTo("not permitted to use X-Gerrit-RunAs");
+ }
+
+ @Test
+ public void runAsInvalidUser() throws Exception {
+ allowRunAs();
+ RestResponse res = adminRestSession.getWithHeader(
+ "/changes/", runAsHeader("doesnotexist"));
+ res.assertForbidden();
+ assertThat(res.getEntityContent())
+ .isEqualTo("no account matches X-Gerrit-RunAs");
+ }
+
+ @Test
+ public void voteUsingRunAsAvoidsRestrictionsOfOnBehalfOf() throws Exception {
+ allowRunAs();
+ PushOneCommit.Result r = createChange();
+
+ setApiUser(user);
+ DraftInput di = new DraftInput();
+ di.path = Patch.COMMIT_MSG;
+ di.side = Side.REVISION;
+ di.line = 1;
+ di.message = "inline comment";
+ gApi.changes().id(r.getChangeId()).current().createDraft(di);
+ setApiUser(admin);
+
+ // Things that aren't allowed with on_behalf_of:
+ // - no labels.
+ // - publish other user's drafts.
+ ReviewInput in = new ReviewInput();
+ in.message = "message";
+ in.drafts = DraftHandling.PUBLISH;
+ RestResponse res = adminRestSession.postWithHeader(
+ "/changes/" + r.getChangeId() + "/revisions/current/review", in,
+ runAsHeader(user.id));
+ res.assertOK();
+
+ ChangeMessageInfo m = Iterables.getLast(
+ gApi.changes().id(r.getChangeId()).get().messages);
+ assertThat(m.message).endsWith(in.message);
+ assertThat(m.author._accountId).isEqualTo(user.id.get());
+
+ CommentInfo c = Iterables.getOnlyElement(
+ gApi.changes().id(r.getChangeId()).comments().get(di.path));
+ assertThat(c.author._accountId).isEqualTo(user.id.get());
+ assertThat(c.message).isEqualTo(di.message);
+
+ setApiUser(user);
+ assertThat(gApi.changes().id(r.getChangeId()).drafts()).isEmpty();
+ }
+
private void allowCodeReviewOnBehalfOf() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
LabelType codeReviewType = Util.codeReview();
@@ -416,4 +521,22 @@
cfg, Permission.READ, new AccountGroup.UUID(group.id), "refs/heads/master");
saveProjectConfig(project, cfg);
}
+
+ private void allowRunAs() throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+ Util.allow(cfg, GlobalCapability.RUN_AS,
+ SystemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID());
+ saveProjectConfig(allProjects, cfg);
+ }
+
+ private void removeRunAs() throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+ Util.remove(cfg, GlobalCapability.RUN_AS,
+ SystemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID());
+ saveProjectConfig(allProjects, cfg);
+ }
+
+ private static Header runAsHeader(Object user) {
+ return new BasicHeader("X-Gerrit-RunAs", user.toString());
+ }
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
index b1c25d0..472559b 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
@@ -102,6 +102,7 @@
public String robotId;
public String robotRunId;
public String url;
+ public Map<String, String> properties;
}
public ReviewInput message(String msg) {
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RobotCommentInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RobotCommentInfo.java
index a6b7593..9028a1d 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RobotCommentInfo.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RobotCommentInfo.java
@@ -14,8 +14,11 @@
package com.google.gerrit.extensions.common;
+import java.util.Map;
+
public class RobotCommentInfo extends CommentInfo {
public String robotId;
public String robotRunId;
public String url;
+ public Map<String, String> properties;
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Assignee.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Assignee.java
index 7921ebe..2956ffc 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Assignee.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Assignee.java
@@ -15,6 +15,7 @@
package com.google.gerrit.client.change;
import com.google.gerrit.client.FormatUtil;
+import com.google.gerrit.client.Gerrit;
import com.google.gerrit.client.NotSignedInDialog;
import com.google.gerrit.client.changes.ChangeApi;
import com.google.gerrit.client.changes.Util;
@@ -36,6 +37,8 @@
import com.google.gwt.uibinder.client.UiBinder;
import com.google.gwt.uibinder.client.UiField;
import com.google.gwt.uibinder.client.UiHandler;
+import com.google.gwt.user.client.DOM;
+import com.google.gwt.user.client.EventListener;
import com.google.gwt.user.client.rpc.StatusCodeException;
import com.google.gwt.user.client.ui.Composite;
import com.google.gwt.user.client.ui.HTMLPanel;
@@ -161,6 +164,10 @@
public void onSuccess(AccountInfo result) {
onCloseForm();
setAssignee(result);
+ Reviewers reviewers = getReviewers();
+ if (reviewers != null) {
+ reviewers.updateReviewerList();
+ }
}
@Override
@@ -180,7 +187,7 @@
private void setAssignee(AccountInfo assignee) {
currentAssignee = assignee;
- assigneeLink.setText(assignee != null ? assignee.name() : null);
+ assigneeLink.setText(assignee != null ? getName(assignee) : null);
assigneeLink.setTargetHistoryToken(assignee != null
? PageLinks.toAssigneeQuery(assignee.name() != null
? assignee.name()
@@ -189,4 +196,26 @@
: String.valueOf(assignee._accountId()))
: "");
}
+
+ private Reviewers getReviewers() {
+ Element e = DOM.getParent(getElement());
+ for (e = DOM.getParent(e); e != null; e = DOM.getParent(e)) {
+ EventListener l = DOM.getEventListener(e);
+ if (l instanceof ChangeScreen) {
+ ChangeScreen screen = (ChangeScreen) l;
+ return screen.reviewers;
+ }
+ }
+ return null;
+ }
+
+ private String getName(AccountInfo info) {
+ if (info.name() != null) {
+ return info.name();
+ }
+ if (info.email() != null) {
+ return info.email();
+ }
+ return Gerrit.info().user().anonymousCowardName();
+ }
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java
index b69d1c0..aa30760 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java
@@ -198,7 +198,7 @@
});
}
- private void updateReviewerList() {
+ void updateReviewerList() {
ChangeApi.detail(changeId.get(),
new GerritCallback<ChangeInfo>() {
@Override
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/RunAsFilter.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/RunAsFilter.java
index 210800d..7e71639 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/RunAsFilter.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/RunAsFilter.java
@@ -90,7 +90,10 @@
}
CurrentUser self = session.get().getUser();
- if (!self.getCapabilities().canRunAs()) {
+ if (!self.getCapabilities().canRunAs()
+ // Always disallow for anonymous users, even if permitted by the ACL,
+ // because that would be crazy.
+ || !self.isIdentifiedUser()) {
replyError(req, res,
SC_FORBIDDEN,
"not permitted to use " + RUN_AS,
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/UrlModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/UrlModule.java
index 8d8937c..18445b3 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/UrlModule.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/UrlModule.java
@@ -98,6 +98,11 @@
serveRegex("^/r/(.+)/?$").with(DirectChangeByCommit.class);
filter("/a/*").through(RequireIdentifiedUserFilter.class);
+
+ // Must be after RequireIdentifiedUserFilter so auth happens before checking
+ // for RunAs capability.
+ install(new RunAsFilter.Module());
+
serveRegex("^/(?:a/)?tools/(.*)$").with(ToolServlet.class);
// Bind servlets for REST root collections.
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java
index fa551e8..d5a4b00 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebModule.java
@@ -56,8 +56,6 @@
bind(RequestScopePropagator.class).to(GuiceRequestScopePropagator.class);
bind(HttpRequestContext.class);
- install(new RunAsFilter.Module());
-
installAuthModule();
if (options.enableMasterFeatures()) {
install(new UrlModule(options, authConfig));
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RobotComment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RobotComment.java
index da9584d..eadcfd0 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RobotComment.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RobotComment.java
@@ -15,12 +15,14 @@
package com.google.gerrit.reviewdb.client;
import java.sql.Timestamp;
+import java.util.Map;
import java.util.Objects;
public class RobotComment extends Comment {
public String robotId;
public String robotRunId;
public String url;
+ public Map<String, String> properties;
public RobotComment(Key key, Account.Id author, Timestamp writtenOn,
short side, String message, String serverId, String robotId,
@@ -47,7 +49,8 @@
.append("range=").append(Objects.toString(range, "")).append(',')
.append("revId=").append(revId != null ? revId : "").append(',')
.append("tag=").append(Objects.toString(tag, "")).append(',')
- .append("url=").append(url)
+ .append("url=").append(url).append(',')
+ .append("properties=").append(properties != null ? properties : "")
.append('}')
.toString();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
index be019fb..54f73bc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
@@ -180,6 +180,7 @@
rci.robotId = c.robotId;
rci.robotRunId = c.robotRunId;
rci.url = c.url;
+ rci.properties = c.properties;
fillCommentInfo(c, rci, loader);
return rci;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index fcb041b..c4c526b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -597,6 +597,7 @@
c.robotId, c.robotRunId);
e.parentUuid = Url.decode(c.inReplyTo);
e.url = c.url;
+ e.properties = c.properties;
e.setLineNbrAndRange(c.line, c.range);
e.tag = in.tag;
setCommentRevId(e, patchListCache, ctx.getChange(), ps);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MultiProgressMonitor.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MultiProgressMonitor.java
index 414ba5f..d081fe6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MultiProgressMonitor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MultiProgressMonitor.java
@@ -96,29 +96,6 @@
}
}
- synchronized void format(StringBuilder s, boolean first) {
- if (count == 0) {
- return;
- }
-
- if (!first) {
- s.append(',');
- }
- s.append(' ');
-
- if (!Strings.isNullOrEmpty(name)) {
- s.append(name).append(": ");
- }
-
- if (total == UNKNOWN) {
- s.append(count);
- } else {
- s.append(String.format("%d%% (%d/%d)",
- count * 100 / total,
- count, total));
- }
- }
-
/**
* Indicate that this sub-task is finished.
* <p>
@@ -339,9 +316,32 @@
StringBuilder s = new StringBuilder().append("\r").append(taskName)
.append(':');
- int firstLength = s.length();
- for (Task t : tasks) {
- t.format(s, s.length() == firstLength);
+ if (!tasks.isEmpty()) {
+ boolean first = true;
+ for (Task t : tasks) {
+ int count = t.count;
+ if (count == 0) {
+ continue;
+ }
+
+ if (!first) {
+ s.append(',');
+ } else {
+ first = false;
+ }
+
+ s.append(' ');
+ if (!Strings.isNullOrEmpty(t.name)) {
+ s.append(t.name).append(": ");
+ }
+ if (t.total == UNKNOWN) {
+ s.append(count);
+ } else {
+ s.append(String.format("%d%% (%d/%d)",
+ count * 100 / t.total,
+ count, t.total));
+ }
+ }
}
if (spinnerState != NO_SPINNER) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index 96cd4c6..18dd7f6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -646,10 +646,8 @@
addFooter(msg, FOOTER_SUBMITTED_WITH)
.append(label.status).append(": ").append(label.label);
if (label.appliedBy != null) {
- PersonIdent ident =
- newIdent(accountCache.get(label.appliedBy).getAccount(), when);
- msg.append(": ").append(ident.getName())
- .append(" <").append(ident.getEmailAddress()).append('>');
+ msg.append(": ");
+ addIdent(msg, label.appliedBy);
}
msg.append('\n');
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java
index a990e19..bc8af82 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java
@@ -31,11 +31,6 @@
private static final Pattern TOPIC_REMOVED_REGEXP =
Pattern.compile("^Topic (.+) removed$");
- private static final Pattern STATUS_ABANDONED_REGEXP =
- Pattern.compile("^Abandoned(\n.*)*$");
- private static final Pattern STATUS_RESTORED_REGEXP =
- Pattern.compile("^Restored(\n.*)*$");
-
private final ChangeMessage message;
private final Change noteDbChange;
@@ -57,7 +52,6 @@
checkUpdate(update);
update.setChangeMessage(message.getMessage());
setTopic(update);
- setStatus(update);
}
private void setTopic(ChangeUpdate update) {
@@ -86,21 +80,4 @@
noteDbChange.setTopic(null);
}
}
-
- private void setStatus(ChangeUpdate update) {
- String msg = message.getMessage();
- if (msg == null) {
- return;
- }
- if (STATUS_ABANDONED_REGEXP.matcher(msg).matches()) {
- update.setStatus(Change.Status.ABANDONED);
- noteDbChange.setStatus(Change.Status.ABANDONED);
- return;
- }
-
- if (STATUS_RESTORED_REGEXP.matcher(msg).matches()) {
- update.setStatus(Change.Status.NEW);
- noteDbChange.setStatus(Change.Status.NEW);
- }
- }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
index d4caa6c..cdd895a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
@@ -355,19 +355,16 @@
Change noteDbChange = new Change(null, null, null, null, null);
for (ChangeMessage msg : bundle.getChangeMessages()) {
- if (msg.getPatchSetId() == null) {
- // No dependency necessary; will get assigned to most recent patch set
- // in sortAndFillEvents.
- events.add(
- new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()));
- continue;
+ List<Event> msgEvents = parseChangeMessage(msg, change, noteDbChange);
+ if (msg.getPatchSetId() != null) {
+ PatchSetEvent pse = patchSetEvents.get(msg.getPatchSetId());
+ if (pse != null) {
+ for (Event e : msgEvents) {
+ e.addDep(pse);
+ }
+ }
}
- PatchSetEvent pse = patchSetEvents.get(msg.getPatchSetId());
- if (pse != null) {
- events.add(
- new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn())
- .addDep(pse));
- }
+ events.addAll(msgEvents);
}
sortAndFillEvents(change, noteDbChange, events, minPsNum);
@@ -396,6 +393,18 @@
}
}
+ private List<Event> parseChangeMessage(ChangeMessage msg, Change change,
+ Change noteDbChange) {
+ List<Event> events = new ArrayList<>(2);
+ events.add(new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()));
+ Optional<StatusChangeEvent> sce =
+ StatusChangeEvent.parseFromMessage(msg, change, noteDbChange);
+ if (sce.isPresent()) {
+ events.add(sce.get());
+ }
+ return events;
+ }
+
private static Integer getMinPatchSetNum(ChangeBundle bundle) {
Integer minPsNum = null;
for (PatchSet ps : bundle.getPatchSets()) {
@@ -418,8 +427,8 @@
private void sortAndFillEvents(Change change, Change noteDbChange,
List<Event> events, Integer minPsNum) {
- new EventSorter(events).sort();
events.add(new FinalUpdatesEvent(change, noteDbChange));
+ new EventSorter(events).sort();
// Ensure the first event in the list creates the change, setting the author
// and any required footers.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java
index d04d1b5..b7b08b0 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java
@@ -79,10 +79,6 @@
abstract void apply(ChangeUpdate update) throws OrmException, IOException;
- protected boolean isPatchSet() {
- return false;
- }
-
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
@@ -95,6 +91,7 @@
@Override
public int compareTo(Event other) {
return ComparisonChain.start()
+ .compareFalseFirst(this.isFinalUpdates(), other.isFinalUpdates())
.compare(this.when, other.when)
.compareTrueFirst(isPatchSet(), isPatchSet())
.compareTrueFirst(this.predatesChange, other.predatesChange)
@@ -103,4 +100,12 @@
ReviewDbUtil.intKeyOrdering().nullsLast())
.result();
}
+
+ private boolean isPatchSet() {
+ return this instanceof PatchSetEvent;
+ }
+
+ private boolean isFinalUpdates() {
+ return this instanceof FinalUpdatesEvent;
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java
index 3080be7..26f6b1f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java
@@ -46,9 +46,14 @@
// TODO(dborowitz): Stamp approximate approvals at this time.
update.fixStatus(change.getStatus());
}
- if (change.getSubmissionId() != null) {
+ if (change.getSubmissionId() != null
+ && noteDbChange.getSubmissionId() == null) {
update.setSubmissionId(change.getSubmissionId());
}
+ if (!Objects.equals(change.getAssignee(), noteDbChange.getAssignee())) {
+ // TODO(dborowitz): Parse intermediate values out from messages.
+ update.setAssignee(change.getAssignee());
+ }
if (!update.isEmpty()) {
update.setSubjectForCommit("Final NoteDb migration updates");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/PatchSetEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/PatchSetEvent.java
index 5baddd3..c3fb267 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/PatchSetEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/PatchSetEvent.java
@@ -66,11 +66,6 @@
}
}
- @Override
- protected boolean isPatchSet() {
- return true;
- }
-
private void setRevision(ChangeUpdate update, PatchSet ps)
throws IOException {
String rev = ps.getRevision().get();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java
new file mode 100644
index 0000000..29e0868
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java
@@ -0,0 +1,90 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb.rebuild;
+
+import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gwtorm.server.OrmException;
+
+import java.sql.Timestamp;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+class StatusChangeEvent extends Event {
+ private static final ImmutableMap<Change.Status, Pattern> PATTERNS =
+ ImmutableMap.of(
+ Change.Status.ABANDONED, Pattern.compile("^Abandoned(\n.*)*$"),
+ Change.Status.MERGED, Pattern.compile(
+ "^Change has been successfully"
+ + " (merged|cherry-picked|rebased|pushed).*$"),
+ Change.Status.NEW, Pattern.compile("^Restored(\n.*)*$"));
+
+ static Optional<StatusChangeEvent> parseFromMessage(ChangeMessage message,
+ Change change, Change noteDbChange) {
+ String msg = message.getMessage();
+ if (msg == null) {
+ return Optional.absent();
+ }
+ for (Map.Entry<Change.Status, Pattern> e : PATTERNS.entrySet()) {
+ if (e.getValue().matcher(msg).matches()) {
+ return Optional.of(new StatusChangeEvent(
+ message, change, noteDbChange, e.getKey()));
+ }
+ }
+ return Optional.absent();
+ }
+
+ private final Change change;
+ private final Change noteDbChange;
+ private final Change.Status status;
+
+ private StatusChangeEvent(ChangeMessage message, Change change,
+ Change noteDbChange, Change.Status status) {
+ this(message.getPatchSetId(), message.getAuthor(),
+ message.getWrittenOn(), change, noteDbChange, message.getTag(),
+ status);
+ }
+
+ private StatusChangeEvent(PatchSet.Id psId, Account.Id author,
+ Timestamp when, Change change, Change noteDbChange,
+ String tag, Change.Status status) {
+ super(psId, author, when, change.getCreatedOn(), tag);
+ this.change = change;
+ this.noteDbChange = noteDbChange;
+ this.status = status;
+ }
+
+ @Override
+ boolean uniquePerUpdate() {
+ return true;
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
+ void apply(ChangeUpdate update) throws OrmException {
+ checkUpdate(update);
+ update.fixStatus(status);
+ noteDbChange.setStatus(status);
+ if (status == Change.Status.MERGED) {
+ update.setSubmissionId(change.getSubmissionId());
+ noteDbChange.setSubmissionId(change.getSubmissionId());
+ }
+ }
+}
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 dcb2ca7..87e03dc 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
@@ -201,7 +201,8 @@
</div>
</div>
<gr-diff
- hidden$="[[_computeHiddenState(file.__expanded)]]"
+ hidden$="[[!file.__expanded]]"
+ expanded="[[file.__expanded]]"
project="[[change.project]]"
commit="[[change.current_revision]]"
change-num="[[changeNum]]"
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 dae798d..a562338 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
@@ -437,10 +437,6 @@
return expanded ? '▼' : '◀';
},
- _computeHiddenState: function(expanded) {
- return !expanded;
- },
-
_computeFilesShown: function(numFilesShown, files) {
return files.base.slice(0, numFilesShown);
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
index c3b6233..864712d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
@@ -44,15 +44,20 @@
padding: .5em .7em;
}
.header {
+ cursor: pointer;
display: flex;
- padding-bottom: 0;
font-family: 'Open Sans', sans-serif;
+ padding-bottom: 0;
}
- .headerLeft {
+ .headerMiddle {
+ color: #666;
flex: 1;
+ overflow: hidden;
}
.authorName,
.draftLabel {
+ display: block;
+ float: left;
font-weight: bold;
}
.draftLabel {
@@ -62,6 +67,7 @@
.date {
justify-content: flex-end;
margin-left: 5px;
+ white-space: nowrap;
}
a.date:link,
a.date:visited {
@@ -113,19 +119,62 @@
background-color: #fff;
display: block;
}
+ .show-hide {
+ margin-left: .4em;
+ }
+ input.show-hide {
+ display: none;
+ }
+ label.show-hide {
+ color: #000;
+ cursor: pointer;
+ display: block;
+ font-size: .8em;
+ height: 1.1em;
+ margin-top: .1em;
+ }
+ #container .collapsedContent {
+ display: none;
+ }
+ #container.collapsed {
+ padding-bottom: 3px;
+ }
+ #container.collapsed .collapsedContent {
+ display: block;
+ overflow: hidden;
+ padding-left: 5px;
+ text-overflow: ellipsis;
+ white-space: nowrap;
+ }
+ #container.collapsed .actions,
+ #container.collapsed gr-linked-text,
+ #container.collapsed iron-autogrow-textarea {
+ display: none;
+ }
</style>
<div id="container"
class="container"
on-mouseenter="_handleMouseEnter"
on-mouseleave="_handleMouseLeave">
- <div class="header" id="header">
+ <div class="header" id="header" on-click="_handleToggleCollapsed">
<div class="headerLeft">
<span class="authorName">[[comment.author.name]]</span>
<span class="draftLabel">DRAFT</span>
</div>
+ <div class="headerMiddle">
+ <span class="collapsedContent">[[comment.message]]</span>
+ </div>
<a class="date" href$="[[_computeLinkToComment(comment)]]" on-tap="_handleLinkTap">
<gr-date-formatter date-str="[[comment.updated]]"></gr-date-formatter>
</a>
+ <div class="show-hide">
+ <label class="show-hide">
+ <input type="checkbox" class="show-hide"
+ checked$="[[_commentCollapsed]]"
+ on-change="_handleToggleCollapsed">
+ [[_computeShowHideText(_commentCollapsed)]]
+ </label>
+ </div>
</div>
<iron-autogrow-textarea
id="editTextarea"
@@ -137,6 +186,7 @@
<gr-linked-text class="message"
pre
content="[[comment.message]]"
+ collapsed="[[_commentCollapsed]]"
config="[[projectConfig.commentlinks]]"></gr-linked-text>
<div class="actions" hidden$="[[!showActions]]">
<gr-button class="action reply" on-tap="_handleReply">Reply</gr-button>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
index 07badbf..791f949 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -81,6 +81,11 @@
},
patchNum: String,
showActions: Boolean,
+ _commentCollapsed: {
+ type: Boolean,
+ value: true,
+ observer: '_toggleCollapseClass',
+ },
projectConfig: Object,
_xhrPromise: Object, // Used for testing.
@@ -96,10 +101,20 @@
'_loadLocalDraft(changeNum, patchNum, comment)',
],
+ attached: function() {
+ if (this.editing) {
+ this._commentCollapsed = false;
+ }
+ },
+
detached: function() {
this.cancelDebouncer('fire-update');
},
+ _computeShowHideText: function(collapsed) {
+ return collapsed ? '◀' : '▼';
+ },
+
save: function() {
this.comment.message = this._messageText;
this.disabled = true;
@@ -210,6 +225,18 @@
}
},
+ _handleToggleCollapsed: function() {
+ this._commentCollapsed = !this._commentCollapsed;
+ },
+
+ _toggleCollapseClass: function(_commentCollapsed) {
+ if (_commentCollapsed) {
+ this.$.container.classList.add('collapsed');
+ } else {
+ this.$.container.classList.remove('collapsed');
+ }
+ },
+
_commentMessageChanged: function(message) {
this._messageText = message || '';
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
index bddc3ab..b05746e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -39,6 +39,12 @@
</test-fixture>
<script>
+
+ function isVisible(el) {
+ assert.ok(el);
+ return getComputedStyle(el).getPropertyValue('display') !== 'none';
+ }
+
suite('gr-diff-comment tests', function() {
var element;
setup(function() {
@@ -58,6 +64,32 @@
};
});
+ test('collapsible comments', function() {
+ // When a comment (not draft) is loaded, it should be collapsed
+ assert.isFalse(isVisible(element.$$('gr-linked-text')),
+ 'gr-linked-text is not visible');
+ assert.isFalse(isVisible(element.$$('.actions')),
+ 'actions are not visible');
+ assert.isFalse(isVisible(element.$$('iron-autogrow-textarea')),
+ 'textarea is not visible');
+
+ // The header middle content is only visible when comments are collapsed.
+ // It shows the message in a condensed way, and limits to a single line.
+ assert.isTrue(isVisible(element.$$('.collapsedContent')),
+ 'header middle content is visible');
+
+ // When the header row is clicked, the comment should expand
+ MockInteractions.tap(element.$.header);
+ assert.isTrue(isVisible(element.$$('gr-linked-text')),
+ 'gr-linked-text is visible');
+ assert.isTrue(isVisible(element.$$('.actions')),
+ 'actions are visible');
+ assert.isFalse(isVisible(element.$$('iron-autogrow-textarea')),
+ 'textarea is not visible');
+ assert.isFalse(isVisible(element.$$('.collapsedContent')),
+ 'header middle content is not visible');
+ });
+
test('proper event fires on reply', function(done) {
element.addEventListener('reply', function(e) {
assert.ok(e.detail.comment);
@@ -135,11 +167,6 @@
};
});
- function isVisible(el) {
- assert.ok(el);
- return getComputedStyle(el).getPropertyValue('display') != 'none';
- }
-
test('button visibility states', function() {
element.showActions = false;
assert.isTrue(element.$$('.actions').hasAttribute('hidden'));
@@ -181,6 +208,67 @@
assert.isTrue(isVisible(element.$$('.cancel')), 'cancel is visible');
});
+ test('collapsible drafts', function() {
+ element.addEventListener('reply', function(e) {
+ assert.ok(e.detail.comment);
+ done();
+ });
+ assert.isFalse(isVisible(element.$$('gr-linked-text')),
+ 'gr-linked-text is not visible');
+ assert.isFalse(isVisible(element.$$('.actions')),
+ 'actions are not visible');
+ assert.isFalse(isVisible(element.$$('iron-autogrow-textarea')),
+ 'textarea is not visible');
+ assert.isTrue(isVisible(element.$$('.collapsedContent')),
+ 'header middle content is visible');
+
+ MockInteractions.tap(element.$.header);
+ assert.isTrue(isVisible(element.$$('gr-linked-text')),
+ 'gr-linked-text is visible');
+ assert.isTrue(isVisible(element.$$('.actions')),
+ 'actions are visible');
+ assert.isFalse(isVisible(element.$$('iron-autogrow-textarea')),
+ 'textarea is not visible');
+ assert.isFalse(isVisible(element.$$('.collapsedContent')),
+ 'header middle content is is not visible');
+
+ // When the edit button is pressed, should still see the actions
+ // and also textarea
+ MockInteractions.tap(element.$$('.edit'));
+ assert.isFalse(isVisible(element.$$('gr-linked-text')),
+ 'gr-linked-text is not visible');
+ assert.isTrue(isVisible(element.$$('.actions')),
+ 'actions are visible');
+ assert.isTrue(isVisible(element.$$('iron-autogrow-textarea')),
+ 'textarea is visible');
+ assert.isFalse(isVisible(element.$$('.collapsedContent')),
+ 'header middle content is not visible');
+
+ // When toggle again, everything should be hidden except for textarea
+ // and header middle content should be visible
+ MockInteractions.tap(element.$.header);
+ assert.isFalse(isVisible(element.$$('gr-linked-text')),
+ 'gr-linked-text is not visible');
+ assert.isFalse(isVisible(element.$$('.actions')),
+ 'actions are not visible');
+ assert.isFalse(isVisible(element.$$('iron-autogrow-textarea')),
+ 'textarea is not visible');
+ assert.isTrue(isVisible(element.$$('.collapsedContent')),
+ 'header middle content is visible');
+
+ // When toggle again, textarea should remain open in the state it was
+ // before
+ MockInteractions.tap(element.$.header);
+ assert.isFalse(isVisible(element.$$('gr-linked-text')),
+ 'gr-linked-text is not visible');
+ assert.isTrue(isVisible(element.$$('.actions')),
+ 'actions are visible');
+ assert.isTrue(isVisible(element.$$('iron-autogrow-textarea')),
+ 'textarea is visible');
+ assert.isFalse(isVisible(element.$$('.collapsedContent')),
+ 'header middle content is not visible');
+ });
+
test('draft creation/cancelation', function(done) {
assert.isFalse(element.editing);
MockInteractions.tap(element.$$('.edit'));
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
index 2dd4c91..f91c8ef 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
@@ -86,6 +86,7 @@
},
detached: function() {
+ this.cancel();
this.unlisten(window, 'scroll', '_handleWindowScroll');
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html
index 4f8c532..c89d9b5 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html
@@ -591,5 +591,12 @@
});
});
});
+
+ test('detaching cancels', function() {
+ element = fixture('basic');
+ sandbox.stub(element, 'cancel');
+ element.detached();
+ assert(element.cancel.called);
+ });
});
</script>
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 dc818e3..1a6759b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -34,8 +34,9 @@
properties: {
changeNum: String,
- hidden: {
+ expanded: {
type: Boolean,
+ value: true,
observer: '_handleShowDiff',
},
patchRange: Object,
@@ -92,14 +93,7 @@
},
ready: function() {
- // Reload only if the diff is not hidden and params are supplied.
- if (this.changeNum && this.patchRange && this.path && !this.hidden) {
- this.reload();
- }
- },
-
- _handleShowDiff: function(hidden) {
- if (!hidden) {
+ if (this._canRender()) {
this.reload();
}
},
@@ -126,7 +120,7 @@
},
getCursorStops: function() {
- if (this.hidden) {
+ if (!this.expanded) {
return [];
}
@@ -159,6 +153,16 @@
this.toggleClass('no-left');
},
+ _handleShowDiff: function() {
+ if (this._canRender()) {
+ this.reload();
+ }
+ },
+
+ _canRender: function() {
+ return this.changeNum && this.patchRange && this.path && this.expanded;
+ },
+
_getCommentThreads: function() {
return Polymer.dom(this.root).querySelectorAll('gr-diff-comment-thread');
},
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 2f0b6bd..b9b49c0 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
@@ -468,15 +468,20 @@
assert.equal(drafts[0].id, id);
});
- test('_handleShowDiff reloads when hidden is made false',
+ test('_handleShowDiff reloads when expanded is made true',
function(done) {
+ element.expanded = false;
+ element.changeNum = element._comments.meta.changeNum;
+ element.patchRange = element._comments.meta.patchRange;
+ element.path = element._comments.meta.path;
+
var stub = sinon.stub(element, 'reload', function() {
assert.isTrue(stub.called);
stub.restore();
done();
});
var spy = sinon.spy(element, '_handleShowDiff');
- element.set('hidden', false);
+ element.set('expanded', true);
});
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.html b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.html
index d3585ef..8d89692 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.html
+++ b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.html
@@ -36,7 +36,8 @@
<span>
<a href$="[[_computeOwnerLink(account)]]">
<gr-account-label account="[[account]]"
- avatar-image-size="[[avatarImageSize]]"></gr-account-label>
+ avatar-image-size="[[avatarImageSize]]"
+ show-email="[[_computeShowEmail(account)]]"></gr-account-label>
</a>
</span>
</template>
diff --git a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.js b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.js
index 058b27d..0c2ad0b 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.js
+++ b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link.js
@@ -30,5 +30,9 @@
var accountID = account.email || account._account_id;
return '/q/owner:' + encodeURIComponent(accountID) + '+status:open';
},
+
+ _computeShowEmail: function(account) {
+ return !!(account && !account.name);
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_test.html b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_test.html
index 2b5b831..1a84d15 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_test.html
@@ -48,6 +48,10 @@
assert.equal(element._computeOwnerLink({_account_id: 42}),
'/q/owner:42+status:open');
+
+ assert.equal(element._computeShowEmail({name: 'asd'}), false);
+
+ assert.equal(element._computeShowEmail({}), true);
});
});
