Always use REVIEW state in ReviewDb
There are two benefits to this change. First, any test code that
verifies reviewer state that isn't dealing with CC no longer needs to
check the migration mode and implement alternate logic for ReviewDb.
Second, when a change is rebuilt from ReviewDb, PolyGerrit will no
longer display reviewers as "downgraded" to CC. Instead, CCs will be
"upgraded" to Reviewer status, which is harmless for now, and less
common.
Change-Id: Ib9a1f40b69e28e560ba2b9a532d345640c853a00
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 fcde440..0150b854 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
@@ -563,9 +563,7 @@
// 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<AccountInfo> reviewers = NoteDbMode.readWrite()
- ? c.reviewers.get(REVIEWER)
- : c.reviewers.get(CC);
+ Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId)
@@ -602,9 +600,7 @@
ChangeInfo c = gApi.changes()
.id(r.getChangeId())
.get();
- Collection<AccountInfo> reviewers = NoteDbMode.readWrite()
- ? c.reviewers.get(REVIEWER)
- : c.reviewers.get(CC);
+ Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId)
@@ -647,27 +643,13 @@
.id(r.getChangeId())
.get();
reviewers = c.reviewers.get(REVIEWER);
- if (NoteDbMode.readWrite()) {
- // When NoteDb is enabled adding a reviewer records that user as reviewer
- // in NoteDb.
- assertThat(reviewers).hasSize(2);
- Iterator<AccountInfo> 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<AccountInfo> ccs = c.reviewers.get(CC);
- assertThat(ccs).hasSize(1);
- assertThat(ccs.iterator().next()._accountId)
- .isEqualTo(user.getId().get());
- }
+ assertThat(reviewers).hasSize(2);
+ Iterator<AccountInfo> reviewerIt = reviewers.iterator();
+ assertThat(reviewerIt.next()._accountId)
+ .isEqualTo(admin.getId().get());
+ assertThat(reviewerIt.next()._accountId)
+ .isEqualTo(user.getId().get());
+ assertThat(c.reviewers).doesNotContainKey(CC);
}
@Test
@@ -878,21 +860,9 @@
assertThat(message.author._accountId).isEqualTo(admin.getId().get());
assertThat(message.message).isEqualTo(
"Removed Code-Review+1 by User <user@example.com>\n");
- if (NoteDbMode.readWrite()) {
- // When NoteDb is enabled each reviewer is explicitly recorded in the
- // NoteDb and this record stays even when all votes of that user have been
- // deleted.
- assertThat(getReviewers(c.reviewers.get(REVIEWER)))
- .containsExactlyElementsIn(
- ImmutableSet.of(admin.getId(), user.getId()));
- } else {
- // When NoteDb is disabled users that have only dummy 0 approvals on the
- // change are returned as CC and not as REVIEWER.
- assertThat(getReviewers(c.reviewers.get(REVIEWER)))
- .containsExactlyElementsIn(ImmutableSet.of(admin.getId()));
- assertThat(getReviewers(c.reviewers.get(CC)))
- .containsExactlyElementsIn(ImmutableSet.of(user.getId()));
- }
+ assertThat(getReviewers(c.reviewers.get(REVIEWER)))
+ .containsExactlyElementsIn(
+ ImmutableSet.of(admin.getId(), user.getId()));
}
@Test
@@ -979,16 +949,9 @@
.id(changeId)
.addReviewer(in);
c = gApi.changes().id(changeId).get();
- if (NoteDbMode.readWrite()) {
- assertThat(getReviewers(c.reviewers.get(REVIEWER)))
- .containsExactlyElementsIn(ImmutableSet.of(
- admin.getId(), user.getId()));
- } else {
- assertThat(getReviewers(c.reviewers.get(REVIEWER)))
- .containsExactlyElementsIn(ImmutableSet.of(admin.getId()));
- assertThat(getReviewers(c.reviewers.get(CC)))
- .containsExactlyElementsIn(ImmutableSet.of(user.getId()));
- }
+ assertThat(getReviewers(c.reviewers.get(REVIEWER)))
+ .containsExactlyElementsIn(ImmutableSet.of(
+ admin.getId(), user.getId()));
// Approve the change as user, then remove the approval
// (only to confirm that the user does have Code-Review+2 permission)
@@ -1011,16 +974,9 @@
// User should still be on the change
c = gApi.changes().id(changeId).get();
- if (NoteDbMode.readWrite()) {
- assertThat(getReviewers(c.reviewers.get(REVIEWER)))
- .containsExactlyElementsIn(ImmutableSet.of(
- admin.getId(), user.getId()));
- } else {
- assertThat(getReviewers(c.reviewers.get(REVIEWER)))
- .containsExactlyElementsIn(ImmutableSet.of(admin.getId()));
- assertThat(getReviewers(c.reviewers.get(CC)))
- .containsExactlyElementsIn(ImmutableSet.of(user.getId()));
- }
+ assertThat(getReviewers(c.reviewers.get(REVIEWER)))
+ .containsExactlyElementsIn(ImmutableSet.of(
+ admin.getId(), user.getId()));
}
@Test
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
index 50641b5..aa7e864 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -100,8 +100,7 @@
// Verify that group members were added as reviewers.
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
- assertReviewers(c, notesMigration.readChanges() ? REVIEWER : CC,
- users.subList(0, mediumGroupSize));
+ assertReviewers(c, REVIEWER, users.subList(0, mediumGroupSize));
}
@Test
@@ -128,7 +127,7 @@
assertThat(result.reviewers).hasSize(1);
AccountInfo ai = result.reviewers.get(0);
assertThat(ai._accountId).isEqualTo(user.id.get());
- assertReviewers(c, CC, user);
+ assertReviewers(c, REVIEWER, user);
}
// Verify email was sent to CCed account.
@@ -174,7 +173,12 @@
assertThat(result.ccs).isNull();
}
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
- assertReviewers(c, CC, firstUsers);
+ if (notesMigration.readChanges()) {
+ assertReviewers(c, CC, firstUsers);
+ } else {
+ assertReviewers(c, REVIEWER, firstUsers);
+ assertReviewers(c, CC);
+ }
// Verify emails were sent to each of the group's accounts.
List<Message> messages = sender.getMessages();
@@ -212,7 +216,7 @@
List<TestAccount> expectedUsers = new ArrayList<>(users.size() + 2);
expectedUsers.addAll(users);
expectedUsers.add(reviewer);
- assertReviewers(c, CC, expectedUsers);
+ assertReviewers(c, REVIEWER, expectedUsers);
}
messages = sender.getMessages();
@@ -222,9 +226,12 @@
for (int i = 0; i < 3; i++) {
expectedAddresses.add(users.get(users.size() - i - 1).emailAddress);
}
- if (notesMigration.readChanges()) {
- expectedAddresses.add(reviewer.emailAddress);
+ if (!notesMigration.readChanges()) {
+ for (int i = 0; i < 3; i++) {
+ expectedAddresses.add(users.get(i).emailAddress);
+ }
}
+ expectedAddresses.add(reviewer.emailAddress);
assertThat(m.rcpt()).containsExactlyElementsIn(expectedAddresses);
}
@@ -237,20 +244,19 @@
in.state = CC;
addReviewer(changeId, in);
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
- assertReviewers(c, REVIEWER);
- assertReviewers(c, CC, user);
+ if (notesMigration.readChanges()) {
+ assertReviewers(c, REVIEWER);
+ assertReviewers(c, CC, user);
+ } else {
+ assertReviewers(c, REVIEWER, user);
+ assertReviewers(c, CC);
+ }
in.state = REVIEWER;
addReviewer(changeId, in);
c = gApi.changes().id(r.getChangeId()).get();
- if (notesMigration.readChanges()) {
- assertReviewers(c, REVIEWER, user);
- assertReviewers(c, CC);
- } else {
- // If NoteDb not enabled, should have had no effect.
- assertReviewers(c, REVIEWER);
- assertReviewers(c, CC, user);
- }
+ assertReviewers(c, REVIEWER, user);
+ assertReviewers(c, CC);
}
@Test
@@ -275,9 +281,9 @@
assertReviewers(c, REVIEWER, admin, user);
assertReviewers(c, CC, observer);
} else {
- // In legacy mode, change owner should be the only reviewer.
- assertReviewers(c, REVIEWER, admin);
- assertReviewers(c, CC, user, observer);
+ // In legacy mode, everyone should be a reviewer.
+ assertReviewers(c, REVIEWER, admin, user, observer);
+ assertReviewers(c, CC);
}
// Verify emails were sent to added reviewers.
@@ -285,7 +291,12 @@
assertThat(messages).hasSize(3);
// First email to user.
Message m = messages.get(0);
- assertThat(m.rcpt()).containsExactly(user.emailAddress);
+ if (notesMigration.readChanges()) {
+ assertThat(m.rcpt()).containsExactly(user.emailAddress);
+ } else {
+ assertThat(m.rcpt()).containsExactly(
+ user.emailAddress, observer.emailAddress);
+ }
assertThat(m.body()).contains("Hello " + user.fullName + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
@@ -295,7 +306,7 @@
assertThat(m.rcpt()).containsExactly(user.emailAddress, observer.emailAddress);
assertThat(m.body()).contains(admin.fullName + " has uploaded a new change for review.");
} else {
- assertThat(m.rcpt()).containsExactly(observer.emailAddress);
+ assertThat(m.rcpt()).containsExactly(user.emailAddress, observer.emailAddress);
assertThat(m.body()).contains("Hello " + observer.fullName + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
}
@@ -398,11 +409,12 @@
assertReviewers(c, REVIEWER, admin, user);
assertReviewers(c, CC, users.subList(0, mediumGroupSize));
} else {
- // If not in NoteDb mode, then user is returned with the CC group.
- assertReviewers(c, REVIEWER, admin);
- List<TestAccount> expectedCC = users.subList(0, mediumGroupSize);
- expectedCC.add(user);
- assertReviewers(c, CC, expectedCC);
+ // If not in NoteDb mode, then everyone is a REVIEWER.
+ List<TestAccount> expected = users.subList(0, mediumGroupSize);
+ expected.add(admin);
+ expected.add(user);
+ assertReviewers(c, REVIEWER, expected);
+ assertReviewers(c, CC);
}
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
index 3228a22a..2a32abe 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
@@ -31,7 +31,6 @@
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.testutil.ConfigSuite;
-import com.google.gerrit.testutil.NoteDbMode;
import org.eclipse.jgit.lib.Config;
import org.junit.Test;
@@ -129,9 +128,7 @@
assertThat(label.all.get(0)._accountId).isEqualTo(user.id.get());
assertThat(label.all.get(0).value).isEqualTo(0);
- ReviewerState rs = NoteDbMode.readWrite()
- ? ReviewerState.REVIEWER : ReviewerState.CC;
- Collection<AccountInfo> ccs = info.reviewers.get(rs);
+ Collection<AccountInfo> ccs = info.reviewers.get(ReviewerState.REVIEWER);
assertThat(ccs).hasSize(1);
assertThat(ccs.iterator().next()._accountId).isEqualTo(user.id.get());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java
index 515cef7..5e0f77b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java
@@ -54,11 +54,9 @@
"multiple change IDs: %s, %s", first.getKey(), psa.getKey());
}
Account.Id id = psa.getAccountId();
+ reviewers.put(REVIEWER, id, psa.getGranted());
if (psa.getValue() != 0) {
- reviewers.put(REVIEWER, id, psa.getGranted());
reviewers.remove(CC, id);
- } else if (!reviewers.contains(REVIEWER, id)) {
- reviewers.put(CC, id, psa.getGranted());
}
}
return new ReviewerSet(reviewers);
