Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DS-4107 Represent DSO metadata as a map in REST #2287

Merged
merged 2 commits into from Feb 21, 2019

Conversation

Projects
None yet
4 participants
@cwilper
Copy link
Contributor

commented Dec 11, 2018

REST Contract: DSpace/Rest7Contract#39

This modifies the DSpace 7 REST API implementation to model embedded DSO metadata as maps, as described here: https://jira.duraspace.org/browse/DS-4107

Most of these changes were lifted from the POC PR: #2175 ...where representing metadata as a map was identified as useful keep.

Overall lines of code were reduced because the change provided an opportunity to share code in some places.

ITs have also been updated to expect the new form.

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch from e8d6295 to 694d622 Dec 23, 2018

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch 2 times, most recently from 7b0c6ce to 553ff24 Jan 2, 2019

@terrywbrady terrywbrady self-requested a review Jan 31, 2019

@terrywbrady
Copy link
Contributor

left a comment

@cwilper , this is a nice cleanup of the code. The changes look good.

I completed my review. I see that a re-base of the code is needed. Ping me once that is resolved and I will run a quick test.

@abollini
Copy link
Member

left a comment

Hi @cwilper thanks for this PR: the implementation looks fine to me. Please add the necessary javadoc for public method and solve the conflict so that we can formally approve the PR. This PR will have a big impact on the angular side so I recommend to get a correspondent PR on the angular side ready before to merge

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch from 553ff24 to 927ac6f Feb 7, 2019

@cwilper

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Thanks for taking a look @terrywbrady and @abollini . I have rebased on master and believe I have implemented all suggested improvements (mainly javadocs).

Note that the angular side of this was done a few weeks ago. I will shortly be rebasing that as well, after which I will link here. I do recommend this only be merged when the related angular changes are also ready to be merged.

@abollini
Copy link
Member

left a comment

thanks for the javadoc. It is fine for me now

@cwilper

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Angular implementation has been rebased and is ready for review: DSpace/dspace-angular#347

@terrywbrady terrywbrady self-requested a review Feb 7, 2019

@terrywbrady
Copy link
Contributor

left a comment

The changes look good. I did a quick test in HAL and I see that the map is present for collections and items.

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch from de505d8 to 0609a6f Feb 20, 2019

@cwilper

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Rebased to latest master

@tdonohue

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Merging as this is at +2, and looks good to me too!

@tdonohue tdonohue merged commit cc21394 into DSpace:master Feb 21, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 29.872%
Details

@artlowel artlowel referenced this pull request Feb 28, 2019

Merged

[DS-3695] Upgrade Solr to 7.3.1. #2058

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.