collective / sc.social.like

Social: Like Actions is a Plone package (add-on) providing simple Google+, Twitter and Facebook integration for Plone Content Types.
7 stars 23 forks source link

Fix support for canonical URL in Facebook's Open Graph #110

Closed hvelarde closed 7 years ago

hvelarde commented 7 years ago

WIP

fixes #104 closes #108

hvelarde commented 7 years ago

this is almost ready but I want second opinions on the following:

idgserpro commented 7 years ago

add an upgrade step to automatically assign the Social Media behavior to all Dexterity-based content types already listed in the enabled_portal_types field of the configlet

You thought about assign_canonical_url. What about unassign_canonical_url, for removing the behavior when a content type is removed from enabled_portal_types field of the configlet?

hvelarde commented 7 years ago

@idgserpro I don't know, the code is already plenty of hacks and I don't want to keep adding more; the field makes no harm even if it's there for content not showing the widgets.

idgserpro commented 7 years ago

We can help from a code perspective, but we're not that familiar with this Facebook problem to see if this is the best approach, ok?

rodfersou commented 7 years ago

add an upgrade step to automatically assign the Social Media behavior to all Dexterity-based content types already listed in the enabled_portal_types field of the configlet

The important here is to don't change the behavior for the users that don't use it, and document how to enable for the users that need it.

calculate the canonical URL at publication time instead of at creation time; IMO, that could be probably more correct since is at publication time that the Facebook crawler will get the actual URL

Agree to calculate at publication time

tcurvelo commented 7 years ago

This PR seems fine to me. But how do you plan to merge stats from 2 canonicals urls from a same item? Eg.:

hvelarde commented 7 years ago

@tcurvelo Facebook is supposed to do so:

[Canonical URL] ensures that all actions such as likes and shares aggregate at the same URL rather than spreading across multiple versions of a page.

we'll discover that later in production.

hvelarde commented 7 years ago

@rodfersou @tcurvelo I got the following error while upgrading a site in production:

017-07-17T15:39:25 ERROR Zope.SiteErrorLog 1500316765.590.614936443294 http://localhost:8381/Plone/portal_setup/manage_doUpgrades
Traceback (innermost last):
  Module ZPublisher.Publish, line 138, in publish
  Module ZPublisher.mapply, line 77, in mapply
  Module ZPublisher.Publish, line 48, in call_object
  Module Products.GenericSetup.tool, line 1034, in manage_doUpgrades
  Module Products.GenericSetup.upgrade, line 166, in doStep
  Module sc.social.like.upgrades.v3045, line 13, in enable_social_media_behavior
  Module <decorator-gen-6>, line 2, in get_registry_record
  Module plone.api.validation, line 70, in wrapped
  Module plone.api.portal, line 278, in get_registry_record
  Module plone.registry.registry, line 78, in forInterface
KeyError: 'Interface `sc.social.like.interfaces.ISocialLikeSettings` defines a field `canonical_domain`, for which there is no record.'
hvelarde commented 7 years ago

@tcurvelo FYI, the share_count on HTTPS is now adding the value from HTTP:

http://graph.facebook.com/https://www.cartacapital.com.br/sociedade/pau-grande-mas-subjugado-vai-ter-preto-humilhado-sim

{
   "share": {
      "comment_count": 0,
      "share_count": 30496
   },
   "og_object": {
      "id": "1508557802517973",
      "description": "Do estere\u00f3tipo do \"bem dotado\" \u00e0 marginaliza\u00e7\u00e3o, o Brasil mostra ser o pa\u00eds do esc\u00e1rnio e da viol\u00eancia",
      "title": "Pau grande, mas subjugado: vai ter preto humilhado sim",
      "type": "article",
      "updated_time": "2017-07-08T20:23:26+0000"
   },
   "id": "https://www.cartacapital.com.br/sociedade/pau-grande-mas-subjugado-vai-ter-preto-humilhado-sim"
}

strangely enough Facebook's stupid widget still shows the wrong value:

selection_023

I hope that's a caching issue on their side; we'll have to wait to see if it changes.