Skip to content

Fix slug generation for uppercase branch names #1396

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jul 6, 2015
Merged

Fix slug generation for uppercase branch names #1396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bb1253d
Fix slugification for uppercase letters in the slug.
gregmuellegger Jul 2, 2015
b66fce3
Use 'unkown' as fallback name if the verbose_name exclusively contain…
gregmuellegger Jul 2, 2015
7cd34dc
Adding another slug normalizing migration to fix slugs for branch nam…
gregmuellegger Jul 2, 2015
13f989b
Test slugs after generation that they are really valid.
gregmuellegger Jul 2, 2015
fcf5a09
Fix spelling unkown -> unknown.
gregmuellegger Jul 3, 2015
db52f37
Using regexes to slugify version slugs.
gregmuellegger Jul 3, 2015
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
162 changes: 162 additions & 0 deletions readthedocs/builds/migrations/0024_fix_slugs_with_leading_placeholders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
# -*- coding: utf-8 -*-
from south.utils import datetime_utils as datetime
from south.db import db
from south.v2 import DataMigration
from django.db import models

class Migration(DataMigration):

def forwards(self, orm):
"Write your forwards methods here."
# Note: Don't use "from appname.models import ModelName".
# Use orm.ModelName to refer to models in this application,
# and orm['appname.ModelName'] for models in other applications.

Version = orm['builds.Version']
slug_field = Version._meta.get_field_by_name('slug')[0]

bad_slug = Version.objects.exclude(
slug__regex=r'^[a-z0-9][-._a-z0-9]+$')
for version in bad_slug:
version.slug = slug_field.create_slug(version)
version.save()

def backwards(self, orm):
"Write your backwards methods here."

# No backwards operation possible.

