From cc8bc80943c91424c7c638b6772d5ed1a97bdade Mon Sep 17 00:00:00 2001 From: Rick Mohr Date: Thu, 8 Jan 2015 15:50:41 -0500 Subject: [PATCH] Don't store importer errors in the database * Create table of error messages by error code * `append_error` saves only error code, not error message * Add `errors_by_array_with_messages` for situations where messages are needed * Fix a few tests * No data migration needed because error fields are accessed by name, not position --- opentreemap/importer/errors.py | 99 ++++++++++--------- opentreemap/importer/models/base.py | 33 +++++-- .../importer/partials/file_status.html | 2 +- opentreemap/importer/tests.py | 21 +--- opentreemap/importer/views.py | 7 +- 5 files changed, 85 insertions(+), 77 deletions(-) diff --git a/opentreemap/importer/errors.py b/opentreemap/importer/errors.py index 941519e6b..984d8aee2 100644 --- a/opentreemap/importer/errors.py +++ b/opentreemap/importer/errors.py @@ -3,13 +3,19 @@ from __future__ import unicode_literals from __future__ import division -# TODO: stop storing error text in the database in order to -# make lazy translation possible. -# See https://github.com/OpenTreeMap/OTM2/issues/1880 -# from django.utils.translation import ugettext_lazy as trans -trans = lambda x: x +from django.utils.translation import ugettext_lazy as trans + +_messages_by_code = {} + + +def e(code, message, fatal): + _messages_by_code[code] = message + return (code, message, fatal) + + +def get_message(code): + return _messages_by_code[code] -# 3 tuples (error id, error descr, fatal) ###################################### # FILE LEVEL ERRORS @@ -18,10 +24,10 @@ # Errors that are attributed to the file and prevent the # rows from being loaded and validated. # -EMPTY_FILE = (1, trans('No rows found'), True) -UNMATCHED_FIELDS = (3, trans('Unrecognized fields in header row'), False) -MISSING_FIELD = (5, trans('This field is required'), True) -GENERIC_ERROR = ( +EMPTY_FILE = e(1, trans('No rows found'), True) +UNMATCHED_FIELDS = e(3, trans('Unrecognized fields in header row'), False) +MISSING_FIELD = e(5, trans('This field is required'), True) +GENERIC_ERROR = e( 6, trans('An exception was raised while uploading the file'), True) ###################################### @@ -30,55 +36,56 @@ # # Errors that are attributed to rows # -INVALID_GEOM = (10, trans('Longitude must be between -180 and 180 and ' - 'latitude must be betwen -90 and 90'), True) +INVALID_GEOM = e(10, trans('Longitude must be between -180 and 180 and ' + 'latitude must be betwen -90 and 90'), True) -GEOM_OUT_OF_BOUNDS = ( +GEOM_OUT_OF_BOUNDS = e( 11, trans('Geometry must be inside the map bounds'), True) -EXCL_ZONE = (12, trans('Geometry may not be in an exclusion zone'), True) +EXCL_ZONE = e(12, trans('Geometry may not be in an exclusion zone'), True) -INVALID_SPECIES = (20, trans('Could not find species with the given ' - 'scientific name.'), True) -DUPLICATE_SPECIES = (21, trans('More than one species matches the given ' - 'scientific name.'), True) +INVALID_SPECIES = e(20, trans('Could not find species with the given ' + 'scientific name.'), True) +DUPLICATE_SPECIES = e(21, trans('More than one species matches the given ' + 'scientific name.'), True) -INVALID_OTM_ID = (30, trans('The given Open Tree Map ID does not exist ' - 'in the system. This ID is automatically ' - 'generated by Open Tree Map and should only ' - 'be used for updating existing records'), True) +INVALID_OTM_ID = e(30, trans('The given Open Tree Map ID does not exist ' + 'in the system. This ID is automatically ' + 'generated by Open Tree Map and should only ' + 'be used for updating existing records'), True) -FLOAT_ERROR = (40, trans('Not formatted as a number'), True) -POS_FLOAT_ERROR = (41, trans('Not formatted as a positive number'), True) -INT_ERROR = (42, trans('Not formatted as an integer'), True) -POS_INT_ERROR = (43, trans('Not formatted as a positive integer'), True) -BOOL_ERROR = (44, trans('Not formatted as a boolean'), True) -STRING_TOO_LONG = (45, trans('Strings must be less than 255 characters'), True) -INVALID_DATE = (46, trans('Invalid date (must by YYYY-MM-DD)'), True) +FLOAT_ERROR = e(40, trans('Not formatted as a number'), True) +POS_FLOAT_ERROR = e(41, trans('Not formatted as a positive number'), True) +INT_ERROR = e(42, trans('Not formatted as an integer'), True) +POS_INT_ERROR = e(43, trans('Not formatted as a positive integer'), True) +BOOL_ERROR = e(44, trans('Not formatted as a boolean'), True) +STRING_TOO_LONG = e(45, trans('Strings must be less than 255 characters'), + True) +INVALID_DATE = e(46, trans('Invalid date (must by YYYY-MM-DD)'), True) -INVALID_UDF_VALUE = (50, trans('Invalid value for custom field'), True) +INVALID_UDF_VALUE = e(50, trans('Invalid value for custom field'), True) -INVALID_ITREE_REGION = (60, trans('Unknown i-Tree region'), True) -ITREE_REGION_NOT_IN_INSTANCE = ( +INVALID_ITREE_REGION = e(60, trans('Unknown i-Tree region'), True) +ITREE_REGION_NOT_IN_INSTANCE = e( 61, trans('i-Tree region not valid for this treemap'), True) -INVALID_ITREE_CODE = (62, trans('Unknown i-Tree code'), True) -ITREE_CODE_NOT_IN_REGION = ( +INVALID_ITREE_CODE = e(62, trans('Unknown i-Tree code'), True) +ITREE_CODE_NOT_IN_REGION = e( 63, trans('i-Tree code not defined for region'), True) -INSTANCE_HAS_NO_ITREE_REGION = (64, trans('This treemap intersects no ' - 'i-Tree regions and has no ' - 'default region'), True) -INSTANCE_HAS_MULTIPLE_ITREE_REGIONS = ( +INSTANCE_HAS_NO_ITREE_REGION = e(64, trans('This treemap intersects no ' + 'i-Tree regions and has no ' + 'default region'), True) +INSTANCE_HAS_MULTIPLE_ITREE_REGIONS = e( 65, trans('This treemap intersects more than one i-Tree region'), True) -MERGE_REQUIRED = (71, trans('This row must be merged'), False) +MERGE_REQUIRED = e(71, trans('This row must be merged'), False) -NEARBY_TREES = ( +NEARBY_TREES = e( 1050, trans('There are already trees very close to this one'), False) -SPECIES_DBH_TOO_HIGH = (1060, - trans('The diameter is too large for this species'), - False) +SPECIES_DBH_TOO_HIGH = e(1060, + trans('The diameter is too large for this species'), + False) -SPECIES_HEIGHT_TOO_HIGH = (1061, - trans('The height is too large for this species'), - False) +SPECIES_HEIGHT_TOO_HIGH = e(1061, + trans('The height is too large for this species'), + False) diff --git a/opentreemap/importer/models/base.py b/opentreemap/importer/models/base.py index 3ddc2b6f6..ef0f3b3c1 100644 --- a/opentreemap/importer/models/base.py +++ b/opentreemap/importer/models/base.py @@ -120,26 +120,31 @@ def append_error(self, err, data=None): self.errors = '[]' self.errors = json.dumps( - self.errors_as_array() + [ + self._errors_as_array() + [ {'code': code, - 'msg': msg, 'data': data, 'fatal': fatal}]) return self - def errors_as_array(self): + def _errors_as_array(self): if self.errors is None or self.errors == '': return [] else: return json.loads(self.errors) + def errors_array_with_messages(self): + errs = self._errors_as_array() + for error in errs: + error['msg'] = errors.get_message(error['code']) + return errs + def has_errors(self): - return len(self.errors_as_array()) > 0 + return len(self._errors_as_array()) > 0 def has_error(self, error): code, msg, fatal = error - error_codes = {e['code'] for e in self.errors_as_array()} + error_codes = {e['code'] for e in self._errors_as_array()} return code in error_codes def row_set(self): @@ -224,20 +229,31 @@ def datadict(self, v): self.jsondata = v self.data = json.dumps(self.jsondata) - def errors_as_array(self): + def _errors_as_array(self): if self.errors is None or self.errors == '': return [] else: return json.loads(self.errors) + def errors_array_with_messages(self): + errs = self._errors_as_array() + for error in errs: + error['msg'] = errors.get_message(error['code']) + return errs + + def errors_array_without_merge_errors(self): + errs = [e for e in self._errors_as_array() + if e['code'] != errors.MERGE_REQUIRED[0]] + return errs + def has_errors(self): - return len(self.errors_as_array()) > 0 + return len(self._errors_as_array()) > 0 def get_fields_with_error(self): data = {} datadict = self.datadict - for e in self.errors_as_array(): + for e in self._errors_as_array(): for field in e['fields']: data[field] = datadict[field] @@ -266,7 +282,6 @@ def append_error(self, err, fields, data=None): json.loads(self.errors) + [ {'code': code, 'fields': fields, - 'msg': msg, 'data': data, 'fatal': fatal}]) diff --git a/opentreemap/importer/templates/importer/partials/file_status.html b/opentreemap/importer/templates/importer/partials/file_status.html index be531e66e..8a4dc681c 100644 --- a/opentreemap/importer/templates/importer/partials/file_status.html +++ b/opentreemap/importer/templates/importer/partials/file_status.html @@ -5,7 +5,7 @@

