-
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 24835 526 v2 section 8 direct deposit information validation #12818
Api 24835 526 v2 section 8 direct deposit information validation #12818
Conversation
Generated by 🚫 Danger |
modules/claims_api/app/controllers/concerns/claims_api/v2/disability_compensation_validation.rb
Outdated
Show resolved
Hide resolved
modules/claims_api/app/controllers/concerns/claims_api/v2/disability_compensation_validation.rb
Show resolved
Hide resolved
modules/claims_api/app/controllers/concerns/claims_api/v2/disability_compensation_validation.rb
Show resolved
Hide resolved
modules/claims_api/spec/fixtures/v2/veterans/disability_compensation/form_526_json_api.json
Outdated
Show resolved
Hide resolved
@@ -195,7 +195,7 @@ | |||
}, | |||
"directDeposit": { | |||
"accountType": "CHECKING", | |||
"accountNumber": "123456789", | |||
"accountNumber": "123123123123", | |||
"routingNumber": "123456789", | |||
"financialInstituteName": "Chase", |
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.
Typo in example field name...this should be "financialInstitutionName"
return if direct_deposit.blank? | ||
|
||
direct_deposit['noAccount'] == true ? validate_no_account! : validate_account_values! | ||
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.
Question: What if there is not a value for noAccount
?
irb(main):009:0> form_attributes = { "directDeposit"=> { "accountType"=> 'Checking', 'accountNumber'=> '3453425235' } }
=> {"directDeposit"=>{"accountType"=>"Checking", "accountNumber"=>"3453425235"}}
irb(main):010:0> direct_deposit = form_attributes['directDeposit']
=> {"accountType"=>"Checking", "accountNumber"=>"3453425235"}
irb(main):011:0> direct_deposit['noAccount'] == true ? 'yes' : 'no'
=> "no"
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 think that is okay, just want to run it by you.
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.
you're right, it can be more robust, updating that now. It is kind of unintentionally safe. Since we only get to that point if values are present, if 'noAccount' is not present we still go the right place, but looking at the code it would be better to be more intentional on that safeguarding both for the code and for readability by others.
) * RSpec stes written and validation methods added, waiting on some feedback on questions to continue * Updates schema for discussion in stand up * Updates and validation, leaving in place until standup discussion * Adds direct deposit validation - section 8 API-24835 * Adds validataion for direct deposit elements * Updates 526.json schema file and related files to ensure correct validations * Updates rswag docs with update schema * Adds RSpec tests for validations Changes to be committed: modified: modules/claims_api/app/controllers/concerns/claims_api/v2/disability_compensation_validation.rb modified: modules/claims_api/app/swagger/claims_api/v2/dev/swagger.json modified: modules/claims_api/config/schemas/v2/526.json modified: modules/claims_api/config/schemas/v2/request_bodies/disability_compensation/example.json modified: modules/claims_api/config/schemas/v2/request_bodies/disability_compensation/request.json modified: modules/claims_api/spec/fixtures/v2/veterans/disability_compensation/form_526_json_api.json modified: modules/claims_api/spec/requests/v2/veterans/disability_compensation_request_spec.rb modified: spec/support/schemas/claims_api/v2/forms/disability/submission.json * Fixing my Gemfile.lock include * Rollbacks incorrect inclusions when fixing merge conflict * Merges master, tests running green, ran swager compile * Refactors validation file, strengthens tests up * Fixes strings being passed into the error raised * Updates test value so it matches a valid account number * Fixes account number REGEX after latest merge test was red, tests green and in original state,updates swagger and all related json files * Refactoring in validation file to make check more rebust and give better name to deposit error method
Adds direct deposit validation - section 8
Summary
Related issue(s)
API-24835
Testing done
RSpec
What areas of the site does it impact?
modified: modules/claims_api/app/controllers/concerns/claims_api/v2/disability_compensation_validation.rb
modified: modules/claims_api/app/swagger/claims_api/v2/dev/swagger.json
modified: modules/claims_api/config/schemas/v2/526.json
modified: modules/claims_api/config/schemas/v2/request_bodies/disability_compensation/example.json
modified: modules/claims_api/config/schemas/v2/request_bodies/disability_compensation/request.json
modified: modules/claims_api/spec/fixtures/v2/veterans/disability_compensation/form_526_json_api.json
modified: modules/claims_api/spec/requests/v2/veterans/disability_compensation_request_spec.rb
modified: spec/support/schemas/claims_api/v2/forms/disability/submission.json
Acceptance criteria
Requested Feedback
I had to update the schema for the direct deposit object. Specifically one area that could use a heavier glance is the 'accountType' set up. It does pass the tests scenarios and validations but it was the first time I had to use
oneOf
. I believe I have it correct according to the OAS docs but if someone wants to give that an extra glance it might be worth digging at a little bit just to make sure.