-
Notifications
You must be signed in to change notification settings - Fork 70
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
[ARP PoA submission] (#2) Lighthouse 2122 submission (#99310) #20641
Conversation
5cfd7c9
to
490e3d0
Compare
08c3f04
to
760aa9e
Compare
490e3d0
to
2c83434
Compare
2c83434
to
fd2c474
Compare
bb62f04
to
81c62b8
Compare
81c62b8
to
376a2b6
Compare
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.
Don't use double (which defeats the request from actually happening and getting captured by VCR). Also use full match_requests_on
options (e.g. body explained a little here in weirdly organized VCR docs).
Actually, you'd probably run into a really crappy way in which bearer token filtering defeats the headers
part of the VCR matching. But this is an example where the headers aren't worth matching because they aren't dynamic in a way that is correlated to the program's input. So you could leave headers
out of the matching when that bearer token filtering happens.
Full matching (or as full as you can get) and asserting about the entire response together constitute a test that fully checks inputs and outputs. All this is the best practice way to do VCR based specs (which is why we have a spec helper for default full matching in a forthcoming PR).
|
||
context 'successful submit' do | ||
it 'submits the correct data to lighthouse' do | ||
allow(Common::Client::Base).to receive(:configuration).and_return lh_config |
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 double is stopping a VCR-mediated request from being made. I think we would completely test this functionality with just using using VCR with strictest request matching and not doing the extra work of making these manual receive expectations.
You can see what is meant by "strictest request matching" here in an spec helper we're adding to the ARP module in an upcoming PR (we have a tiny fix PR that adds body
to the request matching which we forgot first time around).
Since this is outside the ARP module (and that code wasn't introduced yet anyway), you could just merge in these VCR options directly:
{ match_requests_on: %i[method uri headers body] }
Then we can remove the double.
expect(lh_config).to receive(:post).with( | ||
'1012666183V089914/2122', expected_data, 'lh_client_id', 'key_path', {} | ||
) | ||
VCR.use_cassette('lighthouse/benefits_claims/power_of_attorney_decision/202_response.yml') do |
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.
Minor point but removing the .yml
extension in cassette name will result in .yml
cassette rather than _yml.yml
cassette.
context 'rep does not have poa for veteran' do | ||
it 'returns a not_found response' do | ||
@service = BenefitsClaims::Service.new('1012666183V089914') | ||
VCR.use_cassette('lighthouse/benefits_claims/power_of_attorney_decision/404_response.yml') do |
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.
Minor point but removing the .yml
extension in cassette name will result in .yml
cassette rather than _yml.yml
cassette.
'1012666183V089914/2122', expected_data, 'lh_client_id', 'key_path', {} | ||
) | ||
VCR.use_cassette('lighthouse/benefits_claims/power_of_attorney_decision/202_response.yml') do | ||
@service.submit2122(attributes, 'lh_client_id', 'key_path') |
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 make a complete assertion about the entire response object and status and stuff here.
- Added LH methods for 2122 submit (#99310) - Testing for LH submit2122 - Added VCR files, rubocop - allow pass-thru of all attributes
376a2b6
to
4852ce1
Compare
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.
LGTM
* Add LH methods for 2122 submit (#99310) - Added LH methods for 2122 submit (#99310) - Testing for LH submit2122 - Added VCR files, rubocop - allow pass-thru of all attributes * Changes per Oren review
This is PR No. 2 of a stack concerning ticket #99310 and #101919
This code handles the methods added to consume the LH API for the form submission and RSpec tests. The controller action, and code that creates our local
PowerOfAttorneyFormSubmission
object are being moved to the next PR.Summary
This work is behind a feature toggle (flipper): YES/NO Yes
(Summarize the changes that have been made to the platform)
Updated endpoint for submission of 2122 form to submit decision to Lighthouse
(If bug, how to reproduce) feature
(What is the solution, why is this the solution?)
⭐ Implement POA Establishment Method In Lighthouse API Client va.gov-team#99310
(Which team do you work for, does your team own the maintenance of this component?) ARF
(If introducing a flipper, what is the success criteria being targeted?)
Related issue(s)
Testing done
New code is covered by unit tests
Describe what the old behavior was prior to the change
method did not exist
Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing
If this work is behind a flipper:
Screenshots
Note: Optional
What areas of the site does it impact?
(Describe what parts of the site are impacted andifcode touched other areas)
AR portal
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?