commit 8add794425e110472928668c5d1e73c545c25f11
author Patrick Hiesel <hiesel@google.com> Mon Apr 10 17:19:06 2017 +0200
committer Patrick Hiesel <hiesel@google.com> Mon Apr 10 17:22:01 2017 +0200
tree a7811d73e6073ae0700a96e01de8738f043a2d2e
parent 563634e70ae11382f1c4510167d945c3e88f827e

Prevent overwriting of reviewers in ChangeJson

This change fixes a bug where reviewers would be overwritten if the
change also contains reviewers by email.

Bug: Issue 5957
Change-Id: I423d4badc84410231b98adddc68286b6eba3b823

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java

diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
index d8c00cf..17c3488 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
@@ -72,6 +72,33 @@
   }
 
   @Test
+  public void addByEmailAndById() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    AccountInfo byEmail = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
+    AccountInfo byId = new AccountInfo(user.id.get());
+
+    for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) {
+      PushOneCommit.Result r = createChange();
+
+      AddReviewerInput inputByEmail = new AddReviewerInput();
+      inputByEmail.reviewer = toRfcAddressString(byEmail);
+      inputByEmail.state = state;
+      gApi.changes().id(r.getChangeId()).addReviewer(inputByEmail);
+
+      AddReviewerInput inputById = new AddReviewerInput();
+      inputById.reviewer = user.email;
+      inputById.state = state;
+      gApi.changes().id(r.getChangeId()).addReviewer(inputById);
+
+      ChangeInfo info =
+          gApi.changes().id(r.getChangeId()).get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
+      assertThat(info.reviewers).isEqualTo(ImmutableMap.of(state, ImmutableList.of(byId, byEmail)));
+      // All reviewers (both by id and by email) should be removable
+      assertThat(info.removableReviewers).isEqualTo(ImmutableList.of(byId, byEmail));
+    }
+  }
+
+  @Test
   public void removeByEmail() throws Exception {
     assume().that(notesMigration.readChanges()).isTrue();
     AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

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 3b17eb0..190f59b 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
@@ -528,14 +528,15 @@
       }
 
       out.reviewers = new HashMap<>();
-      for (Map.Entry<ReviewerStateInternal, Map<Account.Id, Timestamp>> e :
-          cd.reviewers().asTable().rowMap().entrySet()) {
-        out.reviewers.put(e.getKey().asReviewerState(), toAccountInfo(e.getValue().keySet()));
-      }
-      for (Map.Entry<ReviewerStateInternal, Map<Address, Timestamp>> e :
-          cd.reviewersByEmail().asTable().rowMap().entrySet()) {
-        out.reviewers.put(
-            e.getKey().asReviewerState(), toAccountInfoByEmail(e.getValue().keySet()));
+      for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
+        if (state == ReviewerStateInternal.REMOVED) {
+          continue;
+        }
+        Collection<AccountInfo> reviewers = toAccountInfo(cd.reviewers().byState(state));
+        reviewers.addAll(toAccountInfoByEmail(cd.reviewersByEmail().byState(state)));
+        if (!reviewers.isEmpty()) {
+          out.reviewers.put(state.asReviewerState(), reviewers);
+        }
       }
 
       out.removableReviewers = removableReviewers(ctl, out);