From 66af3d8517f4ea6aeeb1b192cdb78dea24ecd3a2 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 10 Nov 2015 17:38:40 -0800 Subject: [PATCH] Return list of reviewers as part of ChangeInfo ChangeInfo now includes the list of reviewers when DETAILED_LABELS are requested. So far the list of reviewers could only indirectly be computed from the returned approvals, but this relied on having a dummy 0 approval for users that were added as reviewer, but that haven't voted yet. When notedb is enabled we no longer store dummy 0 approvals for reviewers, but we track the list of reviewers explicitly in notedb. When notedb was enabled the ChangeIT#addReviewerToClosedChange() test was failing because it relied on the dummy 0 approvals. With this change, reviewers are now included in ChangeInfo, adapt the tests to look at this field instead of computing the list of reviewers from the approvals. When notedb is enabled the PostReviewers REST endpoint records users as 'REVIEWER' in notedb. If notedb is disabled the PostReviewers REST endpoint behaves differently. In this case it adds dummy 0 approvals for the users, but these translate into 'CC's when the ChangeInfo is constructed. Hence the tests that check on reviewers must do different assertions depending on whether notedb is enabled or not. Change-Id: I5e7b99d85d97abc43ac17da44b7179f4ceb3e268 Signed-off-by: Edwin Kempin --- Documentation/rest-api-changes.txt | 57 +++++++++++++- .../acceptance/api/change/ChangeIT.java | 75 +++++++++++++------ .../extensions/client}/ReviewerState.java | 23 ++---- .../gerrit/extensions/common/ChangeInfo.java | 2 + .../google/gerrit/server/ApprovalsUtil.java | 26 ++++--- .../gerrit/server/change/ChangeJson.java | 33 ++++++++ .../server/change/PatchSetInserter.java | 10 ++- .../gerrit/server/change/PostReview.java | 4 +- .../com/google/gerrit/server/git/MergeOp.java | 4 +- .../gerrit/server/mail/ChangeEmail.java | 5 +- .../google/gerrit/server/mail/MailUtil.java | 11 ++- .../gerrit/server/notedb/ChangeNotes.java | 8 +- .../server/notedb/ChangeNotesParser.java | 12 +-- .../gerrit/server/notedb/ChangeUpdate.java | 10 +-- .../server/notedb/ReviewerStateInternal.java | 64 ++++++++++++++++ .../server/query/change/ChangeData.java | 4 +- .../gerrit/server/notedb/ChangeNotesTest.java | 4 +- .../notedb/CommitMessageOutputTest.java | 4 +- 18 files changed, 268 insertions(+), 88 deletions(-) rename {gerrit-server/src/main/java/com/google/gerrit/server/notedb => gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client}/ReviewerState.java (63%) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 8ae56e5d59..46791583c9 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -209,8 +209,8 @@ default. Optional fields are: -- * `DETAILED_LABELS`: detailed label information, including numeric values of all existing approvals, recognized label values, values - permitted to be set by the current user, and reviewers that may be - removed by the current user. + permitted to be set by the current user, all reviewers by state, and + reviewers that may be removed by the current user. -- [[current-revision]] @@ -641,6 +641,22 @@ REJECTED > APPROVED > DISLIKED > RECOMMENDED. "username": "jroe" } ], + "reviewers": { + "REVIEWER": [ + { + "_account_id": 1000096, + "name": "John Doe", + "email": "john.doe@example.com", + "username": "jdoe" + }, + { + "_account_id": 1000097, + "name": "Jane Roe", + "email": "jane.roe@example.com", + "username": "jroe" + } + ] + }, "messages": [ { "id": "YH-egE", @@ -1204,6 +1220,13 @@ The list consists of: "_account_id": 1000000 } ], + "reviewers": { + "REVIEWER": [ + { + "_account_id": 1000000 + } + ] + }, "current_revision": "9adb9f4c7b40eeee0646e235de818d09164d7379", "revisions": { "9adb9f4c7b40eeee0646e235de818d09164d7379": { @@ -1315,6 +1338,13 @@ The list consists of: "_account_id": 1000000 } ], + "reviewers": { + "REVIEWER": [ + { + "_account_id": 1000000 + } + ] + }, "current_revision": "1bd7c12a38854a2c6de426feec28800623f492c4", "revisions": { "1bd7c12a38854a2c6de426feec28800623f492c4": { @@ -2447,6 +2477,20 @@ for the current patch set. "email": "jane.roe@example.com" } ], + "reviewers": { + "REVIEWER": [ + { + "_account_id": 1000096, + "name": "John Doe", + "email": "john.doe@example.com" + }, + { + "_account_id": 1000097, + "name": "Jane Roe", + "email": "jane.roe@example.com" + } + ] + }, "current_revision": "674ac754f91e64a0efb8087e59a176484bd534d1", "revisions": { "674ac754f91e64a0efb8087e59a176484bd534d1": { @@ -3884,6 +3928,15 @@ Only set if link:#detailed-labels[detailed labels] are requested. The reviewers that can be removed by the calling user as a list of link:rest-api-accounts.html#account-info[AccountInfo] entities. + Only set if link:#detailed-labels[detailed labels] are requested. +|`reviewers` || +The reviewers as a map that maps a reviewer state to a list of +link:rest-api-accounts.html#account-info[AccountInfo] entities. +Possible reviewer states are `REVIEWER`, `CC` and `REMOVED`. + +`REVIEWER`: Users with at least one non-zero vote on the change. + +`CC`: Users that were added to the change, but have not voted. + +`REMOVED`: Users that were previously reviewers on the change, but have +been removed. + +Only set if link:#detailed-labels[detailed labels] are requested. |`messages`|optional| Messages associated with the change as a list of link:#change-message-info[ChangeMessageInfo] entities. + diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 92d7980d5a..159877461f 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -17,15 +17,16 @@ package com.google.gerrit.acceptance.api.change; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; +import static com.google.gerrit.extensions.client.ReviewerState.CC; +import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER; import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static com.google.gerrit.server.project.Util.blockLabel; import static com.google.gerrit.server.project.Util.category; import static com.google.gerrit.server.project.Util.value; +import static com.google.gerrit.testutil.GerritServerTests.isNoteDbTestEnabled; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; -import com.google.common.collect.Sets; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; @@ -38,6 +39,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.RevisionApi; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.ListChangesOption; +import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ApprovalInfo; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeMessageInfo; @@ -45,7 +47,6 @@ import com.google.gerrit.extensions.common.GitPerson; import com.google.gerrit.extensions.common.LabelInfo; import com.google.gerrit.extensions.common.RevisionInfo; import com.google.gerrit.extensions.restapi.ResourceConflictException; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; @@ -58,9 +59,10 @@ import org.junit.Test; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.EnumSet; +import java.util.Iterator; import java.util.List; -import java.util.Set; @NoHttpd public class ChangeIT extends AbstractDaemonTest { @@ -297,17 +299,6 @@ public class ChangeIT extends AbstractDaemonTest { .rebase(ri); } - private Set getReviewers(String changeId) throws Exception { - ChangeInfo ci = gApi.changes().id(changeId).get(); - Set result = Sets.newHashSet(); - for (LabelInfo li : ci.labels.values()) { - for (ApprovalInfo ai : li.all) { - result.add(new Account.Id(ai._accountId)); - } - } - return result; - } - @Test public void addReviewer() throws Exception { PushOneCommit.Result r = createChange(); @@ -317,8 +308,20 @@ public class ChangeIT extends AbstractDaemonTest { .id(r.getChangeId()) .addReviewer(in); - assertThat(getReviewers(r.getChangeId())) - .containsExactlyElementsIn(ImmutableSet.of(user.id)); + ChangeInfo c = gApi.changes() + .id(r.getChangeId()) + .get(); + + // When notedb is enabled adding a reviewer records that user as reviewer + // in notedb. When notedb is disabled adding a reviewer results in a dummy 0 + // approval on the change which is treated as CC when the ChangeInfo is + // created. + Collection reviewers = isNoteDbTestEnabled() + ? c.reviewers.get(REVIEWER) + : c.reviewers.get(CC); + assertThat(reviewers).hasSize(1); + assertThat(reviewers.iterator().next()._accountId) + .isEqualTo(user.getId().get()); } @Test @@ -333,16 +336,46 @@ public class ChangeIT extends AbstractDaemonTest { .revision(r.getCommit().name()) .submit(); - assertThat(getReviewers(r.getChangeId())) - .containsExactlyElementsIn(ImmutableSet.of(admin.getId())); + ChangeInfo c = gApi.changes() + .id(r.getChangeId()) + .get(); + Collection reviewers = c.reviewers.get(REVIEWER); + assertThat(reviewers).hasSize(1); + assertThat(reviewers.iterator().next()._accountId) + .isEqualTo(admin.getId().get()); + assertThat(c.reviewers).doesNotContainKey(CC); AddReviewerInput in = new AddReviewerInput(); in.reviewer = user.email; gApi.changes() .id(r.getChangeId()) .addReviewer(in); - assertThat(getReviewers(r.getChangeId())) - .containsExactlyElementsIn(ImmutableSet.of(admin.getId(), user.id)); + + c = gApi.changes() + .id(r.getChangeId()) + .get(); + reviewers = c.reviewers.get(REVIEWER); + if (isNoteDbTestEnabled()) { + // When notedb is enabled adding a reviewer records that user as reviewer + // in notedb. + assertThat(reviewers).hasSize(2); + Iterator reviewerIt = reviewers.iterator(); + assertThat(reviewerIt.next()._accountId) + .isEqualTo(admin.getId().get()); + assertThat(reviewerIt.next()._accountId) + .isEqualTo(user.getId().get()); + assertThat(c.reviewers).doesNotContainKey(CC); + } else { + // When notedb is disabled adding a reviewer results in a dummy 0 approval + // on the change which is treated as CC when the ChangeInfo is created. + assertThat(reviewers).hasSize(1); + assertThat(reviewers.iterator().next()._accountId) + .isEqualTo(admin.getId().get()); + Collection ccs = c.reviewers.get(CC); + assertThat(ccs).hasSize(1); + assertThat(ccs.iterator().next()._accountId) + .isEqualTo(user.getId().get()); + } } @Test diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerState.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ReviewerState.java similarity index 63% rename from gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerState.java rename to gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ReviewerState.java index b829a69f4b..a58c9591a9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerState.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ReviewerState.java @@ -1,4 +1,4 @@ -// Copyright (C) 2013 The Android Open Source Project +// Copyright (C) 2015 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. @@ -12,28 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -package com.google.gerrit.server.notedb; +package com.google.gerrit.extensions.client; -import org.eclipse.jgit.revwalk.FooterKey; - -/** State of a reviewer on a change. */ public enum ReviewerState { /** The user has contributed at least one nonzero vote on the change. */ - REVIEWER(new FooterKey("Reviewer")), + REVIEWER, /** The reviewer was added to the change, but has not voted. */ - CC(new FooterKey("CC")), + CC, /** The user was previously a reviewer on the change, but was removed. */ - REMOVED(new FooterKey("Removed")); - - private final FooterKey footerKey; - - private ReviewerState(FooterKey footerKey) { - this.footerKey = footerKey; - } - - FooterKey getFooterKey() { - return footerKey; - } + REMOVED; } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java index cdfe0c6372..455243a86f 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java @@ -15,6 +15,7 @@ package com.google.gerrit.extensions.common; import com.google.gerrit.extensions.client.ChangeStatus; +import com.google.gerrit.extensions.client.ReviewerState; import java.sql.Timestamp; import java.util.Collection; @@ -48,6 +49,7 @@ public class ChangeInfo { public Map labels; public Map> permittedLabels; public Collection removableReviewers; + public Map> reviewers; public Collection messages; public String currentRevision; 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 31058bc343..7551d5f4e6 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 @@ -15,6 +15,8 @@ package com.google.gerrit.server; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; @@ -44,7 +46,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.NotesMigration; -import com.google.gerrit.server.notedb.ReviewerState; +import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -114,10 +116,10 @@ public class ApprovalsUtil { * @param notes change notes. * @return multimap of reviewers keyed by state, where each account appears * exactly once in {@link SetMultimap#values()}, and - * {@link ReviewerState#REMOVED} is not present. + * {@link ReviewerStateInternal#REMOVED} is not present. * @throws OrmException if reviewers for the change could not be read. */ - public ImmutableSetMultimap getReviewers( + public ImmutableSetMultimap getReviewers( ReviewDb db, ChangeNotes notes) throws OrmException { if (!migration.readChanges()) { return getReviewers(db.patchSetApprovals().byChange(notes.getChangeId())); @@ -132,9 +134,9 @@ public class ApprovalsUtil { * change. * @return multimap of reviewers keyed by state, where each account appears * exactly once in {@link SetMultimap#values()}, and - * {@link ReviewerState#REMOVED} is not present. + * {@link ReviewerStateInternal#REMOVED} is not present. */ - public ImmutableSetMultimap getReviewers( + public ImmutableSetMultimap getReviewers( ChangeNotes notes, Iterable allApprovals) throws OrmException { if (!migration.readChanges()) { @@ -143,10 +145,10 @@ public class ApprovalsUtil { return notes.load().getReviewers(); } - private static ImmutableSetMultimap getReviewers( + private static ImmutableSetMultimap getReviewers( Iterable allApprovals) { PatchSetApproval first = null; - SetMultimap reviewers = + SetMultimap reviewers = LinkedHashMultimap.create(); for (PatchSetApproval psa : allApprovals) { if (first == null) { @@ -159,10 +161,10 @@ public class ApprovalsUtil { } Account.Id id = psa.getAccountId(); if (psa.getValue() != 0) { - reviewers.put(ReviewerState.REVIEWER, id); - reviewers.remove(ReviewerState.CC, id); - } else if (!reviewers.containsEntry(ReviewerState.REVIEWER, id)) { - reviewers.put(ReviewerState.CC, id); + reviewers.put(REVIEWER, id); + reviewers.remove(CC, id); + } else if (!reviewers.containsEntry(REVIEWER, id)) { + reviewers.put(CC, id); } } return ImmutableSetMultimap.copyOf(reviewers); @@ -215,7 +217,7 @@ public class ApprovalsUtil { cells.add(new PatchSetApproval( new PatchSetApproval.Key(psId, account, labelId), (short) 0, TimeUtil.nowTs())); - update.putReviewer(account, ReviewerState.REVIEWER); + update.putReviewer(account, REVIEWER); } db.patchSetApprovals().insert(cells); return Collections.unmodifiableList(cells); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 733d7a26ea..a02f0db780 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -40,6 +40,7 @@ import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Optional; import com.google.common.base.Throwables; +import com.google.common.collect.ComparisonChain; import com.google.common.collect.FluentIterable; import com.google.common.collect.HashBasedTable; import com.google.common.collect.HashMultimap; @@ -48,6 +49,7 @@ import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Multimap; +import com.google.common.collect.Ordering; import com.google.common.collect.SetMultimap; import com.google.common.collect.Sets; import com.google.common.collect.Table; @@ -94,6 +96,7 @@ import com.google.gerrit.server.api.accounts.GpgApiAdapter; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.LabelNormalizer; import com.google.gerrit.server.git.MergeUtil; +import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; @@ -118,7 +121,9 @@ import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Comparator; import java.util.EnumSet; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -437,6 +442,13 @@ public class ChangeJson { out.permittedLabels = permittedLabels(ctl, cd); } out.removableReviewers = removableReviewers(ctl, out.labels.values()); + + out.reviewers = new HashMap<>(); + for (Map.Entry> e + : cd.reviewers().asMap().entrySet()) { + out.reviewers.put(e.getKey().asReviewerState(), + toAccountInfo(e.getValue())); + } } boolean needMessages = has(MESSAGES); @@ -828,6 +840,27 @@ public class ChangeJson { return result; } + private Collection toAccountInfo( + Collection accounts) { + return FluentIterable.from(accounts) + .transform(new Function() { + @Override + public AccountInfo apply(Account.Id id) { + return accountLoader.get(id); + } + }) + .toSortedList(new Comparator() { + @Override + public int compare(AccountInfo a, AccountInfo b) { + return ComparisonChain.start() + .compare(a.name, b.name, Ordering.natural().nullsFirst()) + .compare(a.email, b.email, Ordering.natural().nullsFirst()) + .compare(a._accountId, b._accountId, + Ordering.natural().nullsFirst()).result(); + } + }); + } + private Map revisions(ChangeControl ctl, Map map) throws PatchListNotAvailableException, GpgException, OrmException, IOException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java index c34fb4326c..efe31e3d57 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java @@ -17,6 +17,8 @@ package com.google.gerrit.server.change; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import com.google.common.collect.SetMultimap; import com.google.gerrit.common.ChangeHooks; @@ -42,7 +44,7 @@ import com.google.gerrit.server.git.GroupCollector; import com.google.gerrit.server.git.validators.CommitValidationException; import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.mail.ReplacePatchSetSender; -import com.google.gerrit.server.notedb.ReviewerState; +import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeModifiedException; @@ -106,7 +108,7 @@ public class PatchSetInserter extends BatchUpdate.Op { private PatchSet patchSet; private PatchSetInfo patchSetInfo; private ChangeMessage changeMessage; - private SetMultimap oldReviewers; + private SetMultimap oldReviewers; @AssistedInject public PatchSetInserter(ChangeHooks hooks, @@ -280,8 +282,8 @@ public class PatchSetInserter extends BatchUpdate.Op { cm.setFrom(ctx.getUser().getAccountId()); cm.setPatchSet(patchSet, patchSetInfo); cm.setChangeMessage(changeMessage); - cm.addReviewers(oldReviewers.get(ReviewerState.REVIEWER)); - cm.addExtraCC(oldReviewers.get(ReviewerState.CC)); + cm.addReviewers(oldReviewers.get(REVIEWER)); + cm.addExtraCC(oldReviewers.get(CC)); cm.send(); } catch (Exception err) { log.error("Cannot send email for new patch set on change " 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 39f7bb4661..f2d0ffa4a4 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 @@ -17,6 +17,7 @@ package com.google.gerrit.server.change; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.auto.value.AutoValue; @@ -64,7 +65,6 @@ import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.BatchUpdate.Context; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.notedb.ChangeUpdate; -import com.google.gerrit.server.notedb.ReviewerState; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.query.change.ChangeData; @@ -549,7 +549,7 @@ public class PostReview implements RestModifyView ups.add(c); addLabelDelta(normName, c.getValue()); categories.put(normName, c.getValue()); - update.putReviewer(user.getAccountId(), ReviewerState.REVIEWER); + update.putReviewer(user.getAccountId(), REVIEWER); update.putApproval(ent.getKey(), ent.getValue()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 05e0ceccc9..a2d71fbfab 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.git; import static com.google.common.base.Preconditions.checkState; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static org.eclipse.jgit.lib.RefDatabase.ALL; import com.google.common.base.Optional; @@ -61,7 +62,6 @@ import com.google.gerrit.server.git.validators.MergeValidators; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; -import com.google.gerrit.server.notedb.ReviewerState; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.ChangeControl; @@ -1053,7 +1053,7 @@ public class MergeOp { logDebug("Add approval for " + cd + " from user " + user); ChangeUpdate update = updateFactory.create(control, timestamp); - update.putReviewer(user.getAccountId(), ReviewerState.REVIEWER); + update.putReviewer(user.getAccountId(), REVIEWER); List record = records.get(cd.change().getId()); if (record != null) { update.merge(record); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index 8948ce3f7c..43d02fbecf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.mail; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; + import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; @@ -26,7 +28,6 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo; import com.google.gerrit.reviewdb.client.StarredChange; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.mail.ProjectWatch.Watchers; -import com.google.gerrit.server.notedb.ReviewerState; import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListEntry; import com.google.gerrit.server.patch.PatchListNotAvailableException; @@ -329,7 +330,7 @@ public abstract class ChangeEmail extends NotificationEmail { /** Users who have non-zero approval codes on the change. */ protected void ccExistingReviewers() { try { - for (Account.Id id : changeData.reviewers().get(ReviewerState.REVIEWER)) { + for (Account.Id id : changeData.reviewers().get(REVIEWER)) { add(RecipientType.CC, id); } } catch (OrmException err) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java index c6e59eb44d..a3a048a854 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java @@ -14,13 +14,16 @@ package com.google.gerrit.server.mail; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; + import com.google.common.collect.Multimap; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.errors.NoSuchAccountException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.server.account.AccountResolver; -import com.google.gerrit.server.notedb.ReviewerState; +import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gwtorm.server.OrmException; import org.eclipse.jgit.revwalk.FooterKey; @@ -56,10 +59,10 @@ public class MailUtil { } public static MailRecipients getRecipientsFromReviewers( - Multimap reviewers) { + Multimap reviewers) { MailRecipients recipients = new MailRecipients(); - recipients.reviewers.addAll(reviewers.get(ReviewerState.REVIEWER)); - recipients.cc.addAll(reviewers.get(ReviewerState.CC)); + recipients.reviewers.addAll(reviewers.get(REVIEWER)); + recipients.cc.addAll(reviewers.get(CC)); return recipients; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index 7331954a44..801f172194 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -116,7 +116,7 @@ public class ChangeNotes extends AbstractChangeNotes { private final Change change; private ImmutableListMultimap approvals; - private ImmutableSetMultimap reviewers; + private ImmutableSetMultimap reviewers; private ImmutableList allPastReviewers; private ImmutableList submitRecords; private ImmutableList allChangeMessages; @@ -144,7 +144,7 @@ public class ChangeNotes extends AbstractChangeNotes { return approvals; } - public ImmutableSetMultimap getReviewers() { + public ImmutableSetMultimap getReviewers() { return reviewers; } @@ -270,9 +270,9 @@ public class ChangeNotes extends AbstractChangeNotes { } else { hashtags = ImmutableSet.of(); } - ImmutableSetMultimap.Builder reviewers = + ImmutableSetMultimap.Builder reviewers = ImmutableSetMultimap.builder(); - for (Map.Entry e + for (Map.Entry e : parser.reviewers.entrySet()) { reviewers.put(e.getValue(), e.getKey()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index fd4817f28c..b58548c9b2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -71,7 +71,7 @@ import java.util.Map; import java.util.Set; class ChangeNotesParser implements AutoCloseable { - final Map reviewers; + final Map reviewers; final List allPastReviewers; final List submitRecords; final Multimap comments; @@ -168,7 +168,7 @@ class ChangeNotesParser implements AutoCloseable { parseApproval(psId, accountId, commit, line); } - for (ReviewerState state : ReviewerState.values()) { + for (ReviewerStateInternal state : ReviewerStateInternal.values()) { for (String line : commit.getFooterLines(state.getFooterKey())) { parseReviewer(state, line); } @@ -394,7 +394,7 @@ class ChangeNotesParser implements AutoCloseable { GERRIT_PLACEHOLDER_HOST, email); } - private void parseReviewer(ReviewerState state, String line) + private void parseReviewer(ReviewerStateInternal state, String line) throws ConfigInvalidException { PersonIdent ident = RawParseUtils.parsePersonIdent(line); if (ident == null) { @@ -407,11 +407,11 @@ class ChangeNotesParser implements AutoCloseable { } private void pruneReviewers() { - Iterator> rit = + Iterator> rit = reviewers.entrySet().iterator(); while (rit.hasNext()) { - Map.Entry e = rit.next(); - if (e.getValue() == ReviewerState.REMOVED) { + Map.Entry e = rit.next(); + if (e.getValue() == ReviewerStateInternal.REMOVED) { rit.remove(); for (Table curr : approvals.values()) { curr.rowKeySet().remove(e.getKey()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 92a9856146..1d8e507691 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -86,7 +86,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { private final AccountCache accountCache; private final Map> approvals; - private final Map reviewers; + private final Map reviewers; private Change.Status status; private String subject; private List submitRecords; @@ -320,13 +320,13 @@ public class ChangeUpdate extends AbstractChangeUpdate { this.hashtags = hashtags; } - public void putReviewer(Account.Id reviewer, ReviewerState type) { - checkArgument(type != ReviewerState.REMOVED, "invalid ReviewerType"); + public void putReviewer(Account.Id reviewer, ReviewerStateInternal type) { + checkArgument(type != ReviewerStateInternal.REMOVED, "invalid ReviewerType"); reviewers.put(reviewer, type); } public void removeReviewer(Account.Id reviewer) { - reviewers.put(reviewer, ReviewerState.REMOVED); + reviewers.put(reviewer, ReviewerStateInternal.REMOVED); } /** @return the tree id for the updated tree */ @@ -422,7 +422,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { addFooter(msg, FOOTER_HASHTAGS, Joiner.on(",").join(hashtags)); } - for (Map.Entry e : reviewers.entrySet()) { + for (Map.Entry e : reviewers.entrySet()) { Account account = accountCache.get(e.getKey()).getAccount(); PersonIdent ident = newIdent(account, when); addFooter(msg, e.getValue().getFooterKey()) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java new file mode 100644 index 0000000000..3bf2135d1b --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java @@ -0,0 +1,64 @@ +// Copyright (C) 2013 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.notedb; + +import com.google.gerrit.extensions.client.ReviewerState; + +import org.eclipse.jgit.revwalk.FooterKey; + +import java.util.Arrays; + +/** State of a reviewer on a change. */ +public enum ReviewerStateInternal { + /** The user has contributed at least one nonzero vote on the change. */ + REVIEWER(new FooterKey("Reviewer"), ReviewerState.REVIEWER), + + /** The reviewer was added to the change, but has not voted. */ + CC(new FooterKey("CC"), ReviewerState.CC), + + /** The user was previously a reviewer on the change, but was removed. */ + REMOVED(new FooterKey("Removed"), ReviewerState.REMOVED); + + static { + boolean ok = true; + if (ReviewerStateInternal.values().length != ReviewerState.values().length) { + ok = false; + } + for (ReviewerStateInternal s : ReviewerStateInternal.values()) { + ok &= s.name().equals(s.state.name()); + } + if (!ok) { + throw new IllegalStateException("Mismatched reviewer state mapping: " + + Arrays.asList(ReviewerStateInternal.values()) + " != " + + Arrays.asList(ReviewerState.values())); + } + } + + private final FooterKey footerKey; + private final ReviewerState state; + + private ReviewerStateInternal(FooterKey footerKey, ReviewerState state) { + this.footerKey = footerKey; + this.state = state; + } + + FooterKey getFooterKey() { + return footerKey; + } + + public ReviewerState asReviewerState() { + return state; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 1b7713bded..c06029b2ae 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -45,7 +45,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; -import com.google.gerrit.server.notedb.ReviewerState; +import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchListEntry; @@ -707,7 +707,7 @@ public class ChangeData { return allApprovals; } - public SetMultimap reviewers() + public SetMultimap reviewers() throws OrmException { return approvalsUtil.getReviewers(notes(), approvals().values()); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index e74df8fdcf..7debe94109 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -15,8 +15,8 @@ package com.google.gerrit.server.notedb; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.server.notedb.ReviewerState.CC; -import static com.google.gerrit.server.notedb.ReviewerState.REVIEWER; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static com.google.gerrit.testutil.TestChanges.incrementPatchSet; import static java.nio.charset.StandardCharsets.UTF_8; diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java index c20690233f..811cd8a34a 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java @@ -15,8 +15,8 @@ package com.google.gerrit.server.notedb; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.server.notedb.ReviewerState.CC; -import static com.google.gerrit.server.notedb.ReviewerState.REVIEWER; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import com.google.common.collect.ImmutableList; import com.google.gerrit.common.TimeUtil;