opendev/gerrit
Code Issues Proposed changes

RemoveDrafts: disallow creation of new drafts

Browse Source 
Draft workflow in Gerrit will be deprecated and slated for removal.
This change is the first step in this direction.

However, the repo-tool, mainly used by Andorid developers, is still
depending on drafts and has no support for private or wip changes.
We've added '--private' and '--wip' options to the repo-tool so that
users could have alternatives after pushing drafts is rejected. But,
unfortunately, it has proved hard for us to create a new repo-tool
release containing those two new options. That means we have to find
some workarounds instead of rejecting requests for creating drafts
directly.

The most important feature of drafts is privacy (draft changes and
patch sets are only visible to the change owners and reviewers).
Luckily, we can achieve this with the help of private changes and
change edits:

1- If a user tries to create a draft change, we will create a
private change instead.
2- If a user tries to create a draft patch set, we will create a
change edit instead.

The above behaviors will be a little confusing to our users because
the change or the patch set state is inconsistent with the push
option. But they get almost the same feature. And they can use
similar operations to public their changes/edits. Before this,
they click 'Publish drafts', now they click 'Unmark private' or
'Publish edit'.

After the repo-tool supports private/wip changes, we will reject
those requests with draft option completely.

With this change, users can't create new drafts through the git push
or the 'Create Change' REST API. After this change is released, draft
changes and draft patch sets can still exist in Gerrit sites. The
existing draft changes and draft patch sets can still be published
and deleted. Some operations like cherry-pick may create a new draft
patch set if the current patch set of the target change is draft. But
after migrating existing drafts, there is no way to create a new draft
any more.

Given that Change.Status enum still contains DRAFT value and given that
patch set class still has draft flag, the draft state of changes and
patch sets can still be toggled programmatically. Thus we can keep most
drafts related unit tests until existing draft changes and draft patch
sets are migrated to non-draft changes and patch sets.

Migration path for draft changes and patch sets (will be done in
follow-up changes):

* Draft changes will be migrated to private or work-in-progress changes
* Draft patch sets will be published

During this migration, the DRAFT value will be removed from the
Change.Status enum and the draft flag will be removed from the patch set
class.

