Skip to content

Don't store importer errors in the database#1886

Merged
RickMohr merged 1 commit intoOpenTreeMap:masterfrom
RickMohr:no-error-msgs-in-db
Jan 9, 2015
Merged

Don't store importer errors in the database#1886
RickMohr merged 1 commit intoOpenTreeMap:masterfrom
RickMohr:no-error-msgs-in-db

Conversation

@RickMohr
Copy link
Contributor

@RickMohr RickMohr commented Jan 8, 2015

  • 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

@steventlamb
Copy link
Contributor

(pulled down the branch and did some grepping)

With this change, errors_as_array is used almost entirely inside models/base.py. There is one exception:
https://github.com/RickMohr/OTM2/blob/bf06f682b7a8071c8fe53c41a719430cd86917ff/opentreemap/importer/views.py#L534

It was probably always a good idea for this mutation logic to be factored into a model method, but it makes even more sense now. Consider factoring this out to a model method like GenericImportRow.strip_errors_by_code and making errors_as_array a "private" method, ie _errors_as_array. The more encapsulation of error processing, the easier it is to reason about.

Copy link
Contributor

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

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.

@steventlamb
Copy link
Contributor

On initial review, this looks like a nice, clean way to handle this. I'll +1 after I run the branch and look at some db rows.

@RickMohr
Copy link
Contributor Author

RickMohr commented Jan 9, 2015

@steventlamb, other suggestions addressed.

@steventlamb
Copy link
Contributor

+1 I tested that this is reading and writing error messages correctly. This is as expected for new records. For existing records, I took a tree import row with errors on three fields that was already in my database, it had msg attributes for these errors. I corrected one error and inspected the DB and the msg attributes were gone. Excellent.

@steventlamb steventlamb assigned RickMohr and unassigned steventlamb Jan 9, 2015
* 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
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) when pulling cc8bc80 on RickMohr:no-error-msgs-in-db into ba59ccb on OpenTreeMap:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) when pulling cc8bc80 on RickMohr:no-error-msgs-in-db into ba59ccb on OpenTreeMap:master.

RickMohr added a commit that referenced this pull request Jan 9, 2015
Don't store importer errors in the database
@RickMohr RickMohr merged commit a8e1105 into OpenTreeMap:master Jan 9, 2015
@RickMohr RickMohr deleted the no-error-msgs-in-db branch January 9, 2015 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants