diff options
author | David Pursehouse <david.pursehouse@sonymobile.com> | 2016-02-10 11:10:48 +0900 |
---|---|---|
committer | David Pursehouse <david.pursehouse@sonymobile.com> | 2016-02-10 11:10:48 +0900 |
commit | a772a0abc2dd4b02240ffb71d6689ffb4054a232 (patch) | |
tree | f5c75327a80a78674cb6d48f9820f9a92044005c | |
parent | c844bc5c86ea128c672b8ed395aceaaec913cee5 (diff) | |
parent | 0bb6804f3e36063b812a40e43688a9c929903c07 (diff) |
Merge branch 'stable-2.11' into stable-2.12
* stable-2.11:
Set version to 2.11.7
Release notes for Gerrit 2.11.7
Document new allowrcpt restriction on adding user email
OutputStreamQuery: fix comments with current-patch-set option
Add tests for ssh query command
EmailReviewComments: Provide the current user instead of exception
OutputStreamQuery: Take files into account when adding patch sets
Update 2.11.6 release notes
Change-Id: I6b24624da6af024e340a672b2ac527d5510f5219
8 files changed, 386 insertions, 11 deletions
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt index c65c61d785..3cdbcb31ad 100644 --- a/Documentation/rest-api-accounts.txt +++ b/Documentation/rest-api-accounts.txt @@ -496,6 +496,9 @@ For the development mode email addresses are directly added without confirmation. A Gerrit administrator may add an email address without confirmation by setting `no_confirmation` in the link:#email-input[EmailInput]. +If link:config-gerrit.html#sendemail.allowrcpt[sendemail.allowrcpt] is +configured, the added email address must belong to a domain that is +allowed, unless `no_confirmation` is set. In the request body additional data for the email address can be provided as link:#email-input[EmailInput]. diff --git a/ReleaseNotes/ReleaseNotes-2.11.6.txt b/ReleaseNotes/ReleaseNotes-2.11.6.txt index 40c1ffec24..d6f939f3c5 100644 --- a/ReleaseNotes/ReleaseNotes-2.11.6.txt +++ b/ReleaseNotes/ReleaseNotes-2.11.6.txt @@ -90,6 +90,10 @@ to new Gerrit version. The default was 'No' which resulted in some sites not upgrading core plugins and running the wrong versions. +* Fix reading of plugin documentation. ++ +Under some circumstances it was possible to fail with an IO error. + * Replication ** Recursively include parent groups of groups specified in `authGroup`. diff --git a/ReleaseNotes/ReleaseNotes-2.11.7.txt b/ReleaseNotes/ReleaseNotes-2.11.7.txt new file mode 100644 index 0000000000..7a0de2db29 --- /dev/null +++ b/ReleaseNotes/ReleaseNotes-2.11.7.txt @@ -0,0 +1,44 @@ +Release notes for Gerrit 2.11.7 +=============================== + +Gerrit 2.11.7 is now available: + +link:https://gerrit-releases.storage.googleapis.com/gerrit-2.11.7.war[ +https://gerrit-releases.storage.googleapis.com/gerrit-2.11.7.war] + +There are no schema changes from link:ReleaseNotes-2.11.6.html[2.11.6]. + +Bug Fixes +--------- + +* link:https://code.google.com/p/gerrit/issues/detail?id=3882[Issue 3882]: +Fix 'No user on email thread' exception when label with group parameter is +used in watched projects predicate. + +* link:https://code.google.com/p/gerrit/issues/detail?id=3877[Issue 3877]: +Include files in output when using `gerrit query` with combination of +search operators. ++ +A regression introduced in version 2.11.6 caused files to be omitted +in the output. + +* Include comments in output when using `gerrit query` with the +`--current-patch-set` option. ++ +Comments were added at the change level but were not added at the +patch set level. + +* Honor the `sendemail.allowrcpt` setting when adding new email address. ++ +When adding a new email address via the UI or REST API, it was possible for +the user to add an address that does not belong to a domain allowed by the +`sendemail.allowrcpt` configuration. However, when sending the verification +email, the recipient address was (correctly) dropped, and the email had no +recipients. This resulted in an error from the SMTP server and an 'Internal +server error' message to the user. + +* Remove unnecessary log messages. ++ +The messages 'Assuming empty group membership' and 'Skipping delivery of +email' do not add any value and were filling up the error log. + diff --git a/ReleaseNotes/index.txt b/ReleaseNotes/index.txt index d93f83e288..8fada44719 100644 --- a/ReleaseNotes/index.txt +++ b/ReleaseNotes/index.txt @@ -9,6 +9,7 @@ Version 2.12.x [[2_11]] Version 2.11.x -------------- +* link:ReleaseNotes-2.11.7.html[2.11.7] * link:ReleaseNotes-2.11.6.html[2.11.6] * link:ReleaseNotes-2.11.5.html[2.11.5] * link:ReleaseNotes-2.11.4.html[2.11.4] diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java new file mode 100644 index 0000000000..066d3628d7 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java @@ -0,0 +1,320 @@ +// Copyright (C) 2016 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.acceptance.ssh; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assert_; + +import com.google.common.collect.Lists; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.extensions.api.changes.AddReviewerInput; +import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.client.Side; +import com.google.gerrit.server.data.ChangeAttribute; +import com.google.gson.Gson; + +import org.junit.Test; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; + +@NoHttpd +public class QueryIT extends AbstractDaemonTest { + + private static Gson gson = new Gson(); + + @Test + public void testBasicQueryJSON() throws Exception { + String changeId1 = createChange().getChangeId(); + String changeId2 = createChange().getChangeId(); + + List<ChangeAttribute> changes = executeSuccessfulQuery("1234"); + assertThat(changes.size()).isEqualTo(0); + + changes = executeSuccessfulQuery(changeId1); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).project).isEqualTo(project.toString()); + assertThat(changes.get(0).id).isEqualTo(changeId1); + + changes = executeSuccessfulQuery(changeId1 + " OR " + changeId2); + assertThat(changes.size()).isEqualTo(2); + assertThat(changes.get(0).project).isEqualTo(project.toString()); + assertThat(changes.get(0).id).isEqualTo(changeId2); + assertThat(changes.get(1).project).isEqualTo(project.toString()); + assertThat(changes.get(1).id).isEqualTo(changeId1); + + changes = + executeSuccessfulQuery("--start=1 " + changeId1 + " OR " + changeId2); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).project).isEqualTo(project.toString()); + assertThat(changes.get(0).id).isEqualTo(changeId1); + } + + @Test + public void testAllApprovalsOptionJSON() throws Exception { + String changeId = createChange().getChangeId(); + gApi.changes().id(changeId).current().review(ReviewInput.approve()); + List<ChangeAttribute> changes = executeSuccessfulQuery(changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets).isNull(); + + changes = executeSuccessfulQuery("--all-approvals " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).approvals).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).approvals.size()).isEqualTo(1); + } + + @Test + public void testAllReviewersOptionJSON() throws Exception { + String changeId = createChange().getChangeId(); + AddReviewerInput in = new AddReviewerInput(); + in.reviewer = user.email; + gApi.changes().id(changeId).addReviewer(in); + + List<ChangeAttribute> changes = executeSuccessfulQuery(changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).allReviewers).isNull(); + + changes = executeSuccessfulQuery("--all-reviewers " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).allReviewers).isNotNull(); + assertThat(changes.get(0).allReviewers.size()).isEqualTo(1); + } + + @Test + public void testCommitMessageOptionJSON() throws Exception { + String changeId = createChange().getChangeId(); + List<ChangeAttribute> changes = + executeSuccessfulQuery("--commit-message " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).commitMessage).isNotNull(); + assertThat(changes.get(0).commitMessage).contains(PushOneCommit.SUBJECT); + } + + @Test + public void testCurrentPatchSetOptionJSON() throws Exception { + String changeId = createChange().getChangeId(); + amendChange(changeId); + + List<ChangeAttribute> changes = executeSuccessfulQuery(changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).currentPatchSet).isNull(); + + changes = executeSuccessfulQuery("--current-patch-set " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).currentPatchSet).isNotNull(); + assertThat(changes.get(0).currentPatchSet.number).isEqualTo("2"); + + gApi.changes().id(changeId).current().review(ReviewInput.approve()); + changes = executeSuccessfulQuery("--current-patch-set " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).currentPatchSet).isNotNull(); + assertThat(changes.get(0).currentPatchSet.approvals).isNotNull(); + assertThat(changes.get(0).currentPatchSet.approvals.size()).isEqualTo(1); + + } + + @Test + public void testPatchSetsOptionJSON() throws Exception { + String changeId = createChange().getChangeId(); + amendChange(changeId); + amendChange(changeId); + + List<ChangeAttribute> changes = executeSuccessfulQuery(changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets).isNull(); + + changes = executeSuccessfulQuery("--patch-sets " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets).isNotNull(); + assertThat(changes.get(0).patchSets.size()).isEqualTo(3); + } + + @Test + public void shouldFailWithFilesWithoutPatchSetsOrCurrentPatchSetsOption() + throws Exception { + String changeId = createChange().getChangeId(); + sshSession.exec("gerrit query --files " + changeId); + assertThat(sshSession.hasError()).isTrue(); + assertThat(sshSession.getError()).contains( + "needs --patch-sets or --current-patch-set"); + } + + @Test + public void testFileOptionJSON() throws Exception { + String changeId = createChange().getChangeId(); + + List<ChangeAttribute> changes = + executeSuccessfulQuery("--current-patch-set --files " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).currentPatchSet.files).isNotNull(); + assertThat(changes.get(0).currentPatchSet.files.size()).isEqualTo(2); + + changes = executeSuccessfulQuery("--patch-sets --files " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets.get(0).files).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).files.size()).isEqualTo(2); + + gApi.changes().id(changeId).current().review(ReviewInput.approve()); + changes = + executeSuccessfulQuery("--patch-sets --files --all-approvals " + + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets.get(0).files).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).files.size()).isEqualTo(2); + assertThat(changes.get(0).patchSets.get(0).approvals).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).approvals.size()).isEqualTo(1); + } + + @Test + public void testCommentOptionJSON() throws Exception { + String changeId = createChange().getChangeId(); + + List<ChangeAttribute> changes = executeSuccessfulQuery(changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).comments).isNull(); + + changes = executeSuccessfulQuery("--comments " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).comments).isNotNull(); + assertThat(changes.get(0).comments.size()).isEqualTo(1); + } + + @Test + public void testCommentOptionsInCurrentPatchSetJSON() throws Exception { + String changeId = createChange().getChangeId(); + + ReviewInput review = new ReviewInput(); + ReviewInput.CommentInput comment = new ReviewInput.CommentInput(); + comment.path = PushOneCommit.FILE_NAME; + comment.side = Side.REVISION; + comment.message = "comment 1"; + review.comments = new HashMap<>(); + review.comments.put(comment.path, Lists.newArrayList(comment)); + gApi.changes().id(changeId).current().review(review); + + List<ChangeAttribute> changes = + executeSuccessfulQuery("--current-patch-set " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).currentPatchSet.comments).isNull(); + + changes = + executeSuccessfulQuery("--current-patch-set --comments " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).currentPatchSet.comments).isNotNull(); + assertThat(changes.get(0).currentPatchSet.comments.size()).isEqualTo(1); + } + + @Test + public void testCommentOptionInPatchSetsJSON() throws Exception { + String changeId = createChange().getChangeId(); + + ReviewInput review = new ReviewInput(); + ReviewInput.CommentInput comment = new ReviewInput.CommentInput(); + comment.path = PushOneCommit.FILE_NAME; + comment.side = Side.REVISION; + comment.message = "comment 1"; + review.comments = new HashMap<>(); + review.comments.put(comment.path, Lists.newArrayList(comment)); + gApi.changes().id(changeId).current().review(review); + + List<ChangeAttribute> changes = + executeSuccessfulQuery("--patch-sets " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets.get(0).comments).isNull(); + + changes = executeSuccessfulQuery("--patch-sets --comments " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets.get(0).comments).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).comments.size()).isEqualTo(1); + + changes = + executeSuccessfulQuery("--patch-sets --comments --files " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets.get(0).comments).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).comments.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets.get(0).files).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).files.size()).isEqualTo(2); + + gApi.changes().id(changeId).current().review(ReviewInput.approve()); + changes = + executeSuccessfulQuery("--patch-sets --comments --files --all-approvals " + + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets.get(0).comments).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).comments.size()).isEqualTo(1); + assertThat(changes.get(0).patchSets.get(0).files).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).files.size()).isEqualTo(2); + assertThat(changes.get(0).patchSets.get(0).approvals).isNotNull(); + assertThat(changes.get(0).patchSets.get(0).approvals.size()).isEqualTo(1); + } + + @Test + public void testDependenciesOptionJSON() throws Exception { + String changeId1 = createChange().getChangeId(); + String changeId2 = createChange().getChangeId(); + List<ChangeAttribute> changes = executeSuccessfulQuery(changeId1); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).dependsOn).isNull(); + + changes = executeSuccessfulQuery("--dependencies " + changeId1); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).dependsOn).isNull(); + + changes = executeSuccessfulQuery(changeId2); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).dependsOn).isNull(); + + changes = executeSuccessfulQuery("--dependencies " + changeId2); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).dependsOn).isNotNull(); + assertThat(changes.get(0).dependsOn.size()).isEqualTo(1); + } + + @Test + public void testSubmitRecordsOptionJSON() throws Exception { + String changeId = createChange().getChangeId(); + List<ChangeAttribute> changes = executeSuccessfulQuery(changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).submitRecords).isNull(); + + changes = executeSuccessfulQuery("--submit-records " + changeId); + assertThat(changes.size()).isEqualTo(1); + assertThat(changes.get(0).submitRecords).isNotNull(); + assertThat(changes.get(0).submitRecords.size()).isEqualTo(1); + } + + private List<ChangeAttribute> executeSuccessfulQuery(String params) + throws Exception { + String rawResponse = + sshSession.exec("gerrit query --format=JSON " + params); + assert_().withFailureMessage(sshSession.getError()) + .that(sshSession.hasError()).isFalse(); + return getChanges(rawResponse); + } + + private static List<ChangeAttribute> getChanges(String rawResponse) { + String[] lines = rawResponse.split("\\n"); + List<ChangeAttribute> changes = new ArrayList<>(lines.length - 1); + for (int i = 0; i < lines.length - 1; i++) { + changes.add(gson.fromJson(lines[i], ChangeAttribute.class)); + } + return changes; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java index e416398191..f1dff8810a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java @@ -17,13 +17,13 @@ package com.google.gerrit.server.change; import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER; import com.google.gerrit.extensions.api.changes.ReviewInput.NotifyHandling; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.git.SendEmailExecutor; import com.google.gerrit.server.mail.CommentSender; import com.google.gerrit.server.patch.PatchSetInfoFactory; @@ -32,7 +32,6 @@ import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; -import com.google.inject.OutOfScopeException; import com.google.inject.Provider; import com.google.inject.ProvisionException; import com.google.inject.assistedinject.Assisted; @@ -51,7 +50,7 @@ public class EmailReviewComments implements Runnable, RequestContext { NotifyHandling notify, Change change, PatchSet patchSet, - Account.Id authorId, + IdentifiedUser user, ChangeMessage message, List<PatchLineComment> comments); } @@ -65,13 +64,13 @@ public class EmailReviewComments implements Runnable, RequestContext { private final NotifyHandling notify; private final Change change; private final PatchSet patchSet; - private final Account.Id authorId; + private final IdentifiedUser user; private final ChangeMessage message; private List<PatchLineComment> comments; private ReviewDb db; @Inject - EmailReviewComments ( + EmailReviewComments( @SendEmailExecutor ExecutorService executor, PatchSetInfoFactory patchSetInfoFactory, CommentSender.Factory commentSenderFactory, @@ -80,7 +79,7 @@ public class EmailReviewComments implements Runnable, RequestContext { @Assisted NotifyHandling notify, @Assisted Change change, @Assisted PatchSet patchSet, - @Assisted Account.Id authorId, + @Assisted IdentifiedUser user, @Assisted ChangeMessage message, @Assisted List<PatchLineComment> comments) { this.sendEmailsExecutor = executor; @@ -91,7 +90,7 @@ public class EmailReviewComments implements Runnable, RequestContext { this.notify = notify; this.change = change; this.patchSet = patchSet; - this.authorId = authorId; + this.user = user; this.message = message; this.comments = PLC_ORDER.sortedCopy(comments); } @@ -106,7 +105,7 @@ public class EmailReviewComments implements Runnable, RequestContext { try { CommentSender cm = commentSenderFactory.create(notify, change.getId()); - cm.setFrom(authorId); + cm.setFrom(user.getAccountId()); cm.setPatchSet(patchSet, patchSetInfoFactory.get(change, patchSet)); cm.setChangeMessage(message); cm.setPatchLineComments(comments); @@ -129,7 +128,7 @@ public class EmailReviewComments implements Runnable, RequestContext { @Override public CurrentUser getUser() { - throw new OutOfScopeException("No user on email thread"); + return user.getRealUser(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index e2473e7db5..7a575c8b0f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -376,7 +376,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput> in.notify, change, ps, - user.getAccountId(), + user, message, comments).sendAsync(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java index 237bbf2097..7e3ab2874d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java @@ -293,6 +293,10 @@ public class OutputStreamQuery { eventFactory.addPatchSetFileNames(c.currentPatchSet, d.change(), d.currentPatchSet()); } + if (includeComments) { + eventFactory.addPatchSetComments(c.currentPatchSet, + d.publishedComments()); + } } } @@ -301,7 +305,7 @@ public class OutputStreamQuery { if (includePatchSets) { eventFactory.addPatchSets(db.get(), rw, c, d.patchSets(), includeApprovals ? d.approvals().asMap() : null, - labelTypes); + includeFiles, d.change(), labelTypes); for (PatchSetAttribute attribute : c.patchSets) { eventFactory.addPatchSetComments( attribute, d.publishedComments()); |