opendev
/
gerrit
Code Issues Proposed changes

Revert "Always use REVIEW state in ReviewDb" 

ReviewDb persistence layer doesn't provide any means to store internal
"reviewer" or "cc" state, like it is possible with NoteDb. Instead,
in ReviewDb code path, the voted reviewers were assigned "reviewer"
state and non voted reviewers were assigned "cc" state. This is
important for implementation of notification channel, that should reach
only specific reviewer state and not all reviewers.

The most prominent examples for this differentiation is adding reviewer
and upload new patch set use cases. Power users and maintainers are
often added to very high amount of changes as reviewers. Not for all of
them they do really care. Until actually voting on those changes
notifications on these events shouldn't be sent to non voted reviewers.

To prevent this, in the mentioned use cases only voted reviewers should
be notified. NoteDb doesn't provide this feature. In NoteDb code path,
the voted reviewer state was never implemented. Instead of promoting any
reviewer state in NoteDb code path to voted reviewer state only, to
provide the battle proven notification firehouse experience in ReviewDb
code path, since Ib9a1f40b69e the internal voted reviewer state was
demoted in ReviewDb code path from voted reviewer state to any state:
voted or non voted reviewer on a change.

This broke all caller sites in ReviewDb code path for:

  changeData.reviewers().byState(ReviewerStateInternal.REVIEWER)

that expected to get a reviewer with non-zero votes. Those caller
sites were not adjusted. The comments are still claiming:

  /** Users who have non-zero approval codes on the change. */

Particularly, this broke "Add Reviewer" and "Upload new patch set" use
cases, that started to add all existing reviewers and not only voted
users to CC for notification for these use cases.

In 2.13 release this change was even not justified, as NoteDb is not
ready for production use anyway. It could be done in one or another form
in later releases, with announcement or, better, with offering of viable
alternative like Work-In-Progress workflow implementation, where users
can have fine granular control of whether notifications are sent or not.

This reverts commit e8da8bfbc3.

Bug: Issue 5190
Change-Id: I1d67e36882b1b4b31d2c2e1a34ce5bbfe5ee733c
This commit is contained in:
David Ostrovsky 2017-03-06 08:31:55 +01:00
parent 9eb3b2fd86
commit 8eddd8975e
4 changed files with 94 additions and 52 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

@ -652,7 +652,9 @@ 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 = c.reviewers.get(REVIEWER);
Collection<AccountInfo> reviewers = NoteDbMode.readWrite()
? c.reviewers.get(REVIEWER)
: c.reviewers.get(CC);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId)
@ -689,7 +691,9 @@ public class ChangeIT extends AbstractDaemonTest {
ChangeInfo c = gApi.changes()
.id(r.getChangeId())
.get();
Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
Collection<AccountInfo> reviewers = NoteDbMode.readWrite()
? c.reviewers.get(REVIEWER)
: c.reviewers.get(CC);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId)
@ -732,13 +736,27 @@ public class ChangeIT extends AbstractDaemonTest {
.id(r.getChangeId())
.get();
reviewers = c.reviewers.get(REVIEWER);
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);
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());
}
}
@Test
@ -949,9 +967,21 @@ 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");
assertThat(getReviewers(c.reviewers.get(REVIEWER)))
.containsExactlyElementsIn(
ImmutableSet.of(admin.getId(), user.getId()));
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()));
}
}
@Test
@ -1038,9 +1068,16 @@ public class ChangeIT extends AbstractDaemonTest {
.id(changeId)
.addReviewer(in);
c = gApi.changes().id(changeId).get();
assertThat(getReviewers(c.reviewers.get(REVIEWER)))
.containsExactlyElementsIn(ImmutableSet.of(
admin.getId(), user.getId()));
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()));
}
// Approve the change as user, then remove the approval
// (only to confirm that the user does have Code-Review+2 permission)
@ -1063,9 +1100,16 @@ public class ChangeIT extends AbstractDaemonTest {
// User should still be on the change
c = gApi.changes().id(changeId).get();
assertThat(getReviewers(c.reviewers.get(REVIEWER)))
.containsExactlyElementsIn(ImmutableSet.of(
admin.getId(), user.getId()));
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()));
}
}
@Test

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

