[ENG-4377] Google dataset discovery #1842
Conversation
There was a problem hiding this comment.
Jotting down some of the comments made during pairing. Several major concerns:
- We need to make sure we are including the json-ld data in the correct route. The scope of the ticket says we should include it for registrations.
- The logic to fetch the json-ld data is a bit convoluted. It should be done in less 10 lines, maybe less than 3 if we don't count formatting.
- Adding a
script-tagsservice solely for the purpose of setting<script>tags is redundant. We can either reuse themeta-tagsservice or use thehead-tagsservice directly.
chth0n1x
changed the title
Pull Request Test Coverage Report for Build 5189939723
💛 - Coveralls |
chth0n1x
force-pushed
the
feature/google-dataset-discovery
branch
3 times, most recently
from
064d9ff
to
cf61611
Compare
…ing, updated translation file.
…ing object return value.
There was a problem hiding this comment.
One subtle thing I see is that in overview/route.ts, we are setting
this.headTags = this.metaTags.getHeadTags(metaTagsData); in line 83,
and then pushing to this.headTags in line 96 and line 104. I've definitely run into issues before when adding/removing elements to an array and things that depend/read-from the array not updating properly. Since headTags isn't tracked or an EmberArray, I don't know if the tags pushed in line 96 and 104 are going to be included just by looking at this code, so I would double-check and make sure those are indeed visible in the <head>.
The previous implementation was adding all the tags to the variable const headTags and then doing this.set('headTags', headTags), so this may be the safest way of implementing the headTags to make sure things aren't lost.
Ok, that sounds good. The goal was to reduce the total number of lines but I agree. I can add a separate placeholder for the headTags value to push to before setting this.headTags. |
Purpose
The purpose of these changes is to return a Structured Data JSON-LD object to provide Google with a head element tag to be crawled and indexed by its bots. The aim of these changes is to improve the overall crawl rate and with the indexing and ranking of OSF resources.
Summary of Changes
-Added the returnStructuredData() method and the ScriptTagAttrs type to the Registrations Overview route to return the JSON-LD object from the
osf.io/<guid>/metadata/?format=google-dataset-json-ldendpoint.-Added ScriptTagAttrs to the exported interface and updated the Content type to include 'object' for the meta-tags service.
-Added error handling and messaging strings to the translations file when failing to return the object from the endpoint.
-Recommend: renaming the meta-tags service to the HeadTagsService to reference the script and link tags also included by the service.
Screenshot(s)
Side Effects
On different staging environments, the code will need to be re-tested to ensure the proper endpoint is being reached for the parent guid. It will fail if the flag is not set for the endpoint.
QA Notes
-Use DevTools to verify that the head element script tag displays the proper MIME type attribute as "application/ld+json" and has a valid JSON object as its content. e.g. <script type='application/ld+json' src='osf.io/...'>{jsonObject}<script/>.
-Is the script tag in its current implementation secure and efficient? Are there any attributes you would suggest adding to the tag to ensure further web application security?
<async>comes to mind and a more complete list for consideration may be found here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script.-Mobile checks may not be necessary, but adding the view-source: prefix to the URL as in view-source:https://osf.io// would be a good check.
-Does the JSON object pass both a formatting test and this Google test? Links for each: https://jsonlint.com/ and https://search.google.com/test/rich-results. If you have any questions, please ask!