-
Notifications
You must be signed in to change notification settings - Fork 66
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
API-22858: Add supplemental claim form v3 fields and data (part 1) #11866
Conversation
Generated by 🚫 Danger |
@@ -130,7 +130,7 @@ def claimant? | |||
end | |||
|
|||
def domestic_phone? | |||
phone_country_code == '1' | |||
phone_country_code.blank? || phone_country_code == '1' |
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.
I'm not sure it's correct to change this here, but I noticed that this would consider phone numbers without an explicit country code to be international.
This test case fails if this change isn't made - I could fix this in the FormData logic instead, but I thought I'd try here first. Happy to update if this change seems too risky (or if this is actually the intended behavior)
# an extension, we put it in the international field instead. | ||
signing_appellant.domestic_phone? && signing_appellant.phone_data['phoneNumberExt'].blank? | ||
end | ||
|
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.
These data getters for phones and signatures return nil when they should not be used, even if data is technically available - I found that this helped avoid putting some logic in the form structure code (see my older PR linked in the description)
signing_appellant_zip_code: { at: [290.7, 393.4], width: 82, height: 14.7 }, | ||
international_phone: { at: [324, 361], width: 200, height: 14 }, | ||
signing_appellant_email: { at: [-1, 333.7], width: 524, height: 12.9 }, | ||
contestable_issues: Structure::MAX_ISSUES_ON_MAIN_FORM.times.map do |i| |
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.
The only reason this isn't failing tests by referring to the Strucure
is that I don't have tests for boxes
- this value will come in my next PR - see how that will look in my original PR here, which was closed for being too long
Summary
Related issue(s)
Testing done
What areas of the site does it impact?
None yet
Acceptance criteria