-
Notifications
You must be signed in to change notification settings - Fork 437
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
Fix issues with meta tags #1228
Fix issues with meta tags #1228
Conversation
This pull request introduces 1 alert when merging 64049fd into d253790 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 04b4f1c into d253790 - view on LGTM.com new alerts:
|
aab8bd3
to
34b117e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ybnd and @artlowel : Overall, this fixes the issues you've described, but I've noticed an odd caching issue when testing with this PR. It seems the <meta>
tags don't always refresh when you browse to a new page. Here's what I'm doing:
- Start on a Community page, click on a Collection, then on an Item. Select "Inspect" in your browser and look at the
<head>
section.- You'll see a
<meta property='title'>
with the Collection Name and a second with the Item name. - You'll see a
<meta property='citation_title
>` with the Collection Name and a second with the Item name. - All other
<meta>
tags for the Item will be there though, which is correct.
- You'll see a
- Click back to the Collection. Select "Inspect" in your browser and look at the
<head>
section.- The Item
<meta>
tags (from step 1) are still visible. They seem to be cached, as they obviously shouldn't appear on a Collection.
- The Item
- Click on a different Item in the same Collection. Select "Inspect" in your browser and look at the
<head>
section.- Now,
<meta>
tags are still odd...Some still appear from the first Item (from step 1) while others are correct for the Item you are on.
- Now,
Overall, the behavior I'm seeing almost seems like a caching issue...like these <meta>
tags are not being entirely "reset" (to empty) when you browse to a new page, and this results in very odd behavior. (NOTE This caching issue doesn't appear to be specific to this PR...as I can also sometimes get it to occur on demo7.dspace.org. So, if you'd rather treat this as a separate bug, we can do so. Besides this bug, the rest of the behavior of this PR seems correct.)
@tdonohue @artlowel After a bit of poking around on the current demo7.dspace.org I only ever saw the Looks like this |
@tdonohue and @ybnd I fixed the issue. The reason was not only the distinctUntilKeyChanged issue @ybnd mentioned, but the fact that the service responded both to the route changing, and calls for DSO page components initializing. With the second call often starting before the first had finished processing, which caused some tags that were just added to be immediately removed again, others added twice etc. I fixed it by removing the calls on DSO pages, and only listening to route changes. That's possible now (but wasn't when the metadataservice was originally written) because we're using resolvers on those pages, and they all put the resolved object in the I spent an additional 4 hours on this though, most of that not on the solution itself, but on the tests. They were basically integration tests in a unit test file. Didn't mock anything which caused them to break with the slightest change. I practically had to rewrite them from scratch, getting rid of anything that isn't strictly necessary for the test. |
Thanks @artlowel . I'll add 4 hours to the original ticket and re-test this as soon as possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@artlowel & @ybnd : I retested today. With @artlowel 's updates, the behavior seems improved, however, I'm still seeing the citation_author
field (and seemingly only that field) be cached between pages. Here's what I'm doing:
- Running
yarn start
- Bring up UI. Immediately do
inspect
and open up the<head>
section in a separate window - Now, in the UI, browse to an Item. I see the
<meta>
tags auto-refresh in my "inspect" window. - From that Item page, click on the Collection in breadcrumbs. Most
<meta>
tags auto-refresh, but thecitation_author
tags from Item still exist from the previous page. - Click on Community in Breadcrumbs. Again, most
<meta>
tags auto-refresh, but thecitation_author
tags still exist from Item.
Last time I tested, I saw this behavior across the <meta property="title">
and <meta property="citation_title">
tags...that bug seems fixed. But somehow the behavior still exists for citation_author
. Is there something in the UI that could be caching the author names themselves? It's odd that this specific field is now the issue.
Overall, again, I'm in favor of this PR. I'd just really like to find a way to improve SEO by ensuring we always have accurate <meta>
tags. If you both feel this final bug needs to be a separate ticket, we can split it out...but I'd still rate it as high priority for 7.0.
@tdonohue Should be fixed now 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging this immediately, as it was previously discussed as |
[DSC-1446] added default file thumbnail Approved-by: Stefano Maffei
References
<meta>
tags #1197Description
Implement Google Scholar review suggestions for meta tags
Instructions for Reviewers
The following meta tags were changed:
citation_pdf_url
: select Bitstreams ~ MIME type allowlist (if more than one option and no primary in ORIGINAL Bundle)citation_abstract_html_url
: usedc.identifier.uri
if available, fall back to current URLcitation_publisher
: usedc.publisher
(except dissertations & tech reports, those use other tags)citation_date
→citation_publication_date
og:title
andog:description
meta tagsTesting
Open some Item pages and inspect the
head > meta
in their HTMLcitation_abstract_html_url
should not contain links to localhost, but should contain links ~ the Item's handledc.identifier.uri
is filled out. Otherwise, it should match the Item page URLcitation_publisher
should be present ifdc.publisher
is filled outcitation_publication_date
should be presentcitation_date
,og:title
,og:description
For Items with one ORIGINAL Bitstream
citation_pdf_url
should contain a link to that BitstreamFor Items with more than one ORIGINAL Bitstream
citation_pdf_url
should link to itcitation_pdf_url
should contain a link to the first ORIGINAL Bitstream with one of these MIME typesChecklist
yarn run lint
package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.