Replacing vets-json-schema with rails validation for COE Form Submissions#27781
Conversation
Generated by 🚫 Danger |
| { status: e.status, messsage: e.message, body: e.body }) | ||
| { status: e.status, message: e.message, body: e.body }) | ||
| else | ||
| Rails.logger.error('LGY API returned error', { status: e.status, messsage: e.message, body: e.body }) | ||
| Rails.logger.error('LGY API returned error', { status: e.status, message: e.message, body: e.body }) |
There was a problem hiding this comment.
fixing a typo but unrelated to this ticket
…rm-submission-replacing-vets-json-schema
| # Otherwise, submit the claim to the LGY API | ||
| else | ||
| Rails.logger.info('Begin COE claim submission to LGY API', guid:) | ||
| response = lgy_service.put_application(payload: prepare_form_data) | ||
| Rails.logger.info('COE claim submitted to LGY API', guid:, reference_number: response['reference_number']) | ||
|
|
||
| process_attachments! | ||
| response['reference_number'] |
There was a problem hiding this comment.
else statement was unnecessary
There was a problem hiding this comment.
There was a problem hiding this comment.
For some reason this feels like a very, "FE dev" thing to do, but I like it. I would have stuffed it all into a class constant, which would've made reading the source code more difficult
| module Coe | ||
| module ServiceBranch | ||
| CONFIG_PATH = Rails.root.join('config', 'coe', 'service_branch.json') | ||
| TABLE = JSON.parse(CONFIG_PATH.read).freeze |
There was a problem hiding this comment.
feel like there may be a better name than table to describe the json structure but couldn't think of it
There was a problem hiding this comment.
naming things is hard
| it 'sends the right data to LGY' do | ||
| # rubocop:disable Layout/LineLength | ||
| coe_claim = create(:coe_claim, form: '{"relevantPriorLoans":[{"dateRange":{"from":"2017-01-01T00:00:00.000Z","to":""},"propertyAddress":{"propertyAddress1":"234","propertyAddress2":"234","propertyCity":"asdf","propertyState":"AL","propertyZip":"11111"},"propertyOwned":false,"vaLoanNumber":"123123123123","intent":"IRRRL"},{"dateRange":{"from":"2010-01-01T00:00:00.000Z","to":"2011-01-01T00:00:00.000Z"},"propertyAddress":{"propertyAddress1":"939393","propertyAddress2":"234","propertyCity":"asdf","propertyState":"AL","propertyZip":"11111"},"propertyOwned":true,"vaLoanNumber":"123123123123","intent":"REFI"}],"vaLoanIndicator":true,"periodsOfService":[{"serviceBranch":"Air Force","dateRange":{"from":"2000-01-01T00:00:00.000Z","to":"2010-01-16T00:00:00.000Z"}}],"identity":"ADSM","contactPhone":"2223334444","contactEmail":"[email protected]","fullName":{"first":"Eddie","middle":"Joseph","last":"Caldwell"},"dateOfBirth":"1933-10-27","applicantAddress":{"country":"USA","street":"123 ANY ST","city":"ANYTOWN","state":"AL","postalCode":"54321"},"privacyAgreementAccepted":true}') | ||
| coe_claim = create(:coe_claim, form: '{"relevantPriorLoans":[{"dateRange":{"from":"2017-01-01T00:00:00.000Z","to":""},"propertyAddress":{"propertyAddress1":"234","propertyAddress2":"234","propertyCity":"asdf","propertyState":"AL","propertyZip":"11111"},"propertyOwned":false,"vaLoanNumber":"123123123123","intent":"IRRRL"},{"dateRange":{"from":"2010-01-01T00:00:00.000Z","to":"2011-01-01T00:00:00.000Z"},"propertyAddress":{"propertyAddress1":"939393","propertyAddress2":"234","propertyCity":"asdf","propertyState":"AL","propertyZip":"11111"},"propertyOwned":true,"vaLoanNumber":"123123123123","intent":"REFI"}],"vaLoanIndicator":true,"periodsOfService":[{"serviceBranch":"AF","dateRange":{"from":"2000-01-01T00:00:00.000Z","to":"2010-01-16T00:00:00.000Z"}}],"identity":"ADSM","contactPhone":"2223334444","contactEmail":"[email protected]","fullName":{"first":"Eddie","middle":"Joseph","last":"Caldwell"},"dateOfBirth":"1933-10-27","applicantAddress":{"country":"USA","street":"123 ANY ST","city":"ANYTOWN","state":"AL","postalCode":"54321"},"privacyAgreementAccepted":true}') |
There was a problem hiding this comment.
had to update these as the validations were causing failure in other tests
There was a problem hiding this comment.
Wow that is ugly. I don't blame you for just changing "Air Force" -> "AF" to get this one over the line, but it would be good to have a factory function to create different form objects. Would you mind creating a ticket for that, we can throw it on our backlog to do later.
There was a problem hiding this comment.
yeah ill make a ticket for it
| # values from the FE for military_branch are: | ||
| # ["Air Force", "Air Force Reserve", "Air National Guard", "Army", "Army National Guard", "Army Reserve", | ||
| # "Coast Guard", "Coast Guard Reserve", "Marine Corps", "Marine Corps Reserve", "Navy", "Navy Reserve"] | ||
| # these need to be formatted because LGY only accepts [ARMY, NAVY, MARINES, AIR_FORCE, COAST_GUARD, OTHER] | ||
| # and then we have to pass in ACTIVE_DUTY or RESERVE_NATIONAL_GUARD for service_type | ||
| military_branch = service_info['serviceBranch'].parameterize.underscore.upcase | ||
| service_type = 'ACTIVE_DUTY' | ||
|
|
||
| # "Marine Corps" must be converted to "Marines" here, so that the `.any` | ||
| # block below can convert "Marine Corps" and "Marine Corps Reserve" to | ||
| # "MARINES", to meet LGY's requirements. | ||
| military_branch = military_branch.gsub('MARINE_CORPS', 'MARINES') | ||
|
|
||
| %w[RESERVE NATIONAL_GUARD].any? do |service_branch| | ||
| next unless military_branch.include?(service_branch) | ||
|
|
||
| index = military_branch.index('_NATIONAL_GUARD') || military_branch.index('_RESERVE') | ||
| military_branch = military_branch[0, index] | ||
| # "Air National Guard", unlike "Air Force Reserve", needs to be manually | ||
| # transformed to AIR_FORCE here, to meet LGY's requirements. | ||
| military_branch = 'AIR_FORCE' if military_branch == 'AIR' | ||
| service_type = 'RESERVE_NATIONAL_GUARD' |
There was a problem hiding this comment.
Because I neglected to include information in the ticket about feature toggles (sorry), this stuff will need to be reinstated, and an if clause introduced, to use the COE::ServiceBranch helper
| 'vaLoanIndicator' => true, | ||
| 'periodsOfService' => [{ | ||
| 'serviceBranch' => 'Air National Guard', | ||
| 'serviceBranch' => 'ANG', |
There was a problem hiding this comment.
I love that you thought to update the swagger documentation here. I would have forgotten this.
Is the rest of the incoming object really identical, from current version to the stuff that the FE guys have rebuilt? Seems like we should check this (although i am totally fine with putting it into a separate PR)
There was a problem hiding this comment.
yeah ill make a follow up ticket
AdamKing0126
left a comment
There was a problem hiding this comment.
This looks good so far. Once you insert logic for the feature toggle, I will approve.
AdamKing0126
left a comment
There was a problem hiding this comment.
Looks good, no notes. I appreciate your flexibility wrt feature flag stuff.
Keep your PR as a Draft until it's ready for Platform review. A PR is ready for Platform review when it has a teammate approval and tests, linting, and settings checks pass CI. See these tips on how to avoid common delays in getting your PR merged.
Summary
Related issue(s)
2489
Testing done
Screenshots
Note: Optional
What areas of the site does it impact?
(Describe what parts of the site are impacted andifcode touched other areas)
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?