models = {
u'auth.group': {
'Meta': {'object_name': 'Group'},
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': u"orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
},
u'auth.permission': {
'Meta': {'ordering': "(u'content_type__app_label', u'content_type__model', u'codename')", 'unique_together': "((u'content_type', u'codename'),)", 'object_name': 'Permission'},
'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['contenttypes.ContentType']"}),
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
},
u'auth.user': {
'Meta': {'object_name': 'User'},
'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}),
'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
'groups': ('django.db.models.fields.related.ManyToManyField', [], {'symmetrical': 'False', 'related_name': "u'user_set'", 'blank': 'True', 'to': u"orm['auth.Group']"}),
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'symmetrical': 'False', 'related_name': "u'user_set'", 'blank': 'True', 'to': u"orm['auth.Permission']"}),
'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
},
u'builds.build': {
'Meta': {'ordering': "['-date']", 'object_name': 'Build'},
'builder': ('django.db.models.fields.CharField', [], {'max_length': '255', 'null': 'True', 'blank': 'True'}),
'commit': ('django.db.models.fields.CharField', [], {'max_length': '255', 'null': 'True', 'blank': 'True'}),
'date': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
'error': ('django.db.models.fields.TextField', [], {'default': "''", 'blank': 'True'}),
'exit_code': ('django.db.models.fields.IntegerField', [], {'max_length': '3', 'null': 'True', 'blank': 'True'}),
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'length': ('django.db.models.fields.IntegerField', [], {'max_length': '10', 'null': 'True', 'blank': 'True'}),
'output': ('django.db.models.fields.TextField', [], {'default': "''", 'blank': 'True'}),
'project': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'builds'", 'to': u"orm['projects.Project']"}),
'setup': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}),
'setup_error': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}),
'state': ('django.db.models.fields.CharField', [], {'default': "'finished'", 'max_length': '55'}),
'success': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
'type': ('django.db.models.fields.CharField', [], {'default': "'html'", 'max_length': '55'}),
'version': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'builds'", 'null': 'True', 'to': u"orm['builds.Version']"})
},
u'builds.version': {
'Meta': {'ordering': "['-verbose_name']", 'unique_together': "[('project', 'slug')]", 'object_name': 'Version'},
'active': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'built': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'identifier': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
'machine': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'privacy_level': ('django.db.models.fields.CharField', [], {'default': "'public'", 'max_length': '20'}),
'project': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'versions'", 'to': u"orm['projects.Project']"}),
'slug': ('builds.version_slug.VersionSlugField', [], {'max_length': '255', 'populate_from': "'verbose_name'", 'db_index': 'True'}),
'supported': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
'type': ('django.db.models.fields.CharField', [], {'default': "'unknown'", 'max_length': '20'}),
'uploaded': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'verbose_name': ('django.db.models.fields.CharField', [], {'max_length': '255'})
},
u'builds.versionalias': {
'Meta': {'object_name': 'VersionAlias'},
'from_slug': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '255'}),
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'largest': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'project': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'aliases'", 'to': u"orm['projects.Project']"}),
'to_slug': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '255', 'blank': 'True'})
},
u'contenttypes.contenttype': {
'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
},
u'projects.project': {
'Meta': {'ordering': "('slug',)", 'object_name': 'Project'},
'allow_comments': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'analytics_code': ('django.db.models.fields.CharField', [], {'max_length': '50', 'null': 'True', 'blank': 'True'}),
'canonical_url': ('django.db.models.fields.URLField', [], {'max_length': '200', 'blank': 'True'}),
'comment_moderation': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'conf_py_file': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '255', 'blank': 'True'}),
'copyright': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
'default_branch': ('django.db.models.fields.CharField', [], {'default': 'None', 'max_length': '255', 'null': 'True', 'blank': 'True'}),
'default_version': ('django.db.models.fields.CharField', [], {'default': "'latest'", 'max_length': '255'}),
'description': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
'django_packages_url': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}),
'documentation_type': ('django.db.models.fields.CharField', [], {'default': "'auto'", 'max_length': '20'}),
'enable_epub_build': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
'enable_pdf_build': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
'featured': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'language': ('django.db.models.fields.CharField', [], {'default': "'en'", 'max_length': '20'}),
'main_language_project': ('django.db.models.fields.related.ForeignKey', [], {'blank': 'True', 'related_name': "'translations'", 'null': 'True', 'to': u"orm['projects.Project']"}),
'mirror': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'modified_date': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}),
'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
'num_major': ('django.db.models.fields.IntegerField', [], {'default': '2', 'max_length': '3', 'null': 'True', 'blank': 'True'}),
'num_minor': ('django.db.models.fields.IntegerField', [], {'default': '2', 'max_length': '3', 'null': 'True', 'blank': 'True'}),
'num_point': ('django.db.models.fields.IntegerField', [], {'default': '2', 'max_length': '3', 'null': 'True', 'blank': 'True'}),
'path': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
'privacy_level': ('django.db.models.fields.CharField', [], {'default': "'public'", 'max_length': '20'}),
'programming_language': ('django.db.models.fields.CharField', [], {'default': "'words'", 'max_length': '20', 'blank': 'True'}),
'project_url': ('django.db.models.fields.URLField', [], {'max_length': '200', 'blank': 'True'}),
'pub_date': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
'python_interpreter': ('django.db.models.fields.CharField', [], {'default': "'python'", 'max_length': '20'}),
'related_projects': ('django.db.models.fields.related.ManyToManyField', [], {'symmetrical': 'False', 'to': u"orm['projects.Project']", 'null': 'True', 'through': u"orm['projects.ProjectRelationship']", 'blank': 'True'}),
'repo': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
'repo_type': ('django.db.models.fields.CharField', [], {'default': "'git'", 'max_length': '10'}),
'requirements_file': ('django.db.models.fields.CharField', [], {'default': 'None', 'max_length': '255', 'null': 'True', 'blank': 'True'}),
'single_version': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'skip': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'slug': ('django.db.models.fields.SlugField', [], {'unique': 'True', 'max_length': '255'}),
'suffix': ('django.db.models.fields.CharField', [], {'default': "'.rst'", 'max_length': '10'}),
'theme': ('django.db.models.fields.CharField', [], {'default': "'default'", 'max_length': '20'}),
'use_system_packages': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'use_virtualenv': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'users': ('django.db.models.fields.related.ManyToManyField', [], {'related_name': "'projects'", 'symmetrical': 'False', 'to': u"orm['auth.User']"}),
'version': ('django.db.models.fields.CharField', [], {'max_length': '100', 'blank': 'True'}),
'version_privacy_level': ('django.db.models.fields.CharField', [], {'default': "'public'", 'max_length': '20'})
},
u'projects.projectrelationship': {
'Meta': {'object_name': 'ProjectRelationship'},
'child': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'superprojects'", 'to': u"orm['projects.Project']"}),
u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'parent': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'subprojects'", 'to': u"orm['projects.Project']"})
}
}

