Remove reviewers from a change

- The new UI adds a "Remove" column and an "X" button on each
  reviewer row to ApprovalTable.  Clicking this button will cause
  a remote call to removeReviewer

- Added a new removeReviewer remote method to PatchDetailService

- Both the addReviewer and removeReviewer remote methods use the
  same result class, which I renamed from AddReviewerResult to
  ReviewerResult

- ReviewerResult received a new error type "COULD_NOT_REMOVE"

Note: the error handling of this CL is not optimal for 2 reasons:

- Since PatchDetailService always catches exceptions and adds them
  to an errors field in ReviewerResult, the call always succeeds
  even if it actually failed.

- We can display a dialog box if the removal failed, but it would
  be better simply not to display the Delete button if the user
  doesn't have enough privileges.

A cleaner way to implement this (future change list) would be to
return this information in ChangeDetail so that ChangeScreen will
only display Delete buttons that will actually work.

Bug: issue 218
Bug: issue 289
Change-Id: I38ce63d76a3588c85472f1fd723cd097aeafccb3

gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java

@@ -50,7 +50,11 @@ public interface PatchDetailService extends RemoteJsonService {
 
   @SignInRequired
   void addReviewers(Change.Id id, List<String> reviewers,
-      AsyncCallback<AddReviewerResult> callback);
+      AsyncCallback<ReviewerResult> callback);
+
+  @SignInRequired
+  void removeReviewer(Change.Id id, Account.Id reviewerId,
+      AsyncCallback<ReviewerResult> callback);
 
   void userApprovals(Set<Change.Id> cids, Account.Id aid,
       AsyncCallback<ApprovalSummarySet> callback);

gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java

@@ -18,12 +18,14 @@ package com.google.gerrit.common.data;
 import java.util.ArrayList;
 import java.util.List;
 
-/** Result from adding a reviewer to a change. */
+/**
-public class AddReviewerResult {
+ * Result from adding or removing a reviewer from a change.
+ */
+public class ReviewerResult {
   protected List<Error> errors;
   protected ChangeDetail change;
 
-  public AddReviewerResult() {
+  public ReviewerResult() {
     errors = new ArrayList<Error>();
   }
 
@@ -49,7 +51,10 @@ public class AddReviewerResult {
       ACCOUNT_NOT_FOUND,
 
       /** The account is not permitted to see the change. */
-      CHANGE_NOT_VISIBLE
+      CHANGE_NOT_VISIBLE,
+
+      /** Could not remove this reviewer from the change. */
+      COULD_NOT_REMOVE
     }
 
     protected Type type;

gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritCss.java

@@ -27,6 +27,7 @@ public interface GerritCss extends CssResource {
   String accountName();
   String activeRow();
   String addReviewer();
+  String removeReviewer();
   String addSshKeyPanel();
   String approvalCategoryList();
   String approvalTable();

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java

@@ -21,10 +21,10 @@ import com.google.gerrit.client.rpc.GerritCallback;
 import com.google.gerrit.client.ui.AccountDashboardLink;
 import com.google.gerrit.client.ui.AddMemberBox;
 import com.google.gerrit.common.data.AccountInfoCache;
-import com.google.gerrit.common.data.AddReviewerResult;
 import com.google.gerrit.common.data.ApprovalDetail;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ChangeDetail;
+import com.google.gerrit.common.data.ReviewerResult;
 import com.google.gerrit.reviewdb.Account;
 import com.google.gerrit.reviewdb.ApprovalCategory;
 import com.google.gerrit.reviewdb.ApprovalCategoryValue;
@@ -34,6 +34,7 @@ import com.google.gwt.event.dom.client.ClickEvent;
 import com.google.gwt.event.dom.client.ClickHandler;
 import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.Element;
+import com.google.gwt.user.client.ui.Button;
 import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.FlowPanel;
 import com.google.gwt.user.client.ui.Grid;
@@ -61,7 +62,7 @@ public class ApprovalTable extends Composite {
 
   public ApprovalTable() {
     types = Gerrit.getConfig().getApprovalTypes().getApprovalTypes();
-    table = new Grid(1, 3 + types.size());
+    table = new Grid(1, 4 + types.size());
     table.addStyleName(Gerrit.RESOURCES.css().infoTable());
     displayHeader();
 
@@ -102,6 +103,7 @@ public class ApprovalTable extends Composite {
     for (final ApprovalType t : types) {
       header(col++, t.getCategory().getName());
     }
+    header(col++, Util.C.removeReviewer());
     applyEdgeStyles(0);
   }
 
@@ -114,7 +116,8 @@ public class ApprovalTable extends Composite {
     final CellFormatter fmt = table.getCellFormatter();
     fmt.addStyleName(row, 1, Gerrit.RESOURCES.css().approvalrole());
     fmt.addStyleName(row, 1 + types.size(), Gerrit.RESOURCES.css().rightmost());
-    fmt.addStyleName(row, 2 + types.size(), Gerrit.RESOURCES.css().approvalhint());
+    fmt.addStyleName(row, 2 + types.size(), Gerrit.RESOURCES.css().rightmost());
+    fmt.addStyleName(row, 3 + types.size(), Gerrit.RESOURCES.css().approvalhint());
   }
 
   private void applyScoreStyles(final int row) {
@@ -189,14 +192,14 @@ public class ApprovalTable extends Composite {
     reviewers.add(nameEmail);
 
     PatchUtil.DETAIL_SVC.addReviewers(changeId, reviewers,
-        new GerritCallback<AddReviewerResult>() {
+        new GerritCallback<ReviewerResult>() {
-          public void onSuccess(final AddReviewerResult result) {
+          public void onSuccess(final ReviewerResult result) {
             addMemberBox.setEnabled(true);
             addMemberBox.setText("");
 
             if (!result.getErrors().isEmpty()) {
               final SafeHtmlBuilder r = new SafeHtmlBuilder();
-              for (final AddReviewerResult.Error e : result.getErrors()) {
+              for (final ReviewerResult.Error e : result.getErrors()) {
                 switch (e.getType()) {
                   case ACCOUNT_NOT_FOUND:
                     r.append(Util.M.accountNotFound(e.getName()));
@@ -278,6 +281,31 @@ public class ApprovalTable extends Composite {
       col++;
     }
 
+    //
+    // Remove button
+    //
+    if (Gerrit.isSignedIn()) {
+      Button removeButton = new Button("X");
+      removeButton.setStyleName(Gerrit.RESOURCES.css().removeReviewer());
+      removeButton.addClickHandler(new ClickHandler() {
+        @Override
+        public void onClick(ClickEvent event) {
+          PatchUtil.DETAIL_SVC.removeReviewer(changeId, ad.getAccount(),
+              new GerritCallback<ReviewerResult>() {
+                @Override
+                public void onSuccess(ReviewerResult result) {
+                  if (result.getErrors().isEmpty()) {
+                    final ChangeDetail r = result.getChange();
+                    display(r.getChange(), r.getMissingApprovals(), r.getApprovals());
+                  } else {
+                    new ErrorDialog(result.getErrors().get(0).toString()).center();
+                  }
+                }
+          });
+        }
+      });
+      table.setWidget(row, col++, removeButton);
+    }
     table.setText(row, col++, hint.toString());
   }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java

@@ -69,6 +69,7 @@ public interface ChangeConstants extends Constants {
   String changeScreenComments();
 
   String approvalTableReviewer();
+  String removeReviewer();
   String approvalTableAddReviewer();
 
   String changeInfoBlockOwner();

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties

@@ -46,6 +46,7 @@ changeScreenNeededBy = Needed By
 changeScreenComments = Comments
 
 approvalTableReviewer = Reviewer
+removeReviewer = Remove
 approvalTableAddReviewer = Add Reviewer
 
 changeInfoBlockOwner = Owner

gerrit-gwtui/src/main/java/com/google/gerrit/client/gerrit.css

@@ -881,7 +881,9 @@
   margin-top: 5px;
   white-space: nowrap;
 }
-
+.removeReviewer {
+  width: 30px;
+}
 td.downloadLinkListCell {
   padding: 0px;
 }

gerrit-httpd/src/main/java/com/google/gerrit/httpd/RequireSslFilter.java

@@ -79,7 +79,7 @@ class RequireSslFilter implements Filter {
         url = b.toString();
 
       } else {
-        url = urlProvider.get();
+        url = urlProvider.get() + req.getServletPath();
       }
       rsp.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY);
       rsp.setHeader("Location", url);

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewer.java

@@ -14,7 +14,7 @@
 
 package com.google.gerrit.httpd.rpc.patch;
 
-import com.google.gerrit.common.data.AddReviewerResult;
+import com.google.gerrit.common.data.ReviewerResult;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
 import com.google.gerrit.httpd.rpc.Handler;
@@ -39,7 +39,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
-class AddReviewer extends Handler<AddReviewerResult> {
+class AddReviewer extends Handler<ReviewerResult> {
   interface Factory {
     AddReviewer create(Change.Id changeId, Collection<String> nameOrEmails);
   }
@@ -82,23 +82,23 @@ class AddReviewer extends Handler<AddReviewerResult> {
   }
 
   @Override
-  public AddReviewerResult call() throws Exception {
+  public ReviewerResult call() throws Exception {
     final Set<Account.Id> reviewerIds = new HashSet<Account.Id>();
     final ChangeControl control = changeControlFactory.validateFor(changeId);
 
-    final AddReviewerResult result = new AddReviewerResult();
+    final ReviewerResult result = new ReviewerResult();
     for (final String nameOrEmail : reviewers) {
       final Account account = accountResolver.find(nameOrEmail);
       if (account == null) {
-        result.addError(new AddReviewerResult.Error(
+        result.addError(new ReviewerResult.Error(
-            AddReviewerResult.Error.Type.ACCOUNT_NOT_FOUND, nameOrEmail));
+            ReviewerResult.Error.Type.ACCOUNT_NOT_FOUND, nameOrEmail));
         continue;
       }
 
       final IdentifiedUser user = identifiedUserFactory.create(account.getId());
       if (!control.forUser(user).isVisible()) {
-        result.addError(new AddReviewerResult.Error(
+        result.addError(new ReviewerResult.Error(
-            AddReviewerResult.Error.Type.CHANGE_NOT_VISIBLE, nameOrEmail));
+            ReviewerResult.Error.Type.CHANGE_NOT_VISIBLE, nameOrEmail));
         continue;
       }
 

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java

@@ -14,7 +14,7 @@
 
 package com.google.gerrit.httpd.rpc.patch;
 
-import com.google.gerrit.common.data.AddReviewerResult;
+import com.google.gerrit.common.data.ReviewerResult;
 import com.google.gerrit.common.data.ApprovalSummary;
 import com.google.gerrit.common.data.ApprovalSummarySet;
 import com.google.gerrit.common.data.ApprovalTypes;
@@ -60,6 +60,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
   private final AccountInfoCacheFactory.Factory accountInfoCacheFactory;
   private final AddReviewer.Factory addReviewerFactory;
   private final ChangeControl.Factory changeControlFactory;
+  private final RemoveReviewer.Factory removeReviewerFactory;
   private final FunctionState.Factory functionStateFactory;
   private final PublishComments.Factory publishCommentsFactory;
   private final PatchScriptFactory.Factory patchScriptFactoryFactory;
@@ -71,6 +72,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
       final ApprovalTypes approvalTypes,
       final AccountInfoCacheFactory.Factory accountInfoCacheFactory,
       final AddReviewer.Factory addReviewerFactory,
+      final RemoveReviewer.Factory removeReviewerFactory,
       final ChangeControl.Factory changeControlFactory,
       final FunctionState.Factory functionStateFactory,
       final PatchScriptFactory.Factory patchScriptFactoryFactory,
@@ -81,6 +83,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
 
     this.accountInfoCacheFactory = accountInfoCacheFactory;
     this.addReviewerFactory = addReviewerFactory;
+    this.removeReviewerFactory = removeReviewerFactory;
     this.changeControlFactory = changeControlFactory;
     this.functionStateFactory = functionStateFactory;
     this.patchScriptFactoryFactory = patchScriptFactoryFactory;
@@ -152,10 +155,15 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
   }
 
   public void addReviewers(final Change.Id id, final List<String> reviewers,
-      final AsyncCallback<AddReviewerResult> callback) {
+      final AsyncCallback<ReviewerResult> callback) {
     addReviewerFactory.create(id, reviewers).to(callback);
   }
 
+  public void removeReviewer(final Change.Id id, final Account.Id reviewerId,
+      final AsyncCallback<ReviewerResult> callback) {
+    removeReviewerFactory.create(id, reviewerId).to(callback);
+  }
+
   public void userApprovals(final Set<Change.Id> cids, final Account.Id aid,
       final AsyncCallback<ApprovalSummarySet> callback) {
     run(callback, new Action<ApprovalSummarySet>() {

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchModule.java

@@ -29,6 +29,7 @@ public class PatchModule extends RpcServletModule {
       @Override
       protected void configure() {
         factory(AddReviewer.Factory.class);
+        factory(RemoveReviewer.Factory.class);
         factory(PatchScriptFactory.Factory.class);
         factory(SaveDraft.Factory.class);
       }

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/RemoveReviewer.java

@@ -0,0 +1,96 @@
+// Copyright (C) 2010 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.patch;
+
+import com.google.gerrit.common.data.ReviewerResult;
+import com.google.gerrit.httpd.rpc.Handler;
+import com.google.gerrit.httpd.rpc.changedetail.ChangeDetailFactory;
+import com.google.gerrit.reviewdb.Account;
+import com.google.gerrit.reviewdb.Change;
+import com.google.gerrit.reviewdb.PatchSetApproval;
+import com.google.gerrit.reviewdb.ReviewDb;
+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 java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Implement the remote logic that removes a reviewer from a change.
+ */
+class RemoveReviewer extends Handler<ReviewerResult> {
+  interface Factory {
+    RemoveReviewer create(Change.Id changeId, Account.Id reviewerId);
+  }
+
+  private final Account.Id reviewerId;
+  private final ChangeControl.Factory changeControlFactory;
+  private final ReviewDb db;
+  private final Change.Id changeId;
+  private final ChangeDetailFactory.Factory changeDetailFactory;
+
+  @Inject
+  RemoveReviewer(final ReviewDb db, final ChangeControl.Factory changeControlFactory,
+      final ChangeDetailFactory.Factory changeDetailFactory,
+      @Assisted Change.Id changeId, @Assisted Account.Id reviewerId) {
+    this.db = db;
+    this.changeControlFactory = changeControlFactory;
+    this.changeId = changeId;
+    this.reviewerId = reviewerId;
+    this.changeDetailFactory = changeDetailFactory;
+  }
+
+  @Override
+  public ReviewerResult call() throws Exception {
+    ReviewerResult result = new ReviewerResult();
+    List<Account.Id> accounts = new ArrayList<Account.Id>();
+    ChangeControl ctl = changeControlFactory.validateFor(changeId);
+    boolean permitted = true;
+
+    List<PatchSetApproval> toDelete = new ArrayList<PatchSetApproval>();
+    for (PatchSetApproval psa : db.patchSetApprovals().byChange(changeId)) {
+      if (psa.getAccountId().equals(reviewerId)) {
+        if (ctl.canRemoveReviewer(psa)) {
+          toDelete.add(psa);
+        } else {
+          permitted = false;
+          break;
+        }
+      }
+    }
+
+    if (permitted) {
+      try {
+        db.patchSetApprovals().delete(toDelete);
+      } catch (OrmException ex) {
+        result.addError(new ReviewerResult.Error(
+            ReviewerResult.Error.Type.COULD_NOT_REMOVE,
+            "Could not remove reviewer " + reviewerId));
+      }
+    } else {
+      result.addError(new ReviewerResult.Error(
+          ReviewerResult.Error.Type.COULD_NOT_REMOVE,
+          "Not allowed to remove reviewer " + reviewerId));
+    }
+
+    // Note: call setChange() after the deletion has been made or it will still
+    // contain the reviewer we want to delete.
+    result.setChange(changeDetailFactory.create(changeId).call());
+    return result;
+  }
+
+}

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -14,7 +14,9 @@
 
 package com.google.gerrit.server.project;
 
+import com.google.gerrit.reviewdb.Account;
 import com.google.gerrit.reviewdb.Change;
+import com.google.gerrit.reviewdb.PatchSetApproval;
 import com.google.gerrit.reviewdb.Project;
 import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
@@ -141,4 +143,34 @@ public class ChangeControl {
     }
     return false;
   }
+
+  /** @return true if the user is allowed to remove this reviewer. */
+  public boolean canRemoveReviewer(PatchSetApproval approval) {
+    if (getChange().getStatus().isOpen()) {
+      // A user can always remove themselves.
+      //
+      if (getCurrentUser() instanceof IdentifiedUser) {
+        final IdentifiedUser i = (IdentifiedUser) getCurrentUser();
+        if (i.getAccountId().equals(approval.getAccountId())) {
+          return true; // can remove self
+        }
+      }
+
+      // The change owner may remove any zero or positive score.
+      //
+      if (isOwner() && 0 <= approval.getValue()) {
+        return true;
+      }
+
+      // The branch owner, project owner, site admin can remove anyone.
+      //
+      if (getRefControl().isOwner() // branch owner
+          || getProjectControl().isOwner() // project owner
+          || getCurrentUser().isAdministrator()) {
+        return true;
+      }
+    }
+
+    return false;
+  }
 }