DANS/7564 messaging from workflows #7635

qqmyers · 2021-02-24T21:54:46Z

What this PR does / why we need it: Defines a mechanism for workflows to send a messages (strings) - one that will be logged and one to be shown to the user upon successful/unsuccessful completion. The user message is added as a new enum type of UserNotification (resulting in an email and addition on the notification tab), and is show as a green/yellow(warning - for Failures) banner if the user remains on/returns to the associated dataset page. The latter mirrors the functionality for the overall publication process. The new mechanism is required because workflows run asynchronously (and don't otherwise interact with the DatasetPage after being launched).

Expected initial uses include DANS' migration process and a potential refactoring of the invocation of spam processing prior to dataset publication.

Which issue(s) this PR closes:

Closes #7564

Special notes for your reviewer: aligned with the initial slack discussion I had with Gustavo about ways of providing a banner.

Suggestions on how to test this: The easiest way is to setup the new pause/message step and to then respond with a success/failure json object per the new workflow documentation on that step. The optional Reason and Message entries in the object should show in the log and as a banner/notification (respectively).

Does this PR introduce a user interface change? If mockups are available, please link/include them here: uses existing GUI elements for two new events (workflow success/failure), but doesn't change the display otherwise.

Is there a release notes update needed for this change?: could note the functionality, but it developer-level. The only config change required is the flyway script, so automatic.

Additional documentation:

landreev · 2021-03-10T02:26:44Z

src/main/java/edu/harvard/iq/dataverse/MailServiceBean.java

+            case WORKFLOW_FAILURE:
+                version =  (DatasetVersion) targetObject;
+                pattern = BundleUtil.getStringFromBundle("notification.email.workflow.failure");
+                String[] paramArrayWorkflowFailure = {version.getDataset().getDisplayName(), getDatasetLink(version.getDataset())};


Am I reading this correctly, that it's not populating the 3rd parameter of notification.email.workflow.failure pattern here?
So, should probably be
{version.getDataset().getDisplayName(), getDatasetLink(version.getDataset()), comment};
(it's probably more important to show the "reason" comment in the failure email, than in the success one...)

Good catch - not sure why I left it out - perhaps I just tested failure prior to adding the messaging part and didn't update it later. In any case, added now.

landreev · 2021-03-10T02:28:35Z

src/main/java/edu/harvard/iq/dataverse/workflow/step/Success.java

+    }
+
+    /**
+     * Holds the user-friendly message explaining the failure.


Not very "friendly", to claim it's a failure, even though it's a success! :)
(I know, it's just a copy-and-paste thing...)

"Sadly this workflow successfully completed as designed. Please contact support if you are disappointed because you were expecting something less reliable." :-) - fixed.

landreev · 2021-03-10T02:42:20Z

src/main/java/edu/harvard/iq/dataverse/workflow/WorkflowServiceBean.java

@@ -171,10 +178,20 @@ private void doResume(PendingWorkflowInvocation pending, String body) {
        final WorkflowContext ctxt = refresh(newCtxt,retrieveRequestedSettings( wf.getRequiredSettings()), getCurrentApiToken(newCtxt.getRequest().getAuthenticatedUser()));
        WorkflowStepResult res = pendingStep.resume(ctxt, pending.getLocalData(), body);
        if (res instanceof Failure) {
+            userNotificationService.sendNotification(ctxt.getRequest().getAuthenticatedUser(), Timestamp.from(Instant.now()), UserNotification.Type.WORKFLOW_FAILURE, ctxt.getDataset().getLatestVersion().getId(), ((Failure) res).getMessage());
+            //UserNotification isn't meant to be a long-term record and doesn't store the comment, so we'll also keep it as a workflow comment


Do I understand this correctly, that these notifications for workflow success and failure are email-only; in that you are purposefully excluding them from being shown on the dataverseuser.xhtml page? In part, I'm assuming, because of the above, that the custom comment/message doesn't get stored in the database. And with the assumption that the message will be shown to the user using the workflow UI messaging...

... having read the PR description one more time though, you are saying

a new enum type of UserNotification (resulting in an email and addition on the notification tab)

so, is it "purposefully", or not?

The notification should be standard and show up in both places. The comment is just noting that UserNotification doesn't include a comment field, so the comment goes out in the email but isn't saved as part of the UserNotification . So, when I need to show something on the page which includes the comment, I needed a place to store the comment.
An alternate design would be to add a comment field to UserNotification but that was bigger, not necessarily something we'd want for other notifications, and the WorkflowComment class seemed to be ~intended for this.

Hmm. I may be being slow here, but I just didn't see any code being added to dataverseuser.xhtml, that would show these new notifications in that tab. So my first assumption was that it was on purpose. Although, on a 2nd or 3rd thought, that could be confusing - if the little red alert in the account header is showing that there's one new notification, but nothing new is showing in the notifications tab - ?

Not intentional - I missed it, so the count goes up and a line in the table shows the date (done for all notifications), but there's not dataverseuser.xhtml fragment to provide the info specific to these types. I'll add them.

OK, makes sense. I obviously didn't remember how that tab was working either; my recollection was that every table entry had to be hard-coded in full.

landreev

I really like this solution for reporting the results of tasks that are executed asynchronously.
I mostly figured out how the actual messaging/notifications work. But please see my individual comments.

The following is not important for the purposes of approving and merging the PR, but something I've been thinking about:

Should/could this WorkflowComment mechanism be used for informing the user about the status of other things that happen during asynchronous publication, that are not part of a specific workflow? Specifically, the global id registration. We didn't have any way of communicating to the user about datacite failures during asynchronous publishing; so we solved it by adding a special UserNotification for such failures. But adding a WorkflowComment as well would be an easy way to get a more prominent message right on the dataset page - ?

... and is shown as a green/yellow(warning - for Failures) banner if the user remains on/returns to the associated dataset page. The latter mirrors the functionality for the overall publication process.

There is actually no overall "success" message with async publishing; there's a UserNotification (and there's the fact that the dataset is no longer locked, and the version is no longer draft), but there's no explicit "success! ..." banner. And as of 5.0, publishing is always asynchronous.

In fact Mike just opened an issue, asking for a "publish success" banner (#7653). Again, is another WorkflowComment an obvious solution here?

landreev · 2021-03-10T04:03:36Z

(I'm assuming the 3 conflicts are being reported because #7568 has been merged - but these need to be resolved)

qqmyers · 2021-03-12T18:07:28Z

@landreev - Thanks for the thorough review - yeah, aside from WorkflowComment potentially implying a Workflow, I was also thinking it could be a general mechanism for async processing (and #7653 in particular) (and for external tools/api actions?). Minimally I was hoping this would cover doing the spam detection as a workflow, but broader use for anything async should work - maybe a future refactor to rename the class? Or adding the comment field and has been shown flag to UserNotification at some point?

DANS/7564_messaging_from_workflows Conflicts: doc/sphinx-guides/source/developers/workflows.rst src/main/java/edu/harvard/iq/dataverse/UserNotification.java src/main/java/edu/harvard/iq/dataverse/workflow/internalspi/AuthorizedExternalStep.java src/main/java/edu/harvard/iq/dataverse/workflow/step/Success.java

landreev · 2021-03-15T13:44:01Z

The branch is looking good now.
I would support renaming that "WorkflowComment" to something more generic. (When I was thinking of solutions for reporting publication errors, I was considering creating a "CommandStatus" object - but we don't want it to be command-specific either).
Just to confirm, would you prefer to handle that renaming separately, in the future? - I'll approve and move the PR into QA then.

qqmyers · 2021-03-15T13:51:25Z

~yeah - a name change is easy, but I guess bundling that with the first non-workflow use would make more sense.

landreev

Moving the PR into QA.

kcondon · 2021-03-19T18:08:49Z

This works but did find a few issues for review:

Need an example of pause response body, and pause response command line.
Pause response body requires upper case for key names, other json in workflows provide lower case examples. What should happen here? I didn't realize it was case sensitive and composed a file using lower case, didn't work.
No indication of failure on command line when can't parse json response status (I think it is known we don't pass along json parsing errors in other api endpoints) but does log something in server.log
Publish lock banner does not appear after clicking publish, need page refresh.
Email notification when json parsing error has null in it.


Hello,
A workflow running on test pause workflow message (view at https://dataverse-internal.iq.harvard.edu/dataset.xhtml?persistentId=doi:10.70122/FK2/8WTCFE) failed: null
You may contact us for support at support@dataverse.harvard.edu.
Thank you,
Harvard Dataverse Support

a couple typos

[2021-03-18T19:36:46.224+0000] [Payara 5.2020.6] [WARNING] [] [edu.harvard.iq.dataverse.workflow.internalspi.PauseWithMessageStep] [tid: _ThreadID=287 _ThreadName=__ejb-thread-pool14] [timeMillis: 1616096206224] [levelValue: 900] [[
 Remote system returned a bad reposonse: ]]

typo in error msg above (should be response rather than reposonse)

one in guides: The latter provides a means for the asynchronous workflow execution to report success or failure anologous to the way the publication and other processes report on the page. (should be analogous rather. than anologous)

succcess on draft of published shows success banner and draft not found banner (url still draft, known issue?)
a success response on pause response command line is just the endpoint? /api/datasets/80702

qqmyers · 2021-03-22T16:37:37Z

Just committed fixes for the issues above (including both '5's :-) ) except:
3/7 - the /api/workflows/invocation_id endpoint calls an async method so all we can do is acknowledge receipt of the message (202 Accepted). If the invocation_id can't be found, or the caller is not in the whitelist, error messages are sent. A larger redesign could look at whether the methods to start/resume workflows themselves should be @async or if they should do some basic processing to validate inputs, etc. beofre calling an @async submethod.
4/6) I looked at the code and don't see that this code does anything different that when there's no workflow involved (so this is just another manifestation of #7653 due to asynchronous processing to complete the publication.) Hopefully any fix there will fix these too, if not, these could get added as issues after that one is resolved.

kcondon · 2021-03-23T14:45:21Z

Have not been able to get null in email when failure due to parsing error to work. Passing back for now.

qqmyers added 4 commits February 24, 2021 16:13

pulling out messaging only for IQSS#7564

35f5eab

update pause/message step to accept the same json as http/extauth

26e7ecc

update docs

f50b1d7

flyway sript

6f77d98

scolapasta assigned landreev Mar 5, 2021

qqmyers added the GDCC: DANS related to GDCC work for DANS label Mar 6, 2021

landreev self-requested a review March 8, 2021 05:25

landreev reviewed Mar 10, 2021

View reviewed changes

landreev removed their assignment Mar 10, 2021

djbrooke assigned qqmyers Mar 11, 2021

qqmyers added 2 commits March 12, 2021 12:47

send message with failure - per review catch

eecd60a

Don't imply success is a problem :-)

a68a777

qqmyers added 2 commits March 12, 2021 13:15

Add messages to notification panel per review

54c5c15

landreev approved these changes Mar 15, 2021

View reviewed changes

djbrooke unassigned qqmyers Mar 15, 2021

kcondon self-assigned this Mar 18, 2021

correct flyway number

b6b2298

handle lower case keys, consolidate common code, fix typos.

4da0064

qqmyers added 4 commits March 22, 2021 12:05

add example in docs

1a0ad95

lower case for message types

4c03de3

handle null message

b495342

typo

626d1ce

add reason logging, better log msgs

746955f

kcondon assigned qqmyers and unassigned kcondon Mar 23, 2021

fix null when sent in Workflow Failure

99d47bc

kcondon merged commit dd36c7e into IQSS:develop Mar 23, 2021

kcondon assigned kcondon and unassigned qqmyers Mar 23, 2021

djbrooke added this to the 5.4 milestone Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DANS/7564 messaging from workflows #7635

DANS/7564 messaging from workflows #7635

qqmyers commented Feb 24, 2021

landreev Mar 10, 2021

qqmyers Mar 12, 2021

landreev Mar 10, 2021

qqmyers Mar 12, 2021

landreev Mar 10, 2021

landreev Mar 10, 2021

qqmyers Mar 12, 2021

landreev Mar 12, 2021

qqmyers Mar 12, 2021

landreev Mar 12, 2021

landreev left a comment

landreev commented Mar 10, 2021

qqmyers commented Mar 12, 2021

landreev commented Mar 15, 2021

qqmyers commented Mar 15, 2021

landreev left a comment

kcondon commented Mar 19, 2021 •

edited

Loading

qqmyers commented Mar 22, 2021

kcondon commented Mar 23, 2021

DANS/7564 messaging from workflows #7635

DANS/7564 messaging from workflows #7635

Conversation

qqmyers commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landreev left a comment

Choose a reason for hiding this comment

landreev commented Mar 10, 2021

qqmyers commented Mar 12, 2021

landreev commented Mar 15, 2021

qqmyers commented Mar 15, 2021

landreev left a comment

Choose a reason for hiding this comment

kcondon commented Mar 19, 2021 • edited Loading

qqmyers commented Mar 22, 2021

kcondon commented Mar 23, 2021

kcondon commented Mar 19, 2021 •

edited

Loading