opendev/gerrit
Code Issues Proposed changes

Always use REVIEW state in ReviewDb

Browse Source 
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
This commit is contained in:
Logan Hanks
2016-08-12 15:44:28 -07:00
parent cf806f5dfd
commit e8da8bfbc3
4 changed files with 59 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files

80
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -563,9 +563,7 @@ public class ChangeIT extends AbstractDaemonTest {
// 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 @@ public class ChangeIT extends AbstractDaemonTest {
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 @@ public class ChangeIT extends AbstractDaemonTest {
.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 @@ public class ChangeIT extends AbstractDaemonTest {
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 @@ public class ChangeIT extends AbstractDaemonTest {
.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 @@ public class ChangeIT extends AbstractDaemonTest {
// 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

66
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
View File

@@ -100,8 +100,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
// 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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
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);
}
}

5
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
View File

@@ -31,7 +31,6 @@ import com.google.gerrit.extensions.common.LabelInfo;
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 @@ public class DraftChangeIT extends AbstractDaemonTest {
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());

4
gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java
View File

@@ -54,11 +54,9 @@ public class ReviewerSet {
"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);