From 6f3ee4b6513f5db03e85b4f72ca15111f3f6c6c9 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 7 Sep 2018 13:18:10 -0700 Subject: [PATCH] Add API for deleting draft comments of a user Now that changes with unresolved comments appear in dashboards, some users are discovering that they have a lot of them and would like to clean them up. Add an account API endpoint to do so, where callers may restrict the set of changes to delete draft comments on by passing a query in the input. This endpoint is not yet wired up into the UI, but the intent is that the input structure is flexible enough to accommodate a specialized flow in the UI to delete draft comments on some or all changes in the dashboard section, as well as custom queries from the command line. An interface to select and delete subsets of comments from matching changes is not in scope, although in theory the DeleteDraftCommentsInput class could be extended to support this. Change-Id: Id05adcc1a63b213d67b163d5fcaa47ab8c2679c3 --- Documentation/rest-api-accounts.txt | 94 ++++++++ .../gerrit/acceptance/AccountCreator.java | 5 + .../extensions/api/accounts/AccountApi.java | 9 + .../accounts/DeleteDraftCommentsInput.java | 35 +++ .../api/accounts/DeletedDraftCommentInfo.java | 24 ++ .../server/api/accounts/AccountApiImpl.java | 16 ++ .../restapi/account/DeleteDraftComments.java | 211 ++++++++++++++++++ .../gerrit/server/restapi/account/Module.java | 2 + .../server/restapi/change/CommentJson.java | 4 +- .../acceptance/api/accounts/AccountIT.java | 161 +++++++++++++ .../rest/AccountsRestApiBindingsIT.java | 1 + 11 files changed, 560 insertions(+), 2 deletions(-) create mode 100644 java/com/google/gerrit/extensions/api/accounts/DeleteDraftCommentsInput.java create mode 100644 java/com/google/gerrit/extensions/api/accounts/DeletedDraftCommentInfo.java create mode 100644 java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt index e28a9c47c9..1050d56326 100644 --- a/Documentation/rest-api-accounts.txt +++ b/Documentation/rest-api-accounts.txt @@ -1805,6 +1805,69 @@ of link:#contributor-agreement-info[ContributorAgreementInfo] entities. ] ---- +[delete-draft-comments] +=== Delete Draft Comments +-- +'POST /accounts/link:#account-id[\{account-id\}]/drafts:delete' +-- + +Deletes some or all of a user's draft comments. The set of comments to delete is +specified as a link:#delete-draft-comments-input[DeleteDraftCommentsInput] +entity. An empty input entity deletes all comments. + +Only drafts belonging to the caller may be deleted. + +.Request +---- + POST /accounts/self/drafts.delete HTTP/1.0 + Content-Type: application/json; charset=UTF-8 + + { + "query": "is:abandoned" + } +---- + +As a response, a list of +link:#deleted-draft-comment-info[DeletedDraftCommentInfo] entities is returned. + +.Response +---- + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + [ + { + "change": { + "id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940", + "project": "myProject", + "branch": "master", + "change_id": "I8473b95934b5732ac55d26311a706c9c2bde9940", + "subject": "Implementing Feature X", + "status": "ABANDONED", + "created": "2013-02-01 09:59:32.126000000", + "updated": "2013-02-21 11:16:36.775000000", + "insertions": 34, + "deletions": 101, + "_number": 3965, + "owner": { + "name": "John Doe" + } + }, + "deleted": [ + { + "id": "TvcXrmjM", + "path": "gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java", + "line": 23, + "message": "[nit] trailing whitespace", + "updated": "2013-02-26 15:40:43.986000000" + } + ] + } + ] +---- + [[sign-contributor-agreement]] === Sign Contributor Agreement -- @@ -2316,6 +2379,37 @@ new contributor agreement. |`name` |The name of the agreement. |================================= +[[delete-draft-comments-input]] +=== DeleteDraftCommentsInput +The `DeleteDraftCommentsInput` entity contains information specifying a set of +draft comments that should be deleted. + +[options="header",cols="1,^1,5"] +|================================= +|Field Name ||Description +|`query` |optional| +A link:user-search.html[change query] limiting results to changes matching this +query; `has:draft` is implied and not necessary to list explicitly. If not set, +matches all changes with drafts. +|================================= + +[[deleted-draft-comment-info]] +=== DeletedDraftCommentInfo +The `DeletedDraftCommentInfo` entity contains information about draft comments +that were deleted. + +[options="header",cols="1,6"] +|================================= +|Field Name |Description +|`change` | +link:rest-api-changes.html#change-info[ChangeInfo] entity describing the change +on which one or more comments was deleted. Populated with only the +link:rest-api-changes.html#skip_mergeable[SKIP_MERGEABLE] option. +|`deleted` | +List of link:rest-api-changes.html#comment-info[CommentInfo] entities for each +comment that was deleted. +|================================= + [[diff-preferences-info]] === DiffPreferencesInfo The `DiffPreferencesInfo` entity contains information about the diff diff --git a/java/com/google/gerrit/acceptance/AccountCreator.java b/java/com/google/gerrit/acceptance/AccountCreator.java index 14167977e5..69492a95eb 100644 --- a/java/com/google/gerrit/acceptance/AccountCreator.java +++ b/java/com/google/gerrit/acceptance/AccountCreator.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance; import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.gerrit.common.Nullable; @@ -150,6 +151,10 @@ public class AccountCreator { accounts.values().removeIf(a -> ids.contains(a.id)); } + public ImmutableList getAll() { + return ImmutableList.copyOf(accounts.values()); + } + private void addGroupMember(AccountGroup.UUID groupUuid, Account.Id accountId) throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException { InternalGroupUpdate groupUpdate = diff --git a/java/com/google/gerrit/extensions/api/accounts/AccountApi.java b/java/com/google/gerrit/extensions/api/accounts/AccountApi.java index 6adc978116..b7fce5f344 100644 --- a/java/com/google/gerrit/extensions/api/accounts/AccountApi.java +++ b/java/com/google/gerrit/extensions/api/accounts/AccountApi.java @@ -109,6 +109,9 @@ public interface AccountApi { void deleteExternalIds(List externalIds) throws RestApiException; + List deleteDraftComments(DeleteDraftCommentsInput input) + throws RestApiException; + /** * A default implementation which allows source compatibility when adding new methods to the * interface. @@ -301,5 +304,11 @@ public interface AccountApi { public void deleteExternalIds(List externalIds) throws RestApiException { throw new NotImplementedException(); } + + @Override + public List deleteDraftComments(DeleteDraftCommentsInput input) + throws RestApiException { + throw new NotImplementedException(); + } } } diff --git a/java/com/google/gerrit/extensions/api/accounts/DeleteDraftCommentsInput.java b/java/com/google/gerrit/extensions/api/accounts/DeleteDraftCommentsInput.java new file mode 100644 index 0000000000..113f3d42d5 --- /dev/null +++ b/java/com/google/gerrit/extensions/api/accounts/DeleteDraftCommentsInput.java @@ -0,0 +1,35 @@ +// Copyright (C) 2018 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.extensions.api.accounts; + +import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.restapi.DefaultInput; + +public class DeleteDraftCommentsInput { + /** + * Delete comments only on changes that match this query. + * + *

