Re-do and repair Conduit's maniphestEdit
maniphestEdit was split out over a few methods, nested in unnecessary
`if`s, and added projects already on a Maniphest and did not
effectively allow to delete projects. We simplify the code, make it
work, and add tests.
Change-Id: I2f114cd58fa2c6d173c7f9bfb92d9081a54af673
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/PhabricatorItsFacade.java b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/PhabricatorItsFacade.java
index e3d2e11..94ad847 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/PhabricatorItsFacade.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/PhabricatorItsFacade.java
@@ -48,7 +48,7 @@
public void addComment(final String bugId, final String comment) throws IOException {
int task_id = Integer.parseInt(bugId);
try {
- conduit.maniphestEdit(task_id, comment);
+ conduit.maniphestEdit(task_id, comment, null, null);
} catch (ConduitException e) {
throw new IOException("Could not update message for task " + task_id, e);
}
@@ -81,19 +81,21 @@
String chopped[] = actionString.split(" ");
if (chopped.length >= 1) {
String action = chopped[0];
- switch (action) {
- case "add-project":
- assertParameters(action, chopped, 1);
-
- conduit.maniphestEdit(chopped[1], taskId, Conduit.ACTION_PROJECT_ADD);
- break;
- case "remove-project":
- assertParameters(action, chopped, 1);
-
- conduit.maniphestEdit(chopped[1], taskId, Conduit.ACTION_PROJECT_REMOVE);
- break;
- default:
- throw new IOException("Unknown action " + action);
+ try {
+ switch (action) {
+ case "add-project":
+ assertParameters(action, chopped, 1);
+ conduit.maniphestEdit(taskId, null, chopped[1], null);
+ break;
+ case "remove-project":
+ assertParameters(action, chopped, 1);
+ conduit.maniphestEdit(taskId, null, null, chopped[1]);
+ break;
+ default:
+ throw new IOException("Unknown action " + action);
+ }
+ } catch (ConduitException e) {
+ throw new IOException("Could not perform action " + action, e);
}
} else {
throw new IOException("Could not parse action " + actionString);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/Conduit.java b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/Conduit.java
index 0a61f34..cea0908 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/Conduit.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/Conduit.java
@@ -14,9 +14,9 @@
package com.googlesource.gerrit.plugins.its.phabricator.conduit;
+import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Sets;
import com.google.gson.Gson;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
@@ -26,12 +26,10 @@
import com.googlesource.gerrit.plugins.its.phabricator.conduit.results.ManiphestEdit;
import com.googlesource.gerrit.plugins.its.phabricator.conduit.results.ManiphestSearch;
import com.googlesource.gerrit.plugins.its.phabricator.conduit.results.ProjectSearch;
-import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.Set;
/**
* Bindings for Phabricator's Conduit API
@@ -92,68 +90,44 @@
}
/** Runs the API's 'maniphest.edit' method */
- public ManiphestEdit maniphestEdit(int taskId, String comment) throws ConduitException {
- return maniphestEdit(taskId, comment, null, ACTION_COMMENT);
- }
-
- /** Runs the API's 'maniphest.edit' method */
- public ManiphestEdit maniphestEdit(int taskId, Iterable<String> projects, String action)
- throws ConduitException {
- return maniphestEdit(taskId, null, projects, action);
- }
-
- public void maniphestEdit(String projectName, int taskId, String actions) throws IOException {
- try {
- ProjectSearch projectSearch = projectSearch(projectName);
- String projectPhid = projectSearch.getPhid();
-
- Set<String> projectPhids = Sets.newHashSet(projectPhid);
-
- ManiphestSearch taskSearch = maniphestSearch(taskId);
- for (JsonElement jsonElement2 :
- taskSearch.getAttachments().getProjects().getProjectPHIDs().getAsJsonArray()) {
- projectPhids.add(jsonElement2.getAsString());
- }
-
- maniphestEdit(taskId, projectPhids, actions);
- } catch (ConduitException e) {
- throw new IOException("Error on conduit", e);
- }
- }
-
- /** Runs the API's 'maniphest.edit' method */
public ManiphestEdit maniphestEdit(
- int taskId, String comment, Iterable<String> projects, String action)
+ int taskId, String comment, String projectNameToAdd, String projectNameToRemove)
throws ConduitException {
- HashMap<String, Object> params = new HashMap<>();
- HashMap<String, Object> transaction = new HashMap<>();
+ ManiphestEdit result = null;
List<Object> transactions = new ArrayList<>();
- transaction.put("type", action);
- if (action.equals(ACTION_COMMENT)) {
- if (comment == null) {
- throw new IllegalArgumentException(
- "The value of comment (null) is invalid for the action" + action);
- }
+
+ if (!Strings.isNullOrEmpty(comment)) {
+ HashMap<String, Object> transaction = new HashMap<>();
+ transaction.put("type", ACTION_COMMENT);
transaction.put("value", comment);
- }
- if (action.equals(ACTION_PROJECT_ADD) || action.equals(ACTION_PROJECT_REMOVE)) {
- if ((action.equals(ACTION_PROJECT_ADD) || action.equals(ACTION_PROJECT_REMOVE))
- && projects == null) {
- throw new IllegalArgumentException(
- "The value of projects (null) is invalid for the action " + action);
- }
- transaction.put("value", projects);
- }
-
- if (!transaction.isEmpty()) {
transactions.add(transaction);
- params.put("transactions", transactions);
}
- params.put("objectIdentifier", taskId);
- JsonElement callResult = conduitConnection.call("maniphest.edit", params, token);
- ManiphestEdit result = gson.fromJson(callResult, ManiphestEdit.class);
+ if (!Strings.isNullOrEmpty(projectNameToAdd)) {
+ HashMap<String, Object> transaction = new HashMap<>();
+ transaction.put("type", ACTION_PROJECT_ADD);
+ transaction.put("value", ImmutableList.of(projectSearch(projectNameToAdd).getPhid()));
+
+ transactions.add(transaction);
+ }
+
+ if (!Strings.isNullOrEmpty(projectNameToRemove)) {
+ HashMap<String, Object> transaction = new HashMap<>();
+ transaction.put("type", ACTION_PROJECT_REMOVE);
+ transaction.put("value", ImmutableList.of(projectSearch(projectNameToRemove).getPhid()));
+
+ transactions.add(transaction);
+ }
+
+ if (!transactions.isEmpty()) {
+ HashMap<String, Object> params = new HashMap<>();
+ params.put("objectIdentifier", taskId);
+ params.put("transactions", transactions);
+ JsonElement callResult = conduitConnection.call("maniphest.edit", params, token);
+ result = gson.fromJson(callResult, ManiphestEdit.class);
+ }
+
return result;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/GenericEdit.java b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/GenericEdit.java
new file mode 100644
index 0000000..b01db44
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/GenericEdit.java
@@ -0,0 +1,57 @@
+// Copyright (C) 2020 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.googlesource.gerrit.plugins.its.phabricator.conduit.results;
+
+import java.util.List;
+
+/**
+ * Models the result for API methods
+ *
+ * <p>JSON looks like:
+ *
+ * <pre>
+ * {
+ * "object":{
+ * "id":2,
+ * "phid":"PHID-TASK-wzydcwamkp5rjhg45ocq"
+ * },
+ * "transactions":[
+ * {"phid":"PHID-XACT-TASK-sghfp7saytwmun3"}
+ * ]
+ * }
+ * </pre>
+ */
+public class GenericEdit {
+ private ResultObject object;
+ private List<Transaction> transactions;
+
+ public ResultObject getObject() {
+ return object;
+ }
+
+ public List<Transaction> getTransactions() {
+ return transactions;
+ }
+
+ public class Transaction extends PhabObject {}
+
+ public class ResultObject extends PhabObject {
+ private int id;
+
+ public int getId() {
+ return id;
+ }
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/ManiphestEdit.java b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/ManiphestEdit.java
index e9e3805..dc416dd 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/ManiphestEdit.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/ManiphestEdit.java
@@ -20,31 +20,14 @@
*
* <pre>
* {
- * "id":"48",
- * "phid":"PHID-TASK-pemd324eosnymq3tdkyo",
- * "authorPHID":"PHID-USER-na3one2sht11aone",
- * "ownerPHID":null,
- * "ccPHIDs":[
- * "PHID-USER-h4n62fq2kt2v3a2qjyqh"
- * ],
- * "status":"open",
- * "statusName":"Open",
- * "isClosed":false,
- * "priority": "Needs Triage",
- * "priorityColor":"violet",
- * "title":"QChris test task",
- * "description":"",
- * "projectPHIDs":[],
- * "uri":"https://phab-01.wmflabs.org/T47",
- * "auxiliary":{
- * "std:maniphest:security_topic":"default",
- * "isdc:sprint:storypoints":null
+ * "object":{
+ * "id":2,
+ * "phid":"PHID-TASK-wzydcwamkp5rjhg45ocq"
* },
- * "objectName":"T47",
- * "dateCreated":"1413484594",
- * "dateModified":1413549869,
- * "dependsOnTaskPHIDs":[]
+ * "transactions":[
+ * {"phid":"PHID-XACT-TASK-sghfp7saytwmun3"}
+ * ]
* }
* </pre>
*/
-public class ManiphestEdit extends Task {}
+public class ManiphestEdit extends GenericEdit {}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/PhabObject.java b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/PhabObject.java
new file mode 100644
index 0000000..bc2cd46
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/results/PhabObject.java
@@ -0,0 +1,23 @@
+// Copyright (C) 2020 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.googlesource.gerrit.plugins.its.phabricator.conduit.results;
+
+public class PhabObject {
+ private String phid;
+
+ public String getPhid() {
+ return phid;
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/ConduitTest.java b/src/test/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/ConduitTest.java
index 20f9f17..6a57860 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/ConduitTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/its/phabricator/conduit/ConduitTest.java
@@ -15,11 +15,15 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gson.JsonArray;
import com.google.gson.JsonElement;
@@ -27,6 +31,7 @@
import com.google.gson.JsonPrimitive;
import com.googlesource.gerrit.plugins.its.base.testutil.LoggingMockingTestCase;
import com.googlesource.gerrit.plugins.its.phabricator.conduit.results.ConduitPing;
+import com.googlesource.gerrit.plugins.its.phabricator.conduit.results.ManiphestEdit;
import com.googlesource.gerrit.plugins.its.phabricator.conduit.results.ProjectSearch;
import java.util.HashMap;
import java.util.Map;
@@ -109,7 +114,166 @@
assertThat(actual.getPhid()).isEqualTo("PHID-PROJ-foo");
}
- // TODO: Add tests for maniphestEdit
+ @Test
+ public void testManiphestEditNoop() throws Exception {
+ Conduit conduit = createConduit();
+ ManiphestEdit actual = conduit.maniphestEdit(4711, null, null, null);
+
+ verifyZeroInteractions(conduitConnection);
+ assertThat(actual).isNull();
+ }
+
+ @Test
+ public void testManiphestEditEmpty() throws Exception {
+ Conduit conduit = createConduit();
+ ManiphestEdit actual = conduit.maniphestEdit(4711, "", "", "");
+
+ verifyZeroInteractions(conduitConnection);
+ assertThat(actual).isNull();
+ }
+
+ @Test
+ public void testManiphestEditAddComment() throws Exception {
+ Map<String, Object> transaction = new HashMap<>();
+ transaction.put("type", "comment");
+ transaction.put("value", "foo");
+
+ Map<String, Object> params = new HashMap<>();
+ params.put("objectIdentifier", 4711);
+ params.put("transactions", ImmutableList.of(transaction));
+
+ JsonArray data = new JsonArray();
+ data.add(createProjectJson(2, "foo"));
+
+ JsonObject response = createEditResponse(1);
+ when(conduitConnection.call("maniphest.edit", params, TOKEN)).thenReturn(response);
+
+ Conduit conduit = createConduit();
+
+ ManiphestEdit actual = conduit.maniphestEdit(4711, "foo", null, null);
+
+ assertThat(actual.getObject().getId()).isEqualTo(4712);
+ assertThat(actual.getObject().getPhid()).isEqualTo("PHID-foo");
+ assertThat(actual.getTransactions()).hasSize(1);
+ assertThat(actual.getTransactions().get(0).getPhid()).isEqualTo("trans@0");
+ }
+
+ @Test
+ public void testManiphestEditAddProject() throws Exception {
+ Map<String, Object> transaction = new HashMap<>();
+ transaction.put("type", "projects.add");
+ transaction.put("value", ImmutableList.of("PHID-bar"));
+
+ Map<String, Object> params = new HashMap<>();
+ params.put("objectIdentifier", 4711);
+ params.put("transactions", ImmutableList.of(transaction));
+
+ JsonArray data = new JsonArray();
+ data.add(createProjectJson(2, "foo"));
+
+ JsonObject response = createEditResponse(1);
+ when(conduitConnection.call("maniphest.edit", params, TOKEN)).thenReturn(response);
+
+ Conduit conduit = spy(createConduit());
+
+ // shortcut the needed project search
+ doReturn(new ProjectSearch(12, "PHID-bar")).when(conduit).projectSearch("foo");
+
+ ManiphestEdit actual = conduit.maniphestEdit(4711, null, "foo", null);
+
+ assertThat(actual.getObject().getId()).isEqualTo(4712);
+ assertThat(actual.getObject().getPhid()).isEqualTo("PHID-foo");
+ assertThat(actual.getTransactions()).hasSize(1);
+ assertThat(actual.getTransactions().get(0).getPhid()).isEqualTo("trans@0");
+ }
+
+ @Test
+ public void testManiphestEditRemoveProject() throws Exception {
+ Map<String, Object> transaction = new HashMap<>();
+ transaction.put("type", "projects.remove");
+ transaction.put("value", ImmutableList.of("PHID-bar"));
+
+ Map<String, Object> params = new HashMap<>();
+ params.put("objectIdentifier", 4711);
+ params.put("transactions", ImmutableList.of(transaction));
+
+ JsonArray data = new JsonArray();
+ data.add(createProjectJson(2, "foo"));
+
+ JsonObject response = createEditResponse(1);
+ when(conduitConnection.call("maniphest.edit", params, TOKEN)).thenReturn(response);
+
+ Conduit conduit = spy(createConduit());
+
+ // shortcut the needed project search
+ doReturn(new ProjectSearch(12, "PHID-bar")).when(conduit).projectSearch("foo");
+
+ ManiphestEdit actual = conduit.maniphestEdit(4711, null, null, "foo");
+
+ assertThat(actual.getObject().getId()).isEqualTo(4712);
+ assertThat(actual.getObject().getPhid()).isEqualTo("PHID-foo");
+ assertThat(actual.getTransactions()).hasSize(1);
+ assertThat(actual.getTransactions().get(0).getPhid()).isEqualTo("trans@0");
+ }
+
+ @Test
+ public void testManiphestEditAllParams() throws Exception {
+ Map<String, Object> transaction1 = new HashMap<>();
+ transaction1.put("type", "comment");
+ transaction1.put("value", "foo");
+
+ Map<String, Object> transaction2 = new HashMap<>();
+ transaction2.put("type", "projects.add");
+ transaction2.put("value", ImmutableList.of("PHID-bar"));
+
+ Map<String, Object> transaction3 = new HashMap<>();
+ transaction3.put("type", "projects.remove");
+ transaction3.put("value", ImmutableList.of("PHID-baz"));
+
+ Map<String, Object> params = new HashMap<>();
+ params.put("objectIdentifier", 4711);
+ params.put("transactions", ImmutableList.of(transaction1, transaction2, transaction3));
+
+ JsonArray data = new JsonArray();
+ data.add(createProjectJson(2, "foo"));
+
+ JsonObject response = createEditResponse(3);
+ when(conduitConnection.call("maniphest.edit", params, TOKEN)).thenReturn(response);
+
+ Conduit conduit = spy(createConduit());
+
+ // shortcut the needed project searches
+ doReturn(new ProjectSearch(12, "PHID-bar")).when(conduit).projectSearch("bar");
+ doReturn(new ProjectSearch(12, "PHID-baz")).when(conduit).projectSearch("baz");
+
+ ManiphestEdit actual = conduit.maniphestEdit(4711, "foo", "bar", "baz");
+
+ assertThat(actual.getObject().getId()).isEqualTo(4712);
+ assertThat(actual.getObject().getPhid()).isEqualTo("PHID-foo");
+ assertThat(actual.getTransactions()).hasSize(3);
+ assertThat(actual.getTransactions().get(0).getPhid()).isEqualTo("trans@0");
+ assertThat(actual.getTransactions().get(1).getPhid()).isEqualTo("trans@1");
+ assertThat(actual.getTransactions().get(2).getPhid()).isEqualTo("trans@2");
+ }
+
+ private JsonObject createEditResponse(int transactions) {
+ JsonObject resultObject = new JsonObject();
+ resultObject.addProperty("id", 4712);
+ resultObject.addProperty("phid", "PHID-foo");
+
+ JsonArray transactionArray = new JsonArray();
+ for (int i = 0; i < transactions; i++) {
+ JsonObject transaction = new JsonObject();
+ transaction.addProperty("phid", "trans@" + i);
+ transactionArray.add(transaction);
+ }
+
+ JsonObject response = new JsonObject();
+ response.add("object", resultObject);
+ response.add("transactions", transactionArray);
+
+ return response;
+ }
private JsonObject createProjectJson(int id, String name) {
JsonObject fields = new JsonObject();