complete_apps = ['builds']
symmetrical = True
24 changes: 14 additions & 10 deletions readthedocs/builds/version_slug.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,16 @@
VERSION_SLUG_REGEX = '(?:[a-z0-9][-._a-z0-9]+?)'


version_slug_regex = re.compile(VERSION_SLUG_REGEX)


class VersionSlugField(models.CharField):
"""
Implementation inspired by ``django_extensions.db.fields.AutoSlugField``.
"""

allowed_chars = string.lowercase + string.digits + '-._'
invalid_chars_re = re.compile('[^-._a-z0-9]')
leading_punctuation_re = re.compile('^[-._]+')
placeholder = '-'
fallback_slug = 'unknown'
test_pattern = re.compile('^{pattern}$'.format(pattern=VERSION_SLUG_REGEX))

def __init__(self, *args, **kwargs):
kwargs.setdefault('db_index', True)
Expand All @@ -63,12 +63,13 @@ def get_queryset(self, model_cls, slug_field):
def slugify(self, content):
if not content:
return ''
slugified = ''
for char in content:
if char not in self.allowed_chars:
slugified += self.placeholder
else:
slugified += char

slugified = content.lower()
slugified = self.invalid_chars_re.sub(self.placeholder, slugified)
slugified = self.leading_punctuation_re.sub('', slugified)

if not slugified:
return self.fallback_slug
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also be +1 on replacing these blocks with regex comparisons/substitutions. These two blocks could be a single regex substitution, with a check against the final matches for empty/invalid slug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, that is addressed with the latest push. I've not used regex.sub before that's why I didn't think about it. It reduced the code quite a bit. Thanks for the suggestion!

return slugified

def uniquifying_suffix(self, iteration):
Expand Down Expand Up @@ -139,6 +140,9 @@ def create_slug(self, model_instance):
slug = slug + end
kwargs[self.attname] = slug
next += 1

assert self.test_pattern.match(slug), (
'Invalid generated slug: {slug}'.format(slug=slug))
return slug

def pre_save(self, model_instance, add):
Expand Down
23 changes: 23 additions & 0 deletions readthedocs/rtd_tests/tests/test_version_slug.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,29 @@ def test_normalizing_slashes(self):
project=self.pip)
self.assertEqual(version.slug, 'releases-1.0')

def test_uppercase(self):
version = Version.objects.create(
verbose_name='SomeString-charclass',
project=self.pip)
self.assertEqual(version.slug, 'somestring-charclass')

def test_placeholder_as_name(self):
version = Version.objects.create(
verbose_name='-',
project=self.pip)
self.assertEqual(version.slug, 'unknown')

def test_multiple_empty_names(self):
version = Version.objects.create(
verbose_name='-',
project=self.pip)
self.assertEqual(version.slug, 'unknown')

version = Version.objects.create(
verbose_name='-./.-',
project=self.pip)
self.assertEqual(version.slug, 'unknown_a')

def test_uniqueness(self):
version = Version.objects.create(
verbose_name='1!0',
Expand Down