opendev/gerrit
Code Issues Proposed changes

Fix inconsistent notifications on vote removal

Browse Source 
Change Ie4f05fafb73a implemented a feature to allow direct removal
of a vote from the Gerrit UI.  Clicking reply on the Gerrit UI and
setting the vote to 0 is essentially the same thing as clicking
'X' on the vote.  The difference is that Gerrit will fire a
comment-added event on the reply however it will not fire
an event when users remove the vote by clicking the 'X'.
This is inconsistent behavior.  This change attempts to fix this
inconsistent behavior by also sending a comment-added event when
users click on the 'X' to remove a vote.

The vote removal appear as the following comment:
"Zaro Removed Code-Review-2 by Russell Westbrook <rwestbrook@localhost.com>"

Email preview: http://paste.openstack.org/show/485472

Bug: issue 3701
Change-Id: I279cb5b411d8c8e37243a5d3b734a8f002523d97
This commit is contained in:
Khai Do
2015-12-08 23:44:34 -08:00
parent cb4afa34a8
commit f6b479a533
6 changed files with 184 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
Documentation/config-mail.txt
View File

@@ -56,6 +56,12 @@ The `CommentFooter.vm` template will determine the contents of the footer
text that will be appended to emails related to a user submitting comments on
changes. See `ChangeSubject.vm`, `Comment.vm` and `ChangeFooter.vm`.
=== DeleteVote.vm
The `DeleteVote.vm` template will determine the contents of the email related
to removing votes on changes. It is a `ChangeEmail`: see `ChangeSubject.vm`
and `ChangeFooter.vm`.
=== Footer.vm
The `Footer.vm` template will determine the contents of the footer text

1
gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java
View File

