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

Fix simple item view author link

Open WWelling opened this issue 1 year ago • 7 comments

References

Add references/links to any related issues or PRs. These may include:

  • Fixes #2948

Description

Fixes the simple item view author links.

Instructions for Reviewers

See issue above.

List of changes in this PR:

  • First, returned this.value.type, which is hardcoded "valueList", instead of data type that could never evaluate to the desired query parameter for the URL link to the browse by author.
  • Second, update tests to test the static reference to this.value.type.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • [x] 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.
  • [x] My PR passes ESLint validation using yarn lint
  • [x] 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.
  • [x] 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.

WWelling avatar Apr 17 '24 14:04 WWelling

@tdonohue does port to main label require another PR into main for this or will there be additional work involved to resolve for acceptance into main?

WWelling avatar Apr 26 '24 13:04 WWelling

@wwelling : This is simply waiting on a reviewer. Because the PR is not built on main it has a lower priority right now as we are finishing up the 8.0 release (and have a lot of outstanding bug fixes we are working on reviewing). If you wish to create a version on main you are welcome to do so. However, assuming the code you modified exists on both dspace-7_x and main (i.e. this PR can be merged cleanly into main as well), then it will be ported automatically once merged.

If you'd like to speed up the review process for any of your PRs, we have a PR Trading option where you can volunteer to review others PRs in exchange for this one being reviewed more quickly. https://wiki.lyrasis.org/display/DSPACE/Trading+reviews+on+Pull+Requests

Otherwise, this PR will be reviewed as soon as we find a volunteer who has time to review it. Or, as soon as someone else finds this useful and just tests it and reports back on this PR.

tdonohue avatar Apr 26 '24 14:04 tdonohue

@wwelling : I found time to look more closely at this today. Because of the changes in this area of the code between 7.x and 8.x, this PR is not able to be applied to pre-8.0 (main). I'm not certain how to move this forward, as the code on main is quite different.

If you find time to create a secondary PR for 8.0 (against main) that could help this PR along. Overall, I'm trying to ensure any bug fixes can be applied to both branches (dspace-7_x and main), assuming they affect both branches.

tdonohue avatar May 10 '24 16:05 tdonohue

@tdonohue this resolves 7x and the PR for 8x as you discovered will be significantly different. How is this 7x patch being blocked with that discovery and patching 8x?

Does the bug still manifest itself in 8x?

WWelling avatar May 24 '24 14:05 WWelling

@wwelling : When a bug is discovered in DSpace, we obviously want to ensure it is patched in both 7.x and 8.x if it affects both. I've not had time to verify yet whether this affects both, but my assumption was that this bug likely does (unless I've overlooked us patching it in a different manner in 8.x).

In general, we refrain from fixing bugs only in 7.x unless they are verified to no longer impact 8.x. The reason is that we simply don't want a bug to be fixed in an older release while it still appears in a newer release....that can causes institutions upgrading to be frustrated as bugs that were previously fixed will reappear.

I haven't had time to get back to this PR myself to do a more thorough analysis (as I'm busy getting 8.0 finished up). I'm hoping another developer can test whether this bug is truly 7.x specific and no longer exists on 8.x. Perhaps one easier way to do so is to look at the differences in behavior on demo.dspace.org (running 7.x) versus sandbox.dspace.org (running 8.x), if it's possible to reproduce the behaviors there.

tdonohue avatar May 24 '24 14:05 tdonohue

Is still kind of a bug in 8. This is where the author link on the simple item view is from dc.contributor.author and not part of relationship such as relation.isAuthorOfPublication.

https://sandbox.dspace.org/entities/publication/e1ffc1c5-284d-492c-9742-b18f0c4f8a43

image

https://sandbox.dspace.org/browse/author?startsWith=Gillen,%20Martie

image

https://sandbox.dspace.org/browse/author?value=Gillen,%20Martie

image

This finally gets to the expected results of publications by the author.

WWelling avatar May 24 '24 15:05 WWelling

It sounds like this fix needs to also have a main PR. Otherwise, we only fix this issue in 7.x and then 8.0 will revert back to doing a startsWith=[name] search instead of a value=[name] search as noted above.

I'd hesitate to only merge this into 7.x because of that behavior change. Ideally, we should have this feature working the same way in both 7.x and 8.x. (And, as noted, the implementation in this PR won't work for 8.x. The modified ValueListBrowseDefinition.getRenderType() method now returns a BrowseByDataType instead of a simple string in 8.x.)

UPDATE: I meant to add, I'm currently seeing identical behavior on demo (running 7.x) and sandbox (running 8.x). So, this bug looks the same on both. Here's an example item that has two plain text authors...it exists on both demo and sandbox:

  • https://demo.dspace.org/entities/publication/12623672-25a9-4df2-ab36-699c4c240c7e
  • https://sandbox.dspace.org/entities/publication/12623672-25a9-4df2-ab36-699c4c240c7e

In both scenarios the author link works. But, it goes to the startsWith=[name] search instead of the value=[name] search.

tdonohue avatar Jun 05 '24 20:06 tdonohue

An alternative fix has been added in #3159 (to potentially replace this PR). This alternative PR seems more compatible with 8.x, so we may want to verify whether it is a complete replacement for this PR or not.

tdonohue avatar Jul 11 '24 15:07 tdonohue