opendev
/
gerrit
Code Issues Proposed changes

Add hasReviewStarted, pendingReviewers to Change 

The introduction of the WIP property on changes gives us the ability to
define a new concept: whether a change has started review. A change
starts review as soon as its WIP property is set to false. Starting
review is permanent; hasReviewStarted remains true even if the change
returns to WIP. Review also starts immediately on any change that
is not WIP at creation time. This makes it possible to suppress
notifications on WIP changes according to whether notifications have
previously been sent.

Another two properties we can add now are pendingReviewers and
pendingReviewersByEmail. These are the sets of reviewer mutations that
have occurred since the change entered its current WIP phase. This is
necessary so we can correctly address all modified reviewers in
notifications when the change leaves WIP, and so PolyGerrit can indicate
to the change owner which reviewers haven't been notified yet.

The hasReviewStarted property is a simple boolean column in ReviewDb,
and is derived by NoteDb from Work-in-progress footers.

The pendingReviewers and pendingReviewersByEmail properties are only
available when changes are read from NoteDb (they reset to empty when a
change is read or rebuilt from ReviewDb). They are also derived from
Work-in-progress footers. Changes that are ready for review have no
pending reviewers. Pending reviewers are indexed, but we do not provide
any new search operators for them at this time.

Change-Id: I05c7a21b078e5b1a49940d16c1196c296cd3e25c
This commit is contained in:
Logan Hanks 2017-05-03 15:14:41 -07:00
parent 1290bb0a8b
commit 296cd898f3
29 changed files with 678 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
Documentation/rest-api-changes.txt
View File

@ -5588,7 +5588,7 @@ Only set if link:#detailed-labels[detailed labels] are requested.
The reviewers that can be removed by the calling user as a list of
link:rest-api-accounts.html#account-info[AccountInfo] entities. +
Only set if link:#detailed-labels[detailed labels] are requested.
|`reviewers` ||
|`reviewers` |optional|
The reviewers as a map that maps a reviewer state to a list of
link:rest-api-accounts.html#account-info[AccountInfo] entities.
Possible reviewer states are `REVIEWER`, `CC` and `REMOVED`. +
@ -5597,6 +5597,12 @@ Possible reviewer states are `REVIEWER`, `CC` and `REMOVED`. +
`REMOVED`: Users that were previously reviewers on the change, but have
been removed. +
Only set if link:#detailed-labels[detailed labels] are requested.
|`pending_reviewers` |optional|
Updates to `reviewers` that have been made while the change was in the
WIP state. Only present on WIP changes and only if there are pending
reviewer updates to report. These are reviewers who have not yet been
notified about being added to or removed from the change. +
Only set if link:#detailed-labels[detailed labels] are requested.
|`reviewer_updates`|optional|
Updates to reviewers set for the change as
link:#review-update-info[ReviewerUpdateInfo] entities.
@ -5626,6 +5632,8 @@ problems with this change. Only set if link:#check[CHECK] is set.
When present, change is marked as private.
|`work_in_progress` |optional, not set if `false`|
When present, change is marked as Work In Progress.
|`has_started_review` |optional, not set if `false`|
When present, change has been marked Ready at some point in time.
|==================================
[[change-input]]

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