@@ -105,6 +105,7 @@ public class SitePathInitializer {
extractMailExample("ChangeSubject.vm");
extractMailExample("Comment.vm");
extractMailExample("CommentFooter.vm");
extractMailExample("DeleteVote.vm");
extractMailExample("Footer.vm");
extractMailExample("Merged.vm");
extractMailExample("MergeFail.vm");

82
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
View File

@@ -14,7 +14,10 @@
package com.google.gerrit.server.change;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
@@ -30,9 +33,13 @@ import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.DeleteVote.Input;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.BatchUpdate.Context;
import com.google.gerrit.server.mail.DeleteVoteSender;
import com.google.gerrit.server.mail.ReplyToChangeSender;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
@@ -40,30 +47,46 @@ import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
@Singleton
public class DeleteVote implements RestModifyView<VoteResource, Input> {
private static final Logger log = LoggerFactory.getLogger(DeleteVote.class);
public static class Input {
}
private final Provider<ReviewDb> db;
private final BatchUpdate.Factory batchUpdateFactory;
private final ApprovalsUtil approvalsUtil;
private final PatchSetUtil psUtil;
private final ChangeMessagesUtil cmUtil;
private final IdentifiedUser.GenericFactory userFactory;
private final ChangeHooks hooks;
private final DeleteVoteSender.Factory deleteVoteSenderFactory;
@Inject
DeleteVote(Provider<ReviewDb> db,
BatchUpdate.Factory batchUpdateFactory,
ApprovalsUtil approvalsUtil,
PatchSetUtil psUtil,
ChangeMessagesUtil cmUtil,
IdentifiedUser.GenericFactory userFactory) {
IdentifiedUser.GenericFactory userFactory,
ChangeHooks hooks,
DeleteVoteSender.Factory deleteVoteSenderFactory) {
this.db = db;
this.batchUpdateFactory = batchUpdateFactory;
this.approvalsUtil = approvalsUtil;
this.psUtil = psUtil;
this.cmUtil = cmUtil;
this.userFactory = userFactory;
this.hooks = hooks;
this.deleteVoteSenderFactory = deleteVoteSenderFactory;
}
@Override
@@ -84,6 +107,11 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
private class Op extends BatchUpdate.Op {
private final Account.Id accountId;
private final String label;
private ChangeMessage changeMessage;
private Change change;
private PatchSet ps;
private Map<String, Short> newApprovals = new HashMap<>();
private Map<String, Short> oldApprovals = new HashMap<>();
private Op(Account.Id accountId, String label) {
this.accountId = accountId;
@@ -93,17 +121,36 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
@Override
public boolean updateChange(ChangeContext ctx)
throws OrmException, AuthException, ResourceNotFoundException {
IdentifiedUser user = ctx.getUser().asIdentifiedUser();
Change change = ctx.getChange();
ChangeControl ctl = ctx.getControl();
change = ctl.getChange();
PatchSet.Id psId = change.currentPatchSetId();
ps = psUtil.current(db.get(), ctl.getNotes());
PatchSetApproval psa = null;
StringBuilder msg = new StringBuilder();
// get all of the current approvals
LabelTypes labelTypes = ctx.getControl().getLabelTypes();
Map<String, Short> currentApprovals = new HashMap<>();
for (LabelType lt : labelTypes.getLabelTypes()) {
currentApprovals.put(lt.getName(), (short) 0);
for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
ctx.getDb(), ctl, psId, accountId)) {
if (lt.getLabelId().equals(a.getLabelId())) {
currentApprovals.put(lt.getName(), a.getValue());
}
}
}
// removing votes so we need to determine the new set of approval scores
newApprovals.putAll(currentApprovals);
for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
ctx.getDb(), ctl, psId, accountId)) {
if (ctl.canRemoveReviewer(a)) {
if (a.getLabel().equals(label)) {
// set the approval to 0 if vote is being removed
newApprovals.put(a.getLabel(), (short) 0);
// set old value only if the vote changed
oldApprovals.put(a.getLabel(), a.getValue());
msg.append("Removed ")
.append(a.getLabel()).append(formatLabelValue(a.getValue()))
.append(" by ").append(userFactory.create(a.getAccountId())
@@ -125,10 +172,10 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
ctx.getDb().patchSetApprovals().update(Collections.singleton(psa));
if (msg.length() > 0) {
ChangeMessage changeMessage =
changeMessage =
new ChangeMessage(new ChangeMessage.Key(change.getId(),
ChangeUtil.messageUUID(ctx.getDb())),
user.getAccountId(),
ctx.getUser().asIdentifiedUser().getAccountId(),
ctx.getWhen(),
change.currentPatchSetId());
changeMessage.setMessage(msg.toString());
@@ -137,6 +184,31 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
}
return true;
}
@Override
public void postUpdate(Context ctx) {
if (changeMessage == null) {
return;
}
IdentifiedUser user = ctx.getUser().asIdentifiedUser();
try {
ReplyToChangeSender cm = deleteVoteSenderFactory.create(
ctx.getProject(), change.getId());
cm.setFrom(user.getAccountId());
cm.setChangeMessage(changeMessage);
cm.send();
} catch (Exception e) {
log.error("Cannot email update for change " + change.getId(), e);
}
try {
hooks.doCommentAddedHook(change, user.getAccount(), ps,
changeMessage.getMessage(), newApprovals, oldApprovals, ctx.getDb());
} catch (OrmException e) {
log.warn("ChangeHook.doCommentAddedHook delivery failed", e);
}
}
}
private static String formatLabelValue(short value) {

54
gerrit-server/src/main/java/com/google/gerrit/server/mail/DeleteVoteSender.java Normal file
View File

@@ -0,0 +1,54 @@
// Copyright (C) 2016 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
/** Send notice about a vote that was removed from a change. */
public class DeleteVoteSender extends ReplyToChangeSender {
public static interface Factory extends
ReplyToChangeSender.Factory<DeleteVoteSender> {
@Override
DeleteVoteSender create(Project.NameKey project, Change.Id change);
}
@Inject
protected DeleteVoteSender(EmailArguments ea,
@Assisted Project.NameKey project,
@Assisted Change.Id id)
throws OrmException {
super(ea, "deleteVote", newChangeData(ea, project, id));
}
@Override
protected void init() throws EmailException {
super.init();
ccAllApprovals();
bccStarredBy();
includeWatchers(NotifyType.ALL_COMMENTS);
}
@Override
protected void formatChange() throws EmailException {
appendText(velocifyFile("DeleteVote.vm"));
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailModule.java
View File

@@ -21,7 +21,8 @@ public class EmailModule extends FactoryModule {
protected void configure() {
factory(AbandonedSender.Factory.class);
factory(CommentSender.Factory.class);
factory(RevertedSender.Factory.class);
factory(DeleteVoteSender.Factory.class);
factory(RestoredSender.Factory.class);
factory(RevertedSender.Factory.class);
}
}

44
gerrit-server/src/main/resources/com/google/gerrit/server/mail/DeleteVote.vm Normal file
View File

@@ -0,0 +1,44 @@
## 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.
##
##
## Template Type:
## -------------
## This is a velocity mail template, see: http://velocity.apache.org and the
## gerrit-docs:config-mail.txt for more info on modifying gerrit mail templates.
##
## Template File Names and extensions:
## ----------------------------------
## Gerrit will use templates ending in ".vm" but will ignore templates ending
## in ".vm.example". If a .vm template does not exist, the default internal
## gerrit template which is the same as the .vm.example will be used. If you
## want to override the default template, copy the .vm.example file to a .vm
## file and edit it appropriately.
##
## This Template:
## --------------
## The DeleteVote.vm template will determine the contents of the email related
## to removing votes on changes. It is a ChangeEmail: see ChangeSubject.vm
## and ChangeFooter.vm.
##
$fromName has removed a vote on this change.
Change subject: $change.subject
......................................................................
#if ($coverLetter)
$coverLetter
#end