If null or empty, delete comments on all changes. + */ + @DefaultInput public String query; + + public DeleteDraftCommentsInput() { + this(null); + } + + public DeleteDraftCommentsInput(@Nullable String query) { + this.query = query; + } +} diff --git a/java/com/google/gerrit/extensions/api/accounts/DeletedDraftCommentInfo.java b/java/com/google/gerrit/extensions/api/accounts/DeletedDraftCommentInfo.java new file mode 100644 index 0000000000..c15d5bc1e1 --- /dev/null +++ b/java/com/google/gerrit/extensions/api/accounts/DeletedDraftCommentInfo.java @@ -0,0 +1,24 @@ +// Copyright (C) 2018 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.extensions.api.accounts; + +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.CommentInfo; +import java.util.List; + +public class DeletedDraftCommentInfo { + public ChangeInfo change; + public List deleted; +} diff --git a/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java b/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java index 9ef5753175..15e21fe629 100644 --- a/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java +++ b/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java @@ -20,6 +20,8 @@ import static javax.servlet.http.HttpServletResponse.SC_OK; import com.google.gerrit.common.RawInputUtil; import com.google.gerrit.extensions.api.accounts.AccountApi; import com.google.gerrit.extensions.api.accounts.AgreementInput; +import com.google.gerrit.extensions.api.accounts.DeleteDraftCommentsInput; +import com.google.gerrit.extensions.api.accounts.DeletedDraftCommentInfo; import com.google.gerrit.extensions.api.accounts.EmailApi; import com.google.gerrit.extensions.api.accounts.EmailInput; import com.google.gerrit.extensions.api.accounts.GpgKeyApi; @@ -51,6 +53,7 @@ import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.restapi.account.AddSshKey; import com.google.gerrit.server.restapi.account.CreateEmail; import com.google.gerrit.server.restapi.account.DeleteActive; +import com.google.gerrit.server.restapi.account.DeleteDraftComments; import com.google.gerrit.server.restapi.account.DeleteEmail; import com.google.gerrit.server.restapi.account.DeleteExternalIds; import com.google.gerrit.server.restapi.account.DeleteSshKey; @@ -125,6 +128,7 @@ public class AccountApiImpl implements AccountApi { private final Index index; private final GetExternalIds getExternalIds; private final DeleteExternalIds deleteExternalIds; + private final DeleteDraftComments deleteDraftComments; private final PutStatus putStatus; private final GetGroups getGroups; private final EmailApiImpl.Factory emailApi; @@ -165,6 +169,7 @@ public class AccountApiImpl implements AccountApi { Index index, GetExternalIds getExternalIds, DeleteExternalIds deleteExternalIds, + DeleteDraftComments deleteDraftComments, PutStatus putStatus, GetGroups getGroups, EmailApiImpl.Factory emailApi, @@ -204,6 +209,7 @@ public class AccountApiImpl implements AccountApi { this.index = index; this.getExternalIds = getExternalIds; this.deleteExternalIds = deleteExternalIds; + this.deleteDraftComments = deleteDraftComments; this.putStatus = putStatus; this.getGroups = getGroups; this.emailApi = emailApi; @@ -561,4 +567,14 @@ public class AccountApiImpl implements AccountApi { throw asRestApiException("Cannot delete external IDs", e); } } + + @Override + public List deleteDraftComments(DeleteDraftCommentsInput input) + throws RestApiException { + try { + return deleteDraftComments.apply(account, input); + } catch (Exception e) { + throw asRestApiException("Cannot delete draft comments", e); + } + } } diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java new file mode 100644 index 0000000000..26d6cf4446 --- /dev/null +++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java @@ -0,0 +1,211 @@ +// Copyright (C) 2018 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.restapi.account; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.gerrit.server.CommentsUtil.setCommentRevId; + +import com.google.common.base.CharMatcher; +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.api.accounts.DeleteDraftCommentsInput; +import com.google.gerrit.extensions.api.accounts.DeletedDraftCommentInfo; +import com.google.gerrit.extensions.client.ListChangesOption; +import com.google.gerrit.extensions.common.CommentInfo; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.index.query.Predicate; +import com.google.gerrit.index.query.QueryParseException; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Account.Id; +import com.google.gerrit.reviewdb.client.Comment; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CommentsUtil; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.PatchSetUtil; +import com.google.gerrit.server.account.AccountResource; +import com.google.gerrit.server.change.ChangeJson; +import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListNotAvailableException; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.ChangeQueryBuilder; +import com.google.gerrit.server.query.change.HasDraftByPredicate; +import com.google.gerrit.server.query.change.InternalChangeQuery; +import com.google.gerrit.server.restapi.change.CommentJson; +import com.google.gerrit.server.restapi.change.CommentJson.CommentFormatter; +import com.google.gerrit.server.update.BatchUpdate; +import com.google.gerrit.server.update.BatchUpdate.Factory; +import com.google.gerrit.server.update.BatchUpdateListener; +import com.google.gerrit.server.update.BatchUpdateOp; +import com.google.gerrit.server.update.ChangeContext; +import com.google.gerrit.server.update.UpdateException; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +@Singleton +public class DeleteDraftComments + implements RestModifyView { + + private final Provider db; + private final Provider userProvider; + private final BatchUpdate.Factory batchUpdateFactory; + private final Provider queryBuilderProvider; + private final Provider queryProvider; + private final ChangeData.Factory changeDataFactory; + private final ChangeJson.Factory changeJsonFactory; + private final Provider commentJsonProvider; + private final CommentsUtil commentsUtil; + private final PatchSetUtil psUtil; + private final PatchListCache patchListCache; + + @Inject + DeleteDraftComments( + Provider db, + Provider userProvider, + Factory batchUpdateFactory, + Provider queryBuilderProvider, + Provider queryProvider, + ChangeData.Factory changeDataFactory, + ChangeJson.Factory changeJsonFactory, + Provider commentJsonProvider, + CommentsUtil commentsUtil, + PatchSetUtil psUtil, + PatchListCache patchListCache) { + this.db = db; + this.userProvider = userProvider; + this.batchUpdateFactory = batchUpdateFactory; + this.queryBuilderProvider = queryBuilderProvider; + this.queryProvider = queryProvider; + this.changeDataFactory = changeDataFactory; + this.changeJsonFactory = changeJsonFactory; + this.commentJsonProvider = commentJsonProvider; + this.commentsUtil = commentsUtil; + this.psUtil = psUtil; + this.patchListCache = patchListCache; + } + + @Override + public ImmutableList apply( + AccountResource rsrc, DeleteDraftCommentsInput input) + throws RestApiException, OrmException, UpdateException { + CurrentUser user = userProvider.get(); + if (!user.isIdentifiedUser()) { + throw new AuthException("Authentication required"); + } + if (!rsrc.getUser().hasSameAccountId(user)) { + // Disallow even for admins or users with Modify Account. Drafts are not like preferences or + // other account info; there is no way even for admins to read or delete another user's drafts + // using the normal draft endpoints under the change resource, so disallow it here as well. + // (Admins may still call this endpoint with impersonation, but in that case it would pass the + // hasSameAccountId check.) + throw new AuthException("Cannot delete drafts of other user"); + } + + CommentFormatter commentFormatter = commentJsonProvider.get().newCommentFormatter(); + Account.Id accountId = rsrc.getUser().getAccountId(); + Timestamp now = TimeUtil.nowTs(); + Map updates = new LinkedHashMap<>(); + List ops = new ArrayList<>(); + for (ChangeData cd : + queryProvider + .get() + // Don't attempt to mutate any changes the user can't currently see. + .enforceVisibility(true) + .query(predicate(accountId, input))) { + BatchUpdate update = + updates.computeIfAbsent( + cd.project(), p -> batchUpdateFactory.create(db.get(), p, rsrc.getUser(), now)); + Op op = new Op(commentFormatter, accountId); + update.addOp(cd.getId(), op); + ops.add(op); + } + + // Currently there's no way to let some updates succeed even if others fail. Even if there were, + // all updates from this operation only happen in All-Users and thus are fully atomic, so + // allowing partial failure would have little value. + batchUpdateFactory.execute(updates.values(), BatchUpdateListener.NONE, false); + + return ops.stream().map(Op::getResult).filter(Objects::nonNull).collect(toImmutableList()); + } + + private Predicate predicate(Account.Id accountId, DeleteDraftCommentsInput input) + throws BadRequestException { + Predicate hasDraft = new HasDraftByPredicate(accountId); + if (CharMatcher.whitespace().trimFrom(Strings.nullToEmpty(input.query)).isEmpty()) { + return hasDraft; + } + try { + return Predicate.and(hasDraft, queryBuilderProvider.get().parse(input.query)); + } catch (QueryParseException e) { + throw new BadRequestException("Invalid query: " + e.getMessage(), e); + } + } + + private class Op implements BatchUpdateOp { + private final CommentFormatter commentFormatter; + private final Account.Id accountId; + private DeletedDraftCommentInfo result; + + Op(CommentFormatter commentFormatter, Id accountId) { + this.commentFormatter = commentFormatter; + this.accountId = accountId; + } + + @Override + public boolean updateChange(ChangeContext ctx) + throws OrmException, PatchListNotAvailableException, PermissionBackendException { + ImmutableList.Builder comments = ImmutableList.builder(); + boolean dirty = false; + for (Comment c : commentsUtil.draftByChangeAuthor(ctx.getDb(), ctx.getNotes(), accountId)) { + dirty = true; + PatchSet.Id psId = new PatchSet.Id(ctx.getChange().getId(), c.key.patchSetId); + setCommentRevId( + c, patchListCache, ctx.getChange(), psUtil.get(ctx.getDb(), ctx.getNotes(), psId)); + commentsUtil.deleteComments(ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(c)); + comments.add(commentFormatter.format(c)); + } + if (dirty) { + result = new DeletedDraftCommentInfo(); + result.change = + changeJsonFactory + .create(ListChangesOption.SKIP_MERGEABLE) + .format(changeDataFactory.create(ctx.getDb(), ctx.getNotes())); + result.deleted = comments.build(); + } + return dirty; + } + + @Nullable + DeletedDraftCommentInfo getResult() { + return result; + } + } +} diff --git a/java/com/google/gerrit/server/restapi/account/Module.java b/java/com/google/gerrit/server/restapi/account/Module.java index be0924a6f5..9b012f7e7d 100644 --- a/java/com/google/gerrit/server/restapi/account/Module.java +++ b/java/com/google/gerrit/server/restapi/account/Module.java @@ -107,6 +107,8 @@ public class Module extends RestApiModule { get(ACCOUNT_KIND, "external.ids").to(GetExternalIds.class); post(ACCOUNT_KIND, "external.ids:delete").to(DeleteExternalIds.class); + post(ACCOUNT_KIND, "drafts:delete").to(DeleteDraftComments.class); + // The gpgkeys REST endpoints are bound via GpgApiModule. factory(AccountsUpdate.Factory.class); diff --git a/java/com/google/gerrit/server/restapi/change/CommentJson.java b/java/com/google/gerrit/server/restapi/change/CommentJson.java index a562592360..7112bbfb44 100644 --- a/java/com/google/gerrit/server/restapi/change/CommentJson.java +++ b/java/com/google/gerrit/server/restapi/change/CommentJson.java @@ -41,7 +41,7 @@ import java.util.List; import java.util.Map; import java.util.TreeMap; -class CommentJson { +public class CommentJson { private final AccountLoader.Factory accountLoaderFactory; @@ -161,7 +161,7 @@ class CommentJson { } } - class CommentFormatter extends BaseCommentFormatter { + public class CommentFormatter extends BaseCommentFormatter { @Override protected CommentInfo toInfo(Comment c, AccountLoader loader) { CommentInfo ci = new CommentInfo(); diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index de66b87dd3..1eaadca078 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -15,8 +15,10 @@ package com.google.gerrit.acceptance.api.accounts; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.common.truth.Truth.assert_; import static com.google.common.truth.Truth8.assertThat; import static com.google.gerrit.acceptance.GitUtil.deleteRef; import static com.google.gerrit.acceptance.GitUtil.fetch; @@ -62,8 +64,11 @@ import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule.Action; import com.google.gerrit.extensions.api.accounts.AccountInput; +import com.google.gerrit.extensions.api.accounts.DeleteDraftCommentsInput; +import com.google.gerrit.extensions.api.accounts.DeletedDraftCommentInfo; import com.google.gerrit.extensions.api.accounts.EmailInput; import com.google.gerrit.extensions.api.changes.AddReviewerInput; +import com.google.gerrit.extensions.api.changes.DraftInput; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.StarsInput; import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo; @@ -73,6 +78,7 @@ import com.google.gerrit.extensions.api.config.ConsistencyCheckInput.CheckAccoun import com.google.gerrit.extensions.common.AccountDetailInfo; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.common.EmailInfo; import com.google.gerrit.extensions.common.GpgKeyInfo; import com.google.gerrit.extensions.common.GroupInfo; @@ -92,6 +98,7 @@ import com.google.gerrit.gpg.PublicKeyStore; import com.google.gerrit.gpg.testing.TestKey; import com.google.gerrit.mail.Address; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; @@ -2583,6 +2590,160 @@ public class AccountIT extends AbstractDaemonTest { assertThat(stalenessChecker.isStale(accountId)).isFalse(); } + @Test + public void deleteAllDraftComments() throws Exception { + try { + TestTimeUtil.resetWithClockStep(1, SECONDS); + Project.NameKey project2 = createProject("project2"); + PushOneCommit.Result r1 = createChange(); + + TestRepository tr2 = cloneProject(project2); + PushOneCommit.Result r2 = + createChange( + tr2, + "refs/heads/master", + "Change in project2", + PushOneCommit.FILE_NAME, + "content2", + null); + + // Create 2 drafts each on both changes for user. + setApiUser(user); + createDraft(r1, PushOneCommit.FILE_NAME, "draft 1a"); + createDraft(r1, PushOneCommit.FILE_NAME, "draft 1b"); + createDraft(r2, PushOneCommit.FILE_NAME, "draft 2a"); + createDraft(r2, PushOneCommit.FILE_NAME, "draft 2b"); + assertThat(gApi.changes().id(r1.getChangeId()).current().draftsAsList()).hasSize(2); + assertThat(gApi.changes().id(r2.getChangeId()).current().draftsAsList()).hasSize(2); + + // Create 1 draft on first change for admin. + setApiUser(admin); + createDraft(r1, PushOneCommit.FILE_NAME, "admin draft"); + assertThat(gApi.changes().id(r1.getChangeId()).current().draftsAsList()).hasSize(1); + + // Delete user's draft comments; leave admin's alone. + setApiUser(user); + List result = + gApi.accounts().self().deleteDraftComments(new DeleteDraftCommentsInput()); + + // Results are ordered according to the change search, most recently updated first. + assertThat(result).hasSize(2); + DeletedDraftCommentInfo del2 = result.get(0); + assertThat(del2.change.changeId).isEqualTo(r2.getChangeId()); + assertThat(del2.deleted.stream().map(c -> c.message)).containsExactly("draft 2a", "draft 2b"); + DeletedDraftCommentInfo del1 = result.get(1); + assertThat(del1.change.changeId).isEqualTo(r1.getChangeId()); + assertThat(del1.deleted.stream().map(c -> c.message)).containsExactly("draft 1a", "draft 1b"); + + assertThat(gApi.changes().id(r1.getChangeId()).current().draftsAsList()).isEmpty(); + assertThat(gApi.changes().id(r2.getChangeId()).current().draftsAsList()).isEmpty(); + + setApiUser(admin); + assertThat(gApi.changes().id(r1.getChangeId()).current().draftsAsList()).hasSize(1); + } finally { + cleanUpDrafts(); + } + } + + @Test + public void deleteDraftCommentsByQuery() throws Exception { + try { + PushOneCommit.Result r1 = createChange(); + PushOneCommit.Result r2 = createChange(); + + createDraft(r1, PushOneCommit.FILE_NAME, "draft a"); + createDraft(r2, PushOneCommit.FILE_NAME, "draft b"); + assertThat(gApi.changes().id(r1.getChangeId()).current().draftsAsList()).hasSize(1); + assertThat(gApi.changes().id(r2.getChangeId()).current().draftsAsList()).hasSize(1); + + List result = + gApi.accounts() + .self() + .deleteDraftComments(new DeleteDraftCommentsInput("change:" + r1.getChangeId())); + assertThat(result).hasSize(1); + assertThat(result.get(0).change.changeId).isEqualTo(r1.getChangeId()); + assertThat(result.get(0).deleted.stream().map(c -> c.message)).containsExactly("draft a"); + + assertThat(gApi.changes().id(r1.getChangeId()).current().draftsAsList()).isEmpty(); + assertThat(gApi.changes().id(r2.getChangeId()).current().draftsAsList()).hasSize(1); + } finally { + cleanUpDrafts(); + } + } + + @Test + public void deleteOtherUsersDraftCommentsDisallowed() throws Exception { + try { + PushOneCommit.Result r = createChange(); + setApiUser(user); + createDraft(r, PushOneCommit.FILE_NAME, "draft"); + setApiUser(admin); + try { + gApi.accounts().id(user.id.get()).deleteDraftComments(new DeleteDraftCommentsInput()); + assert_().fail("expected AuthException"); + } catch (AuthException e) { + assertThat(e).hasMessageThat().isEqualTo("Cannot delete drafts of other user"); + } + } finally { + cleanUpDrafts(); + } + } + + @Test + public void deleteDraftCommentsSkipsInvisibleChanges() throws Exception { + try { + createBranch(new Branch.NameKey(project, "secret")); + PushOneCommit.Result r1 = createChange(); + PushOneCommit.Result r2 = createChange("refs/for/secret"); + + setApiUser(user); + createDraft(r1, PushOneCommit.FILE_NAME, "draft a"); + createDraft(r2, PushOneCommit.FILE_NAME, "draft b"); + assertThat(gApi.changes().id(r1.getChangeId()).current().draftsAsList()).hasSize(1); + assertThat(gApi.changes().id(r2.getChangeId()).current().draftsAsList()).hasSize(1); + + block(project, "refs/heads/secret", Permission.READ, REGISTERED_USERS); + List result = + gApi.accounts().self().deleteDraftComments(new DeleteDraftCommentsInput()); + assertThat(result).hasSize(1); + assertThat(result.get(0).change.changeId).isEqualTo(r1.getChangeId()); + assertThat(result.get(0).deleted.stream().map(c -> c.message)).containsExactly("draft a"); + + removePermission(project, "refs/heads/secret", Permission.READ); + assertThat(gApi.changes().id(r1.getChangeId()).current().draftsAsList()).isEmpty(); + // Draft still exists since change wasn't visible when drafts where deleted. + assertThat(gApi.changes().id(r2.getChangeId()).current().draftsAsList()).hasSize(1); + } finally { + cleanUpDrafts(); + } + } + + private void createDraft(PushOneCommit.Result r, String path, String message) throws Exception { + DraftInput in = new DraftInput(); + in.path = path; + in.line = 1; + in.message = message; + gApi.changes().id(r.getChangeId()).current().createDraft(in); + } + + private void cleanUpDrafts() throws Exception { + for (TestAccount testAccount : accountCreator.getAll()) { + setApiUser(testAccount); + for (ChangeInfo changeInfo : gApi.changes().query("has:draft").get()) { + for (CommentInfo c : + gApi.changes() + .id(changeInfo.id) + .drafts() + .values() + .stream() + .flatMap(List::stream) + .collect(toImmutableList())) { + gApi.changes().id(changeInfo.id).revision(c.patchSet).draft(c.id).delete(); + } + } + } + } + private static Correspondence getGroupToNameCorrespondence() { return new Correspondence() { @Override diff --git a/javatests/com/google/gerrit/acceptance/rest/AccountsRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/AccountsRestApiBindingsIT.java index 1a401b075c..b0adba7d2b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/AccountsRestApiBindingsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/AccountsRestApiBindingsIT.java @@ -88,6 +88,7 @@ public class AccountsRestApiBindingsIT extends AbstractRestApiBindingsTest { RestCall.put("/accounts/%s/agreements"), RestCall.get("/accounts/%s/external.ids"), RestCall.post("/accounts/%s/external.ids:delete"), + RestCall.post("/accounts/%s/drafts:delete"), RestCall.get("/accounts/%s/oauthtoken"), RestCall.get("/accounts/%s/capabilities"), RestCall.get("/accounts/%s/capabilities/viewPlugins"),