API-34957 v1 error formatter update

[rockwellwindsor-va]

Summary

• This PR addresses san error formatting issue. In the claim_establisher there was a wrapper being placed around ::Common::Exceptions::BackendServiceException when they were rescued. That wrapper set the key: value as an integer. That caused the error formatter method in the claims_controller, format_evss_errors, to break when someone would try to view that claim because .gsub was being called on that value (integer). So it would return a claim not found error when the claim was found it's evss response just broke the formatter.
• This PR removes that wrapper when saving the error message as the evss_response, formats the evss_responses so that they have the correct information, and also updates the format_evss_errors method in the claims_controller so it can display any previously saved claims with the incorrect wrapper.
• Builds off the work done in a previous PR here

Related issue(s)

API-33934, failure
API-34957

Testing done

• New code is covered by unit tests
• Describe what the old behavior was prior to the change
• 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:
  • Tests need to be written for both the flipper on and flipper off scenarios. Docs.
  • What is the testing plan for rolling out the feature?

Testing Notes

• Any way to raise an error in evss_service/base will be a good test. Make sure to turn off mocking.
• You can mess up the brls numbers in the submit and validation methods by putting this on lines 24 or 36

@auth_headers['va_eauth_birlsfilenumber'] = '79612'

• You can add and raise an error, you would place this in place of lines 26-28 in the submit method or lines38 - 41 in the validate method.

err_body = [
          {"key"=>"form526.submit.establishClaim.serviceError","severity"=>"FATAL","text"=>"Claim not established. System error with BGS. GUID: 00797c5d-89d4-4da6-aab7-24b4ad0e4a4f"},
          {"key"=>"form526.submit.establishClaim.serviceError","severity"=>"FATAL","text"=>"Claim not established. System error with BGS. GUID: 00797c5d-89d4-4da6-aab7-24b4ad0e4a4f"}
        ]
        raise ::Common::Exceptions::BackendServiceException.new(nil, {}, 400, err_body)

• Once the claim gets saved with the error you will want to then view the claim via http://localhost:3000/services/claims/v1/claims/:claim_id to see the evss response error message saved on the claim

• The original cause of this issue as stated before was a wrapper that saved an error with a key: value that was an integer. If you want to mirror this behavior to test that it will now handle showing these claims, you can take a claim you have saved in your DB and make its evss_response equal to the error below. You should still be able to view the claim and see the error message, the details will not be the same because of the wrapper but it should not return claim not found

[{ 'key' => 400,
                    'severity' => 'FATAL',
                    'text' =>
      { 'messages' =>
      [

        { 'key' => 'form526.submit.establishClaim.serviceError', 'severity' => 'FATAL',
          'text' => 'Claim not established. System error with BGS. GUID: 00797c5d-89d4-4da6-aab7-24b4ad0e4a4f' }
      ] } }]

Screenshots

Note: Optional

What areas of the site does it impact?

• modified: modules/claims_api/app/controllers/claims_api/v1/claims_controller.rb
• modified: modules/claims_api/app/controllers/claims_api/v1/forms/disability_compensation_controller.rb
• modified: modules/claims_api/app/sidekiq/claims_api/claim_establisher.rb
• modified: modules/claims_api/app/sidekiq/claims_api/service_base.rb
• modified: modules/claims_api/lib/custom_error.rb
• modified: modules/claims_api/spec/requests/v1/claims_request_spec.rb
• modified: modules/claims_api/spec/sidekiq/claim_custom_error_spec.rb
• modified: modules/claims_api/spec/sidekiq/claim_establisher_spec.rb

Acceptance criteria

• I fixed|updated|added unit tests and integration tests for each feature (if applicable).
• No error nor warning in the console.
• Events are being sent to the appropriate logging solution
• Documentation has been updated (link to documentation)
• No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
• Feature/bug has a monitor built into Datadog or Grafana (if applicable)
• If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
• I added a screenshot of the developed feature

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?

