dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

Various browse link rendering fixes (value vs startsWith, authority checks)

Open kshepherd opened this issue 1 year ago • 0 comments

Various browse link rendering fixes (value vs startsWith, authority checks)

References

  • Fixes #2955
  • This is an alternate fix of PR #2949. @wwelling , we had different approaches to this so I'm interested to hear any feedback you have. This PR can be backported to 7.x fairly easily (my first dev version was for 7.x).

Description

Browse links for metadata representations and plain values were incorrectly (or confusingly?) comparing the getRenderType() of the browse definition (e.g. text, date, item) to e.g. VALUE_LIST_BROWSE_DEFINITION.value (e.g. valueList, flatBrowse, etc.).

Since browse links were not supported in early 7.x, I think there was some mismatch of render hints and configuration when they did get introduced. I think using BrowseDataType enums makes the most sense for matching our configuration and expected behaviour:

export enum BrowseByDataType {
  Title = 'title',
  Metadata = 'text',
  Date = 'date',
  Hierarchy = 'hierarchy',
}

These are the browse by pages we want our links to route to, based on the last segment of the configuration as below:

webui.browse.index.1 = dateissued:item:dateissued
webui.browse.index.2 = author:metadata:dc.contributor.*\,dc.creator:text
webui.browse.index.3 = title:item:title
webui.browse.index.4 = subject:metadata:dc.subject.*:text

In addition, I extended the MetadataRepresentation model to allow a getAuthority getter: this is necessary as it's not enough to say "is authority controlled" when passing a value to be rendered as a browse link -- we also need the key.

I shifted getQueryParams() to the parent MetadataRepresentationListElement component and changed its behaviour, along with the same method for MetadataValues component:

  • If the the browse definition render type (data type) is BrowseByDataType.Metadata, then:
    • If the metadata value or representation has non-virtual authority key (does not start with virtual::) then return value and authority as query params
    • Otherwise just return value in query params
  • Otherwise, just return startsWith in query params

Instructions for Reviewers

Compare the behaviour of dspace-8.0 / main branch with this PR. The default dspace.cfg is enough to run with but I recommend adding some additional webui.browse.link.<n> configs like subject, and testing different data type config in the last segment, and compare results when using both <ds-generic-item-page-field> components (metadata value lists) and <ds-metadata-representation-list> components (reps of related items or values, with different components responsible for actual value display)

Other notes

As I noted, this might need discussion. I think some of the way we have separated representations from item page fields -> values leads to a bit of duplicate handling and confusion in some areas...

Alternate fix

The difference between this and https://github.com/DSpace/dspace-angular/pull/2949 is that I don't think valueList is really the right thing to send as a 'browse by data type', I think it is a render hint used by some other angular components (as well as flatBrowse, hierarchicalBrowse, etc.) but is a different concept to the "data types" we have used in configuration and other parts of DSpace to determine which type/level of browse page to use: a metadata browse page using value or startsWith, a 'title' page listing item titles with startsWith filter, a date page with issue date breakdowns, etc.

In any case I think there are some configuration cleanups needed since the list of browse types is not one-for-one that I can see, and perhaps needs a rethink since it was really designed for JSPUI 1.x.

Needs tests

If this PR looks good conceptually, it needs tests to handle extension of metadata rep model, and changes in query param methods.

  • [ ] 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.
  • [ ] My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • [ ] My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • [ ] 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.

kshepherd avatar Jul 03 '24 16:07 kshepherd