Merge pull request #608 from TOMToolkit/feature/alias_testing

fix alias bugs

tom_targets/models.py

@@ -35,7 +35,12 @@
 
 
 class TargetMatchManager(models.Manager):
-    """Search for matches amongst Target names and Aliases"""
+    """
+    Search for matches amongst Target names and Aliases
+    Return Queryset containing relevant TARGET matches.
+    NOTE: check_for_fuzzy_match looks for Target names and aliases ignoring capitalization, spaces, dashes, underscores,
+    and parentheses. Additional matching functions can be added.
+    """
     def check_for_fuzzy_match(self, name):
         """
         Check for case-insensitive names ignoring spaces, dashes, underscore, and parentheses.
@@ -289,18 +294,23 @@ def save(self, *args, **kwargs):
 
     def validate_unique(self, *args, **kwargs):
         """
-        Ensures that Target.name and all aliases of the target are unique. Called automatically on save.
+        Ensures that Target.name and all aliases of the target are unique.
+        Called automatically when checking form.is_valid().
+        Should call Target.full_clean() to validate before save.
         """
         super().validate_unique(*args, **kwargs)
         # Check DB for similar target/alias names.
         matches = Target.matches.check_for_fuzzy_match(self.name)
         for match in matches:
+            # Ignore the fact that this target's name matches itself.
             if match.id is not self.id:
                 raise ValidationError(f'Target with Name or alias similar to {self.name} already exists')
         # Alias Check only necessary when updating target existing target. Reverse relationships require Primary Key.
-        if self.pk:
+        # If nothing has changed for the Target, do not validate against existing aliases.
+        if self.pk and self.name != Target.objects.get(pk=self.pk).name:
             for alias in self.aliases.all():
-                if alias.name == self.name:
+                # Check for fuzzy matching
+                if Target.matches.make_simple_name(alias.name) == Target.matches.make_simple_name(self.name):
                     raise ValidationError('Target name and target aliases must be different')
 
     def __str__(self):
@@ -403,29 +413,38 @@ class TargetName(models.Model):
     target = models.ForeignKey(Target, on_delete=models.CASCADE, related_name='aliases')
     name = models.CharField(max_length=100, unique=True, verbose_name='Alias')
     created = models.DateTimeField(
-        auto_now_add=True, help_text='The time which this target name was created.'
+        auto_now_add=True, help_text='The time at which this target name was created.'
     )
     modified = models.DateTimeField(
         auto_now=True, verbose_name='Last Modified',
-        help_text='The time which this target name was changed in the TOM database.'
+        help_text='The time at which this target name was changed in the TOM database.'
     )
 
     def __str__(self):
         return self.name
 
     def validate_unique(self, *args, **kwargs):
         """
