Repository: saleor/saleor
Pull request: SEO friendly description for products.
This pull request has for goal to close #1845.

----

### Proposal

My current proposal is to add a new column inside the `Product` table which would only be used for the SEO friendly description.

This column, `Product.seo_description`, is set to:
  - a max length of 255, which should be way enough;
  - optional (can be blank in forms).

----

### Process

`ProductForm` (dashboard) now has both a `description` and a `seo_description` (optional) field.

- if `seo_description` is not set (blank), its value is set to the description but transformed/ adapted:
  1. Uses the clean-strip feature of bleach:
    - Removes any HTML tag from description;
    - Removes any whitespaces (tabs, multiple spaces and new lines).
  2. Trim the new description to a max length of `settings.GENERATED_SEO_MAX_LENGTH` (155 by default, since it's the recommended value) and add a suffix (`'[...]'` by default) if trimmed.
- if `seo_description` is set, nothing other than the default cleaning is done (it's a simple text field).

*Note: the reason behind implementing the changes inside the form instead of the models, is because `Product.description` is containing escaped HTML, using another parser (our own) than bleach is unrecommended. And doing just like in the migration section below would waste CPU time.*

----

### Migration

Existing products have their `seo_description` set to a generated SEO friendly description using the same method as described in [Process](#Process) (stripped whitespaces and HTML tags, max. 155 chars and a suffix if too long).

----

I will be waiting for your feedback on this suggestion and changes.

----

### Pull Request Checklist

(Please keep this section. It will make maintainer's life easier.)

1. [x] Privileged views and APIs are guarded by proper permission checks.
1. [x] All visible strings are translated with proper context.
1. [x] All data-formatting is locale-aware (dates, numbers, and so on).
1. [x] Database queries are optimized and the number of queries is constant.
1. [x] The changes are tested.
1. [x] The code is documented (docstrings, project documentation).
1. [x] Python code quality checks pass: `pycodestyle`, `pydocstyle`, `pylint`.
1. [x] JavaScript code quality checks pass: `eslint`.


Problem statement:
SEO: meta description are showing HTML tags
Hello,

If we create a product, let's say the following description:

----
Saleor is the **best** brand.
And here is a new line as well.

----

It results to:
```html
<meta name="description" content="&lt;p&gt;Saleor is the &lt;b&gt;best&lt;/b&gt; brand.&lt;/p&gt;&lt;p&gt;And here is a new line as well.&lt;/p&gt;">
<meta property="og:description" content="&lt;p&gt;Saleor is the &lt;b&gt;best&lt;/b&gt; brand.&lt;/p&gt;&lt;p&gt;And here is a new line as well.&lt;/p&gt;">
```

----


Or even just an empty description will render to:
```html
<meta name="description" content="&lt;br&gt;">
<meta property="og:description" content="&lt;br&gt;">
```

I think we should add a column that would be a cleaned description and SEO friendly (which should be a short. The recommendation being of 155 characters).

I know that for example, not sure if it's still the case, on wordpress we had the possibility to write a short SEO friendly description or else it would take and clean a small part of the article.

Hints from issue discussion:
I think we may use [builtin strip_tags templatetag](https://docs.djangoproject.com/en/dev/ref/templates/builtins/#striptags)

Input length is another thing, as I remember there's some SEO related ticket in the roadmap, which should provide additional fields for overriding such properties.
But I think it should also be kept in memory, there is no use doing that every time a product is rendered, the value of the description almost never change, it's a waste of CPU cycles.

Commit message: Two more cases added:
  - punctuation at the end of the last word of the cut sentence means there is nothing to process (should fix partial);
  - make sure a line that is not too long is not trimmed.

Patch to review:
diff --git a/requirements.in b/requirements.in
index 72084bfdde9..74452ef7dec 100644
--- a/requirements.in
+++ b/requirements.in
@@ -44,6 +44,7 @@ pytest-django
 pytest-vcr
 pytest-mock
 vcrpy
+regex
 requests>=1.2.0
 social-auth-app-django>=2.0.0
 text-unidecode
diff --git a/requirements.txt b/requirements.txt
index ceae2ec0eec..4257649f5e8 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -89,6 +89,7 @@ python3-openid==3.1.0     # via social-auth-core
 pytz==2018.3              # via babel, celery, django
 pyyaml==3.12              # via vcrpy
 redis==2.10.6             # via celery, django-redis
+regex==2018.2.21
 requests-oauthlib==0.8.0  # via social-auth-core
 requests==2.18.4
 rx==1.6.0                 # via graphql-core
diff --git a/saleor/core/utils/random_data.py b/saleor/core/utils/random_data.py
index 48cbde77678..e8e3d6195e2 100644
--- a/saleor/core/utils/random_data.py
+++ b/saleor/core/utils/random_data.py
@@ -268,10 +268,12 @@ def get_or_create_collection(name, **kwargs):
 
 
 def create_product(**kwargs):
+    description = fake.paragraphs(5)
     defaults = {
         'name': fake.company(),
         'price': fake.money(),
-        'description': '\n\n'.join(fake.paragraphs(5))}
+        'description': '\n\n'.join(description),
+        'seo_description': description[0][:300]}
     defaults.update(kwargs)
     return Product.objects.create(**defaults)
 
diff --git a/saleor/core/utils/text.py b/saleor/core/utils/text.py
new file mode 100644
index 00000000000..1ced87d36f1
--- /dev/null
+++ b/saleor/core/utils/text.py
@@ -0,0 +1,106 @@
+# coding: utf-8
+
+import bleach
+import regex as re
+from django.forms import fields
+from html5lib.serializer import HTMLSerializer
+
+# This will match from a cut word to a punctuation or whitespace
+LAST_WORD_SENTENCE_RE = re.compile(r'[\p{Po}\s]\w*$', re.U)
+
+
+def _get_last_word_separator_pos(text: str):
+    """Get the position of the first punctuation or whitespace next to the
+    first full word from the end of the sentence.
+
+    :param text:
+    :type text: str
+
+    :rtype: Union[int, None]
+    """
+    match = LAST_WORD_SENTENCE_RE.search(text)
+    if match:
+        # position of the last stop char
+        return match.start()
+    return None
+
+
+def trim_fullword(text: str, max_length: int):
+    """Trim text to a length lesser than `max_length` without cutting words
+    in their middle.
+
+    If it failed to trim the text (first word too long), the word will be cut.
+
+    :param text: The text to trim.
+    :type text: str
+
+    :param max_length: The maximal length of the new string to return.
+    :type max_length: int
+
+    :rtype: str
+    """
+    new_text = text[:max_length]
+
+    if len(text) > max_length:
+        # if the last char wasn't a punctuation or whitespace,
+        # try to cut in another place or if unable,
+        # just return the sentence with the word cut
+        if not _get_last_word_separator_pos(text[max_length]):
+            end_pos = _get_last_word_separator_pos(new_text)
+            new_text = new_text[:end_pos]
+    return new_text
+
+
+def get_cleaner(**serializer_kwargs: bool):
+    """
+    :param serializer_kwargs:
+     options:
+        - alphabetical_attributes
+        - escape_lt_in_attrs
+        - escape_rcdata
+        - inject_meta_charset
+        - minimize_boolean_attributes
+        - omit_optional_tags
+        - quote_attr_values
+        - quote_char
+        - resolve_entities
+        - sanitize
+        - space_before_trailing_solidus
+        - strip_whitespace
+        - use_best_quote_char
+        - use_trailing_solidus
+    :type serializer_kwargs: Dict[str, bool]
+
+    :rtype: bleach.Cleaner
+    """
+    cleaner = bleach.Cleaner([], strip=True)
+    for k, v in serializer_kwargs.items():
+        if k not in HTMLSerializer.options:
+            raise ValueError(
+                'Parameter %s is not a valid option for HTMLSerializer' % k)
+        setattr(cleaner.serializer, k, v)
+    return cleaner
+
+
+def strip_html(text: str, **serializer_kwargs: bool):
+    """Removes (strips) HTML tags from text.
+    Can also take additional parameters
+    to be passed to the serializer (see `get_cleaner`).
+
+    :param text:
+     :type text: str
+    :param serializer_kwargs:
+     :type serializer_kwargs: Dict[str, bool]
+    :rtype: str
+    """
+    cleaner = get_cleaner(**serializer_kwargs)
+    text = cleaner.clean(text)
+    return text
+
+
+def generate_seo_description(html_text: str, target_field: fields.CharField):
+    """Strips HTML tags and whitespaces from text,
+    then trim the description."""
+    text = strip_html(html_text, strip_whitespace=True)
+    text = trim_fullword(text, target_field.max_length)
+    return text
diff --git a/saleor/dashboard/product/forms.py b/saleor/dashboard/product/forms.py
index 95e220530e0..8271455fcbb 100644
--- a/saleor/dashboard/product/forms.py
+++ b/saleor/dashboard/product/forms.py
@@ -9,6 +9,7 @@
 from django.utils.translation import pgettext_lazy
 
 from . import ProductBulkAction
+from ...core.utils.text import generate_seo_description
 from ...product.models import (
     AttributeChoiceValue, Collection, Product, ProductAttribute, ProductImage,
     ProductType, ProductVariant, Stock, StockLocation, VariantImage)
@@ -141,6 +142,8 @@ class Meta:
         labels = {
             'name': pgettext_lazy('Item name', 'Name'),
             'description': pgettext_lazy('Description', 'Description'),
+            'seo_description': pgettext_lazy(
+                'A SEO friendly description', 'SEO Friendly Description'),
             'category': pgettext_lazy('Category', 'Category'),
             'price': pgettext_lazy('Currency amount', 'Price'),
             'available_on': pgettext_lazy(
@@ -167,6 +170,21 @@ def __init__(self, *args, **kwargs):
         self.fields["collections"].initial = Collection.objects.filter(
             products__name=self.instance)
 
+    def clean_seo_description(self):
+        seo_description = self.cleaned_data['seo_description']
+
+        # if there is no SEO friendly description set,
+        # generate it from the HTML description
+        if not seo_description:
+            # get the non-safe description (has non escaped HTML tags in it)
+            product_description = self.data['description']
+
+            # generate a SEO friendly from HTML description
+            seo_description = generate_seo_description(
+                product_description, self.fields['seo_description'])
+
+        return seo_description
+
     def prepare_fields_for_attributes(self):
         for attribute in self.product_attributes:
             field_defaults = {
diff --git a/saleor/product/migrations/0053_product_seo_description.py b/saleor/product/migrations/0053_product_seo_description.py
new file mode 100644
index 00000000000..a937e604f48
--- /dev/null
+++ b/saleor/product/migrations/0053_product_seo_description.py
@@ -0,0 +1,45 @@
+# Generated by Django 2.0.2 on 2018-03-11 18:54
+import html
+
+from django.core.validators import MaxLengthValidator
+from django.db import migrations, models
+from saleor.core.utils.text import generate_seo_description
+
+
+def to_seo_friendly(cls, text):
+    # saleor descriptions are stored as escaped HTML,
+    # we need to decode them before processing them
+    text = html.unescape(text)
+
+    # cleanup the description and make it seo friendly
+    return generate_seo_description(text, cls._meta.get_field(
+        'seo_description'))
+
+
+def assign_seo_descriptions(apps, schema_editor):
+    Product = apps.get_model('product', 'Product')
+    for product in Product.objects.all():
+        if product.seo_description is None:
+            product.seo_description = to_seo_friendly(
+                Product,
+                product.description)
+            product.save()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('product', '0052_slug_field_length'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='product',
+            name='seo_description',
+            field=models.CharField(
+                blank=True, null=True, max_length=300,
+                validators=[MaxLengthValidator(300)]),
+            preserve_default=False,
+        ),
+        migrations.RunPython(assign_seo_descriptions)
+    ]
diff --git a/saleor/product/models.py b/saleor/product/models.py
index ac639ed3485..101962112d8 100644
--- a/saleor/product/models.py
+++ b/saleor/product/models.py
@@ -3,7 +3,8 @@
 
 from django.conf import settings
 from django.contrib.postgres.fields import HStoreField
-from django.core.validators import MinValueValidator, RegexValidator
+from django.core.validators import (
+    MaxLengthValidator, MinValueValidator, RegexValidator)
 from django.db import models
 from django.db.models import F, Max, Q
 from django.urls import reverse
@@ -92,6 +93,9 @@ class Product(models.Model):
         ProductType, related_name='products', on_delete=models.CASCADE)
     name = models.CharField(max_length=128)
     description = models.TextField()
+    seo_description = models.CharField(
+        max_length=300, blank=True, null=True,
+        validators=[MaxLengthValidator(300)])
     category = models.ForeignKey(
         Category, related_name='products', on_delete=models.CASCADE)
     price = MoneyField(
diff --git a/templates/dashboard/product/form.html b/templates/dashboard/product/form.html
index 0a5f0057547..d42f4c4c93c 100644
--- a/templates/dashboard/product/form.html
+++ b/templates/dashboard/product/form.html
@@ -72,6 +72,9 @@
                 <div class="row">
                   {{ product_form.name|materializecss }}
                 </div>
+                <div class="row">
+                  {{ product_form.seo_description|materializecss }}
+                </div>
                 {% for attribute_field in product_form.iter_attribute_fields %}
                   <div class="row">
                     {{ attribute_field|materializecss:"input-field s12" }}
diff --git a/templates/product/details.html b/templates/product/details.html
index d2946e3925a..724d2567afb 100644
--- a/templates/product/details.html
+++ b/templates/product/details.html
@@ -12,11 +12,11 @@
 
 {% block title %}{{ product }} — {{ block.super }}{% endblock %}
 
-{% block meta_description %}{{ product.description }}{% endblock %}
+{% block meta_description %}{{ product.seo_description }}{% endblock %}
 
 {% block meta_tags %}
   <meta property="og:title" content="{{ product }}">
-  <meta property="og:description" content="{{ product.description }}">
+  <meta property="og:description" content="{{ product.seo_description }}">
 
   {% build_absolute_uri request=request location=product.get_absolute_url as product_url %}
   <meta property="og:url" content="{{ product_url }}">
diff --git a/tests/dashboard/test_product.py b/tests/dashboard/test_product.py
index 0e122d99ebd..56218cbc340 100644
--- a/tests/dashboard/test_product.py
+++ b/tests/dashboard/test_product.py
@@ -695,5 +695,58 @@ def test_sanitize_product_description(product_type, default_category):
     form.save()
     assert product.description == (
         '<b>bold</b><p><i>italic</i></p><h2>Header</h2><h3>subheader</h3>'
-        '<blockquote>quote</blockquote><p><a href="www.mirumee.com">link</a></p>'
+        '<blockquote>quote</blockquote>'
+        '<p><a href="www.mirumee.com">link</a></p>'
         '<p>an &lt;script&gt;evil()&lt;/script&gt;example</p>')
+
+    assert product.seo_description == (
+        'bolditalicHeadersubheaderquotelinkan evil()example')
+
+
+def test_set_product_seo_description(unavailable_product):
+    seo_description = (
+        'This is a dummy product. '
+        'HTML <b>shouldn\'t be removed</b> since it\'s a simple text field.')
+    data = model_to_dict(unavailable_product)
+    data['price'] = 20
+    data['description'] = 'a description'
+    data['seo_description'] = seo_description
+
+    form = ProductForm(data, instance=unavailable_product)
+
+    assert form.is_valid()
+    form.save()
+    assert unavailable_product.seo_description == seo_description
+
+
+def test_set_product_description_too_long_for_seo(unavailable_product):
+    description = (
+        'Saying it fourth made saw light bring beginning kind over herb '
+        'won\'t creepeth multiply dry rule divided fish herb cattle greater '
+        'fly divided midst, gathering can\'t moveth seed greater subdue. '
+        'Lesser meat living fowl called. Dry don\'t wherein. Doesn\'t above '
+        'form sixth. Image moving earth without forth light whales. Seas '
+        'were first form fruit that form they\'re, shall air. And. Good of' 
+        'signs darkness be place. Was. Is form it. Whose. Herb signs stars '
+        'fill own fruit wherein. '
+        'Don\'t set man face living fifth Thing the whales were. '
+        'You fish kind. '
+        'Them, his under wherein place first you night gathering.')
+
+    data = model_to_dict(unavailable_product)
+    data['price'] = 20
+    data['description'] = description
+
+    form = ProductForm(data, instance=unavailable_product)
+
+    assert form.is_valid()
+    form.save()
+
+    assert len(unavailable_product.seo_description) < 300
+    assert unavailable_product.seo_description == (
+        'Saying it fourth made saw light bring beginning kind over herb '
+        'won\'t creepeth multiply dry rule divided fish herb cattle greater '
+        'fly divided midst, gathering can\'t moveth seed greater subdue. '
+        'Lesser meat living fowl called. Dry don\'t wherein. Doesn\'t above '
+        'form sixth. Image moving earth without'
+    )
diff --git a/tests/test_core.py b/tests/test_core.py
index 62c90df4ae4..a11f3ec9f35 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -8,6 +8,8 @@
 from saleor.core.utils import (
     Country, create_superuser, format_money, get_country_by_ip,
     get_currency_for_country, random_data)
+from saleor.core.utils.text import (
+    _get_last_word_separator_pos, get_cleaner, strip_html, trim_fullword)
 from saleor.discount.models import Sale, Voucher
 from saleor.order.models import Order
 from saleor.product.models import Product
@@ -160,3 +162,34 @@ def test_manifest(client, site_settings):
     assert content['name'] == site_settings.site.name
     assert content['short_name'] == site_settings.site.name
     assert content['description'] == site_settings.description
+
+
+def test_utils_get_last_stop_char():
+    assert _get_last_word_separator_pos('') is None
+    assert _get_last_word_separator_pos('this a cut sentenc') == 10
+    assert _get_last_word_separator_pos('.') == 0
+    assert _get_last_word_separator_pos('こんにちは。') == 5
+    assert _get_last_word_separator_pos('여보세요;세계') == 4
+
+
+def test_trim_fullword():
+    assert trim_fullword('it\'s too long. but period.', 14) == 'it\'s too long'
+    assert trim_fullword('it\'s not too long.', 50) == 'it\'s not too long.'
+    assert trim_fullword('don\'t break', 10) == 'don\'t'
+    assert trim_fullword('this is a sentence', 5) == 'this'
+    assert trim_fullword('thisIsTooLongJustCutIt', 4) == 'this'
+    assert trim_fullword('this\nis\nmultiline', 5) == 'this'
+    assert trim_fullword('', 5) == ''
+
+
+def test_utils_get_cleaner_invalid_parameters():
+    with pytest.raises(ValueError):
+        get_cleaner(bad=True)
+
+
+def test_utils_strip_html():
+    base_text = ('<p>Hello</p>'
+                 '\n\n'
+                 '\t<b>World</b>')
+    text = strip_html(base_text, strip_whitespace=True)
+    assert text == 'Hello World'


Code review comments:
diff_hunk: @@ -0,0 +1,45 @@
+# Generated by Django 2.0.2 on 2018-03-11 18:54
+import html
+
+from django.core.validators import MaxLengthValidator
+from django.db import migrations, models
+from saleor.core.utils.text import generate_seo_description
+
+
+def to_seo_friendly(cls, text):
path: saleor/product/migrations/0053_product_seo_description.py
text: @user1:
Is `cls` used for something else than getting fields `max_length` param?
Can we pass `max_length` as an `int` instead?

@author:
I didn't make to allow to be passed as int, the reason behind this was to make the code and reuses more understandable. The idea is to directly take the target field and then directly take the `max_length` attribute. Which would make the code and reuses less repetitive.