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

Implement vocabulary value selectors in item’s metadata edit form #2653

Merged
merged 9 commits into from
Feb 23, 2024

Conversation

toniprieto
Copy link
Contributor

@toniprieto toniprieto commented Nov 20, 2023

References

Description

This PR uses the existent DsDynamicOneboxComponent and DsDynamicScrollableDropdownComponent components to be able to edit metadata controlled by vocabularies in item's metadata edit form in the same way that is done in submission form.

Instructions for Reviewers

List of changes in this PR:

  • Use components DsDynamicOneboxComponent and DsDynamicScrollableDropdownComponent in item's metadata edit form for fields controlled by vocabularies:
    • DsDynamicOneboxComponent: is used for vocabularies that use a autocomplete functionality and for hierarchical vocabularies
    • DsDynamicScrollableDropdownComponent: is used for vocabularies configured with value-pairs element in input-forms.xml (dc.type, dc.language.iso, etc.)
  • Add a function to the Vocabulary service to retrieve whether a metadata uses controlled vocabularies
  • Add a field to edit the authority key in a similar way to previous versions
  • Modify the AuthorityConfidenceStateDirective to work with MetadataValue
  • Move Drag Button inside btn-group to fix border display
  • Modify DsDynamicOneboxComponent:
    • Add aria-label attribute for accessibility. This component normally uses the aria-labelledby attribute but these labels are not included in the edit form
    • Add a property to show that the initial value is being loaded. Some authority sources that use API, ORCID or Sherpa Romeo, may be somewhat slow when checking if the initial value is validated. This property allows you to show that the information is being loaded.

For testing vocabularies that use a autocomplete functionality, include this in local.cfg (uses the authorities from Sherpa Romeo and ORCID).

For use ORCID integration is also required to edit your config/spring/api/orcid-authority-services.xml uncommenting the ORCID part.

orcid.api-url = https://pub.sandbox.orcid.org/v3.0

plugin.named.org.dspace.content.authority.ChoiceAuthority = \
 org.dspace.content.authority.SHERPARoMEOPublisher = SRPublisher, \
 org.dspace.content.authority.SHERPARoMEOJournalTitle = SRJournalTitle, \
 org.dspace.content.authority.SolrAuthority = SolrAuthorAuthority
 
choices.plugin.dc.title.alternative = SRJournalTitle
choices.presentation.dc.title.alternative = suggest
authority.controlled.dc.title.alternative = true

choices.plugin.dc.publisher = SRPublisher
choices.presentation.dc.publisher = suggest
authority.controlled.dc.publisher = true

choices.plugin.dc.contributor.author = SolrAuthorAuthority
choices.presentation.dc.contributor.author = authorLookup
authority.controlled.dc.contributor.author = true
authority.author.indexer.field.1=dc.contributor.author

sherpa.romeo.apikey = <sherpa-api-key>

NOTE: although it should be possible to implement it, these changes do not apply right now to the first field to add new metadata

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@alanorth
Copy link
Contributor

alanorth commented Nov 20, 2023

@toniprieto this is very cool. I tested your changes on DSpace 7.6.1 and it Just Works™. The technique of using the mechanism as in the submission form is clever because it doesn't require any extra configuration.

We have about ten fields that use controlled vocabularies. I tested editing and saving on several fields that have a mix of value-pairs and external XML controlled vocabularies. Works great! I have not tested any authority-controlled fields like ORCID–only normal controlled vocabularies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toniprieto i'd try to re-use the existing directive https://github.com/DSpace/dspace-angular/blob/main/src/app/shared/form/directives/authority-confidence-state.directive.ts instead of implementing a new component to do the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @atarix83 ! You're right, I just removed the new component and used the directive instead although I still want to check if I can improve the display of the indicator when it is displayed outside of an input field like in this PR.

@tdonohue tdonohue added component: Item (Archived) Item display or editing component: authority control Related to internal authority control system labels Nov 21, 2023
Copy link

github-actions bot commented Dec 6, 2023

Hi @toniprieto,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

@kshepherd
Copy link
Member

Just want to add my feedback, not a full review, but I've tried out this PR and it is working great with some new external data lookups (via choiceauthority) and traditional CV in various presentations

@alanorth
Copy link
Contributor

Testing this again via a squashed version of this commit (I think the final pull request should be the finished version of this, without the intermediate work, and without the whitespace additions).

Still working well on DSpace 7.6.2-SNAPSHOT.

@toniprieto
Copy link
Contributor Author

I think this is ready for review. I have squashed/reorganized the commits and updated the PR description with more information.

@alanorth
Copy link
Contributor

alanorth commented Jan 4, 2024

Thanks, @toniprieto! Happy to see the unnecessary whitespace removed and extra commits squashed.

