Append approval info to every comment-added stream event and hook
This change is driven by the RFC proposal[1] to show label
transitions more explicitly in Gerrit. Stream events only
provides label information on review events that contain
a vote transition. When a user submits a review with only a
reply message the approvals information is not added to the
stream event and hooks. This change will make Gerrit append
approvals info to every comment-added event including the
ones that do not contain vote changes.
Since labels are shown for every event we will add an 'oldValue'
property to the approvalAttribute to distinguish when a vote
transition has occurred. The 'oldValue' attribute contains
the previous vote score. This attribute will only appear when
there is a vote transition.
The comment-added stream event will look like this:
"approvals":[{"type":"Code-Review","description":"Code-Review",
"value":"-1", "oldValue":"0"}, {"type":"Verified",
"description":"Verified","value":"0"}],
"comment":"Patch Set 1: Code-Review+1\n\nMy Message" ...
This change will also a '--$LabelName-oldValue' parameter to
corresponding comment-added change hook:
hook[comment-added] output:
--change I2cd8327360ff89338a4e7f0591bf0c037e9aa5db
--is-draft false --change-url http://localhost:8080/201
--change-owner John Smith (jsmith@gmail.com) --project okc
--branch master --author John Smith (jsmith@gmail.com)
--commit 55079187f7ce1c461de804c13f2dd71d64a85280
--comment Patch Set 1: Code-Review-1 --Code-Review -1
--Code-Review-oldValue 0 --Verified 0
[1] https://groups.google.com/d/msg/repo-discuss/soqmGjRpl-4/IIfAF4jbCQAJ
Bug: Issue 3220
Change-Id: Ibc1c64d70e790c7b507db79931f31fd77f25e276
diff --git a/Documentation/config-hooks.txt b/Documentation/config-hooks.txt
index 3068cc5..5b1f5e3 100644
--- a/Documentation/config-hooks.txt
+++ b/Documentation/config-hooks.txt
@@ -67,7 +67,7 @@
This is called whenever a comment is added to a change.
====
- comment-added --change <change id> --is-draft <boolean> --change-url <change url> --change-owner <change owner> --project <project name> --branch <branch> --topic <topic> --author <comment author> --commit <commit> --comment <comment> [--<approval category id> <score> --<approval category id> <score> ...]
+ comment-added --change <change id> --is-draft <boolean> --change-url <change url> --change-owner <change owner> --project <project name> --branch <branch> --topic <topic> --author <comment author> --commit <commit> --comment <comment> [--<approval category id> <score> --<approval category id> <score> --<approval category id>-oldValue <score> ...]
====
=== change-merged
diff --git a/Documentation/json.txt b/Documentation/json.txt
index 32fa472..db22212 100644
--- a/Documentation/json.txt
+++ b/Documentation/json.txt
@@ -140,6 +140,8 @@
value:: Value assigned by the approval, usually a numerical score.
+oldValue:: The previous approval score, only present if the value changed as a result of this event.
+
grantedOn:: Time in seconds since the UNIX epoch when this approval
was added or last updated.
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java
new file mode 100644
index 0000000..3c99dc3
--- /dev/null
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/event/CommentAddedEventIT.java
@@ -0,0 +1,253 @@
+// 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.acceptance.server.event;
+
+import static com.google.common.truth.Truth.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 org.junit.Assert.fail;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.common.UserScopedEventListener;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.LabelInfo;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.registration.RegistrationHandle;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.data.ApprovalAttribute;
+import com.google.gerrit.server.events.CommentAddedEvent;
+import com.google.gerrit.server.events.Event;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.gerrit.server.project.Util;
+import com.google.inject.Inject;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+@NoHttpd
+public class CommentAddedEventIT extends AbstractDaemonTest {
+
+ @Inject
+ private IdentifiedUser.GenericFactory factory;
+
+ @Inject
+ private DynamicSet<UserScopedEventListener> source;
+
+ private final LabelType label = category("CustomLabel",
+ value(1, "Positive"),
+ value(0, "No score"),
+ value(-1, "Negative"));
+
+ private final LabelType pLabel = category("CustomLabel2",
+ value(1, "Positive"),
+ value(0, "No score"));
+
+ private RegistrationHandle eventListenerRegistration;
+ private CommentAddedEvent lastCommentAddedEvent;
+
+ @Before
+ public void setUp() throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ AccountGroup.UUID anonymousUsers =
+ SystemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID();
+ Util.allow(cfg, Permission.forLabel(label.getName()), -1, 1, anonymousUsers,
+ "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(pLabel.getName()), 0, 1, anonymousUsers,
+ "refs/heads/*");
+ saveProjectConfig(project, cfg);
+
+ eventListenerRegistration = source.add(new UserScopedEventListener() {
+ @Override
+ public void onEvent(Event event) {
+ if (event instanceof CommentAddedEvent) {
+ lastCommentAddedEvent = (CommentAddedEvent) event;
+ }
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ return factory.create(user.id);
+ }
+ });
+ }
+
+ @After
+ public void cleanup() {
+ eventListenerRegistration.remove();
+ }
+
+ private void saveLabelConfig() throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ cfg.getLabelSections().put(label.getName(), label);
+ cfg.getLabelSections().put(pLabel.getName(), pLabel);
+ saveProjectConfig(project, cfg);
+ }
+
+ @Test
+ public void newChangeWithVote() throws Exception {
+ saveLabelConfig();
+
+ // push a new change with -1 vote
+ PushOneCommit.Result r = createChange();
+ ReviewInput reviewInput = new ReviewInput().label(
+ label.getName(), (short)-1);
+ revision(r).review(reviewInput);
+ String newVote = lastCommentAddedEvent.approvals.get()[0].value;
+ String oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
+ assertThat(oldVote).isEqualTo("0");
+ assertThat(newVote).isEqualTo("-1");
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1: %s-1", label.getName()));
+ }
+
+ @Test
+ public void newPatchSetWithVote() throws Exception {
+ saveLabelConfig();
+
+ // push a new change
+ PushOneCommit.Result r = createChange();
+ ReviewInput reviewInput = new ReviewInput().message(label.getName());
+ revision(r).review(reviewInput);
+
+ // push a new revision with +1 vote
+ ChangeInfo c = get(r.getChangeId());
+ r = amendChange(c.changeId);
+ reviewInput = new ReviewInput().label(
+ label.getName(), (short)1);
+ revision(r).review(reviewInput);
+ String newVote = lastCommentAddedEvent.approvals.get()[0].value;
+ String oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
+ assertThat(oldVote).isEqualTo("0");
+ assertThat(newVote).isEqualTo("1");
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 2: %s+1", label.getName()));
+ }
+
+ @Test
+ public void reviewChange() throws Exception {
+ saveLabelConfig();
+
+ // push a change
+ PushOneCommit.Result r = createChange();
+
+ // review with message only, do not apply votes
+ ReviewInput reviewInput = new ReviewInput().message(label.getName());
+ revision(r).review(reviewInput);
+ // reply message only so votes are excluded from comment
+ assertThat(lastCommentAddedEvent.approvals.get()).isNull();
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1:\n\n%s", label.getName()));
+
+ // transition from un-voted to -1 vote
+ reviewInput = new ReviewInput().label(label.getName(), -1);
+ revision(r).review(reviewInput);
+ String newVote = lastCommentAddedEvent.approvals.get()[0].value;
+ String oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
+ assertThat(oldVote).isEqualTo("0");
+ assertThat(newVote).isEqualTo("-1");
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1: %s-1", label.getName()));
+
+ // transition vote from -1 to 0
+ reviewInput = new ReviewInput().label(label.getName(), 0);
+ revision(r).review(reviewInput);
+ newVote = lastCommentAddedEvent.approvals.get()[0].value;
+ oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
+ assertThat(oldVote).isEqualTo("-1");
+ assertThat(newVote).isEqualTo("0");
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1: -%s", label.getName()));
+
+ // transition vote from 0 to 1
+ reviewInput = new ReviewInput().label(label.getName(), 1);
+ revision(r).review(reviewInput);
+ newVote = lastCommentAddedEvent.approvals.get()[0].value;
+ oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
+ assertThat(oldVote).isEqualTo("0");
+ assertThat(newVote).isEqualTo("1");
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1: %s+1", label.getName()));
+
+ // transition vote from 1 to -1
+ reviewInput = new ReviewInput().label(label.getName(), -1);
+ revision(r).review(reviewInput);
+ newVote = lastCommentAddedEvent.approvals.get()[0].value;
+ oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
+ assertThat(oldVote).isEqualTo("1");
+ assertThat(newVote).isEqualTo("-1");
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1: %s-1", label.getName()));
+
+ // review with message only, do not apply votes
+ reviewInput = new ReviewInput().message(label.getName());
+ revision(r).review(reviewInput);
+ newVote = lastCommentAddedEvent.approvals.get()[0].value;
+ oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
+ assertThat(oldVote).isEqualTo(null); // no vote change so not included
+ assertThat(newVote).isEqualTo("-1");
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1:\n\n%s", label.getName()));
+ }
+
+ @Test
+ public void reviewChange_MultipleVotes() throws Exception {
+ saveLabelConfig();
+ PushOneCommit.Result r = createChange();
+ ReviewInput reviewInput = new ReviewInput().label(label.getName(), -1);
+ reviewInput.message = label.getName();
+ revision(r).review(reviewInput);
+
+ ChangeInfo c = get(r.getChangeId());
+ LabelInfo q = c.labels.get(label.getName());
+ assertThat(q.all).hasSize(1);
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1: %s-1\n\n%s",
+ label.getName(), label.getName()));
+
+ reviewInput = new ReviewInput().label(pLabel.getName(), 1);
+ reviewInput.message = pLabel.getName();
+ revision(r).review(reviewInput);
+
+ c = get(r.getChangeId());
+ q = c.labels.get(label.getName());
+ assertThat(q.all).hasSize(1);
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ String.format("Patch Set 1: %s+1\n\n%s",
+ pLabel.getName(), pLabel.getName()));
+
+ assertThat(lastCommentAddedEvent.approvals.get()).hasLength(2);
+ for (ApprovalAttribute approval : lastCommentAddedEvent.approvals.get()) {
+ if (approval.type.equals(label.getName())) {
+ assertThat(approval.value).isEqualTo("-1");
+ assertThat(approval.oldValue).isNull();
+ } else if (approval.type.equals(pLabel.getName())) {
+ assertThat(approval.value).isEqualTo("1");
+ assertThat(approval.oldValue).isEqualTo("0");
+ } else {
+ fail("Unexpected label: " + approval.type);
+ }
+ }
+ }
+}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
index 357f268..bdefb00 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
@@ -22,23 +22,38 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.common.UserScopedEventListener;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelInfo;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.events.CommentAddedEvent;
+import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.Util;
+import com.google.inject.Inject;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@NoHttpd
public class CustomLabelIT extends AbstractDaemonTest {
+ @Inject
+ private IdentifiedUser.GenericFactory factory;
+
+ @Inject
+ private DynamicSet<UserScopedEventListener> source;
+
private final LabelType label = category("CustomLabel",
value(1, "Positive"),
value(0, "No score"),
@@ -48,6 +63,9 @@
value(1, "Positive"),
value(0, "No score"));
+ private CommentAddedEvent lastCommentAddedEvent;
+ private RegistrationHandle eventListenerRegistration;
+
@Before
public void setUp() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
@@ -58,6 +76,26 @@
Util.allow(cfg, Permission.forLabel(P.getName()), 0, 1, anonymousUsers,
"refs/heads/*");
saveProjectConfig(project, cfg);
+
+ eventListenerRegistration = source.add(new UserScopedEventListener() {
+ @Override
+ public void onEvent(Event event) {
+ if (event instanceof CommentAddedEvent) {
+ lastCommentAddedEvent = (CommentAddedEvent) event;
+ }
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ return factory.create(user.id);
+ }
+ });
+ }
+
+ @After
+ public void cleanup() {
+ eventListenerRegistration.remove();
+ db.close();
}
@Test
@@ -124,13 +162,18 @@
.id(r.getChangeId())
.addReviewer(in);
- revision(r).review(new ReviewInput().label(P.getName(), 0));
+ ReviewInput input = new ReviewInput().label(P.getName(), 0);
+ input.message = "foo";
+
+ revision(r).review(input);
ChangeInfo c = get(r.getChangeId());
LabelInfo q = c.labels.get(P.getName());
assertThat(q.all).hasSize(2);
assertThat(q.disliked).isNull();
assertThat(q.rejected).isNull();
assertThat(q.blocking).isNull();
+ assertThat(lastCommentAddedEvent.comment).isEqualTo(
+ "Patch Set 1:\n\n" + input.message);
}
@Test
diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java
index 3daeaaf..be5d1c8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java
@@ -423,7 +423,8 @@
@Override
public void doCommentAddedHook(final Change change, Account account,
PatchSet patchSet, String comment, final Map<String, Short> approvals,
- ReviewDb db) throws OrmException {
+ final Map<String, Short> oldApprovals, ReviewDb db)
+ throws OrmException {
CommentAddedEvent event = new CommentAddedEvent(change);
Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
@@ -441,7 +442,8 @@
ApprovalAttribute[] r = new ApprovalAttribute[approvals.size()];
int i = 0;
for (Map.Entry<String, Short> approval : approvals.entrySet()) {
- r[i++] = getApprovalAttribute(labelTypes, approval);
+ r[i++] = getApprovalAttribute(labelTypes, approval,
+ oldApprovals);
}
return r;
}
@@ -473,11 +475,17 @@
change.getProject()).getLabelTypes();
for (Map.Entry<String, Short> approval : approvals.entrySet()) {
LabelType lt = labelTypes.byLabel(approval.getKey());
- if (lt != null) {
+ if (lt != null && approval.getValue() != null) {
addArg(args, "--" + lt.getName(), Short.toString(approval.getValue()));
+ if (oldApprovals != null && !oldApprovals.isEmpty()) {
+ Short oldValue = oldApprovals.get(approval.getKey());
+ if (oldValue != null) {
+ addArg(args, "--" + lt.getName() + "-oldValue",
+ Short.toString(oldValue));
+ }
+ }
}
}
-
runHook(change.getProject(), commentAddedHook, args);
}
@@ -856,18 +864,29 @@
/**
* Create an ApprovalAttribute for the given approval suitable for serialization to JSON.
+ * @param labelTypes
* @param approval
+ * @param oldApprovals
* @return object suitable for serialization to JSON
*/
private ApprovalAttribute getApprovalAttribute(LabelTypes labelTypes,
- Entry<String, Short> approval) {
+ Entry<String, Short> approval,
+ Map<String, Short> oldApprovals) {
ApprovalAttribute a = new ApprovalAttribute();
a.type = approval.getKey();
+
+ if (oldApprovals != null && !oldApprovals.isEmpty()) {
+ if (oldApprovals.get(approval.getKey()) != null) {
+ a.oldValue = Short.toString(oldApprovals.get(approval.getKey()));
+ }
+ }
LabelType lt = labelTypes.byLabel(approval.getKey());
if (lt != null) {
a.description = lt.getName();
}
- a.value = Short.toString(approval.getValue());
+ if (approval.getValue() != null) {
+ a.value = Short.toString(approval.getValue());
+ }
return a;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java
index 8214a1c..ee1de65 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java
@@ -60,13 +60,15 @@
* @param account The gerrit user who added the comment.
* @param patchSet The patchset this comment is related to.
* @param comment The comment given.
- * @param approvals Map of label IDs to scores.
+ * @param approvals Map of label IDs to scores
+ * @param oldApprovals Map of label IDs to old approval scores
* @param db The review database.
* @throws OrmException
*/
void doCommentAddedHook(Change change, Account account,
PatchSet patchSet, String comment,
- Map<String, Short> approvals, ReviewDb db)
+ Map<String, Short> approvals, Map<String, Short> oldApprovals,
+ ReviewDb db)
throws OrmException;
/**
diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java b/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java
index 832b3d6..bec0b7e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java
@@ -61,7 +61,8 @@
@Override
public void doCommentAddedHook(Change change, Account account,
PatchSet patchSet, String comment,
- Map<String, Short> approvals, ReviewDb db) {
+ Map<String, Short> approvals, Map<String, Short> oldApprovals,
+ ReviewDb db) {
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
index 562d881..c8905ee 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -21,6 +21,7 @@
import com.google.common.base.MoreObjects;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.FooterConstants;
+import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.extensions.api.changes.ReviewInput.NotifyHandling;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -49,6 +50,7 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.RefControl;
@@ -68,6 +70,7 @@
import java.io.IOException;
import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -82,6 +85,7 @@
LoggerFactory.getLogger(ChangeInserter.class);
private final ProjectControl.GenericFactory projectControlFactory;
+ private final ChangeControl.GenericFactory changeControlFactory;
private final PatchSetInfoFactory patchSetInfoFactory;
private final PatchSetUtil psUtil;
private final ChangeHooks hooks;
@@ -121,6 +125,7 @@
@Inject
ChangeInserter(ProjectControl.GenericFactory projectControlFactory,
+ ChangeControl.GenericFactory changeControlFactory,
PatchSetInfoFactory patchSetInfoFactory,
PatchSetUtil psUtil,
ChangeHooks hooks,
@@ -133,6 +138,7 @@
@Assisted RevCommit commit,
@Assisted String refName) {
this.projectControlFactory = projectControlFactory;
+ this.changeControlFactory = changeControlFactory;
this.patchSetInfoFactory = patchSetInfoFactory;
this.psUtil = psUtil;
this.hooks = hooks;
@@ -355,7 +361,7 @@
}
@Override
- public void postUpdate(Context ctx) throws OrmException {
+ public void postUpdate(Context ctx) throws OrmException, NoSuchChangeException {
if (sendMail) {
Runnable sender = new Runnable() {
@Override
@@ -386,13 +392,33 @@
}
}
+ /** For labels that are not set in this operation, show the "current" value
+ * of 0, and no oldValue as the value was not modified by this operation.
+ * For labels that are set in this operation, the value was modified, so
+ * show a transition from an oldValue of 0 to the new value.
+ **/
if (runHooks) {
ReviewDb db = ctx.getDb();
hooks.doPatchsetCreatedHook(change, patchSet, db);
if (approvals != null && !approvals.isEmpty()) {
+ ChangeControl changeControl = changeControlFactory.controlFor(
+ db, change, ctx.getUser());
+ List<LabelType> labels = changeControl.getLabelTypes().getLabelTypes();
+ Map<String, Short> allApprovals = new HashMap<>();
+ Map<String, Short> oldApprovals = new HashMap<>();
+ for (LabelType lt : labels){
+ allApprovals.put(lt.getName(), (short) 0);
+ oldApprovals.put(lt.getName(), null);
+ }
+ for (Map.Entry<String, Short> entry : approvals.entrySet()) {
+ if (entry.getValue() != 0) {
+ allApprovals.put(entry.getKey(), entry.getValue());
+ oldApprovals.put(entry.getKey(), (short) 0);
+ }
+ }
hooks.doCommentAddedHook(change,
ctx.getUser().asIdentifiedUser().getAccount(), patchSet, null,
- approvals, db);
+ allApprovals, oldApprovals, db);
}
}
}
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 c2c67eb..fbfcc87 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
@@ -21,6 +21,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.auto.value.AutoValue;
+import com.google.common.base.MoreObjects;
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
@@ -82,6 +83,7 @@
import java.sql.Timestamp;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
@@ -350,7 +352,8 @@
private ChangeMessage message;
private List<PatchLineComment> comments = new ArrayList<>();
private List<String> labelDelta = new ArrayList<>();
- private Map<String, Short> categories = new HashMap<>();
+ private Map<String, Short> approvals = new HashMap<>();
+ private Map<String, Short> oldApprovals = new HashMap<>();
private Op(PatchSet.Id psId, ReviewInput in) {
this.psId = psId;
@@ -393,7 +396,7 @@
}
try {
hooks.doCommentAddedHook(notes.getChange(), user.getAccount(), ps,
- message.getMessage(), categories, ctx.getDb());
+ message.getMessage(), approvals, oldApprovals, ctx.getDb());
} catch (OrmException e) {
log.warn("ChangeHook.doCommentAddedHook delivery failed", e);
}
@@ -514,43 +517,77 @@
return drafts;
}
+ private Map<String, Short> approvalsByKey(
+ Collection<PatchSetApproval> patchsetApprovals) {
+ Map<String, Short> labels = new HashMap<>();
+ for (PatchSetApproval psa : patchsetApprovals) {
+ labels.put(psa.getLabel(), psa.getValue());
+ }
+ return labels;
+ }
+
private boolean updateLabels(ChangeContext ctx)
throws OrmException, ResourceConflictException {
- Map<String, Short> labels = in.labels;
- if (labels == null) {
- labels = Collections.emptyMap();
- }
+ Map<String, Short> inLabels = MoreObjects.firstNonNull(in.labels,
+ Collections.<String, Short> emptyMap());
List<PatchSetApproval> del = Lists.newArrayList();
List<PatchSetApproval> ups = Lists.newArrayList();
Map<String, PatchSetApproval> current = scanLabels(ctx, del);
+ // get all approvals in cases of quick approve vote
+ Map<String, Short> allApprovals = Collections.emptyMap();
+ if (current != null) {
+ allApprovals = approvalsByKey(current.values());
+ }
+ allApprovals.putAll(inLabels);
+
+ // get previous label votes
+ Map<String, Short> currentLabels = new HashMap<>();
+ for (Map.Entry<String, PatchSetApproval> ent : current.entrySet()) {
+ currentLabels.put(ent.getValue().getLabel(), ent.getValue().getValue());
+ }
+ Map<String, Short> previous = new HashMap<>();
+ for (Map.Entry<String, Short> ent : allApprovals.entrySet()) {
+ if (!currentLabels.containsKey(ent.getKey())) {
+ previous.put(ent.getKey(), (short)0);
+ } else {
+ previous.put(ent.getKey(), currentLabels.get(ent.getKey()));
+ }
+ }
+
ChangeUpdate update = ctx.getUpdate(psId);
LabelTypes labelTypes = ctx.getControl().getLabelTypes();
- for (Map.Entry<String, Short> ent : labels.entrySet()) {
+ for (Map.Entry<String, Short> ent : allApprovals.entrySet()) {
String name = ent.getKey();
LabelType lt = checkNotNull(labelTypes.byLabel(name), name);
PatchSetApproval c = current.remove(lt.getName());
String normName = lt.getName();
+ approvals.put(normName, (short) 0);
if (ent.getValue() == null || ent.getValue() == 0) {
// User requested delete of this label.
+ oldApprovals.put(normName, null);
if (c != null) {
if (c.getValue() != 0) {
addLabelDelta(normName, (short) 0);
+ oldApprovals.put(normName, previous.get(normName));
}
del.add(c);
- update.putApproval(ent.getKey(), (short) 0);
+ update.putApproval(normName, (short) 0);
}
} else if (c != null && c.getValue() != ent.getValue()) {
c.setValue(ent.getValue());
c.setGranted(ctx.getWhen());
ups.add(c);
addLabelDelta(normName, c.getValue());
- categories.put(normName, c.getValue());
- update.putApproval(ent.getKey(), ent.getValue());
+ oldApprovals.put(normName, previous.get(normName));
+ approvals.put(normName, c.getValue());
+ update.putApproval(normName, ent.getValue());
} else if (c != null && c.getValue() == ent.getValue()) {
current.put(normName, c);
+ oldApprovals.put(normName, null);
+ approvals.put(normName, c.getValue());
} else if (c == null) {
c = new PatchSetApproval(new PatchSetApproval.Key(
psId,
@@ -560,9 +597,10 @@
c.setGranted(ctx.getWhen());
ups.add(c);
addLabelDelta(normName, c.getValue());
- categories.put(normName, c.getValue());
+ oldApprovals.put(normName, previous.get(normName));
+ approvals.put(normName, c.getValue());
update.putReviewer(user.getAccountId(), REVIEWER);
- update.putApproval(ent.getKey(), ent.getValue());
+ update.putApproval(normName, ent.getValue());
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/data/ApprovalAttribute.java b/gerrit-server/src/main/java/com/google/gerrit/server/data/ApprovalAttribute.java
index 3059be3..90b6fc4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/data/ApprovalAttribute.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/data/ApprovalAttribute.java
@@ -18,7 +18,7 @@
public String type;
public String description;
public String value;
-
+ public String oldValue;
public Long grantedOn;
public AccountAttribute by;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
index 0f894ce..50d02df 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
@@ -620,6 +620,7 @@
a.value = Short.toString(approval.getValue());
a.by = asAccountAttribute(approval.getAccountId());
a.grantedOn = approval.getGranted().getTime() / 1000L;
+ a.oldValue = null;
LabelType lt = labelTypes.byLabel(approval.getLabelId());
if (lt != null) {
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 3de1fc1..ba9795a 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
@@ -45,6 +45,8 @@
import com.google.gerrit.server.mail.MergedSender;
import com.google.gerrit.server.mail.ReplacePatchSetSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.util.LabelVote;
@@ -93,6 +95,7 @@
private final PatchSetUtil psUtil;
private final ChangeData.Factory changeDataFactory;
+ private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeKindCache changeKindCache;
private final ChangeMessagesUtil cmUtil;
private final ChangeHooks hooks;
@@ -127,6 +130,7 @@
@AssistedInject
ReplaceOp(PatchSetUtil psUtil,
ChangeData.Factory changeDataFactory,
+ ChangeControl.GenericFactory changeControlFactory,
ChangeKindCache changeKindCache,
ChangeMessagesUtil cmUtil,
ChangeHooks hooks,
@@ -149,6 +153,7 @@
@Assisted @Nullable PushCertificate pushCertificate) {
this.psUtil = psUtil;
this.changeDataFactory = changeDataFactory;
+ this.changeControlFactory = changeControlFactory;
this.changeKindCache = changeKindCache;
this.cmUtil = cmUtil;
this.hooks = hooks;
@@ -391,12 +396,45 @@
hooks.doChangeMergedHook(change, account, newPatchSet, ctx.getDb(),
commit.getName());
}
- if (!approvals.isEmpty()) {
- hooks.doCommentAddedHook(change, account, newPatchSet, null, approvals,
- ctx.getDb());
+ try{
+ runHook(ctx);
+ } catch (Exception e) {
+ log.warn("ChangeHook.doCommentAddedHook delivery failed", e);
}
}
+ private void runHook(final Context ctx) throws NoSuchChangeException,
+ OrmException {
+ if (approvals.isEmpty()) {
+ return;
+ }
+
+ /** For labels that are not set in this operation, show the "current" value
+ * of 0, and no oldValue as the value was not modified by this operation.
+ * For labels that are set in this operation, the value was modified, so
+ * show a transition from an oldValue of 0 to the new value.
+ **/
+ ChangeControl changeControl = changeControlFactory.controlFor(
+ ctx.getDb(), change, ctx.getUser());
+ List<LabelType> labels = changeControl.getLabelTypes().getLabelTypes();
+ Map<String, Short> allApprovals = new HashMap<>();
+ Map<String, Short> oldApprovals = new HashMap<>();
+ for (LabelType lt : labels){
+ allApprovals.put(lt.getName(), (short) 0);
+ oldApprovals.put(lt.getName(), null);
+ }
+ for (Map.Entry<String, Short> entry : approvals.entrySet()) {
+ if (entry.getValue() != 0) {
+ allApprovals.put(entry.getKey(), entry.getValue());
+ oldApprovals.put(entry.getKey(), (short) 0);
+ }
+ }
+
+ hooks.doCommentAddedHook(change,
+ ctx.getUser().asIdentifiedUser().getAccount(), newPatchSet, null,
+ allApprovals, oldApprovals, ctx.getDb());
+ }
+
private void sendMergedEmail(final Context ctx) {
sendEmailExecutor.submit(requestScopePropagator.wrap(new Runnable() {
@Override
