Skip to content

Don't store importer errors in the database #1886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
RickMohr merged 1 commit into from
Jan 9, 2015
Merged

Don't store importer errors in the database #1886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 53 additions & 46 deletions opentreemap/importer/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

@steventlamb steventlamb Jan 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider e -> _e

Copy link
Contributor Author

@RickMohr RickMohr Jan 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try that, and thought it hurt readability. So I'm leaving it even though _e is more correct.

_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
Expand All @@ -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)

######################################
Expand All @@ -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)
33 changes: 24 additions & 9 deletions opentreemap/importer/models/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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]

Expand Down Expand Up @@ -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}])

Expand Down
2 changes: 1 addition & 1 deletion opentreemap/importer/templates/importer/partials/file_status.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<div class="col-md-12">
<div class="well">
<h4>{% trans "There were errors in the file structure: " %} </h4>
{% with errors=ie.errors_as_array %}
{% with errors=ie.errors_array_with_messages %}
{% if errors %}
<ul class="no-bullet">
{% for error in errors %}
Expand Down
21 changes: 5 additions & 16 deletions opentreemap/importer/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ def test_fatal_error(self):
})


class FileLevelTreeValidationTest(TestCase):
class FileLevelTreeValidationTest(ValidationTest):
def write_csv(self, stuff):
t = tempfile.NamedTemporaryFile()

Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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']))


Expand Down
7 changes: 2 additions & 5 deletions opentreemap/importer/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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':
Expand Down