#7784 If name is not set use 'Dataverse administrator' as email sender

[pkiraly]

What this PR does / why we need it: If the :SystemEmail value does not contain a name, only an email address, and the user use contact form or the "Contact owner" form to send a mesage, the sender of the email will look like this:

null on behalf of name@example.com dataverse@example.edu

This change use the phrase "Dataverse administrator" instead of null if there is no name set.

Which issue(s) this PR closes: #7784

Closes #7784

Special notes for your reviewer: There is no integration test for this commit. I haven't found tests for sending emails. If you know one, please let me know and I will create a test for this. I also removed some empty lines and unwanted whitespaces from the file.

Suggestions on how to test this: I've created unit tests in src/test/java/edu/harvard/iq/dataverse/MailServiceBeanTest.java

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: No

[scolapasta]

Not a full review, just yet, but can we put this in the bundle? (also let's call it as "Dataverse Installation Admin")

[scolapasta]

Actually, maybe a better alternative, rather than in a bundle, what if it used the branding name of the installation (using BrandingUtil.getInstallationBrandName())? Thoughts @qqmyers @djbrooke

[djbrooke]

Dataverse Installation Administrator is better than "null" so I'd be happy to accept a PR with that change, but if @pkiraly would be willing to add it, the branding name would be a better option.

[qqmyers]

That makes sense. There are also a getSupportTeamName()/getSupportTeamEmail() methods that might help -the former uses the brand name.

[pkiraly]

I made another version which solves it in a three step process:

        String personal = fromAddress.getPersonal() != null
            ? fromAddress.getPersonal()
            : BrandingUtil.getInstallationBrandName() != null
                ? BrandingUtil.getInstallationBrandName()
                : BundleUtil.getStringFromBundle("contact.delegation.default_personal");

In order to test it with unit tests I had to add some extra null check into getInstallationBrandName() method.

[qqmyers]

Sorry - I think there are two options. BrandingUtil requires some setup to work in tests (see the BrandingUtilTest class).

1. Add a line like

   dataverse/src/test/java/edu/harvard/iq/dataverse/branding/BrandingUtilTest.java

   Line 57 in 473cde6

   Mockito.when(dataverseSvc.getRootDataverseName()).thenReturn("LibraScholar");

   after line 62 resetting the return value to null, and probably to the MailUtilTest as well, and/or
2. Add

   dataverse/src/test/java/edu/harvard/iq/dataverse/branding/BrandingUtilTest.java

   Lines 35 to 39 in 473cde6

   	Mockito.when(settingsSvc.getValueForKey(SettingsServiceBean.Key.InstallationName)).thenReturn(null);
   	//And configure the mock DataverseService to pretend the root collection name is as shown
   	Mockito.when(dataverseSvc.getRootDataverseName()).thenReturn("LibraScholar");
   	BrandingUtil.injectServices(dataverseSvc, settingsSvc);

   to your test to configure BrandingUtil with the values you want.

(Nominally, "LibraData" was the value that was returned prior to the recent updates to Dataverse to have BrandingUtil use services. With only BrandingUtilTest and MailUtilTest using BrandingUtil, I didn't worry about resetting for other tests. I guess that (1) would be cleaner, but doing (2) in your own tests should always work as well.)

[pkiraly]

@qqmyers I tried both. (1) did not work at all, but (2) did with a modification. Mockito.when throws an UnnecessaryStubbingException. After some searching I found https://www.baeldung.com/mockito-unnecessary-stubbing-exception, which suggested to use Mockito.lenient().when and it worked. Now tests cover 4 cases:

• the :SystemEmail contains a name part
• if not, there is an :InstallationName
• if not, the root Dataverse has a name
• if not, there is a bundle property called contact.delegation.default_personal

[qqmyers]

@pkiraly - glad you got it to work. Thinking back, that stubbing exception may have been why I didn't do (1) myself. I can see why in (2), where you have the setup in a private method you could get the same exception - seems like Mockito is just checking whether the mock is used later in the same method - so I'm glad lenient works there. (Did you try lenient in (1)? Just curious - not suggesting that you need to modify the PR).

[pkiraly]

@qqmyers No, with (1) there were no effect at all, my impression was, that something did not happened at all. One more point: I wanted that these tests should work both alone and together with all the other tests. I guess when it was running alone it did not triggered BrandingUtilTest tests.

[qqmyers]

Hmm - I just tried (1) and get the unnecessary stubbing error - when the BrandingUtilTest runs. Using lenient() there gets rid of the warning, but I didn't test whether that would have any effect on other test classes (perhaps lenient() just eats the warning and doesn't actually reset the mock at all - which would be consistent if you see no effect of (1)).

In any case, I think what you have now works when testing one class or all of them and that's what needed.

[scolapasta]

One request; once resolved, I can approve this.

[scolapasta]

Are these changes still needed now that you put initBrandingUtilWithRootDataverse() in each test? If not, please revert; if so, could we remove from her and do the necessary set up in the tests (for cleaner code here)?

[pkiraly]

@scolapasta You are right, I restored the original version (plus some small code formatting).

[kcondon]

@pkiraly Would you mind refreshing this branch from develop? The version has changed so I can't build and deploy it.

[pkiraly]

@kcondon
I updated the develop branch with

git checkout develop
git fetch upstream develop
git rebase upstream/develop
git push

then I updated branch 7784-prevent-null-as-email-sender which belongs to this PR:

git checkout 7784-prevent-null-as-email-sender
git merge develop
git push

Is that what you expected? If not, please let me know.

[kcondon]

@pkiraly Thanks, testing now.

[pkiraly]

This PR was developed by Göttingen eResearch Alliance, Germany, and funded by SSHOC, "Social Sciences and Humanities Open Cloud". SSHOC has received funding from the European Union’s Horizon 2020 project call H2020-INFRAEOSC-04-2018, grant agreement #823782.