LG-9825: Add back-end validation for the DOB on the state id form#8501
LG-9825: Add back-end validation for the DOB on the state id form#8501dawei-nava merged 25 commits intomainfrom
Conversation
and test cases. changelog: Internal, In-Person Proofing, Add DOB validation for minimal age.
dawei-nava
force-pushed
the
dwang/LG-9825-dob-validation
branch
from
8f3e122 to
60faf4d
Compare
tomas-nava
changed the title
| @@ -0,0 +1,37 @@ | |||
| module UspsInPersonProofing | |||
| # Validator that can be attached to a form or other model | |||
| # to verify that specific supported fields are comparable to other dates. | |||
There was a problem hiding this comment.
This validator is to make sure a user's age is within an accepted range right? Would it be more accurate/clear to say "...to verify that user input in supported date fields fall within an accepted range".
There was a problem hiding this comment.
@svalexander, it not necessarily a single range, can be combinations of following:
greater_than, greater_than_or_equal_to, equal_to, less_than, less_than_or_equal_to and other_than.
|
|
||
| # @param [String,Date,#to_hash] param | ||
| # @return [Date] | ||
| # It's caller's responsibility to ensure the param contains required entries |
There was a problem hiding this comment.
i'm not quite sure I understand this comment. what are required entries?
There was a problem hiding this comment.
@svalexander, the param can be a type of ActionController::Parameters from state id form submission, where it contains 3 keys, year, month, day. Here we treat it as a hash, so if you pass in anything can be treated as a hash(.to_hash method), it assumes that these three entries exists, it's caller's responsibility to make sure they exists to avoid error.
There was a problem hiding this comment.
ahh ok thank you for clarifying
NavaTim
left a comment
NavaTim
left a comment
There was a problem hiding this comment.
Main issues are to correct the test file naming and ensure that the tests are using the values that they're setting. Other changes are recommended, but I'd be willing to approve without them.
There was a problem hiding this comment.
Typo in file name - should be address_form_spec.rb.
| good_params[:same_address_as_id] = false | ||
| result = subject.submit(bad_params) |
There was a problem hiding this comment.
You're setting a value on good_params here but instead using bad_params.
| good_params[:same_address_as_id] = false | ||
| result = subject.submit(bad_params) |
There was a problem hiding this comment.
Similar to the above:
You're setting a value on
good_paramshere but instead usingbad_params.
| let(:good_params) do | ||
| { | ||
| address1: Faker::Address.street_address, | ||
| address2: Faker::Address.secondary_address, | ||
| zipcode: Faker::Address.zip_code, | ||
| state: Faker::Address.state_abbr, | ||
| city: Faker::Address.city, | ||
| } | ||
| end | ||
| let(:invalid_char) { '$' } | ||
| let(:bad_params) do | ||
| { | ||
| address1: invalid_char + Faker::Address.street_address, | ||
| address2: invalid_char + Faker::Address.secondary_address + invalid_char, | ||
| zipcode: Faker::Address.zip_code, | ||
| state: Faker::Address.state_abbr, | ||
| city: invalid_char + Faker::Address.city, | ||
| } | ||
| end |
There was a problem hiding this comment.
It would be good to wrap these in an additional context indicating that these values and the following tests pertain specifically to transliteration. i.e. especially since the usage of good/bad here could otherwise seem ambiguous.
| expect(subject.errors.to_hash).to include(:address1, :address2, :city) | ||
| expect(result).to be_kind_of(FormResponse) | ||
| expect(result.success?).to be(false) | ||
| expect(result.errors.empty?).to be(true) |
There was a problem hiding this comment.
It would be best to intentionally include at least one error unrelated to transliteration in this test in order to ensure that the combined behavior is correct.
| expect(subject.errors[:first_name]).to eq [ | ||
| I18n.t( | ||
| 'in_person_proofing.form.state_id.errors.unsupported_chars', | ||
| char_list: [invalid_char].join(', '), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
It would be good here to add an additional line or test confirming that the result errors don't include the transliteration error.
There was a problem hiding this comment.
@NavaTim addressed all comments on tests.
NavaTim
left a comment
NavaTim
left a comment
There was a problem hiding this comment.
The PR works, but I recommend restricting the usage of the # prefix to usages like:
context '#submit' doi.e. only in a context/describe block (not it/test), and only when the string name passed exactly matches the name of the method being tested.
| before(:each) do | ||
| allow(IdentityConfig.store).to receive(:usps_ipp_transliteration_enabled).and_return(false) | ||
| end | ||
| it '#submit success with good params' do |
There was a problem hiding this comment.
| it '#submit success with good params' do | |
| it 'submit success with good params' do |
| expect(result.success?).to be(true) | ||
| end | ||
|
|
||
| it '#submit success with good params' do |
There was a problem hiding this comment.
| it '#submit success with good params' do | |
| it 'submit success with good params' do |
| expect(result.success?).to be(true) | ||
| end | ||
|
|
||
| it '#submit success with bad params' do |
There was a problem hiding this comment.
| it '#submit success with bad params' do | |
| it 'submit success with bad params' do |
| before(:each) do | ||
| allow(IdentityConfig.store).to receive(:usps_ipp_transliteration_enabled).and_return(true) | ||
| end | ||
| it '#submit success with good params' do |
There was a problem hiding this comment.
| it '#submit success with good params' do | |
| it 'submit success with good params' do |
| expect(result).to be_kind_of(FormResponse) | ||
| expect(result.success?).to be(true) | ||
| end | ||
| it '#submit failure with bad params' do |
There was a problem hiding this comment.
| it '#submit failure with bad params' do | |
| it 'submit failure with bad params' do |
| end | ||
| context 'when capture_secondary_id_enabled is true' do | ||
| let(:subject) { described_class.new(capture_secondary_id_enabled: true) } | ||
| it '#submit with missing same_address_as_id should be successful' do |
There was a problem hiding this comment.
| it '#submit with missing same_address_as_id should be successful' do | |
| it 'submit with missing same_address_as_id should be successful' do |
| end | ||
| context 'when capture_secondary_id_enabled is false' do | ||
| let(:subject) { described_class.new(capture_secondary_id_enabled: false) } | ||
| it '#submit with missing same_address_as_id will fail' do |
There was a problem hiding this comment.
| it '#submit with missing same_address_as_id will fail' do | |
| it 'submit with missing same_address_as_id will fail' do |
7 participants
🎫 Ticket LG9825 Backend validation of DOB.
🛠 Summary of changes
Validate the DOB for minimum age requirement on the state id form.
📜 Testing Plan
Provide a checklist of steps to confirm the changes.
Time.zone.today - 200.years👀 Screenshots
Disable both frontend and backend validation:
After restore backend validation