diff --git a/Documentation/config-hooks.txt b/Documentation/config-hooks.txt index 3068cc5daf..5b1f5e3fe5 100644 --- a/Documentation/config-hooks.txt +++ b/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 --is-draft --change-url --change-owner --project --branch --topic --author --commit --comment [-- -- ...] + comment-added --change --is-draft --change-url --change-owner --project --branch --topic --author --commit --comment [-- -- ---oldValue ...] ==== === change-merged diff --git a/Documentation/json.txt b/Documentation/json.txt index 32fa47216c..db22212df3 100644 --- a/Documentation/json.txt +++ b/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. 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 0000000000..3c99dc34ab --- /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 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 357f268364..bdefb0045e 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 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 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 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 3daeaaffb0..be5d1c8579 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 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener, @Override public void doCommentAddedHook(final Change change, Account account, PatchSet patchSet, String comment, final Map approvals, - ReviewDb db) throws OrmException { + final Map oldApprovals, ReviewDb db) + throws OrmException { CommentAddedEvent event = new CommentAddedEvent(change); Supplier 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 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 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 approval) { + Entry approval, + Map 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 8214a1c118..ee1de65fc5 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 @@ 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 approvals, ReviewDb db) + Map approvals, Map 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 832b3d64d4..bec0b7e514 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 @@ public final class DisabledChangeHooks implements ChangeHooks, EventDispatcher { @Override public void doCommentAddedHook(Change change, Account account, PatchSet patchSet, String comment, - Map approvals, ReviewDb db) { + Map approvals, Map 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 562d8814a7..c8905ee8ce 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 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 labels = changeControl.getLabelTypes().getLabelTypes(); + Map allApprovals = new HashMap<>(); + Map oldApprovals = new HashMap<>(); + for (LabelType lt : labels){ + allApprovals.put(lt.getName(), (short) 0); + oldApprovals.put(lt.getName(), null); + } + for (Map.Entry 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 c2c67ebbf9..fbfcc872b0 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 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 private ChangeMessage message; private List comments = new ArrayList<>(); private List labelDelta = new ArrayList<>(); - private Map categories = new HashMap<>(); + private Map approvals = new HashMap<>(); + private Map oldApprovals = new HashMap<>(); private Op(PatchSet.Id psId, ReviewInput in) { this.psId = psId; @@ -393,7 +396,7 @@ public class PostReview implements RestModifyView } 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 return drafts; } + private Map approvalsByKey( + Collection patchsetApprovals) { + Map labels = new HashMap<>(); + for (PatchSetApproval psa : patchsetApprovals) { + labels.put(psa.getLabel(), psa.getValue()); + } + return labels; + } + private boolean updateLabels(ChangeContext ctx) throws OrmException, ResourceConflictException { - Map labels = in.labels; - if (labels == null) { - labels = Collections.emptyMap(); - } + Map inLabels = MoreObjects.firstNonNull(in.labels, + Collections. emptyMap()); List del = Lists.newArrayList(); List ups = Lists.newArrayList(); Map current = scanLabels(ctx, del); + // get all approvals in cases of quick approve vote + Map allApprovals = Collections.emptyMap(); + if (current != null) { + allApprovals = approvalsByKey(current.values()); + } + allApprovals.putAll(inLabels); + + // get previous label votes + Map currentLabels = new HashMap<>(); + for (Map.Entry ent : current.entrySet()) { + currentLabels.put(ent.getValue().getLabel(), ent.getValue().getValue()); + } + Map previous = new HashMap<>(); + for (Map.Entry 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 ent : labels.entrySet()) { + for (Map.Entry 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 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 3059be3760..90b6fc4d3e 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 class ApprovalAttribute { 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 0f894ce0a8..50d02df6f3 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 @@ 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) { 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 3de1fc1553..ba9795a2ca 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.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 labels = changeControl.getLabelTypes().getLabelTypes(); + Map allApprovals = new HashMap<>(); + Map oldApprovals = new HashMap<>(); + for (LabelType lt : labels){ + allApprovals.put(lt.getName(), (short) 0); + oldApprovals.put(lt.getName(), null); + } + for (Map.Entry 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