send emails from onSuccess

[qqmyers]

What this PR does / why we need it: Makes sure the email notification sent when publication of a dataset succeeds include changes from the newly published version including any change to its title.

Which issue(s) this PR closes:

Closes #7467

Special notes for your reviewer: This is related to #7467, but I'm not sure if it should close the issue since there could be similar problems for other messages (Dataverse publication?)

Suggestions on how to test this: Publish a dataset, edit the title, and publish again. Check the email and verify it has your updated title.

Does this PR introduce a user interface change? If mockups are available, please link/include them here: no

Is there a release notes update needed for this change?: no

Additional documentation:

[coveralls]

Coverage decreased (-0.0005%) to 19.481% when pulling 8852c6d on QualitativeDataRepository:IQSS/7467 into b2286f6 on IQSS:develop.

[pdurbin]

This fix looks straightforward enough. I didn't test it but the sending of the notification was moved to onSuccess. This should only affect the publishing of datasets.

I'm approving this and moving it to QA but I'm going to tag @djbrooke for any housekeeping of #7467 which is pull request references but does not close. Perhaps we should have an issue with narrower scope that this pull request does close? It seems to be fixing a real bug.

[pdurbin]

Whoops, this branch is still at 5.2. I'll attempt to merge develop into it before putting it in QA.

Update: I merged develop in 8852c6d. I'll wait until I see the results from Jenkins before moving this to QA.

[djbrooke]

Thanks @pdurbin for the ping. I'd be OK closing #7467 for this and I'll update the description above to reflect.

I think the last part of #7467 that Jim added is important to live on as a new, separate issue:

"It might still be worth thinking about moving success messaging into onSuccess generally to avoid any possibilities should the message contents change in the future."

@scolapasta any thoughts?

[scolapasta]

@djbrooke agreed, this is the kind of thing onSuccess was designed for. One thing we'll need to consider, though, is API calls. I think (but am not certain) that many of these success messages are UI only, because we were worried about creating hundred or thousands of notifications when running through the API (and it was an open question on whether we should or should not). Moving to the onSuccess would have them always generated - if we don't want this, we may also need a mechanism to determine in the Command if it was called via API.

[pdurbin]

@scolapasta good point about the API and notifications. Here's a related issue:

• notification/email inconsistencies when creating a dataset via API notification/email inconsistencies when creating a dataset via API #4043

[djbrooke]

@scolapasta thanks -- sounds good. Does the concern about API calls generating a bunch of notifications come into play on this PR?

[pdurbin]

The results are in from https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-7468/8/testReport/

All the tests passed and the API tests took the appropriate amount on time.

I'm moving this to QA.

@djbrooke thanks for the issue cleanup. In this case the notification is not specific to the UI. It's in a command that both the UI and API use. Also, publishing is a relatively rare event. I'm not sure when people get flooded with notifications but I bet Sonia could tell us. There might even be an open issue about this.

[scolapasta]

@pdurbin @djbrooke I could imagine if you create 1000 datasets and publish (all via API) you may not want 1000 notifications. (which may just be something you would do when testing or migrating, but that recent law dataverse comes to mind). Regardless in this case, @pdurbin is correct that is was already in the Command and just moved methods.

We may want to revisit the logic / come up with generalized rules, but there's no reason not to move this one currently forward.