From 2107d8847b0ffbac201b735108b7e9dd40544339 Mon Sep 17 00:00:00 2001 From: Stephen Kraffmiller Date: Tue, 18 Jul 2017 16:14:36 -0400 Subject: [PATCH] #3943 - Remove reasons for return from get Notifications API Also, completely remove Workflow Action. --- .../harvard/iq/dataverse/api/Datasets.java | 2 - .../iq/dataverse/api/Notifications.java | 3 + .../impl/ReturnDatasetToAuthorCommand.java | 4 +- .../dataverse/workflows/WorkflowAction.java | 90 ------------------- .../dataverse/workflows/WorkflowComment.java | 21 +++-- .../iq/dataverse/api/InReviewWorkflowIT.java | 45 +++++----- 6 files changed, 41 insertions(+), 124 deletions(-) delete mode 100644 src/main/java/edu/harvard/iq/dataverse/workflows/WorkflowAction.java diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java b/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java index 7a20dbfcce2..7eebd4899b0 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java @@ -62,8 +62,6 @@ import edu.harvard.iq.dataverse.util.SystemConfig; import edu.harvard.iq.dataverse.util.json.JsonParseException; import static edu.harvard.iq.dataverse.util.json.JsonPrinter.*; -import edu.harvard.iq.dataverse.workflows.WorkflowAction; -import edu.harvard.iq.dataverse.workflows.WorkflowComment; import java.io.InputStream; import java.io.StringReader; import java.util.Collections; diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Notifications.java b/src/main/java/edu/harvard/iq/dataverse/api/Notifications.java index 6361e7a81f5..4067e61a31c 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Notifications.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Notifications.java @@ -44,6 +44,8 @@ public Response getAllNotificationsForUser() { Type type = notification.getType(); notificationObjectBuilder.add("id", notification.getId()); notificationObjectBuilder.add("type", type.toString()); + /* FIXME - Re-add reasons for return if/when they are added to the notifications page. + if (Type.RETURNEDDS.equals(type) || Type.SUBMITTEDDS.equals(type)) { JsonArrayBuilder reasons = getReasonsForReturn(notification); for (JsonValue reason : reasons.build()) { @@ -51,6 +53,7 @@ public Response getAllNotificationsForUser() { } notificationObjectBuilder.add("reasonsForReturn", reasonsForReturn); } + */ jsonArrayBuilder.add(notificationObjectBuilder); } JsonObjectBuilder result = Json.createObjectBuilder().add("notifications", jsonArrayBuilder); diff --git a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/ReturnDatasetToAuthorCommand.java b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/ReturnDatasetToAuthorCommand.java index 3a620452131..b7b927f7b6d 100644 --- a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/ReturnDatasetToAuthorCommand.java +++ b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/ReturnDatasetToAuthorCommand.java @@ -12,8 +12,6 @@ import edu.harvard.iq.dataverse.engine.command.exception.CommandException; import edu.harvard.iq.dataverse.engine.command.exception.IllegalCommandException; import edu.harvard.iq.dataverse.util.BundleUtil; -import edu.harvard.iq.dataverse.workflows.WorkflowAction; -import static edu.harvard.iq.dataverse.workflows.WorkflowAction.Type.RETURN_TO_AUTHOR; import edu.harvard.iq.dataverse.workflows.WorkflowComment; import java.sql.Timestamp; import java.util.Date; @@ -62,7 +60,7 @@ public Dataset save(CommandContext ctxt) throws CommandException { DatasetVersionUser ddu = ctxt.datasets().getDatasetVersionUser(theDataset.getLatestVersion(), this.getUser()); - WorkflowComment workflowComment = new WorkflowComment(new WorkflowAction(theDataset.getEditVersion(), RETURN_TO_AUTHOR), comment, (AuthenticatedUser) this.getUser()); + WorkflowComment workflowComment = new WorkflowComment(theDataset.getEditVersion(), WorkflowComment.Type.RETURN_TO_AUTHOR, comment, (AuthenticatedUser) this.getUser()); ctxt.datasets().addWorkflowComment(workflowComment); if (ddu != null) { diff --git a/src/main/java/edu/harvard/iq/dataverse/workflows/WorkflowAction.java b/src/main/java/edu/harvard/iq/dataverse/workflows/WorkflowAction.java deleted file mode 100644 index 9a895dd761b..00000000000 --- a/src/main/java/edu/harvard/iq/dataverse/workflows/WorkflowAction.java +++ /dev/null @@ -1,90 +0,0 @@ -package edu.harvard.iq.dataverse.workflows; - -public class WorkflowAction { - - /** - * Each action belongs to a workflow, such as the "publication" or "request - * access" workflow. - * - * - The "publication" workflow, which might include the author clicking - * "Submit for Review" and the curator clicking "Return to Author" or - * "Publish". - * - * - The "request access" workflow, which might include users asking for - * access to files and curators either granting or denying access. - * Requesting a contributor role for a dataset is just an idea at this - * point, included to show how other entities might be in play in the - * future. - * - * It's certainly possible that other workflows may be considered in the - * future such as "invite collaborator" ( - * https://github.com/IQSS/dataverse/issues/1334 ) and other workflows we - * haven't even considered. - * - * Some workflows might look like a conversation or a tennis match with a - * half a dozen clicks of "Submit for Review" and "Return to Author" before - * the dataset version is ultimately published. - */ - public enum Type { - RETURN_TO_AUTHOR, - // These are potential future action types for existing functionality. - SUBMIT_FOR_REVIEW, - PUBLISH, - REQUEST_FILE_ACCESS, - GRANT_FILE_ACCESS, - DENY_FILE_ACCESS, - // This is functionality we don't even have yet. - REQUEST_DATASET_CONTRIBUTOR_ACCESS, - GRANT_DATASET_CONTRIBUTOR_ACCESS, - DATASET_CONTRIBUTOR_ACCESS - }; - - public enum WorkFlow { - PUBLICATION, - REQUEST_ACCESS, - }; - - private Object target; - private Type type; - private WorkFlow workFlow; - - public WorkflowAction(Object target, Type type) { - this.target = target; - this.type = type; - switch (this.type) { - case RETURN_TO_AUTHOR: - this.workFlow = WorkFlow.PUBLICATION; - break; - case SUBMIT_FOR_REVIEW: - this.workFlow = WorkFlow.PUBLICATION; - break; - case PUBLISH: - this.workFlow = WorkFlow.PUBLICATION; - break; - case REQUEST_FILE_ACCESS: - this.workFlow = WorkFlow.REQUEST_ACCESS; - break; - case GRANT_FILE_ACCESS: - this.workFlow = WorkFlow.REQUEST_ACCESS; - break; - case DENY_FILE_ACCESS: - this.workFlow = WorkFlow.REQUEST_ACCESS; - break; - default: - break; - } - } - - public Object getTarget() { - return target; - } - - public Type getType() { - return type; - } - - public WorkFlow getWorkFlow() { - return workFlow; - } - -} diff --git a/src/main/java/edu/harvard/iq/dataverse/workflows/WorkflowComment.java b/src/main/java/edu/harvard/iq/dataverse/workflows/WorkflowComment.java index 8eb1e608b84..b028e5b3915 100644 --- a/src/main/java/edu/harvard/iq/dataverse/workflows/WorkflowComment.java +++ b/src/main/java/edu/harvard/iq/dataverse/workflows/WorkflowComment.java @@ -17,6 +17,14 @@ @Entity public class WorkflowComment implements Serializable { + /* + This release only supports Return to Author as a comment type + More may be added in future releases, + */ + public enum Type { + RETURN_TO_AUTHOR //, SUBMIT_FOR_REVIEW not available in this release but may be added in the future + }; + @Id @GeneratedValue(strategy = GenerationType.IDENTITY) private Long id; @@ -40,7 +48,7 @@ public class WorkflowComment implements Serializable { */ @Column(nullable = false) @Enumerated(EnumType.STRING) - private WorkflowAction.Type type; + private Type type; // This is nullable because when you submit for review, especially the first time, you don't have to enter a message. @Column(nullable = true, columnDefinition = "TEXT") @@ -59,11 +67,10 @@ public class WorkflowComment implements Serializable { // TODO: How should we best associate these entries to notifications, which can go to multiple authors and curators? // @Transient // private List notifications; - public WorkflowComment(WorkflowAction workflowAction, String message, AuthenticatedUser authenticatedUser) { - this.type = workflowAction.getType(); - if (this.type.equals(WorkflowAction.Type.RETURN_TO_AUTHOR)) { - DatasetVersion datasetVersionFromAction = (DatasetVersion) workflowAction.getTarget(); - this.datasetVersion = datasetVersionFromAction; + public WorkflowComment(DatasetVersion version, WorkflowComment.Type type, String message, AuthenticatedUser authenticatedUser) { + this.type = type; + if (this.type.equals(WorkflowComment.Type.RETURN_TO_AUTHOR)) { + this.datasetVersion = version; } this.message = message; this.authenticatedUser = authenticatedUser; @@ -92,7 +99,7 @@ public DatasetVersion getDatasetVersion() { return datasetVersion; } - public WorkflowAction.Type getType() { + public Type getType() { return type; } diff --git a/src/test/java/edu/harvard/iq/dataverse/api/InReviewWorkflowIT.java b/src/test/java/edu/harvard/iq/dataverse/api/InReviewWorkflowIT.java index 53148de6cb4..22021ae78e9 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/InReviewWorkflowIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/InReviewWorkflowIT.java @@ -173,15 +173,16 @@ public void testCuratorSendsCommentsToAuthor() { returnToAuthorAlreadyReturned.then().assertThat() .body("message", equalTo("This dataset cannot be return to the author(s) because the latest version is not In Review. The author(s) needs to click Submit for Review first.")) .statusCode(FORBIDDEN.getStatusCode()); - + //FIXME when/if reasons for return are returned to notifications page and the API is + // updated appropriately, these tests will have to be updated. Response authorChecksForCommentsAgain = UtilIT.getNotifications(authorApiToken); authorChecksForCommentsAgain.prettyPrint(); authorChecksForCommentsAgain.then().assertThat() .body("data.notifications[0].type", equalTo("RETURNEDDS")) // The author thinks, "This why we have curators!" - .body("data.notifications[0].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + //.body("data.notifications[0].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) .body("data.notifications[1].type", equalTo("CREATEACC")) - .body("data.notifications[1].reasonsForReturn", equalTo(null)) + //.body("data.notifications[1].reasonsForReturn", equalTo(null)) .statusCode(OK.getStatusCode()); // The author upload the file she forgot. @@ -206,12 +207,12 @@ public void testCuratorSendsCommentsToAuthor() { curatorChecksNotifications.then().assertThat() // TODO: Test this issue from the UI as well: https://github.com/IQSS/dataverse/issues/2526 .body("data.notifications[0].type", equalTo("SUBMITTEDDS")) - .body("data.notifications[0].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + //.body("data.notifications[0].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) .body("data.notifications[1].type", equalTo("SUBMITTEDDS")) // Yes, it's a little weird that the first "SUBMITTEDDS" notification now shows the return reason when it showed nothing before. For now we are simply always showing all the reasons for return. They start to stack up. That way you can see the history. - .body("data.notifications[1].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + //.body("data.notifications[1].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) .body("data.notifications[2].type", equalTo("CREATEACC")) - .body("data.notifications[2].reasonsForReturn", equalTo(null)) + //.body("data.notifications[2].reasonsForReturn", equalTo(null)) .statusCode(OK.getStatusCode()); String reasonForReturn2 = "A README is required."; @@ -226,14 +227,14 @@ public void testCuratorSendsCommentsToAuthor() { authorChecksForComments3.prettyPrint(); authorChecksForComments3.then().assertThat() .body("data.notifications[0].type", equalTo("RETURNEDDS")) - .body("data.notifications[0].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + // .body("data.notifications[0].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) .body("data.notifications[0].reasonsForReturn[1].message", equalTo("A README is required.")) .body("data.notifications[1].type", equalTo("RETURNEDDS")) // Yes, it's a little weird that the reason for return on the first "RETURNEDDS" changed. We're showing the history. - .body("data.notifications[1].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) - .body("data.notifications[1].reasonsForReturn[1].message", equalTo("A README is required.")) + // .body("data.notifications[1].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + // .body("data.notifications[1].reasonsForReturn[1].message", equalTo("A README is required.")) .body("data.notifications[2].type", equalTo("CREATEACC")) - .body("data.notifications[2].reasonsForReturn", equalTo(null)) + // .body("data.notifications[2].reasonsForReturn", equalTo(null)) .statusCode(OK.getStatusCode()); String pathToReadme = "README.md"; @@ -257,17 +258,17 @@ public void testCuratorSendsCommentsToAuthor() { curatorHopesTheReadmeIsThereNow.then().assertThat() // TODO: Test this issue from the UI as well: https://github.com/IQSS/dataverse/issues/2526 .body("data.notifications[0].type", equalTo("SUBMITTEDDS")) - .body("data.notifications[0].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) - .body("data.notifications[0].reasonsForReturn[1].message", equalTo("A README is required.")) + // .body("data.notifications[0].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + // .body("data.notifications[0].reasonsForReturn[1].message", equalTo("A README is required.")) .body("data.notifications[1].type", equalTo("SUBMITTEDDS")) - .body("data.notifications[1].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) - .body("data.notifications[1].reasonsForReturn[1].message", equalTo("A README is required.")) + // .body("data.notifications[1].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + // .body("data.notifications[1].reasonsForReturn[1].message", equalTo("A README is required.")) .body("data.notifications[2].type", equalTo("SUBMITTEDDS")) // Yes, it's a little weird that the first "SUBMITTEDDS" notification now shows the return reason when it showed nothing before. We're showing the history. - .body("data.notifications[2].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) - .body("data.notifications[2].reasonsForReturn[1].message", equalTo("A README is required.")) + // .body("data.notifications[2].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + // .body("data.notifications[2].reasonsForReturn[1].message", equalTo("A README is required.")) .body("data.notifications[3].type", equalTo("CREATEACC")) - .body("data.notifications[3].reasonsForReturn", equalTo(null)) + // .body("data.notifications[3].reasonsForReturn", equalTo(null)) .statusCode(OK.getStatusCode()); // The curator publishes the dataverse. @@ -288,14 +289,14 @@ public void testCuratorSendsCommentsToAuthor() { // FIXME: Why is this ASSIGNROLE and not "your dataset has been published"? .body("data.notifications[0].type", equalTo("ASSIGNROLE")) .body("data.notifications[1].type", equalTo("RETURNEDDS")) - .body("data.notifications[1].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) - .body("data.notifications[1].reasonsForReturn[1].message", equalTo("A README is required.")) + // .body("data.notifications[1].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + // .body("data.notifications[1].reasonsForReturn[1].message", equalTo("A README is required.")) .body("data.notifications[2].type", equalTo("RETURNEDDS")) // Yes, it's a little weird that the reason for return on the first "RETURNEDDS" changed. For now we are always showing the most recent reason for return. - .body("data.notifications[2].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) + // .body("data.notifications[2].reasonsForReturn[0].message", equalTo("You forgot to upload any files.")) .body("data.notifications[2].reasonsForReturn[1].message", equalTo("A README is required.")) - .body("data.notifications[3].type", equalTo("CREATEACC")) - .body("data.notifications[3].reasonsForReturn", equalTo(null)) + // .body("data.notifications[3].type", equalTo("CREATEACC")) + // .body("data.notifications[3].reasonsForReturn", equalTo(null)) .statusCode(OK.getStatusCode()); // These println's are here in case you want to log into the GUI to see what notifications look like.