addressing comments

Author: stevenwinship

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/MoveDatasetCommand.java

@@ -42,10 +42,11 @@ public class MoveDatasetCommand extends AbstractVoidCommand {
     final Dataset moved;
     final Dataverse destination;
     final Boolean force;
-    private boolean allowSelfNotification = false;
+    final Boolean allowSelfNotification;
+    final Dataverse originalOwner;
 
     public MoveDatasetCommand(DataverseRequest aRequest, Dataset moved, Dataverse destination, Boolean force) {
-        this( aRequest, moved, destination, force, Boolean.FALSE);
+        this( aRequest, moved, destination, force, null);
     }
     public MoveDatasetCommand(DataverseRequest aRequest, Dataset moved, Dataverse destination, Boolean force, Boolean allowSelfNotification) {
         super(
@@ -57,6 +58,7 @@ public MoveDatasetCommand(DataverseRequest aRequest, Dataset moved, Dataverse de
         this.destination = destination;
         this.force= force;
         this.allowSelfNotification = allowSelfNotification;
+        this.originalOwner = moved.getOwner();
     }
 
     @Override
@@ -148,15 +150,20 @@ public void executeImpl(CommandContext ctxt) throws CommandException {
         }
 
         // OK, move
-        Dataverse originalOwner = moved.getOwner();
         moved.setOwner(destination);
         ctxt.em().merge(moved);
-        sendNotification(moved, originalOwner, ctxt);
 
         boolean doNormalSolrDocCleanUp = true;
         ctxt.index().asyncIndexDataset(moved, doNormalSolrDocCleanUp);
 
     }
+
+    @Override
+    public boolean onSuccess(CommandContext ctxt, Object r) {
+        sendNotification(moved, originalOwner, ctxt);
+        return true;
+    }
+
     /**
      * Sends notifications to those able to publish the dataset upon the successful move of a dataset.
      * <p>
@@ -181,14 +188,17 @@ protected void sendNotification(Dataset dataset, Dataverse originalOwner, Comman
 
         // 3. Get all users with publish permission on the dataset's original owner (dataverse) and notify them.
         Map<String, AuthenticatedUser> recipients = ctxt.permissions().getDistinctUsersWithPermissionOn(Permission.PublishDataset, originalOwner);
-        // make sure the requestor is in the recipient list in case they don't match the permission
+        // make sure the requestor is in the recipient list in case they don't match the permission but only if allowSelfNotification is true
         if (requestor != null) {
-            recipients.put(requestor.getIdentifier(), requestor);
+            if (Boolean.TRUE.equals(allowSelfNotification)) {
+                recipients.put(requestor.getIdentifier(), requestor);
+            } else {
+                recipients.remove(requestor.getIdentifier());
+            }
         }
 
         recipients.values()
                 .stream()
-                .filter(recipient -> allowSelfNotification || !recipient.equals(requestor))
                 .forEach(recipient -> ctxt.notifications().sendNotification(
                         recipient,
                         Timestamp.from(Instant.now()),

src/test/java/edu/harvard/iq/dataverse/api/MoveIT.java

@@ -23,6 +23,7 @@
 
 import static org.hamcrest.CoreMatchers.*;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
@@ -195,10 +196,6 @@ public void testMoveDatasetNotification() {
                 .statusCode(CREATED.getStatusCode());
         Integer datasetId = UtilIT.getDatasetIdFromResponse(createDataset);
 
-        // clear existing notifications (so the DATASETMOVED notification will be the only one)
-        clearNotifications(user1ApiToken);
-        clearNotifications(user2ApiToken);
-
         // User1(superuser) moves the dataset from dataverse2 to dataverse1
         Response moveDataset = UtilIT.moveDataset(datasetId.toString(), dataverseAlias1, user1ApiToken);
         moveDataset.prettyPrint();
@@ -209,30 +206,30 @@ public void testMoveDatasetNotification() {
         // verify that a notification was sent to user1
         Response getNotifications = UtilIT.getNotifications(user1ApiToken);
         getNotifications.prettyPrint();
-        getNotifications.then().assertThat()
-                .statusCode(OK.getStatusCode())
-                .body("data.notifications[0].type", equalTo("DATASETMOVED"))
-                .body("data.notifications[0].displayAsRead", equalTo(false))
-                .body("data.notifications[0].subjectText", containsString("has been moved"))
-                .body("data.notifications[0].messageText", startsWith(BundleUtil.getStringFromBundle("notification.email.greeting")));
+        verifyNotification(getNotifications, dataverseAlias1);
+
         // verify that a notification was sent to user2
         getNotifications = UtilIT.getNotifications(user2ApiToken);
         getNotifications.prettyPrint();
-        getNotifications.then().assertThat()
-                .statusCode(OK.getStatusCode())
-                .body("data.notifications[0].type", equalTo("DATASETMOVED"))
-                .body("data.notifications[0].displayAsRead", equalTo(false))
-                .body("data.notifications[0].subjectText", containsString("has been moved"))
-                .body("data.notifications[0].messageText", startsWith(BundleUtil.getStringFromBundle("notification.email.greeting")));
+        verifyNotification(getNotifications, dataverseAlias1);
     }
 
-    private void clearNotifications(String apiToken) {
-        Response getNotifications = UtilIT.getNotifications(apiToken);
-        List<Object> notifications = JsonPath.from(getNotifications.body().asString()).getList("data.notifications");
-        for (Object obj : notifications) {
-            Object id = ((Map) obj).get("id");
-            UtilIT.deleteNotification(Long.parseLong(id.toString()), apiToken).prettyPrint();
+    private void verifyNotification(Response notificationListResponse, String dataverseAlias) {
+        notificationListResponse.then().assertThat()
+                .statusCode(OK.getStatusCode());
+        boolean found = false;
+        List<Map<String, String>> notifications = notificationListResponse.body().jsonPath().getList("data.notifications");
+
+        for (Map<String, String> notification : notifications) {
+            if ("DATASETMOVED".equalsIgnoreCase(notification.get("type"))) {
+                if (notification.get("messageText") != null && notification.get("messageText").contains(dataverseAlias)) {
+                    found = true;
+                    assertTrue(notification.get("subjectText") != null && notification.get("subjectText").contains("has been moved"));
+                    assertTrue(notification.get("messageText") != null && notification.get("messageText").startsWith(BundleUtil.getStringFromBundle("notification.email.greeting")));
+                }
+            }
         }
+        assertTrue(found);
     }
 
     @Test