Bug: Issue 6881
Change-Id: I8663f0580b90d7ce10a597b42abecf8734d3f9b2
This commit is contained in:
David Ostrovsky
2017-07-27 08:05:43 +02:00
parent 03077adb37
commit 4b44bdc702
23 changed files with 268 additions and 401 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -834,7 +834,7 @@ public class ChangeIT extends AbstractDaemonTest {
@Test
public void publish() throws Exception {
PushOneCommit.Result r = createChange("refs/drafts/master");
PushOneCommit.Result r = createDraftChange();
assertThat(info(r.getChangeId()).status).isEqualTo(ChangeStatus.DRAFT);
gApi.changes().id(r.getChangeId()).publish();
assertThat(info(r.getChangeId()).status).isEqualTo(ChangeStatus.NEW);
@@ -842,7 +842,7 @@ public class ChangeIT extends AbstractDaemonTest {
@Test
public void deleteDraftChange() throws Exception {
PushOneCommit.Result r = createChange("refs/drafts/master");
PushOneCommit.Result r = createDraftChange();
assertThat(query(r.getChangeId())).hasSize(1);
assertThat(info(r.getChangeId()).status).isEqualTo(ChangeStatus.DRAFT);
gApi.changes().id(r.getChangeId()).delete();
@@ -2472,8 +2472,9 @@ public class ChangeIT extends AbstractDaemonTest {
// Amend draft as admin
PushOneCommit.Result r2 =
amendChange(r1.getChangeId(), "refs/drafts/master", admin, adminTestRepo);
amendChange(r1.getChangeId(), "refs/for/master", admin, adminTestRepo);
r2.assertOkStatus();
setCurrentPatchSetAsDraft(r2.getChange().getId());
// Add user as reviewer to make this patch set visible
AddReviewerInput in = new AddReviewerInput();
@@ -2485,9 +2486,9 @@ public class ChangeIT extends AbstractDaemonTest {
userTestRepo.reset("ps");
// Amend change as user
PushOneCommit.Result r3 =
amendChange(r2.getChangeId(), "refs/drafts/master", user, userTestRepo);
PushOneCommit.Result r3 = amendChange(r2.getChangeId(), "refs/for/master", user, userTestRepo);
r3.assertOkStatus();
setCurrentPatchSetAsDraft(r3.getChange().getId());
}
@Test
@@ -2503,8 +2504,9 @@ public class ChangeIT extends AbstractDaemonTest {
// Amend draft as admin
PushOneCommit.Result r2 =
amendChange(r1.getChangeId(), "refs/drafts/master", admin, adminTestRepo);
amendChange(r1.getChangeId(), "refs/for/master", admin, adminTestRepo);
r2.assertOkStatus();
setCurrentPatchSetAsDraft(r2.getChange().getId());
// Fetch change
GitUtil.fetch(userTestRepo, r1.getPatchSet().getRefName() + ":ps");
@@ -2594,8 +2596,9 @@ public class ChangeIT extends AbstractDaemonTest {
// Create change as admin
PushOneCommit push = pushFactory.create(db, admin.getIdent(), adminTestRepo);
PushOneCommit.Result r1 = push.to("refs/drafts/master");
PushOneCommit.Result r1 = push.to("refs/for/master");
r1.assertOkStatus();
markChangeAsDraft(r1.getChange().getId());
// Add user as reviewer
AddReviewerInput in = new AddReviewerInput();
@@ -2624,8 +2627,9 @@ public class ChangeIT extends AbstractDaemonTest {
// Create change as admin
PushOneCommit push = pushFactory.create(db, admin.getIdent(), adminTestRepo);
PushOneCommit.Result r1 = push.to("refs/drafts/master");
PushOneCommit.Result r1 = push.to("refs/for/master");
r1.assertOkStatus();
markChangeAsDraft(p, r1.getChange().getId());
// Add user as reviewer
AddReviewerInput in = new AddReviewerInput();
@@ -2637,8 +2641,7 @@ public class ChangeIT extends AbstractDaemonTest {
userTestRepo.reset("ps");
// Amend change as user
PushOneCommit.Result r2 =
amendChange(r1.getChangeId(), "refs/drafts/master", user, userTestRepo);
PushOneCommit.Result r2 = amendChange(r1.getChangeId(), "refs/for/master", user, userTestRepo);
r2.assertErrorStatus("cannot add patch set to " + r1.getChange().getId().id + ".");
}

7
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
View File

@@ -303,7 +303,7 @@ public class RevisionIT extends AbstractDaemonTest {
@Test
public void deleteDraft() throws Exception {
PushOneCommit.Result r = createDraft();
PushOneCommit.Result r = createDraftChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).delete();
}
@@ -1305,11 +1305,6 @@ public class RevisionIT extends AbstractDaemonTest {
return push.to("refs/for/master");
}
private PushOneCommit.Result createDraft() throws Exception {
PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
return push.to("refs/drafts/master");
}
private RevisionApi current(PushOneCommit.Result r) throws Exception {
return gApi.changes().id(r.getChangeId()).current();
}

14
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@@ -622,21 +622,23 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
@Test
public void pushForMasterAsDraft() throws Exception {
// create draft by pushing to 'refs/drafts/'
// create draft by pushing to 'refs/drafts/' will get a private change.
PushOneCommit.Result r = pushTo("refs/drafts/master");
r.assertOkStatus();
r.assertChange(Change.Status.DRAFT, null);
r.assertChange(Change.Status.NEW, null);
// create draft by using 'draft' option
assertThat(gApi.changes().id(r.getChangeId()).get().isPrivate).isTrue();
// create draft by using 'draft' option will get a private change, too.
r = pushTo("refs/for/master%draft");
r.assertOkStatus();
r.assertChange(Change.Status.DRAFT, null);
r.assertChange(Change.Status.NEW, null);
assertThat(gApi.changes().id(r.getChangeId()).get().isPrivate).isTrue();
}
@Test
public void publishDraftChangeByPushingNonDraftPatchSet() throws Exception {
// create draft change
PushOneCommit.Result r = pushTo("refs/drafts/master");
PushOneCommit.Result r = createDraftChange();
r.assertOkStatus();
r.assertChange(Change.Status.DRAFT, null);

3
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
View File

@@ -323,8 +323,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
public void uploadPackDraftRefs() throws Exception {
allow("refs/heads/*", Permission.READ, REGISTERED_USERS);
PushOneCommit.Result br =
pushFactory.create(db, admin.getIdent(), testRepo).to("refs/drafts/master");
PushOneCommit.Result br = createDraftChange();
br.assertOkStatus();
Change.Id c5 = br.getChange().getId();
String r5 = changeRefPrefix(c5);

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java
View File

@@ -172,7 +172,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
@Test
public void submitOnPushingDraft_Error() throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master%draft,submit");
r.assertErrorStatus("cannot submit draft");
r.assertErrorStatus();
}
@Test

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
View File

@@ -551,7 +551,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
@Test
public void submitDraftPatchSet() throws Exception {
PushOneCommit.Result change = createChange();
PushOneCommit.Result draft = amendChangeAsDraft(change.getChangeId());
PushOneCommit.Result draft = amendChangeAndMarkPatchSetAsDraft(change.getChangeId());
Change.Id num = draft.getChange().getId();
submitWithConflict(

38
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
View File

@@ -22,7 +22,6 @@ import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVI
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.extensions.api.changes.ActionVisitor;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -43,8 +42,6 @@ import java.util.EnumSet;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.junit.After;
import org.junit.Before;
@@ -151,7 +148,7 @@ public class ActionsIT extends AbstractDaemonTest {
String etag1 = getETag(change);
setApiUser(admin);
String draft = createDraftWithTopic().getChangeId();
String draft = createDraftChange("topic").getChangeId();
approve(draft);
setApiUser(user);
@@ -225,7 +222,7 @@ public class ActionsIT extends AbstractDaemonTest {
// create another change with the same topic
String changeId2 =
createChangeWithTopic(testRepo, "foo2", "touching b", "b.txt", "real content")
createChangeWithTopic(testRepo, "topic", "touching b", "b.txt", "real content")
.getChangeId();
int changeNum2 = gApi.changes().id(changeId2).info()._number;
approve(changeId2);
@@ -477,35 +474,4 @@ public class ActionsIT extends AbstractDaemonTest {
assertThat(actions).containsKey("description");
assertThat(actions).containsKey("rebase");
}
private PushOneCommit.Result createCommitAndPush(
TestRepository<InMemoryRepository> repo,
String ref,
String commitMsg,
String fileName,
String content)
throws Exception {
return pushFactory.create(db, admin.getIdent(), repo, commitMsg, fileName, content).to(ref);
}
private PushOneCommit.Result createChangeWithTopic(
TestRepository<InMemoryRepository> repo,
String topic,
String commitMsg,
String fileName,
String content)
throws Exception {
assertThat(topic).isNotEmpty();
return createCommitAndPush(
repo, "refs/for/master/" + name(topic), commitMsg, fileName, content);
}
private PushOneCommit.Result createChangeWithTopic() throws Exception {
return createChangeWithTopic(testRepo, "foo2", "a message", "a.txt", "content\n");
}
private PushOneCommit.Result createDraftWithTopic() throws Exception {
return createCommitAndPush(
testRepo, "refs/drafts/master/" + name("foo2"), "a message", "a.txt", "content\n");
}
}

21
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
View File

@@ -26,6 +26,7 @@ import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.RestResponse;
@@ -43,7 +44,6 @@ import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.MergeInput;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -51,13 +51,11 @@ import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
import com.google.gerrit.server.git.ChangeAlreadyMergedException;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gerrit.testutil.FakeEmailSender.Message;
import com.google.gerrit.testutil.TestTimeUtil;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
@@ -69,11 +67,6 @@ import org.junit.BeforeClass;
import org.junit.Test;
public class CreateChangeIT extends AbstractDaemonTest {
@ConfigSuite.Config
public static Config allowDraftsDisabled() {
return allowDraftsDisabledConfig();
}
@BeforeClass
public static void setTimeForTesting() {
TestTimeUtil.resetWithClockStep(1, SECONDS);
@@ -135,8 +128,8 @@ public class CreateChangeIT extends AbstractDaemonTest {
@Test
public void createNewChangeSignedOffByFooter() throws Exception {
assume().that(isAllowDrafts()).isTrue();
setSignedOffByFooter();
ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
String message = info.revisions.get(info.currentRevision).commit.message;
assertThat(message)
@@ -146,16 +139,10 @@ public class CreateChangeIT extends AbstractDaemonTest {
}
@Test
public void createNewDraftChange() throws Exception {
assume().that(isAllowDrafts()).isTrue();
assertCreateSucceeds(newChangeInput(ChangeStatus.DRAFT));
}
@Test
@GerritConfig(name = "change.allowDrafts", value = "true")
public void createNewDraftChangeNotAllowed() throws Exception {
assume().that(isAllowDrafts()).isFalse();
ChangeInput ci = newChangeInput(ChangeStatus.DRAFT);
assertCreateFails(ci, MethodNotAllowedException.class, "draft workflow is disabled");
assertCreateFails(ci, BadRequestException.class, "unsupported change status");
}
@Test

45
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
View File

@@ -21,7 +21,6 @@ import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -40,8 +39,6 @@ import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import java.util.HashMap;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
@@ -189,14 +186,9 @@ public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
@Test
public void deleteCurrentDraftPatchSetWhenPreviousPatchSetDoesNotExist() throws Exception {
PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
String changeId = push.to("refs/for/master").getChangeId();
pushFactory
.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT, "b.txt", "foo", changeId)
.to("refs/drafts/master");
pushFactory
.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT, "b.txt", "bar", changeId)
.to("refs/drafts/master");
String changeId = createChange().getChangeId();
amendChangeAndMarkPatchSetAsDraft(changeId);
amendChangeAndMarkPatchSetAsDraft(changeId);
deletePatchSet(changeId, 2);
deletePatchSet(changeId, 3);
@@ -209,20 +201,13 @@ public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
@Test
public void deleteDraftPatchSetAndPushNewDraftPatchSet() throws Exception {
String ref = "refs/drafts/master";
// Clone repository
TestRepository<InMemoryRepository> testRepo = cloneProject(project, admin);
// Create change
PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
PushOneCommit.Result r1 = push.to(ref);
r1.assertOkStatus();
PushOneCommit.Result r1 = createDraftChange();
String changeId = r1.getChangeId();
String revPs1 = r1.getChange().currentPatchSet().getRevision().get();
// Push draft patch set
PushOneCommit.Result r2 = amendChange(r1.getChangeId(), ref, admin, testRepo);
r2.assertOkStatus();
PushOneCommit.Result r2 = amendChangeAndMarkPatchSetAsDraft(changeId);
String revPs2 = r2.getChange().currentPatchSet().getRevision().get();
assertThat(gApi.changes().id(r1.getChange().getId().get()).get().currentRevision)
@@ -235,8 +220,7 @@ public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
.isEqualTo(revPs1);
// Push new draft patch set
PushOneCommit.Result r3 = amendChange(r1.getChangeId(), ref, admin, testRepo);
r3.assertOkStatus();
amendChangeAndMarkPatchSetAsDraft(changeId);
String revPs3 = r2.getChange().currentPatchSet().getRevision().get();
assertThat(gApi.changes().id(r1.getChange().getId().get()).get().currentRevision)
@@ -259,21 +243,6 @@ public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
}
}
private String createDraftChangeWith2PS() throws Exception {
PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
Result result = push.to("refs/drafts/master");
push =
pushFactory.create(
db,
admin.getIdent(),
testRepo,
PushOneCommit.SUBJECT,
"b.txt",
"4711",
result.getChangeId());
return push.to("refs/drafts/master").getChangeId();
}
private PatchSet getCurrentPatchSet(String changeId) throws Exception {
return getChange(changeId).currentPatchSet();
}

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

@@ -29,6 +29,8 @@ import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.EditInfoSubject;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
@@ -38,6 +40,7 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.testutil.ConfigSuite;
import java.util.Collection;
import java.util.Optional;
import org.eclipse.jgit.lib.Config;
import org.junit.Test;
@@ -49,7 +52,7 @@ public class DraftChangeIT extends AbstractDaemonTest {
@Test
public void forceCreateAndPublishDraftChangeWhenAllowDraftsDisabled() throws Exception {
PushOneCommit.Result result = forceDraftChange();
PushOneCommit.Result result = createDraftChange();
result.assertOkStatus();
String changeId = result.getChangeId();
String triplet = project.get() + "~master~" + changeId;
@@ -105,7 +108,7 @@ public class DraftChangeIT extends AbstractDaemonTest {
pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
Change.Id id = changeResult.getChange().getId();
markChangeAsDraft(id);
setDraftStatusOfPatchSetsOfChange(id, true);
setDraftStatusOfPatchSets(id, true);
String changeId = changeResult.getChangeId();
exception.expect(MethodNotAllowedException.class);
@@ -125,7 +128,7 @@ public class DraftChangeIT extends AbstractDaemonTest {
pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
Change.Id id = changeResult.getChange().getId();
markChangeAsDraft(id);
setDraftStatusOfPatchSetsOfChange(id, true);
setDraftStatusOfPatchSets(id, true);
String changeId = changeResult.getChangeId();
@@ -151,7 +154,7 @@ public class DraftChangeIT extends AbstractDaemonTest {
PushOneCommit.Result changeResult = createDraftChange();
Change.Id id = changeResult.getChange().getId();
setDraftStatusOfPatchSetsOfChange(id, false);
setDraftStatusOfPatchSets(id, false);
String changeId = changeResult.getChangeId();
exception.expect(ResourceConflictException.class);
@@ -194,8 +197,8 @@ public class DraftChangeIT extends AbstractDaemonTest {
@Test
public void createDraftChangeWhenDraftsNotAllowed() throws Exception {
assume().that(isAllowDrafts()).isFalse();
PushOneCommit.Result r = createDraftChange();
r.assertErrorStatus("draft workflow is disabled");
PushOneCommit.Result r = pushTo("refs/drafts/master");
r.assertErrorStatus();
}
@Test
@@ -228,6 +231,53 @@ public class DraftChangeIT extends AbstractDaemonTest {
assertThat(label.all.get(0).value).isEqualTo(1);
}
@Test
public void pushWithDraftOptionGetsPrivateChange() throws Exception {
assume().that(isAllowDrafts()).isTrue();
PushOneCommit.Result result = createChange("refs/drafts/master");
String changeId = result.getChangeId();
ChangeInfo changeInfo = gApi.changes().id(changeId).get();
assertThat(changeInfo.status).isEqualTo(ChangeStatus.NEW);
assertThat(changeInfo.isPrivate).isEqualTo(true);
assertThat(changeInfo.revisions).hasSize(1);
assertThat(Iterables.getOnlyElement(changeInfo.revisions.values()).draft).isNull();
}
@Test
public void pushWithDraftOptionToExistingNewChangeGetsChangeEdit() throws Exception {
assume().that(isAllowDrafts()).isTrue();
pushWithDraftOptionToExistingChangeGetsChangeEdit(createChange().getChangeId());
}
@Test
public void pushWithDraftOptionToExistingDraftChangeGetsChangeEdit() throws Exception {
assume().that(isAllowDrafts()).isTrue();
pushWithDraftOptionToExistingChangeGetsChangeEdit(createDraftChange().getChangeId());
}
private void pushWithDraftOptionToExistingChangeGetsChangeEdit(String changeId) throws Exception {
Optional<EditInfo> edit = getEdit(changeId);
EditInfoSubject.assertThat(edit).isAbsent();
ChangeInfo changeInfo = gApi.changes().id(changeId).get();
ChangeStatus originalChangeStatus = changeInfo.status;
Boolean originalPatchSetStatus = Iterables.getOnlyElement(changeInfo.revisions.values()).draft;
PushOneCommit.Result result = amendChange(changeId, "refs/drafts/master");
result.assertOkStatus();
changeInfo = gApi.changes().id(changeId).get();
assertThat(changeInfo.status).isEqualTo(originalChangeStatus);
assertThat(changeInfo.isPrivate).isNull();
assertThat(changeInfo.revisions).hasSize(1);
assertThat(Iterables.getOnlyElement(changeInfo.revisions.values()).draft)
.isEqualTo(originalPatchSetStatus);
edit = getEdit(changeId);
EditInfoSubject.assertThat(edit).isPresent();
}
private static RestResponse deleteChange(String changeId, RestSession s) throws Exception {
return s.delete("/changes/" + changeId);
}

41
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
View File

@@ -29,6 +29,7 @@ import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.testutil.ConfigSuite;
import java.util.EnumSet;
@@ -156,14 +157,8 @@ public class SubmittedTogetherIT extends AbstractDaemonTest {
@Test
public void hiddenDraftInTopic() throws Exception {
RevCommit initialHead = getRemoteHead();
RevCommit a = commitBuilder().add("a", "1").message("change 1").create();
pushHead(testRepo, "refs/for/master/" + name("topic"), false);
String id1 = getChangeId(a);
testRepo.reset(initialHead);
commitBuilder().add("b", "2").message("invisible change").create();
pushHead(testRepo, "refs/drafts/master/" + name("topic"), false);
String id1 = createChange("subject", "a", "1", "topic").getChangeId();
createDraftChange("topic");
setApiUser(user);
SubmittedTogetherInfo result =
@@ -181,14 +176,8 @@ public class SubmittedTogetherIT extends AbstractDaemonTest {
@Test
public void hiddenDraftInTopicOldApi() throws Exception {
RevCommit initialHead = getRemoteHead();
RevCommit a = commitBuilder().add("a", "1").message("change 1").create();
pushHead(testRepo, "refs/for/master/" + name("topic"), false);
String id1 = getChangeId(a);
testRepo.reset(initialHead);
commitBuilder().add("b", "2").message("invisible change").create();
pushHead(testRepo, "refs/drafts/master/" + name("topic"), false);
String id1 = createChange("subject", "a", "1", "topic").getChangeId();
createDraftChange("topic");
setApiUser(user);
if (isSubmitWholeTopicEnabled()) {
@@ -209,18 +198,16 @@ public class SubmittedTogetherIT extends AbstractDaemonTest {
String id1 = getChangeId(a1);
testRepo.reset(initialHead);
RevCommit parent = commitBuilder().message("parent").create();
pushHead(testRepo, "refs/for/master", false);
String parentId = getChangeId(parent);
String parentId = createChange().getChangeId();
// TODO(jrn): use insertChangeId(id1) once jgit TestRepository accepts
// the leading "I".
// TODO(jrn): use insertChangeId(id1) once jgit TestRepository accepts the leading "I".
commitBuilder()
.insertChangeId(id1.substring(1))
.add("a", "2")
.message("draft patch set on change 1")
.create();
pushHead(testRepo, "refs/drafts/master/" + name("topic"), false);
pushHead(testRepo, "refs/for/master/" + name("topic"), false);
setCurrentPatchSetAsDraft(new Change.Id(gApi.changes().id(id1).get()._number));
testRepo.reset(initialHead);
RevCommit b = commitBuilder().message("change with same topic").create();
@@ -243,16 +230,12 @@ public class SubmittedTogetherIT extends AbstractDaemonTest {
@Test
public void doNotRevealVisibleAncestorOfHiddenDraft() throws Exception {
RevCommit initialHead = getRemoteHead();
commitBuilder().message("parent").create();
pushHead(testRepo, "refs/for/master", false);
createChange().getChangeId();
commitBuilder().message("draft").create();
pushHead(testRepo, "refs/drafts/master/" + name("topic"), false);
createDraftChange("topic");
testRepo.reset(initialHead);
RevCommit change = commitBuilder().message("same topic").create();
pushHead(testRepo, "refs/for/master/" + name("topic"), false);
String id = getChangeId(change);
String id = createChange("subject", "b", "1", "topic").getChangeId();
setApiUser(user);
SubmittedTogetherInfo result =

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

@@ -42,6 +42,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -485,23 +486,23 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
// TODO(dborowitz): Re-enable these assertions once we fix auto-rebuilding
// in the BatchUpdate path.
//// As an implementation detail, change wasn't actually rebuilt inside the
//// BatchUpdate transaction, but it was rebuilt during read for the
//// subsequent reindex. Thus it's impossible to actually observe an
//// out-of-date state in the caller.
//assertChangeUpToDate(true, id);
// As an implementation detail, change wasn't actually rebuilt inside the
// BatchUpdate transaction, but it was rebuilt during read for the
// subsequent reindex. Thus it's impossible to actually observe an
// out-of-date state in the caller.
// assertChangeUpToDate(true, id);
//// Check that the bundles are equal.
//ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
//ChangeBundle actual = ChangeBundle.fromNotes(commentsUtil, notes);
//ChangeBundle expected = bundleReader.fromReviewDb(getUnwrappedDb(), id);
//assertThat(actual.differencesFrom(expected)).isEmpty();
//assertThat(
// Check that the bundles are equal.
// ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
// ChangeBundle actual = ChangeBundle.fromNotes(commentsUtil, notes);
// ChangeBundle expected = bundleReader.fromReviewDb(getUnwrappedDb(), id);
// assertThat(actual.differencesFrom(expected)).isEmpty();
// assertThat(
// Iterables.transform(
// notes.getChangeMessages(),
// ChangeMessage::getMessage))
// .contains(msg);
//assertThat(actual.getChange().getTopic()).isEqualTo(topic);
// assertThat(actual.getChange().getTopic()).isEqualTo(topic);
}
@Test
@@ -817,28 +818,16 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
@Test
public void deleteDraftPS1WithNoOtherEntities() throws Exception {
PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
PushOneCommit.Result r = push.to("refs/drafts/master");
push =
pushFactory.create(
db,
admin.getIdent(),
testRepo,
PushOneCommit.SUBJECT,
"b.txt",
"4711",
r.getChangeId());
r = push.to("refs/drafts/master");
PatchSet.Id psId = r.getPatchSetId();
Change.Id id = psId.getParentKey();
gApi.changes().id(r.getChangeId()).revision(1).delete();
String r = createDraftChangeWith2PS();
gApi.changes().id(r).revision(1).delete();
ChangeInfo changeInfo = get(r);
Change.Id id = new Change.Id(changeInfo._number);
checker.rebuildAndCheckChanges(id);
setNotesMigration(true, true);
ChangeNotes notes = notesFactory.create(db, project, id);
assertThat(notes.getPatchSets().keySet()).containsExactly(psId);
assertThat(notes.getPatchSets().keySet()).hasSize(1);
}
@Test

130
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
View File

@@ -93,72 +93,6 @@ public class ProjectWatchIT extends AbstractDaemonTest {
assertThat(m.body()).contains("Gerrit-PatchSet: 2\n");
}
@Test
public void noNotificationForDraftChangesForWatchersInNotifyConfig() throws Exception {
Address addr = new Address("Watcher", "watcher@example.com");
NotifyConfig nc = new NotifyConfig();
nc.addEmail(addr);
nc.setName("team");
nc.setHeader(NotifyConfig.Header.TO);
nc.setTypes(EnumSet.of(NotifyType.NEW_CHANGES, NotifyType.ALL_COMMENTS));
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
cfg.putNotifyConfig("team", nc);
saveProjectConfig(project, cfg);
PushOneCommit.Result r =
pushFactory
.create(db, admin.getIdent(), testRepo, "draft change", "a", "a1")
.to("refs/for/master%draft");
r.assertOkStatus();
assertThat(sender.getMessages()).isEmpty();
setApiUser(admin);
ReviewInput in = new ReviewInput();
in.message = "comment";
gApi.changes().id(r.getChangeId()).current().review(in);
assertThat(sender.getMessages()).isEmpty();
}
@Test
public void noNotificationForDraftPatchSetsForWatchersInNotifyConfig() throws Exception {
Address addr = new Address("Watcher", "watcher@example.com");
NotifyConfig nc = new NotifyConfig();
nc.addEmail(addr);
nc.setName("team");
nc.setHeader(NotifyConfig.Header.TO);
nc.setTypes(EnumSet.of(NotifyType.NEW_PATCHSETS, NotifyType.ALL_COMMENTS));
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
cfg.putNotifyConfig("team", nc);
saveProjectConfig(project, cfg);
PushOneCommit.Result r =
pushFactory
.create(db, admin.getIdent(), testRepo, "subject", "a", "a1")
.to("refs/for/master");
r.assertOkStatus();
sender.clear();
r =
pushFactory
.create(db, admin.getIdent(), testRepo, "subject", "a", "a2", r.getChangeId())
.to("refs/for/master%draft");
r.assertOkStatus();
assertThat(sender.getMessages()).isEmpty();
setApiUser(admin);
ReviewInput in = new ReviewInput();
in.message = "comment";
gApi.changes().id(r.getChangeId()).current().review(in);
assertThat(sender.getMessages()).isEmpty();
}
@Test
public void noNotificationForPrivateChangesForWatchersInNotifyConfig() throws Exception {
Address addr = new Address("Watcher", "watcher@example.com");
@@ -528,70 +462,6 @@ public class ProjectWatchIT extends AbstractDaemonTest {
assertThat(sender.getMessages()).isEmpty();
}
@Test
public void watchProjectNoNotificationForDraftChange() throws Exception {
// watch project
String watchedProject = createProject("watchedProject").get();
setApiUser(user);
watch(watchedProject);
// push a draft change to watched project -> should not trigger email notification
setApiUser(admin);
TestRepository<InMemoryRepository> watchedRepo =
cloneProject(new Project.NameKey(watchedProject), admin);
PushOneCommit.Result r =
pushFactory
.create(db, admin.getIdent(), watchedRepo, "draft change", "a", "a1")
.to("refs/for/master%draft");
r.assertOkStatus();
// assert email notification
assertThat(sender.getMessages()).isEmpty();
}
@Test
public void watchProjectNotifyOnDraftChange() throws Exception {
String watchedProject = createProject("watchedProject").get();
// create group that can view all drafts
GroupInfo groupThatCanViewDrafts = gApi.groups().create("groupThatCanViewDrafts").get();
grant(
new Project.NameKey(watchedProject),
"refs/*",
Permission.VIEW_DRAFTS,
false,
new AccountGroup.UUID(groupThatCanViewDrafts.id));
// watch project as user that can't view drafts
setApiUser(user);
watch(watchedProject);
// watch project as user that can view all drafts
TestAccount userThatCanViewDrafts =
accountCreator.create("user2", "user2@test.com", "User2", groupThatCanViewDrafts.name);
setApiUser(userThatCanViewDrafts);
watch(watchedProject);
// push a draft change to watched project -> should trigger email notification for
// userThatCanViewDrafts, but not for user
setApiUser(admin);
TestRepository<InMemoryRepository> watchedRepo =
cloneProject(new Project.NameKey(watchedProject), admin);
PushOneCommit.Result r =
pushFactory
.create(db, admin.getIdent(), watchedRepo, "TRIGGER", "a", "a1")
.to("refs/for/master%draft");
r.assertOkStatus();
// assert email notification
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
Message m = messages.get(0);
assertThat(m.rcpt()).containsExactly(userThatCanViewDrafts.emailAddress);
assertThat(m.body()).contains("Change subject: TRIGGER\n");
assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
}
@Test
public void watchProjectNoNotificationForIgnoredChange() throws Exception {
// watch project

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java
View File

@@ -292,7 +292,7 @@ public class QueryIT extends AbstractDaemonTest {
@Test
public void queryWithNonVisibleCurrentPatchSet() throws Exception {
String changeId = createChange().getChangeId();
amendChangeAsDraft(changeId);
amendChangeAndMarkPatchSetAsDraft(changeId);
String query = "--current-patch-set --patch-sets " + changeId;
List<ChangeAttribute> changes = executeSuccessfulQuery(query);
assertThat(changes.size()).isEqualTo(1);