-
Notifications
You must be signed in to change notification settings - Fork 293
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.
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
Fix COFA validation for USStateField #303
Conversation
Codecov Report
@@ Coverage Diff @@
## master #303 +/- ##
=======================================
Coverage 95.82% 95.82%
=======================================
Files 153 153
Lines 3878 3878
Branches 517 517
=======================================
Hits 3716 3716
Misses 98 98
Partials 64 64
Continue to review full report at Codecov.
|
22d87d9
to
a984b6a
Compare
Is it possible or does it make sense to add some sort of test? |
I'm not against it. If it checked that all fields in the dropdown validate, it would have caught this bug. |
That sounds good. I'll cherry-pick this change to the 1.5.x branch as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR needs a test before it can be merged. Thanks.
@elliottcf Are you still interested in contributing this fix? If so, it would be great to get a test for this change so that I can merge it in. Thanks. |
Yes, I am. I've just been busy with other things so I haven't gotten around to working on the tests you suggested. |
No problem, I know the feeling. :-) It would be nice to get this into the 2.0 release which I'm hoping to release before the end of the year (see #322). I can always add this fix for 2.0.x and 1.6.x if it doesn't make it into 2.0. Thanks for your help with this. |
Adding a test for this change would need tests for all of STATES_NORMALIZED
and this can be added in another PR if desired.
The USPS codes for COFA states aren't in the
STATES_NORMALIZED
dictionary so they won't validate correctly in aUSStateField
form field.I should have caught this in my previous PR, sorry.