#130 - Implement fallback image #138

claytonc · 2017-09-14T19:26:02Z

closes #130

hvelarde

the changelog entry is missing also; we will need to wait until #136 is merged.

hvelarde · 2017-09-14T23:52:23Z

sc/social/like/browser/imagefallback.py

+        self.filename = None
+        self.data = None
+
+        registry = getUtility(IRegistry)


use the API instead: api.portal.get_registry_record() with None as default value.

hvelarde · 2017-09-14T23:53:14Z

sc/social/like/browser/imagefallback.py

+
+    def __init__(self, context, request):
+        super(ImageFallBack, self).__init__(context, request)
+        self.filename = None


you can initialize both in one line: self.filename = self.data = None

how about:

class ImageFallBack(Download): filename = None data = None def __call__(self): self.setup() super(ImageFallBack, self).__call__() def setup(self): # add rest of init method here

hvelarde · 2017-09-14T23:54:50Z

sc/social/like/interfaces.py

+
+    model.fieldset(
+        'open_graph',
+        label=u'Open Graph',


this needs to be internationalized.

hvelarde · 2017-09-14T23:55:44Z

sc/social/like/interfaces.py

+        label=u'Open Graph',
+        fields=[
+            'image_fallback',
+            'image_scale'


I would prefer to put the whole list in one line, but you may add a trailing comma if you like this way.

hvelarde · 2017-09-14T23:56:15Z

sc/social/like/interfaces.py

+    )
+
+    form.widget('image_fallback', NamedImageFieldWidget)
+    image_fallback = schema.ASCII(


let's rename the field to fallback_image.

hvelarde · 2017-09-15T00:07:07Z

sc/social/like/browser/viewlets.py

@@ -79,6 +80,7 @@ def update(self):
        self.site_name = portal.Title()
        self.language = facebook_language(get_language(self.context), 'en_US')
        self.image = get_content_image(self.context)
+        self.image_fallback = get_image_fallback(portal)


we only need to get the fallback image if the content has no lead image; you have to load this later and only if needed.

hvelarde · 2017-09-15T00:10:10Z

sc/social/like/browser/configure.zcml

@@ -77,4 +77,11 @@
        permission="cmf.ManagePortal"
        />

+    <browser:page
+       for="*"


register this view for site root only as the fallback image is the same all over the place; also, add a browser layer here.

hvelarde · 2017-09-15T00:11:22Z

sc/social/like/browser/viewlets.py

@@ -110,7 +112,7 @@ def image_url(self):
        if img:
            return img.url
        else:
-            return self.portal_url() + '/logo.png'
+            return self.image_fallback


use self.portal_url() + get_image_fallback() instead.

hvelarde · 2017-09-15T00:16:34Z

sc/social/like/utils.py

+
+    if getattr(settings, 'image_fallback', False):
+        filename, data = b64decode_file(settings.image_fallback)
+        return '{0}/@@sociallike-image-fallback/{1}'.format(


let's just return the path relative to the site root; also, don't use format() in this case as is pretty slow; a simple concatenation will make the work faster: return '/@@sociallike-image-fallback/' + filename

@hvelarde you forgot about the site_url variable in your example

no, I want to remove that call from here.

hvelarde · 2017-09-15T00:18:40Z

sc/social/like/utils.py

+#
+#     filename, data = b64decode_file(value)
+#     image = NamedBlobImage(data=data, filename=filename)
+#     msg = validate_image_social(image)


the validator no longer returns the message: it will rise ValueError with the error message; you may intercept and raise Invalid(e.message) instead.

I did not finish waiting for # 136

rodfersou · 2017-09-15T01:12:39Z

sc/social/like/interfaces.py

+        required=True,
+        default=u'large',
+        vocabulary='sc.social.likes.ImagesScales',
+    )


this field looks like it don't belongs to this issue, should be addressed to the right issue and pull request

Yes, this field was requested in redmine.

rodfersou · 2017-09-15T01:14:57Z

sc/social/like/upgrades/v3047/configure.zcml

+        title="Adds new field to the control panel."
+        description="This fields will be used to generate 'og:image' when content not image."
+        handler=".update_registry"
+        />


you can do this without a handler, take a look at this PR https://github.com/collective/sc.social.like/pull/136/files#diff-8101f2cdf8c9e74b0408e13d6e1eed44R10

rodfersou · 2017-09-15T01:22:12Z

sc/social/like/vocabularies.py

+                mapping={'width': str(width), 'height': str(height)})
+            terms.append(SimpleTerm(scale, scale, translated))
+
+    return SimpleVocabulary(terms)


how about to filter the result of the default vocabulary?
https://github.com/plone/plone.app.vocabularies/blob/2.1.x/plone/app/vocabularies/images.py#L13

I do not think so.

ScalesVocabulary(context)._terms[0].title
u'imagescale_mini'

ScalesVocabulary(context)._terms[0].value
'mini'

ScalesVocabulary(context)._terms[0].token
'mini'

why not? that way we can skip the i18n.

rodfersou

Waiting the PR be ready, and all changes described to read again

hvelarde · 2017-09-15T20:40:20Z

sc/social/like/browser/imagefallback.py

+        super(ImageFallBack, self).__call__()
+
+    def setup(self):
+        fallback_image = api.portal.get_registry_record('fallback_image', interface=ISocialLikeSettings)


you forgot the default value; without it it will fail if the upgrade step has not been run.

hvelarde · 2017-09-15T20:41:07Z

sc/social/like/utils.py

+def get_fallback_image():
+    from sc.social.like.interfaces import ISocialLikeSettings
+
+    fallback_image = api.portal.get_registry_record('fallback_image', interface=ISocialLikeSettings)


you're missing default value here also.

hvelarde · 2017-09-18T14:51:16Z

@claytonc please check the rebase; I see a mixture of things that are already in the master branch here.

hvelarde

We are still missing documentation on the README file and an entry on the changelog.

hvelarde · 2017-09-18T17:19:42Z

sc/social/like/browser/configure.zcml

+    <browser:page
+       name="sociallike-fallback-image"
+       for="Products.CMFPlone.interfaces.IPloneSiteRoot"
+       class=".imagefallback.ImageFallBack"


.fallbackimage.FallBackImage

hvelarde · 2017-09-18T17:20:25Z

sc/social/like/browser/imagefallback.py

+
+
+class ImageFallBack(Download):
+


please add a docstring expaining the purpose of this helper view.

hvelarde · 2017-09-18T17:22:14Z

sc/social/like/interfaces.py

+
+    form.widget('fallback_image', NamedImageFieldWidget)
+    fallback_image = schema.ASCII(
+        title=_(u'Image fallback'),


Fallback image

hvelarde · 2017-09-18T17:22:41Z

sc/social/like/interfaces.py

+        title=_(u'Image fallback'),
+        description=_(
+            u'help_fallback_image',
+            default=u'Content that does not have an image will be displayed on the share.\n '


this messages makes no sense; please review.

hvelarde · 2017-09-18T17:23:22Z

sc/social/like/upgrades/v3047/configure.zcml

@@ -3,8 +3,8 @@
    xmlns:genericsetup="http://namespaces.zope.org/genericsetup">

  <genericsetup:upgradeSteps
-      source="3046"
-      destination="3047"
+      source="3047"


this is wrong; you need to register a new upgrade step.

… now

[skip ci]

hvelarde

LGTM

This will be done before the release to make easier the code review.

rodfersou · 2017-09-28T00:32:55Z

sc/social/like/utils.py

@@ -198,3 +200,30 @@ def validate_og_lead_image(image):
        raise ValueError(MSG_INVALID_OG_LEAD_IMAGE_ASPECT_RATIO)

    return True
+
+
+def validate_og_fallback_image(value):


please add a TODO comment to move code to share this logic with validate_og_lead_image method

I was thinking about it, but it's not so easy as the code only looks similar.

rodfersou · 2017-09-28T00:37:42Z

sc/social/like/browser/viewlets.py

+    def get_fallback_image(self):
+        fallback_image = self.settings.fallback_image
+        if fallback_image is not None:
+            filename, data = b64decode_file(fallback_image)


filename, _ = b64decode_file(fallback_image)

rodfersou

LGTM, just added some comments

idgserpro · 2017-09-28T13:48:40Z

Sorry we didn't review it, since you're all adding lots of comments we we're waiting for all your suggestions to be merged before doing a full review.

hvelarde · 2017-09-28T14:25:25Z

@idgserpro you can review it now ;)

hvelarde requested review from hvelarde, rodfersou and idgserpro September 14, 2017 23:46

hvelarde requested changes Sep 15, 2017

View reviewed changes

rodfersou reviewed Sep 15, 2017

View reviewed changes

rodfersou requested changes Sep 15, 2017

View reviewed changes

hvelarde reviewed Sep 15, 2017

View reviewed changes

hvelarde force-pushed the issue_130 branch from 0f4ad5f to 1277685 Compare September 18, 2017 17:09

hvelarde requested changes Sep 18, 2017

View reviewed changes

p_991010 and others added 17 commits September 27, 2017 16:42

#130 - Implement fallback image

204af2b

#130 - fix travis code analysis

7b416d9

#130 - Changed as suggested # pullrequestreview-62926659

6e0368e

#130 - Changed as suggested #discussion_r139045269

65af89c

#130 - Changed as suggested #discussion_r139037985

4f13216

#130 - rename field, use APi,

3ca2c17

#130 - rebase

e79a1c9

#130 - fix rebase

6de329e

- remove upgrade steps.

2e016bc

- add validade image fallback

f0ada70

- set default value

3232272

- rename file and class

dda3981

- fix upgarde step

5fc9706

- add upgrade step

5b6cc83

- altered messagens

20be0c9

- remove i18n

3b56c0c

Fix fallback image view and add tests

70c4af3

hvelarde added 5 commits September 27, 2017 16:42

Remove the image scale feature as it doesn't seem to be justified for…

dd2c366

… now

Refactor

e43bb60

Add tests for upgrade step

688b1f0

Update changelog

48e780f

Refactor

417c795

hvelarde force-pushed the issue_130 branch from 4b5e9a8 to 09440e8 Compare September 27, 2017 19:43

Add tests for form field validator

3e38fa3

hvelarde force-pushed the issue_130 branch 2 times, most recently from c5e88a9 to b20bdee Compare September 27, 2017 21:47

Fix package documentation

88d6d9a

[skip ci]

hvelarde force-pushed the issue_130 branch from b20bdee to ceaf027 Compare September 27, 2017 21:48

hvelarde approved these changes Sep 27, 2017

View reviewed changes

Revert i18n update

9599421

This will be done before the release to make easier the code review.

hvelarde force-pushed the issue_130 branch from ceaf027 to 9599421 Compare September 27, 2017 21:55

rodfersou reviewed Sep 28, 2017

View reviewed changes

rodfersou approved these changes Sep 28, 2017

View reviewed changes

Rename unused variable

dd69860

hvelarde merged commit 2e67159 into master Sep 28, 2017

hvelarde deleted the issue_130 branch September 28, 2017 02:54

#130 - Implement fallback image #138

#130 - Implement fallback image #138

Conversation

claytonc commented Sep 14, 2017 • edited by hvelarde

hvelarde left a comment • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claytonc Sep 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodfersou 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

hvelarde commented Sep 18, 2017

hvelarde left a comment • edited

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

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

rodfersou left a comment

Choose a reason for hiding this comment

idgserpro commented Sep 28, 2017

hvelarde commented Sep 28, 2017

claytonc commented Sep 14, 2017 •

edited by hvelarde

hvelarde left a comment •

edited

claytonc Sep 15, 2017 •

edited

hvelarde left a comment •

edited