-
Notifications
You must be signed in to change notification settings - Fork 65
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 42951 poa v2 zip code updates #19665
Conversation
* Updates schema * updates validations * Adjusts/add tests modified: modules/claims_api/app/controllers/claims_api/v2/veterans/power_of_attorney/base_controller.rb modified: modules/claims_api/app/controllers/concerns/claims_api/v2/power_of_attorney_validation.rb modified: modules/claims_api/config/schemas/v2/2122.json modified: modules/claims_api/config/schemas/v2/2122a.json modified: modules/claims_api/spec/requests/v2/veterans/power_of_attorney/2122_spec.rb modified: modules/claims_api/spec/requests/v2/veterans/power_of_attorney/2122a_spec.rb modified: modules/claims_api/spec/requests/v2/veterans/power_of_attorney/power_of_attorney_request_spec.rb
Generated by 🚫 Danger |
* Updates documentation to reflect changes for zipCode modified: modules/claims_api/app/swagger/claims_api/v2/dev/swagger.json modified: modules/claims_api/app/swagger/claims_api/v2/production/swagger.json
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.
tested V2 2122 & 2122a. LGTM
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.
looking good so far. are we using country
or countryCode
? seems to be a mix of both throughout.
modules/claims_api/app/controllers/concerns/claims_api/v2/power_of_attorney_validation.rb
Outdated
Show resolved
Hide resolved
modules/claims_api/app/controllers/concerns/claims_api/v2/power_of_attorney_validation.rb
Outdated
Show resolved
Hide resolved
modules/claims_api/app/controllers/concerns/claims_api/v2/power_of_attorney_validation.rb
Outdated
Show resolved
Hide resolved
modules/claims_api/app/controllers/concerns/claims_api/v2/power_of_attorney_validation.rb
Outdated
Show resolved
Hide resolved
modules/claims_api/app/controllers/concerns/claims_api/v2/power_of_attorney_validation.rb
Outdated
Show resolved
Hide resolved
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.
we should resolve country vs countryCode before merging.
Yep totally got some bleed over on that term from the other PR with that update in it. Should have just merged in order but now that https://github.com/department-of-veterans-affairs/vets-api/pull/19650[)](https://github.com/department-of-veterans-affairs/vets-api/commit/4c52087c513b9296c62326dd81f57b925e29eb78) has merged in can utilize Also address the other comments:
Added new screenshots in PR |
* Fixes misplaced `countryCode` usage * Adjusts missing missing spaces in the docs * Compiles docs w/ updates * Refactors `validate_address_zip_code` with `validate_zip` method to remove code duplication * A few other small linting fixes based on PR feedback modified: modules/claims_api/app/controllers/concerns/claims_api/v2/power_of_attorney_validation.rb modified: modules/claims_api/app/swagger/claims_api/v2/dev/swagger.json modified: modules/claims_api/app/swagger/claims_api/v2/production/swagger.json modified: modules/claims_api/config/schemas/v2/2122a.json
…t update to go in to finish
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.
looks great!
# the Claimant object is already being validatated below, | ||
# so that code has been adjusted and it is being left out of this workflow |
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.
We shouldnʼt need the comment now that the method name is self-descriptive.
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.
ah, yep cabc789
Summary
zipCode
is only required now when thecountryCode
is 'US'Related issue(s)
API-42951
Testing done
Testing Notes
countryCode
andzipCode
are taken out/put in will test the behaviorScreenshots
2122
2122a
What areas of the site does it impact?
Acceptance criteria
Requested Feedback
(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?