From 7b6ac472fb1ec624f18fc47598f4e5560d896f1c Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 24 Aug 2016 11:53:32 -0400 Subject: [PATCH] Simplify DeleteVote to only use a single loop The old logic was a little confusing: - Looked up approvals twice. - Looped over label types. - Created the message in the loop body. Change-Id: Ib59df4cb3032800a305efb6928567e4d67dac07e --- .../gerrit/server/change/DeleteVote.java | 108 +++++++++--------- .../google/gerrit/server/util/LabelVote.java | 20 ++-- 2 files changed, 66 insertions(+), 62 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java index 687aac55cc..e25e229244 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java @@ -14,8 +14,9 @@ package com.google.gerrit.server.change; +import static com.google.common.base.Preconditions.checkNotNull; + 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.api.changes.DeleteVoteInput; import com.google.gerrit.extensions.api.changes.NotifyHandling; @@ -28,6 +29,7 @@ import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; +import com.google.gerrit.reviewdb.client.LabelId; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -44,6 +46,7 @@ import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.mail.DeleteVoteSender; import com.google.gerrit.server.mail.ReplyToChangeSender; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.util.LabelVote; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -137,64 +140,66 @@ public class DeleteVote 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 + boolean found = false; LabelTypes labelTypes = ctx.getControl().getLabelTypes(); - Map 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()) - .getNameEmail()) - .append("\n"); - psa = a; - a.setValue((short)0); - ctx.getUpdate(psId).removeApprovalFor(a.getAccountId(), label); - break; - } - } else { + ctx.getDb(), ctl, psId, accountId)) { + if (labelTypes.byLabel(a.getLabelId()) == null) { + continue; // Ignore undefined labels. + } else if (!a.getLabel().equals(label)) { + // Populate map for non-matching labels, needed by VoteDeleted. + newApprovals.put(a.getLabel(), a.getValue()); + continue; + } else if (!ctl.canRemoveReviewer(a)) { throw new AuthException("delete vote not permitted"); } + // Set the approval to 0 if vote is being removed. + newApprovals.put(a.getLabel(), (short) 0); + found = true; + + // Set old value, as required by VoteDeleted. + oldApprovals.put(a.getLabel(), a.getValue()); + break; } - if (psa == null) { + if (!found) { throw new ResourceNotFoundException(); } - ctx.getDb().patchSetApprovals().upsert(Collections.singleton(psa)); - if (msg.length() > 0) { - changeMessage = - new ChangeMessage(new ChangeMessage.Key(change.getId(), - ChangeUtil.messageUUID(ctx.getDb())), - ctx.getAccountId(), - ctx.getWhen(), - change.currentPatchSetId()); - changeMessage.setMessage(msg.toString()); - cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), - changeMessage); - } + ctx.getUpdate(psId).removeApprovalFor(accountId, label); + ctx.getDb().patchSetApprovals().upsert( + Collections.singleton(deletedApproval(ctx))); + + changeMessage = + new ChangeMessage(new ChangeMessage.Key(change.getId(), + ChangeUtil.messageUUID(ctx.getDb())), + ctx.getAccountId(), + ctx.getWhen(), + change.currentPatchSetId()); + StringBuilder msg = new StringBuilder(); + msg.append("Removed "); + LabelVote.appendTo(msg, label, checkNotNull(oldApprovals.get(label))); + changeMessage.setMessage( + msg.append(" by ") + .append(userFactory.create(accountId).getNameEmail()) + .append("\n") + .toString()); + cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), + changeMessage); + return true; } + private PatchSetApproval deletedApproval(ChangeContext ctx) { + return new PatchSetApproval( + new PatchSetApproval.Key( + ps.getId(), + accountId, + new LabelId(label)), + (short) 0, + ctx.getWhen()); + } + @Override public void postUpdate(Context ctx) { if (changeMessage == null) { @@ -220,11 +225,4 @@ public class DeleteVote user.getAccount(), ctx.getWhen()); } } - - private static String formatLabelValue(short value) { - if (value > 0) { - return "+" + value; - } - return Short.toString(value); - } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java index fab0b34386..030383a318 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java @@ -58,6 +58,16 @@ public abstract class LabelVote { Short.parseShort(text.substring(e + 1), text.length())); } + public static StringBuilder appendTo(StringBuilder sb, String label, + short value) { + if (value == (short) 0) { + return sb.append('-').append(label); + } else if (value < 0) { + return sb.append(label).append(value); + } + return sb.append(label).append('+').append(value); + } + public static LabelVote create(String label, short value) { return new AutoValue_LabelVote(LabelType.checkNameInternal(label), value); } @@ -70,13 +80,9 @@ public abstract class LabelVote { public abstract short value(); public String format() { - if (value() == (short) 0) { - return '-' + label(); - } else if (value() < 0) { - return label() + value(); - } else { - return label() + '+' + value(); - } + // Max short string length is "-32768".length() == 6. + return appendTo(new StringBuilder(label().length() + 6), label(), value()) + .toString(); } public String formatWithEquals() {