Verified webapp name validates as unique (bug 692965)

- Also updated ReverseNameLookup to use separate keys for webapps.

apps/addons/cron.py

@@ -36,10 +36,12 @@ recs_log = logging.getLogger('z.recs')
 def build_reverse_name_lookup():
     """Builds a Reverse Name lookup table in REDIS."""
     ReverseNameLookup().clear()
+    ReverseNameLookup(webapp=True).clear()
 
     # Get all add-on name ids
     names = (Addon.objects.filter(
-        name__isnull=False, type__in=[amo.ADDON_EXTENSION, amo.ADDON_THEME])
+        name__isnull=False, type__in=[amo.ADDON_EXTENSION, amo.ADDON_THEME,
+                                      amo.ADDON_WEBAPP ])
         .values_list('name_id', 'id'))
 
     for chunk in chunked(names, 100):
@@ -52,14 +54,17 @@ def _build_reverse_name_lookup(names, **kw):
     translations = (Translation.objects.filter(id__in=names)
                     .values_list('id', 'localized_string'))
 
+    addons = dict([a.id, a] for a in
+                  Addon.objects.filter(id__in=names.values()))
+
     if clear:
         for addon_id in names.values():
-            ReverseNameLookup().delete(addon_id)
+            ReverseNameLookup(addons[addon_id].is_webapp()).delete(addon_id)
 
     for t_id, string in translations:
         if string:
-            ReverseNameLookup().add(string, names[t_id])
-
+            (ReverseNameLookup(addons[names[t_id]].is_webapp())
+                .add(string, names[t_id]))
 
 # TODO(jbalogh): removed from cron on 6/27/11. If the site doesn't break,
 # delete it.

apps/addons/forms.py

@@ -34,12 +34,17 @@ log = commonware.log.getLogger('z.addons')
 
 
 def clean_name(name, instance=None):
-    id = ReverseNameLookup().get(name)
+    if not instance:
+        log.debug('clean_name called without an instance: %s' % name)
+    if instance:
+        id = ReverseNameLookup(instance.is_webapp()).get(name)
+    else:
+        id = ReverseNameLookup().get(name)
 
     # If we get an id and either there's no instance or the instance.id != id.
     if id and (not instance or id != instance.id):
-        raise forms.ValidationError(_('This add-on name is already in use.  '
-                                      'Please choose another.'))
+        raise forms.ValidationError(_('This name is already in use. Please '
+                                      'choose another.'))
     return name
 
 

apps/addons/models.py

@@ -1058,8 +1058,9 @@ def update_name_table(sender, **kw):
     from . import cron
     if not kw.get('raw'):
         addon = kw['instance']
