diff --git a/tom_targets/models.py b/tom_targets/models.py index 07fc09cb7..3955cc486 100644 --- a/tom_targets/models.py +++ b/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,11 +413,11 @@ 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): @@ -415,17 +425,26 @@ def __str__(self): 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): diff --git a/tom_targets/tests/tests.py b/tom_targets/tests/tests.py index 36aad8a70..e31b79ef2 100644 --- a/tom_targets/tests/tests.py +++ b/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): diff --git a/tom_targets/views.py b/tom_targets/views.py index f8f545d3e..500c1e472 100644 --- a/tom_targets/views.py +++ b/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):