I rebased this patch on DSpace 7.6.1 and it works well. I don't have any more feedback on the functionality, so I will let someone with Angular experience comment on the implementation.

@tdonohue since this technically restores functionality that was in DSpace 6 I think we should consider porting this to dspace-7_x. On that note, there might be some documentation updates needed.

alanorth pushed a commit to ilri/dspace-angular that referenced this pull request Jan 6, 2024
- Implement getVocabularyByMetadataAndCollection in VocabularyService
- Add aria-label and loadingInitialValue property to DsDynamicOneboxComponent
- Add support for MetadataValue class to AuthorityConfidenceStateDirective
- Implementation of controlled vocabularies value selectors during item editing

See: DSpace#2653
@tdonohue
Copy link
Member

tdonohue commented Jan 8, 2024

@alanorth : Per a decision by Steering, DSpace 7.6.x will have no new features added in future releases (as 7.6.x is in maintenance mode). So, this new feature will only be possible to add to 8.x.

Copy link

Hi @toniprieto,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toniprieto : I'm testing this today and I'm running into several bugs (at least they appear to be bugs to me...but correct me if I'm wrong).

  1. On the "Edit Metadata" tab, if I just click "+ Add", there's no option to search in an authority / controlled vocab. It appears you first must type in a value, click "Confirm" (green checkmark) and then edit it again to see the vocabulary search. Is that purposeful? Here's a small video to show the behavior. no-vocabulary-until edit
  2. If I type in an author name that matches an existing authority value, sometimes the authority is auto-assigned without my approval. In this video, there is an authority record in my SolrAuthorAuthority for "Donohue, Timothy". Notice how it gets auto-assigned in this video without my selecting it.
    authority-add-automatically
  3. It appears I can select virtual::* authority fields which are managed by Configurable Entities. When I do so, I can no longer save the metadata value... all buttons are disabled with popups that explain that I cannot edit this field anymore. Here's a video again:
    virtual-metadata
  4. The lookup for Publisher is changing my initial value when I edit it. In this video, I add a new Publisher named "School". If I edit it, this triggers the lookup which changes my value automatically to "Association of Schools of Allied Health Professions". It doesn't let me select a value.. it selects one for me. selected-school

Overall, the basics appear to work. I'm just finding the experience to be very buggy. I'm running this using production mode (yarn build:prod; yarn serve:ssr) with a configuration similar to that you recommended (I also have a sherpa romeo key and ORCID sandbox configured alongside this):

solr.authority.server=${solr.server}/authority
plugin.named.org.dspace.content.authority.ChoiceAuthority = \
 org.dspace.content.authority.SolrAuthority = SolrAuthorAuthority, \
 org.dspace.content.authority.SHERPARoMEOPublisher = SRPublisher,\
 org.dspace.content.authority.SHERPARoMEOJournalTitle = SRJournalTitle

choices.plugin.dc.contributor.author = SolrAuthorAuthority
choices.presentation.dc.contributor.author = authorLookup
authority.controlled.dc.contributor.author = true
authority.author.indexer.field.1=dc.contributor.author

choices.plugin.dc.publisher = SRPublisher
choices.presentation.dc.publisher = lookup
authority.controlled.dc.publisher = true

choices.plugin.dc.relation.ispartof = SRJournalTitle
choices.presentation.dc.relation.ispartof = lookup
authority.controlled.dc.relation.ispartof = true

@toniprieto
Copy link
Contributor Author

toniprieto commented Feb 9, 2024

@tdonohue Thanks for the detailed feedback!

Yes, I also think they are bugs, I will try to fix them these days. Regarding the first one, although it is done on purpose, I now think it is better if it is also implemented for the new fields. I'll work on that.

I think some of these bugs are backend issues and can possibly be reproduced in the current submission form as well. For example, the SolrAuthority that is used for the ORCID authority should not index/return virtual authorities, or the issue 4 which is partly due to a poor implementation of the method getBestMatch() of the Sherpa Romeo authorities. I will look at it in more detail and make new PRs for the backend if I confirm that it is the problem.

@tdonohue
Copy link
Member

@toniprieto : Just wanted to make sure you are aware that our 8.0 feature PR merger deadline is coming up next week on Friday, Feb 23. I'd love to see this PR included in 8.0, if you can find time to get those bugs fixed. (Extensions are possible if the PR is really close, but ideally we'd get this re-reviewed and merged in the next week.)

@toniprieto
Copy link
Contributor Author

@tdonohue I have updated the PR, the 4 issues should be addressed:

  1. The vocabulary selector is now implemented when a new value is added.
  2. This is a issue related with the behavior of the ORCID SolrAuthority. With this kind of authorities, when a new value is added the consumer authority automatically assign a new authority key with confidence equals to 0 (NOVALUE). I suppose that you have this consumer configured in:
event.dispatcher.default.consumers = versioning, discovery, eperson, submissionconfig, qaeventsdelete, authority

I have added a change to hide the authority key when the confidence value is equals to 0 (NOVALUE) or -1 (UNSET) to address this issue.
This authority key is displayed in the input field if you edit this field:

imagen

Additional note: To make the value of the confidence field more apparent while editing, I have made a change to the Confidence directive to allow its use in a different "mode" that uses fontawesome icons, which should be more identifiable. I have tried to limit this change to a single commit in case it turns out to be odd and needs to be reverted (5d2977a)

  1. I think that the main problem of this issue is that virtual metadata should not be indexed in Solr Authority Core. I have submitted a Backend PR to avoid this: Avoid index virtual metadata in Solr Authority core DSpace#9326
  2. And this issue is related with a simple implementation of the getBestMatch() of Sherpa Romeo Authorities. It should be solved by this backend PR: Improve implementation of getBestMatch() of SHERPARoMEO authorities DSpace#9327

@toniprieto
Copy link
Contributor Author

toniprieto commented Feb 16, 2024

@tdonohue Additional note, I'm investigating another issue that can be reproduced with:

  1. Add a new field and select the dc.publisher
  2. Enter a word to get suggestions. For example "Mechanics"
  3. Wait for the suggestions to appear, and do not select any
  4. With the suggestions popup open, click on the "Confirm button"
  5. The field is added empty (But ideally, it should have added "Mechanics")

I think it is a bug in the DsDynamicOneboxComponent component that can also be reproduced in the submission form. I am trying to fix it.

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toniprieto : Re-tested this today with the two backend PRs. It's working much better.... and the prior issues appear to be fixed. That said, I'm able to reproduce two new issues (one of which you found too).

  1. First, I can reproduce the issue you described in your latest comment
  2. Second, when I edit an text entry that does NOT have an authority entry, I'm finding the experience odd. Here's a video of what I see. This Author is not linked to an authority, but when I click edit it appears to be linked to an authority entry
    editing-entry

To set this second bug up I did the following:

  • Add a new Author who has a name that matches an authority record. Do not select the authority record. Just leave the author name unattached to an authority.
  • Save that Author
  • Now, click edit on that Author. The matching authority ID appears in the edit screen & a green circle appears. Both imply the Author is attached to an authority record, but it is not.

Beyond those two issues, everything else is looking good. Thanks!

…fontawesome icons instead of class defined in style property
…namicOneBoxComponent when the user doesn't select any typeahead suggestion. The previous code attempted to maintain focus on the input field to not lose changes but it wasn't working.
…odel into the DsDynamicOneboxComponent. Previously, the accepted confidence value was always displayed if a valid vocabulary entry was retrieved, incorrectly displaying it for values without accepted authority.
@toniprieto
Copy link
Contributor Author

Hi @tdonohue,

The first issue should be resolved.

About the second, I have done a change to prevent the green circle from being displayed when the current confidence value is not ACCEPTED. The behavior has improved, although it is not perfect. The authority key still appears, but it is because the SolrAuthority plugin used for the dc.contributor.author field always adds them when saving a value without authority, although with an invalid confidence.

An e2e test has failed, i don't know if it is a random failure.

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks @toniprieto ! I retested today, and verified everything is fixed except for the minor issue you noted

In my opinion, that's "good enough" for 8.0, as the behavior is much better than in 7.x (where these options didn't exist). We can solve anything else in future PRs as necessary. Thanks again!

@tdonohue tdonohue added this to the 8.0 milestone Feb 23, 2024
@tdonohue tdonohue merged commit fc4f0ec into DSpace:main Feb 23, 2024
13 checks passed
@tdonohue
Copy link
Member

Adding the needs documentation flag temporarily to remind us that the docs may need to be updated for 8.0 regarding this feature.

@tdonohue tdonohue added the needs documentation PR is missing documentation. All new features and config changes require documentation. label Feb 23, 2024
@toniprieto
Copy link
Contributor Author

toniprieto commented Mar 28, 2024

@tdonohue I have updated the submission settings section of the "User Interface Configuration" to update the icon configuration modified by this PR: https://wiki.lyrasis.org/display/DSDOC8x/User+Interface+Configuration#UserInterfaceConfiguration-SubmissionSettings

And I have added a new section "Add or edit authority controlled metadata fields" to the Edit Metadata page: https://wiki.lyrasis.org/display/DSDOC8x/Edit+Metadata
Feel free to rephrase it if it can be explained better. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: authority control Related to internal authority control system component: Item (Archived) Item display or editing new feature
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Unable to lookup for metadata controlled by authority in edit view
5 participants