-        cron._build_reverse_name_lookup.delay({addon.name_id: addon.id},
-                                              clear=True)
+        if addon.name:
+            cron._build_reverse_name_lookup.delay({addon.name_id: addon.id},
+                                                  clear=True)
 
 
 @receiver(dbsignals.pre_delete, sender=Addon,
@@ -1067,7 +1068,7 @@ def update_name_table(sender, **kw):
 def clear_name_table(sender, **kw):
     if not kw.get('raw'):
         addon = kw['instance']
-        ReverseNameLookup().delete(addon.id)
+        ReverseNameLookup(addon.is_webapp()).delete(addon.id)
 
 
 @receiver(signals.version_changed, dispatch_uid='version_changed')

apps/addons/tests/test_addon_utils.py

@@ -31,3 +31,24 @@ class TestReverseNameLookup(amo.tests.TestCase):
 
     def test_get_case(self):
         eq_(ReverseNameLookup().get('delicious bookmarks'), 3615)
+
+    def test_addon_and_app_namespaces(self):
+        eq_(ReverseNameLookup(webapp=False).get('Delicious Bookmarks'), 3615)
+        eq_(ReverseNameLookup(webapp=True).get('Delicious Bookmarks'), None)
+
+        # Note: The factory creates the app which calls the ReverseNameLookup
+        # in a post_save signal, so no need to call it explicitly here.
+        app = amo.tests.addon_factory(type=amo.ADDON_WEBAPP)
+        self.assertTrue(app.is_webapp())
+
+        eq_(ReverseNameLookup(webapp=False).get(app.name), None)
+        eq_(ReverseNameLookup(webapp=True).get(app.name), app.id)
+
+        # Show we can also create an app with the same name as an addon
+        name = 'Delicious Bookmarks'
+        app = amo.tests.addon_factory(name=name, type=amo.ADDON_WEBAPP)
+        self.assertTrue(app.is_webapp())
+        eq_(ReverseNameLookup(webapp=False).get(name), 3615)
+        eq_(ReverseNameLookup(webapp=True).get(name), app.id)
+
+

apps/addons/tests/test_forms.py

@@ -25,8 +25,7 @@ class FormsTest(amo.tests.TestCase):
         super(FormsTest, self).setUp()
         cron.build_reverse_name_lookup()
         self.existing_name = 'Delicious Bookmarks'
-        self.error_msg = ('This add-on name is already in use. '
-                          'Please choose another.')
+        self.error_msg = 'This name is already in use. Please choose another.'
 
     def test_new(self):
         """

apps/addons/tests/test_views.py

@@ -1668,19 +1668,19 @@ class TestSubmitPersona(amo.tests.TestCase):
         """Make sure name is unique."""
         r = self.client.post(self.url, self.get_dict(name='Cooliris'))
         self.assertFormError(r, 'form', 'name',
-            'This add-on name is already in use. Please choose another.')
+            'This name is already in use. Please choose another.')
 
     def test_submit_name_unique_strip(self):
         """Make sure we can't sneak in a name by adding a space or two."""
         r = self.client.post(self.url, self.get_dict(name='  Cooliris  '))
         self.assertFormError(r, 'form', 'name',
-            'This add-on name is already in use. Please choose another.')
+            'This name is already in use. Please choose another.')
 
     def test_submit_name_unique_case(self):
         """Make sure unique names aren't case sensitive."""
         r = self.client.post(self.url, self.get_dict(name='cooliris'))
         self.assertFormError(r, 'form', 'name',
-            'This add-on name is already in use. Please choose another.')
+            'This name is already in use. Please choose another.')
 
     def test_submit_name_required(self):
         """Make sure name is required."""

apps/addons/utils.py

@@ -20,20 +20,22 @@ rnlog = logging.getLogger('z.rn')
 
 
 class ReverseNameLookup(object):
-    prefix = 'amo:addon:name'
-    names = prefix + ':names'
-    addons = prefix + ':addons'
-    keys = prefix + ':keys'
 
-    def __init__(self):
+    def __init__(self, webapp=False):
         self.redis = redisutils.connections['master']
+        self.type = 'app' if webapp else 'addon'
+        self.prefix = 'amo:%s:name' % self.type
+        self.names = self.prefix + ':names'
+        self.addons = self.prefix + ':addons'
+        self.keys = self.prefix + ':keys'
 
     def add(self, name, addon_id):
         hash = safe_key(name)
         if not self.redis.hsetnx(self.names, hash, addon_id):
-            rnlog.warning('Duplicate name: %s (%s).' % (name, addon_id))
+            rnlog.warning('Duplicate %s name: %s (%s).' % (
+                self.type, name, addon_id))
             return
-        rnlog.info('[%s] has a lock on "%s"' % (addon_id, name))
+        rnlog.info('[%s:%s] has a lock on "%s"' % (self.type, addon_id, name))
         self.redis.sadd('%s:%s' % (self.addons, addon_id), hash)
         self.redis.sadd(self.keys, addon_id)
 
@@ -50,7 +52,7 @@ class ReverseNameLookup(object):
                 self.add(unicode(translation.localized_string), addon.id)
 
     def delete(self, addon_id):
-        rnlog.info('[%s] Releasing locked names.' % addon_id)
+        rnlog.info('[%s:%s] Releasing locked names.' % (self.type, addon_id))
         hashes = self.redis.smembers('%s:%s' % (self.addons, addon_id))
         for hash in hashes:
             self.redis.hdel(self.names, hash)
@@ -58,7 +60,7 @@ class ReverseNameLookup(object):
         self.redis.srem(self.keys, addon_id)
 
     def clear(self):
-        rnlog.info('Clearing the ReverseName table.')
+        rnlog.info('Clearing the %s ReverseName table.' % self.type)
         self.redis.delete(self.names)
         for key in self.redis.smembers(self.keys):
             self.redis.delete(key)

apps/amo/tests/__init__.py

@@ -258,14 +258,14 @@ def addon_factory(version_kw={}, file_kw={}, **kw):
     a.weekly_downloads = random.randint(200, 2000)
     a.created = a.last_updated = datetime(2011, 6, 6, random.randint(0, 23),
                                           random.randint(0, 59))
