make Category.slug unique

[PetrDlouhy]

This makes slug unique as mentioned in #116. I think it is very helpful in many usages of slug. This change could cause problems during migration, if the user have categories with duplicate slug. I could make data migration which adds numbering to duplicate slugs, but I think it is better to let the the projects handle it by their own.

[PetrDlouhy]

I fixed the tests and added one to ensure slug uniqueness. @epicserve Could you please review?

[epicserve]

@PetrDlouhy,

Your PR looks good to me. I'm personally not excited about adding a breaking change though. This isn't a feature that I've seen requested by others and it's not one that I personally need since any time I need django-category functionality I use the abstract model 'categories.base.CategoryBase'. If I was voting I think I would vote on taking out categories.models.Category entirely and just forces people to use the base model. It might be good to consider adding the unique attribute to the base model, but even then it would break everyone's app.

[PetrDlouhy]

Well, I think, that the current state might be also harmful. As a developer I would expect, that SlugField is unique, so I would easily write expressions like Category.objects.get(slug=some_slug) to my app. Then some user adds accidentally duplicated slug, and that can lead to the whole broken website.

On the other hand, there is only broken migration, which can be easily fixed. And as I said, I can write data migration for that (maybe even with input needed to confirm the change).

@epicserve If you can think of a use case, when the duplicated slugs are actually needed, I could also add some override variable (like CATEGORIES_ALLOW_DUPLICATES) to the settings.

[epicserve]

@PetrDlouhy,

How would you write a data migration that would fix duplicates before applying the unique constraint?

[PetrDlouhy]

@epicserve I would prompt the user, and if he press 'y', I would select the duplicated categories and make sequence of them (if there is 3 times slug cat, I would make them cat, cat_1, cat_2).

[epicserve]

@PetrDlouhy,

I don't think prompting the user would be good, because this wouldn't work for anyone that is using CI/CD. May it would be best to do a count by the slug field and then do as you suggest and incremental numbers for each duplicate. You would also want to let me people know in the README so that they're aware of what will happen.

[PetrDlouhy]

Hi @epicserve I have written the migration, it will add the sequence without prompting user. I created the migration tests. I had to fix #152 in order to make the example project migrations working. Now the migrations are better more tested as whole. I have rewritten the README note with information about the migration.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 95.45455% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 66.66%. Comparing base (fc489a8) to head (0114a6a). Report is 5 commits behind head on master.

:exclamation: Current head 0114a6a differs from pull request most recent head 0e3ed41. Consider uploading reports for the commit 0e3ed41 to get more accurate results

Files	Patch %	Lines
categories/fields.py	50.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   65.78%   66.66%   +0.88%     
==========================================
  Files          24       25       +1     
  Lines        1128     1143      +15     
  Branches      218      256      +38     
==========================================
+ Hits          742      762      +20     
+ Misses        292      290       -2     
+ Partials       94       91       -3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[PetrDlouhy]

I have rebased this on top of #167

[PetrDlouhy]

@coordt @epicserve I have updated this to the ~~newest master~~ on top of #173 (otherwise tests don't pass), all tests are passing. Is there anything more I can do to get this merged?

[epicserve]

@PetrDlouhy, this looks pretty good to me at a quick glance. I would think this would need to be a 2.0 release since this is could potentially be a breaking change. Even though you're taking care of the duplicates with the data migration, I wonder if it could end up breaking someone's application if they are using a slug in some way that got changed from foo to foo_2.

[coordt]

Is there more changes we want to make/merge before doing a 2.0 release? If so, I would recommend doing them all at once.

[epicserve]

@PetrDlouhy, I took a look at this, and I was going to make this commit to optimize the migration because the migration would be pretty slow on a large database. You would have to follow this guide to allow maintainers to contribute to your PR.

The following is the patch for the commit I was going to make if you want to make that change.

I think we would also want to update the docs explaining that this is a potential breaking change. I'm thinking this should be a 2.0 release.

From 712d3a86c5cbcc4d93cf96dcb804de913469051c Mon Sep 17 00:00:00 2001
From: Brent O'Connor <[email protected]>
Date: Sat, 24 Sep 2022 10:45:43 -0500
Subject: [PATCH] Optimize slug duplication migration

