Move metadata to the viewlet template #124

rodfersou · 2017-08-24T04:49:30Z

refs. #112

hvelarde · 2017-08-24T12:54:56Z

sc/social/like/browser/viewlets.py

@@ -61,7 +67,16 @@ class SocialMetadataViewlet(BaseLikeViewlet):
    """Viewlet used to insert metadata into page header
    """
    render = ViewPageTemplateFile('templates/metadata.pt')
-    render_method = 'metadata'
+
+    def __init__(self, context, request, view, manager):


once again, don't add any code into the __init__() method or we can get unexpected errors.

hvelarde · 2017-08-24T12:55:37Z

sc/social/like/browser/viewlets.py

+    def via(self):
+        record = ISocialLikeSettings.__identifier__ + '.twitter_username'
+        try:
+            return api.portal.get_registry_record(record)


api.portal.get_registry_record() now supports a default value, use it instead.

hvelarde · 2017-08-24T12:57:16Z

sc/social/like/browser/viewlets.py

+        self.description = context.Description()
+        self.portal_state = getMultiAdapter((self.context, self.request),
+                                            name=u'plone_portal_state')
+        self.site_url = self.portal_state.portal_url()


use the API: self.site_url = api.portal.get().absolute_url()

hvelarde · 2017-08-24T12:57:51Z

sc/social/like/browser/templates/metadata.pt

+<meta property="og:locale" tal:attributes="content view/language" />
+<meta property="og:title" tal:attributes="content view/title" />
+<meta property="og:description" tal:attributes="content view/description" />
+<meta property="og:image" tal:attributes="content view/image_url" />


og:image must include metadata about the image itself.

ok, just for curiosity.. why? this is not a requirement to make it work AFAIK

that's a best practice and the way it works right now.

hvelarde · 2017-08-24T12:58:53Z

sc/social/like/browser/viewlets.py

+        if img:
+            return img.url
+        else:
+            return '{0}/logo.png'.format(self.site_url)


use string concatenation as it's faster than format().

that's an unexpected information, thank you for sharing.

anyway, do you plan to build a rocket? https://hackernoon.com/yes-python-is-slow-and-i-dont-care-13763980b5a1

😄

I'm not building a rocket but... which line is easier to read and understand (plus 3 times faster) in this case?

return '{0}/logo.png'.format(self.site_url)

or:

return self.site_url + '/logo.png'

the answer, again, is obvious to me.

hvelarde · 2017-08-24T13:01:19Z

sc/social/like/plugins/facebook/templates/plugin.pt

-                         data-href view/canonical_url;"></div>
-</tal:fb>
+<!-- Facebook -->
+<script type="application/javascript" tal:content="view/fbjs" />


don't use the type attribute for JavaScript.

hvelarde · 2017-08-24T13:02:03Z

sc/social/like/plugins/gplus/templates/plugin.pt

+<!-- Google+ -->
+<script type="application/javascript">


don't use the type attribute for JavaScript.

hvelarde · 2017-08-24T13:02:37Z

sc/social/like/plugins/linkedin/templates/plugin.pt

@@ -1,6 +1,17 @@
 <tal:linkedin>
    <!-- Linkedin -->
+    <script type="application/javascript">


don't use the type attribute for JavaScript.

hvelarde · 2017-08-24T13:04:08Z

overall work is looking good; this change will leave the code way faster an easier to maintain.

FYI @claytonc

rodfersou · 2017-08-24T19:20:30Z

sc/social/like/browser/viewlets.py

+            (self.context, self.request), name=u'plone_portal_state')
+        self.site_url = portal_state.portal_url()
+        self.url = self.context.absolute_url()
+        self.language = facebook_language(get_language(self.context), self.language)


I forgot this attribute (what gave an exception) adding in this commit.

rodfersou · 2017-08-24T19:33:46Z

sc/social/like/utils.py

-    return img
+    if view is None:
+        return
+    # XXX: If I recall correctly, Dexterity works different to do this


Yeah, that's right: https://github.com/collective/collective.nitf/blob/master/src/collective/nitf/browser/scaling.py#L55-L58

Looking with more attention, we are using the view.getImageSize method, not image.getImageSize method, so it is probably "ok" like this.

rodfersou · 2017-08-25T13:36:27Z

sc/social/like/tests/test_plugin_facebook.py

-        og_type = view.type()
-        self.assertIn('website', og_type)
-
-    def test_plugin_view_document(self):


Tests moved at viewlet, when test metadata.

rodfersou · 2017-08-25T13:37:49Z

sc/social/like/tests/test_plugin_facebook.py

-        og_type = view.type()
-        self.assertIn('website', og_type)
-
-    def test_plugin_view_image(self):


These tests are not needed anymore since we are using scale large by default for all plugins.

rodfersou · 2017-08-25T13:37:57Z

sc/social/like/tests/test_plugin_facebook.py

-        self.assertEqual(view.image_width(), 1200)
-        self.assertEqual(view.image_height(), 675)
-
-    def test_plugin_view_image_large(self):


These tests are not needed anymore since we are using scale large by default for all plugins.

rodfersou · 2017-08-25T13:38:05Z

sc/social/like/tests/test_plugin_facebook.py

-        self.assertEqual(view.image_width(), 1200)
-        self.assertEqual(view.image_height(), 675)
-
-    def test_plugin_view_newsitem(self):


These tests are not needed anymore since we are using scale large by default for all plugins.

rodfersou · 2017-08-25T13:38:11Z

sc/social/like/tests/test_plugin_facebook.py

-        self.assertEqual(view.image_width(), 1024)
-        self.assertEqual(view.image_height(), 768)
-
-    def test_plugin_view_newsitem_large(self):


These tests are not needed anymore since we are using scale large by default for all plugins.

rodfersou · 2017-08-25T13:38:35Z

sc/social/like/tests/test_plugin_facebook.py


-    # FIXME: we need to rethink this feature
-    @unittest.skipIf(IS_PLONE_5, 'Metadata viewlet is disabled in Plone 5')


Now metadata is always shown

rodfersou · 2017-08-25T13:39:16Z

sc/social/like/tests/test_plugin_gplus.py

@@ -92,13 +92,13 @@ def test_privacy_plugin_view_html(self):
        html = view.link()
        self.assertIn('Share on Google+', html)

-    def test_plugin_view_metadata(self):


Keep testing javascript existence, but in plugin template now.

rodfersou · 2017-08-25T13:39:40Z

sc/social/like/tests/test_plugin_facebook.py


-    # FIXME: we need to rethink this feature
-    @unittest.skipIf(IS_PLONE_5, 'Metadata viewlet is disabled in Plone 5')
    def test_plugin_language(self):


Language (and script) moved into plugin template

rodfersou · 2017-08-25T13:40:01Z

sc/social/like/tests/test_plugin_twitter.py

@@ -68,27 +67,6 @@ def setUp(self):
        self.plugins = dict(getUtilitiesFor(IPlugin))
        self.plugin = self.plugins[name]

-    # FIXME: we need to rethink this feature
-    @unittest.skipIf(IS_PLONE_5, 'Metadata viewlet is disabled in Plone 5')
-    def test_plugin_view_metadata(self):


Test moved into viewlet test

hvelarde · 2017-08-25T14:02:28Z

sc/social/like/tests/test_viewlets.py

+                title='My document',
+                description='Lorem Ipsum',
+            )
+        set_image_field(self.document, load_image(1024, 768), 'image/png')


Document content type doesn't have image field.

rodfersou · 2017-08-25T14:15:57Z

@hvelarde green 🍻

hvelarde

Please rebase against master after #126 is merged.

hvelarde · 2017-08-25T14:34:50Z

sc/social/like/plugins/facebook/templates/metadata.pt

-  <meta property="og:image:type" tal:attributes="content view/image_type" />
-  </tal:image>
-  <meta tal:condition="view/admins" property="fb:admins" tal:attributes="content view/admins" />
-  <meta tal:condition="view/app_id" property="fb:app_id" tal:attributes="content view/app_id" />


you're completely removing fb:admins and fb:app_id properties from the code; this needs to be fixed.

please add those to the test so this doesn't happen anymore without notice.

Okay, I misunderstand the issue.. I'll add back now, thank you.

hvelarde · 2017-08-25T14:37:11Z

sc/social/like/plugins/facebook/templates/metadata.pt

-  Plone 5 already includes metadata for Facebook and Twitter.
-</tal:comment>
-<tal:fb condition="not:view/is_plone_5">
-  <meta property="og:site_name" tal:attributes="content view/portal_title" />


you're completely removing og:site_name property from the code; this needs to be fixed.

please add those to the test so this doesn't happen anymore without notice.

Okay, I misunderstand the issue.. I'll add back now, thank you.

hvelarde · 2017-08-25T14:37:57Z

sc/social/like/browser/viewlets.py

+    def update(self):
+        self.setup()
+
+    def setup(self):


rename setup() to update().

hvelarde · 2017-08-25T17:32:32Z

sc/social/like/browser/viewlets.py

@@ -74,6 +94,66 @@ def enabled(self):
            return True
        return self.helper.enabled(self.view)

+    @property
+    def via(self):
+        record = dict(


use here the following pattern instead for better readability:

record = ISocialLikeSettings.__identifier__ + '.twitter_username' return api.portal.get_registry_record(record, default='')

The plugins() method inherited from BaseLikeViewlet takes too long and is not necessary here.

idgserpro

We can't give a full review to all modifications because we're not familiar with the whole integration with all these external services (Twitter, Facebook and such) and what your changes may implicate, so we reviewed from a Plone/Python perspective, ok?

idgserpro · 2017-08-29T19:14:27Z

sc/social/like/browser/viewlets.py

@@ -57,15 +65,33 @@ def update(self):
        return


-class SocialMetadataViewlet(BaseLikeViewlet):
-    """Viewlet used to insert metadata into page header
+class SocialMetadataViewlet(ViewletBase):


Shouldn't you still inherit BaseLikeViewlet before removing it completely if your plan is backwards compatibility? If you're no inheriting it, what's the motivation of keeping it above and adding a TODO to remove instead of removing it already?

it's more complicated: in fact we broke the plugin feature in release 2.5 after migrating everything to plone.app.registry. I was thinking about that yesterday when I was reviewing the work done. We didn't want to change too many things in this PR but it's inevitable because the code is way too complex.

if we really want to maintain the plugin extensibility we need to redesign it from scratch to use only one utility and one view per plugin, and using the extension mechanism already present in z3c.form. unfortunately, we have not enough heads and hands to do so in the way we may want.

we'll need to rework the whole PR.

idgserpro · 2017-08-29T19:14:31Z

sc/social/like/browser/viewlets.py

+    index = ViewPageTemplateFile('templates/metadata.pt')
+
+    def update(self):
+        registry = getUtility(IRegistry)


Any reason for not using plone.api directly since it's already a dependency of sc.social.like

sc.social.like/setup.py

Line 45 in 40cfc4c

'plone.api',

in app_id, admins and twitter_site?

because plone.api sucks at handling the registry as I would have to add one call to get_registry_record() for every field and that leads to more lines of code at the end.

we need to implement something like api.registry.get_record() and api.registry.get_proxy().

idgserpro · 2017-08-29T19:14:46Z

sc/social/like/tests/test_viewlets.py

 import re
 import unittest


+# set the "SKIP_CODE_PROFILING" environent variable to skip profiling


Where does SKIP_CODE_PROFILING comes from? Is it from .travis.yml? If so, a small doc here would be a nice addition.

I just added that for running test locally as the profiling pollutes the test results.

rodfersou · 2017-08-31T01:13:22Z

sc/social/like/browser/templates/metadata.pt

+<meta property="og:title" tal:attributes="content view/title" />
+<meta property="og:description" tal:attributes="content view/description" />
+<meta property="og:image" tal:attributes="content view/image_url" />
+<tal:image tal:condition="nocall:view/image">


no need to write tal:condition inside a <tal: tag, you can write just condition instead.

hvelarde · 2017-09-13T03:01:05Z

superseded by #135

rodfersou force-pushed the unify-metadata branch 2 times, most recently from 494c868 to 1f4a3c2 Compare August 24, 2017 04:50

rodfersou changed the title ~~Move metadata to the view template~~ Move metadata to the viewlet template Aug 24, 2017

rodfersou force-pushed the unify-metadata branch 2 times, most recently from 0e922f9 to 67be257 Compare August 24, 2017 04:53

hvelarde reviewed Aug 24, 2017

View reviewed changes

rodfersou commented Aug 24, 2017

View reviewed changes

rodfersou force-pushed the unify-metadata branch 4 times, most recently from 8a145c2 to c51aa31 Compare August 25, 2017 13:25

rodfersou commented Aug 25, 2017

View reviewed changes

rodfersou force-pushed the unify-metadata branch from 05c141d to d3cd2a4 Compare August 25, 2017 13:43

hvelarde reviewed Aug 25, 2017

View reviewed changes

rodfersou force-pushed the unify-metadata branch from d3cd2a4 to a14c6c7 Compare August 25, 2017 14:04

rodfersou added the enhancement label Aug 25, 2017

rodfersou self-assigned this Aug 25, 2017

hvelarde requested changes Aug 25, 2017

View reviewed changes

rodfersou added 5 commits August 25, 2017 16:06

Move metadata to the viewlet template

9b65d09

Add back image metadata

f9c99f5

Add setup method at metadata viewlet

00585bb

Viewlet refactor

618a6cf

Remove type attribute for tag script

5207d5c

rodfersou force-pushed the unify-metadata branch from a14c6c7 to aa5d8ff Compare August 25, 2017 19:13

rodfersou added 4 commits August 25, 2017 16:19

Fix tests

b2002d3

Update changelog

aa5d8ff

Code review

a787531

Fix tests and remove some unused attributes

7c9cb45

hvelarde requested a review from idgserpro August 25, 2017 20:49

hvelarde added 4 commits August 25, 2017 18:07

Fix performance issue

80819e7

The plugins() method inherited from BaseLikeViewlet takes too long and is not necessary here.

Allow skipping code profiling

f0e9a5a

Code optimization and clean up

8411b85

Fix changelog

dec0455

hvelarde approved these changes Aug 26, 2017

View reviewed changes

hvelarde requested a review from claytonc August 26, 2017 01:44

idgserpro reviewed Aug 29, 2017

View reviewed changes

rodfersou commented Aug 31, 2017

View reviewed changes

hvelarde closed this Sep 13, 2017

hvelarde deleted the unify-metadata branch September 13, 2017 03:01


		# FIXME: we need to rethink this feature
		@unittest.skipIf(IS_PLONE_5, 'Metadata viewlet is disabled in Plone 5')

Move metadata to the viewlet template #124

Move metadata to the viewlet template #124

Conversation

rodfersou commented Aug 24, 2017 • edited by hvelarde Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvelarde Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvelarde Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvelarde commented Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodfersou commented Aug 25, 2017

hvelarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idgserpro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvelarde Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvelarde Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvelarde commented Sep 13, 2017

rodfersou commented Aug 24, 2017 •

edited by hvelarde

Loading

hvelarde Aug 24, 2017 •

edited

Loading

hvelarde Aug 24, 2017 •

edited

Loading

hvelarde commented Aug 24, 2017 •

edited

Loading

hvelarde Aug 29, 2017 •

edited

Loading

hvelarde Aug 29, 2017 •

edited

Loading