Adding father_alive field to importer and fixtures #149
Conversation
| mandatory: false | ||
| type: String | ||
| - field: father_cause_of_death | ||
| column: G | ||
| column: H |
There was a problem hiding this comment.
Seeing this makes me wonder how we can avoid the future pains of reformatting the entire importer config every time a change needs to be made. And as a more immediate concern: should there be a validation on the settings to ensure at least that the sequence of column names is not broken?
motasem-salem
force-pushed
the
update_importer
branch
from
e0a5db0 to
5bde386
Compare
| validates :father_alive, inclusion: { in: [true, false] }, exclusion: { in: [nil] } | ||
| validates :father_alive, inclusion: { in: [false] }, exclusion: { in: [true] }, if: :father_is_martyr | ||
| validates :father_alive, inclusion: { in: [true] }, exclusion: { in: [false] }, if: 'father_date_of_death.nil?' | ||
| validates :father_alive, inclusion: { in: [false] }, exclusion: { in: [true] }, unless: 'father_date_of_death.nil?' |
There was a problem hiding this comment.
This feels awkward to me. What are peoples thoughts on this?
There was a problem hiding this comment.
Does father_alive need to be in the db? Or could it be calculated as needed by a method from the other attributes
father_alive= !(father_is_martyr || father_date_of_death)
There was a problem hiding this comment.
validates inclusion in: -> { [false, !(father_is_martyr || !(father_date_of_death.nil?)) ] }
This covers the edge case where he isn't alive, isn't martyr, and the date of death is unknown
There was a problem hiding this comment.
Doesn't inclusion: { in: [false] } automatically handle exclusion: { in: [anything else] }?
Would (Right, right, Booleans. But is exclusion: { in: [nil] } not be better done with presence: true?exclusion still necessary?)
Overall, I agree with the awkwardness - perhaps everything related to father's status would be better handled by a custom validator(s).
@theB264: I think it needs to be in the db to ensure record validity: if date of death is not recorded, does that mean that it's missing or that the father is alive? It seems that OSRA do not operate under the assumption that the date of death can be unknown. I would also prefer clarity over conciseness in the validation.
There was a problem hiding this comment.
@nik :+1 for the explanation on why it needs to be in the db. I was going to reply with something similar to @theB264.
I started with a plan to write a custom validator that takes these 3 attributes (father_alive, father_is_martyr & father_date_of_death) and validates their value combination. But then I thought it might be better to use existing one-liner validations. This is the result (which I didn't like much). I submitted it to get your feedback.
I can take all of that out and have a custom validator which will be later used by pending_orphan and importer as well. What do you guys think?
There was a problem hiding this comment.
Especially if it's going to be reused by other models, extracting all this into a custom validator sounds like a terrific idea.
There was a problem hiding this comment.
@motasem-salem Please see #162 and my WIP on the Father model. Perhaps at least some of this refactoring can be done now, and it would address the concerns raised here.
|
Superseded by #179 |
Jira story: https://osraav.atlassian.net/browse/OSRA-221
Jira tasks:
OSRA-224#Code-Review
OSRA-226#Code-Review