Implement Signposting on angular side#2248
Conversation
…ction when on item page
…ace-angular into feature/CST-5729
tdonohue
added
new feature
1 APPROVAL
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
@atarix83 : Overall, this looks good and it appears to work. I do have some minor suggestions to improve the code inline below. But, overall the direction looks good to me
I also noticed that the Link header is only returned when SSR is used. I'm assuming that probably the correct behavior, but I wanted to verify to be certain that's expected. I never see that Link header when CSR is used. If this is proper behavior, is there some way to ensure the logic in item-page.component.ts which builds that Link header is only used during SSR?
I can verify though that the HTML <link> tags in the <head> appear for either SSR or CSR.
|
Hi @atarix83, |
# Conflicts: # src/app/bitstream-page/bitstream-download-page/bitstream-download-page.component.ts
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
@atarix83 : I re-reviewed this today and tested it with the backend. The code looks good & it is working. I can verify that in the Link Header is only returned in SSR mode... and both CSR and SSR will return the HTML links embedded in the page.
I've also verified that nothing crashes when Signposting is turned off (signposting.enabled = false in local.cfg). However, I have noticed that there's a somewhat odd, extra error in the DevTools console with Signposting turned off. Here's how to reproduce it:
- Turn off signposting on backend (
signposting.enabled = falsein local.cfg) - Restart Tomcat (required to turn it off)
- Now, visit any Item page in the UI, and you'll see two errors in the Console (anytime you access an Item page)
- First, you'll get the expected 404 response from the signposting endpoint.
- But, immediately after that you'll get a second error which appears to come from the Angular codebase related to that 404?
If this second error is possible to avoid, that'd be nice. However, I don't quite understand why it's displayed every time the (expected) 404 error occurs.
Overall, I'm +1 this PR. But, I'll need to retest it once the backend PR is updated/fixed based on my latest review. Thanks!
|
there was an additional log error message that i've removed because is not neccesary, could you check if is ok now ? |
abollini
added
the
claimed: 4Science
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
@atarix83 : This looks good to me now & works well. That said, I did find a very minor issue (at least I think it's an issue) where the Link type is sometimes returned as "undefined" like this:
<https://demo7.dspace.org/handle/123456789/233> ; rel="cite-as" ; type="undefined" ,
The same behavior occurs in the HTML <link> tags. I don't think it's harmful, but it might be good to simply leave out the type attribute if it is empty or undefined.
|
undefined type fixed |
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
@atarix83 : This is looking great, but I noticed a new small bug in today's tests. It appears the UI Proxy for Signposting URLs is incorrect.
The generated URLs appear to always be [dspace.ui.url]/signposting/links*.... but the Proxy in this PR expects them to be [dspace.ui.url]/links*, so the generated links always return a 404.
I think it's probably best to fix the Proxy to use the /signposting path. See inline below. Once this minor change is made, everything else in this PR looks great!
server.ts
Outdated
| /** | ||
| * Proxy the linksets | ||
| */ | ||
| router.use('/links**', createProxyMiddleware({ |
There was a problem hiding this comment.
This proxy appears to be incorrect. Re-tested today and the signposting response for a Bitstream (http://localhost:8080/server/signposting/links/[bitstream-uuid]) looks like this:
[{"href":"http://localhost:4000/signposting/linksets/94554d4d-cfc5-44bb-96ab-1a80ed637a24","rel":"linkset","type":"application/linkset"},
{"href":"http://localhost:4000/signposting/linksets/94554d4d-cfc5-44bb-96ab-1a80ed637a24/json","rel":"linkset","type":"application/linkset+json"},
{"href":"http://localhost:4000/entities/publication/94554d4d-cfc5-44bb-96ab-1a80ed637a24","rel":"collection","type":"text/html"}]
Notice how the Angular UI URLs all start with /signposting/links*... but this Router appears to expect them to start with /links*
[DSC-1906] add link to processes page in user menu Approved-by: Giuseppe Digilio
3 participants
References
Description
Add angular implementation to provide signposting functionality
Instructions for Reviewers
List of changes in this PR:
/server/signpostingendpointInclude guidance for how to test or review your PR.
yarn start:prodin order the SSR is enabledChecklist
yarn lintyarn check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.