Skip to main content
summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRileyDavidson2016-10-04 21:08:10 -0400
committerGerrit Code Review @ Eclipse.org2016-11-25 13:38:22 -0500
commit18959764c1e741bff66407eae236c91ef0a4f896 (patch)
treecf31c898e14183e5a67d4423571f8f03a2961a99
parentfbd2282d75a5df8f08fa1798d1528e6b9efd9a98 (diff)
downloadorg.eclipse.mylyn.reviews-18959764c1e741bff66407eae236c91ef0a4f896.tar.gz
org.eclipse.mylyn.reviews-18959764c1e741bff66407eae236c91ef0a4f896.tar.xz
org.eclipse.mylyn.reviews-18959764c1e741bff66407eae236c91ef0a4f896.zip
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>
-rw-r--r--org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java59
-rw-r--r--org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritReviewRemoteFactoryTest.java147
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());

Back to the top