Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#302 Slugify tags with theirs groups #304

Merged
merged 5 commits into from
Mar 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions catalog/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,9 @@ def get_siblings(self, offset):
# is not arch design, but about ORM hack.
class TagGroup(models.Model):

SLUG_HASH_SIZE = 5
SLUG_MAX_LENGTH = 25

class Meta:
abstract = True

Expand All @@ -220,6 +223,28 @@ class Meta:
default=0, blank=True, db_index=True, verbose_name=_('position'),
)

# @todo #302:30m Resolve slug code doubling.
# See `TagGroup.slug` and `Tag._get_slug`.
@property
def slug(self) -> str:
"""Make a slug from the name."""
# Translate all punctuation chars to "_".
# It doesn't conflict with `slugify`, which translate spaces to "-"
# and punctuation chars to "".
slug = slugify(unidecode(self.name.translate(
{ord(p): '_' for p in string.punctuation}
)))

# Keep the slug length less then SLUG_MAX_LENGTH
if len(slug) < self.SLUG_MAX_LENGTH:
return slug

slug_length = self.SLUG_MAX_LENGTH - self.SLUG_HASH_SIZE - 1
return randomize_slug(
slug[:slug_length],
hash_size=self.SLUG_HASH_SIZE
)

def __str__(self):
return self.name

Expand Down Expand Up @@ -401,15 +426,16 @@ class Meta:
def __str__(self):
return self.name

def _slugify(self) -> None:
"""Make a slug from the name."""
def _get_slug(self) -> str:
# Translate all punctuation chars to "_".
# It doesn't conflict with `slugify`, which translate spaces to "-"
# and punctuation chars to "".
slug = slugify(unidecode(self.name.translate(
{ord(p): '_' for p in string.punctuation}
)))

slug = '__'.join([self.group.slug, slug])

# Keep the slug length less then SLUG_MAX_LENGTH
if len(slug) < self.SLUG_MAX_LENGTH:
return slug
Expand All @@ -422,7 +448,7 @@ def _slugify(self) -> None:

def save(self, *args, **kwargs):
if not self.slug:
self.slug = self._slugify()
self.slug = self._get_slug()
super(Tag, self).save(*args, **kwargs)

# @todo #168:15m Move `Tags.parse_url_tags` Tags context.
Expand Down
34 changes: 30 additions & 4 deletions tests/catalog/test_models.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Defines tests for models in Catalog app."""
import string
import unittest

from django.db import DataError
from django.test import TestCase
Expand Down Expand Up @@ -249,14 +250,15 @@ class Tag(TestCase):

def test_tag_doubled_save_slug_postfix(self):
"""Tag should preserve it's slug value after several saves."""
slug = '12-v'
group = catalog_models.MockTagGroup.objects.create(name='Напряжение вход')
tag = catalog_models.MockTag.objects.create(
name='12 В',
group=group
)
self.assertEqual(tag.slug, '12-v')
self.assertEqual(tag.slug[-len(slug):], slug)
tag.save()
self.assertEqual(tag.slug, '12-v')
self.assertEqual(tag.slug[-len(slug):], slug)

def test_long_name(self):
"""
Expand All @@ -266,21 +268,45 @@ def test_long_name(self):
It may create problems for tag with long name.
"""
name = 'Имя ' * 50
group = catalog_models.MockTagGroup.objects.first()
group = catalog_models.MockTagGroup.objects.create(name='Some group')
try:
tag = catalog_models.MockTag.objects.create(group=group, name=name)
self.assertLessEqual(len(tag.slug), catalog.models.Tag.SLUG_MAX_LENGTH)
except DataError as e:
self.assertTrue(False, f'Tag has too long name. {e}')

def test_slugify_conflicts(self):
group = catalog_models.MockTagGroup.objects.create(name='Some group')
slugs = [
catalog_models.MockTag.objects.create(name=name).slug
catalog_models.MockTag.objects.create(group=group, name=name).slug
for name in ['11 A', '1/1 A', '1 1 A']
]

self.assertEqual(len(slugs), len(set(slugs)), msg=slugs)

# @todo #302:30m Process more special symbols for slugs.
@unittest.expectedFailure
def test_slug_special_symbols(self):
slugs = [
catalog_models.MockTag.objects.create(name=name).slug
for name in ['11 A', '1/1 A', '1 1 A', '1.1 A', '1-1 A', '1_1 A']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a separate skipped test (e.g. test_slugify_punctuation_conflicts) for the set, that led to the conflict to preserve testing of the set that has no such conflicts 11 A, 1/1 A, 1 1 A

This will help to avoid regressions if we break the slugify for non-conflict set. In common i'd prefer stick to such rule: new tests for new bugs. Up to you

]

self.assertEqual(len(slugs), len(set(slugs)), msg=slugs)

def test_slugs_for_cloned_tag_values(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_slugs_for_cloned_tag_values(self):
def test_cloned_tag_varied_group_slugs(self):

Up to you

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll leave it because of few points:

  • i want to leave test_slugs_ at the beginning
  • tag values are the same. It's not very common situation, so it's the corner case. That's the point of test problem
  • y, groups varying, but it's a normal situation

groups = [
catalog_models.MockTagGroup.objects.create(name=name)
for name in ['Length', 'Width', 'Height']
]
values = ['11 A']*3
slugs = [
catalog_models.MockTag.objects.create(group=group, name=value).slug
for group, value in zip(groups, values)
]

self.assertEqual(len(slugs), len(set(slugs)), msg=slugs)

def test_group_tags(self):
groups = [
catalog_models.MockTagGroup.objects.create(name=name)
Expand Down