Local census records fixes #3773
Conversation
Senen
force-pushed
the
local-census-records-fixes
branch
from
7129f5b
to
d0b1e47
Compare
app/models/local_census_record.rb
Outdated
| before_validation :sanitize | ||
|
|
||
| validates :document_number, presence: true | ||
| validates :document_type, presence: true | ||
| validates :document_type, inclusion: { in: ALLOWED_DOCUMENT_TYPES.values }, | ||
| if: -> { document_type.present? } |
There was a problem hiding this comment.
Just curious: why is this if condition necessary? 🤔
There was a problem hiding this comment.
Good question @javierm, what i was trying to do is to not validate inclussion when document_type is empty, but i think a better way to do the same should be to use allow_blank validation option to achieve the same.
validates :document_type, presence: true, inclusion: { in: ALLOWED_DOCUMENT_TYPES.values, allow_blank: true }
The inclussion validation is only needed when running import of local census records or if anyone is creating records from rails console, to help the user to understand what values are the accepted ones. This problems does not exists when an administrator creates local census records through the web interface cause we are forcing to select document_typefrom correct values always.
What do you think? Maybe you prefer to run both validation always?
There was a problem hiding this comment.
Finally update inclussion validation with allow_blank option set to true in place of using an if condition.
There was a problem hiding this comment.
I've just realized document_type cannot be empty because the line above this one says:
validates :document_type, presence: trueSo the allow_blank option doesn't seem necessary 🤔. I've tried creating a record without a document type and I get a validation error because of the presence condition.
So what do you think about:
validates :document_type, presence: true, inclusion: { in: ["1", "2", "3"] }There was a problem hiding this comment.
Yes, of course, but there is a little difference between both versions. Without allow_blank option model will have 2 validation errors when the document_type is empty, the will see:
- can't be blank
2 is not included in the list, allowed values are 1 for DNI, 2 for Passport and 3 for Residence Card
When the allow_blank option is present and set to true the user will see only one validation error at a time.
There was a problem hiding this comment.
Interesting! I didn't know that.
Senen
force-pushed
the
local-census-records-fixes
branch
6 times, most recently
from
f248a6b
to
157ef91
Compare
* Add custom message for inclusion validation to include the allowed values. * Force user to choose document_type from select lik the one shown at verification form. * Convert stored document_type to a human readable text
Senen
force-pushed
the
local-census-records-fixes
branch
from
157ef91
to
a150f21
Compare
…records-fixes Local census records fixes
References
Related pull request: #3479, #3500
Closes #3742
Objectives
Regarding LocalCensusRecord CRUD interface
Display a select with allowed document types in place of a text field to make easier to users to create correct local census records.
Regarding local census records import
Add a validation to detect invalid document_types provided through the CSV import process.
Visual Changes
Regarding LocalCensusRecord CRUD interface
To check previous version screenshots got to #3479.
Regarding local census records import
To check previous version screenshots got to #3500.
Notes
None