Skip to content

Commit cf20973

Browse files
stevenwinshipstevenwinship
committed
addressing comments
1 parent 58328ec commit cf20973

File tree

2 files changed

+51
-43
lines changed

2 files changed

+51
-43
lines changed

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

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@
3232
* @author skraffmi
3333
*/
3434
@RequiredPermissionsMap({
35-
@RequiredPermissions(dataverseName = "moved", value = {Permission.PublishDataset})
36-
, @RequiredPermissions(dataverseName = "destination", value = {Permission.AddDataset, Permission.PublishDataset})
35+
@RequiredPermissions(dataverseName = "moved", value = {Permission.PublishDataset})
36+
, @RequiredPermissions(dataverseName = "destination", value = {Permission.AddDataset, Permission.PublishDataset})
3737
})
3838
public class MoveDatasetCommand extends AbstractVoidCommand {
3939

@@ -42,10 +42,11 @@ public class MoveDatasetCommand extends AbstractVoidCommand {
4242
final Dataset moved;
4343
final Dataverse destination;
4444
final Boolean force;
45-
private boolean allowSelfNotification = false;
45+
final Boolean allowSelfNotification;
46+
final Dataverse originalOwner;
4647

4748
public MoveDatasetCommand(DataverseRequest aRequest, Dataset moved, Dataverse destination, Boolean force) {
48-
this( aRequest, moved, destination, force, Boolean.FALSE);
49+
this( aRequest, moved, destination, force, null);
4950
}
5051
public MoveDatasetCommand(DataverseRequest aRequest, Dataset moved, Dataverse destination, Boolean force, Boolean allowSelfNotification) {
5152
super(
@@ -57,6 +58,7 @@ public MoveDatasetCommand(DataverseRequest aRequest, Dataset moved, Dataverse de
5758
this.destination = destination;
5859
this.force= force;
5960
this.allowSelfNotification = allowSelfNotification;
61+
this.originalOwner = moved.getOwner();
6062
}
6163

6264
@Override
@@ -75,13 +77,13 @@ public void executeImpl(CommandContext ctxt) throws CommandException {
7577
if (moved.getOwner().equals(destination)) {
7678
throw new IllegalCommandException(BundleUtil.getStringFromBundle("dashboard.move.dataset.command.error.targetDataverseSameAsOriginalDataverse"), this);
7779
}
78-
80+
7981
// if dataset is published make sure that its target is published
80-
82+
8183
if (moved.isReleased() && !destination.isReleased()){
8284
throw new IllegalCommandException(BundleUtil.getStringFromBundle("dashboard.move.dataset.command.error.targetDataverseUnpublishedDatasetPublished", Arrays.asList(destination.getDisplayName())), this);
8385
}
84-
86+
8587
//if the datasets guestbook is not contained in the new dataverse then remove it
8688
if (moved.getGuestbook() != null) {
8789
Guestbook gb = moved.getGuestbook();
@@ -100,15 +102,15 @@ public void executeImpl(CommandContext ctxt) throws CommandException {
100102
}
101103
}
102104
}
103-
105+
104106
// generate list of all possible parent dataverses to check against
105107
List<Dataverse> ownersToCheck = new ArrayList<>();
106108
ownersToCheck.add(destination);
107109
if (destination.getOwners() != null) {
108110
ownersToCheck.addAll(destination.getOwners());
109111
}
110-
111-
// if the dataset is linked to the new dataverse or any of
112+
113+
// if the dataset is linked to the new dataverse or any of
112114
// its parent dataverses then remove the link
113115
List<DatasetLinkingDataverse> linkingDatasets = new ArrayList<>();
114116
if (moved.getDatasetLinkingDataverses() != null) {
@@ -127,7 +129,7 @@ public void executeImpl(CommandContext ctxt) throws CommandException {
127129
}
128130
}
129131
}
130-
132+
131133
if (removeGuestbook || removeLinkDs) {
132134
StringBuilder errorString = new StringBuilder();
133135
if (removeGuestbook) {
@@ -138,25 +140,30 @@ public void executeImpl(CommandContext ctxt) throws CommandException {
138140
}
139141
throw new UnforcedCommandException(errorString.toString(), this);
140142
}
141-
143+
142144
// 6575 if dataset is submitted for review and the default contributor
143145
// role includes dataset publish then remove the lock
144-
146+
145147
if (moved.isLockedFor(DatasetLock.Reason.InReview)
146148
&& destination.getDefaultContributorRole().permissions().contains(Permission.PublishDataset)) {
147149
ctxt.datasets().removeDatasetLocks(moved, DatasetLock.Reason.InReview);
148150
}
149151

150152
// OK, move
151-
Dataverse originalOwner = moved.getOwner();
152153
moved.setOwner(destination);
153154
ctxt.em().merge(moved);
154-
sendNotification(moved, originalOwner, ctxt);
155155

156156
boolean doNormalSolrDocCleanUp = true;
157157
ctxt.index().asyncIndexDataset(moved, doNormalSolrDocCleanUp);
158158

159159
}
160+
161+
@Override
162+
public boolean onSuccess(CommandContext ctxt, Object r) {
163+
sendNotification(moved, originalOwner, ctxt);
164+
return true;
165+
}
166+
160167
/**
161168
* Sends notifications to those able to publish the dataset upon the successful move of a dataset.
162169
* <p>
@@ -181,14 +188,17 @@ protected void sendNotification(Dataset dataset, Dataverse originalOwner, Comman
181188

182189
// 3. Get all users with publish permission on the dataset's original owner (dataverse) and notify them.
183190
Map<String, AuthenticatedUser> recipients = ctxt.permissions().getDistinctUsersWithPermissionOn(Permission.PublishDataset, originalOwner);
184-
// make sure the requestor is in the recipient list in case they don't match the permission
191+
// make sure the requestor is in the recipient list in case they don't match the permission but only if allowSelfNotification is true
185192
if (requestor != null) {
186-
recipients.put(requestor.getIdentifier(), requestor);
193+
if (Boolean.TRUE.equals(allowSelfNotification)) {
194+
recipients.put(requestor.getIdentifier(), requestor);
195+
} else {
196+
recipients.remove(requestor.getIdentifier());
197+
}
187198
}
188199

189200
recipients.values()
190201
.stream()
191-
.filter(recipient -> allowSelfNotification || !recipient.equals(requestor))
192202
.forEach(recipient -> ctxt.notifications().sendNotification(
193203
recipient,
194204
Timestamp.from(Instant.now()),
@@ -200,3 +210,4 @@ protected void sendNotification(Dataset dataset, Dataverse originalOwner, Comman
200210
));
201211
}
202212
}
213+

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

Lines changed: 22 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import static org.hamcrest.CoreMatchers.*;
2525
import static org.junit.jupiter.api.Assertions.assertEquals;
26+
import static org.junit.jupiter.api.Assertions.assertTrue;
2627

2728
import org.junit.jupiter.api.AfterAll;
2829
import org.junit.jupiter.api.BeforeAll;
@@ -68,7 +69,7 @@ public void testMoveDataset() {
6869
Response noPermToCreateDataset = UtilIT.createRandomDatasetViaNativeApi(curatorDataverseAlias1, authorApiToken);
6970
noPermToCreateDataset.prettyPrint();
7071
noPermToCreateDataset.then().assertThat().statusCode(UNAUTHORIZED.getStatusCode());
71-
72+
7273
Response grantAuthorAddDataset = UtilIT.grantRoleOnDataverse(curatorDataverseAlias1, DataverseRole.DS_CONTRIBUTOR.toString(), "@" + authorUsername, curatorApiToken);
7374
grantAuthorAddDataset.prettyPrint();
7475
grantAuthorAddDataset.then().assertThat()
@@ -195,10 +196,6 @@ public void testMoveDatasetNotification() {
195196
.statusCode(CREATED.getStatusCode());
196197
Integer datasetId = UtilIT.getDatasetIdFromResponse(createDataset);
197198

198-
// clear existing notifications (so the DATASETMOVED notification will be the only one)
199-
clearNotifications(user1ApiToken);
200-
clearNotifications(user2ApiToken);
201-
202199
// User1(superuser) moves the dataset from dataverse2 to dataverse1
203200
Response moveDataset = UtilIT.moveDataset(datasetId.toString(), dataverseAlias1, user1ApiToken);
204201
moveDataset.prettyPrint();
@@ -209,30 +206,30 @@ public void testMoveDatasetNotification() {
209206
// verify that a notification was sent to user1
210207
Response getNotifications = UtilIT.getNotifications(user1ApiToken);
211208
getNotifications.prettyPrint();
212-
getNotifications.then().assertThat()
213-
.statusCode(OK.getStatusCode())
214-
.body("data.notifications[0].type", equalTo("DATASETMOVED"))
215-
.body("data.notifications[0].displayAsRead", equalTo(false))
216-
.body("data.notifications[0].subjectText", containsString("has been moved"))
217-
.body("data.notifications[0].messageText", startsWith(BundleUtil.getStringFromBundle("notification.email.greeting")));
209+
verifyNotification(getNotifications, dataverseAlias1);
210+
218211
// verify that a notification was sent to user2
219212
getNotifications = UtilIT.getNotifications(user2ApiToken);
220213
getNotifications.prettyPrint();
221-
getNotifications.then().assertThat()
222-
.statusCode(OK.getStatusCode())
223-
.body("data.notifications[0].type", equalTo("DATASETMOVED"))
224-
.body("data.notifications[0].displayAsRead", equalTo(false))
225-
.body("data.notifications[0].subjectText", containsString("has been moved"))
226-
.body("data.notifications[0].messageText", startsWith(BundleUtil.getStringFromBundle("notification.email.greeting")));
214+
verifyNotification(getNotifications, dataverseAlias1);
227215
}
228216

229-
private void clearNotifications(String apiToken) {
230-
Response getNotifications = UtilIT.getNotifications(apiToken);
231-
List<Object> notifications = JsonPath.from(getNotifications.body().asString()).getList("data.notifications");
232-
for (Object obj : notifications) {
233-
Object id = ((Map) obj).get("id");
234-
UtilIT.deleteNotification(Long.parseLong(id.toString()), apiToken).prettyPrint();
217+
private void verifyNotification(Response notificationListResponse, String dataverseAlias) {
218+
notificationListResponse.then().assertThat()
219+
.statusCode(OK.getStatusCode());
220+
boolean found = false;
221+
List<Map<String, String>> notifications = notificationListResponse.body().jsonPath().getList("data.notifications");
222+
223+
for (Map<String, String> notification : notifications) {
224+
if ("DATASETMOVED".equalsIgnoreCase(notification.get("type"))) {
225+
if (notification.get("messageText") != null && notification.get("messageText").contains(dataverseAlias)) {
226+
found = true;
227+
assertTrue(notification.get("subjectText") != null && notification.get("subjectText").contains("has been moved"));
228+
assertTrue(notification.get("messageText") != null && notification.get("messageText").startsWith(BundleUtil.getStringFromBundle("notification.email.greeting")));
229+
}
230+
}
235231
}
232+
assertTrue(found);
236233
}
237234

238235
@Test
@@ -400,12 +397,12 @@ public void testMoveLinkedDataset() {
400397
assertEquals("OK", linksAfterData.getString("status"));
401398
assertEquals(0, linksAfterData.getJsonObject("data").getJsonArray("linked-dataverses").size());
402399
}
403-
400+
404401
@Test
405402
public void testMoveDatasetsPerms() {
406403

407404
/*
408-
Verify that permissions set on a dataset remain
405+
Verify that permissions set on a dataset remain
409406
after that dataaset is moved
410407
*/
411408
Response createCurator = UtilIT.createRandomUser();

0 commit comments

Comments
 (0)