[github-actions]

1 Warning

⚠️

This PR changes 347 LoC (not counting whitespace/newlines). In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding 200. Expect some delays getting reviews. File Summary Files modules/claims_api/app/controllers/claims_api/v1/claims_controller.rb (+5/-3) modules/claims_api/app/controllers/claims_api/v1/forms/disability_compensation_controller.rb (+4/-3) modules/claims_api/app/sidekiq/claims_api/claim_establisher.rb (+3/-3) modules/claims_api/app/sidekiq/claims_api/service_base.rb (+4/-0) modules/claims_api/lib/custom_error.rb (+41/-74) modules/claims_api/lib/evss_service/base.rb (+6/-14) modules/claims_api/spec/controllers/v1/disability_compensation_controller_spec.rb (+26/-0) modules/claims_api/spec/requests/v1/claims_request_spec.rb (+40/-0) modules/claims_api/spec/sidekiq/claim_custom_error_spec.rb (+46/-62) modules/claims_api/spec/sidekiq/claim_establisher_spec.rb (+6/-7) Note: We exclude files matching the following when considering PR size: *.csv, *.json, *.tsv, *.txt, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/, *.bru Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

[FonzMP]

I think this is redundant logic. we can just pass e.original_body to line :34 and get the same result.

(byebug) @error.original_body
{:messages=>[{:key=>"header.va_eauth_birlsfilenumber.Invalid", :severity=>"ERROR", :text=>"Size must be between 8 and 9"}]}
   30:       error_details = get_error_details(error)
   31:
=> 32:       raise ::Common::Exceptions::BackendServiceException.new(
   33:         key,
   34:         { source: },
   35:         error&.original_status,
   36:         error_details
(byebug) error_details
[{:key=>"header.va_eauth_birlsfilenumber.Invalid", :severity=>"ERROR", :text=>"Size must be between 8 and 9"}]

actually this throws an error in the validate. without the :messages.

            "code": "500",
            "status": "500",
            "meta": {
                "exception": "no implicit conversion of Symbol into Integer",

=> 180:           error_details = e&.original_body&.[](:messages)
   181:           raise ::Common::Exceptions::UnprocessableEntity.new(errors: format_526_errors(error_details))
   182:         rescue ::Common::Exceptions::GatewayTimeout,
   183:                ::Timeout::Error,
   184:                ::Faraday::TimeoutError,
(byebug) e&.original_body&.[](:messages)
*** TypeError Exception: no implicit conversion of Symbol into Integer

nil

if we pass the original body we can just change the claim_establisher to record evss_response = e.messages. unless you saw a differing format in the submit call?

[rockwellwindsor-va]

@FonzMP Good catch. I think there is a different format in the claims_controller. I think the fix might actually be to remove the line error_details = e&.original_body&.[](:messages) in the V1 disability_compensation_controller. This was added when building the V2 validate service because of the different format from the docker container. I think the current update renders this unnecessary now. The error here and in the V1 claims_controller should be in the same format I believe, so not having :messages over there (v1 claims_controller formatter ) would mean we no longer want to account for it over here too I suspect. Thoughts?

[FonzMP]

still don't know if I'm quite following because the docker response is the same for v1 and v2. no formatting has been changed. plus the below logic for error_details just extracts the :messages we get from the container? Anywho, it behaves in the same manner, so 🤷

(byebug) error.original_body
{:messages=>[{:key=>"header.va_eauth_birlsfilenumber.Invalid", :severity=>"ERROR", :text=>"Size must be between 8 and 9"}]}
(byebug) error_details
[{:key=>"header.va_eauth_birlsfilenumber.Invalid", :severity=>"ERROR", :text=>"Size must be between 8 and 9"}]

[FonzMP]

I think I see where you're driving that.. it's because of the loss of the ability to call e.messages from the former error.

Service Exception being raised
Error class
parent giving us the goods

this structure essentially extracted one level out... hence the use of e.messages

[FonzMP]