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 Metadata as map updates for Angular #347

Merged
merged 6 commits into from Feb 21, 2019

Conversation

Projects
None yet
5 participants
@cwilper
Copy link
Contributor

cwilper commented Dec 23, 2018

Updates the angular code to consume and work with metadata modeled as a map. Some code consolidation was done here, so metadata-processing code is done in a common place, which is now well-covered by an included set of tests.

To work, this requires REST PR: DSpace/DSpace#2287 which returns DSO metadata in the expected map form.

For more information, see https://jira.duraspace.org/browse/DS-4107

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch from 6bb16b7 to 6742011 Dec 23, 2018

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch from 6742011 to ea1fe59 Feb 7, 2019

@cwilper

This comment has been minimized.

Copy link
Contributor Author

cwilper commented Feb 7, 2019

Rebased on latest master. This is ready for review and when approved, should be merged at the same time as the corresponding REST implementation: DSpace/DSpace#2287

@atarix83
Copy link
Contributor

atarix83 left a comment

I added some minor changes required.
After changed and resolved conflict with master branch this could be merged for me

Show resolved Hide resolved src/app/core/browse/browse.service.ts
Show resolved Hide resolved src/app/core/shared/dspace-object.model.ts Outdated
Show resolved Hide resolved src/app/core/shared/dspace-object.model.ts Outdated
Show resolved Hide resolved src/app/core/shared/dspace-object.model.ts Outdated
Show resolved Hide resolved src/app/core/shared/dspace-object.model.ts Outdated
Show resolved Hide resolved src/app/core/shared/metadata.model.ts Outdated
Show resolved Hide resolved ...-grid/search-result-grid-element/search-result-grid-element.component.ts Outdated
Show resolved Hide resolved ...-grid/search-result-grid-element/search-result-grid-element.component.ts Outdated
Show resolved Hide resolved ...-list/search-result-list-element/search-result-list-element.component.ts Outdated
Show resolved Hide resolved ...-list/search-result-list-element/search-result-list-element.component.ts Outdated

@cwilper cwilper force-pushed the atmire:DS-4107_Metadata_as_map branch from ea1fe59 to e499f79 Feb 14, 2019

@cwilper

This comment has been minimized.

Copy link
Contributor Author

cwilper commented Feb 14, 2019

Thanks for the the review @atarix83 . I have rebased, resolving conflicts, and improved TypeDocs and used isUndefined/isNotUndefined as suggested.

@cwilper

This comment has been minimized.

Copy link
Contributor Author

cwilper commented Feb 15, 2019

Looks like checks have passed, so once @atarix83 approves, I believe this can be merged.

Note: whoever merges this, please merge the REST impl side at the same time (already approved): DSpace/DSpace#2287

@atarix83

This comment has been minimized.

Copy link
Contributor

atarix83 commented Feb 18, 2019

@cwilper thanks for the changes.
It looks good to me now

@paulo-graca
Copy link

paulo-graca left a comment

This is a structural change with a big code refactoring. My comments are to just avoid having names as value.value. I will tested it.

@paulo-graca

This comment has been minimized.

Copy link

paulo-graca commented Feb 20, 2019

This PR doesn't work properly, metadata isn't showed. I'm using atmire's REST endpoints. Please check my print-screen (https://drive.google.com/open?id=1bJypXyDXECJP7BlJ3LXsTGtR45dX2PXz).

@cwilper

This comment has been minimized.

Copy link
Contributor Author

cwilper commented Feb 20, 2019

@paulo-graca thanks for taking a look. I have adjusted variable naming to hopefully be more clear. I liked your suggestion about using mdValue and mdValues for clarity, and implemented that. You also suggested renaming entry to mdItem. I agree with making it more specific (prefix with md, consistent with the others), but I think "Entry" is really the right term here rather than "Item" -- it is a single entry of a map, which contains a key and value property. So I called it mdEntry. Please let me know if that works ok for you.

This PR doesn't work properly, metadata isn't showed.

This PR requires that you run with a specific REST PR or it won't work (see description). That PR is not yet running on Atmire's server and probably won't (permanently) until both PRs are merged. So I would suggest running a local copy of the the REST PR in order to test functionality.

@paulo-graca

This comment has been minimized.

Copy link

paulo-graca commented Feb 20, 2019

👍 @cwilper thank you for considering my suggestions! I will try to do a new test with the dependency DSpace/DSpace#2287

@paulo-graca
Copy link

paulo-graca left a comment

I managed to get it work on my development environment using the PR2287. Great job!

@tdonohue

This comment has been minimized.

Copy link
Member

tdonohue commented Feb 21, 2019

Already at +2, and it looks great to me too (did a quick scan of the code). Merging. Thanks @cwilper!

@tdonohue tdonohue merged commit 562160b 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.2%) to 78.111%
Details

@wafflebot wafflebot bot removed the needs review label Feb 21, 2019

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.