opendev/gerrit
Code Issues Proposed changes

Merge "Always use REVIEW state in ReviewDb"

Browse Source
This commit is contained in:
Dave Borowitz 2016-08-15 15:14:37 +00:00 committed by Gerrit Code Review
commit 93a4c8e42d
4 changed files with 59 additions and 96 deletions
Show Stats 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

@ -565,9 +565,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)
@ -604,9 +602,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)
@ -649,27 +645,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
@ -880,21 +862,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
@ -981,16 +951,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)
@ -1013,16 +976,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);