Enhance Thumbnail Functionality #304

studybuffalo · 2020-01-17T23:31:19Z

This is a pull request to address the dependency issues caused by conflicting versions of django-newsletter, sorl-thumbnail, and Django. It removes the hard dependency on sorl-thumbnail and provides alternative fallbacks if sorl-thumbnail is not used. With this work it was easy enough to extend functionality to include other thumbnail applications. Thus, I have also included easy-thumbnails support to give users another option and serve as a template for any future support for other thumbnail applications.

Major Changes

Removed sorl-thumbnail as a dependency.
Added a new setting (NEWSLETTER_THUMBNAIL) that allows user to specify a third party thumbnail application for use with django-newsletter.
Added support for two thumbnail applications to start: sorl-thumbnail and easy-thumbnails
Created a template tag that performs rudimentary thumbnailing with Pillow. This acts as a fallback if the user hasn't specified a third party thumbnail application. This has been tested to support common image file formats: JPG, PNG, GIF, BMP, TIFF.
Reworked the image field in the Article model to support swapping between thumbnail applications by creating a model field that will inherit from the appropriate application. This avoids issues where Django will try to make a new migration anytime the user swaps the app. By default, this field will fallback to the Django ImageField if there is no thumbnail app specified.
Added logic to the SubmissionArchiveDetailView to select an appropriate template to thumbnail any Article images. Users can override the newsletter/message/thumbnail/django_newletter.html or newsletter/message/message.html template if they need to implement a different template.
Added logic to the admin ArticleInline class to make use of the appropriate thumbnail application widget as specified by the user (or the normal Django widet if not specified).
Added unit tests for all new functionality
Updated relevant documentation sections to outline the new functionality and modifications to the install steps.
Removed testing steps from travis.yml that stops installing directly from requirements.txt. This forces the tests to install as though it were a proper PyPI package and catch any odd requirement issues.

Other Comments

the upgrading.rst file has not been updated yet, as I am not sure which version this would fall under and if there are major changes to still take place.

Questions before review

I am not sure if we want to go ahead with the basic thumbnail functionality via Pillow as a fallback or not. I added it since the work was already done and it was minimal effort at this point. That said, I understand if we don't want this application to dive into something that could become quite complex. I think this works for the common uses, but we can't guarantee that for anything unusual. If we opt not to use it, I think the fallback becomes to just display the image as is and resize it via HTML attributes (i.e. no actual performance benefits, just making sure it visually looks the same as the other thumbnail applications).
I opted to create a more rigid implementation of specifying the thumbnail application. We could go with a situation where the user can specify things down to the specific model field they want to use, the admin mixin, the templates, etc. I felt that it wasn't functionality most users were going to use and we should encourage developers to add their thumbnail application of choice as an option within django-newsletter.

Relevant Issues: #302, #303

…ed in template

…o pass

Adding proper Python specifications for Pillow

coveralls · 2020-01-17T23:35:27Z

Coverage increased (+0.4%) to 88.81% when pulling 620ac61 on studybuffalo:enhance-thumbnail-functionality into 2147d3d on dokterbob:master.

dokterbob

Super thanks for a great contribg!

My suggestion would be to default to sorl for a while, as it is not 'broken' anymore, and this is what all users are using right now. However, we should yield a deprecation error when no thumbnailing is explicitly configured - and we might add a no-op thumbnailer for users which don't want to use any kind of thumbnailer.

Once more, GREAT WORK! It's a pleasure to review such a great piece of code. Sorry it took a while to get to it!

dokterbob · 2020-05-18T11:13:57Z

.travis.yml

@@ -39,8 +39,6 @@ install:
  # Latest PIP uses wheel by default
  - pip install --upgrade pip
  - pip install $DJANGO
-  - pip install -r requirements.txt
-  - pip install -r requirements_test.txt


Why are these removed?

I believe this was because I was noticing some odd behaviour in testing. I unfortunately cannot recall exactly what was happening, but I believe I was getting tests that passed locally with the setup.py test command, but then failing in Travis-CI. Removing these lines resolved the issue and all other tests continue to pass. My Travis-CI history unfortunately has since removed the offending tests from the history.

If I were to make an educated guess, I think we were getting weird version conflicts because we install these packages into the environment and then install things again with the setup.py test command.

If we need a definitive answer I can add the lines back and see what happens in testing!

I would prefer you take this file straight from master, where CI seems working just fine. :)

Updated with requested changes.

dokterbob · 2020-05-18T11:17:28Z

