Merge "Enable PolyGerrit UI per default"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index e5073b2..a258b32 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -1767,13 +1767,19 @@
HTTP/1.1 204 No Content
----
-[[delete-draft-change]]
-=== Delete Draft Change
+[[delete-change]]
+=== Delete Change
--
'DELETE /changes/link:#change-id[\{change-id\}]'
--
-Deletes a draft change.
+Deletes a change.
+
+New or abandoned changes can only be deleted by administrators. The deletion of
+merged changes isn't supported at the moment. Draft changes can only be deleted
+by their owner or other users who have the permissions to view and delete
+drafts. If the draft workflow is disabled, only administrators with those
+permissions may delete draft changes.
.Request
----
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/TestAccount.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/TestAccount.java
index c1a815a..63e0fa7 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/TestAccount.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/TestAccount.java
@@ -14,7 +14,8 @@
package com.google.gerrit.acceptance;
-import com.google.common.collect.FluentIterable;
+import static java.util.stream.Collectors.toList;
+
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.mail.Address;
@@ -24,22 +25,22 @@
import java.io.ByteArrayOutputStream;
import java.util.Arrays;
+import java.util.List;
public class TestAccount {
- public static FluentIterable<Account.Id> ids(
- Iterable<TestAccount> accounts) {
- return FluentIterable.from(accounts).transform(a -> a.id);
+ public static List<Account.Id> ids(List<TestAccount> accounts) {
+ return accounts.stream().map(a -> a.id).collect(toList());
}
- public static FluentIterable<Account.Id> ids(TestAccount... accounts) {
+ public static List<Account.Id> ids(TestAccount... accounts) {
return ids(Arrays.asList(accounts));
}
- public static FluentIterable<String> names(Iterable<TestAccount> accounts) {
- return FluentIterable.from(accounts).transform(a -> a.fullName);
+ public static List<String> names(List<TestAccount> accounts) {
+ return accounts.stream().map(a -> a.fullName).collect(toList());
}
- public static FluentIterable<String> names(TestAccount... accounts) {
+ public static List<String> names(TestAccount... accounts) {
return names(Arrays.asList(accounts));
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index d995be6..8af4098 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -68,6 +68,7 @@
import com.google.gerrit.extensions.common.MergePatchSetInput;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -398,7 +399,7 @@
}
@Test
- public void delete() throws Exception {
+ public void deleteDraftChange() throws Exception {
PushOneCommit.Result r = createChange("refs/drafts/master");
assertThat(query(r.getChangeId())).hasSize(1);
assertThat(info(r.getChangeId()).status).isEqualTo(ChangeStatus.DRAFT);
@@ -409,6 +410,110 @@
}
@Test
+ public void deleteNewChangeAsAdmin() throws Exception {
+ PushOneCommit.Result changeResult = createChange();
+ String changeId = changeResult.getChangeId();
+
+ gApi.changes()
+ .id(changeId)
+ .delete();
+
+ assertThat(query(changeId)).isEmpty();
+ }
+
+ @Test
+ @TestProjectInput(cloneAs = "user")
+ public void deleteNewChangeAsNormalUser() throws Exception {
+ PushOneCommit.Result changeResult =
+ pushFactory.create(db, user.getIdent(), testRepo)
+ .to("refs/for/master");
+ String changeId = changeResult.getChangeId();
+ Change.Id id = changeResult.getChange().getId();
+
+ setApiUser(user);
+ exception.expect(AuthException.class);
+ exception.expectMessage(String.format(
+ "Deleting change %s is not permitted", id));
+ gApi.changes()
+ .id(changeId)
+ .delete();
+ }
+
+ @Test
+ @TestProjectInput(cloneAs = "user")
+ public void deleteNewChangeOfAnotherUserAsAdmin() throws Exception {
+ PushOneCommit.Result changeResult =
+ pushFactory.create(db, user.getIdent(), testRepo)
+ .to("refs/for/master");
+ changeResult.assertOkStatus();
+ String changeId = changeResult.getChangeId();
+
+ setApiUser(admin);
+ gApi.changes()
+ .id(changeId)
+ .delete();
+
+ assertThat(query(changeId)).isEmpty();
+ }
+
+ @Test
+ @TestProjectInput(cloneAs = "user")
+ public void deleteAbandonedChangeAsNormalUser() throws Exception {
+ PushOneCommit.Result changeResult =
+ pushFactory.create(db, user.getIdent(), testRepo)
+ .to("refs/for/master");
+ String changeId = changeResult.getChangeId();
+ Change.Id id = changeResult.getChange().getId();
+
+ setApiUser(user);
+ gApi.changes()
+ .id(changeId)
+ .abandon();
+
+ exception.expect(AuthException.class);
+ exception.expectMessage(String.format(
+ "Deleting change %s is not permitted", id));
+ gApi.changes()
+ .id(changeId)
+ .delete();
+ }
+
+ @Test
+ @TestProjectInput(cloneAs = "user")
+ public void deleteAbandonedChangeOfAnotherUserAsAdmin() throws Exception {
+ PushOneCommit.Result changeResult =
+ pushFactory.create(db, user.getIdent(), testRepo)
+ .to("refs/for/master");
+ String changeId = changeResult.getChangeId();
+
+ gApi.changes()
+ .id(changeId)
+ .abandon();
+
+ gApi.changes()
+ .id(changeId)
+ .delete();
+
+ assertThat(query(changeId)).isEmpty();
+ }
+
+ @Test
+ public void deleteMergedChange() throws Exception {
+ PushOneCommit.Result changeResult = createChange();
+ String changeId = changeResult.getChangeId();
+ Change.Id id = changeResult.getChange().getId();
+
+ merge(changeResult);
+
+ exception.expect(MethodNotAllowedException.class);
+ exception.expectMessage(String.format(
+ "Deleting merged change %s is not allowed", id));
+ gApi.changes()
+ .id(changeId)
+ .delete();
+ }
+
+ @Test
public void rebaseUpToDateChange() throws Exception {
PushOneCommit.Result r = createChange();
exception.expect(ResourceConflictException.class);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 78c0cda..620b9ea 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -17,10 +17,9 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.api.group.GroupAssert.assertGroupInfo;
import static com.google.gerrit.acceptance.rest.account.AccountAssert.assertAccountInfos;
+import static java.util.stream.Collectors.toList;
-import com.google.common.collect.FluentIterable;
import com.google.common.collect.Iterables;
-import com.google.common.collect.Ordering;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.TestAccount;
@@ -46,9 +45,12 @@
import java.sql.Timestamp;
import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.stream.StreamSupport;
@NoHttpd
public class GroupsIT extends AbstractDaemonTest {
@@ -397,10 +399,12 @@
@Test
public void testListAllGroups() throws Exception {
- List<String> expectedGroups = FluentIterable
- .from(groupCache.all())
- .transform(AccountGroup::getName)
- .toSortedList(Ordering.natural());
+ List<String> expectedGroups =
+ StreamSupport.stream(groupCache.all().spliterator(), false)
+ .map(a -> a.getName())
+ .collect(toList());
+ Collections.sort(expectedGroups, Comparator.naturalOrder());
+
assertThat(expectedGroups.size()).isAtLeast(2);
assertThat(gApi.groups().list().getAsMap().keySet())
.containsExactlyElementsIn(expectedGroups).inOrder();
@@ -504,7 +508,7 @@
throws Exception {
assertMembers(
gApi.groups().id(group).members(),
- TestAccount.names(expectedMembers).toArray(String.class));
+ TestAccount.names(expectedMembers).stream().toArray(String[]::new));
assertAccountInfos(
Arrays.asList(expectedMembers),
gApi.groups().id(group).members());
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
index 2a32abe..eb6e433 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
@@ -22,20 +22,35 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.RestSession;
+import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.client.ListChangesOption;
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.LabelInfo;
+import com.google.gerrit.extensions.common.RevisionInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.git.BatchUpdate;
+import com.google.gerrit.server.notedb.PatchSetState;
import com.google.gerrit.testutil.ConfigSuite;
+import com.google.inject.Inject;
import org.eclipse.jgit.lib.Config;
import org.junit.Test;
import java.util.Collection;
+import java.util.EnumSet;
+import java.util.List;
+import java.util.stream.Collectors;
public class DraftChangeIT extends AbstractDaemonTest {
@ConfigSuite.Config
@@ -43,20 +58,8 @@
return allowDraftsDisabledConfig();
}
- @Test
- public void deleteChange() throws Exception {
- PushOneCommit.Result result = createChange();
- result.assertOkStatus();
- String changeId = result.getChangeId();
- String triplet = project.get() + "~master~" + changeId;
- ChangeInfo c = get(triplet);
- assertThat(c.id).isEqualTo(triplet);
- assertThat(c.status).isEqualTo(ChangeStatus.NEW);
- RestResponse response = deleteChange(changeId, adminRestSession);
- assertThat(response.getEntityContent())
- .isEqualTo("Change is not a draft: " + c._number);
- response.assertConflict();
- }
+ @Inject
+ private BatchUpdate.Factory updateFactory;
@Test
public void deleteDraftChange() throws Exception {
@@ -75,6 +78,104 @@
}
@Test
+ public void deleteDraftChangeOfAnotherUser() throws Exception {
+ assume().that(isAllowDrafts()).isTrue();
+ PushOneCommit.Result changeResult = createDraftChange();
+ changeResult.assertOkStatus();
+ String changeId = changeResult.getChangeId();
+ Change.Id id = changeResult.getChange().getId();
+
+ // The user needs to be able to see the draft change (which reviewers can).
+ gApi.changes()
+ .id(changeId)
+ .addReviewer(user.fullName);
+
+ setApiUser(user);
+ exception.expect(AuthException.class);
+ exception.expectMessage(String.format(
+ "Deleting change %s is not permitted", id));
+ gApi.changes()
+ .id(changeId)
+ .delete();
+ }
+
+ @Test
+ @TestProjectInput(cloneAs = "user")
+ public void deleteDraftChangeWhenDraftsNotAllowedAsNormalUser()
+ throws Exception {
+ assume().that(isAllowDrafts()).isFalse();
+
+ setApiUser(user);
+ // We can't create a draft change while the draft workflow is disabled.
+ // For this reason, we create a normal change and modify the database.
+ PushOneCommit.Result changeResult =
+ pushFactory.create(db, user.getIdent(), testRepo)
+ .to("refs/for/master");
+ Change.Id id = changeResult.getChange().getId();
+ markChangeAsDraft(id);
+ setDraftStatusOfPatchSetsOfChange(id, true);
+
+ String changeId = changeResult.getChangeId();
+ exception.expect(MethodNotAllowedException.class);
+ exception.expectMessage("Draft workflow is disabled");
+ gApi.changes()
+ .id(changeId)
+ .delete();
+ }
+
+ @Test
+ @TestProjectInput(cloneAs = "user")
+ public void deleteDraftChangeWhenDraftsNotAllowedAsAdmin() throws Exception {
+ assume().that(isAllowDrafts()).isFalse();
+
+ setApiUser(user);
+ // We can't create a draft change while the draft workflow is disabled.
+ // For this reason, we create a normal change and modify the database.
+ PushOneCommit.Result changeResult =
+ pushFactory.create(db, user.getIdent(), testRepo)
+ .to("refs/for/master");
+ Change.Id id = changeResult.getChange().getId();
+ markChangeAsDraft(id);
+ setDraftStatusOfPatchSetsOfChange(id, true);
+
+ String changeId = changeResult.getChangeId();
+
+ // Grant those permissions to admins.
+ grant(Permission.VIEW_DRAFTS, project, "refs/*");
+ grant(Permission.DELETE_DRAFTS, project, "refs/*");
+
+ try {
+ setApiUser(admin);
+ gApi.changes()
+ .id(changeId)
+ .delete();
+ } finally {
+ removePermission(Permission.DELETE_DRAFTS, project, "refs/*");
+ removePermission(Permission.VIEW_DRAFTS, project, "refs/*");
+ }
+
+ setApiUser(user);
+ assertThat(query(changeId)).isEmpty();
+ }
+
+ @Test
+ public void deleteDraftChangeWithNonDraftPatchSet() throws Exception {
+ assume().that(isAllowDrafts()).isTrue();
+
+ PushOneCommit.Result changeResult = createDraftChange();
+ Change.Id id = changeResult.getChange().getId();
+ setDraftStatusOfPatchSetsOfChange(id, false);
+
+ String changeId = changeResult.getChangeId();
+ exception.expect(ResourceConflictException.class);
+ exception.expectMessage(String.format(
+ "Cannot delete draft change %s: patch set 1 is not a draft", id));
+ gApi.changes()
+ .id(changeId)
+ .delete();
+ }
+
+ @Test
public void publishDraftChange() throws Exception {
assume().that(isAllowDrafts()).isTrue();
PushOneCommit.Result result = createDraftChange();
@@ -160,4 +261,90 @@
+ patchSet.getRevision().get()
+ "/publish");
}
+
+ private void markChangeAsDraft(Change.Id id) throws Exception {
+ try (BatchUpdate batchUpdate = updateFactory
+ .create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
+ batchUpdate.addOp(id, new MarkChangeAsDraftUpdateOp());
+ batchUpdate.execute();
+ }
+
+ ChangeStatus changeStatus = gApi.changes()
+ .id(id.get())
+ .get()
+ .status;
+ assertThat(changeStatus).isEqualTo(ChangeStatus.DRAFT);
+ }
+
+ private void setDraftStatusOfPatchSetsOfChange(Change.Id id,
+ boolean draftStatus) throws Exception {
+ try (BatchUpdate batchUpdate = updateFactory
+ .create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
+ batchUpdate.addOp(id, new DraftStatusOfPatchSetsUpdateOp(draftStatus));
+ batchUpdate.execute();
+ }
+
+ Boolean expectedDraftStatus = draftStatus ? Boolean.TRUE : null;
+ List<Boolean> patchSetDraftStatuses = getPatchSetDraftStatuses(id);
+ patchSetDraftStatuses.forEach(status ->
+ assertThat(status).isEqualTo(expectedDraftStatus));
+ }
+
+ private List<Boolean> getPatchSetDraftStatuses(Change.Id id)
+ throws Exception {
+ Collection<RevisionInfo> revisionInfos = gApi.changes()
+ .id(id.get())
+ .get(EnumSet.of(ListChangesOption.ALL_REVISIONS))
+ .revisions
+ .values();
+ return revisionInfos.stream()
+ .map(revisionInfo -> revisionInfo.draft)
+ .collect(Collectors.toList());
+ }
+
+ private class MarkChangeAsDraftUpdateOp extends BatchUpdate.Op {
+ @Override
+ public boolean updateChange(BatchUpdate.ChangeContext ctx)
+ throws Exception {
+ Change change = ctx.getChange();
+
+ // Change status in database.
+ change.setStatus(Change.Status.DRAFT);
+
+ // Change status in NoteDb.
+ PatchSet.Id currentPatchSetId = change.currentPatchSetId();
+ ctx.getUpdate(currentPatchSetId).setStatus(Change.Status.DRAFT);
+
+ return true;
+ }
+ }
+
+ private class DraftStatusOfPatchSetsUpdateOp extends BatchUpdate.Op {
+ private final boolean draftStatus;
+
+ DraftStatusOfPatchSetsUpdateOp(boolean draftStatus) {
+ this.draftStatus = draftStatus;
+ }
+
+ @Override
+ public boolean updateChange(BatchUpdate.ChangeContext ctx)
+ throws Exception {
+ Collection<PatchSet> patchSets = psUtil.byChange(db, ctx.getNotes());
+
+ // Change status in database.
+ patchSets.forEach(patchSet -> patchSet.setDraft(draftStatus));
+ db.patchSets().update(patchSets);
+
+ // Change status in NoteDb.
+ PatchSetState patchSetState = draftStatus ? PatchSetState.DRAFT
+ : PatchSetState.PUBLISHED;
+ patchSets.stream()
+ .map(PatchSet::getId)
+ .map(ctx::getUpdate)
+ .forEach(changeUpdate ->
+ changeUpdate.setPatchSetState(patchSetState));
+
+ return true;
+ }
+ }
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
index bd5dd69..629ad97 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -112,7 +112,7 @@
void publish() throws RestApiException;
/**
- * Deletes a draft change.
+ * Deletes a change.
*/
void delete() throws RestApiException;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java
index 63de389..99f3b9f2 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java
@@ -51,6 +51,6 @@
String abandoned();
String deleteChangeEdit();
- String deleteDraftChange();
+ String deleteChange();
String deleteDraftRevision();
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties
index 5b4f18f..dd4760d 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties
@@ -34,5 +34,5 @@
deleteChangeEdit = Delete Change Edit?\n\
\n\
All changes made in the edit revision will be lost.
-deleteDraftChange = Delete Draft Change?
+deleteChange = Delete Change?
deleteDraftRevision = Delete Draft Revision?
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
index 436e0c3..fa3855e 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
@@ -495,15 +495,13 @@
}
private void initChangeAction(ChangeInfo info) {
- if (info.status() == Status.DRAFT) {
- NativeMap<ActionInfo> actions = info.hasActions()
- ? info.actions()
- : NativeMap.<ActionInfo> create();
- actions.copyKeysIntoChildren("id");
- if (actions.containsKey("/")) {
- deleteChange.setVisible(true);
- deleteChange.setTitle(actions.get("/").title());
- }
+ NativeMap<ActionInfo> actions = info.hasActions()
+ ? info.actions()
+ : NativeMap.create();
+ actions.copyKeysIntoChildren("id");
+ if (actions.containsKey("/")) {
+ deleteChange.setVisible(true);
+ deleteChange.setTitle(actions.get("/").title());
}
}
@@ -679,7 +677,7 @@
@UiHandler("deleteChange")
void onDeleteChange(@SuppressWarnings("unused") ClickEvent e) {
- if (Window.confirm(Resources.C.deleteDraftChange())) {
+ if (Window.confirm(Resources.C.deleteChange())) {
DraftActions.delete(changeId, publish, deleteRevision, deleteChange);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
index 31c70d9..a265160 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
@@ -45,7 +45,7 @@
import com.google.gerrit.server.change.Check;
import com.google.gerrit.server.change.CreateMergePatchSet;
import com.google.gerrit.server.change.DeleteAssignee;
-import com.google.gerrit.server.change.DeleteDraftChange;
+import com.google.gerrit.server.change.DeleteChange;
import com.google.gerrit.server.change.GetAssignee;
import com.google.gerrit.server.change.GetHashtags;
import com.google.gerrit.server.change.GetPastAssignees;
@@ -98,7 +98,7 @@
private final Provider<SubmittedTogether> submittedTogether;
private final PublishDraftPatchSet.CurrentRevision
publishDraftChange;
- private final DeleteDraftChange deleteDraftChange;
+ private final DeleteChange deleteChange;
private final GetTopic getTopic;
private final PutTopic putTopic;
private final PostReviewers postReviewers;
@@ -129,7 +129,7 @@
CreateMergePatchSet updateByMerge,
Provider<SubmittedTogether> submittedTogether,
PublishDraftPatchSet.CurrentRevision publishDraftChange,
- DeleteDraftChange deleteDraftChange,
+ DeleteChange deleteChange,
GetTopic getTopic,
PutTopic putTopic,
PostReviewers postReviewers,
@@ -159,7 +159,7 @@
this.updateByMerge = updateByMerge;
this.submittedTogether = submittedTogether;
this.publishDraftChange = publishDraftChange;
- this.deleteDraftChange = deleteDraftChange;
+ this.deleteChange = deleteChange;
this.getTopic = getTopic;
this.putTopic = putTopic;
this.postReviewers = postReviewers;
@@ -324,7 +324,7 @@
@Override
public void delete() throws RestApiException {
try {
- deleteDraftChange.apply(change, null);
+ deleteChange.apply(change, null);
} catch (UpdateException e) {
throw new RestApiException("Cannot delete change", e);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
index 30ed82f..20e5b9d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
@@ -658,7 +658,7 @@
public boolean updateChange(ChangeContext ctx)
throws OrmException, PatchSetInfoNotAvailableException {
// Delete dangling key references.
- ReviewDb db = DeleteDraftChangeOp.unwrap(ctx.getDb());
+ ReviewDb db = DeleteChangeOp.unwrap(ctx.getDb());
accountPatchReviewStore.get().clearReviewed(psId);
db.changeMessages().delete(
db.changeMessages().byChange(psId.getParentKey()));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java
similarity index 75%
rename from gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChange.java
rename to gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java
index a125272..18d7074 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java
@@ -22,10 +22,11 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.change.DeleteDraftChange.Input;
+import com.google.gerrit.server.change.DeleteChange.Input;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.UpdateException;
+import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -34,25 +35,25 @@
import org.eclipse.jgit.lib.Config;
@Singleton
-public class DeleteDraftChange implements
+public class DeleteChange implements
RestModifyView<ChangeResource, Input>, UiAction<ChangeResource> {
public static class Input {
}
private final Provider<ReviewDb> db;
private final BatchUpdate.Factory updateFactory;
- private final Provider<DeleteDraftChangeOp> opProvider;
+ private final Provider<DeleteChangeOp> opProvider;
private final boolean allowDrafts;
@Inject
- public DeleteDraftChange(Provider<ReviewDb> db,
+ public DeleteChange(Provider<ReviewDb> db,
BatchUpdate.Factory updateFactory,
- Provider<DeleteDraftChangeOp> opProvider,
+ Provider<DeleteChangeOp> opProvider,
@GerritServerConfig Config cfg) {
this.db = db;
this.updateFactory = updateFactory;
this.opProvider = opProvider;
- this.allowDrafts = DeleteDraftChangeOp.allowDrafts(cfg);
+ this.allowDrafts = DeleteChangeOp.allowDrafts(cfg);
}
@Override
@@ -71,14 +72,21 @@
@Override
public UiAction.Description getDescription(ChangeResource rsrc) {
try {
+ Change.Status status = rsrc.getChange().getStatus();
+ ChangeControl changeControl = rsrc.getControl();
+ boolean visible = isActionAllowed(changeControl, status)
+ && changeControl.canDelete(db.get(), status);
return new UiAction.Description()
.setLabel("Delete")
- .setTitle("Delete draft change " + rsrc.getId())
- .setVisible(allowDrafts
- && rsrc.getChange().getStatus() == Status.DRAFT
- && rsrc.getControl().canDeleteDraft(db.get()));
+ .setTitle("Delete change " + rsrc.getId())
+ .setVisible(visible);
} catch (OrmException e) {
throw new IllegalStateException(e);
}
}
+
+ private boolean isActionAllowed(ChangeControl changeControl,
+ Status status) {
+ return status != Status.DRAFT || allowDrafts || changeControl.isAdmin();
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java
similarity index 82%
rename from gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java
rename to gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java
index 4ed6a25..0e77a50 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java
@@ -44,7 +44,7 @@
import java.io.IOException;
import java.util.Collection;
-class DeleteDraftChangeOp extends BatchUpdate.Op {
+class DeleteChangeOp extends BatchUpdate.Op {
static boolean allowDrafts(Config cfg) {
return cfg.getBoolean("change", "allowDrafts", true);
}
@@ -68,7 +68,7 @@
private Change.Id id;
@Inject
- DeleteDraftChangeOp(PatchSetUtil psUtil,
+ DeleteChangeOp(PatchSetUtil psUtil,
StarredChangesUtil starredChangesUtil,
DynamicItem<AccountPatchReviewStore> accountPatchReviewStore,
@GerritServerConfig Config cfg) {
@@ -82,16 +82,18 @@
public boolean updateChange(ChangeContext ctx) throws RestApiException,
OrmException, IOException, NoSuchChangeException {
checkState(ctx.getOrder() == BatchUpdate.Order.DB_BEFORE_REPO,
- "must use DeleteDraftChangeOp with DB_BEFORE_REPO");
- checkState(id == null, "cannot reuse DeleteDraftChangeOp");
+ "must use DeleteChangeOp with DB_BEFORE_REPO");
+ checkState(id == null, "cannot reuse DeleteChangeOp");
id = ctx.getChange().getId();
Collection<PatchSet> patchSets = psUtil.byChange(ctx.getDb(),
ctx.getNotes());
ensureDeletable(ctx, id, patchSets);
- deleteChangeElementsFromDb(ctx, id);
+ // Cleaning up is only possible as long as the change and its elements are
+ // still part of the database.
cleanUpReferences(ctx, id, patchSets);
+ deleteChangeElementsFromDb(ctx, id);
ctx.deleteChange();
return true;
@@ -100,19 +102,25 @@
private void ensureDeletable(ChangeContext ctx, Change.Id id,
Collection<PatchSet> patchSets) throws ResourceConflictException,
MethodNotAllowedException, OrmException, AuthException {
- if (ctx.getChange().getStatus() != Change.Status.DRAFT) {
- throw new ResourceConflictException("Change is not a draft: " + id);
+ Change.Status status = ctx.getChange().getStatus();
+ if (status == Change.Status.MERGED) {
+ throw new MethodNotAllowedException("Deleting merged change " + id
+ + " is not allowed");
}
- if (!allowDrafts) {
- throw new MethodNotAllowedException("Draft workflow is disabled");
+
+ if (!ctx.getControl().canDelete(ctx.getDb(), status)) {
+ throw new AuthException("Deleting change " + id + " is not permitted");
}
- if (!ctx.getControl().canDeleteDraft(ctx.getDb())) {
- throw new AuthException("Not permitted to delete this draft change");
- }
- for (PatchSet ps : patchSets) {
- if (!ps.isDraft()) {
- throw new ResourceConflictException("Cannot delete draft change " + id
- + ": patch set " + ps.getPatchSetId() + " is not a draft");
+
+ if (status == Change.Status.DRAFT) {
+ if (!allowDrafts && !ctx.getControl().isAdmin()) {
+ throw new MethodNotAllowedException("Draft workflow is disabled");
+ }
+ for (PatchSet ps : patchSets) {
+ if (!ps.isDraft()) {
+ throw new ResourceConflictException("Cannot delete draft change " + id
+ + ": patch set " + ps.getPatchSetId() + " is not a draft");
+ }
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
index 1cd8726..e473e39 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
@@ -59,7 +59,7 @@
private final BatchUpdate.Factory updateFactory;
private final PatchSetInfoFactory patchSetInfoFactory;
private final PatchSetUtil psUtil;
- private final Provider<DeleteDraftChangeOp> deleteChangeOpProvider;
+ private final Provider<DeleteChangeOp> deleteChangeOpProvider;
private final DynamicItem<AccountPatchReviewStore> accountPatchReviewStore;
private final boolean allowDrafts;
@@ -68,7 +68,7 @@
BatchUpdate.Factory updateFactory,
PatchSetInfoFactory patchSetInfoFactory,
PatchSetUtil psUtil,
- Provider<DeleteDraftChangeOp> deleteChangeOpProvider,
+ Provider<DeleteChangeOp> deleteChangeOpProvider,
DynamicItem<AccountPatchReviewStore> accountPatchReviewStore,
@GerritServerConfig Config cfg) {
this.db = db;
@@ -97,7 +97,7 @@
private Collection<PatchSet> patchSetsBeforeDeletion;
private PatchSet patchSet;
- private DeleteDraftChangeOp deleteChangeOp;
+ private DeleteChangeOp deleteChangeOp;
private Op(PatchSet.Id psId) {
this.psId = psId;
@@ -116,7 +116,7 @@
if (!allowDrafts) {
throw new MethodNotAllowedException("Draft workflow is disabled");
}
- if (!ctx.getControl().canDeleteDraft(ctx.getDb())) {
+ if (!ctx.getControl().canDelete(ctx.getDb(), Change.Status.DRAFT)) {
throw new AuthException("Not permitted to delete this draft patch set");
}
@@ -146,8 +146,8 @@
psUtil.delete(ctx.getDb(), ctx.getUpdate(patchSet.getId()), patchSet);
accountPatchReviewStore.get().clearReviewed(psId);
- // Use the unwrap from DeleteDraftChangeOp to handle BatchUpdateReviewDb.
- ReviewDb db = DeleteDraftChangeOp.unwrap(ctx.getDb());
+ // Use the unwrap from DeleteChangeOp to handle BatchUpdateReviewDb.
+ ReviewDb db = DeleteChangeOp.unwrap(ctx.getDb());
db.changeMessages().delete(db.changeMessages().byPatchSet(psId));
db.patchComments().delete(db.patchComments().byPatchSet(psId));
db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(psId));
@@ -195,7 +195,7 @@
rsrc.getPatchSet().getPatchSetId()))
.setVisible(allowDrafts
&& rsrc.getPatchSet().isDraft()
- && rsrc.getControl().canDeleteDraft(db.get())
+ && rsrc.getControl().canDelete(db.get(), Change.Status.DRAFT)
&& psCount > 1);
} catch (OrmException e) {
throw new IllegalStateException(e);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
index bb76084..9ff9833 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
@@ -68,7 +68,7 @@
post(CHANGE_KIND, "check").to(Check.class);
put(CHANGE_KIND, "topic").to(PutTopic.class);
delete(CHANGE_KIND, "topic").to(PutTopic.class);
- delete(CHANGE_KIND).to(DeleteDraftChange.class);
+ delete(CHANGE_KIND).to(DeleteChange.class);
post(CHANGE_KIND, "abandon").to(Abandon.class);
post(CHANGE_KIND, "hashtags").to(PostHashtags.class);
post(CHANGE_KIND, "publish").to(PublishDraftPatchSet.CurrentRevision.class);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index 5cc461f..c4d8dcd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -261,10 +261,23 @@
&& isVisible(db);
}
- /** Can this user delete this draft change or any draft patch set of this change? */
- public boolean canDeleteDraft(final ReviewDb db) throws OrmException {
- return (isOwner() || getRefControl().canDeleteDrafts())
- && isVisible(db);
+ /** Can this user delete this change or any patch set of this change? */
+ public boolean canDelete(ReviewDb db, Change.Status status)
+ throws OrmException {
+ if (!isVisible(db)) {
+ return false;
+ }
+
+ switch (status) {
+ case DRAFT:
+ return (isOwner() || getRefControl().canDeleteDrafts());
+ case NEW:
+ case ABANDONED:
+ return isAdmin();
+ case MERGED:
+ default:
+ return false;
+ }
}
/** Can this user rebase this change? */
@@ -377,6 +390,10 @@
return false;
}
+ public boolean isAdmin() {
+ return getUser().getCapabilities().canAdministrateServer();
+ }
+
/** @return true if the user is allowed to remove this reviewer. */
public boolean canRemoveReviewer(PatchSetApproval approval) {
return canRemoveReviewer(approval.getAccountId(), approval.getValue());
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
index 5686dc3..1ff5aac 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
@@ -36,6 +36,8 @@
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.schema.SchemaCreator;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gerrit.server.util.RequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.gerrit.testutil.ConfigSuite;
@@ -97,6 +99,9 @@
@Inject
protected ThreadLocalRequestContext requestContext;
+ @Inject
+ protected OneOffRequestContext oneOffRequestContext;
+
protected LifecycleManager lifecycle;
protected ReviewDb db;
protected AccountInfo currentUserInfo;
@@ -405,18 +410,20 @@
private Account.Id createAccount(String username, String fullName,
String email, boolean active) throws Exception {
- Account.Id id =
- accountManager.authenticate(AuthRequest.forUser(username)).getAccountId();
- if (email != null) {
- accountManager.link(id, AuthRequest.forEmail(email));
+ try (ManualRequestContext ctx = oneOffRequestContext.open()) {
+ Account.Id id =
+ accountManager.authenticate(AuthRequest.forUser(username)).getAccountId();
+ if (email != null) {
+ accountManager.link(id, AuthRequest.forEmail(email));
+ }
+ Account a = db.accounts().get(id);
+ a.setFullName(fullName);
+ a.setPreferredEmail(email);
+ a.setActive(active);
+ db.accounts().update(ImmutableList.of(a));
+ accountCache.evict(id);
+ return id;
}
- Account a = db.accounts().get(id);
- a.setFullName(fullName);
- a.setPreferredEmail(email);
- a.setActive(active);
- db.accounts().update(ImmutableList.of(a));
- accountCache.evict(id);
- return id;
}
private void addEmails(AccountInfo account, String... emails)
diff --git a/tools/bzl/javadoc.bzl b/tools/bzl/javadoc.bzl
index c479e2c..dcaa611 100644
--- a/tools/bzl/javadoc.bzl
+++ b/tools/bzl/javadoc.bzl
@@ -26,6 +26,7 @@
transitive_jar_paths = [j.path for j in transitive_jar_set]
dir = ctx.outputs.zip.path + ".dir"
source = ctx.outputs.zip.path + ".source"
+ external_docs = ["http://docs.oracle.com/javase/8/docs/api"] + ctx.attr.external_docs
cmd = [
"rm -rf %s" % source,
"mkdir %s" % source,
@@ -41,7 +42,7 @@
"-notimestamp",
"-quiet",
"-windowtitle '%s'" % ctx.attr.title,
- "-link", "http://docs.oracle.com/javase/8/docs/api",
+ " ".join(['-link %s' % url for url in external_docs]),
"-sourcepath %s" % source,
"-subpackages ",
":".join(ctx.attr.pkgs),
@@ -61,6 +62,7 @@
"libs": attr.label_list(allow_files = False),
"pkgs": attr.string_list(),
"title": attr.string(),
+ "external_docs": attr.string_list(),
"_javadoc": attr.label(
default = Label("@local_jdk//:bin/javadoc"),
single_file = True,
