Ensure entry slugs fit in a slug field. Fixes #35

[bradmontgomery]

Here's a first attempt at resolving #35. It does the following:

• moves the slugify code into a method outside of save, _slugify_title.
• limits slugs to 50 characters (ensuring there's no trailing dash)
• uses the existing technique to prevent duplicate slugs (adding a timestamp to the end), but does so in a way that still ensures the slug is less than 50 chars.
• adds a test case for each of the above.

Let me know what you think. I'm open to revising any of this.

[bradmontgomery]

Oops! 😢 Unicode fix coming soon.

[ivanvenosdel]

This looks pretty good. Nice testing! I see that this covers both the character restriction and the slash cases.

The only case that is sticking in my brain is how the logic will cut off words.

For example:

>>> slugify("Please tell me where everyone is getting their assumptions about me
?")[:50].strip('-')
u'please-tell-me-where-everyone-is-getting-their-ass'

Now something like that of course can happen when words are cut off:

>>> slugify("About the incident... You think I am crazy? Well I am not!")[:50].strip('-')
u'about-the-incident-you-think-i-am-crazy-well-i-am'

Though the first one is at least preventable. So I think we may as well address it.

[bradmontgomery]

Yeah, good point. I'll try to address this and update the PR in the next day or so.

Another option that we haven't talked about is to create a custom SlugField without the 50-character restriction. URLS are ok up to 2,083 characters, so it's feasible to have slugs that are the same length as the title and not go over that limit.

[ivanvenosdel]

SlugField without the 50-character restriction

I have been thinking about this lately as well and I might be able to get some time for it later this week.

[ivanvenosdel]

Looks like the max we can go is 256 as I would like to keep the project portable.
https://docs.djangoproject.com/en/1.8/ref/databases/#character-fields

Increasing the slug to 255 and reducing the title to 255 would be a viable solution on it's own.
Though I would have to release it as a Major version as it would technically be backwards incompatible to those with titles > 255.

Otherwise if we only increase the size of the slug field the truncation logic will have to be finished.

I am all right with any of these approaches:

1. The finished truncation logic.
2. The finished truncation logic plus a slug field length increase to 255.
3. No truncation logic but a title field decrease to 255 and slug field increase to 255.

[ivanvenosdel]

One more 4th option which is probably the most ideal:

4. A title field decrease to 255 and slug field increase to 255. With truncation logic in a migration to bring titles > 255 in line. (This would still be a major but would be easier for those who don't care)

I have a little time today and tomorrow to do it if you prefer 3 or 4 but don't feel comfortable/don't have time to make the change.

[bradmontgomery]

👍 for option 4. It seems like the best approach to me, though you may want to keep the existing technique to prevent duplicate slugs (I altered it slightly so that it would keep the total length of the slug < 50).

I'd be happy to do this work, but I won't be available until the weekend. Feel free to take over from here if you want.

[ivanvenosdel]

I had missed that you already have the truncation logic finished. Nice.

I would like to see your original pull request be merged in so I thought about doing a PR against your branch... That seemed more complicated then just waiting for your weekend time and there isn't much left now. So have at it. :)

[bradmontgomery]

The last few commits add the following:

1. A data migration to truncate titles to 255 characters; it chops off a word until the whole length of the title is < 255 so it doesn't truncate something mid-word
2. Updated Entry.title and Entry.slug so they both have a max_length=255 and added the associated migration.
3. Improved the slug's truncation logic a bit, so it doesn't introduce a double-dash, e.g. some-slug--123456789
4. Updated the tests.

Let me know if there's anything in there that looks out of place, or if you'd like to do anything differently.

[ivanvenosdel]

Thanks! I can take a look at this tommorow afternoon.

[ivanvenosdel]

Code looks great. I manually tested the following:

1. Migration with > 255 title
2. Migration with < 255 title
3. Migration from empty db
4. Attempting save (from admin) with > 255 title

[ivanvenosdel]

This has been released on PyPI.
https://pypi.python.org/pypi/django-andablog/2.0.0

[bradmontgomery]

Excellent! 👍