diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java index d1d51d0a60..66f4ae9c0e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java @@ -14,6 +14,7 @@ package com.google.gerrit.server; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Sets; @@ -31,6 +32,7 @@ import com.google.gerrit.server.util.TimeUtil; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import java.util.Collections; import java.util.List; import java.util.Set; @@ -43,6 +45,8 @@ import java.util.Set; * even if the reviewer hasn't actually given a score to the change. To * mark the "no score" case, a dummy approval, which may live in any of * the available categories, with a score of 0 is used. + *

+ * The methods in this class do not begin/commit transactions. */ public class ApprovalsUtil { @Inject @@ -92,38 +96,58 @@ public class ApprovalsUtil { db.patchSetApprovals().insert(copied); } - public void addReviewers(ReviewDb db, LabelTypes labelTypes, Change change, - PatchSet ps, PatchSetInfo info, Set wantReviewers, - Set existingReviewers) throws OrmException { + public List addReviewers(ReviewDb db, LabelTypes labelTypes, + Change change, PatchSet ps, PatchSetInfo info, + Iterable wantReviewers, Set existingReviewers) + throws OrmException { + return addReviewers(db, labelTypes, change, ps.getId(), ps.isDraft(), + info.getAuthor().getAccount(), info.getCommitter().getAccount(), + wantReviewers, existingReviewers); + } + + public List addReviewers(ReviewDb db, LabelTypes labelTypes, + Change change, Iterable wantReviewers) throws OrmException { + PatchSet.Id psId = change.currentPatchSetId(); + Set existing = Sets.newHashSet(); + for (PatchSetApproval psa : db.patchSetApprovals().byPatchSet(psId)) { + existing.add(psa.getAccountId()); + } + return addReviewers(db, labelTypes, change, psId, false, null, null, + wantReviewers, existing); + } + + private List addReviewers(ReviewDb db, + LabelTypes labelTypes, Change change, PatchSet.Id psId, boolean isDraft, + Account.Id authorId, Account.Id committerId, + Iterable wantReviewers, Set existingReviewers) + throws OrmException { List allTypes = labelTypes.getLabelTypes(); if (allTypes.isEmpty()) { - return; + return ImmutableList.of(); } - Set need = Sets.newHashSet(wantReviewers); - Account.Id authorId = info.getAuthor() != null - ? info.getAuthor().getAccount() - : null; - if (authorId != null && !ps.isDraft()) { + Set need = Sets.newLinkedHashSet(wantReviewers); + if (authorId != null && !isDraft) { need.add(authorId); } - Account.Id committerId = info.getCommitter() != null - ? info.getCommitter().getAccount() - : null; - if (committerId != null && !ps.isDraft()) { + if (committerId != null && !isDraft) { need.add(committerId); } need.remove(change.getOwner()); need.removeAll(existingReviewers); + if (need.isEmpty()) { + return ImmutableList.of(); + } List cells = Lists.newArrayListWithCapacity(need.size()); LabelId labelId = Iterables.getLast(allTypes).getLabelId(); for (Account.Id account : need) { cells.add(new PatchSetApproval( - new PatchSetApproval.Key(ps.getId(), account, labelId), + new PatchSetApproval.Key(psId, account, labelId), (short) 0, TimeUtil.nowTs())); } db.patchSetApprovals().insert(cells); + return Collections.unmodifiableList(cells); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index 5389a105b4..1d6dfe788b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -15,10 +15,9 @@ package com.google.gerrit.server.change; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; -import com.google.common.collect.Sets; +import com.google.common.collect.Maps; import com.google.common.util.concurrent.CheckedFuture; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.data.GroupDescription; @@ -34,8 +33,8 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; -import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; @@ -51,7 +50,6 @@ import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.mail.AddReviewerSender; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchProjectException; -import com.google.gerrit.server.util.TimeUtil; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -63,6 +61,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.text.MessageFormat; import java.util.List; +import java.util.Map; import java.util.Set; public class PostReviewers implements RestModifyView { @@ -74,6 +73,7 @@ public class PostReviewers implements RestModifyView groupsCollection; private final GroupMembers.Factory groupMembersFactory; @@ -90,6 +90,7 @@ public class PostReviewers implements RestModifyView groupsCollection, GroupMembers.Factory groupMembersFactory, @@ -104,6 +105,7 @@ public class PostReviewers implements RestModifyView reviewers = Sets.newLinkedHashSet(); + Map reviewers = Maps.newHashMap(); ChangeControl control = rsrc.getControl(); Set members; try { @@ -199,7 +204,7 @@ public class PostReviewers implements RestModifyView reviewers) + Map reviewers) throws OrmException, EmailException, IOException { - if (reviewers.isEmpty()) { - result.reviewers = ImmutableList.of(); - return; - } - ReviewDb db = dbProvider.get(); - PatchSet.Id psid = rsrc.getChange().currentPatchSetId(); - Set existing = Sets.newHashSet(); - for (PatchSetApproval psa : db.patchSetApprovals().byPatchSet(psid)) { - existing.add(psa.getAccountId()); - } - - result.reviewers = Lists.newArrayListWithCapacity(reviewers.size()); - List toInsert = - Lists.newArrayListWithCapacity(reviewers.size()); - for (IdentifiedUser user : reviewers) { - Account.Id id = user.getAccountId(); - if (existing.contains(id)) { - continue; - } - ChangeControl control = rsrc.getControl().forUser(user); - PatchSetApproval psa = dummyApproval(control, psid, id); - result.reviewers.add(json.format( - new ReviewerInfo(id), control, ImmutableList.of(psa))); - toInsert.add(psa); - } - if (toInsert.isEmpty()) { - return; - } - + List added; db.changes().beginTransaction(rsrc.getChange().getId()); try { ChangeUtil.bumpRowVersionNotLastUpdatedOn(rsrc.getChange().getId(), db); - db.patchSetApprovals().insert(toInsert); + added = approvalsUtil.addReviewers(db, rsrc.getControl().getLabelTypes(), + rsrc.getChange(), reviewers.keySet()); db.commit(); } finally { db.rollback(); } CheckedFuture indexFuture = indexer.indexAsync(rsrc.getChange()); + result.reviewers = Lists.newArrayListWithCapacity(added.size()); + for (PatchSetApproval psa : added) { + result.reviewers.add(json.format( + new ReviewerInfo(psa.getAccountId()), + reviewers.get(psa.getAccountId()), + ImmutableList.of(psa))); + } + accountLoaderFactory.create(true).fill(result.reviewers); - postAdd(rsrc.getChange(), result); + postAdd(rsrc.getChange(), added); indexFuture.checkedGet(); } - private void postAdd(Change change, PostResult result) + private void postAdd(Change change, List added) throws OrmException, EmailException { - if (result.reviewers.isEmpty()) { + if (added.isEmpty()) { return; } // Execute hook for added reviewers // PatchSet patchSet = dbProvider.get().patchSets().get(change.currentPatchSetId()); - for (AccountInfo info : result.reviewers) { - Account account = accountCache.get(info._id).getAccount(); + for (PatchSetApproval psa : added) { + Account account = accountCache.get(psa.getAccountId()).getAccount(); hooks.doReviewerAddedHook(change, account, patchSet, dbProvider.get()); } // Email the reviewers // // The user knows they added themselves, don't bother emailing them. - List added = - Lists.newArrayListWithCapacity(result.reviewers.size()); - for (AccountInfo info : result.reviewers) { - if (!info._id.equals(currentUser.getAccountId())) { - added.add(info._id); + List toMail = Lists.newArrayListWithCapacity(added.size()); + for (PatchSetApproval psa : added) { + if (!psa.getAccountId().equals(currentUser.getAccountId())) { + toMail.add(psa.getAccountId()); } } if (!added.isEmpty()) { try { AddReviewerSender cm = addReviewerSenderFactory.create(change); cm.setFrom(currentUser.getAccountId()); - cm.addReviewers(added); + cm.addReviewers(toMail); cm.send(); } catch (Exception err) { log.error("Cannot send email to new reviewers of change " @@ -296,13 +281,4 @@ public class PostReviewers implements RestModifyView