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 #39

Merged
merged 3 commits into from Jan 31, 2019

Conversation

Projects
None yet
3 participants
@cwilper
Copy link
Contributor

commented Dec 13, 2018

Updates the REST contract to show DSO metadata modeled as a map.

This brings the structure of the metadata for all DSOs in line with how it is modeled for workspace items: https://github.com/DSpace/Rest7Contract/blob/master/workspaceitem-data-metadata.md

See jira issue https://jira.duraspace.org/browse/DS-4107 for links to PRs for rest and angular impls.

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch from 8bd7608 to f5ce797 Dec 30, 2018

@abollini
Copy link
Member

left a comment

It looks fine to me. Some comments inline related to minor details to fix. I suggest to merge this PR only after than the actual implementation as been merged (please link the code PR here if/when available)

"dc.description": [
{
"value": "<p>This is a <em>DSpace Collection</em> which contains sample DSpace Items.</p>\r\n<p><strong>Collections in DSpace may only contain Items.</strong></p>\r\n<p>This particular Collection has its own logo (the <a href=\"http://www.opensource.org/\">Open Source Initiative</a> logo).</p>\r\n<p>This introductory text is editable by System Administrators, Community Administrators (of a parent Community) or Collection Administrators (of this Collection).</p>",
"language": null

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 0

This comment has been minimized.

Copy link
@cwilper

cwilper Jan 2, 2019

Author Contributor

Thanks for taking a look, @abollini. I have added authority: null, confidence: -1 to all examples below, and merged master into the branch so it's up-to-date.

However, I did omit place from the json representation intentionally -- it is already expressed by the position in the array and is modified via a json patch that changes the position in the array, rather than by mutating the property. I have thus also annotated this property as @JsonIgnore in the associated REST code PR, here: https://github.com/DSpace/DSpace/pull/2287/files#diff-a00adfba6d1ea813d3dc12f39446f60bR28 Is that sufficient to address your concern?

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 9, 2019

Member

This resolution seems reasonable to me. I'm wondering if we should add a comment though near that @JsonIgnore annotation to explain why it is ignored (that place is represented instead by the array order).

UPDATE: Actually, perhaps that also should be included (as a textual description) in the contract. I like the updated examples in the Contract, but just based on those examples it is not entirely clear that the array order matters. Perhaps we should find a way to explicitly state that in the contract (especially the Item contract, since Item's are much more likely to have multi-valued metadata fields, e.g. dc.contributor.author).

{
"value": "This collection contains sample items.",
"language": null
}

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 0

"dc.description.tableofcontents": [
{
"value": "<p>This is the <strong>news</strong> section for this Collection. System Administrators, Community Administrators (of a parent Community) or Collection Administrators (of this Collection) can edit this News field.</p>",
"language": null

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 0

"dc.provenance": [
{
"value": "This field is for private provenance information. It is only visible to Administrative users and is not displayed in the user interface by default.",
"language": null

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 0

},
{
"value": "Second provenance value",
"language": null

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 1 (note place: 1 as it is the second value)

items.md Outdated
"dc.contributor.author": [
{
"value": "Stvilia, Besiki",
"language": "en"

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 0

items.md Outdated
},
{
"value": "Lee, Dong Joon",
"language": "en"

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 1

items.md Outdated
"dc.identifier.url": [
{
"value": "http://europepmc.org/abstract/MED/28301533",
"language": "en"

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 0

items.md Outdated
"dc.title": [
{
"value": "Practices of research data curation in institutional repositories: A qualitative view from repository staff",
"language": "en"

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 0

items.md Outdated
"dc.type": [
{
"value": "Journal Article",
"language": "en"

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

please add also authority: null, confidence: -1, place: 0

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch from b8d14e2 to a6fc1d4 Jan 2, 2019

@tdonohue
Copy link
Member

left a comment

👍 This looks good to me. Thanks @cwilper! Assuming @abollini has no further comments, I think this direction sounds fine. I'll wait on final approval of this Contract though before thorough review of the related PRs.

@cwilper

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

@abollini Did you have any further comments on this? I think the review of a few other PRs is being held up by lack of progress on this one.

Other related PRs, currently (hopefully temporarily) taking different approaches:

  • Mixing entities and plain text values - See Ben's comment: Once Chris' work has been approved and merged, I would prefer to remove the changes here and use Chris' work instead.
  • Endpoint for eperson profile patches - See Michael's comment: Both PR's appear to be in conflict with #46 and work on metadata patches by cwilper. If that approach is taken then I think this PR (and it's related PR) will become smaller in scope, ...
@abollini
Copy link
Member

left a comment

as already stated in #39 (review) I'm fine with this approach. My minor suggestion have been applied.

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.