---
 .../migrations/0005_unique_category_slug.py   |  9 ++++---
 categories/tests/test_migrations.py           | 24 +++++++++----------
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/categories/migrations/0005_unique_category_slug.py b/categories/migrations/0005_unique_category_slug.py
index 05f019e..bc56a6d 100644
--- a/categories/migrations/0005_unique_category_slug.py
+++ b/categories/migrations/0005_unique_category_slug.py
@@ -2,10 +2,12 @@
 
 from django.db import migrations, models
 
+from categories.models import Category
+
 
 def make_slugs_unique(apps, schema_editor):
-    Category = apps.get_model("categories", "Category")
     duplicates = Category.tree.values("slug").annotate(slug_count=models.Count("slug")).filter(slug_count__gt=1)
+    category_objs = []
     for duplicate in duplicates:
         slug = duplicate["slug"]
         categories = Category.tree.filter(slug=slug)
@@ -13,9 +15,10 @@ def make_slugs_unique(apps, schema_editor):
         i = 0
         for category in categories.all():
             if i != 0:
-                category.slug = "%s_%s" % (slug, str(i).zfill(len(str(count))))
-                category.save()
+                category.slug = "{}-{}".format(slug, str(i).zfill(len(str(count))))
+                category_objs.append(category)
             i += 1
+    Category.objects.bulk_update(category_objs, ['slug'])
 
 
 class Migration(migrations.Migration):
diff --git a/categories/tests/test_migrations.py b/categories/tests/test_migrations.py
index 4c506f0..4866b20 100644
--- a/categories/tests/test_migrations.py
+++ b/categories/tests/test_migrations.py
@@ -25,19 +25,19 @@ if sys.version_info >= (3, 0):
                 list(Category.tree.values_list("slug", flat=True)),
                 [
                     "foo",
-                    "foo_1",
-                    "foo_2",
+                    "foo-1",
+                    "foo-2",
                     "bar",
-                    "bar_01",
-                    "bar_02",
-                    "bar_03",
-                    "bar_04",
-                    "bar_05",
-                    "bar_06",
-                    "bar_07",
-                    "bar_08",
-                    "bar_09",
-                    "bar_10",
+                    "bar-01",
+                    "bar-02",
+                    "bar-03",
+                    "bar-04",
+                    "bar-05",
+                    "bar-06",
+                    "bar-07",
+                    "bar-08",
+                    "bar-09",
+                    "bar-10",
                     "baz",
                 ],
             )
-- 
2.36.1

[PetrDlouhy]

@epicserve Thank you for the patch, I applied it. I also added some note to CHANGELOG.md. Should there be any more info about this change?

Thank you for the guide link, I didn't know about this. But I am not sure what else to set, because I already have that option enabled:

[PetrDlouhy]

Seems like bulk_update is not present in Django 2.1 which we are still testing for. Should we drop support for that or make a fallback in the migration?

[epicserve]

@PetrDlouhy, your changes look good to me. Regarding allowing edits to PRs, I'm not sure what is happening. It could be that I'm no longer a maintainer of this project. @coordt, do you know?

Regarding removing support for 2.1, that's fine with me. Especially since it's a new 2.0 version. According to the Django docs, only 3.2 and newer are supported by Django. I think it's good to match what Django supports.

@coordt, do you have any thoughts on this and we can get this merged in and made into a release?

[PetrDlouhy]

@epicserve Can we get this merged? Is there any work left?

[epicserve]

@PetrDlouhy, probably. It would need to be a 2.0 release, I think.

[PetrDlouhy]

@epicserve Are there any plans for the 2.0 release?

[epicserve]

@PetrDlouhy I don't personally have plans. I think it would be great to get your contributions merged in, though.

I'm unsure who is steering the ship anymore since Jazzband took over. @coordt, do you know?

[PetrDlouhy]

I have at least rebased this and simplified the changelog.

[PetrDlouhy]

Since I have gained permissions to release new version and I have already released cleanup version 1.9.3, I also released this code as 2.0.0a1. So anyone involved, please test that version.