From 49cb3e126d139b74e2adc6f767b9a121a86eeaa6 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 29 Jun 2011 14:35:14 +0200 Subject: [PATCH] Allow adding groups as reviewer On the ChangeScreen it is now possible to add a group as reviewer for a change. When a group is added as reviewer the group is resolved and all its members are added as reviewers to the change. To avoid that users accidentily add groups as reviewers that have a large amount of members, Gerrit administrators can configure a maximum number of reviewers that can be added at once by adding a group as reviewer. In addition Gerrit administrators can configure that users should confirm the adding of the reviewers if the number of reviewers that should be added is over a certain limit. It is also possible to add the system group 'Project Owners' as reviewer. In this case all users which own the project are added as reviewers. If a user and a group have the same name, only the user is added as reviewer. Signed-off-by: Edwin Kempin Bug: issue 881 Change-Id: I78a359f89ce045a81306e0a447384c37cda07d00 --- Documentation/config-gerrit.txt | 23 +++ .../common/data/PatchDetailService.java | 2 +- .../gerrit/common/data/ReviewerInfo.java | 58 ++++++ .../gerrit/common/data/ReviewerResult.java | 31 ++- .../gerrit/common/data/SuggestService.java | 12 ++ .../gerrit/client/admin/AdminConstants.java | 1 + .../client/admin/AdminConstants.properties | 1 + .../gerrit/client/changes/ApprovalTable.java | 80 ++++++-- .../client/changes/ChangeConstants.java | 1 + .../client/changes/ChangeConstants.properties | 3 +- .../gerrit/client/changes/ChangeMessages.java | 6 +- .../client/changes/ChangeMessages.properties | 6 +- .../client/ui/ReviewerSuggestOracle.java | 83 ++++++++ .../gerrit/httpd/rpc/SuggestServiceImpl.java | 188 +++++++++++++----- .../httpd/rpc/account/AccountModule.java | 2 +- .../rpc/account/GroupAdminServiceImpl.java | 4 +- .../httpd/rpc/account/GroupDetailHandler.java | 46 +++++ .../gerrit/httpd/rpc/account/RenameGroup.java | 1 + .../httpd/rpc/patch/AddReviewerHandler.java | 12 +- .../rpc/patch/PatchDetailServiceImpl.java | 4 +- .../server}/account/GroupDetailFactory.java | 12 +- .../server/account/GroupMembersFactory.java | 122 ++++++++++++ .../server/config/GerritRequestModule.java | 4 + .../gerrit/server/patch/AddReviewer.java | 102 +++++++++- .../sshd/commands/ModifyReviewersCommand.java | 5 +- 25 files changed, 701 insertions(+), 108 deletions(-) create mode 100644 gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerInfo.java create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java create mode 100644 gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailHandler.java rename {gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc => gerrit-server/src/main/java/com/google/gerrit/server}/account/GroupDetailFactory.java (92%) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembersFactory.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index e3d696e194..58f238e566 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -24,6 +24,29 @@ Sample `etc/gerrit.config`: diskbuffer = 10 m ---- +[[addreviewer]]Section addreviewer +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +[[addreviewer.maxWithoutConfirmation]]addreviewer.maxWithoutConfirmation:: ++ +The maximum number of reviewers a user can add at once by adding a +group as reviewer without being asked to confirm the operation. ++ +If set to 0, the user will never be asked to confirm adding a group +as reviewer. ++ +Default is 10. + +[[addreviewer.maxAllowed]]addreviewer.maxAllowed:: ++ +The maximum number of reviewers a user can add at once by adding a +group as reviewer. ++ +If set to 0, there is no limit for the number of reviewers that can +be added at once by adding a group as reviewer. ++ +Default is 20. + [[auth]]Section auth ~~~~~~~~~~~~~~~~~~~~ diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java index 5aec0c766a..0899b6fc77 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java @@ -50,7 +50,7 @@ public interface PatchDetailService extends RemoteJsonService { AsyncCallback callback); @SignInRequired - void addReviewers(Change.Id id, List reviewers, + void addReviewers(Change.Id id, List reviewers, boolean confirmed, AsyncCallback callback); @SignInRequired diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerInfo.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerInfo.java new file mode 100644 index 0000000000..063f13be4e --- /dev/null +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerInfo.java @@ -0,0 +1,58 @@ +// Copyright (C) 2011 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.common.data; + +/** + * Suggested reviewer for a change. Can be a user ({@link AccountInfo}) or a + * group ({@link GroupReference}). + */ +public class ReviewerInfo implements Comparable { + private AccountInfo accountInfo; + private GroupReference groupReference; + + protected ReviewerInfo() { + } + + public ReviewerInfo(final AccountInfo accountInfo) { + this.accountInfo = accountInfo; + } + + public ReviewerInfo(final GroupReference groupReference) { + this.groupReference = groupReference; + } + + public AccountInfo getAccountInfo() { + return accountInfo; + } + + public GroupReference getGroup() { + return groupReference; + } + + @Override + public int compareTo(final ReviewerInfo o) { + return getSortValue().compareTo(o.getSortValue()); + } + + private String getSortValue() { + if (accountInfo != null) { + if (accountInfo.getPreferredEmail() != null) { + return accountInfo.getPreferredEmail(); + } + return accountInfo.getFullName().toString(); + } + return groupReference.getName(); + } +} diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java index a535c17357..d85095ad6b 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java @@ -24,6 +24,8 @@ import java.util.List; public class ReviewerResult { protected List errors; protected ChangeDetail change; + protected int memberCount; + protected boolean askForConfirmation; public ReviewerResult() { errors = new ArrayList(); @@ -45,10 +47,26 @@ public class ReviewerResult { change = d; } + public int getMemberCount() { + return memberCount; + } + + public void setMemberCount(final int memberCount) { + this.memberCount = memberCount; + } + + public boolean askForConfirmation() { + return askForConfirmation; + } + + public void setAskForConfirmation(final boolean askForConfirmation) { + this.askForConfirmation = askForConfirmation; + } + public static class Error { public static enum Type { - /** Name supplied does not match to a registered account. */ - ACCOUNT_NOT_FOUND, + /** Name supplied does not match to a registered account or account group. */ + REVIEWER_NOT_FOUND, /** The account is inactive. */ ACCOUNT_INACTIVE, @@ -56,6 +74,15 @@ public class ReviewerResult { /** The account is not permitted to see the change. */ CHANGE_NOT_VISIBLE, + /** The groups has no members. */ + GROUP_EMPTY, + + /** The groups has too many members. */ + GROUP_HAS_TOO_MANY_MEMBERS, + + /** The group is not allowed to be added as reviewer. */ + GROUP_NOT_ALLOWED, + /** Could not remove this reviewer from the change due to ORMException. */ COULD_NOT_REMOVE, diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java index 976004fb04..12c8b60c49 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java @@ -14,6 +14,7 @@ package com.google.gerrit.common.data; +import com.google.gerrit.reviewdb.AccountGroup; import com.google.gerrit.reviewdb.Project; import com.google.gwt.user.client.rpc.AsyncCallback; import com.google.gwtjsonrpc.client.RemoteJsonService; @@ -32,4 +33,15 @@ public interface SuggestService extends RemoteJsonService { void suggestAccountGroup(String query, int limit, AsyncCallback> callback); + + /** + * Suggests reviewers. A reviewer can be a user or a group. Inactive users, + * the system groups {@link AccountGroup#ANONYMOUS_USERS} and + * {@link AccountGroup#REGISTERED_USERS} and groups that have more than the + * configured addReviewer.maxAllowed members are not suggested as + * reviewers. + * @param project the project for which reviewers should be suggested + */ + void suggestReviewer(Project.NameKey project, String query, int limit, + AsyncCallback> callback); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java index 712dee6100..4b3ea97aac 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java @@ -46,6 +46,7 @@ public interface AdminConstants extends Constants { String emailOnlyAuthors(); String descriptionNotifications(); String buttonSaveGroupOptions(); + String suggestedGroupLabel(); String headingOwner(); String headingParentProjectName(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties index 3b907ecff5..86b0e4050a 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties @@ -23,6 +23,7 @@ requireChangeID = Require "); + message.append(Util.M.groupManyMembersConfirmation(groupName, + memberCount)); + message.append(""); + final ConfirmationDialog confirmationDialog = + new ConfirmationDialog(Util.C + .approvalTableAddManyReviewersConfirmationDialogTitle(), + new HTML(message.toString()), new ConfirmationCallback() { + @Override + public void onOk() { + addReviewers(reviewers, true); + } + }); + confirmationDialog.center(); + } @Override public void onFailure(final Throwable caught) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java index d9bd6a6fd2..f80c0a3150 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java @@ -81,6 +81,7 @@ public interface ChangeConstants extends Constants { String approvalTableRemoveNotPermitted(); String approvalTableCouldNotRemove(); String approvalTableAddReviewerHint(); + String approvalTableAddManyReviewersConfirmationDialogTitle(); String changeInfoBlockOwner(); String changeInfoBlockProject(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties index ee452588a8..9223d2e951 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties @@ -57,7 +57,8 @@ approvalTableReviewer = Reviewer approvalTableAddReviewer = Add Reviewer approvalTableRemoveNotPermitted = Not allowed to remove reviewer approvalTableCouldNotRemove = Could not remove reviewer -approvalTableAddReviewerHint = Username or Email +approvalTableAddReviewerHint = Name or Email or Group +approvalTableAddManyReviewersConfirmationDialogTitle = Adding many reviewers changeInfoBlockOwner = Owner changeInfoBlockProject = Project diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java index 728d56734a..41dc6c1a92 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java @@ -51,9 +51,13 @@ public interface ChangeMessages extends Messages { String changeQueryWindowTitle(String query); String changeQueryPageTitle(String query); - String accountNotFound(String who); + String reviewerNotFound(String who); String accountInactive(String who); String changeNotVisibleTo(String who); + String groupIsEmpty(String group); + String groupIsNotAllowed(String group); + String groupHasTooManyMembers(String group); + String groupManyMembersConfirmation(String group, int memberCount); String anonymousDownload(String protocol); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties index 3b908f3154..d41b6e6af6 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties @@ -32,8 +32,12 @@ lineHeader = Line {0}: changeQueryWindowTitle = {0} changeQueryPageTitle = Search for {0} -accountNotFound = {0} is not a registered user. +reviewerNotFound = {0} is neither a registered user nor a group. accountInactive = {0} is not an active user. changeNotVisibleTo = {0} cannot access the change. +groupIsEmpty = {0} has no members. +groupIsNotAllowed = {0} cannot be added as reviewer. +groupHasTooManyMembers = {0} has too many members. +groupManyMembersConfirmation = {0} has {1} members. Do you want to add them all as reviewers? anonymousDownload = Anonymous {0} diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java new file mode 100644 index 0000000000..75a3b8341d --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java @@ -0,0 +1,83 @@ +// Copyright (C) 2011 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.client.ui; + +import com.google.gerrit.client.FormatUtil; +import com.google.gerrit.client.RpcStatus; +import com.google.gerrit.client.admin.Util; +import com.google.gerrit.client.rpc.GerritCallback; +import com.google.gerrit.common.data.AccountInfo; +import com.google.gerrit.common.data.ReviewerInfo; +import com.google.gerrit.reviewdb.Project; +import com.google.gwt.user.client.ui.SuggestOracle; +import com.google.gwtexpui.safehtml.client.HighlightSuggestOracle; + +import java.util.ArrayList; +import java.util.List; + +/** Suggestion Oracle for reviewers. */ +public class ReviewerSuggestOracle extends HighlightSuggestOracle { + + private Project.NameKey project; + + @Override + protected void onRequestSuggestions(final Request req, final Callback callback) { + RpcStatus.hide(new Runnable() { + public void run() { + SuggestUtil.SVC.suggestReviewer(project, req.getQuery(), + req.getLimit(), new GerritCallback>() { + public void onSuccess(final List result) { + final List r = + new ArrayList(result + .size()); + for (final ReviewerInfo reviewer : result) { + r.add(new ReviewerSuggestion(reviewer)); + } + callback.onSuggestionsReady(req, new Response(r)); + } + }); + } + }); + } + + public void setProject(final Project.NameKey project) { + this.project = project; + } + + private static class ReviewerSuggestion implements SuggestOracle.Suggestion { + private final ReviewerInfo reviewerInfo; + + ReviewerSuggestion(final ReviewerInfo reviewerInfo) { + this.reviewerInfo = reviewerInfo; + } + + public String getDisplayString() { + final AccountInfo accountInfo = reviewerInfo.getAccountInfo(); + if (accountInfo != null) { + return FormatUtil.nameEmail(accountInfo); + } + return reviewerInfo.getGroup().getName() + " (" + + Util.C.suggestedGroupLabel() + ")"; + } + + public String getReplacementString() { + final AccountInfo accountInfo = reviewerInfo.getAccountInfo(); + if (accountInfo != null) { + return FormatUtil.nameEmail(accountInfo); + } + return reviewerInfo.getGroup().getName(); + } + } +} diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java index bc05ec6a2f..273bd9d657 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java @@ -16,6 +16,7 @@ package com.google.gerrit.httpd.rpc; import com.google.gerrit.common.data.AccountInfo; import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.common.data.ReviewerInfo; import com.google.gerrit.common.data.SuggestService; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.reviewdb.Account; @@ -29,7 +30,10 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.account.GroupControl; +import com.google.gerrit.server.account.GroupMembersFactory; +import com.google.gerrit.server.account.GroupMembersFactory.Factory; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.patch.AddReviewer; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; @@ -56,29 +60,34 @@ class SuggestServiceImpl extends BaseServiceImplementation implements private final ProjectCache projectCache; private final AccountCache accountCache; private final GroupControl.Factory groupControlFactory; + private final Factory groupMembersFactory; private final IdentifiedUser.GenericFactory userFactory; private final Provider currentUser; private final SuggestAccountsEnum suggestAccounts; + private final Config cfg; private final GroupCache groupCache; + @Inject SuggestServiceImpl(final Provider schema, final ProjectControl.Factory projectControlFactory, final ProjectCache projectCache, final AccountCache accountCache, final GroupControl.Factory groupControlFactory, + final GroupMembersFactory.Factory groupMembersFactory, final IdentifiedUser.GenericFactory userFactory, final Provider currentUser, - @GerritServerConfig final Config cfg, - final GroupCache groupCache) { + @GerritServerConfig final Config cfg, final GroupCache groupCache) { super(schema, currentUser); this.projectControlFactory = projectControlFactory; this.projectCache = projectCache; this.accountCache = accountCache; this.groupControlFactory = groupControlFactory; + this.groupMembersFactory = groupMembersFactory; this.userFactory = userFactory; this.currentUser = currentUser; this.suggestAccounts = cfg.getEnum("suggest", null, "accounts", SuggestAccountsEnum.ALL); + this.cfg = cfg; this.groupCache = groupCache; } @@ -106,45 +115,50 @@ class SuggestServiceImpl extends BaseServiceImplementation implements public void suggestAccount(final String query, final Boolean active, final int limit, final AsyncCallback> callback) { - if (suggestAccounts == SuggestAccountsEnum.OFF) { - callback.onSuccess(Collections. emptyList()); - return; - } - run(callback, new Action>() { public List run(final ReviewDb db) throws OrmException { - final String a = query; - final String b = a + MAX_SUFFIX; - final int max = 10; - final int n = limit <= 0 ? max : Math.min(limit, max); - - final LinkedHashMap r = - new LinkedHashMap(); - for (final Account p : db.accounts().suggestByFullName(a, b, n)) { - addSuggestion(r, p, new AccountInfo(p), active); - } - if (r.size() < n) { - for (final Account p : db.accounts().suggestByPreferredEmail(a, b, - n - r.size())) { - addSuggestion(r, p, new AccountInfo(p), active); - } - } - if (r.size() < n) { - for (final AccountExternalId e : db.accountExternalIds() - .suggestByEmailAddress(a, b, n - r.size())) { - if (!r.containsKey(e.getAccountId())) { - final Account p = accountCache.get(e.getAccountId()).getAccount(); - final AccountInfo info = new AccountInfo(p); - info.setPreferredEmail(e.getEmailAddress()); - addSuggestion(r, p, info, active); - } - } - } - return new ArrayList(r.values()); + return suggestAccount(db, query, active, limit); } }); } + private List suggestAccount(final ReviewDb db, + final String query, final Boolean active, final int limit) + throws OrmException { + if (suggestAccounts == SuggestAccountsEnum.OFF) { + return Collections. emptyList(); + } + + final String a = query; + final String b = a + MAX_SUFFIX; + final int max = 10; + final int n = limit <= 0 ? max : Math.min(limit, max); + + final LinkedHashMap r = + new LinkedHashMap(); + for (final Account p : db.accounts().suggestByFullName(a, b, n)) { + addSuggestion(r, p, new AccountInfo(p), active); + } + if (r.size() < n) { + for (final Account p : db.accounts().suggestByPreferredEmail(a, b, + n - r.size())) { + addSuggestion(r, p, new AccountInfo(p), active); + } + } + if (r.size() < n) { + for (final AccountExternalId e : db.accountExternalIds() + .suggestByEmailAddress(a, b, n - r.size())) { + if (!r.containsKey(e.getAccountId())) { + final Account p = accountCache.get(e.getAccountId()).getAccount(); + final AccountInfo info = new AccountInfo(p); + info.setPreferredEmail(e.getEmailAddress()); + addSuggestion(r, p, info, active); + } + } + } + return new ArrayList(r.values()); + } + private void addSuggestion(Map map, Account account, AccountInfo info, Boolean active) { if (map.containsKey(account.getId())) { @@ -201,26 +215,90 @@ class SuggestServiceImpl extends BaseServiceImplementation implements final AsyncCallback> callback) { run(callback, new Action>() { public List run(final ReviewDb db) throws OrmException { - final String a = query; - final String b = a + MAX_SUFFIX; - final int max = 10; - final int n = limit <= 0 ? max : Math.min(limit, max); - List r = new ArrayList(n); - for (AccountGroupName group : db.accountGroupNames() - .suggestByName(a, b, n)) { - try { - if (groupControlFactory.controlFor(group.getId()).isVisible()) { - AccountGroup g = groupCache.get(group.getId()); - if (g != null && g.getGroupUUID() != null) { - r.add(GroupReference.forGroup(g)); - } - } - } catch (NoSuchGroupException e) { - continue; - } - } - return r; + return suggestAccountGroup(db, query, limit); } }); } -} + + private List suggestAccountGroup(final ReviewDb db, + final String query, final int limit) throws OrmException { + final String a = query; + final String b = a + MAX_SUFFIX; + final int max = 10; + final int n = limit <= 0 ? max : Math.min(limit, max); + List r = new ArrayList(n); + for (AccountGroupName group : db.accountGroupNames().suggestByName(a, b, n)) { + try { + if (groupControlFactory.controlFor(group.getId()).isVisible()) { + AccountGroup g = groupCache.get(group.getId()); + if (g != null && g.getGroupUUID() != null) { + r.add(GroupReference.forGroup(g)); + } + } + } catch (NoSuchGroupException e) { + continue; + } + } + return r; + } + + @Override + public void suggestReviewer(final Project.NameKey project, + final String query, final int limit, + final AsyncCallback> callback) { + run(callback, new Action>() { + public List run(final ReviewDb db) throws OrmException { + final List suggestedAccounts = + suggestAccount(db, query, Boolean.TRUE, limit); + final List reviewer = + new ArrayList(suggestedAccounts.size()); + for (final AccountInfo a : suggestedAccounts) { + reviewer.add(new ReviewerInfo(a)); + } + final List suggestedAccountGroups = + suggestAccountGroup(db, query, limit); + for (final GroupReference g : suggestedAccountGroups) { + if (suggestGroupAsReviewer(project, g)) { + reviewer.add(new ReviewerInfo(g)); + } + } + + Collections.sort(reviewer); + if (reviewer.size() <= limit) { + return reviewer; + } else { + return reviewer.subList(0, limit); + } + } + }); + } + + private boolean suggestGroupAsReviewer(final Project.NameKey project, + final GroupReference group) throws OrmException { + if (!AddReviewer.isLegalReviewerGroup(group.getUUID())) { + return false; + } + + try { + final Set members = + groupMembersFactory.create(project, group.getUUID()).call(); + + if (members.isEmpty()) { + return false; + } + + final int maxAllowed = + cfg.getInt("addreviewer", "maxAllowed", + AddReviewer.DEFAULT_MAX_REVIEWERS); + if (maxAllowed > 0 && members.size() > maxAllowed) { + return false; + } + } catch (NoSuchGroupException e) { + return false; + } catch (NoSuchProjectException e) { + return false; + } + + return true; + } +} \ No newline at end of file diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountModule.java index 13dd9b4389..91c8aa9235 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountModule.java @@ -32,7 +32,7 @@ public class AccountModule extends RpcServletModule { factory(CreateGroup.Factory.class); factory(DeleteExternalIds.Factory.class); factory(ExternalIdDetailFactory.Factory.class); - factory(GroupDetailFactory.Factory.class); + factory(GroupDetailHandler.Factory.class); factory(MyGroupsFactory.Factory.class); factory(RenameGroup.Factory.class); } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java index f89ef095c8..157c89628a 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java @@ -63,7 +63,7 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements private final CreateGroup.Factory createGroupFactory; private final RenameGroup.Factory renameGroupFactory; - private final GroupDetailFactory.Factory groupDetailFactory; + private final GroupDetailHandler.Factory groupDetailFactory; @Inject GroupAdminServiceImpl(final Provider schema, @@ -75,7 +75,7 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements final GroupControl.Factory groupControlFactory, final CreateGroup.Factory createGroupFactory, final RenameGroup.Factory renameGroupFactory, - final GroupDetailFactory.Factory groupDetailFactory) { + final GroupDetailHandler.Factory groupDetailFactory) { super(schema, currentUser); this.identifiedUser = currentUser; this.accountCache = accountCache; diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailHandler.java new file mode 100644 index 0000000000..513c90f7a2 --- /dev/null +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailHandler.java @@ -0,0 +1,46 @@ +// Copyright (C) 2011 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.httpd.rpc.account; + +import com.google.gerrit.common.data.GroupDetail; +import com.google.gerrit.common.errors.NoSuchGroupException; +import com.google.gerrit.httpd.rpc.Handler; +import com.google.gerrit.reviewdb.AccountGroup; +import com.google.gerrit.server.account.GroupDetailFactory; +import com.google.gwtorm.client.OrmException; +import com.google.inject.Inject; +import com.google.inject.assistedinject.Assisted; + +public class GroupDetailHandler extends Handler { + public interface Factory { + GroupDetailHandler create(AccountGroup.Id groupId); + } + + private final GroupDetailFactory.Factory groupDetailFactory; + + private final AccountGroup.Id groupId; + + @Inject + GroupDetailHandler(final GroupDetailFactory.Factory groupDetailFactory, + @Assisted final AccountGroup.Id groupId) { + this.groupDetailFactory = groupDetailFactory; + this.groupId = groupId; + } + + @Override + public GroupDetail call() throws OrmException, NoSuchGroupException { + return groupDetailFactory.create(groupId).call(); + } +} diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/RenameGroup.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/RenameGroup.java index c8bf519b17..d7cd195bad 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/RenameGroup.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/RenameGroup.java @@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.account.GroupControl; +import com.google.gerrit.server.account.GroupDetailFactory; import com.google.gerrit.server.git.RenameGroupOp; import com.google.gwtorm.client.OrmDuplicateKeyException; import com.google.gwtorm.client.OrmException; diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewerHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewerHandler.java index 4765eac549..953657b27b 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewerHandler.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewerHandler.java @@ -26,7 +26,8 @@ import java.util.Collection; class AddReviewerHandler extends Handler { interface Factory { - AddReviewerHandler create(Change.Id changeId, Collection nameOrEmails); + AddReviewerHandler create(Change.Id changeId, Collection reviewers, + boolean confirmed); } private final AddReviewer.Factory addReviewerFactory; @@ -34,23 +35,26 @@ class AddReviewerHandler extends Handler { private final Change.Id changeId; private final Collection reviewers; + private final boolean confirmed; @Inject AddReviewerHandler(final AddReviewer.Factory addReviewerFactory, final ChangeDetailFactory.Factory changeDetailFactory, @Assisted final Change.Id changeId, - @Assisted final Collection nameOrEmails) { + @Assisted final Collection reviewers, + @Assisted final boolean confirmed) { this.addReviewerFactory = addReviewerFactory; this.changeDetailFactory = changeDetailFactory; this.changeId = changeId; - this.reviewers = nameOrEmails; + this.reviewers = reviewers; + this.confirmed = confirmed; } @Override public ReviewerResult call() throws Exception { - ReviewerResult result = addReviewerFactory.create(changeId, reviewers).call(); + ReviewerResult result = addReviewerFactory.create(changeId, reviewers, confirmed).call(); result.setChange(changeDetailFactory.create(changeId).call()); return result; } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java index 59755e3528..0f3b4f8eae 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java @@ -155,8 +155,8 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements } public void addReviewers(final Change.Id id, final List reviewers, - final AsyncCallback callback) { - addReviewerHandlerFactory.create(id, reviewers).to(callback); + final boolean confirmed, final AsyncCallback callback) { + addReviewerHandlerFactory.create(id, reviewers, confirmed).to(callback); } public void removeReviewer(final Change.Id id, final Account.Id reviewerId, diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java similarity index 92% rename from gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailFactory.java rename to gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java index da6ab65ef7..d4c8576028 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java @@ -12,20 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -package com.google.gerrit.httpd.rpc.account; +package com.google.gerrit.server.account; import com.google.gerrit.common.data.GroupDetail; import com.google.gerrit.common.errors.NoSuchGroupException; -import com.google.gerrit.httpd.rpc.Handler; import com.google.gerrit.reviewdb.Account; import com.google.gerrit.reviewdb.AccountGroup; import com.google.gerrit.reviewdb.AccountGroupInclude; import com.google.gerrit.reviewdb.AccountGroupMember; import com.google.gerrit.reviewdb.ReviewDb; -import com.google.gerrit.server.account.AccountInfoCacheFactory; -import com.google.gerrit.server.account.GroupCache; -import com.google.gerrit.server.account.GroupControl; -import com.google.gerrit.server.account.GroupInfoCacheFactory; import com.google.gwtorm.client.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -34,9 +29,10 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.concurrent.Callable; -class GroupDetailFactory extends Handler { - interface Factory { +public class GroupDetailFactory implements Callable { + public interface Factory { GroupDetailFactory create(AccountGroup.Id groupId); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembersFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembersFactory.java new file mode 100644 index 0000000000..aae3a90903 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembersFactory.java @@ -0,0 +1,122 @@ +// Copyright (C) 2011 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.account; + +import com.google.gerrit.common.data.GroupDetail; +import com.google.gerrit.common.errors.NoSuchGroupException; +import com.google.gerrit.reviewdb.Account; +import com.google.gerrit.reviewdb.AccountGroup; +import com.google.gerrit.reviewdb.AccountGroupInclude; +import com.google.gerrit.reviewdb.AccountGroupMember; +import com.google.gerrit.reviewdb.Project; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.project.NoSuchProjectException; +import com.google.gerrit.server.project.ProjectControl; +import com.google.gwtorm.client.OrmException; +import com.google.inject.Inject; +import com.google.inject.assistedinject.Assisted; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.Callable; + +public class GroupMembersFactory implements Callable> { + public interface Factory { + GroupMembersFactory create(Project.NameKey project, + AccountGroup.UUID groupUUID); + } + + private GroupCache groupCache; + private final GroupDetailFactory.Factory groupDetailFactory; + private final AccountCache accountCache; + private final ProjectControl.GenericFactory projectControl; + private final IdentifiedUser currentUser; + + private final Project.NameKey project; + private final AccountGroup.UUID groupUUID; + + + @Inject + GroupMembersFactory(final GroupCache groupCache, + final GroupDetailFactory.Factory groupDetailFactory, + final AccountCache accountCache, + final ProjectControl.GenericFactory projectControl, + final IdentifiedUser currentUser, + @Assisted final Project.NameKey project, + @Assisted final AccountGroup.UUID groupUUID) { + this.groupCache = groupCache; + this.groupDetailFactory = groupDetailFactory; + this.accountCache = accountCache; + this.projectControl = projectControl; + this.currentUser = currentUser; + + this.project = project; + this.groupUUID = groupUUID; + } + + @Override + public Set call() throws NoSuchGroupException, + NoSuchProjectException, OrmException { + if (AccountGroup.PROJECT_OWNERS.equals(groupUUID)) { + return getProjectOwners(); + } + + return getAllGroupMembers(groupCache.get(groupUUID), + new HashSet()); + } + + private Set getProjectOwners() throws NoSuchProjectException, + NoSuchGroupException, OrmException { + if (project == null) { + return Collections.emptySet(); + } + + final Set ownerGroups = + projectControl.controlFor(project, currentUser).getProjectState() + .getOwners(); + + final HashSet projectOwners = new HashSet(); + for (final AccountGroup.UUID ownerGroup : ownerGroups) { + projectOwners.addAll(getAllGroupMembers(groupCache.get(ownerGroup), + new HashSet())); + } + return projectOwners; + } + + private Set getAllGroupMembers(final AccountGroup group, + final Set seen) throws NoSuchGroupException, + OrmException { + seen.add(group.getId()); + final GroupDetail groupDetail = + groupDetailFactory.create(group.getId()).call(); + + final Set members = new HashSet(); + if (groupDetail.members != null) { + for (final AccountGroupMember member : groupDetail.members) { + members.add(accountCache.get(member.getAccountId()).getAccount()); + } + } + if (groupDetail.includes != null) { + for (AccountGroupInclude groupInclude : groupDetail.includes) { + if (!seen.contains(groupInclude.getIncludeId())) { + members.addAll(getAllGroupMembers( + groupCache.get(groupInclude.getIncludeId()), seen)); + } + } + } + return members; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java index 72dcd31dbb..3ba9aab409 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java @@ -22,6 +22,8 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.RequestCleanup; import com.google.gerrit.server.account.AccountResolver; import com.google.gerrit.server.account.GroupControl; +import com.google.gerrit.server.account.GroupDetailFactory; +import com.google.gerrit.server.account.GroupMembersFactory; import com.google.gerrit.server.account.PerformCreateGroup; import com.google.gerrit.server.git.CreateCodeReviewNotes; import com.google.gerrit.server.git.MergeOp; @@ -87,5 +89,7 @@ public class GerritRequestModule extends FactoryModule { factory(MergeFailSender.Factory.class); factory(RegisterNewEmailSender.Factory.class); factory(PerformCreateGroup.Factory.class); + factory(GroupDetailFactory.Factory.class); + factory(GroupMembersFactory.Factory.class); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java index 4cb31987d5..be0ddacf23 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java @@ -19,6 +19,7 @@ import com.google.gerrit.common.data.ApprovalType; import com.google.gerrit.common.data.ApprovalTypes; import com.google.gerrit.common.data.ReviewerResult; import com.google.gerrit.reviewdb.Account; +import com.google.gerrit.reviewdb.AccountGroup; import com.google.gerrit.reviewdb.ApprovalCategory; import com.google.gerrit.reviewdb.Change; import com.google.gerrit.reviewdb.PatchSet; @@ -26,12 +27,17 @@ import com.google.gerrit.reviewdb.PatchSetApproval; import com.google.gerrit.reviewdb.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountResolver; +import com.google.gerrit.server.account.GroupCache; +import com.google.gerrit.server.account.GroupMembersFactory; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.mail.AddReviewerSender; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.client.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; +import org.eclipse.jgit.lib.Config; + import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; @@ -40,42 +46,56 @@ import java.util.Set; import java.util.concurrent.Callable; public class AddReviewer implements Callable { + public final static int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10; + public final static int DEFAULT_MAX_REVIEWERS = 20; + public interface Factory { - AddReviewer create(Change.Id changeId, Collection nameOrEmails); + AddReviewer create(Change.Id changeId, + Collection userNameOrEmailOrGroupNames, boolean confirmed); } private final AddReviewerSender.Factory addReviewerSenderFactory; private final AccountResolver accountResolver; + private final GroupCache groupCache; + private final GroupMembersFactory.Factory groupMembersFactory; private final ChangeControl.Factory changeControlFactory; private final ReviewDb db; private final IdentifiedUser currentUser; private final IdentifiedUser.GenericFactory identifiedUserFactory; private final ApprovalCategory.Id addReviewerCategoryId; + private final Config cfg; private final Change.Id changeId; private final Collection reviewers; + private final boolean confirmed; @Inject AddReviewer(final AddReviewerSender.Factory addReviewerSenderFactory, - final AccountResolver accountResolver, + final AccountResolver accountResolver, final GroupCache groupCache, + final GroupMembersFactory.Factory groupMembersFactory, final ChangeControl.Factory changeControlFactory, final ReviewDb db, final IdentifiedUser.GenericFactory identifiedUserFactory, final IdentifiedUser currentUser, final ApprovalTypes approvalTypes, - @Assisted final Change.Id changeId, - @Assisted final Collection nameOrEmails) { + final @GerritServerConfig Config cfg, @Assisted final Change.Id changeId, + @Assisted final Collection reviewers, + @Assisted final boolean confirmed) { this.addReviewerSenderFactory = addReviewerSenderFactory; this.accountResolver = accountResolver; + this.groupCache = groupCache; + this.groupMembersFactory = groupMembersFactory; this.db = db; this.changeControlFactory = changeControlFactory; this.identifiedUserFactory = identifiedUserFactory; this.currentUser = currentUser; + this.cfg = cfg; final List allTypes = approvalTypes.getApprovalTypes(); addReviewerCategoryId = allTypes.get(allTypes.size() - 1).getCategory().getId(); this.changeId = changeId; - this.reviewers = nameOrEmails; + this.reviewers = reviewers; + this.confirmed = confirmed; } @Override @@ -84,18 +104,73 @@ public class AddReviewer implements Callable { final ChangeControl control = changeControlFactory.validateFor(changeId); final ReviewerResult result = new ReviewerResult(); - for (final String nameOrEmail : reviewers) { - final Account account = accountResolver.find(nameOrEmail); + for (final String reviewer : reviewers) { + final Account account = accountResolver.find(reviewer); if (account == null) { - result.addError(new ReviewerResult.Error( - ReviewerResult.Error.Type.ACCOUNT_NOT_FOUND, nameOrEmail)); + AccountGroup group = groupCache.get(new AccountGroup.NameKey(reviewer)); + + if (group == null) { + result.addError(new ReviewerResult.Error( + ReviewerResult.Error.Type.REVIEWER_NOT_FOUND, reviewer)); + continue; + } + + if (!isLegalReviewerGroup(group.getGroupUUID())) { + result.addError(new ReviewerResult.Error( + ReviewerResult.Error.Type.GROUP_NOT_ALLOWED, reviewer)); + continue; + } + + final Set members = + groupMembersFactory.create(control.getProject().getNameKey(), + group.getGroupUUID()).call(); + if (members == null || members.size() == 0) { + result.addError(new ReviewerResult.Error( + ReviewerResult.Error.Type.GROUP_EMPTY, reviewer)); + continue; + } + + // if maxAllowed is set to 0, it is allowed to add any number of + // reviewers + final int maxAllowed = + cfg.getInt("addreviewer", "maxAllowed", DEFAULT_MAX_REVIEWERS); + if (maxAllowed > 0 && members.size() > maxAllowed) { + result.setMemberCount(members.size()); + result.setAskForConfirmation(false); + result.addError(new ReviewerResult.Error( + ReviewerResult.Error.Type.GROUP_HAS_TOO_MANY_MEMBERS, reviewer)); + continue; + } + + // if maxWithoutCheck is set to 0, we never ask for confirmation + final int maxWithoutConfirmation = + cfg.getInt("addreviewer", "maxWithoutConfirmation", + DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK); + if (!confirmed && maxWithoutConfirmation > 0 + && members.size() > maxWithoutConfirmation) { + result.setMemberCount(members.size()); + result.setAskForConfirmation(true); + result.addError(new ReviewerResult.Error( + ReviewerResult.Error.Type.GROUP_HAS_TOO_MANY_MEMBERS, reviewer)); + continue; + } + + for (final Account member : members) { + if (member.isActive()) { + final IdentifiedUser user = + identifiedUserFactory.create(member.getId()); + if (control.forUser(user).isVisible()) { + reviewerIds.add(member.getId()); + } + } + } continue; } if (!account.isActive()) { result.addError(new ReviewerResult.Error( ReviewerResult.Error.Type.ACCOUNT_INACTIVE, - formatUser(account, nameOrEmail))); + formatUser(account, reviewer))); continue; } @@ -103,7 +178,7 @@ public class AddReviewer implements Callable { if (!control.forUser(user).isVisible()) { result.addError(new ReviewerResult.Error( ReviewerResult.Error.Type.CHANGE_NOT_VISIBLE, - formatUser(account, nameOrEmail))); + formatUser(account, reviewer))); continue; } @@ -165,4 +240,9 @@ public class AddReviewer implements Callable { return new PatchSetApproval(new PatchSetApproval.Key(patchSetId, reviewerId, addReviewerCategoryId), (short) 0); } + + public static boolean isLegalReviewerGroup(final AccountGroup.UUID groupUUID) { + return !(AccountGroup.ANONYMOUS_USERS.equals(groupUUID) + || AccountGroup.REGISTERED_USERS.equals(groupUUID)); + } } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ModifyReviewersCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ModifyReviewersCommand.java index b351b1630b..29401224a8 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ModifyReviewersCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ModifyReviewersCommand.java @@ -138,12 +138,13 @@ public class ModifyReviewersCommand extends BaseCommand { // Add reviewers // - result = addReviewerFactory.create(changeId, stringSet(toAdd)).call(); + result = + addReviewerFactory.create(changeId, stringSet(toAdd), false).call(); ok &= result.getErrors().isEmpty(); for (ReviewerResult.Error resultError : result.getErrors()) { String message; switch (resultError.getType()) { - case ACCOUNT_NOT_FOUND: + case REVIEWER_NOT_FOUND: message = "account {0} not found"; break; case ACCOUNT_INACTIVE: