357234: support removing reviewers through the review editor

Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=357234

Change-Id: I07e09cc4f3c03e1369705da5cad42cfca655390c
Signed-off-by: RileyDavidson <rd12hq@brocku.ca>

2 files changed, 155 insertions, 51 deletions

diff --git a/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java b/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java
index 6af841ab5..97e753076 100644
--- a/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java
+++ b/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java
@@ -126,7 +126,7 @@ import com.google.gwtjsonrpc.client.VoidResult;
 
 /**
  * Facade to the Gerrit RPC API.
- * 
+ *
  * @author Mikael Kober
  * @author Thomas Westling
  * @author Steffen Pingel
@@ -343,13 +343,13 @@ public abstract class GerritClient extends ReviewsClient {
 			throws GerritException {
 		List<Project> result = new ArrayList<Project>();
 		try {
-			List<ProjectDetailX> projectDetails = restClient.execute(monitor, new Operation<List<ProjectDetailX>>(
-					client) {
-				@Override
-				public void execute(IProgressMonitor monitor) throws GerritException {
-					getProjectAdminService(monitor).visibleProjectDetails(this);
-				}
-			});
+			List<ProjectDetailX> projectDetails = restClient.execute(monitor,
+					new Operation<List<ProjectDetailX>>(client) {
+						@Override
+						public void execute(IProgressMonitor monitor) throws GerritException {
+							getProjectAdminService(monitor).visibleProjectDetails(this);
+						}
+					});
 			for (ProjectDetailX projectDetail : projectDetails) {
 				if (!GerritUtil.isPermissionOnlyProject(projectDetail, gerritConfig)) {
 					result.add(projectDetail.project);
@@ -454,8 +454,8 @@ public abstract class GerritClient extends ReviewsClient {
 		return patchScript;
 	}
 
-	protected void fetchLeftBinaryContent(final PatchScriptX patchScript, final Patch.Key key,
-			final PatchSet.Id leftId, final IProgressMonitor monitor) throws GerritException {
+	protected void fetchLeftBinaryContent(final PatchScriptX patchScript, final Patch.Key key, final PatchSet.Id leftId,
+			final IProgressMonitor monitor) throws GerritException {
 		if (EnumSet.of(ChangeType.DELETED, ChangeType.MODIFIED).contains(patchScript.getChangeType())) {
 			byte[] binaryContent = fetchBinaryContent(getUrlForPatchSetOrBase(key, leftId), monitor);
 			patchScript.setBinaryA(binaryContent);
@@ -499,7 +499,7 @@ public abstract class GerritClient extends ReviewsClient {
 
 	/**
 	 * Checks for the 4 byte header that identifies a ZIP file
-	 * 
+	 *
 	 * @noreference This method is not intended to be referenced by clients.
 	 */
 	public static boolean isZippedContent(byte[] bin) {
@@ -580,8 +580,8 @@ public abstract class GerritClient extends ReviewsClient {
 		ReviewerResult reviewerResult = new ReviewerResult();
 		for (final String reviewerId : reviewers) {
 			try {
-				AddReviewerResult addReviewerResult = restClient.executePostRestRequest(uri, new ReviewerInput(
-						reviewerId), AddReviewerResult.class, null, monitor);
+				AddReviewerResult addReviewerResult = restClient.executePostRestRequest(uri,
+						new ReviewerInput(reviewerId), AddReviewerResult.class, null, monitor);
 				reviewerInfos.addAll(addReviewerResult.getReviewers());
 			} catch (GerritHttpException e) {
 				if (e.getResponseCode() == HttpStatus.SC_UNPROCESSABLE_ENTITY) {
@@ -601,6 +601,39 @@ public abstract class GerritClient extends ReviewsClient {
 		return reviewerResult;
 	}
 
+	public ReviewerResult removeReviewer(String reviewId, String reviewerId, IProgressMonitor monitor)
+			throws GerritException {
+		Change.Id id = new Change.Id(id(reviewId));
+
+		String uri = "/a/changes/" + id.get() + "/reviewers/" + reviewerId; //$NON-NLS-1$ //$NON-NLS-2$
+		ReviewerResult reviewerResult = new ReviewerResult();
+
+		//Try to remove the reviewer from the change
+		try {
+			//Currently using a string to return as the only response from the gerrit api will be an http status
+			String result = restClient.executeDeleteRestRequest(uri, new ReviewerInput(reviewerId), String.class, null,
+					monitor);
+		} catch (GerritHttpException e) {
+			if (e.getResponseCode() == HttpStatus.SC_NOT_FOUND) {
+				reviewerResult.addError(new ReviewerResult.Error(null, reviewerId));
+			}
+
+		}
+
+		//Now that the reviewer has been removed, remove that reviewers approvals on the change
+		ChangeDetail changeDetail = getChangeDetail(id.get(), monitor);
+		List<ApprovalDetail> approvalDetails = new ArrayList<>();
+		for (ApprovalDetail approval : changeDetail.getApprovals()) {
+			//If this approval is not made by the user we are removing, add it to the list of approvals
+			if (!approval.getAccount().equals(reviewerId)) {
+				approvalDetails.add(approval);
+			}
+		}
+		changeDetail.setApprovals(approvalDetails);
+		reviewerResult.setChange(changeDetail);
+		return reviewerResult;
+	}
+
 	protected void merge(AccountInfoCache accounts, List<ReviewerInfo> reviewers) {
 		Set<com.google.gerrit.common.data.AccountInfo> accountInfos = new HashSet<com.google.gerrit.common.data.AccountInfo>(
 				reviewers.size());
diff --git a/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java b/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java
index c1a55949c..19179f1c6 100644
--- a/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java
+++ b/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java
@@ -178,7 +178,7 @@ public class GerritReviewRemoteFactoryTest extends GerritRemoteTest {
 		CommitCommand commandDep1 = reviewHarness.createCommitCommand(changeIdDep1);
 		reviewHarness.addFile("testFile1.txt", "test 2");
 		CommitResult resultDep1 = reviewHarness.commitAndPush(commandDep1);
-		String resultIdDep1 = reviewHarness.parseShortId(resultDep1.push.getMessages());
+		String resultIdDep1 = ReviewHarness.parseShortId(resultDep1.push.getMessages());
 		assertThat("Bad Push: " + resultDep1.push.getMessages(), resultIdDep1.length(), greaterThan(0));
 
 		TestRemoteObserverConsumer<IRepository, IReview, String, GerritChange, String, Date> consumerDep1 = retrieveForRemoteKey(
@@ -303,11 +303,10 @@ public class GerritReviewRemoteFactoryTest extends GerritRemoteTest {
 
 	@Test
 	public void testAddSomeInvalidReviewers() throws Exception {
-		List<String> reviewers = Arrays.asList(new String[] { "tests", "foo" });
-		int userid = 1000001; //user id for tests
+
 		//use "admin " since this is a valid user in 2.9
-		reviewers = Arrays.asList(new String[] { "admin", "foo" });
-		userid = 1000000; //user id for admin
+		List<String> reviewers = Arrays.asList(new String[] { "admin", "foo" });
+		List<Integer> userid = Arrays.asList(new Integer[] { 1000000 }); //user id for tests
 
 		ReviewerResult reviewerResult = reviewHarness.getClient().addReviewers(reviewHarness.getShortId(), reviewers,
 				new NullProgressMonitor());
@@ -318,25 +317,90 @@ public class GerritReviewRemoteFactoryTest extends GerritRemoteTest {
 	@Test
 	public void testAddReviewers() throws Exception {
 		assertThat(getReview().getReviewerApprovals().isEmpty(), is(true));
-		List<String> reviewers = Arrays.asList(new String[] { "tests" });
-		int userid = 1000001; //user id for tests
-		//Need a user and not the review owner
-		reviewers = Arrays.asList(new String[] { "admin" });
-		userid = 1000000; //user id for admin
 
-		ReviewerResult reviewerResult = reviewHarness.getClient().addReviewers(reviewHarness.getShortId(), reviewers,
+		addAdminToReviewAndVerify();
+	}
+
+	@Test
+	public void testAddReviewerThenRemoveReviewer() throws Exception {
+		assertThat(getReview().getReviewerApprovals().isEmpty(), is(true)); //Make sure theres no reviewers
+
+		addAdminToReviewAndVerify();
+
+		String reviewer = "admin";
+		ReviewerResult removeReviewerResult = reviewHarness.getClient().removeReviewer(reviewHarness.getShortId(),
+				reviewer, new NullProgressMonitor());
+		reviewHarness.retrieve();
+
+		//I went through the checks after we add a reviewer and reversed them to look for empty rather than for a new reviewer
+		assertThat(getReview().getReviewerApprovals().isEmpty(), is(true));
+		assertThat(removeReviewerResult, notNullValue());
+		assertTrue(removeReviewerResult.getErrors().isEmpty());
+
+		List<ApprovalDetail> approvals = removeReviewerResult.getChange().getApprovals();
+		assertThat(approvals.isEmpty(), is(true));
+		assertThat(approvals.size(), is(0));
+
+	}
+
+	@Test
+	public void testAddReviewerAndRemoveInvalidReviewer() throws Exception {
+		addAdminToReviewAndVerify();
+
+		//Foo isnt a valid user
+		String reviewer = "foo";
+
+		ReviewerResult removeReviewerResult = reviewHarness.getClient().removeReviewer(reviewHarness.getShortId(),
+				reviewer, new NullProgressMonitor());
+
+		reviewHarness.retrieve();
+
+		assertThat(removeReviewerResult, notNullValue());
+		assertThat(removeReviewerResult.getErrors().size(), is(1));
+		assertThat(removeReviewerResult.getErrors().get(0).getType(), nullValue());
+		assertThat(removeReviewerResult.getErrors().get(0).getName(), is(reviewer));
+
+		List<ApprovalDetail> approvals = removeReviewerResult.getChange().getApprovals();
+		assertThat(approvals.isEmpty(), is(false));
+		assertThat(approvals.size(), is(1));
+
+	}
+
+	@Test
+	public void testAddReviewerAndRemoveValidReviewerNotOnReview() throws Exception {
+		addAdminToReviewAndVerify();
+
+		String reviewer = "tests";
+
+		ReviewerResult removeReviewerResult = reviewHarness.getClient().removeReviewer(reviewHarness.getShortId(),
+				reviewer, new NullProgressMonitor());
+
+		reviewHarness.retrieve();
+
+		assertThat(removeReviewerResult, notNullValue());
+		assertThat(removeReviewerResult.getErrors().size(), is(1));
+		assertThat(removeReviewerResult.getErrors().get(0).getType(), nullValue());
+		assertThat(removeReviewerResult.getErrors().get(0).getName(), is(reviewer));
+
+		List<ApprovalDetail> approvals = removeReviewerResult.getChange().getApprovals();
+		assertThat(approvals.isEmpty(), is(false));
+		assertThat(approvals.size(), is(1));
+
+	}
+
+	private void addAdminToReviewAndVerify() throws Exception {
+		List<String> reviewers = Arrays.asList(new String[] { "admin" });
+		List<Integer> userid = Arrays.asList(new Integer[] { 1000000 });
+		ReviewerResult addReviewerResult = reviewHarness.getClient().addReviewers(reviewHarness.getShortId(), reviewers,
 				new NullProgressMonitor());
 		reviewHarness.retrieve();
-		assertReviewerResult(reviewerResult, null, userid);
+		assertReviewerResult(addReviewerResult, null, userid);
 	}
 
 	@Test
 	public void testAddReviewersByEmail() throws Exception {
-		List<String> reviewers = Arrays.asList(new String[] { "tests@mylyn.eclipse.org" });
-		int userid = 1000001; //user id for tests
-		//Need a user and not the review owner
-		reviewers = Arrays.asList(new String[] { "admin@mylyn.eclipse.org" });
-		userid = 1000000; //user id for admin
+		List<String> reviewers = Arrays.asList(new String[] { "admin@mylyn.eclipse.org" });
+		List<Integer> userid = Arrays.asList(new Integer[] { 1000000 }); //user id for tests
 
 		ReviewerResult reviewerResult = reviewHarness.getClient().addReviewers(reviewHarness.getShortId(), reviewers,
 				new NullProgressMonitor());
@@ -344,9 +408,11 @@ public class GerritReviewRemoteFactoryTest extends GerritRemoteTest {
 		assertReviewerResult(reviewerResult, null, userid);
 	}
 
-	private void assertReviewerResult(ReviewerResult reviewerResult, String nameInErrors, int userId) {
+	private void assertReviewerResult(ReviewerResult reviewerResult, String nameInErrors, List<Integer> userIds) {
 		assertThat(reviewerResult, notNullValue());
 
+		int numReviewers = userIds.size();
+
 		assertThat(reviewerResult.getErrors().isEmpty(), is(nameInErrors == null));
 		if (nameInErrors != null) {
 			assertThat(reviewerResult.getErrors().size(), is(1));
@@ -356,26 +422,31 @@ public class GerritReviewRemoteFactoryTest extends GerritRemoteTest {
 
 		List<ApprovalDetail> approvals = reviewerResult.getChange().getApprovals();
 		assertThat(approvals.isEmpty(), is(false));
-		assertThat(approvals.size(), is(1));
-		assertThat(approvals.get(0).getAccount().get(), is(userId));
-
-		Map<ApprovalCategory.Id, PatchSetApproval> approvalMap = approvals.get(0).getApprovalMap();
-		assertThat(approvalMap, notNullValue());
-		assertThat(approvalMap.isEmpty(), is(false));
-		assertThat(approvalMap.size(), is(1));
-
-		PatchSetApproval crvw = approvalMap.get(CRVW.getCategory().getId());
-		assertThat(crvw, notNullValue());
-		assertThat(crvw.getAccountId().get(), is(userId));
-		assertThat(crvw.getValue(), is((short) 0));
-		assertThat(crvw.getGranted(), notNullValue());
-		assertThat(crvw.getPatchSetId(), notNullValue());
-		assertThat(crvw.getPatchSetId().get(), is(1));
-		assertThat(crvw.getPatchSetId().getParentKey().get(), is(Integer.parseInt(getReview().getId())));
+		assertThat(approvals.size(), is(numReviewers));
+
+		for (int i = 0; i < numReviewers; i++) {
+			assertThat(approvals.get(i).getAccount().get(), is(userIds.get(i)));
+
+			Map<ApprovalCategory.Id, PatchSetApproval> approvalMap = approvals.get(i).getApprovalMap();
+			assertThat(approvalMap, notNullValue());
+			assertThat(approvalMap.isEmpty(), is(false));
+			assertThat(approvalMap.size(), is(1));
+
+			PatchSetApproval crvw = approvalMap.get(CRVW.getCategory().getId());
+			assertThat(crvw, notNullValue());
+			assertThat(crvw.getAccountId().get(), is(userIds.get(i)));
+			assertThat(crvw.getValue(), is((short) 0));
+			assertThat(crvw.getGranted(), notNullValue());
+			assertThat(crvw.getPatchSetId(), notNullValue());
+			assertThat(crvw.getPatchSetId().get(), is(1));
+			assertThat(crvw.getPatchSetId().getParentKey().get(), is(Integer.parseInt(getReview().getId())));
+
+			assertThat(getReview().getReviewerApprovals().get(i), nullValue());
+		}
 
 		assertThat(getReview().getReviewerApprovals().isEmpty(), is(false));
-		assertThat(getReview().getReviewerApprovals().size(), is(1));
-		assertThat(getReview().getReviewerApprovals().get(0), nullValue());
+		assertThat(getReview().getReviewerApprovals().size(), is(numReviewers));
+
 	}
 
 	@Test
@@ -551,7 +622,7 @@ public class GerritReviewRemoteFactoryTest extends GerritRemoteTest {
 		CommitCommand commandNewChange = reviewHarness.createCommitCommand(changeIdNewChange);
 		reviewHarness.addFile("testFileNewChange.txt");
 		CommitResult result = reviewHarness.commitAndPush(commandNewChange);
-		String newReviewShortId = reviewHarness.parseShortId(result.push.getMessages());
+		String newReviewShortId = ReviewHarness.parseShortId(result.push.getMessages());
 
 		TestRemoteObserver<IRepository, IReview, String, Date> newReviewListener = new TestRemoteObserver<IRepository, IReview, String, Date>(
 				reviewHarness.getProvider().getReviewFactory());
@@ -632,7 +703,7 @@ public class GerritReviewRemoteFactoryTest extends GerritRemoteTest {
 		CommitCommand commandNewChange = reviewHarness.createCommitCommand(changeIdNewChange);
 		reviewHarness.addFile("testFileNewChange.txt");
 		CommitResult result = reviewHarness.commitAndPush(commandNewChange);
-		String newReviewShortId = reviewHarness.parseShortId(result.push.getMessages());
+		String newReviewShortId = ReviewHarness.parseShortId(result.push.getMessages());
 
 		TestRemoteObserver<IRepository, IReview, String, Date> newReviewListener = new TestRemoteObserver<IRepository, IReview, String, Date>(
 				reviewHarness.getProvider().getReviewFactory());