Custom Installation Name, Optional Use as DDI Distributor by qqmyers · Pull Request #7661 · IQSS/dataverse

qqmyers · 2021-03-06T18:08:43Z

What this PR does / why we need it: Adds a new setting that will be used instead of the Root Dataverse Collection Name for the Brand Name (BrandingUtil.getInstallationBrandName() ) if set and a second setting that avoids adding the brand name as a Distributor in the DDI export studyDsc if the Distributor is already set in the metadata (part of the Citation block).

Which issue(s) this PR closes:

Closes #7387

Special notes for your reviewer: Per discussions/issue comments:

This PR changes the broadly used getInstallationBrandName method so if the new setting is used, the change will occur everywhere that is called (primarily in metadata exports and email notifications). If there are any places where this isn't desired, there is a BrandingUtil.getRootDataverseName() call that can be used instead.
One new setting is implemented as an MPconfig variable that also has a settings alias.
- The variables and setting name (in microprofile-aliases.properties, better name welcome) are:

dataverse.export.distributor.excludeinstallationifset=dataverse.settings.fromdb.ExportInstallationAsDistributorOnlyWhenNotSet

(Note that #7680 prevented doing this for

dataverse.branding.installation.name=dataverse.settings.fromdb.InstallationName

which is just a standard setting in the final PR.)

In trying to get this to work, (heads up to @poikilotherm ) I discovered [three bugs in the DbSettingsConfigSource class](see this commit and this one) which could be considered in a separate PR if needed.
As far as I can tell, this is the first use of microprofile vars in the code / the first use of the new DbSetting and Alias ConfigSources. FWIW, I ended up choosing the programmatic approach versus injecting the variable itself - I wasn't able to figure out how to make that work (possibly due to testing using a db setting (see BrandingUtilTest) and therefore relying on the DbSettingConfigSource which requires some initialization.)
If we want to use this approach to support incremental change in the code, one additional change might be useful - to switch the warning in AliasConfigSource when an alias is used to use logger.fine() from logger.warning() (at least for now. If at some point we recommend not using the db settings, we could make the warning more prominent.)

Suggestions on how to test this:

This PR depends on Update util classes #7657, so that should be tested/merged first.
The basic functionality is probably easiest to test by setting the :InstallationName and :ExportInstallationAsDistributorOnlyWhenNotSet properties. The first should override the use of the root dataverse collection name in emails and metadata exports (see Distributor tag automatically added in the OAI-DDI and DDI export #7387 for a list - in a comment near the end). The second should only affect whether the DDI exports (DDI, DDI HTML Codebook, OpenAire) have an extra stdyDsc distr element when the dataset metadata already has a Distributor entry (true -> only the value from the metadata appears) (It's the contributor contributorType="Distributor" element in OpenAire). (Note that the doc dstr in the DDI export will still use the Installation Name regardless, so make sure you're checking the right element.)
I assume, but have not tested that you can also set dataverse.export.distributor.excludeinstallationifset via micro-profile directly (perhaps @poikilotherm can provide a suggestion).

Does this PR introduce a user interface change? If mockups are available, please link/include them here: only to the text shown where the brand name previously was always the root dataverse collection name.

Is there a release notes update needed for this change?: just noting the new options.

Additional documentation: documented the new settings in the guide

@Inject

just committing to record the code I tried to @Inject - did this and tried just a static private String, but both result in null pointer exceptions. The Provider<String> suggests it will dynamically get new values, but the javadocs does say something about checking at the start to see if the property exists (which it doesn't for DbSettingsConfigSource?)

clear old properties on update invert logic to make caching work

in DDI export stdyDcr - only use installation name if there is no distributor defined in metadata

qqmyers · 2021-03-06T18:26:47Z

doc/sphinx-guides/source/installation/config.rst


 Custom Homepage
-++++++++++++++++
+++++++++++++++


FWIW: I think my rst plugin is catching these now. I just added the two entries at the bottom.

djbrooke · 2021-03-08T16:04:56Z

Thanks @qqmyers. Just a quick question - for documentation purposes, if an installation chooses update the setting, will they need to run reexportall to realize the changes?

qqmyers · 2021-03-08T16:37:01Z

@djbrooke - good catch - added notes in the docs.

src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java

src/main/resources/META-INF/microprofile-aliases.properties

doc/sphinx-guides/source/installation/config.rst

src/main/java/edu/harvard/iq/dataverse/branding/BrandingUtil.java

poikilotherm · 2021-03-09T16:30:24Z

src/main/java/edu/harvard/iq/dataverse/settings/source/DbSettingConfigSource.java

    public Map<String, String> getProperties() {
        // if the cache is at least XX number of seconds old, update before serving data.
-        if (lastUpdate == null || Instant.now().minus(Duration.ofSeconds(60)).isBefore(lastUpdate)) {
+        if (lastUpdate == null || Instant.now().minus(Duration.ofSeconds(60)).isAfter(lastUpdate)) {


Good catches, thank you @qqmyers

Should we put these in a seperated PR and add tests so this is getting checked? (Happy to contribute)

FWIW: I found one more w.r.t. mapping the setting names via Alias. I'd say that unless you have concerns about the fixes, it's not worth another PR - these classes aren't used anywhere else yet and the changes are small.

seeing the following when using the alias/db sources: Caused by: java.lang.IllegalStateException: Recursive update at java.base/java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1983) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getValue(PayaraConfig.java:157) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getValueInternal(PayaraConfig.java:127) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getOptionalValue(PayaraConfig.java:123) at edu.harvard.iq.dataverse.settings.source.AliasConfigSource.getValue(AliasConfigSource.java:87) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getSourceValue(PayaraConfig.java:183) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getValueConverted(PayaraConfig.java:166) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.lambda$getValue$1(PayaraConfig.java:161) at java.base/java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1908) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getValue(PayaraConfig.java:157) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getValueInternal(PayaraConfig.java:127) at fish.payara.nucleus.microprofile.config.spi.PayaraConfig.getOptionalValue(PayaraConfig.java:123) at edu.harvard.iq.dataverse.branding.BrandingUtil.getInstallationBrandName(BrandingUtil.java:19) at edu.harvard.iq.dataverse.util.MailUtil.getSubjectTextBasedOnNotification(MailUtil.java:33) at edu.harvard.iq.dataverse.MailServiceBean.sendNotificationEmail(MailServiceBean.java:247)

Conflicts: src/main/java/edu/harvard/iq/dataverse/branding/BrandingUtil.java src/main/java/edu/harvard/iq/dataverse/branding/BrandingUtilHelper.java src/test/java/edu/harvard/iq/dataverse/branding/BrandingUtilTest.java

poikilotherm · 2021-03-26T13:44:42Z

@qqmyers @sekmiller @kcondon

Looks like #7680 might be a blocker for this. Will resolve with a PR for #7639, but to not stop the show going on here, we might remove the MPCONFIG parts first.

qqmyers · 2021-03-26T13:53:46Z

It's not a blocker (as it works as it is now), but I can do a quick edit to remove the DB source class that you plan to remove given your new design.

qqmyers · 2021-04-01T19:19:43Z

An update - with the discussion and new direction with microprofile support, this PR is now independent of them. It now just introduces:

an :InstallationName setting that, when set, is used instead of the root dataverse name in places where that name was being used for branding (i.e. in metadata exports, but not changing the name displayed for the root dataverse it the UI.)
an :ExportInstallationAsDistributorOnlyWhenNotSet that avoids adding the repository brand name as a "Distributor" in the DDI metadata export stdyDsc element if a Distributor was already set in a Dataset's metadata. (So there will always be a stdyDsc Distributor element, but if this setting is true, there won't be an additional one listing the repository brand name.)

qqmyers added 9 commits March 5, 2021 17:32

Bug fixes

0df20d6

clear old properties on update invert logic to make caching work

add installation setting and make including it as distr optional

6e89b9b

in DDI export stdyDcr - only use installation name if there is no distributor defined in metadata

simplify logic with getOptionalValue()

72028e6

don't send settingsService through ExportService

7942737

Merge branch 'SPO/7649_micro' into SPO/7387

b424915

null is not an option

cdc99d5

remove imports

d52634d

settings doc

a06c56c

qqmyers commented Mar 6, 2021

View reviewed changes

qqmyers added the GDCC: SciencesPO related to GDCC work for Sciences PO label Mar 6, 2021

update per review

42534fc

poikilotherm reviewed Mar 9, 2021

View reviewed changes

qqmyers added 10 commits March 11, 2021 13:35

use helper

cca7a3c

use helper

d91d4ac

change the dataset-level publisher field to use brand name

f92dab7

DbSetting bug fix

3238a56

add new settings

df8973f

test updates to match

4ca9618

no longer used

16e1e7b

fix field type name

07e4fd7

add private to Services

b906212

qqmyers mentioned this pull request Mar 12, 2021

Microprofile recursive update exception when trying to use the Alias/DBSetting sources #7680

Closed

Merge branch 'SPO/7649_micro' into SPO/7387

9488bc9

Conflicts: src/main/java/edu/harvard/iq/dataverse/branding/BrandingUtil.java src/main/java/edu/harvard/iq/dataverse/branding/BrandingUtilHelper.java src/test/java/edu/harvard/iq/dataverse/branding/BrandingUtilTest.java

sekmiller self-assigned this Mar 24, 2021

sekmiller approved these changes Mar 26, 2021

View reviewed changes

sekmiller removed their assignment Mar 26, 2021

qqmyers added 2 commits March 26, 2021 12:22

revert to using setting (versus microprofile) per discussion in issues

3e9b353

update tests

848f533

kcondon self-assigned this Apr 1, 2021

Merge remote-tracking branch 'IQSS/develop' into SPO/7387

cafa2a7

kcondon mentioned this pull request Apr 1, 2021

Export: Address :InstallationName gaps #7750

Open

kcondon merged commit 66789fd into IQSS:develop Apr 1, 2021

djbrooke added this to the 5.4 milestone Apr 2, 2021

djbrooke added a commit that referenced this pull request Apr 2, 2021

updates because #7661 got merged

4ad690f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Custom Installation Name, Optional Use as DDI Distributor#7661

Custom Installation Name, Optional Use as DDI Distributor#7661
kcondon merged 24 commits intoIQSS:developfrom
GlobalDataverseCommunityConsortium:SPO/7387

qqmyers commented Mar 6, 2021 •

edited

Loading

Uh oh!

qqmyers Mar 6, 2021

Uh oh!

djbrooke commented Mar 8, 2021

Uh oh!

qqmyers commented Mar 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poikilotherm Mar 9, 2021

Uh oh!

qqmyers Mar 12, 2021

Uh oh!

poikilotherm commented Mar 26, 2021 •

edited

Loading

Uh oh!

qqmyers commented Mar 26, 2021

Uh oh!

qqmyers commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

Conversation

qqmyers commented Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qqmyers Mar 6, 2021

Choose a reason for hiding this comment

Uh oh!

djbrooke commented Mar 8, 2021

Uh oh!

qqmyers commented Mar 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poikilotherm Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

qqmyers Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

poikilotherm commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qqmyers commented Mar 26, 2021

Uh oh!

qqmyers commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qqmyers commented Mar 6, 2021 •

edited

Loading

poikilotherm commented Mar 26, 2021 •

edited

Loading