give webapps a separate slug namespace (bug 673581)

apps/addons/models.py

@@ -113,7 +113,10 @@ class Addon(amo.models.OnChangeMixin, amo.models.ModelBase):
                do_dictsort(settings.LANGUAGES)]
 
     guid = models.CharField(max_length=255, unique=True, null=True)
-    slug = models.CharField(max_length=30, unique=True)
+    slug = models.CharField(max_length=30, unique=True, null=True)
+    # This column is only used for webapps, so they can have a slug namespace
+    # separate from addons and personas.
+    app_slug = models.CharField(max_length=30, unique=True, null=True)
     name = TranslatedField()
     default_locale = models.CharField(max_length=10,
                                       default=settings.LANGUAGE_CODE,
@@ -247,19 +250,16 @@ class Addon(amo.models.OnChangeMixin, amo.models.ModelBase):
 
     def __init__(self, *args, **kw):
         super(Addon, self).__init__(*args, **kw)
-        # Make sure all add-ons have a slug.  save() runs clean_slug.
-        if self.id and not self.slug:
-            self.save()
         self._first_category = {}
 
     def save(self, **kw):
         self.clean_slug()
-        log.info('Setting new slug: %s => %s' % (self.id, self.slug))
         super(Addon, self).save(**kw)
 
     @use_master
-    def clean_slug(self):
-        if not self.slug:
+    def clean_slug(self, slug_field='slug'):
+        slug = getattr(self, slug_field, None)
+        if not slug:
             if not self.name:
                 try:
                     name = Translation.objects.filter(id=self.name_id)[0]
@@ -267,23 +267,24 @@ class Addon(amo.models.OnChangeMixin, amo.models.ModelBase):
                     name = str(self.id)
             else:
                 name = self.name
-            self.slug = slugify(name)[:27]
-        if BlacklistedSlug.blocked(self.slug):
-            self.slug += "~"
-        qs = Addon.objects.values_list('slug', 'id')
-        match = qs.filter(slug=self.slug)
+            slug = slugify(name)[:27]
+        if BlacklistedSlug.blocked(slug):
+            slug += '~'
+        qs = Addon.objects.values_list(slug_field, 'id')
+        match = qs.filter(**{slug_field: slug})
         if match and match[0][1] != self.id:
             if self.id:
-                prefix = '%s-%s' % (self.slug[:-len(str(self.id))], self.id)
+                prefix = '%s-%s' % (slug[:-len(str(self.id))], self.id)
             else:
-                prefix = self.slug
+                prefix = slug
             slugs = dict(qs.filter(slug__startswith='%s-' % prefix))
             slugs.update(match)
             for idx in range(len(slugs)):
                 new = ('%s-%s' % (prefix, idx + 1))[:30]
                 if new not in slugs:
-                    self.slug = new
-                    return
+                    slug = new
+                    break
+        setattr(self, slug_field, slug)
 
     @transaction.commit_on_success
     def delete(self, msg):

apps/amo/fixtures/base/addontag.json

@@ -68,6 +68,7 @@
         "pk": 8680,
         "model": "addons.addon",
         "fields": {
+            "slug": "8680",
             "dev_agreement": 1,
             "eula": null,
             "_current_version": 86740,

apps/webapps/models.py

@@ -2,11 +2,10 @@ import amo
 from addons.models import Addon
 
 
-# I don't know if we'll want to inherit and extend the Addon class so we'll
-# start with a proxy for now.
+# We use super(Addon, self) on purpose to override expectations in Addon that
+# are not true for Webapp. Webapp is just inheriting so it can share the db
+# table.
 class Webapp(Addon):
-
-    # TODO: give apps a separate slug namespace from add-ons.
     # TODO: find a place to store the app version number.
 
     class Meta:
@@ -15,4 +14,9 @@ class Webapp(Addon):
     def save(self, **kw):
         # Make sure we have the right type.
         self.type = amo.ADDON_WEBAPP
-        super(Webapp, self).save(**kw)
+        self.clean_slug(slug_field='app_slug')
+        creating = not self.id
+        super(Addon, self).save(**kw)
+        # Set the slug once we have an id to keep things in order.
+        if creating:
+            self.update(slug='app-%s' % self.id)

apps/webapps/tests/test_models.py

@@ -0,0 +1,33 @@
+import test_utils
+from nose.tools import eq_
+
+import amo
+from addons.models import Addon, BlacklistedSlug
+from webapps.models import Webapp
+
+
+class TestWebapp(test_utils.TestCase):
+
+    def test_webapp_type(self):
+        webapp = Webapp()
+        webapp.save()
+        eq_(webapp.type, amo.ADDON_WEBAPP)
+
+    def test_app_slugs_separate_from_addon_slugs(self):
+        addon = Addon.objects.create(type=1, slug='slug')
+        webapp = Webapp(app_slug='slug')
+        webapp.save()
+        eq_(webapp.slug, 'app-%s' % webapp.id)
+        eq_(webapp.app_slug, 'slug')
+
+    def test_app_slug_collision(self):
+        Webapp(app_slug='slug').save()
+        w2 = Webapp(app_slug='slug')
+        w2.save()
+        eq_(w2.app_slug, 'slug-1')
+
+    def test_app_slug_blocklist(self):
+        BlacklistedSlug.objects.create(name='slug')
+        w = Webapp(app_slug='slug')
+        w.save()
+        eq_(w.app_slug, 'slug~')

migrations/217-app-slugs.sql

@@ -0,0 +1,3 @@
+ALTER TABLE addons
+  ADD COLUMN `app_slug` varchar(30) default NULL,
+  ADD CONSTRAINT UNIQUE KEY `app_slug` (`app_slug`);