@ -22,6 +22,7 @@ import static com.google.gerrit.acceptance.GitUtil.pushHead;
import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REMOVED;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
@ -63,6 +64,7 @@ import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.api.changes.ReviewResult;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.ConfigInput;
@ -128,6 +130,8 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Constants;
@ -369,6 +373,96 @@ public class ChangeIT extends AbstractDaemonTest {
gApi.changes().id(changeId).setReadyForReview();
}
@Test
public void hasReviewStarted() throws Exception {
PushOneCommit.Result r = createWorkInProgressChange();
String changeId = r.getChangeId();
ChangeInfo info = gApi.changes().id(changeId).get();
assertThat(info.hasReviewStarted).isFalse();
gApi.changes().id(changeId).setReadyForReview();
info = gApi.changes().id(changeId).get();
assertThat(info.hasReviewStarted).isTrue();
}
@Test
public void pendingReviewersInNoteDb() throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
ConfigInput conf = new ConfigInput();
conf.enableReviewerByEmail = InheritableBoolean.TRUE;
gApi.projects().name(project.get()).config(conf);
PushOneCommit.Result r = createWorkInProgressChange();
String changeId = r.getChangeId();
assertThat(gApi.changes().id(changeId).get().pendingReviewers).isEmpty();
// Add some pending reviewers.
TestAccount user1 =
accountCreator.create(name("user1"), name("user1") + "@example.com", "User 1");
TestAccount user2 =
accountCreator.create(name("user2"), name("user2") + "@example.com", "User 2");
TestAccount user3 =
accountCreator.create(name("user3"), name("user3") + "@example.com", "User 3");
TestAccount user4 =
accountCreator.create(name("user4"), name("user4") + "@example.com", "User 4");
ReviewInput in =
ReviewInput.noScore()
.reviewer(user1.email)
.reviewer(user2.email)
.reviewer(user3.email, CC, false)
.reviewer(user4.email, CC, false)
.reviewer("byemail1@example.com")
.reviewer("byemail2@example.com")
.reviewer("byemail3@example.com", CC, false)
.reviewer("byemail4@example.com", CC, false);
ReviewResult result = gApi.changes().id(changeId).revision("current").review(in);
assertThat(result.reviewers).isNotEmpty();
ChangeInfo info = gApi.changes().id(changeId).get();
Function<Collection<AccountInfo>, Collection<String>> toEmails =
ais -> ais.stream().map(ai -> ai.email).collect(Collectors.toSet());
assertThat(toEmails.apply(info.pendingReviewers.get(REVIEWER)))
.containsExactly(
admin.email, user1.email, user2.email, "byemail1@example.com", "byemail2@example.com");
assertThat(toEmails.apply(info.pendingReviewers.get(CC)))
.containsExactly(user3.email, user4.email, "byemail3@example.com", "byemail4@example.com");
assertThat(info.pendingReviewers.get(REMOVED)).isNull();
// Stage some pending reviewer removals.
gApi.changes().id(changeId).reviewer(user1.email).remove();
gApi.changes().id(changeId).reviewer(user3.email).remove();
gApi.changes().id(changeId).reviewer("byemail1@example.com").remove();
gApi.changes().id(changeId).reviewer("byemail3@example.com").remove();
info = gApi.changes().id(changeId).get();
assertThat(toEmails.apply(info.pendingReviewers.get(REVIEWER)))
.containsExactly(admin.email, user2.email, "byemail2@example.com");
assertThat(toEmails.apply(info.pendingReviewers.get(CC)))
.containsExactly(user4.email, "byemail4@example.com");
assertThat(toEmails.apply(info.pendingReviewers.get(REMOVED)))
.containsExactly(user1.email, user3.email, "byemail1@example.com", "byemail3@example.com");
// "Undo" a removal.
in = ReviewInput.noScore().reviewer(user1.email);
gApi.changes().id(changeId).revision("current").review(in);
info = gApi.changes().id(changeId).get();
assertThat(toEmails.apply(info.pendingReviewers.get(REVIEWER)))
.containsExactly(admin.email, user1.email, user2.email, "byemail2@example.com");
assertThat(toEmails.apply(info.pendingReviewers.get(CC)))
.containsExactly(user4.email, "byemail4@example.com");
assertThat(toEmails.apply(info.pendingReviewers.get(REMOVED)))
.containsExactly(user3.email, "byemail1@example.com", "byemail3@example.com");
// "Commit" by moving out of WIP.
gApi.changes().id(changeId).setReadyForReview();
info = gApi.changes().id(changeId).get();
assertThat(info.pendingReviewers).isEmpty();
assertThat(toEmails.apply(info.reviewers.get(REVIEWER)))
.containsExactly(admin.email, user1.email, user2.email, "byemail2@example.com");
assertThat(toEmails.apply(info.reviewers.get(CC)))
.containsExactly(user4.email, "byemail4@example.com");
assertThat(info.reviewers.get(REMOVED)).isNull();
}
@Test
public void toggleWorkInProgressState() throws Exception {
PushOneCommit.Result r = createChange();

18
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/AbandonedSenderIT.java
View File

@ -14,9 +14,7 @@
package com.google.gerrit.acceptance.server.mail;
import static com.google.gerrit.extensions.api.changes.NotifyHandling.NONE;
import static com.google.gerrit.extensions.api.changes.NotifyHandling.OWNER;
import static com.google.gerrit.extensions.api.changes.NotifyHandling.OWNER_REVIEWERS;
import static com.google.gerrit.extensions.api.changes.NotifyHandling.*;
import static com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy.CC_ON_OWN_COMMENTS;
import static com.google.gerrit.server.account.WatchConfig.NotifyType.ABANDONED_CHANGES;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
@ -183,6 +181,20 @@ public class AbandonedSenderIT extends AbstractNotificationTest {
.bcc(ABANDONED_CHANGES);
}
@Test
public void abandonWipChangeNotifyAll() throws Exception {
StagedChange sc = stageWipChange(ABANDONED_CHANGES);
abandon(sc.changeId, sc.owner, ALL);
assertThat(sender)
.sent("abandon", sc)
.notTo(sc.owner)
.cc(sc.reviewer, sc.ccer)
.to(sc.reviewerByEmail)
.cc(sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ABANDONED_CHANGES);
}
private void abandon(String changeId, TestAccount by) throws Exception {
abandon(changeId, by, EmailStrategy.ENABLED);
}

1
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@ -213,6 +213,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
Change c = TestChanges.newChange(project, user.getId(), seq.nextChangeId());
c.setCreatedOn(ts);
c.setLastUpdatedOn(ts);
c.setReviewStarted(true);
PatchSet ps =
TestChanges.newPatchSet(
c.currentPatchSetId(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", user.getId());

20
gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java
View File

@ -354,6 +354,26 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
cd.setReviewersByEmail(ReviewerByEmailSet.empty());
}
if (source.get(ChangeField.PENDING_REVIEWER.getName()) != null) {
cd.setPendingReviewers(
ChangeField.parseReviewerFieldValues(
FluentIterable.from(source.get(ChangeField.REVIEWER.getName()).getAsJsonArray())
.transform(JsonElement::getAsString)));
} else if (fields.contains(ChangeField.PENDING_REVIEWER.getName())) {
cd.setPendingReviewers(ReviewerSet.empty());
}
if (source.get(ChangeField.PENDING_REVIEWER_BY_EMAIL.getName()) != null) {
cd.setPendingReviewersByEmail(
ChangeField.parseReviewerByEmailFieldValues(
FluentIterable.from(
source
.get(ChangeField.PENDING_REVIEWER_BY_EMAIL.getName())
.getAsJsonArray())
.transform(JsonElement::getAsString)));
} else if (fields.contains(ChangeField.PENDING_REVIEWER_BY_EMAIL.getName())) {
cd.setPendingReviewersByEmail(ReviewerByEmailSet.empty());
}
decodeSubmitRecords(
source,
ChangeField.STORED_SUBMIT_RECORD_STRICT.getName(),

4
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java
View File

@ -37,7 +37,7 @@ public interface RevisionApi {
void description(String description) throws RestApiException;
void review(ReviewInput in) throws RestApiException;
ReviewResult review(ReviewInput in) throws RestApiException;
void submit() throws RestApiException;
@ -159,7 +159,7 @@ public interface RevisionApi {
}
@Override
public void review(ReviewInput in) {
public ReviewResult review(ReviewInput in) {
throw new NotImplementedException();
}

2
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java
View File

@ -47,6 +47,7 @@ public class ChangeInfo {
public Integer unresolvedCommentCount;
public Boolean isPrivate;
public Boolean workInProgress;
public Boolean hasReviewStarted;
public int _number;
@ -57,6 +58,7 @@ public class ChangeInfo {
public Map<String, Collection<String>> permittedLabels;
public Collection<AccountInfo> removableReviewers;
public Map<ReviewerState, Collection<AccountInfo>> reviewers;
public Map<ReviewerState, Collection<AccountInfo>> pendingReviewers;
public Collection<ReviewerUpdateInfo> reviewerUpdates;
public Collection<ChangeMessageInfo> messages;

24
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
View File

@ -117,6 +117,9 @@ public class LuceneChangeIndex implements ChangeIndex {
private static final String DELETED_FIELD = ChangeField.DELETED.getName();
private static final String MERGEABLE_FIELD = ChangeField.MERGEABLE.getName();
private static final String PATCH_SET_FIELD = ChangeField.PATCH_SET.getName();
private static final String PENDING_REVIEWER_FIELD = ChangeField.PENDING_REVIEWER.getName();
private static final String PENDING_REVIEWER_BY_EMAIL_FIELD =
ChangeField.PENDING_REVIEWER_BY_EMAIL.getName();
private static final String REF_STATE_FIELD = ChangeField.REF_STATE.getName();
private static final String REF_STATE_PATTERN_FIELD = ChangeField.REF_STATE_PATTERN.getName();
private static final String REVIEWEDBY_FIELD = ChangeField.REVIEWEDBY.getName();
@ -463,6 +466,12 @@ public class LuceneChangeIndex implements ChangeIndex {
if (fields.contains(REVIEWER_BY_EMAIL_FIELD)) {
decodeReviewersByEmail(doc, cd);
}
if (fields.contains(PENDING_REVIEWER_FIELD)) {
decodePendingReviewers(doc, cd);
}
if (fields.contains(PENDING_REVIEWER_BY_EMAIL_FIELD)) {
decodePendingReviewersByEmail(doc, cd);
}
decodeSubmitRecords(
doc, SUBMIT_RECORD_STRICT_FIELD, ChangeField.SUBMIT_RULE_OPTIONS_STRICT, cd);
decodeSubmitRecords(
@ -566,6 +575,21 @@ public class LuceneChangeIndex implements ChangeIndex {
.transform(IndexableField::stringValue)));
}
private void decodePendingReviewers(ListMultimap<String, IndexableField> doc, ChangeData cd) {
cd.setPendingReviewers(
ChangeField.parseReviewerFieldValues(
FluentIterable.from(doc.get(PENDING_REVIEWER_FIELD))
.transform(IndexableField::stringValue)));
}
private void decodePendingReviewersByEmail(
ListMultimap<String, IndexableField> doc, ChangeData cd) {
cd.setPendingReviewersByEmail(
ChangeField.parseReviewerByEmailFieldValues(
FluentIterable.from(doc.get(PENDING_REVIEWER_BY_EMAIL_FIELD))
.transform(IndexableField::stringValue)));
}
private void decodeSubmitRecords(
ListMultimap<String, IndexableField> doc,
String field,

13
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
View File

@ -520,6 +520,10 @@ public final class Change {
@Column(id = 21)
protected boolean workInProgress;
/** Whether the change has started review. */
@Column(id = 22)
protected boolean reviewStarted;
/** @see com.google.gerrit.server.notedb.NoteDbChangeState */
@Column(id = 101, notNull = false, length = Integer.MAX_VALUE)
protected String noteDbState;
@ -558,6 +562,7 @@ public final class Change {
topic = other.topic;
isPrivate = other.isPrivate;
workInProgress = other.workInProgress;
reviewStarted = other.reviewStarted;
noteDbState = other.noteDbState;
}
@ -720,6 +725,14 @@ public final class Change {
this.workInProgress = workInProgress;
}
public boolean hasReviewStarted() {
return reviewStarted;
}
public void setReviewStarted(boolean reviewStarted) {
this.reviewStarted = reviewStarted;
}
public String getNoteDbState() {
return noteDbState;
}

5
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
View File

@ -26,6 +26,7 @@ import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.FileApi;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewResult;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.changes.RevisionReviewerApi;
import com.google.gerrit.extensions.api.changes.RobotCommentApi;
@ -211,9 +212,9 @@ class RevisionApiImpl implements RevisionApi {
}
@Override
public void review(ReviewInput in) throws RestApiException {
public ReviewResult review(ReviewInput in) throws RestApiException {
try {
review.apply(revision, in);
return review.apply(revision, in).value();
} catch (Exception e) {
throw asRestApiException("Cannot post review", e);
}

1
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@ -197,6 +197,7 @@ public class ChangeInserter implements InsertChangeOp {
change.setTopic(topic);
change.setPrivate(isPrivate);
change.setWorkInProgress(workInProgress);
change.setReviewStarted(!workInProgress);
return change;
}

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

@ -95,6 +95,8 @@ import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GpgException;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.WebLinks;
@ -449,6 +451,7 @@ public class ChangeJson {
info.problems = result.problems();
info.isPrivate = c.isPrivate() ? true : null;
info.workInProgress = c.isWorkInProgress() ? true : null;
info.hasReviewStarted = c.hasReviewStarted();
finish(info);
} else {
info = new ChangeInfo();
@ -505,6 +508,7 @@ public class ChangeJson {
}
out.isPrivate = in.isPrivate() ? true : null;
out.workInProgress = in.isWorkInProgress() ? true : null;
out.hasReviewStarted = in.hasReviewStarted();
out.subject = in.getSubject();
out.status = in.getStatus().asChangeStatus();
out.owner = accountLoader.get(in.getOwner());
@ -550,18 +554,8 @@ public class ChangeJson {
: ImmutableMap.of();
}
out.reviewers = new HashMap<>();
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.reviewers = reviewerMap(cd.reviewers(), cd.reviewersByEmail(), false);
out.pendingReviewers = reviewerMap(cd.pendingReviewers(), cd.pendingReviewersByEmail(), true);
out.removableReviewers = removableReviewers(ctl, out);
}
@ -603,6 +597,22 @@ public class ChangeJson {
return out;
}
private Map<ReviewerState, Collection<AccountInfo>> reviewerMap(
ReviewerSet reviewers, ReviewerByEmailSet reviewersByEmail, boolean includeRemoved) {
Map<ReviewerState, Collection<AccountInfo>> reviewerMap = new HashMap<>();
for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
if (!includeRemoved && state == ReviewerStateInternal.REMOVED) {
continue;
}
Collection<AccountInfo> reviewersByState = toAccountInfo(reviewers.byState(state));
reviewersByState.addAll(toAccountInfoByEmail(reviewersByEmail.byState(state)));
if (!reviewersByState.isEmpty()) {
reviewerMap.put(state.asReviewerState(), reviewersByState);
}
}
return reviewerMap;
}
private Collection<ReviewerUpdateInfo> reviewerUpdates(ChangeData cd) throws OrmException {
List<ReviewerStatusUpdate> reviewerUpdates = cd.reviewerUpdates();
List<ReviewerUpdateInfo> result = new ArrayList<>(reviewerUpdates.size());

3
gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java
View File

@ -50,6 +50,9 @@ public class WorkInProgressOp implements BatchUpdateOp {
Change change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
change.setWorkInProgress(workInProgress);
if (!change.hasReviewStarted() && !workInProgress) {
change.setReviewStarted(true);
}
change.setLastUpdatedOn(ctx.getWhen());
update.setWorkInProgress(workInProgress);
addMessage(ctx, update);

1
gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
View File

@ -262,6 +262,7 @@ public class ReplaceOp implements BatchUpdateOp {
}
if (magicBranch.ready) {
change.setWorkInProgress(false);
change.setReviewStarted(true);
update.setWorkInProgress(false);
} else if (magicBranch.workInProgress) {
change.setWorkInProgress(true);

17
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java
View File

@ -195,6 +195,18 @@ public class ChangeField {
.stored()
.buildRepeatable(cd -> getReviewerByEmailFieldValues(cd.reviewersByEmail()));
/** Reviewer(s) modified during change's current WIP phase. */
public static final FieldDef<ChangeData, Iterable<String>> PENDING_REVIEWER =
exact(ChangeQueryBuilder.FIELD_PENDING_REVIEWER)
.stored()
.buildRepeatable(cd -> getReviewerFieldValues(cd.pendingReviewers()));
/** Reviewer(s) by email modified during change's current WIP phase. */
public static final FieldDef<ChangeData, Iterable<String>> PENDING_REVIEWER_BY_EMAIL =
exact(ChangeQueryBuilder.FIELD_PENDING_REVIEWER_BY_EMAIL)
.stored()
.buildRepeatable(cd -> getReviewerByEmailFieldValues(cd.pendingReviewersByEmail()));
@VisibleForTesting
static List<String> getReviewerFieldValues(ReviewerSet reviewers) {
List<String> r = new ArrayList<>(reviewers.asTable().size() * 2);
@ -470,6 +482,11 @@ public class ChangeField {
public static final FieldDef<ChangeData, String> WIP =
exact(ChangeQueryBuilder.FIELD_WIP).build(cd -> cd.change().isWorkInProgress() ? "1" : "0");
/** Determines if this change has started review. */
public static final FieldDef<ChangeData, String> STARTED =
exact(ChangeQueryBuilder.FIELD_STARTED)
.build(cd -> cd.change().hasReviewStarted() ? "1" : "0");
/** Users who have commented on this change. */
public static final FieldDef<ChangeData, Iterable<Integer>> COMMENTBY =
integer(ChangeQueryBuilder.FIELD_COMMENTBY)

8
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
View File

@ -74,9 +74,17 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
@Deprecated static final Schema<ChangeData> V41 = schema(V40, ChangeField.REVIEWER_BY_EMAIL);
@Deprecated static final Schema<ChangeData> V42 = schema(V41, ChangeField.WIP);
@Deprecated
static final Schema<ChangeData> V43 =
schema(V42, ChangeField.EXACT_AUTHOR, ChangeField.EXACT_COMMITTER);
static final Schema<ChangeData> V44 =
schema(
V43,
ChangeField.STARTED,
ChangeField.PENDING_REVIEWER,
ChangeField.PENDING_REVIEWER_BY_EMAIL);
public static final String NAME = "changes";
public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions();

2
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
View File

@ -233,7 +233,7 @@ public class ChangeBundle {
// last time this file was updated.
checkColumns(Change.Id.class, 1);
checkColumns(Change.class, 1, 2, 3, 4, 5, 7, 8, 10, 12, 13, 14, 17, 18, 19, 20, 21, 101);
checkColumns(Change.class, 1, 2, 3, 4, 5, 7, 8, 10, 12, 13, 14, 17, 18, 19, 20, 21, 22, 101);
checkColumns(ChangeMessage.Key.class, 1, 2);
checkColumns(ChangeMessage.class, 1, 2, 3, 4, 5, 6, 7);
checkColumns(PatchSet.Id.class, 1, 2);

14
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@ -441,6 +441,16 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return state.reviewersByEmail();
}
/** @return reviewers that were modified during this change's current WIP phase. */
public ReviewerSet getPendingReviewers() {
return state.pendingReviewers();
}
/** @return reviewers by email that were modified during this change's current WIP phase. */
public ReviewerByEmailSet getPendingReviewersByEmail() {
return state.pendingReviewersByEmail();
}
public ImmutableList<ReviewerStatusUpdate> getReviewerUpdates() {
return state.reviewerUpdates();
}
@ -590,6 +600,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return state.isWorkInProgress();
}
public boolean hasReviewStarted() {
return state.hasReviewStarted();
}
@Override
protected void onLoad(LoadHandle handle)
throws NoSuchChangeException, IOException, ConfigInvalidException {

38
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
View File

@ -17,10 +17,13 @@ package com.google.gerrit.server.notedb;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.Cache;
import com.google.common.collect.Table;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.notedb.AbstractChangeNotes.Args;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
@ -111,6 +114,14 @@ public class ChangeNotesCache {
+ P
+ list(state.patchSets(), patchSet())
+ P
+ reviewerSet(state.reviewers(), 2) // REVIEWER or CC
+ P
+ reviewerSet(state.reviewersByEmail(), 2) // REVIEWER or CC
+ P
+ reviewerSet(state.pendingReviewers(), 3) // includes REMOVED
+ P
+ reviewerSet(state.pendingReviewersByEmail(), 3) // includes REMOVED
+ P
+ list(state.allPastReviewers(), approval())
+ P
+ list(state.reviewerUpdates(), 4 * O + K + K + P)
@ -122,7 +133,11 @@ public class ChangeNotesCache {
+ P
+ map(state.changeMessagesByPatchSet().asMap(), patchSetId())
+ P
+ map(state.publishedComments().asMap(), comment());
+ map(state.publishedComments().asMap(), comment())
+ T // readOnlyUntil
+ 1 // isPrivate
+ 1 // workInProgress
+ 1; // hasReviewStarted
}
private static int ptr(Object o, int size) {
@ -176,6 +191,27 @@ public class ChangeNotesCache {
return O + O + n * (P + elemSize);
}
private static int hashBasedTable(
Table<?, ?, ?> table, int numRows, int rowKey, int columnKey, int elemSize) {
return O
+ hashtable(numRows, rowKey + hashtable(0, 0))
+ hashtable(table.size(), columnKey + elemSize);
}
private static int reviewerSet(ReviewerSet reviewers, int numRows) {
final int rowKey = 1; // ReviewerStateInternal
final int columnKey = K; // Account.Id
final int cellValue = T; // Timestamp
return hashBasedTable(reviewers.asTable(), numRows, rowKey, columnKey, cellValue);
}
private static int reviewerSet(ReviewerByEmailSet reviewers, int numRows) {
final int rowKey = 1; // ReviewerStateInternal
final int columnKey = P + 2 * str(20); // name and email, just a guess
final int cellValue = T; // Timestamp
return hashBasedTable(reviewers.asTable(), numRows, rowKey, columnKey, cellValue);
}
private static int patchSet() {
return O
+ P

46
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@ -42,6 +42,7 @@ import com.google.common.base.Enums;
import com.google.common.base.Splitter;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableTable;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
@ -164,6 +165,10 @@ class ChangeNotesParser {
private Timestamp readOnlyUntil;
private Boolean isPrivate;
private Boolean workInProgress;
private Boolean previousWorkInProgressFooter;
private Boolean hasReviewStarted;
private ReviewerSet pendingReviewers;
private ReviewerByEmailSet pendingReviewersByEmail;
ChangeNotesParser(
Change.Id changeId,
@ -180,6 +185,8 @@ class ChangeNotesParser {
bufferedApprovals = new ArrayList<>();
reviewers = HashBasedTable.create();
reviewersByEmail = HashBasedTable.create();
pendingReviewers = ReviewerSet.empty();
pendingReviewersByEmail = ReviewerByEmailSet.empty();
allPastReviewers = new ArrayList<>();
reviewerUpdates = new ArrayList<>();
submitRecords = Lists.newArrayListWithExpectedSize(1);
@ -204,6 +211,13 @@ class ChangeNotesParser {
while ((commit = walk.next()) != null) {
parse(commit);
}
if (hasReviewStarted == null) {
if (previousWorkInProgressFooter == null) {
hasReviewStarted = true;
} else {
hasReviewStarted = !previousWorkInProgressFooter;
}
}
parseNotes();
allPastReviewers.addAll(reviewers.rowKeySet());
pruneReviewers();
@ -242,6 +256,8 @@ class ChangeNotesParser {
buildApprovals(),
ReviewerSet.fromTable(Tables.transpose(reviewers)),
ReviewerByEmailSet.fromTable(Tables.transpose(reviewersByEmail)),
pendingReviewers,
pendingReviewersByEmail,
allPastReviewers,
buildReviewerUpdates(),
submitRecords,
@ -250,7 +266,8 @@ class ChangeNotesParser {
comments,
readOnlyUntil,
isPrivate,
workInProgress);
workInProgress,
hasReviewStarted);
}
private PatchSet.Id buildCurrentPatchSetId() {
@ -398,9 +415,8 @@ class ChangeNotesParser {
parseIsPrivate(commit);
}
if (workInProgress == null) {
parseWorkInProgress(commit);
}
previousWorkInProgressFooter = null;
parseWorkInProgress(commit);
if (lastUpdatedOn == null || ts.after(lastUpdatedOn)) {
lastUpdatedOn = ts;
@ -977,12 +993,30 @@ class ChangeNotesParser {
private void parseWorkInProgress(ChangeNotesCommit commit) throws ConfigInvalidException {
String raw = parseOneFooter(commit, FOOTER_WORK_IN_PROGRESS);
if (raw == null) {
// No change to WIP state in this revision.
previousWorkInProgressFooter = null;
return;
} else if (Boolean.TRUE.toString().equalsIgnoreCase(raw)) {
workInProgress = true;
// This revision moves the change into WIP.
previousWorkInProgressFooter = true;
if (workInProgress == null) {
// Because this is the first time workInProgress is being set, we know
// that this change's current state is WIP. All the reviewer updates
// we've seen so far are pending, so take a snapshot of the reviewers
// and reviewersByEmail tables.
pendingReviewers =
ReviewerSet.fromTable(Tables.transpose(ImmutableTable.copyOf(reviewers)));
pendingReviewersByEmail =
ReviewerByEmailSet.fromTable(Tables.transpose(ImmutableTable.copyOf(reviewersByEmail)));
workInProgress = true;
}
return;
} else if (Boolean.FALSE.toString().equalsIgnoreCase(raw)) {
workInProgress = false;
previousWorkInProgressFooter = false;
hasReviewStarted = true;
if (workInProgress == null) {
workInProgress = false;
}
return;
}
throw invalidFooter(FOOTER_WORK_IN_PROGRESS, raw);

29
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java
View File

@ -67,6 +67,8 @@ public abstract class ChangeNotesState {
ImmutableList.of(),
ReviewerSet.empty(),
ReviewerByEmailSet.empty(),
ReviewerSet.empty(),
ReviewerByEmailSet.empty(),
ImmutableList.of(),
ImmutableList.of(),
ImmutableList.of(),
@ -75,7 +77,8 @@ public abstract class ChangeNotesState {
ImmutableListMultimap.of(),
null,
null,
null);
null,
true);
}
static ChangeNotesState create(
@ -99,6 +102,8 @@ public abstract class ChangeNotesState {
ListMultimap<PatchSet.Id, PatchSetApproval> approvals,
ReviewerSet reviewers,
ReviewerByEmailSet reviewersByEmail,
ReviewerSet pendingReviewers,
ReviewerByEmailSet pendingReviewersByEmail,
List<Account.Id> allPastReviewers,
List<ReviewerStatusUpdate> reviewerUpdates,
List<SubmitRecord> submitRecords,
@ -107,7 +112,8 @@ public abstract class ChangeNotesState {
ListMultimap<RevId, Comment> publishedComments,
@Nullable Timestamp readOnlyUntil,
@Nullable Boolean isPrivate,
@Nullable Boolean workInProgress) {
@Nullable Boolean workInProgress,
boolean hasReviewStarted) {
if (hashtags == null) {
hashtags = ImmutableSet.of();
}
@ -128,13 +134,16 @@ public abstract class ChangeNotesState {
assignee,
status,
isPrivate,
workInProgress),
workInProgress,
hasReviewStarted),
ImmutableSet.copyOf(pastAssignees),
ImmutableSet.copyOf(hashtags),
ImmutableList.copyOf(patchSets.entrySet()),
ImmutableList.copyOf(approvals.entries()),
reviewers,
reviewersByEmail,
pendingReviewers,
pendingReviewersByEmail,
ImmutableList.copyOf(allPastReviewers),
ImmutableList.copyOf(reviewerUpdates),
ImmutableList.copyOf(submitRecords),
@ -143,7 +152,8 @@ public abstract class ChangeNotesState {
ImmutableListMultimap.copyOf(publishedComments),
readOnlyUntil,
isPrivate,
workInProgress);
workInProgress,
hasReviewStarted);
}
/**
@ -192,6 +202,9 @@ public abstract class ChangeNotesState {
@Nullable
abstract Boolean isWorkInProgress();
@Nullable
abstract Boolean hasReviewStarted();
}
// Only null if NoteDb is disabled.
@ -217,6 +230,10 @@ public abstract class ChangeNotesState {
abstract ReviewerByEmailSet reviewersByEmail();
abstract ReviewerSet pendingReviewers();
abstract ReviewerByEmailSet pendingReviewersByEmail();
abstract ImmutableList<Account.Id> allPastReviewers();
abstract ImmutableList<ReviewerStatusUpdate> reviewerUpdates();
@ -238,6 +255,9 @@ public abstract class ChangeNotesState {
@Nullable
abstract Boolean isWorkInProgress();
@Nullable
abstract Boolean hasReviewStarted();
Change newChange(Project.NameKey project) {
ChangeColumns c = checkNotNull(columns(), "columns are required");
Change change =
@ -297,6 +317,7 @@ public abstract class ChangeNotesState {
change.setAssignee(c.assignee());
change.setPrivate(c.isPrivate() == null ? false : c.isPrivate());
change.setWorkInProgress(c.isWorkInProgress() == null ? false : c.isWorkInProgress());
change.setReviewStarted(c.hasReviewStarted() == null ? false : c.hasReviewStarted());
if (!patchSets().isEmpty()) {
change.setCurrentPatchSet(c.currentPatchSetId(), c.subject(), c.originalSubject());

38
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@ -357,6 +357,8 @@ public class ChangeData {
private ImmutableMap<Account.Id, StarRef> starRefs;
private ReviewerSet reviewers;
private ReviewerByEmailSet reviewersByEmail;
private ReviewerSet pendingReviewers;
private ReviewerByEmailSet pendingReviewersByEmail;
private List<ReviewerStatusUpdate> reviewerUpdates;
private PersonIdent author;
private PersonIdent committer;
@ -991,6 +993,42 @@ public class ChangeData {
return reviewersByEmail;
}
public void setPendingReviewers(ReviewerSet pendingReviewers) {
this.pendingReviewers = pendingReviewers;
}
public ReviewerSet getPendingReviewers() {
return this.pendingReviewers;
}
public ReviewerSet pendingReviewers() throws OrmException {
if (pendingReviewers == null) {
if (!lazyLoad) {
return ReviewerSet.empty();
}
pendingReviewers = notes().getPendingReviewers();
}
return pendingReviewers;
}
public void setPendingReviewersByEmail(ReviewerByEmailSet pendingReviewersByEmail) {
this.pendingReviewersByEmail = pendingReviewersByEmail;
}
public ReviewerByEmailSet getPendingReviewersByEmail() {
return pendingReviewersByEmail;
}
public ReviewerByEmailSet pendingReviewersByEmail() throws OrmException {
if (pendingReviewersByEmail == null) {
if (!lazyLoad) {
return ReviewerByEmailSet.empty();
}
pendingReviewersByEmail = notes().getPendingReviewersByEmail();
}
return pendingReviewersByEmail;
}
public List<ReviewerStatusUpdate> reviewerUpdates() throws OrmException {
if (reviewerUpdates == null) {
if (!lazyLoad) {

11
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@ -160,6 +160,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public static final String FIELD_OWNERIN = "ownerin";
public static final String FIELD_PARENTPROJECT = "parentproject";
public static final String FIELD_PATH = "path";
public static final String FIELD_PENDING_REVIEWER = "pendingreviewer";
public static final String FIELD_PENDING_REVIEWER_BY_EMAIL = "pendingreviewerbyemail";
public static final String FIELD_PRIVATE = "private";
public static final String FIELD_PROJECT = "project";
public static final String FIELD_PROJECTS = "projects";
@ -170,6 +172,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public static final String FIELD_STAR = "star";
public static final String FIELD_STARBY = "starby";
public static final String FIELD_STARREDBY = "starredby";
public static final String FIELD_STARTED = "started";
public static final String FIELD_STATUS = "status";
public static final String FIELD_SUBMISSIONID = "submissionid";
public static final String FIELD_TR = "tr";
@ -620,6 +623,14 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
return star("ignore");
}
if ("started".equalsIgnoreCase(value)) {
if (args.getSchema().hasField(ChangeField.STARTED)) {
return new BooleanPredicate(ChangeField.STARTED, args.fillArgs);
}
throw new QueryParseException(
"'is:started' operator is not supported by change index version");
}
if ("wip".equalsIgnoreCase(value)) {
if (args.getSchema().hasField(ChangeField.WIP)) {
return new BooleanPredicate(ChangeField.WIP, args.fillArgs);

2
gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
View File

@ -35,7 +35,7 @@ import java.util.concurrent.TimeUnit;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_152> C = Schema_152.class;
public static final Class<Schema_153> C = Schema_153.class;
public static int getBinaryVersion() {
return guessVersion(C);

41
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_153.java Normal file
View File

@ -0,0 +1,41 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.schema;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.StatementExecutor;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.sql.SQLException;
/** Add reviewStarted field to change. */
public class Schema_153 extends SchemaVersion {
@Inject
Schema_153(Provider<Schema_152> prior) {
super(prior);
}
@Override
protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException {
try (StatementExecutor e = newExecutor(db)) {
// Initialize review_started to a sensible default value according to
// whether change is currently WIP. No migration is needed in NoteDb,
// where the value of review_started is always derived from the history
// of assignments to work_in_progress.
e.execute("UPDATE changes SET review_started = 'Y' WHERE work_in_progress = 'N'");
}
}
}

11
gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
View File

@ -199,15 +199,24 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
System.setProperty("user.timezone", systemTimeZone);
}
protected Change newChange() throws Exception {
protected Change newChange(boolean workInProgress) throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
ChangeUpdate u = newUpdate(c, changeOwner);
u.setChangeId(c.getKey().get());
u.setBranch(c.getDest().get());
u.setWorkInProgress(workInProgress);
u.commit();
return c;
}
protected Change newWorkInProgressChange() throws Exception {
return newChange(true);
}
protected Change newChange() throws Exception {
return newChange(false);
}
protected ChangeUpdate newUpdate(Change c, CurrentUser user) throws Exception {
ChangeUpdate update = TestChanges.newUpdate(injector, c, user);
update.setPatchSetId(c.currentPatchSetId());

67
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
View File

@ -14,6 +14,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;
import com.google.gerrit.common.TimeUtil;
@ -436,6 +437,45 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "Tag: jenkins\n");
}
@Test
public void parseWorkInProgress() throws Exception {
// Change created in WIP remains in WIP.
RevCommit commit = writeCommit("Update WIP change\n" + "\n" + "Patch-set: 1\n", true);
ChangeNotesState state = newParser(commit).parseAll();
assertThat(state.hasReviewStarted()).isFalse();
// Moving change out of WIP starts review.
commit =
writeCommit("New ready change\n" + "\n" + "Patch-set: 1\n" + "Work-in-progress: false\n");
state = newParser(commit).parseAll();
assertThat(state.hasReviewStarted()).isTrue();
// Change created not in WIP has always been in review started state.
state = assertParseSucceeds("New change that doesn't declare WIP\n" + "\n" + "Patch-set: 1\n");
assertThat(state.hasReviewStarted()).isTrue();
}
@Test
public void pendingReviewers() throws Exception {
// Change created in WIP.
RevCommit commit = writeCommit("Update WIP change\n" + "\n" + "Patch-set: 1\n", true);
ChangeNotesState state = newParser(commit).parseAll();
assertThat(state.pendingReviewers().all()).isEmpty();
assertThat(state.pendingReviewersByEmail().all()).isEmpty();
// Reviewers added while in WIP.
commit =
writeCommit(
"Add reviewers\n"
+ "\n"
+ "Patch-set: 1\n"
+ "Reviewer: Change Owner "
+ "<1@gerrit>\n",
true);
state = newParser(commit).parseAll();
assertThat(state.pendingReviewers().byState(ReviewerStateInternal.REVIEWER)).isNotEmpty();
}
@Test
public void caseInsensitiveFooters() throws Exception {
assertParseSucceeds(
@ -460,11 +500,26 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
return writeCommit(
body,
noteUtil.newIdent(
changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent, "Anonymous Coward"));
changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent, "Anonymous Coward"),
false);
}
private RevCommit writeCommit(String body, PersonIdent author) throws Exception {
Change change = newChange();
return writeCommit(body, author, false);
}
private RevCommit writeCommit(String body, boolean initWorkInProgress) throws Exception {
ChangeNoteUtil noteUtil = injector.getInstance(ChangeNoteUtil.class);
return writeCommit(
body,
noteUtil.newIdent(
changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent, "Anonymous Coward"),
initWorkInProgress);
}
private RevCommit writeCommit(String body, PersonIdent author, boolean initWorkInProgress)
throws Exception {
Change change = newChange(initWorkInProgress);
ChangeNotes notes = newNotes(change).load();
try (ObjectInserter ins = testRepo.getRepository().newObjectInserter()) {
CommitBuilder cb = new CommitBuilder();
@ -481,12 +536,12 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
}
}
private void assertParseSucceeds(String body) throws Exception {
assertParseSucceeds(writeCommit(body));
private ChangeNotesState assertParseSucceeds(String body) throws Exception {
return assertParseSucceeds(writeCommit(body));
}
private void assertParseSucceeds(RevCommit commit) throws Exception {
newParser(commit).parseAll();
private ChangeNotesState assertParseSucceeds(RevCommit commit) throws Exception {
return newParser(commit).parseAll();
}
private void assertParseFails(String body) throws Exception {

98
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assert_;
import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REMOVED;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
@ -116,7 +117,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
}
@Test
public void tagInlineCommenrts() throws Exception {
public void tagInlineComments() throws Exception {
String tag = "jenkins";
Change c = newChange();
RevCommit commit = tr.commit().message("PS2").create();
@ -3398,6 +3399,101 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getReviewersByEmail().all()).containsExactly(adr);
}
@Test
public void hasReviewStarted() throws Exception {
ChangeNotes notes = newNotes(newChange());
assertThat(notes.hasReviewStarted()).isTrue();
notes = newNotes(newWorkInProgressChange());
assertThat(notes.hasReviewStarted()).isFalse();
Change c = newWorkInProgressChange();
ChangeUpdate update = newUpdate(c, changeOwner);
update.commit();
notes = newNotes(c);
assertThat(notes.hasReviewStarted()).isFalse();
update = newUpdate(c, changeOwner);
update.setWorkInProgress(true);
update.commit();
notes = newNotes(c);
assertThat(notes.hasReviewStarted()).isFalse();
update = newUpdate(c, changeOwner);
update.setWorkInProgress(false);
update.commit();
notes = newNotes(c);
assertThat(notes.hasReviewStarted()).isTrue();
// Once review is started, setting WIP should have no impact.
c = newChange();
notes = newNotes(c);
assertThat(notes.hasReviewStarted()).isTrue();
update = newUpdate(c, changeOwner);
update.setWorkInProgress(true);
update.commit();
notes = newNotes(c);
assertThat(notes.hasReviewStarted()).isTrue();
}
@Test
public void pendingReviewers() throws Exception {
Address adr1 = new Address("Foo Bar1", "foo.bar1@gerritcodereview.com");
Address adr2 = new Address("Foo Bar2", "foo.bar2@gerritcodereview.com");
Account.Id ownerId = changeOwner.getAccount().getId();
Account.Id otherUserId = otherUser.getAccount().getId();
ChangeNotes notes = newNotes(newChange());
assertThat(notes.getPendingReviewers().asTable()).isEmpty();
assertThat(notes.getPendingReviewersByEmail().asTable()).isEmpty();
Change c = newWorkInProgressChange();
notes = newNotes(c);
assertThat(notes.getPendingReviewers().asTable()).isEmpty();
assertThat(notes.getPendingReviewersByEmail().asTable()).isEmpty();
ChangeUpdate update = newUpdate(c, changeOwner);
update.putReviewer(ownerId, REVIEWER);
update.putReviewer(otherUserId, CC);
update.putReviewerByEmail(adr1, REVIEWER);
update.putReviewerByEmail(adr2, CC);
update.commit();
notes = newNotes(c);
assertThat(notes.getPendingReviewers().byState(REVIEWER)).containsExactly(ownerId);
assertThat(notes.getPendingReviewers().byState(CC)).containsExactly(otherUserId);
assertThat(notes.getPendingReviewers().byState(REMOVED)).isEmpty();
assertThat(notes.getPendingReviewersByEmail().byState(REVIEWER)).containsExactly(adr1);
assertThat(notes.getPendingReviewersByEmail().byState(CC)).containsExactly(adr2);
assertThat(notes.getPendingReviewersByEmail().byState(REMOVED)).isEmpty();
update = newUpdate(c, changeOwner);
update.removeReviewer(ownerId);
update.removeReviewerByEmail(adr1);
update.commit();
notes = newNotes(c);
assertThat(notes.getPendingReviewers().byState(REVIEWER)).isEmpty();
assertThat(notes.getPendingReviewers().byState(CC)).containsExactly(otherUserId);
assertThat(notes.getPendingReviewers().byState(REMOVED)).containsExactly(ownerId);
assertThat(notes.getPendingReviewersByEmail().byState(REVIEWER)).isEmpty();
assertThat(notes.getPendingReviewersByEmail().byState(CC)).containsExactly(adr2);
assertThat(notes.getPendingReviewersByEmail().byState(REMOVED)).containsExactly(adr1);
update = newUpdate(c, changeOwner);
update.setWorkInProgress(false);
update.commit();
notes = newNotes(c);
assertThat(notes.getPendingReviewers().asTable()).isEmpty();
assertThat(notes.getPendingReviewersByEmail().asTable()).isEmpty();
update = newUpdate(c, changeOwner);
update.putReviewer(ownerId, REVIEWER);
update.putReviewerByEmail(adr1, REVIEWER);
update.commit();
notes = newNotes(c);
assertThat(notes.getPendingReviewers().asTable()).isEmpty();
assertThat(notes.getPendingReviewersByEmail().asTable()).isEmpty();
}
private boolean testJson() {
return noteUtil.getWriteJson();
}

102
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@ -460,36 +460,62 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
}
@Test
public void excludeWipChangeFromReviewersDashboards() throws Exception {
public void excludeWipChangeFromReviewersDashboardsBeforeSchema42() throws Exception {
assume().that(getSchemaVersion()).isLessThan(42);
assertMissingField(ChangeField.WIP);
assertFailingQuery("is:wip", "'is:wip' operator is not supported by change index version");
Account.Id user1 = createAccount("user1");
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChange(repo), userId);
Change change1 = insert(repo, newChangeWorkInProgress(repo), userId);
assertQuery("reviewer:" + user1, change1);
gApi.changes().id(change1.getChangeId()).setWorkInProgress();
assertQuery("reviewer:" + user1, change1);
}
AddReviewerInput rin = new AddReviewerInput();
rin.reviewer = user1.toString();
rin.state = ReviewerState.REVIEWER;
gApi.changes().id(change1.getId().get()).addReviewer(rin);
@Test
public void excludeWipChangeFromReviewersDashboards() throws Exception {
assume().that(getSchemaVersion()).isAtLeast(42);
if (getSchemaVersion() >= 42) {
assertQuery("is:wip");
assertQuery("reviewer:" + user1, change1);
Account.Id user1 = createAccount("user1");
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChangeWorkInProgress(repo), userId);
gApi.changes().id(change1.getChangeId()).setWorkInProgress();
assertQuery("is:wip", change1);
assertQuery("reviewer:" + user1);
assertQuery("is:wip", change1);
assertQuery("reviewer:" + user1);
gApi.changes().id(change1.getChangeId()).setReadyForReview();
assertQuery("is:wip");
assertQuery("reviewer:" + user1);
gApi.changes().id(change1.getChangeId()).setReadyForReview();
gApi.changes().id(change1.getChangeId()).setWorkInProgress();
assertQuery("is:wip", change1);
assertQuery("reviewer:" + user1);
}
assertQuery("is:wip");
assertQuery("reviewer:" + user1, change1);
} else {
assertMissingField(ChangeField.WIP);
assertFailingQuery("is:wip", "'is:wip' operator is not supported by change index version");
assertQuery("reviewer:" + user1, change1);
gApi.changes().id(change1.getChangeId()).setWorkInProgress();
assertQuery("reviewer:" + user1, change1);
}
@Test
public void byStartedBeforeSchema44() throws Exception {
assume().that(getSchemaVersion()).isLessThan(44);
assertMissingField(ChangeField.STARTED);
assertFailingQuery(
"is:started", "'is:started' operator is not supported by change index version");
}
@Test
public void byStarted() throws Exception {
assume().that(getSchemaVersion()).isAtLeast(44);
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChangeWorkInProgress(repo));
assertQuery("is:started");
gApi.changes().id(change1.getChangeId()).setReadyForReview();
assertQuery("is:started", change1);
gApi.changes().id(change1.getChangeId()).setWorkInProgress();
assertQuery("is:started", change1);
}
@Test
@ -732,11 +758,11 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
public void byLabel() throws Exception {
accountManager.authenticate(AuthRequest.forUser("anotheruser"));
TestRepository<Repo> repo = createProject("repo");
ChangeInserter ins = newChange(repo, null, null, null, null);
ChangeInserter ins2 = newChange(repo, null, null, null, null);
ChangeInserter ins3 = newChange(repo, null, null, null, null);
ChangeInserter ins4 = newChange(repo, null, null, null, null);
ChangeInserter ins5 = newChange(repo, null, null, null, null);
ChangeInserter ins = newChange(repo, null, null, null, null, false);
ChangeInserter ins2 = newChange(repo, null, null, null, null, false);
ChangeInserter ins3 = newChange(repo, null, null, null, null, false);
ChangeInserter ins4 = newChange(repo, null, null, null, null, false);
ChangeInserter ins5 = newChange(repo, null, null, null, null, false);
Change reviewMinus2Change = insert(repo, ins);
gApi.changes().id(reviewMinus2Change.getId().get()).current().review(ReviewInput.reject());
@ -816,7 +842,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
@Test
public void byLabelNotOwner() throws Exception {
TestRepository<Repo> repo = createProject("repo");
ChangeInserter ins = newChange(repo, null, null, null, null);
ChangeInserter ins = newChange(repo, null, null, null, null, false);
Account.Id user1 = createAccount("user1");
Change reviewPlus1Change = insert(repo, ins);
@ -2098,27 +2124,31 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
}
protected ChangeInserter newChange(TestRepository<Repo> repo) throws Exception {
return newChange(repo, null, null, null, null);
return newChange(repo, null, null, null, null, false);
}
protected ChangeInserter newChangeForCommit(TestRepository<Repo> repo, RevCommit commit)
throws Exception {
return newChange(repo, commit, null, null, null);
return newChange(repo, commit, null, null, null, false);
}
protected ChangeInserter newChangeForBranch(TestRepository<Repo> repo, String branch)
throws Exception {
return newChange(repo, null, branch, null, null);
return newChange(repo, null, branch, null, null, false);
}
protected ChangeInserter newChangeWithStatus(TestRepository<Repo> repo, Change.Status status)
throws Exception {
return newChange(repo, null, null, status, null);
return newChange(repo, null, null, status, null, false);
}
protected ChangeInserter newChangeWithTopic(TestRepository<Repo> repo, String topic)
throws Exception {
return newChange(repo, null, null, null, topic);
return newChange(repo, null, null, null, topic, false);
}
protected ChangeInserter newChangeWorkInProgress(TestRepository<Repo> repo) throws Exception {
return newChange(repo, null, null, null, null, true);
}
protected ChangeInserter newChange(
@ -2126,7 +2156,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
@Nullable RevCommit commit,
@Nullable String branch,
@Nullable Change.Status status,
@Nullable String topic)
@Nullable String topic,
boolean workInProgress)
throws Exception {
if (commit == null) {
commit = repo.parseBody(repo.commit().message("message").create());
@ -2143,7 +2174,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
.create(id, commit, branch)
.setValidate(false)
.setStatus(status)
.setTopic(topic);
.setTopic(topic)
.setWorkInProgress(workInProgress);
return ins;
}