Merge "ChangeJson: Ensure limitToPsId revision is present in output"
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
index 87436e7..239c296 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -33,6 +33,7 @@
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.testutil.ConfigSuite;
@@ -225,6 +226,7 @@
String changeId2 =
createChangeWithTopic(testRepo, "foo2", "touching b", "b.txt", "real content")
.getChangeId();
+ int changeNum2 = gApi.changes().id(changeId2).info()._number;
approve(changeId2);
// collide with the other change in the same topic
@@ -243,7 +245,7 @@
assertThat(info.enabled).isNull();
assertThat(info.label).isEqualTo("Submit whole topic");
assertThat(info.method).isEqualTo("POST");
- assertThat(info.title).isEqualTo("Problems with change(s): 2");
+ assertThat(info.title).isEqualTo("Problems with change(s): " + changeNum2);
} else {
noSubmitWholeTopicAssertions(actions, 1);
}
@@ -357,9 +359,11 @@
}
@Test
- public void revisionActionVisitor() throws Exception {
+ public void currentRevisionActionVisitor() throws Exception {
String id = createChange().getChangeId();
+ amendChange(id);
ChangeInfo origChange = gApi.changes().id(id).get(EnumSet.of(ListChangesOption.CHANGE_ACTIONS));
+ Change.Id changeId = new Change.Id(origChange._number);
class Visitor implements ActionVisitor {
@Override
@@ -373,7 +377,7 @@
assertThat(changeInfo).isNotNull();
assertThat(changeInfo._number).isEqualTo(origChange._number);
assertThat(revisionInfo).isNotNull();
- assertThat(revisionInfo._number).isEqualTo(1);
+ assertThat(revisionInfo._number).isEqualTo(2);
if (name.equals("cherrypick")) {
return false;
}
@@ -393,24 +397,23 @@
// Test different codepaths within ActionJson...
// ...via revision API.
- visitedRevisionActionsAssertions(origActions, gApi.changes().id(id).current().actions());
+ visitedCurrentRevisionActionsAssertions(origActions, gApi.changes().id(id).current().actions());
// ...via change API with option.
EnumSet<ListChangesOption> opts = EnumSet.of(CURRENT_ACTIONS, CURRENT_REVISION);
ChangeInfo changeInfo = gApi.changes().id(id).get(opts);
RevisionInfo revisionInfo = Iterables.getOnlyElement(changeInfo.revisions.values());
- visitedRevisionActionsAssertions(origActions, revisionInfo.actions);
+ visitedCurrentRevisionActionsAssertions(origActions, revisionInfo.actions);
// ...via ChangeJson directly.
- ChangeData cd = changeDataFactory.create(db, project, new Change.Id(origChange._number));
+ ChangeData cd = changeDataFactory.create(db, project, changeId);
revisionInfo =
changeJsonFactory
.create(opts)
- .getRevisionInfo(cd.changeControl(), Iterables.getOnlyElement(cd.patchSets()));
- visitedRevisionActionsAssertions(origActions, revisionInfo.actions);
+ .getRevisionInfo(cd.changeControl(), cd.patchSet(new PatchSet.Id(changeId, 1)));
}
- private void visitedRevisionActionsAssertions(
+ private void visitedCurrentRevisionActionsAssertions(
Map<String, ActionInfo> origActions, Map<String, ActionInfo> newActions) {
assertThat(newActions).isNotNull();
Set<String> expectedNames = new TreeSet<>(origActions.keySet());
@@ -422,6 +425,50 @@
assertThat(rebase.label).isEqualTo("All Your Base");
}
+ @Test
+ public void oldRevisionActionVisitor() throws Exception {
+ String id = createChange().getChangeId();
+ amendChange(id);
+ ChangeInfo origChange = gApi.changes().id(id).get(EnumSet.of(ListChangesOption.CHANGE_ACTIONS));
+
+ class Visitor implements ActionVisitor {
+ @Override
+ public boolean visit(String name, ActionInfo actionInfo, ChangeInfo changeInfo) {
+ return true; // Do nothing; implicitly called for CURRENT_ACTIONS.
+ }
+
+ @Override
+ public boolean visit(
+ String name, ActionInfo actionInfo, ChangeInfo changeInfo, RevisionInfo revisionInfo) {
+ assertThat(changeInfo).isNotNull();
+ assertThat(changeInfo._number).isEqualTo(origChange._number);
+ assertThat(revisionInfo).isNotNull();
+ assertThat(revisionInfo._number).isEqualTo(1);
+ if (name.equals("description")) {
+ actionInfo.label = "Describify";
+ }
+ return true;
+ }
+ }
+
+ Map<String, ActionInfo> origActions = gApi.changes().id(id).revision(1).actions();
+ assertThat(origActions.keySet()).containsExactly("description");
+ assertThat(origActions.get("description").label).isEqualTo("Edit Description");
+
+ Visitor v = new Visitor();
+ visitorHandle = actionVisitors.add(v);
+
+ // Unlike for the current revision, actions for old revisions are only available via the
+ // revision API.
+ Map<String, ActionInfo> newActions = gApi.changes().id(id).revision(1).actions();
+ assertThat(newActions).isNotNull();
+ assertThat(newActions.keySet()).isEqualTo(origActions.keySet());
+
+ ActionInfo description = newActions.get("description");
+ assertThat(description).isNotNull();
+ assertThat(description.label).isEqualTo("Describify");
+ }
+
private void commonActionsAssertions(Map<String, ActionInfo> actions) {
assertThat(actions).hasSize(4);
assertThat(actions).containsKey("cherrypick");
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/revision/RevisionIT.java
index 89fdeff..220254b 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/revision/RevisionIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/revision/RevisionIT.java
@@ -23,6 +23,8 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.util.Base64;
import org.junit.Test;
@@ -75,4 +77,34 @@
response.assertBadRequest();
assertThat(response.getEntityContent()).isEqualTo("invalid parent");
}
+
+ @Test
+ public void getReview() throws Exception {
+ PushOneCommit.Result r = createChange();
+ ObjectId ps1Commit = r.getCommit();
+ r = amendChange(r.getChangeId());
+ ObjectId ps2Commit = r.getCommit();
+
+ ChangeInfo info1 = checkRevisionReview(r, 1, ps1Commit);
+ assertThat(info1.currentRevision).isNull();
+
+ ChangeInfo info2 = checkRevisionReview(r, 2, ps2Commit);
+ assertThat(info2.currentRevision).isEqualTo(ps2Commit.name());
+ }
+
+ private ChangeInfo checkRevisionReview(
+ PushOneCommit.Result r, int psNum, ObjectId expectedRevision) throws Exception {
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
+
+ RestResponse response =
+ adminRestSession.get("/changes/" + r.getChangeId() + "/revisions/" + psNum + "/review");
+ response.assertOK();
+ ChangeInfo info = newGson().fromJson(response.getReader(), ChangeInfo.class);
+
+ // Check for DETAILED_ACCOUNTS, DETAILED_LABELS, and specified revision.
+ assertThat(info.owner.name).isNotNull();
+ assertThat(info.labels.get("Code-Review").all).hasSize(1);
+ assertThat(info.revisions.keySet()).containsExactly(expectedRevision.name());
+ return info;
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 7208de1..7d62c2b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -586,7 +586,7 @@
// This block must come after the ChangeInfo is mostly populated, since
// it will be passed to ActionVisitors as-is.
if (needRevisions) {
- out.revisions = revisions(ctl, cd, src, out);
+ out.revisions = revisions(ctl, cd, src, limitToPsId, out);
if (out.revisions != null) {
for (Map.Entry<String, RevisionInfo> entry : out.revisions.entrySet()) {
if (entry.getValue().isCurrent) {
@@ -1202,14 +1202,26 @@
}
private Map<String, RevisionInfo> revisions(
- ChangeControl ctl, ChangeData cd, Map<PatchSet.Id, PatchSet> map, ChangeInfo changeInfo)
+ ChangeControl ctl,
+ ChangeData cd,
+ Map<PatchSet.Id, PatchSet> map,
+ Optional<PatchSet.Id> limitToPsId,
+ ChangeInfo changeInfo)
throws PatchListNotAvailableException, GpgException, OrmException, IOException {
Map<String, RevisionInfo> res = new LinkedHashMap<>();
try (Repository repo = openRepoIfNecessary(ctl);
RevWalk rw = newRevWalk(repo)) {
for (PatchSet in : map.values()) {
- if ((has(ALL_REVISIONS) || in.getId().equals(ctl.getChange().currentPatchSetId()))
- && ctl.isPatchVisible(in, db.get())) {
+ PatchSet.Id id = in.getId();
+ boolean want = false;
+ if (has(ALL_REVISIONS)) {
+ want = true;
+ } else if (limitToPsId.isPresent()) {
+ want = id.equals(limitToPsId.get());
+ } else {
+ want = id.equals(ctl.getChange().currentPatchSetId());
+ }
+ if (want && ctl.isPatchVisible(in, db.get())) {
res.put(in.getRevision().get(), toRevisionInfo(ctl, cd, in, repo, rw, false, changeInfo));
}
}
