Refactoring authority framework, value pairs and controlled vocabulary #2792

abollini · 2020-06-25T20:25:19Z

References

Wiki page with the preliminary discussion: https://wiki.lyrasis.org/display/DSPACE/4Science+proposal+to+improve+authority+support+in+DSpace7

Rest Contract PR DSpace/RestContract#128

Angular PR: DSpace/dspace-angular#751

Description

This PR restores the previous way of DSpace to store metadata when values are retrieved from valuepairs and controlled vocabularies (without the authority ID in the metadatavalue).
Nevertheless, value-pairs and controlled vocabularies are managed using the same infrastructure previously used for Authority Control, for these reasons we agree to rename all these features under the common name of controlled vocabularies.

Finally, this PR also fix a couple of outstanding issues that were noted on the previous implementation
https://jira.lyrasis.org/browse/DS-4350
https://jira.lyrasis.org/browse/DS-4371
https://jira.lyrasis.org/browse/DS-4408

this also replace the previous PR #2743 adding the necessary features to support the visualization of the hierarchical vocabularies

Instructions for Reviewers

ITs have been added to cover the new VocabularyEntryDetails endpoint and the refactoring related to the link repositories.
Other than reviewing the code by inspection and checking the ITs it could be useful to verify the correct exposition of the default submission definitions where the value-pairs must be exposed as common_types vocabulary. It is also possible to enalbe the srsc controlled vocabulary to verify that it meet the example in the REST Contract

Checklist

My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
My PR passes Checkstyle validation based on the Code Style Guide.
My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
[N/A] If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

the visualization is now responsability of the angular UI so it is better by default present a clean label as the hierarchy is still visible in the otherInformation

…cabulary

lgtm-com · 2020-07-18T14:38:17Z

This pull request fixes 2 alerts when merging e06832fd38fa2e4ee15985300d30d0b7a7f7ecb4 into 6b3bd4d - view on LGTM.com

fixed alerts:

1 for Useless comparison test
1 for Dereferenced variable may be null

lgtm-com · 2020-07-18T15:57:31Z

This pull request fixes 2 alerts when merging 769dd06 into 6b3bd4d - view on LGTM.com

fixed alerts:

1 for Useless comparison test
1 for Dereferenced variable may be null

lgtm-com · 2020-07-18T16:55:30Z

This pull request fixes 2 alerts when merging b232def into 6b3bd4d - view on LGTM.com

fixed alerts:

1 for Useless comparison test
1 for Dereferenced variable may be null

abollini · 2020-07-18T17:11:30Z

@tdonohue @benbosman all your previous feedback have been addressed as far as I can check. To simply the review I have explicitly linked the "solution" under each not yet resolved comment.

About the ORCID test mentioned here #2792 (comment) it cannot be performed because the ORCID integration with the authority framework was dropped sometimes ago but it should be restored (I have created #2891 for that)

tdonohue

👍 Thanks @abollini ! I've re-reviewed the code, and all previous requests (by both @benbosman and I) seem to be addressed. I've also manually tested the endpoints and they respond as documented in the contract per DSpace/RestContract#128

That said, I'm noticing that I cannot merge this PR until the Angular PR (DSpace/dspace-angular#751) is also ready to merged. Unfortunately, without the corresponding Angular PR, the current main submission form is unusable (I get an Error: Authority name is not available. Please check the form configuration file.). That said, I think this PR is ready to go as soon as the Angular PR is also ready.

benbosman · 2020-07-29T11:03:37Z

dspace-api/src/main/java/org/dspace/content/authority/ChoiceAuthorityServiceImpl.java