newsletter/admin.py

            models.TextField: {'widget': newsletter_settings.RICHTEXT_WIDGET},
        }

+    # https://easy-thumbnails.readthedocs.io/en/latest/usage/#forms
+    if newsletter_settings.THUMBNAIL == 'easy-thumbnails':
+        _formfield_overrides = {


This seems to override the _formfield_overrides when a RichtextWidget is set.

It's also not clear to me why a separate _formield_overrides is used instead of directly setting/updating formfield_overrides.

That is correct. Ultimately this is largely a personal stylistic choice so that we only have one spot where we set the attribute value, but we could achieve the same effect with if/elif/else statements and set the value directly.

I would prefer setting them directly as I feel non-functional code does not contribute to clarity.

Updated with requested changes.

dokterbob · 2020-05-18T11:21:06Z

newsletter/templatetags/newsletter_thumbnails.py

+    source_image = parser.compile_filter(tag_args[1])
+
+    # Return the thumbnail
+    return ThumbnailNode(source_image, tag_args[-1])


I do think providing (and thus maintaining!) thumbnail code is really out of scope from django-newsletter!

My suggestion would be:

For now, to default to sorl-thumbnail for backwards-compatibility but to yield a DeprecationError if no thumbnail facility is explicitly configured.

In the next major version, failing hard if none is defined. We might make sure there's a no-op thumbnail facility by default.

I am open to your suggestions on this one though!

Super happy with this great contrib!

I like that plan and I think it makes perfect sense. I have no doubts there are countless pitfalls with proper thumbnail generation we aren't accounting for here and it could create some concerning feature creep.

I'll look at making the suggested updates and remove this block (and updating the other impacted code sections).

Set this up to raise a DeprecationWarning if no NEWSLETTER_THUMBNAIL is declared and then default to sorl-thumbnail. I have set the warning to say this will become a mandatory declaration for version 0.11.0 (I assume the warning itself will kick in for version 0.10.0). I updated some details in the installation and setting docs to cover this change as well.

studybuffalo · 2020-05-18T20:48:42Z

Thanks for taking the time to review and provide the feedback @dokterbob - really appreciate it!

I will try and find some time to make the changes around the thumbnailer functionality in the near future and get another version out.

Adding depreciation warning for users who have not declared NEWSLETTER_THUMBNAIL

Correcting typos in comments

Adding tests for new functionality

Add tests for new functionality and to better test dynamic imports

studybuffalo · 2020-06-07T00:20:11Z

Okay, had some time and pulled out the thumbnail functionality. I left comments to some of your other questions and can make any additional changes as needed quite quickly. Thanks!

dokterbob · 2020-06-08T10:47:04Z

.travis.yml

@@ -39,8 +39,6 @@ install:
  # Latest PIP uses wheel by default
  - pip install --upgrade pip
  - pip install $DJANGO
-  - pip install -r requirements.txt
-  - pip install -r requirements_test.txt


I would prefer you take this file straight from master, where CI seems working just fine. :)

dokterbob · 2020-06-08T10:48:51Z

newsletter/admin.py

            models.TextField: {'widget': newsletter_settings.RICHTEXT_WIDGET},
        }

+    # https://easy-thumbnails.readthedocs.io/en/latest/usage/#forms
+    if newsletter_settings.THUMBNAIL == 'easy-thumbnails':
+        _formfield_overrides = {


I would prefer setting them directly as I feel non-functional code does not contribute to clarity.

…ependency. (#304) * Removed sorl-thumbnail as a dependency. * Added a new setting (NEWSLETTER_THUMBNAIL) which allows users to specify a third party thumbnail application. * Integrated support for two thumbnail applications: sorl-thumbnail and easy-thumbnails * Removing some Python 2.7 conditional imports

…ependency. (#304) * Removed sorl-thumbnail as a dependency. * Added a new setting (NEWSLETTER_THUMBNAIL) which allows users to specify a third party thumbnail application. * Integrated support for two thumbnail applications: sorl-thumbnail and easy-thumbnails * Removing conditional Python 2.7 and pytz (already required by Django) imports in tetst

dokterbob · 2020-10-24T18:28:20Z

Merged, at last! :D

Thanks, @studybuffalo!

Ref: 2ce29ba

frennkie · 2020-10-24T20:02:24Z

@dokterbob the migration that was merged with 2ce29ba is called 0006_switch_from_sorl_imagefield.py. The "attachment" migration already has the prefix 0006. I'm sure that is is no technical problem at all. But it might be a bit confusing to developers/users.

What do you think? Is it necessary to rename this one to 0007_switch_from_sorl_imagefield.py or should we just leave it?

dokterbob · 2020-10-27T08:23:19Z

Oh, that's a silly human bug right there. We might quickly renumber them, before anyone uses it (or else they'll have to manually rewind and re-apply). What do you think?

studybuffalo added 12 commits January 17, 2020 06:59

Adding support to dynamically configure Article image model field

6134540

Adding ability to dynamically setup which thumbnail application is us…

14d3703

…ed in template

Dynamic selection of thumbnail app widget for Article image field

8957bb9

Removing incorrect install steps that were allowing broken installs t…

a074c71

…o pass

Removing sorl-thumbnail

6205e65

Adding proper Python specifications for Pillow

Removing all code for dependency_links

308d6f8

Removing sorl-thumbnail

218b150

Adding tests for the DynamicImageField

df7cc42

Adding tests for the templatetags

2e6678a

Adding tests for the thumbnail template import

290c58d

Updating installation instructions for new thumbnail apps

c2b0e54

Updating details on new thumbnail app setting

f850637

studybuffalo mentioned this pull request Feb 4, 2020

Remove dependency on sorl-thumbnail #303

Closed

Merge branch 'master' into enhance-thumbnail-functionality

347f80d

dokterbob requested changes May 18, 2020

View reviewed changes

studybuffalo added 13 commits June 6, 2020 13:23

Removing thumbnailer template tag

3275ef6

Removing tests for template tags

15d2ca7

Adding back thumbnail application to avoid any errors

b981952

Updating docs to make a thumbnail application a mandatory setting

02029cb

Correcting comments (typos & clarity)

fbb9cc3

Removing default ImageField

258e999

Setting sorl-thumbnail as default thumbnail

39cf377

Adding depreciation warning for users who have not declared NEWSLETTER_THUMBNAIL

Removing default thumbnail template

17cc291

Removing tests for default ImageField

122b9fd

Adding tests for various thumbnail configurations

ebfdd68

Removing tests for default thumbnail template

a42fd92

Updating to match current master

303149d

Removing Python 2.7 conditional imports

b326ac3

Correcting typos in comments

studybuffalo and others added 8 commits June 6, 2020 16:58

Updating Pipfile to master

f94ad45

Adding tests for the thumbnail admin setup

f3fe310

Reworking tests to handle annd test missing imports better

fe1eb46

Handling thumbnail model field not specified

ebab8f8

Adding tests for new functionality

Adding tests for admin fields

1db1d4b

Updating required django version

23c8a15

Fixing version issues with tinymce

8cd8443

Merge pull request #1 from studybuffalo/add-admin-tests

a71ff2a

Add tests for new functionality and to better test dynamic imports

dokterbob self-requested a review June 8, 2020 10:45

dokterbob requested changes Jun 8, 2020

View reviewed changes

studybuffalo added 2 commits June 13, 2020 13:47

Returning install steps as per code review

e540e08

Setting admin formfield_overrides directly as per code review

620ac61

dokterbob closed this Oct 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Thumbnail Functionality #304

Enhance Thumbnail Functionality #304

studybuffalo commented Jan 17, 2020

coveralls commented Jan 17, 2020 •

edited

dokterbob left a comment

dokterbob May 18, 2020

studybuffalo May 18, 2020

dokterbob Jun 8, 2020

studybuffalo Jun 13, 2020

dokterbob May 18, 2020

studybuffalo May 18, 2020

dokterbob Jun 8, 2020

studybuffalo Jun 13, 2020

dokterbob May 18, 2020

studybuffalo May 18, 2020

studybuffalo Jun 7, 2020

studybuffalo commented May 18, 2020

studybuffalo commented Jun 7, 2020

dokterbob Jun 8, 2020

dokterbob Jun 8, 2020

dokterbob commented Oct 24, 2020

frennkie commented Oct 24, 2020

dokterbob commented Oct 27, 2020

Enhance Thumbnail Functionality #304

Enhance Thumbnail Functionality #304

Conversation

studybuffalo commented Jan 17, 2020

Major Changes

Other Comments

Questions before review

coveralls commented Jan 17, 2020 • edited

dokterbob 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

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

studybuffalo commented May 18, 2020

studybuffalo commented Jun 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dokterbob commented Oct 24, 2020

frennkie commented Oct 24, 2020

dokterbob commented Oct 27, 2020

coveralls commented Jan 17, 2020 •

edited