Merge "Append approval info to every comment-added stream event and hook"

Documentation/config-hooks.txt

@@ -67,7 +67,7 @@ This is called whenever a draft change is published.
 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

Documentation/json.txt

@@ -140,6 +140,8 @@ description::  Human readable category of the approval.
 
 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.
 

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);
+      }
+    }
+  }
+}

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java

@@ -22,23 +22,38 @@ import static com.google.gerrit.server.project.Util.value;
 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 @@ public class CustomLabelIT extends AbstractDaemonTest {
       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 @@ public class CustomLabelIT extends AbstractDaemonTest {
     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 @@ public class CustomLabelIT extends AbstractDaemonTest {
         .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

gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java

@@ -423,7 +423,8 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener,
     @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 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener,
                 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 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener,
           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 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener,
 
     /**
      * 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();
       }
+      if (approval.getValue() != null) {
         a.value = Short.toString(approval.getValue());
+      }
       return a;
     }
 

gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java

@@ -60,13 +60,15 @@ public interface ChangeHooks {
    * @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;
 
   /**

gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java

@@ -61,7 +61,8 @@ public final class DisabledChangeHooks implements ChangeHooks, EventDispatcher {
   @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

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java

@@ -21,6 +21,7 @@ import static com.google.gerrit.reviewdb.client.Change.INITIAL_PATCH_SET_ID;
 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.mail.CreateChangeSender;
 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 org.slf4j.LoggerFactory;
 
 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 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
       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 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
 
   @Inject
   ChangeInserter(ProjectControl.GenericFactory projectControlFactory,
+      ChangeControl.GenericFactory changeControlFactory,
       PatchSetInfoFactory patchSetInfoFactory,
       PatchSetUtil psUtil,
       ChangeHooks hooks,
@@ -133,6 +138,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
       @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 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
   }
 
   @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 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
       }
     }
 
+    /** 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);
       }
     }
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -21,6 +21,7 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 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 org.slf4j.LoggerFactory;
 
 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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       }
       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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           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());
         }
       }
 

gerrit-server/src/main/java/com/google/gerrit/server/data/ApprovalAttribute.java

@@ -18,7 +18,7 @@ public class ApprovalAttribute {
     public String type;
     public String description;
     public String value;
-
+    public String oldValue;
     public Long grantedOn;
     public AccountAttribute by;
 }

gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java

@@ -620,6 +620,7 @@ public class EventFactory {
     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) {

gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java

@@ -45,6 +45,8 @@ import com.google.gerrit.server.mail.MailUtil.MailRecipients;
 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 @@ public class ReplaceOp extends BatchUpdate.Op {
 
   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 @@ public class ReplaceOp extends BatchUpdate.Op {
   @AssistedInject
   ReplaceOp(PatchSetUtil psUtil,
       ChangeData.Factory changeDataFactory,
+      ChangeControl.GenericFactory changeControlFactory,
       ChangeKindCache changeKindCache,
       ChangeMessagesUtil cmUtil,
       ChangeHooks hooks,
@@ -149,6 +153,7 @@ public class ReplaceOp extends BatchUpdate.Op {
       @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 @@ public class ReplaceOp extends BatchUpdate.Op {
       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