Conversation
scolapasta
left a comment
scolapasta
left a comment
There was a problem hiding this comment.
Overall, looks as we discussed. Just had one comment and one question.
src/main/java/edu/harvard/iq/dataverse/DataverseServiceBean.java
Outdated
Show resolved
Hide resolved
src/test/java/edu/harvard/iq/dataverse/branding/BrandingUtilTest.java
Outdated
Show resolved
Hide resolved
Note - a similar test will reappear when BrandingUtil supports a property that can replace the rootDataverseName for getInstallationBrandName()
|
@scolapasta - FYI - I made similar changes to avoid settingsService getting passed through the ExportService since you last looked at this. With that I think this is done. |
src/main/java/edu/harvard/iq/dataverse/branding/BrandingUtil.java
Outdated
Show resolved
Hide resolved
|
|
||
| //Convenience to access root name without injecting dataverseService (e.g. in DatasetVersion) | ||
| public static String getRootDataverseCollectionName() { | ||
| return dataverseService.getRootDataverseName(); |
There was a problem hiding this comment.
Shouldn't we do this more consistent then and go for changing the function in DataverseServiceBean, too? (I am aware of the consequences)
scolapasta
left a comment
scolapasta
left a comment
There was a problem hiding this comment.
Discussed with @qqmyers. Longer term we want to get rid of the Helpers and make these Stateless EJBs, but for this contains several big steps getting us there AND unblocks SciencesPO work. So we'll move forward and create an issue for the next steps.
|
Suggested regression testing places were:m where root dv name is fetched and excluding email from export. Have found email exclude is not working. root name is. |
5 participants
What this PR does / why we need it: Gives BrandingUtil, MailUtil and JsonPrinter access to services they require instead of having to have callers send the services or the results of the calls (such as the root dataverse name) to the server as method parameters. This is basically clean-up but also preparation for #7387.
Most of the PR is making the simplifications possible made possible by that - i.e. dropping method params that aren't needed. One additional change was to remove local code to find the rootDataverseName in DatasetVersion and JsonPrinter that were only used to get the name so it could be passed to BrandingUtil.
Also added the same design to OREMap to avoid having to send settingsService through the ExportService.
There are also changes to update tests - removing parameters and adding mock services.
Which issue(s) this PR closes:
Closes #7649
Special notes for your reviewer: FWIW: I think the lack of mocks in tests initially made me think that I couldn't keep static methods, but injecting a static service does work and I was able to avoid having to inject the util classes everywhere (as discussed in the issue)
Suggestions on how to test this: This should change anything w.r.t. functionality, and it shouldn't add new functionality (a PR for #7387 needs the changes here and will add new functionality). The one potential issue I can see would be that some calls to get the root dataverse name that were done by traversing the chain of owners are now replaced with a call to find the root Dataverse from the db. If that results in a measurable performance difference, adding some simple caching to the call to get the name could be done. (I didn't want to add that to this PR without reason, but it could be added here or as a quick follow-on PR.)
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: