Repository: saleor/saleor
Pull request: Api change image thumbnail arg 2445
I want to merge this change because...

closes #2445

### Screenshots

<!-- If your changes affect the UI, providing "before" and "after" screenshots will
greatly reduce the amount of work needed to review your work. -->

### Pull Request Checklist

<!-- Please keep this section. It will make maintainer's life easier. -->

1. [ ] Privileged views and APIs are guarded by proper permission checks.
1. [ ] All visible strings are translated with proper context.
1. [ ] All data-formatting is locale-aware (dates, numbers, and so on).
1. [ ] Database queries are optimized and the number of queries is constant.
1. [ ] The changes are tested.
1. [ ] The code is documented (docstrings, project documentation).


Problem statement:
Add image thumbnails to ProductImage type
API should be able to return image thumbnails in specified size - something like this:
```
{
    product {
    images{
      edges{
        node{
          thumbnail(x:256, y:256)
        }
      }
    }
  }
}
```

This could help us avoid situations where API consumer gets 10 `4000px x 7000px` images just to put them in `120px x 120px` containers.

Hints from issue discussion:
As we won't be able to provide an exact match for 99% of requests, I think this would make more sense:

```graphql
{
  node {
    thumbnail(width: 100, height: 100) {
      height
      width
      url
    }
  }
}
```

or maybe:

```graphql
{
  node {
    thumbnail(type: "product_gallery")
  }
}
```

Edit: In practice is it useful to specify height separately from width given that we're very unlikely to ever crop product images?
I probably didn't understand the feature scope - could You please explain what's the difference between the requested feature and existing possibility to specify the size of the image's url:
```
{
  products(first: 3) {
                edges {
                    node {
                        id
                        name
                        url
                        thumbnailUrl(size: "90x90")
                        images {
                            edges {
                                node {
                                    url(size: "120x120")
                                }
                            }
                        }
                    }
                }
  }
}
```

As @oldPadavan noticed, we already have this feature in our API. We should only change it to accept one of few predefined sizes. In `settings.py` we have `VERSATILEIMAGEFIELD_RENDITION_KEY_SETS` settings which defines what sizes are pre-generated for products, we could use that. The size argument should then be an enum.
I think `url(size: 120)` is easier (notice I've changed it to only accept one value) as otherwise the client has to know the mapping of names to sizes (which cannot be determined within the same query) and has to include a function mapping desired sizes to setting names. I think it's easy enough for the backend to do the matching (select equal size if exists, otherwise select closest larger size if not more than 2 times larger, otherwise select closest smaller size).
Does it mean it's necessary to restrict the sizes to those defined in `VERSATILEIMAGEFIELD_RENDITION_KEY_SETS`?
Yes, we do not want to allow API clients to create arbitrary sizes at will. A malicious user could easily generate terabytes worth of thumbnails by asking for all possible combinations of widths and heights.

Commit message: One more fix for cognitive complexity of get_thumbnail_size

Patch to review:
diff --git a/saleor/graphql/product/types.py b/saleor/graphql/product/types.py
index 745a50f4d9e..1b8b289d0c7 100644
--- a/saleor/graphql/product/types.py
+++ b/saleor/graphql/product/types.py
@@ -3,7 +3,8 @@
 from graphene_django.filter import DjangoFilterConnectionField
 
 from ...product import models
-from ...product.templatetags.product_images import product_first_image
+from ...product.templatetags.product_images import (
+    get_thumbnail, product_first_image)
 from ...product.utils import products_with_details
 from ...product.utils.availability import get_availability
 from ...product.utils.costs import (
@@ -130,8 +131,8 @@ class Product(CountableDjangoObjectType):
     thumbnail_url = graphene.String(
         description='The URL of a main thumbnail for a product.',
         size=graphene.Argument(
-            graphene.String,
-            description='Size of a thumbnail, for example 255x255.'))
+            graphene.Int,
+            description='Size of thumbnail'))
     availability = graphene.Field(
         ProductAvailability,
         description="""Informs about product's availability in the storefront,
@@ -154,7 +155,7 @@ class Meta:
 
     def resolve_thumbnail_url(self, info, *, size=None):
         if not size:
-            size = '255x255'
+            size = 255
         return product_first_image(self, size)
 
     def resolve_url(self, info):
@@ -264,8 +265,8 @@ class ProductImage(CountableDjangoObjectType):
     url = graphene.String(
         required=True,
         description='',
-        size=graphene.String(
-            description='Size of an image, for example 255x255.'))
+        size=graphene.Int(
+            description='Size of image'))
 
     class Meta:
         description = 'Represents a product image.'
@@ -275,5 +276,5 @@ class Meta:
 
     def resolve_url(self, info, *, size=None):
         if size:
-            return self.image.crop[size].url
+            return get_thumbnail(self.image, size, 'crop')
         return self.image.url
diff --git a/saleor/product/templatetags/product_images.py b/saleor/product/templatetags/product_images.py
index 3145c92549d..d0a62121d0a 100644
--- a/saleor/product/templatetags/product_images.py
+++ b/saleor/product/templatetags/product_images.py
@@ -45,26 +45,56 @@ def choose_placeholder(size=''):
     return placeholder
 
 
+def get_available_sizes_by_method(method):
+    sizes = []
+    for available_size in AVAILABLE_SIZES:
+        available_method, avail_size_str = available_size.split('__')
+        if available_method != method:
+            continue
+        sizes.append(min([int(s) for s in avail_size_str.split('x')]))
+    return sizes
+
+
+def get_thumbnail_size(size, method):
+    """ Return closest larger size if not more than 2 times larger, otherwise
+    return closest smaller size
+    """
+    avail_sizes = sorted(get_available_sizes_by_method(method))
+    larger = list(filter(lambda x: size < x <= size * 2, avail_sizes))
+    smaller = list(filter(lambda x: x <= size, avail_sizes))
+
+    if larger:
+        return '%sx%s' % (larger[0], larger[0])
+    elif smaller:
+        return'%sx%s' % (smaller[-1], smaller[-1])
+    return None
+
+
 @register.simple_tag()
 def get_thumbnail(instance, size, method='crop'):
-    size_name = '%s__%s' % (method, size)
+    size_str = '%sx%s' % (size, size)
+    size_name = '%s__%s' % (method, size_str)
     on_demand = settings.VERSATILEIMAGEFIELD_SETTINGS[
         'create_images_on_demand']
-    if instance:
-        if size_name not in AVAILABLE_SIZES and not on_demand:
-            msg = (
-                "Thumbnail size %s is not defined in settings "
-                "and it won't be generated automatically" % size_name)
-            warnings.warn(msg)
-        try:
-            thumbnail = getattr(instance, method)[size]
-        except Exception:
-            logger.exception(
-                'Thumbnail fetch failed',
-                extra={'instance': instance, 'size': size})
-        else:
-            return thumbnail.url
-    return static(choose_placeholder(size))
+    if not instance:
+        return static(choose_placeholder(size_str))
+    used_size = size_str
+    if size_name not in AVAILABLE_SIZES and not on_demand:
+        used_size = get_thumbnail_size(size, method)
+    if not used_size:
+        msg = (
+            "Thumbnail size %s is not defined in settings "
+            "and it won't be generated automatically" % size_name)
+        warnings.warn(msg)
+    try:
+        thumbnail = getattr(instance, method)[used_size]
+    except Exception:
+        logger.exception(
+            'Thumbnail fetch failed',
+            extra={'instance': instance, 'size': size})
+    else:
+        return thumbnail.url
+    return static(choose_placeholder(size_str))
 
 
 @register.simple_tag()
diff --git a/templates/cart_dropdown.html b/templates/cart_dropdown.html
index c966ea9dc73..3afb116e7a8 100644
--- a/templates/cart_dropdown.html
+++ b/templates/cart_dropdown.html
@@ -12,8 +12,8 @@
         <div class="col-md-10">
           <a class="link--clean" href="{{ line.variant_url }}">
             <img class="cart-dropdown__image lazyload lazypreload"
-                 data-src="{% get_thumbnail line.image size="60x60" %}"
-                 data-srcset="{% get_thumbnail line.image size="60x60" %} 1x, {% get_thumbnail line.image size="120x120" %} 2x"
+                 data-src="{% get_thumbnail line.image size=60 %}"
+                 data-srcset="{% get_thumbnail line.image size=60 %} 1x, {% get_thumbnail line.image size=120 %} 2x"
                  alt=""
                  src="{% placeholder size=120 %}">
             <h3>
diff --git a/templates/checkout/index.html b/templates/checkout/index.html
index ea2ee06b86d..bd42c32bdb7 100644
--- a/templates/checkout/index.html
+++ b/templates/checkout/index.html
@@ -42,8 +42,8 @@
           <div class="row">
             <div class="col-7 cart__line__product">
               <a class="link--clean" href="{{ line.variant.get_absolute_url }}">
-                <img class="lazyload lazypreload" data-src="{% get_thumbnail line.variant.product.images.first.image method="crop" size="60x60" %}"
-                     data-srcset="{% get_thumbnail line.variant.product.images.first.image method="crop" size="60x60" %} 1x, {% get_thumbnail line.variant.product.images.first.image method="crop" size="120x120" %} 2x"
+                <img class="lazyload lazypreload" data-src="{% get_thumbnail line.variant.product.images.first.image method="crop" size=60 %}"
+                     data-srcset="{% get_thumbnail line.variant.product.images.first.image method="crop" size=60 %} 1x, {% get_thumbnail line.variant.product.images.first.image method="crop" size=120 %} 2x"
                      alt="">
                 <p>{{ line.variant.product }}<br><small>{{ line.variant }}</small></p>
               </a>
diff --git a/templates/dashboard/product/list.html b/templates/dashboard/product/list.html
index 5e22236b33d..3a68890196f 100644
--- a/templates/dashboard/product/list.html
+++ b/templates/dashboard/product/list.html
@@ -73,8 +73,8 @@
                 <label for="id_products_{{ product.pk }}"></label>
               </td>
               <td class="avatar">
-                <img src="{% product_first_image product size="60x60" method="crop" %}"
-                    srcset="{% product_first_image product size="60x60" method="crop" %} 1x, {% product_first_image product size="120x120" method="crop" %} 2x"
+                <img src="{% product_first_image product size=60 method="crop" %}"
+                    srcset="{% product_first_image product size=60 method="crop" %} 1x, {% product_first_image product size=120 method="crop" %} 2x"
                     alt="">
               </td>
               <td>
diff --git a/templates/dashboard/search/results.html b/templates/dashboard/search/results.html
index 2545205352a..2fcb72a8467 100644
--- a/templates/dashboard/search/results.html
+++ b/templates/dashboard/search/results.html
@@ -27,8 +27,8 @@ <h6>{% trans 'Products' context 'Search results list products' %}</h6>
           {% for product in products %}
             <li class="collection-item avatar list-item">
               <a href="{% url 'dashboard:product-details' pk=product.pk %}">
-                <img src="{% product_first_image product size="60x60" method="crop" %}"
-                     srcset="{% product_first_image product size="60x60" method="crop" %} 1x, {% product_first_image product size="120x120" method="crop" %} 2x"
+                <img src="{% product_first_image product size=60 method="crop" %}"
+                     srcset="{% product_first_image product size=60 method="crop" %} 1x, {% product_first_image product size=120 method="crop" %} 2x"
                      class="circle" alt="{{ product.name }}">
                 <span class="list-item-name">
                   {{ product.name }}
diff --git a/templates/order/_ordered_items_table.html b/templates/order/_ordered_items_table.html
index c53aff1eaaa..6ef13725ddc 100644
--- a/templates/order/_ordered_items_table.html
+++ b/templates/order/_ordered_items_table.html
@@ -50,8 +50,8 @@ <h3>
     <div class="row">
       <div class="col-md-8">
         <a class="link--clean" href="{% if line.variant %}{{ line.variant.get_absolute_url }}{% endif %}">
-          <img data-src="{% product_first_image line.variant.product size="60x60" method="crop" %}"
-                data-srcset="{% product_first_image line.variant.product size="60x60" method="crop" %} 1x, {% product_first_image line.variant.product size="120x120" method="crop" %} 2x"
+          <img data-src="{% product_first_image line.variant.product size=60 method="crop" %}"
+                data-srcset="{% product_first_image line.variant.product size=60 method="crop" %} 1x, {% product_first_image line.variant.product size=120 method="crop" %} 2x"
                 class="float-left lazyload lazypreload"
                 src="{% placeholder size=60 %}">
           <span class="order-details__product__description">{{ line.product_name }}</span>
diff --git a/templates/product/_items.html b/templates/product/_items.html
index ceb4979277a..6e828f57935 100644
--- a/templates/product/_items.html
+++ b/templates/product/_items.html
@@ -11,8 +11,8 @@
       <div class="text-center">
         <div>
           <img class="img-responsive lazyload lazypreload"
-               data-src="{% product_first_image product method="crop" size="255x255" %}"
-               data-srcset="{% product_first_image product method="crop" size="255x255" %} 1x, {% product_first_image product method="crop" size="510x510" %} 2x"
+               data-src="{% product_first_image product method="crop" size=255 %}"
+               data-srcset="{% product_first_image product method="crop" size=255 %} 1x, {% product_first_image product method="crop" size=510 %} 2x"
                alt=""
                src="{% placeholder size=255 %}">
           <span class="product-list-item-name" title="{{ product }}">{{ product }}</span>
diff --git a/templates/product/details.html b/templates/product/details.html
index 5f70b6c8299..575eb9b1077 100644
--- a/templates/product/details.html
+++ b/templates/product/details.html
@@ -27,7 +27,7 @@
   <meta property="og:url" content="{{ product_url }}">
   <link rel="canonical" href="{{ product_url }}">
 
-  {% product_first_image product size="510x510" as product_image %}
+  {% product_first_image product size=510 as product_image %}
   {% if product_image %}
     <meta property="og:image" content="{{ product_image }}" />
     <meta property="og:image:width" content="510">
@@ -74,8 +74,8 @@
               {% for image in images %}
                 <div class="carousel-item{% if forloop.first %} active{% endif %}">
                   <img class="d-block img-fluid lazyload lazypreload"
-                       data-src="{% get_thumbnail image.image method="crop" size="540x540" %}"
-                       data-srcset="{% get_thumbnail image.image method="crop" size="540x540" %} 1x, {% get_thumbnail image.image method="crop" size="1080x1080" %} 2x"
+                       data-src="{% get_thumbnail image.image method="crop" size=540 %}"
+                       data-srcset="{% get_thumbnail image.image method="crop" size=540 %} 1x, {% get_thumbnail image.image method="crop" size=1080 %} 2x"
                        alt=""
                        src="{% placeholder size=540 %}">
                 </div>
@@ -93,8 +93,8 @@
               {% for image in images %}
                 {% if images|length > 1 %}
                   <li data-target="#carousel-example-generic" data-slide-to="{{ forloop.counter0 }}"{% if forloop.first %} class="active"{% endif %}>
-                    <img data-src="{% get_thumbnail image.image method="crop" size="60x60" %}"
-                         data-srcset="{% get_thumbnail image.image method="crop" size="60x60" %} 1x, {% get_thumbnail image.image method="crop" size="120x120" %} 2x"
+                    <img data-src="{% get_thumbnail image.image method="crop" size=60 %}"
+                         data-srcset="{% get_thumbnail image.image method="crop" size=60 %} 1x, {% get_thumbnail image.image method="crop" size=120 %} 2x"
                          alt=""
                          class="lazyload lazypreload"
                          src="{% placeholder size=60 %}">
diff --git a/tests/api/test_graphql.py b/tests/api/test_graphql.py
index cc8e7a23759..96392eccb20 100644
--- a/tests/api/test_graphql.py
+++ b/tests/api/test_graphql.py
@@ -120,8 +120,8 @@ def test_real_query(user_api_client, product):
             ...ProductPriceFragmentQuery
             __typename
         }
-        thumbnailUrl1x: thumbnailUrl(size: "255x255")
-        thumbnailUrl2x: thumbnailUrl(size: "510x510")
+        thumbnailUrl1x: thumbnailUrl(size: 255)
+        thumbnailUrl2x: thumbnailUrl(size: 510)
         url
         __typename
     }
diff --git a/tests/test_product_tags.py b/tests/test_product_tags.py
index 3de94651cf2..2692988789f 100644
--- a/tests/test_product_tags.py
+++ b/tests/test_product_tags.py
@@ -1,20 +1,23 @@
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
 
 from django.contrib.staticfiles.templatetags.staticfiles import static
+from django.test import override_settings
 
 from saleor.product.templatetags.product_images import (
     choose_placeholder, get_thumbnail, product_first_image)
 
 
+@override_settings(
+    VERSATILEIMAGEFIELD_SETTINGS={'create_images_on_demand': True})
 def test_get_thumbnail():
     instance = Mock()
     cropped_value = Mock(url='crop.jpg')
     thumbnail_value = Mock(url='thumb.jpg')
     instance.crop = {'10x10': cropped_value}
     instance.thumbnail = {'10x10': thumbnail_value}
-    cropped = get_thumbnail(instance, '10x10', method='crop')
+    cropped = get_thumbnail(instance, 10, method='crop')
     assert cropped == cropped_value.url
-    thumb = get_thumbnail(instance, '10x10', method='thumbnail')
+    thumb = get_thumbnail(instance, 10, method='thumbnail')
     assert thumb == thumbnail_value.url
 
 
@@ -22,10 +25,54 @@ def test_get_thumbnail_no_instance(monkeypatch):
     monkeypatch.setattr(
         'saleor.product.templatetags.product_images.choose_placeholder',
         lambda x: 'placeholder')
-    output = get_thumbnail(instance=None, size='10x10', method='crop')
+    output = get_thumbnail(instance=None, size=10, method='crop')
     assert output == static('placeholder')
 
 
+@patch(
+    'saleor.product.templatetags.product_images.AVAILABLE_SIZES',
+    {
+        'thumbnail__800x800', 'crop__100x100', 'crop__1000x1000',
+        'crop__2000x2000'})
+@override_settings(
+    VERSATILEIMAGEFIELD_SETTINGS={'create_images_on_demand': False})
+def test_get_thumbnail_to_larger():
+    instance = Mock()
+    cropped_value = Mock(url='crop.jpg')
+    instance.crop = {'1000x1000': cropped_value}
+    cropped = get_thumbnail(instance, 800, method='crop')
+    assert cropped == cropped_value.url
+
+
+@patch(
+    'saleor.product.templatetags.product_images.AVAILABLE_SIZES',
+    {'crop__10x10', 'crop__100x100', 'crop__1000x1000', 'crop__2000x2000'})
+@override_settings(
+    VERSATILEIMAGEFIELD_SETTINGS={'create_images_on_demand': False})
+def test_get_thumbnail_to_smaller():
+    instance = Mock()
+    cropped_value = Mock(url='crop.jpg')
+    instance.crop = {'100x100': cropped_value}
+    cropped = get_thumbnail(instance, 400, method='crop')
+    assert cropped == cropped_value.url
+
+
+@patch(
+    'saleor.product.templatetags.product_images.AVAILABLE_SIZES',
+    {'thumbnail__800x800'})
+@override_settings(
+    VERSATILEIMAGEFIELD_SETTINGS={'create_images_on_demand': False},
+    PLACEHOLDER_IMAGES={1080: 'images/placeholder1080x1080.png'})
+def test_get_thumbnail_no_match_by_method():
+    instance = Mock()
+    cropped_value = Mock(url='crop.jpg')
+    instance.crop = {'1000x1000': cropped_value}
+    cropped = get_thumbnail(instance, 800, method='crop')
+    assert cropped == static('images/placeholder1080x1080.png')
+
+
+@override_settings(
+    VERSATILEIMAGEFIELD_SETTINGS={'create_images_on_demand': True})
 def test_product_first_image():
     mock_product_image = Mock()
     mock_product_image.image = Mock()
@@ -34,7 +81,7 @@ def test_product_first_image():
     mock_queryset = Mock()
     mock_queryset.all.return_value = [mock_product_image]
     mock_product = Mock(images=mock_queryset)
-    out = product_first_image(mock_product, '10x10', method='crop')
+    out = product_first_image(mock_product, 10, method='crop')
     assert out == 'crop.jpg'
 
 


Code review comments:
diff_hunk: @@ -45,26 +45,56 @@ def choose_placeholder(size=''):
     return placeholder
 
 
+def get_available_sizes_by_method(method):
+    sizes = []
+    for available_size in AVAILABLE_SIZES:
+        available_method, avail_size_str = available_size.split('__')
+        if available_method != method:
path: saleor/product/templatetags/product_images.py
text: @user1:
I think that a more idiomatic way in Python would be to check `if available_method == method` and handle the positive case only.

diff_hunk: @@ -45,26 +45,56 @@ def choose_placeholder(size=''):
     return placeholder
 
 
+def get_available_sizes_by_method(method):
+    sizes = []
+    for available_size in AVAILABLE_SIZES:
+        available_method, avail_size_str = available_size.split('__')
+        if available_method != method:
+            continue
+        sizes.append(min([int(s) for s in avail_size_str.split('x')]))
+    return sizes
+
+
+def get_thumbnail_size(size, method):
+    """ Return closest larger size if not more than 2 times larger, otherwise
+    return closest smaller size
+    """
+    avail_sizes = sorted(get_available_sizes_by_method(method))
+    larger = list(filter(lambda x: size < x <= size * 2, avail_sizes))
+    smaller = list(filter(lambda x: x <= size, avail_sizes))
+
+    if larger:
+        return '%sx%s' % (larger[0], larger[0])
+    elif smaller:
+        return'%sx%s' % (smaller[-1], smaller[-1])
+    return None
+
+
 @register.simple_tag()
 def get_thumbnail(instance, size, method='crop'):
-    size_name = '%s__%s' % (method, size)
+    size_str = '%sx%s' % (size, size)
+    size_name = '%s__%s' % (method, size_str)
     on_demand = settings.VERSATILEIMAGEFIELD_SETTINGS[
         'create_images_on_demand']
-    if instance:
-        if size_name not in AVAILABLE_SIZES and not on_demand:
-            msg = (
-                "Thumbnail size %s is not defined in settings "
-                "and it won't be generated automatically" % size_name)
-            warnings.warn(msg)
-        try:
-            thumbnail = getattr(instance, method)[size]
-        except Exception:
-            logger.exception(
-                'Thumbnail fetch failed',
-                extra={'instance': instance, 'size': size})
-        else:
-            return thumbnail.url
-    return static(choose_placeholder(size))
+    if not instance:
+        return static(choose_placeholder(size_str))
path: saleor/product/templatetags/product_images.py
text: @user1:
This line is duplicated within this function. We already have a fallback that returns a placeholder at the end of this function, do we need to return it also here?

@author:
Thanks for Your suggestion! Initially there was only one placeholder return statement, but the code didn't pass codeclimate's cognitive complexity check so I started to remote nested conditions and statements and I had to introduce this part. I'll try to refactor this function to look cleaner

diff_hunk: @@ -45,26 +45,56 @@ def choose_placeholder(size=''):
     return placeholder
 
 
+def get_available_sizes_by_method(method):
+    sizes = []
+    for available_size in AVAILABLE_SIZES:
+        available_method, avail_size_str = available_size.split('__')
+        if available_method != method:
+            continue
+        sizes.append(min([int(s) for s in avail_size_str.split('x')]))
+    return sizes
+
+
+def get_thumbnail_size(size, method):
+    """ Return closest larger size if not more than 2 times larger, otherwise
+    return closest smaller size
+    """
+    avail_sizes = sorted(get_available_sizes_by_method(method))
+    larger = list(filter(lambda x: size < x <= size * 2, avail_sizes))
path: saleor/product/templatetags/product_images.py
text: @user1:
Using `filter` is considered an antipattern in places where a list comprehensions could be used (the same applies to `map`). A more idiomatic way would be: ```[x for x in avail_sizes if size < x <= size * 2]```