{% trans "There were errors in the file structure: " %}

- {% with errors=ie.errors_as_array %} + {% with errors=ie.errors_array_with_messages %} {% if errors %}
    {% for error in errors %} diff --git a/opentreemap/importer/tests.py b/opentreemap/importer/tests.py index 69cf40a2e..e7b2e4179 100644 --- a/opentreemap/importer/tests.py +++ b/opentreemap/importer/tests.py @@ -724,7 +724,7 @@ def test_fatal_error(self): }) -class FileLevelTreeValidationTest(TestCase): +class FileLevelTreeValidationTest(ValidationTest): def write_csv(self, stuff): t = tempfile.NamedTemporaryFile() @@ -754,13 +754,10 @@ def test_empty_file_error(self): self.assertEqual(TreeImportRow.objects.count(), base_rows) self.assertFalse(rslt) - ierrors = json.loads(ie.errors) - # The only error is a bad file error + ierrors = json.loads(ie.errors) self.assertTrue(len(ierrors), 1) - etpl = (ierrors[0]['code'], ierrors[0]['msg'], True) - - self.assertEqual(etpl, errors.EMPTY_FILE) + self.assertHasError(ie, errors.EMPTY_FILE) def test_missing_point_field(self): ie = TreeImportEvent(file_name='file', owner=self.user, @@ -778,12 +775,8 @@ def test_missing_point_field(self): self.assertFalse(rslt) ierrors = json.loads(ie.errors) - - # Should be x/y point error self.assertTrue(len(ierrors), 1) - etpl = (ierrors[0]['code'], ierrors[0]['msg'], True) - - self.assertEqual(etpl, errors.MISSING_FIELD) + self.assertHasError(ie, errors.MISSING_FIELD) def test_unknown_field(self): ie = TreeImportEvent(file_name='file', owner=self.user, @@ -802,12 +795,8 @@ def test_unknown_field(self): self.assertFalse(rslt) ierrors = json.loads(ie.errors) - - # Should be x/y point error self.assertTrue(len(ierrors), 1) - etpl = (ierrors[0]['code'], ierrors[0]['msg'], False) - - self.assertEqual(etpl, errors.UNMATCHED_FIELDS) + self.assertHasError(ie, errors.UNMATCHED_FIELDS) self.assertEqual(set(ierrors[0]['data']), set(['name', 'age'])) diff --git a/opentreemap/importer/views.py b/opentreemap/importer/views.py index 036491924..50c99a996 100644 --- a/opentreemap/importer/views.py +++ b/opentreemap/importer/views.py @@ -334,7 +334,7 @@ def _get_row_data(row, field_names, merge_required): that do not have any errors to produce complete rendering info for a row. """ - row_errors = row.errors_as_array() + row_errors = row.errors_array_with_messages() collected_fields = {} for row_error in row_errors: @@ -531,11 +531,8 @@ def solve(request, instance, import_event_id, row_index): target_species = request.GET['species'] # Strip off merge errors - ierrors = [e for e in row.errors_as_array() - if e['code'] != errors.MERGE_REQUIRED[0]] - #TODO: Json handling is terrible. - row.errors = json.dumps(ierrors) + row.errors = json.dumps(row.errors_array_without_merge_errors()) row.datadict.update(data) if target_species != 'new':