Fix simple item view author link #2949

wwtamu · 2024-04-17T14:35:57Z

References

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

Fixes Item View Authors Link Resolving with Incorrect Browse by Query Parameter #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!

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.

ybnd · 2024-04-17T19:48:58Z

src/app/core/shared/value-list-browse-definition.model.ts

@@ -31,6 +31,6 @@ export class ValueListBrowseDefinition extends NonHierarchicalBrowseDefinition {
  };

  getRenderType(): string {
-    return this.dataType;
+    return this.type.value;


I encountered a few similar issues today while resolving merge conflicts on #2865 (WIP), which included upgrading TSESLint and introducing a new default rule for enum consistency

There were a few cases where the return value of getRenderTypes was being compared with a ResourceType, and I was considering replacing them with type instead

If we change this browse definition we should probably do the same for the other ones

If we use this.type here, do we still need the method at all?

dspace-angular/src/app/core/shared/browse-definition.model.ts

Line 15 in fd000cd

abstract getRenderType(): string;

The abstract inheritance requires it of implementations. I don't think I would make that many changes to resolve a bug in a past release. The component and class structure might require revisiting and that would be a scope creep within this patch. Additionally, as the unit test required breaking apart and it went under the coverage of e2e tests, I would think additional e2e tests (failing in this regard) are required to safely resolve. Then again, I have seen larger changes to fix a bug before.

Perhaps an additional e2e test could be in the forward port to the next release.

Good point about the scope creep, should probably be it's own issue/PR 👍

We could turn these rules off, but it seems that they indicate some important inconsistencies. A few cases have been ignored inline because they should be investigated in more detail, which is out of scope for this PR: - Metadata representation components compare `BrowseByDataType` to a `ResourceType`. Could be related to DSpace#2949. - We assume that dynamic form dates are always represented by strings, but they can also be `Date` or `object` according to the library (see https://github.com/udos86/ng-dynamic-forms/blob/da1742ce051af5cebdcf905b189ab6b55fe365d7/projects/ng-dynamic-forms/core/src/lib/model/dynamic-date-control.model.ts#L5)

wwtamu · 2024-04-26T13:19:58Z

@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?

tdonohue · 2024-04-26T14:37:45Z

@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 · 2024-05-10T16:51:12Z

@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.

wwtamu · 2024-05-24T14:28:15Z

@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?

tdonohue · 2024-05-24T14:52:51Z

@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.

wwtamu · 2024-05-24T15:54:36Z

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

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

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

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

tdonohue · 2024-06-05T20:38:59Z

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:

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

tdonohue · 2024-07-11T15:21:33Z

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.

Return constant type value for explicitly named component

b1824da

github-actions bot assigned wwtamu Apr 17, 2024

Remove tracing comments

fd000cd

ybnd reviewed Apr 17, 2024

View reviewed changes

ybnd mentioned this pull request Apr 18, 2024

Metadata representation components compare BrowseByDataType values to ResourceType #2955

Open

wwtamu removed their assignment Apr 21, 2024

wwtamu changed the title ~~Return Constant Type Value for Explicitly Named Component~~ Fix simple item view author link Apr 21, 2024

kshepherd mentioned this pull request Jul 3, 2024

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

Open

9 tasks

wwtamu closed this Jul 23, 2024

wwtamu deleted the issue-2948 branch July 23, 2024 13:21

alexandrevryghem removed the port to main This PR needs to be ported to `main` branch for the next major release label Nov 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix simple item view author link #2949

Fix simple item view author link #2949

wwtamu commented Apr 17, 2024 •

edited

Loading

ybnd Apr 17, 2024 •

edited

Loading

wwtamu Apr 17, 2024 •

edited

Loading

wwtamu Apr 17, 2024 •

edited

Loading

ybnd Apr 18, 2024

wwtamu commented Apr 26, 2024

tdonohue commented Apr 26, 2024

tdonohue commented May 10, 2024

wwtamu commented May 24, 2024 •

edited

Loading

tdonohue commented May 24, 2024

wwtamu commented May 24, 2024 •

edited

Loading

tdonohue commented Jun 5, 2024 •

edited

Loading

tdonohue commented Jul 11, 2024

Fix simple item view author link #2949

Fix simple item view author link #2949

Conversation

wwtamu commented Apr 17, 2024 • edited Loading

References

Description

Instructions for Reviewers

Checklist

ybnd Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

wwtamu Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

wwtamu Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

ybnd Apr 18, 2024

Choose a reason for hiding this comment

wwtamu commented Apr 26, 2024

tdonohue commented Apr 26, 2024

tdonohue commented May 10, 2024

wwtamu commented May 24, 2024 • edited Loading

tdonohue commented May 24, 2024

wwtamu commented May 24, 2024 • edited Loading

tdonohue commented Jun 5, 2024 • edited Loading

tdonohue commented Jul 11, 2024

wwtamu commented Apr 17, 2024 •

edited

Loading

ybnd Apr 17, 2024 •

edited

Loading

wwtamu Apr 17, 2024 •

edited

Loading

wwtamu Apr 17, 2024 •

edited

Loading

wwtamu commented May 24, 2024 •

edited

Loading

wwtamu commented May 24, 2024 •

edited

Loading

tdonohue commented Jun 5, 2024 •

edited

Loading