-        Ensures that Target.name and all aliases of the target are unique. Called automatically on save.
+        Ensures that Target.name and all aliases of the target are unique.
+        Called automatically when checking form.is_valid().
+        Should call TargetName.full_clean() to validate before save.
         """
         super().validate_unique(*args, **kwargs)
+        # If nothing has changed for the alias, skip rest of uniqueness validation.
+        # We do not want to fail validation for existing objects, only newly added/updated ones.
+        if self.pk and self.name == TargetName.objects.get(pk=self.pk).name:
+            # Skip remaining uniqueness validation.
+            return
+
+        # If Alias name matches Target name, Return error
+        if self.name == self.target.name:
+            raise ValidationError(f'Alias {self.name} has a conflict with the primary name of the target. '
+                                  f'(target_id={self.target.id})')
+
         # Check DB for similar target/alias names.
         matches = Target.matches.check_for_fuzzy_match(self.name)
         if matches:
-            if matches[0] == self.target:
-                raise ValidationError(f'Alias {self.name} has a conflict with the primary name of the target. '
-                                      f'(target_id={self.target.id})')
-            else:
-                raise ValidationError(f'Target with Name or alias similar to {self.name} already exists')
+            raise ValidationError(f'Target with Name or alias similar to {self.name} already exists')
 
 
 class TargetExtra(models.Model):

tom_targets/tests/tests.py

@@ -6,6 +6,7 @@
 from django.contrib.messages.constants import SUCCESS, WARNING
 from django.test import TestCase, override_settings
 from django.urls import reverse
+from django.core.exceptions import ValidationError
 
 from .factories import SiderealTargetFactory, NonSiderealTargetFactory, TargetGroupingFactory, TargetNameFactory
 from tom_targets.models import Target, TargetExtra, TargetList, TargetName
@@ -448,6 +449,7 @@ def test_create_targets_with_conflicting_names(self):
         for i, name in enumerate(names):
             target_data[f'aliases-{i}-name'] = name
         self.client.post(reverse('targets:create'), data=target_data, follow=True)
+        # Create same target again
         second_response = self.client.post(reverse('targets:create'), data=target_data, follow=True)
         self.assertContains(second_response, 'Target with this Name already exists')
 
@@ -473,6 +475,7 @@ def test_create_targets_with_conflicting_aliases(self):
         for i, name in enumerate(names):
             target_data[f'aliases-{i}-name'] = name
         self.client.post(reverse('targets:create'), data=target_data, follow=True)
+        # Create target with different name, same aliases
         target_data['name'] = 'multiple_names_target2'
         second_response = self.client.post(reverse('targets:create'), data=target_data, follow=True)
         self.assertContains(second_response, 'Target name with this Alias already exists.')
@@ -499,6 +502,7 @@ def test_create_target_name_conflicting_with_existing_aliases(self):
         for i, name in enumerate(names):
             target_data[f'aliases-{i}-name'] = name
         self.client.post(reverse('targets:create'), data=target_data, follow=True)
+        # New target with name the same as different target alias
         target_data['name'] = 'John'
         target_data['aliases-TOTAL_FORMS'] = 0
         for i, name in enumerate(names):
@@ -527,6 +531,7 @@ def test_create_target_alias_conflicting_with_existing_target_name(self):
             'aliases-MAX_NUM_FORMS': 1000,
         }
         self.client.post(reverse('targets:create'), data=target_data, follow=True)
+        # Create Target with new name, but alias matches previous target
         names = ['John', 'Doe']
         for i, name in enumerate(names):
             target_data[f'aliases-{i}-name'] = name
@@ -556,6 +561,7 @@ def test_create_target_alias_conflicting_with_target_name(self):
         names = ['John', 'Doe']
         for i, name in enumerate(names):
             target_data[f'aliases-{i}-name'] = name
+        # Create target with alias same as name
         response = self.client.post(reverse('targets:create'), data=target_data, follow=True)
         self.assertTrue(Target.objects.filter(name=target_data['name']).exists())
         target = Target.objects.get(name=target_data['name'])
@@ -688,10 +694,149 @@ def test_invalid_name_update(self):
             'name': 'testtargetname2',
         })
         response = self.client.post(reverse('targets:update', kwargs={'pk': self.target.id}), data=self.form_data)
-        self.assertContains(response, 'Target name and target aliases must be different')
         # Show name change was unsuccessful
         self.target.refresh_from_db()
         self.assertTrue(self.target.name, 'testtarget')
+        # Check for Error Message
+        self.assertContains(response, 'Target name and target aliases must be different')
+
+    def test_2nd_valid_alias_update(self):
+        self.form_data.update({
+            'targetextra_set-TOTAL_FORMS': 1,
+            'targetextra_set-INITIAL_FORMS': 0,
+            'targetextra_set-MIN_NUM_FORMS': 0,
+            'targetextra_set-MAX_NUM_FORMS': 1000,
+            'aliases-TOTAL_FORMS': 2,
+            'aliases-INITIAL_FORMS': 0,
+            'aliases-MIN_NUM_FORMS': 0,
+            'aliases-MAX_NUM_FORMS': 1000,
+            'aliases-0-name': 'testtargetname2',
+            'aliases-0-id': '',
+            'aliases-1-name': '',
+            'aliases-1-id': ''
+        })
+        # Add Alias
+        self.client.post(reverse('targets:update', kwargs={'pk': self.target.id}), data=self.form_data)
+        self.target.refresh_from_db()
+        self.assertTrue(self.target.aliases.filter(name='testtargetname2').exists())
+        # Create 2nd Alias
+        old_alias = TargetName.objects.get(name='testtargetname2')
+        self.form_data.update({'aliases-INITIAL_FORMS': 1,
+                               'aliases-0-id': old_alias.pk,
+                               'aliases-1-name': 'totally_different_name',
+                               'aliases-1-id': ''})
+        self.client.post(reverse('targets:update', kwargs={'pk': self.target.id}), data=self.form_data)
+        self.target.refresh_from_db()
+        self.assertTrue(self.target.aliases.filter(name='totally_different_name').exists())
+
+    def test_raw_update_process(self):
+        """Series of escalating tests to direct DB submissions"""
+        # Add new valid alias
+        new_alias_data = {'name': "alias1",
+                          'target': self.target}
+        new_alias = TargetName(**new_alias_data)
+        new_alias.full_clean()
+        new_alias.save()
+        self.assertTrue(new_alias_data['name'] in self.target.names)
+
+        # Add new invalid alias that fuzzy matches Target Name
+        new_alias_data = {'name': "test Target",
+                          'target': self.target}
+        new_alias = TargetName(**new_alias_data)
+        with self.assertRaises(ValidationError):
+            new_alias.full_clean()
+
+        # Add new invalid alias that fuzzy matched Alias
+        new_alias_data = {'name': "Alias_1",
+                          'target': self.target}
+        new_alias = TargetName(**new_alias_data)
+        with self.assertRaises(ValidationError):
+            new_alias.full_clean()
+
+        # Add new valid alias
+        new_alias_data = {'name': "alias2",
+                          'target': self.target}
+        new_alias = TargetName(**new_alias_data)
+        new_alias.full_clean()
+        new_alias.save()
+        self.assertTrue(new_alias_data['name'] in self.target.names)
+
+        # Update target name to fuzzy match to old name
+        self.assertTrue('testtarget' in self.target.names)
+        new_name = "test_target"
+        self.target.name = new_name
+        self.target.full_clean()
+        self.target.save()
+        self.assertTrue(new_name in self.target.names)
+        self.assertFalse('testtarget' in self.target.names)
+
+        # Update target name to fuzzy match to existing alias
+        self.assertTrue('test_target' in self.target.names)
+        new_name = "alias (1)"
+        self.target.name = new_name
+        with self.assertRaises(ValidationError):
+            self.target.full_clean()
+
+        # Add new valid alias with invalid alias in DB
+        bad_alias_data = {'name': "Test Target",
+                          'target': self.target}
+        bad_alias = TargetName(**bad_alias_data)
+        bad_alias.save()
+        self.assertTrue(bad_alias_data['name'] in self.target.names)
+        new_alias_data = {'name': "alias3",
+                          'target': self.target}
+        new_alias = TargetName(**new_alias_data)
+        new_alias.full_clean()
+        new_alias.save()
+        self.assertTrue(new_alias_data['name'] in self.target.names)
+
+        # Change existing valid alias to invalid alias that fuzzy conflicts with alias name
+        new_alias.name = "alias (2)"
+        with self.assertRaises(ValidationError):
+            new_alias.full_clean()
+
+        # Add new invalid target with name fuzzy matching existing target name
+        bad_target_data = {
+            'name': 'testtarget',
+            'type': Target.SIDEREAL,
+            'ra': 10,
+            'dec': -22.1
+        }
+        bad_target = Target(**bad_target_data)
+        with self.assertRaises(ValidationError):
+            bad_target.full_clean()
+
+        # Add new invalid target with name fuzzy matching existing alias
+        bad_target.name = "Alias__1"
+        with self.assertRaises(ValidationError):
+            bad_target.full_clean()
+
+        # Add new valid target
+        new_target_data = {
+            'name': 'newtesttarget',
+            'type': Target.SIDEREAL,
+            'ra': 10,
+            'dec': -22.1
+        }
+        new_target = Target(**new_target_data)
+        new_target.full_clean()
+        new_target.save()
+        self.assertTrue(Target.objects.filter(name=new_target_data['name']).exists())
+
+        # Add new valid alias to new target
+        new_alias_data = {'name': "new_target_alias",
+                          'target': new_target}
+        new_alias = TargetName(**new_alias_data)
+        new_alias.full_clean()
+        new_alias.save()
+        self.assertTrue(new_alias_data['name'] in new_target.names)
+
+        # Add invalid alias to new target that fuzzy matches main target alias
+        new_alias_data = {'name': "aLiAs2",
+                          'target': new_target}
+        new_alias = TargetName(**new_alias_data)
+        with self.assertRaises(ValidationError):
+            new_alias.full_clean()
 
 
 class TestTargetImport(TestCase):

tom_targets/views.py

@@ -191,10 +191,8 @@ def form_valid(self, form):
         """
         super().form_valid(form)
 
-        extra = TargetExtraFormset(self.request.POST)
-        extra.instance = self.object
-        names = TargetNamesFormset(self.request.POST)
-        names.instance = self.object
+        extra = TargetExtraFormset(self.request.POST, instance=self.object)
+        names = TargetNamesFormset(self.request.POST, instance=self.object)
 
         if extra.is_valid() and names.is_valid():
             extra.save()
@@ -260,6 +258,7 @@ def form_valid(self, form):
         :param form: Form data for target update
         :type form: subclass of TargetCreateForm
         """
+        super().form_valid(form)
         extra = TargetExtraFormset(self.request.POST, instance=self.object)
         names = TargetNamesFormset(self.request.POST, instance=self.object)
         if extra.is_valid() and names.is_valid():
@@ -271,7 +270,6 @@ def form_valid(self, form):
             form.add_error(None, names.errors)
             form.add_error(None, names.non_form_errors())
             return super().form_invalid(form)
-        super().form_valid(form)
         return redirect(self.get_success_url())
 
     def get_queryset(self, *args, **kwargs):