[DSpace-CRIS] Inverse Relations via Authority Framework (Frontend)#5134
[DSpace-CRIS] Inverse Relations via Authority Framework (Frontend)#5134tdonohue merged 11 commits intoDSpace:mainfrom
Conversation
…old module import
tdonohue
added
new feature
DSpace-CRIS merger
|
@FrancescoMolinaro : Could you specify which DSpace-CRIS differences this PR relates to? I think it's important to be clear to reviewers which features or architectural changes are in each PR, and it's unclear to me which one(s) this references |
|
Hi @FrancescoMolinaro, |
|
@FrancescoMolinaro : This appears to require backend PR DSpace/DSpace#11963 , so I've updated the description to note that |
tdonohue
changed the title
KevinVdV
left a comment
KevinVdV
left a comment
There was a problem hiding this comment.
I made an initial review of this PR, I found 2 small issues, see details below.
Issue 1
The backend add support for the following pages to make use of this search box:
- people
- org units
- projects,
But in the PR the only place theds-authority-related-entities-searchis used is in the person entity.
Issue 2
The new authority framework UI component is enabled by default, this will cause with backwards compatibility. Can you modify the configuration so that the authority framework is optional (so it is the same as in the REST code)
github-project-automation
bot
moved this from 🙋 Needs Reviewers Assigned
to 👀 Under Review
in DSpace 10.0 Release
…figure projects and org units
|
Hi @KevinVdV , thanks for the feedback, I have updated the item pages so that also OrgUnit and Projects have their configurations as on the backend. |
|
@FrancescoMolinaro Did you forget to push by any chance ? Cause there are no recent commits in this branch & the feature is still enabled by default. |
|
Hi @KevinVdV yes I didn't realize my commit didn't pass through, now the PR should be up to date, thanks for the catch. |
config/config.example.yml
Outdated
| link: https://ror.org | ||
|
|
||
| # Enable authority based relations in item page | ||
| showAuthorithyRelations: false |
There was a problem hiding this comment.
@FrancescoMolinaro : I haven't fully reviewed/tested this PR, but I have an immediate question about this PR.
How does this showAuthorityRelations frontend setting relate to the new relationship.enable-virtual-metadata = true setting on the backend?
The reason I ask is that I'm trying to understand whether you always would want showAuthorityRelations to be set to true whenever relationship.enable-virtual-metadata is set to false. If so, then we might want to consider reading the value of relationship.enable-virtual-metadata from the backend in order to determine the correct behavior of the frontend.
But, if these configs are unrelated, then we definitely should keep both. I'm just trying to better understand if this PR requires relationship.enable-virtual-metadata=false or not.
There was a problem hiding this comment.
Hi @tdonohue, I think the property relationship.enable-virtual-metadata is necessary for the creation of the relations but not necessary to display existing ones, I am thinking a scenario in which the relations are created but then the feature gets disabled on the rest side.
This should be anyway an edge case and I agree that would be useful to have a single source of truth to enable/disable the feature.
We can maybe check along the rest PR if a new property would be necessary for the scope or we can assume the state of the relations feature from relationship.enable-virtual-metadata and ignore the edge case I mentioned.
I will run a check with @AdamF42 and come back as soon as possible.
There was a problem hiding this comment.
Hi @tdonohue , after checking this with @AdamF42 , we think is best to not rely on the rest property relationship.enable-virtual-metadata as it would create an unnecessary dependency, the framework of the inverse relations is anyway active also if that property is set to false.
We believe that the best approach would be to keep the UI config param as on the rest side there isn't an actual toggle for the inverse relations. I would really appreciate to hear also your thoughts on this.
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
@FrancescoMolinaro : I'm not understanding how to get this PR to work properly. I cannot seem to figure out how to get the tabbed display in your screenshot in the PR description.
Here's what I've done:
- I've installed this PR and the corresponding backend PR (leaving their default settings as-is)
- I've set
relationship.enable-virtual-metadata = falseon the backend anditem > showAuthorityRelations: trueon the frontend. - I've created a single Publication which is linked to three authors (Person entities) via the authority framework.
- I've reindexed the site (
./dspace index-discovery -b) - Then I've visited the Item page for the Publication and also for the Person entities. I do not see the tabbed interface.
Am I misunderstanding how this should work? Could you please walk me through how to create some test data which is valid for this feature? Thanks!
KevinVdV
left a comment
KevinVdV
left a comment
There was a problem hiding this comment.
Trying to review as well, I can get the tabs to show up but having trouble configuring the authority based entities for anything other than author / publications.
I'm using the following configuration, can you maybe tell me what I'm doing wrong ?
plugin.named.org.dspace.content.authority.ChoiceAuthority = \
org.dspace.content.authority.OrcidAuthority = AuthorAuthority, \
org.dspace.content.authority.ItemAuthority = ProjectAuthority,\
org.dspace.content.authority.ItemAuthority = OrgUnitAuthority
cris.ItemAuthority.AuthorAuthority.entityType = Person
cris.ItemAuthority.AuthorAuthority.source = orcid
dc.relation.project: Research project (Project entity)
choices.plugin.dc.relation.project = ProjectAuthority
choices.presentation.dc.relation.project = suggest
authority.controlled.dc.relation.project = true
choices.plugin.dc.publisher = OrgUnitAuthority
choices.presentation.dc.publisher = suggest
authority.controlled.dc.publisher = true
person.affiliation.name: Person's affiliation (links to OrgUnit)
choices.plugin.person.affiliation.name = OrgUnitAuthority
choices.presentation.person.affiliation.name = suggest
authority.controlled.person.affiliation.name = true
Hi @KevinVdV, Additionally, we've cleaned up all unused configurations for inverse relations to ensure consistency with the current changes. Try with the following configuration: # Named Authority Plugins
plugin.named.org.dspace.content.authority.ChoiceAuthority = \
org.dspace.content.authority.OrcidAuthority = AuthorAuthority, \
org.dspace.content.authority.ItemAuthority = ProjectAuthority,\
org.dspace.content.authority.ItemAuthority = OrgUnitAuthority
choices.plugin.dc.relation.project = ProjectAuthority
choices.presentation.dc.relation.project = suggest
authority.controlled.dc.relation.project = true
choices.plugin.person.affiliation.name = OrgUnitAuthority
choices.presentation.person.affiliation.name = suggest
authority.controlled.person.affiliation.name = true
choices.plugin.dc.contributor.author = AuthorAuthority
choices.presentation.dc.contributor.author = suggest
authority.controlled.dc.contributor.author = true
cris.ItemAuthority.AuthorAuthority.source = orcid
cris.ItemAuthority.AuthorAuthority.entityType = Person
cris.ItemAuthority.ProjectAuthority.entityType = Project
cris.ItemAuthority.OrgUnitAuthority.entityType = OrgUnit
choices.plugin.project.investigator = AuthorAuthority
choices.presentation.project.investigator = suggest
authority.controlled.project.investigator = true
choices.plugin.dc.contributor.other = OrgUnitAuthority
choices.presentation.dc.contributor.other = suggest
authority.controlled.dc.contributor.other = true
choices.plugin.dc.description.sponsorship = OrgUnitAuthority
choices.presentation.dc.description.sponsorship = suggest
authority.controlled.dc.description.sponsorship = true
orcid.authority.prefix = will be generated::ORCID::
relationship.enable-virtual-metadata = false |
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
👍 Thanks @FrancescoMolinaro ! I gave this another review and test today. After the fix to the configuration typo, everything is working great! I'm able to now see the new tabs on Person pages, OrgUnit pages, etc. I've mostly tested from the Person perspective, but everything is working well there, and I've verified OrgUnits are working at a basic level as well.
Flagging this as "needs documentation" since this feature will require docs to be added to the 10.0 docs at https://wiki.lyrasis.org/display/DSDOC10x
tdonohue
added
the
needs documentation
|
@KevinVdV : Please rereview / retest this as soon as you can, as this is blocking several other DSpace-CRIS merger PRs. If there are outstanding issues you are finding, we can either ask to get them fixed quickly in this PR, or we can also consider creating follow-up bug tickets. Thanks! |
|
Hi @FrancescoMolinaro, |
|
@FrancescoMolinaro : This PR also now has merge conflicts. If you could quickly rebase this one as well, I'd appreciate it. Thanks! |
|
I did some further testing & was able to get the relationships to work, however I also found that the "Research Project" entity doesn't have the tabs yet (maybe a commit is missing because you mentioned you changed it: #5134 (comment)) What I did successfully test:
|
|
@FrancescoMolinaro and @AdamF42 : It looks like these "Inverse Relations" PRs are really close to being merged. However, I can verify what @KevinVdV notes above is true: Person and OrgUnit entities work well with the tabs. But, Project entities are not displaying the tabs. If necessary, we could move that to a follow-up bug ticket. But, if there's a quick fix that can be applied to these PRs, that'd also be great! Let us know if this is easy to fix or if we should log a bug ticket and fix this in a follow-up PR. |
|
Hi @KevinVdV , @tdonohue , we removed the projects configurations, from frontend and backend, as we noticed they were based on metadata present only on CRIS and this would mean changing the default submission as well. We can always adapt the submission and relation at a later time to support also Projects, but I believe that would require a more general discussion. I would really appreciate to hear your thoughts on this and thanks for the feedback! |
|
@FrancescoMolinaro : Thanks for clarifying. Since that's expected behavior, this PR seems fine to me. Can you please rebase this PR though to fix the merge conflicts (so it can be merged)? There are also merge conflicts on #5145 that need to be fixed (so that it can be tested more easily) UPDATE: I meant to also add that we should be sure to document that Projects don't have tabs yet. I'll create a bug ticket related to that known issue, and we can always close that ticket once this feature has been added. (Bug ticket created in #5318 ) |
KevinVdV
left a comment
KevinVdV
left a comment
There was a problem hiding this comment.
If we fix the project review later on a PR, I'm also at a +1
|
@FrancescoMolinaro and @KevinVdV : I've created a bug ticket to capture this issue with the Project entities. See #5318. This is just to remind us that this needs to be fixed in a future PR. |
|
Merging as this is at +2. Thanks again @FrancescoMolinaro ! As with all other DSpace-CRIS merger PRs, I'm flagging this as "needs documentation" until docs can be added to https://wiki.lyrasis.org/display/DSDOC10x |
github-project-automation
bot
moved this from 👍 Reviewer Approved
to ✅ Done
in DSpace 10.0 Release
4 participants
References
Description
Add new component for tabbed relations in item page.
The new relations are based on a scope parameter which represent the authority of the item in question.
Visually the component still uses the standard search page and is identical:
The new relations can be activated via a flag param in the default.app.config.ts, inside the item config:
showAuthorityRelations
if true the ui components will use the new authority based relations.
Instructions for Reviewers
List of changes in this PR:
Add new UI component for new scope/authority based relations
Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.