Merge pull request #526 from Cadasta/enhancement/#324

Resolve #324: Prevent duplicate org and project names

cadasta/organization/forms.py

@@ -132,10 +132,9 @@ def clean_urls(self):
         return self.to_list(self.data.get('urls'))
 
     def clean_name(self):
-        is_create = not self.instance.id
         name = self.cleaned_data['name']
         invalid_names = settings.CADASTA_INVALID_ENTITY_NAMES
-        if is_create and slugify(name, allow_unicode=True) in invalid_names:
+        if slugify(name, allow_unicode=True) in invalid_names:
             raise forms.ValidationError(
                 _("Organization name cannot be “Add” or “New”."))
         return name
@@ -265,10 +264,25 @@ def __init__(self, *args, **kwargs):
 
     def clean_name(self):
         name = self.cleaned_data['name']
+
+        # Check that name is not restricted
         invalid_names = settings.CADASTA_INVALID_ENTITY_NAMES
         if slugify(name, allow_unicode=True) in invalid_names:
             raise forms.ValidationError(
                 _("Project name cannot be “Add” or “New”."))
+
+        # Check that name is unique org-wide
+        # (Explicit validation because we are using a wizard and the
+        # unique_together validation cannot occur in the proper page)
+        not_unique = Project.objects.filter(
+            organization__slug=self.cleaned_data['organization'],
+            name=name,
+        ).exists()
+        if not_unique:
+            raise forms.ValidationError(
+                _("Project with this name already exists "
+                  "in this organization."))
+
         return name
 
 
@@ -301,6 +315,29 @@ def save(self, *args, **kwargs):
 
         return super().save(*args, **kwargs)
 
+    def clean_name(self):
+        name = self.cleaned_data['name']
+
+        # Check that name is not restricted
+        invalid_names = settings.CADASTA_INVALID_ENTITY_NAMES
+        if slugify(name, allow_unicode=True) in invalid_names:
+            raise forms.ValidationError(
+                _("Project name cannot be “Add” or “New”."))
+
+        # Check that name is unique org-wide
+        # (Explicit validation because we are using a wizard and the
+        # unique_together validation cannot occur in the proper page)
+        not_unique = Project.objects.filter(
+            organization__slug=self.instance.organization.slug,
+            name=name,
+        ).exclude(id=self.instance.id).exists()
+        if not_unique:
+            raise forms.ValidationError(
+                _("Project with this name already exists "
+                  "in this organization."))
+
+        return name
+
 
 class PermissionsForm(SuperUserCheck):
     def check_admin(self, user):

cadasta/organization/migrations/0002_unique_org_project_names.py

@@ -0,0 +1,75 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+import itertools
+import math
+from django.db import migrations, models
+
+
+def make_org_names_unique(apps, schema_editor):
+    Organization = apps.get_model('organization', 'Organization')
+    names = {}
+    max_length = Organization._meta.get_field('name').max_length
+    for org in Organization.objects.all():
+        orig_name = org.name
+        for x in itertools.count(1):
+            if not org.name in names:
+                names[org.name] = True
+                org.save()
+                break
+            name_length = max_length - int(math.log10(x)) - 2
+            trunc_name = orig_name[:name_length]
+            org.name = '{} {}'.format(trunc_name, x)
+
+def make_project_names_unique(apps, schema_editor):
+    Project = apps.get_model('organization', 'Project')
+    names = {}
+    max_length = Project._meta.get_field('name').max_length
+    for project in Project.objects.all():
+        if not project.organization in names:
+            names[project.organization] = {}
+        orig_name = project.name
+        for x in itertools.count(1):
+            if not project.name in names[project.organization]:
+                names[project.organization][project.name] = True
+                project.save()
+                break
+            name_length = max_length - int(math.log10(x)) - 2
+            trunc_name = orig_name[:name_length]
+            project.name = '{} {}'.format(trunc_name, x)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('organization', '0001_initial'),
+    ]
+
+    operations = [
+
+        # Run database modifications to ensure that names are unique
+        migrations.RunPython(
+            make_org_names_unique,
+            reverse_code=migrations.RunPython.noop
+        ),
+        migrations.RunPython(
+            make_project_names_unique,
+            reverse_code=migrations.RunPython.noop
+        ),
+
+        # Run actual database schema modifications
+        migrations.AlterField(
+            model_name='historicalorganization',
+            name='name',
+            field=models.CharField(db_index=True, max_length=200),
+        ),
+        migrations.AlterField(
+            model_name='organization',
+            name='name',
+            field=models.CharField(max_length=200, unique=True),
+        ),
+        migrations.AlterUniqueTogether(
+            name='project',
+            unique_together=set([('organization', 'name')]),
+        ),
+    ]

cadasta/organization/models.py

@@ -30,7 +30,7 @@ def get_policy_instance(policy_name, variables):
 
 @permissioned_model
 class Organization(SlugModel, RandomIDModel):
-    name = models.CharField(max_length=200)
+    name = models.CharField(max_length=200, unique=True)
     slug = models.SlugField(max_length=50, unique=True)
     description = models.TextField(null=True, blank=True)
     archived = models.BooleanField(default=False)
@@ -175,6 +175,7 @@ class Project(ResourceModelMixin, SlugModel, RandomIDModel):
 
     class Meta:
         ordering = ('organization', 'name')
+        unique_together = ('organization', 'name')
 
     class TutelaryMeta:
         perm_type = 'project'

cadasta/organization/serializers.py

@@ -28,9 +28,8 @@ class Meta:
         detail_only_fields = ('users',)
 
     def validate_name(self, value):
-        is_create = not self.instance
         invalid_names = settings.CADASTA_INVALID_ENTITY_NAMES
-        if is_create and slugify(value, allow_unicode=True) in invalid_names:
+        if slugify(value, allow_unicode=True) in invalid_names:
             raise serializers.ValidationError(
                 _("Organization name cannot be “Add” or “New”."))
         return value
@@ -53,11 +52,33 @@ class ProjectSerializer(DetailSerializer, serializers.ModelSerializer):
     country = CountryField(required=False)
 
     def validate_name(self, value):
-        is_create = not self.instance
+
+        # Check that name is not restricted
         invalid_names = settings.CADASTA_INVALID_ENTITY_NAMES
-        if is_create and slugify(value, allow_unicode=True) in invalid_names:
+        if slugify(value, allow_unicode=True) in invalid_names:
             raise serializers.ValidationError(
                 _("Project name cannot be “Add” or “New”."))
+
+        # Check that name is unique org-wide
+        # (Explicit validation: see comment in the Meta class)
+        is_create = not self.instance
+        if is_create:
+            org_slug = self.context['organization'].slug
+        else:
+            org_slug = self.instance.organization.slug
+        queryset = Project.objects.filter(
+            organization__slug=org_slug,
+            name=value,
+        )
+        if is_create:
+            not_unique = queryset.exists()
+        else:
+            not_unique = queryset.exclude(id=self.instance.id).exists()
+        if not_unique:
+            raise serializers.ValidationError(
+                _("Project with this name already exists "
+                  "in this organization."))
+
         return value
 
     class Meta:
@@ -67,6 +88,11 @@ class Meta:
         read_only_fields = ('id', 'country', 'slug')
         detail_only_fields = ('users',)
 
+        # Suppress automatic model-derived UniqueTogetherValidator because
+        # organization is a read-only field in the serializer.
+        # Instead, perform this validation explicitly in validate_name()
+        validators = []
+
     def create(self, validated_data):
         organization = self.context['organization']
         return Project.objects.create(

cadasta/organization/tests/test_forms.py

@@ -52,17 +52,34 @@ def test_add_organization(self):
         assert org.slug == 'org'
         assert OrganizationRole.objects.filter(organization=org).count() == 1
 
-    def test_unique_slugs(self):
+    def test_duplicate_name_error(self):
         self.data['description'] = 'Org description #1'
         self._save(self.data)
         org1 = Organization.objects.first()
         assert org1.slug == 'org'
 
         self.data['description'] = 'Org description #2'
-        self._save(self.data, count=2)
-        orgs = Organization.objects.all()
-        assert len(orgs) == 2
-        assert orgs[0].slug != orgs[1].slug
+        form = forms.OrganizationForm(self.data, user=UserFactory.create())
+        assert form.is_valid() is False
+        assert form.errors == {
+            'name': ["Organization with this Name already exists."]
+        }
+        assert Organization.objects.count() == 1
+
+    # NOTE: This test is no longer possible because unique org names masks
+    # the testing of unique org slugs via OrganizationForm. Unique org slugs
+    # can only be tested via model unit tests.
+    # def test_unique_slugs(self):
+    #     self.data['description'] = 'Org description #1'
+    #     self._save(self.data)
+    #     org1 = Organization.objects.first()
+    #     assert org1.slug == 'org'
+    #
+    #     self.data['description'] = 'Org description #2'
+    #     self._save(self.data, count=2)
+    #     orgs = Organization.objects.all()
+    #     assert len(orgs) == 2
+    #     assert orgs[0].slug != orgs[1].slug
 
     def test_add_organization_with_url(self):
         self.data['urls'] = 'http://example.com'
@@ -118,6 +135,16 @@ def test_update_organization(self):
         assert org.description == self.data['description']
         assert org.slug == 'some-org'
 
+    def test_update_organization_with_restricted_name(self):
+        org = OrganizationFactory.create(slug='some-org')
+        invalid_names = ('add', 'ADD', 'Add', 'new', 'NEW', 'New')
+        self.data['name'] = random.choice(invalid_names)
+        form = forms.OrganizationForm(self.data, instance=org)
+        assert not form.is_valid()
+        assert form.errors == {
+            'name': ["Organization name cannot be “Add” or “New”."]
+        }
+
     def test_remove_all_contacts(self):
         org = OrganizationFactory.create(
             slug='some-org',
@@ -267,6 +294,52 @@ def test_add_new_project_with_restricted_name(self):
             'name': ["Project name cannot be “Add” or “New”."]
         }
 
+    def test_add_new_project_with_duplicate_name(self):
+        org = OrganizationFactory.create()
+        user = UserFactory.create()
+        OrganizationRole.objects.create(
+            organization=org, user=user, admin=True
+        )
+        existing_project = ProjectFactory.create(
+            organization=org,
+            name="Same Project",
+        )
+        data = {
+            'organization': org.slug,
+            'name': existing_project.name,
+            'contacts-TOTAL_FORMS': 1,
+            'contacts-INITIAL_FORMS': 0,
+            'contacts-0-name': '',
+            'contacts-0-email': '',
+            'contacts-0-tel': ''
+        }
+        form = forms.ProjectAddDetails(data=data, user=user)
+        assert not form.is_valid()
+        assert form.errors == {
+            'name': [
+                "Project with this name already exists in this organization."]
+        }
+
+    def test_add_new_project_with_duplicate_name_in_another_org(self):
+        org_1 = OrganizationFactory.create()
+        org_2 = OrganizationFactory.create()
+        user = UserFactory.create()
+        OrganizationRole.objects.create(
+            organization=org_2, user=user, admin=True
+        )
+        existing_project = ProjectFactory.create(organization=org_1)
+        data = {
+            'organization': org_2.slug,
+            'name': existing_project.name,
+            'contacts-TOTAL_FORMS': 1,
+            'contacts-INITIAL_FORMS': 0,
+            'contacts-0-name': '',
+            'contacts-0-email': '',
+            'contacts-0-tel': ''
+        }
+        form = forms.ProjectAddDetails(data=data, user=user)
+        assert form.is_valid()
+
 
 @pytest.mark.usefixtures('make_dirs')
 class ProjectEditDetailsTest(UserTestCase):
@@ -378,6 +451,47 @@ def test_delete_questionnaire(self):
         assert project.name == data['name']
         assert not project.current_questionnaire
 
+    def test_update_project_with_restricted_name(self):
+        project = ProjectFactory.create()
+        invalid_names = ('add', 'ADD', 'Add', 'new', 'NEW', 'New')
+        data = {
+            'name': random.choice(invalid_names),
+            'contacts-TOTAL_FORMS': 1,
+            'contacts-INITIAL_FORMS': 0,
+            'contacts-0-name': '',
+            'contacts-0-email': '',
+            'contacts-0-tel': ''
+        }
+        form = forms.ProjectEditDetails(
+            instance=project,
+            data=data,
+        )
+        assert not form.is_valid()
+        assert form.errors == {
+            'name': ["Project name cannot be “Add” or “New”."]
+        }
+
+    def test_update_project_with_duplicate_name(self):
+        project_1 = ProjectFactory.create()
+        project_2 = ProjectFactory.create(organization=project_1.organization)
+        data = {
+            'name': project_1.name,
+            'contacts-TOTAL_FORMS': 1,
+            'contacts-INITIAL_FORMS': 0,
+            'contacts-0-name': '',
+            'contacts-0-email': '',
+            'contacts-0-tel': ''
+        }
+        form = forms.ProjectEditDetails(
+            instance=project_2,
+            data=data,
+        )
+        assert not form.is_valid()
+        assert form.errors == {
+            'name': [
+                "Project with this name already exists in this organization."]
+        }
+
 
 class UpdateProjectRolesTest(UserTestCase):
     def setUp(self):