-    version_factory(file_kw, addon=a, **version_kw)
-    a.update_version()
     a.status = amo.STATUS_PUBLIC
     for key, value in kw.items():
         setattr(a, key, value)
         if key == 'type' and value == amo.ADDON_PERSONA:
             Persona.objects.create(addon_id=a.id, persona_id=a.id)
     a.save()
+    version_factory(file_kw, addon=a, **version_kw)
+    a.update_version()
     return a
 
 

apps/devhub/tests/test_views.py

@@ -1411,19 +1411,50 @@ class TestSubmitStep3(TestSubmitBase):
     def test_submit_name_unique(self):
         # Make sure name is unique.
         r = self.client.post(self.url, self.get_dict(name='Cooliris'))
-        error = 'This add-on name is already in use. Please choose another.'
+        error = 'This name is already in use. Please choose another.'
         self.assertFormError(r, 'form', 'name', error)
 
     def test_submit_name_unique_strip(self):
         # Make sure we can't sneak in a name by adding a space or two.
         r = self.client.post(self.url, self.get_dict(name='  Cooliris  '))
-        error = 'This add-on name is already in use. Please choose another.'
+        error = 'This name is already in use. Please choose another.'
         self.assertFormError(r, 'form', 'name', error)
 
     def test_submit_name_unique_case(self):
         # Make sure unique names aren't case sensitive.
         r = self.client.post(self.url, self.get_dict(name='cooliris'))
-        error = 'This add-on name is already in use. Please choose another.'
+        error = 'This name is already in use. Please choose another.'
+        self.assertFormError(r, 'form', 'name', error)
+
+    def _setup_webapp(self):
+        # Used unique name tests for webapps
+        waffle.models.Flag.objects.create(name='accept-webapps', everyone=True)
+        app = amo.tests.addon_factory(type=amo.ADDON_WEBAPP, name='Cool App')
+        self.assertTrue(app.is_webapp())
+        eq_(ReverseNameLookup(webapp=True).get(app.name), app.id)
+        self.get_addon().update(type=amo.ADDON_WEBAPP)
+
+    def test_submit_app_name_unique(self):
+        self._setup_webapp()
+        url = reverse('devhub.submit_apps.3', args=['a3615'])
+        r = self.client.post(url, self.get_dict(name='Cool App'))
+        error = 'This name is already in use. Please choose another.'
+        self.assertFormError(r, 'form', 'name', error)
+
+    def test_submit_app_name_unique_strip(self):
+        # Make sure we can't sneak in a name by adding a space or two.
+        self._setup_webapp()
+        url = reverse('devhub.submit_apps.3', args=['a3615'])
+        r = self.client.post(url, self.get_dict(name='  Cool App  '))
+        error = 'This name is already in use. Please choose another.'
+        self.assertFormError(r, 'form', 'name', error)
+
+    def test_submit_app_name_unique_case(self):
+        # Make sure unique names aren't case sensitive.
+        self._setup_webapp()
+        url = reverse('devhub.submit_apps.3', args=['a3615'])
+        r = self.client.post(url, self.get_dict(name='cool app'))
+        error = 'This name is already in use. Please choose another.'
         self.assertFormError(r, 'form', 'name', error)
 
     def test_submit_name_required(self):
@@ -2762,8 +2793,7 @@ class TestCreateAddon(BaseUploadTest, UploadAddon, amo.tests.TestCase):
         ReverseNameLookup().add('xpi name', 34)
         r = self.post(expect_errors=True)
         eq_(r.context['new_addon_form'].non_field_errors(),
-            ['This add-on name is already in use. '
-             'Please choose another.'])
+            ['This name is already in use. Please choose another.'])
 
     def test_success(self):
         eq_(Addon.objects.count(), 0)

apps/devhub/tests/test_views_packager.py

@@ -100,7 +100,7 @@ class TestPackager(amo.tests.TestCase):
         data = self._form_data({'name': 'Delicious Bookmarks'})
         r = self.client.post(self.url, data)
         self.assertFormError(r, 'basic_form', 'name',
-            'This add-on name is already in use. Please choose another.')
+            'This name is already in use. Please choose another.')
 
     def test_name_minlength(self):
         data = self._form_data({'name': 'abcd'})