@ -100,7 +100,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
// Verify that group members were added as reviewers.
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
assertReviewers(c, REVIEWER, users.subList(0, mediumGroupSize));
assertReviewers(c, notesMigration.readChanges() ? REVIEWER : CC,
users.subList(0, mediumGroupSize));
}
@Test
@ -127,7 +128,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, REVIEWER, user);
assertReviewers(c, CC, user);
}
// Verify email was sent to CCed account.
@ -173,12 +174,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
assertThat(result.ccs).isNull();
}
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
if (notesMigration.readChanges()) {
assertReviewers(c, CC, firstUsers);
} else {
assertReviewers(c, REVIEWER, firstUsers);
assertReviewers(c, CC);
}
assertReviewers(c, CC, firstUsers);
// Verify emails were sent to each of the group's accounts.
List<Message> messages = sender.getMessages();
@ -216,7 +212,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
List<TestAccount> expectedUsers = new ArrayList<>(users.size() + 2);
expectedUsers.addAll(users);
expectedUsers.add(reviewer);
assertReviewers(c, REVIEWER, expectedUsers);
assertReviewers(c, CC, expectedUsers);
}
messages = sender.getMessages();
@ -226,12 +222,9 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
for (int i = 0; i < 3; i++) {
expectedAddresses.add(users.get(users.size() - i - 1).emailAddress);
}
if (!notesMigration.readChanges()) {
for (int i = 0; i < 3; i++) {
expectedAddresses.add(users.get(i).emailAddress);
}
if (notesMigration.readChanges()) {
expectedAddresses.add(reviewer.emailAddress);
}
expectedAddresses.add(reviewer.emailAddress);
assertThat(m.rcpt()).containsExactlyElementsIn(expectedAddresses);
}
@ -244,19 +237,20 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
in.state = CC;
addReviewer(changeId, in);
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
if (notesMigration.readChanges()) {
assertReviewers(c, REVIEWER);
assertReviewers(c, CC, user);
} else {
assertReviewers(c, REVIEWER, user);
assertReviewers(c, CC);
}
assertReviewers(c, REVIEWER);
assertReviewers(c, CC, user);
in.state = REVIEWER;
addReviewer(changeId, in);
c = gApi.changes().id(r.getChangeId()).get();
assertReviewers(c, REVIEWER, user);
assertReviewers(c, CC);
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);
}
}
@Test
@ -281,9 +275,9 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
assertReviewers(c, REVIEWER, admin, user);
assertReviewers(c, CC, observer);
} else {
// In legacy mode, everyone should be a reviewer.
assertReviewers(c, REVIEWER, admin, user, observer);
assertReviewers(c, CC);
// In legacy mode, change owner should be the only reviewer.
assertReviewers(c, REVIEWER, admin);
assertReviewers(c, CC, user, observer);
}
// Verify emails were sent to added reviewers.
@ -396,12 +390,11 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
assertReviewers(c, REVIEWER, admin, user);
assertReviewers(c, CC, users.subList(0, mediumGroupSize));
} else {
// 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);
// 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);
}
}

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

@ -31,6 +31,7 @@ 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;
@ -128,7 +129,9 @@ public class DraftChangeIT extends AbstractDaemonTest {
assertThat(label.all.get(0)._accountId).isEqualTo(user.id.get());
assertThat(label.all.get(0).value).isEqualTo(0);
Collection<AccountInfo> ccs = info.reviewers.get(ReviewerState.REVIEWER);
ReviewerState rs = NoteDbMode.readWrite()
? ReviewerState.REVIEWER : ReviewerState.CC;
Collection<AccountInfo> ccs = info.reviewers.get(rs);
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,9 +54,11 @@ 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);