-
Notifications
You must be signed in to change notification settings - Fork 63
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 26589 v2 itf request format #12688
Conversation
…s inside data and attributes
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.
👍 for the extra tests to validate the new, expected format
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 😄
thank you
@@ -133,7 +142,7 @@ def claimant_ssn_equals_vet_ssn?(options) | |||
end | |||
|
|||
def get_bgs_type(params) | |||
itf_types[params[:type].downcase] | |||
params[:data] ? itf_types[params[:data][:attributes][:type].downcase] : itf_types[params[:type].downcase] |
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.
Sorry, I'm late to this party. But this code looks like it could call .downcase
on nil
, and crash with a undefined method error?
Looks like you have some tests build around that scenario, so maybe the error is raised before it would crash.
- non-blocking comment, assuming you'll fix it if needed.
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 is possible. if we don't include attributes and only include type (ex: { data: { type: '' }}
), it will throw this nil
error. i think we could prevent this in a few ways, but maybe updating validate_request_format
is the easiest approach? Ultimately, validate_request!
should prevent all of this, if I'm understanding correctly.
35a58dd
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.
Summary
Updates controller and RSpec tests for new request object format.
Related issue(s)
API-26589
Testing done
RSpec
What areas of the site does it impact?
Acceptance criteria
Requested Feedback