+    private void addFormDetailsToAuthorityCacheMap(String submissionName, String authorityName, String fieldKey) {
+        Map<String, List<String>> authorityName2definitions;
+        if (authoritiesFormDefinitions.containsKey(authorityName)) {
+            authorityName2definitions = authoritiesFormDefinitions.get(authorityName);


This variable name sounds quite confusing. It seems to be a map from the submission name to the fields (and the authorityName is not the same as the submission name)

benbosman · 2020-07-29T12:17:56Z

dspace-api/src/main/java/org/dspace/content/authority/DCInputAuthority.java

@@ -44,10 +50,10 @@
 public class DCInputAuthority extends SelfNamedPlugin implements ChoiceAuthority {
    private static Logger log = org.apache.logging.log4j.LogManager.getLogger(DCInputAuthority.class);

-    private String values[] = null;
-    private String labels[] = null;
+    private Map<String, String[]> values = null;


Can you add JavaDocs explaining what values and labels are now?
If values is a map, it's not clear what the keys and values of the map are (and similar for labels)

benbosman

At this point in time, this branch should not be merged since there's no Angular UI. This also implies I currently can't test the authority control part at all (I have no solr authority core to test with).
Once the Angular UI branch is ready, I will perform another review. Please ping me at that time

I've added a few comments, which can be improved in the meanwhile as well

I noticed another problem with this branch which doesn't need to be solved, but reported:

The Accept-Language is not handled completely yet. When using Accept-Language: nl;q=0.8,en;q=0.7 it will return Dutch values on /server/api/submission/vocabularies/common_iso_languages/entries, but when using Accept-Language: de;q=0.9,nl;q=0.8,en;q=0.7 it will return the English values because German is configured to be supported, but not used for the submission forms. This can perhaps be added to Multi-linguism requires submission-forms to be in sync #2827 as another use case where translations only work if everything is translated

abollini · 2020-07-30T16:09:00Z

latest feedback has been addressed. The angular part is in this PR DSpace/dspace-angular#751 that as by today dev mtg discussion will be updated today by @atarix83 to be in a testable status

lgtm-com · 2020-07-30T16:36:49Z

This pull request fixes 2 alerts when merging ed58d44 into fa3e714 - view on LGTM.com

fixed alerts:

1 for Useless comparison test
1 for Dereferenced variable may be null

lgtm-com · 2020-08-06T09:10:38Z

This pull request fixes 2 alerts when merging 70c861a into 2027b03 - view on LGTM.com

fixed alerts:

1 for Useless comparison test
1 for Dereferenced variable may be null

benbosman

I did not retest it using the UI, but I've discussed it with Art and he hasn't seen any REST issues while testing with DSpace/dspace-angular#751

tdonohue · 2020-09-11T14:41:44Z

This is at +2, as it the corresponding Angular PR (DSpace/dspace-angular#751). So, I'm merging both & pushing an update to the REST API demo site. Thanks @abollini & @Micheleboychuk (and all the reviewers)!

Micheleboychuk and others added 30 commits June 10, 2020 17:00

added ITs for Controlled Vocabularies

e39c2a8

added ITs for Vocabulary Entries

5f6775a

added tests

1765d2a

update tests of Vocabulary

d794c1c

Initial refactoring for vocabularies (Draft)

8afa698

Refactoring for vocabularies, second part (Draft)

17c3833

Merge branch 'CST-3088' into draft_vocabulary

dc2f686

fix checkstyle

9849d6c

renamed classes

4784d34

fix creation of the internal maps of authorities

d7ce794

Remove invalid test, cleanup

3e8b2f8

Update default to reflect the rest contract

be6f7f3

the visualization is now responsability of the angular UI so it is better by default present a clean label as the hierarchy is still visible in the otherInformation

Fix test setup, update response code according to the contract

ec3df2e

Fix typos

9458176

Align test with the contract

ae60714

Implement check for mandatory parameters

f44b28a

Add support for : in string identifier

d38dfdb

Cleanup authority service implementation

61c3460

Add initial implementation for VocabularyEntryDetails repository

e9abbc5

missing filling list of metadata

5376539

refactored tests

1114903

implemented search method byMetadataAndCollection

ea9d5da

refactored search method byMetadataAndCollection

fa27690

added VocabularyEntryResource

52dfbfd

refactored RestResourceController

cd842fd

VocabularyEntryRest must implement RestModel

056a0fb

fix bug

f9f0067

fix tests

de49227

added test

e473f11

renamed embedded link in tests

efb4f6a

abollini added 2 commits July 17, 2020 19:39

Merge branch 'main' of https://github.com/DSpace/DSpace into draft_vo…

07ffb5f

…cabulary

Add link to the vocabularyEntryDetails search methods from the root

9863b1c

Add support and test for locales on vocabularies

769dd06

abollini force-pushed the draft_vocabulary branch from e06832f to 769dd06 Compare July 18, 2020 15:30

Keep the message rethrowing the exception

b232def

abollini mentioned this pull request Jul 18, 2020

Support for ORCID and other external providers in the authority framework is needed #2891

Closed

abollini requested review from tdonohue and benbosman July 18, 2020 17:11

tdonohue approved these changes Jul 20, 2020

View reviewed changes

tdonohue moved this from Under Review to Reviewer Approved in DSpace 7 Beta 4 Jul 23, 2020

benbosman reviewed Jul 29, 2020

View reviewed changes

abollini added 2 commits July 30, 2020 17:53

Describe the structure of the map fields

bd6982b

Use a more meaningful variable name

ed58d44

ddinuzzo and others added 2 commits August 6, 2020 10:20

Merge branch 'main' into draft_vocabulary

5e30cbe

fix builder import

70c861a

benbosman approved these changes Sep 11, 2020

View reviewed changes

tdonohue merged commit 4340b50 into DSpace:main Sep 11, 2020

DSpace 7 Beta 4 automation moved this from Reviewer Approved to Done Sep 11, 2020

abollini deleted the draft_vocabulary branch September 22, 2020 12:43

tdonohue mentioned this pull request Sep 1, 2021

[DS-4475] Add support for Controlled Vocabulary in the REST API v7 #7810

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring authority framework, value pairs and controlled vocabulary #2792

Refactoring authority framework, value pairs and controlled vocabulary #2792

abollini commented Jun 25, 2020 •

edited by tdonohue

lgtm-com bot commented Jul 18, 2020

lgtm-com bot commented Jul 18, 2020

lgtm-com bot commented Jul 18, 2020

abollini commented Jul 18, 2020

tdonohue left a comment

benbosman Jul 29, 2020

benbosman Jul 29, 2020

abollini Jul 30, 2020

benbosman left a comment

abollini commented Jul 30, 2020

lgtm-com bot commented Jul 30, 2020

lgtm-com bot commented Aug 6, 2020

benbosman left a comment

tdonohue commented Sep 11, 2020

Refactoring authority framework, value pairs and controlled vocabulary #2792

Refactoring authority framework, value pairs and controlled vocabulary #2792

Conversation

abollini commented Jun 25, 2020 • edited by tdonohue

References

Description

Instructions for Reviewers

Checklist

lgtm-com bot commented Jul 18, 2020

lgtm-com bot commented Jul 18, 2020

lgtm-com bot commented Jul 18, 2020

abollini commented Jul 18, 2020

tdonohue left a comment

Choose a reason for hiding this comment

benbosman Jul 29, 2020

Choose a reason for hiding this comment

benbosman Jul 29, 2020

Choose a reason for hiding this comment

abollini Jul 30, 2020

Choose a reason for hiding this comment

benbosman left a comment

Choose a reason for hiding this comment

abollini commented Jul 30, 2020

lgtm-com bot commented Jul 30, 2020

lgtm-com bot commented Aug 6, 2020

benbosman left a comment

Choose a reason for hiding this comment

tdonohue commented Sep 11, 2020

abollini commented Jun 25, 2020 •

edited by tdonohue