ysk24ok / jekyll-linkpreview

Jekyll plugin to generate link preview
https://rubygems.org/gems/jekyll-linkpreview
MIT License
29 stars 8 forks source link

Use Non-OGP template if any of required OG tags are missing from the page #42

Closed Clpsplug closed 1 year ago

Clpsplug commented 1 year ago

Closes #37.

This patch modifies the behavior of create_properties_from_page so that if the page lacks any of the required OGP tags, it will use a Non-OGP template. This patch prevents users from getting a blank link preview when all the previewed page's meta tags are unrelated to OGP.

Disregard the part below, I found a way to test this patch :) There are currently no tests because I'm struggling to develop a way to test this behavior. Scraping is mocked away in the existing test suits, so adding a test for this patch to any of the existing test suits did not make much sense. get_properties includes fetching the page and extracting the properties, but should we split this method up just so that we can test this patch?

Clpsplug commented 1 year ago

Thanks @ysk24ok for #43 hot-fixing the CI! I'll look into this patch as to why the test is crashing now

Clpsplug commented 1 year ago

Caught the culprit: an example was missing a required OGP tag og:type. I added it to the test.
That being said, @ysk24ok, I want to discuss whether should og:type be accessible by the template and be cached or not. By that, I mean that in lib/jekyll-linkpreview.rb, I'd do something along the line of:

     class OpenGraphProperties
       @@template_file = 'linkpreview.html'

-      def initialize(title, url, image, description, domain)
+      def initialize(title, type, url, image, description, domain)
         @title = title
+        @type = type
         @url = url
         @image = image
         @description = description

...and include the @type in the hash. I cannot think of any use cases, though, as this value apparently can be anything. So this is just an idea that can be disregarded.

I also found a way to include the OGP/NOGP distinction in the Rake tests, so please take a look.

ysk24ok commented 1 year ago

I want to discuss whether should og:type be accessible by the template and be cached or not

Yes I think basically all properties should be accessible from a template and a cache file. Please add link_type to https://github.com/ysk24ok/jekyll-linkpreview#template-for-pages-where-open-graph-protocol-metadata-exists. You don't need to add the type variable to the default template.

Clpsplug commented 1 year ago

Added as requested! The user should now be able to extract the og:type property via link_type variable.

I also realized that existing users will not get the og:type variable until they bust their caches. The plugin doesn't appear to crash, but a small migration guide may help.