Merge pull request #266 from fasrc/cp_formvalidation_hotfix

Allocation form validation fixes

coldfront/core/allocation/forms.py

@@ -150,18 +150,7 @@ def clean(self):
                 [d[:3], d[3:8], d[8:12], d[12:18], d[18:24], d[24:28], d[28:33]]
             )
             cleaned_expensecode = insert_dashes(replace_productcode(digits_only(expense_code)))
-            if 'ifxbilling' in settings.INSTALLED_APPS:
-                try:
-                    matched_fiineaccts = FiineAPI.listAccounts(code=cleaned_expensecode)
-                    if not matched_fiineaccts:
-                        self.add_error(
-                            "expense_code",
-                            "expense code not found in system - please check the code or get in touch with a system administrator."
-                        )
-                except Exception:
-                    #Not authorized to use accounts_list
-                    pass
-                cleaned_data['expense_code'] = cleaned_expensecode
+            cleaned_data['expense_code'] = cleaned_expensecode
         elif existing_expense_codes and existing_expense_codes != '------':
             cleaned_data['expense_code'] = existing_expense_codes
         return cleaned_data

coldfront/core/allocation/models.py

@@ -532,20 +532,29 @@ def clean(self):
 
         expected_value_type = self.allocation_attribute_type.attribute_type.name.strip()
         error = None
-        if expected_value_type == 'Float' and not isinstance(literal_eval(self.value), (float,int)):
-            error = 'Value must be a float.'
-        elif expected_value_type == 'Int' and not isinstance(literal_eval(self.value), int):
-            error = 'Value must be an integer.'
-        elif expected_value_type == 'Yes/No' and self.value not in ['Yes', 'No']:
+        if expected_value_type in ['Float', 'Int']:
+            try:
+                literal_val = literal_eval(self.value)
+            except SyntaxError as exc:
+                error = 'Value must be entirely numeric. Please remove any non-numeric characters.'
+                raise ValidationError(
+                    f'Invalid Value "{self.value}" for "{self.allocation_attribute_type.name}". {error}'
+                ) from exc
+            if expected_value_type == 'Float' and not isinstance(literal_val, (float,int)):
+                error = 'Value must be a float.'
+            elif expected_value_type == 'Int' and not isinstance(literal_val, int):
+                error = 'Value must be an integer.'
+        elif expected_value_type == "Yes/No" and self.value not in ["Yes", "No"]:
             error = 'Allowed inputs are "Yes" or "No".'
-        elif expected_value_type == 'Date':
+        elif expected_value_type == "Date":
             try:
                 datetime.datetime.strptime(self.value.strip(), '%Y-%m-%d')
             except ValueError:
                 error = 'Date must be in format YYYY-MM-DD'
         if error:
             raise ValidationError(
-                'Invalid Value "%s" for "%s". %s' % (self.value, self.allocation_attribute_type.name, error))
+                f'Invalid Value "{self.value}" for "{self.allocation_attribute_type.name}". {error}'
+            )
 
     def __str__(self):
         return str(self.allocation_attribute_type.name)

coldfront/core/allocation/tests/test_models.py

@@ -1,20 +1,22 @@
 """Unit tests for the allocation models"""
 
 from django.test import TestCase
+from django.core.exceptions import ValidationError
 
 from coldfront.core.test_helpers.factories import setup_models, AllocationFactory
 
 UTIL_FIXTURES = [
         "coldfront/core/test_helpers/test_data/test_fixtures/ifx.json",
 ]
 
+
 class AllocationModelTests(TestCase):
     """tests for Allocation model"""
     fixtures = UTIL_FIXTURES
 
     @classmethod
     def setUpTestData(cls):
-        """Set up project to test model properties and methods"""
+        """Set up allocation to test model properties and methods"""
         setup_models(cls)
 
     def test_allocation_str(self):
@@ -23,9 +25,9 @@ def test_allocation_str(self):
             self.proj_allocation.get_parent_resource.name,
             self.proj_allocation.project.pi
         )
-
         self.assertEqual(str(self.proj_allocation), allocation_str)
 
+
     def test_allocation_usage_property(self):
         """Test that allocation usage property displays correctly"""
         self.assertEqual(self.proj_allocation.usage, 10)
@@ -34,3 +36,27 @@ def test_allocation_usage_property_na(self):
         """Create allocation with no usage. Usage property should return None"""
         allocation = AllocationFactory()
         self.assertIsNone(allocation.usage)
+
+class AllocationAttributeModelTests(TestCase):
+    """Tests for allocationattribute models"""
+    fixtures = UTIL_FIXTURES
+
+    @classmethod
+    def setUpTestData(cls):
+        """Set up allocationattribute to test model properties and methods"""
+        setup_models(cls)
+        cls.allocationattribute = cls.proj_allocation.allocationattribute_set.get(
+            allocation_attribute_type__name='Storage Quota (TB)'
+        )
+
+    def test_allocationattribute_clean_no_error(self):
+        """cleaning a numeric value for an int or float AllocationAttributeType produces no error"""
+        self.allocationattribute.value = "1000"
+        self.allocationattribute.clean()
+
+    def test_allocationattribute_clean_nonnumeric_error(self):
+        """cleaning a non-numeric value for int or float AllocationAttributeTypes returns an informative error message"""
+
+        self.allocationattribute.value = "1000TB"
+        with self.assertRaisesMessage(ValidationError, 'Value must be entirely numeric. Please remove any non-numeric characters.'):
+            self.allocationattribute.clean()

coldfront/core/allocation/views.py

@@ -68,6 +68,7 @@
 
 
 if 'ifxbilling' in settings.INSTALLED_APPS:
+    from fiine.client import API as FiineAPI
     from ifxbilling.models import Account, UserProductAccount
 if 'django_q' in settings.INSTALLED_APPS:
     from django_q.tasks import Task
@@ -711,7 +712,19 @@ def form_valid(self, form):
             'quantity':quantity,
             'nese': nese,
             'used_percentage': used_percentage,
+            'expense_code': expense_code,
+            'unmatched_code': False,
         }
+
+        if 'ifxbilling' in settings.INSTALLED_APPS:
+            try:
+                matched_fiineaccts = FiineAPI.listAccounts(code=expense_code)
+                if not matched_fiineaccts:
+                    other_vars['unmatched_code'] = True
+            except Exception:
+                #Not authorized to use accounts_list
+                pass
+
         send_allocation_admin_email(
             allocation_obj,
             'New Allocation Request',

coldfront/templates/email/new_allocation_request.txt

@@ -8,6 +8,12 @@ Justification:
 {{resource}} was last recorded as {{used_percentage}}% allocated.
 {% endif %}
 
+{% if unmatched_code %}
+The expense code entered does not match any in the FIINE database.
+Please check it for accuracy and ensure that it is added to FIINE:
+{{expense_code}}
+{% endif %}
+
 {% if nese %}
 Here is a draft ticket to send to NESE: