From 1d377aa5bf6e60e81ab591ab84e43cddb5f5da5b Mon Sep 17 00:00:00 2001 From: Liz Townsend <72234279+liztownd@users.noreply.github.com> Date: Mon, 2 Dec 2024 09:55:23 -0600 Subject: [PATCH 1/2] Travel Pay/improve error handling (#19453) * better error handling and messaging, adjust tests * DRY up the code, build metadata object * reinstate validate dates method; adjust tests accordingly * reinstate error test in association spec * add more generic error handling * also log random uknown errors * move all related calls into association service; adjust all tests * don't take this out yet, get below LOC threshold * put tests back, move things around in claims service, try to get under LOC threshold * Revert "put tests back, move things around in claims service, try to get under LOC threshold" This reverts commit aaef0f005b341118baf6252e6651556b37a93ef3. * Revert "don't take this out yet, get below LOC threshold" This reverts commit 3c33b04adc7ebc187698577d00d20b4c598b6529. * trim lines to get under LOC threshold * Revert "trim lines to get under LOC threshold" This reverts commit 3672ac52cff429ccb2c9f06425965b0faa652a59. * sigh. try copying directly from master. * skip the whole extra Faraday bit, just return the metadata object on rescu * fix typos * add comment re: only getting one date in single appointment * sigh. And now that method is too long, disable rubocop on that line * simple up the single appt logic --- .../travel_pay/claim_association_service.rb | 129 +++++++--- .../claims_association_service_spec.rb | 234 +++++++++++++----- 2 files changed, 265 insertions(+), 98 deletions(-) diff --git a/modules/travel_pay/app/services/travel_pay/claim_association_service.rb b/modules/travel_pay/app/services/travel_pay/claim_association_service.rb index fe662eea821..c7cd8febf14 100644 --- a/modules/travel_pay/app/services/travel_pay/claim_association_service.rb +++ b/modules/travel_pay/app/services/travel_pay/claim_association_service.rb @@ -5,6 +5,7 @@ class ClaimAssociationService def initialize(user) @user = user end + # We need to associate an existing claim to a VAOS appointment, matching on date-time # # There will be a 1:1 claimID > appt association @@ -16,7 +17,7 @@ def initialize(user) # metadata => { # status => int (http status code, i.e. 200) # success => boolean - # message => string ('No claim for this appt' | 'Claim service is unavailable') + # message => string ('Data retrieved successfully' | 'Claim service is unavailable') # } # claim => TravelPay::Claim (if a claim matches) # } @@ -30,52 +31,79 @@ def initialize(user) # appointments: [VAOS::Appointment + travelPayClaim] def associate_appointments_to_claims(params = {}) - raw_claims = service.get_claims_by_date_range( - { 'start_date' => params['start_date'], - 'end_date' => params['end_date'] } - ) - if raw_claims - append_claims(params['appointments'], raw_claims[:data], raw_claims[:metadata]) - else - append_error(params['appointments'], { 'status' => 503, - 'success' => false, - 'message' => 'Travel Pay service unavailable.' }) + validate_date_params(params['start_date'], params['end_date']) + + auth_manager.authorize => { veis_token:, btsss_token: } + faraday_response = client.get_claims_by_date(veis_token, btsss_token, + { 'start_date' => params['start_date'], + 'end_date' => params['end_date'] }) + + if faraday_response.status == 200 + raw_claims = faraday_response.body['data'].deep_dup + + data = raw_claims&.map do |sc| + sc['claimStatus'] = sc['claimStatus'].underscore.titleize + sc + end + + append_claims(params['appointments'], + data, + build_metadata(faraday_response.body)) + end + rescue => e + append_error(params['appointments'], + rescue_errors(e)) end def associate_single_appointment_to_claim(params = {}) appt = params['appointment'] + # Because we only receive a single date/time but the external endpoint requires 2 dates + # in this case both start and end dates are the same + validate_date_params(appt['start'], appt['start']) - raw_claim = service.get_claims_by_date_range( - { 'start_date' => appt['start'], - 'end_date' => appt['start'] } - ) - if raw_claim - append_single_claim(appt, raw_claim) - else - appt['travelPayClaim'] = { - 'metadata' => { 'status' => 503, - 'success' => false, - 'message' => 'Travel Pay service unavailable.' } - } - appt - end + auth_manager.authorize => { veis_token:, btsss_token: } + faraday_response = client.get_claims_by_date(veis_token, btsss_token, + { 'start_date' => appt['start'], 'end_date' => appt['start'] }) + + claim_data = faraday_response.body['data']&.dig(0) + appt['travelPayClaim'] = {} + appt['travelPayClaim']['metadata'] = build_metadata(faraday_response.body) + appt['travelPayClaim']['claim'] = claim_data if claim_data + + appt + rescue => e + appt['travelPayClaim'] = { + 'metadata' => rescue_errors(e) + } + appt end private - def append_single_claim(appt, claim_response) - appt['travelPayClaim'] = if claim_response[:data].count - { - 'metadata' => claim_response[:metadata], - 'claim' => claim_response[:data][0] - } - else - { - 'metadata' => claim_response[:metadata] - } - end - appt + def rescue_errors(e) # rubocop:disable Metrics/MethodLength + if e.is_a?(ArgumentError) + Rails.logger.error(message: e.message.to_s) + { + 'status' => 400, + 'message' => e.message.to_s, + 'success' => false + } + elsif e.is_a?(Common::Exceptions::BackendServiceException) + Rails.logger.error(message: "#{e}, #{e.original_body}") + { + 'status' => e.original_status, + 'message' => e.original_body['message'], + 'success' => false + } + else + Rails.logger.error(message: "An unknown error occured: #{e}") + { + 'status' => 520, # Unknown error code + 'message' => "An unknown error occured: #{e}", + 'success' => false + } + end end def append_claims(appts, claims, metadata) @@ -112,9 +140,30 @@ def append_error(appts, metadata) appointments end - def service - auth_manager = TravelPay::AuthManager.new(Settings.travel_pay.client_number, @user) - TravelPay::ClaimsService.new(auth_manager) + def build_metadata(faraday_response_body) + { 'status' => faraday_response_body['statusCode'], + 'success' => faraday_response_body['success'], + 'message' => faraday_response_body['message'] } + end + + def validate_date_params(start_date, end_date) + if start_date && end_date + DateTime.parse(start_date.to_s) && DateTime.parse(end_date.to_s) + else + raise ArgumentError, + message: "Both start and end dates are required, got #{start_date}-#{end_date}." + end + rescue Date::Error => e + raise ArgumentError, + message: "#{e}. Invalid date(s) provided (given: #{start_date} & #{end_date})." + end + + def auth_manager + @auth_manager ||= TravelPay::AuthManager.new(Settings.travel_pay.client_number, @user) + end + + def client + TravelPay::ClaimsClient.new end end end diff --git a/modules/travel_pay/spec/services/claims_association_service_spec.rb b/modules/travel_pay/spec/services/claims_association_service_spec.rb index de516880e66..360b44c8cb0 100644 --- a/modules/travel_pay/spec/services/claims_association_service_spec.rb +++ b/modules/travel_pay/spec/services/claims_association_service_spec.rb @@ -8,12 +8,10 @@ let(:user) { build(:user) } let(:claims_data_success) do { - metadata: { - 'status' => 200, - 'message' => 'Data retrieved successfully.', - 'success' => true - }, - data: [ + 'statusCode' => 200, + 'message' => 'Data retrieved successfully.', + 'success' => true, + 'data' => [ { 'id' => 'uuid1', 'claimNumber' => 'TC0000000000001', @@ -94,17 +92,30 @@ ] end - let(:tokens) { { veis_token: 'veis_token', btsss_token: 'btsss_token' } } + let(:claims_success_response) do + Faraday::Response.new( + response_body: claims_data_success, + status: 200 + ) + end + let(:expected_uuids) { %w[uuid1] } + let(:tokens) { { veis_token: 'veis_token', btsss_token: 'btsss_token' } } + + before do + allow_any_instance_of(TravelPay::AuthManager) + .to receive(:authorize) + .and_return(tokens) + end + it 'returns appointments with matched claims' do - allow_any_instance_of(TravelPay::ClaimsService) - .to receive(:get_claims_by_date_range) - .with( - { 'start_date' => '2024-10-17T09:00:00Z', - 'end_date' => '2024-12-15T16:45:00Z' } - ) - .and_return(claims_data_success) + allow_any_instance_of(TravelPay::ClaimsClient) + .to receive(:get_claims_by_date) + .with(tokens[:veis_token], tokens[:btsss_token], + { 'start_date' => '2024-10-17T09:00:00Z', + 'end_date' => '2024-12-15T16:45:00Z' }) + .and_return(claims_success_response) association_service = TravelPay::ClaimAssociationService.new(user) appts_with_claims = association_service.associate_appointments_to_claims({ 'appointments' => appointments, @@ -126,13 +137,22 @@ end it 'returns appointments with error metadata if claims call fails' do - allow_any_instance_of(TravelPay::ClaimsService) - .to receive(:get_claims_by_date_range) - .with( - { 'start_date' => '2024-10-17T09:00:00Z', - 'end_date' => '2024-12-15T16:45:00Z' } - ) - .and_return(nil) + allow_any_instance_of(TravelPay::ClaimsClient) + .to receive(:get_claims_by_date) + .with(tokens[:veis_token], tokens[:btsss_token], + { 'start_date' => '2024-10-17T09:00:00Z', + 'end_date' => '2024-12-15T16:45:00Z' }) + .and_raise(Common::Exceptions::BackendServiceException.new( + 'VA900', + { source: 'test' }, + 401, + { + 'statusCode' => 401, + 'message' => 'Unauthorized.', + 'success' => false, + 'data' => nil + } + )) association_service = TravelPay::ClaimAssociationService.new(user) appts_with_claims = association_service.associate_appointments_to_claims({ 'appointments' => appointments, @@ -144,9 +164,50 @@ c['travelPayClaim']['claim'] end).to be_nil appts_with_claims.each do |appt| - expect(appt['travelPayClaim']['metadata']['status']).to equal(503) - expect(appt['travelPayClaim']['metadata']['message']).to eq('Travel Pay service unavailable.') + expect(appt['travelPayClaim']['metadata']['status']).to equal(401) + expect(appt['travelPayClaim']['metadata']['message']).to eq('Unauthorized.') + expect(appt['travelPayClaim']['metadata']['success']).to eq(false) + end + end + + it 'handles random, unknown errors' do + allow_any_instance_of(TravelPay::ClaimsClient) + .to receive(:get_claims_by_date) + .and_raise(NameError.new('Uninitialized constant.', 'new_constant')) + + association_service = TravelPay::ClaimAssociationService.new(user) + appts_with_claims = association_service.associate_appointments_to_claims({ 'appointments' => appointments, + 'start_date' => '2024-10-17T09:00:00Z', + 'end_date' => '2024-12-15T16:45:00Z' }) + appts_with_claims.each do |appt| + expect(appt['travelPayClaim']['metadata']['status']).to equal(520) + expect(appt['travelPayClaim']['metadata']['success']).to eq(false) + expect(appt['travelPayClaim']['metadata']['message']).to include(/Uninitialized constant/i) + end + end + + it 'returns 400 with message if both start and end dates are not provided' do + association_service = TravelPay::ClaimAssociationService.new(user) + appts = association_service.associate_appointments_to_claims({ 'appointments' => appointments, + 'start_date' => '2024-10-17T09:00:00Z' }) + + appts.each do |appt| + expect(appt['travelPayClaim']['metadata']['status']).to equal(400) + expect(appt['travelPayClaim']['metadata']['success']).to eq(false) + expect(appt['travelPayClaim']['metadata']['message']).to include(/Both start and end/i) + end + end + + it 'returns 400 with error message if dates are invalid' do + association_service = TravelPay::ClaimAssociationService.new(user) + appts = association_service.associate_appointments_to_claims({ 'appointments' => appointments, + 'start_date' => '2024-10-17T09:00:00Z', + 'end_date' => 'banana' }) + + appts.each do |appt| + expect(appt['travelPayClaim']['metadata']['status']).to equal(400) expect(appt['travelPayClaim']['metadata']['success']).to eq(false) + expect(appt['travelPayClaim']['metadata']['message']).to include(/invalid date/i) end end end @@ -155,12 +216,10 @@ let(:user) { build(:user) } let(:single_claim_data_success) do { - metadata: { - 'status' => 200, - 'message' => 'Data retrieved successfully.', - 'success' => true - }, - data: [ + 'statusCode' => 200, + 'message' => 'Data retrieved successfully.', + 'success' => true, + 'data' => [ { 'id' => 'uuid1', 'claimNumber' => 'TC0000000000001', @@ -174,15 +233,23 @@ } end + let(:single_claim_success_response) do + Faraday::Response.new( + response_body: single_claim_data_success, + status: 200 + ) + end + let(:no_claim_data_success) do - { - metadata: { - 'status' => 200, + Faraday::Response.new( + response_body: { + 'statusCode' => 200, 'message' => 'Data retrieved successfully.', - 'success' => true + 'success' => true, + 'data' => [] }, - data: [] - } + status: 200 + ) end let(:single_appointment) do @@ -198,16 +265,34 @@ } end + let(:single_appt_invalid) do + { + 'id' => '32066', + 'kind' => 'clinic', + 'status' => 'cancelled', + 'patientIcn' => '1012845331V153043', + 'locationId' => '983', + 'clinic' => '1081', + 'start' => 'banana', + 'cancellable' => false + } + end + let(:tokens) { { veis_token: 'veis_token', btsss_token: 'btsss_token' } } + before do + allow_any_instance_of(TravelPay::AuthManager) + .to receive(:authorize) + .and_return(tokens) + end + it 'returns an appointment with a claim' do - allow_any_instance_of(TravelPay::ClaimsService) - .to receive(:get_claims_by_date_range) - .with( - { 'start_date' => '2024-01-01T16:45:34Z', - 'end_date' => '2024-01-01T16:45:34Z' } - ) - .and_return(single_claim_data_success) + allow_any_instance_of(TravelPay::ClaimsClient) + .to receive(:get_claims_by_date) + .with(tokens[:veis_token], tokens[:btsss_token], + { 'start_date' => '2024-01-01T16:45:34Z', + 'end_date' => '2024-01-01T16:45:34Z' }) + .and_return(single_claim_success_response) association_service = TravelPay::ClaimAssociationService.new(user) appt_with_claim = association_service.associate_single_appointment_to_claim( @@ -217,16 +302,15 @@ expect(appt_with_claim['travelPayClaim']['metadata']['status']).to eq(200) expect(appt_with_claim['travelPayClaim']['metadata']['message']).to eq('Data retrieved successfully.') expect(appt_with_claim['travelPayClaim']['metadata']['success']).to eq(true) - expect(appt_with_claim['travelPayClaim']['claim']).to eq(single_claim_data_success[:data][0]) + expect(appt_with_claim['travelPayClaim']['claim']['id']).to eq(single_claim_data_success['data'][0]['id']) end it 'returns an appointment with success metadata but no claim' do - allow_any_instance_of(TravelPay::ClaimsService) - .to receive(:get_claims_by_date_range) - .with( - { 'start_date' => '2024-01-01T16:45:34Z', - 'end_date' => '2024-01-01T16:45:34Z' } - ) + allow_any_instance_of(TravelPay::ClaimsClient) + .to receive(:get_claims_by_date) + .with(tokens[:veis_token], tokens[:btsss_token], + { 'start_date' => '2024-01-01T16:45:34Z', + 'end_date' => '2024-01-01T16:45:34Z' }) .and_return(no_claim_data_success) association_service = TravelPay::ClaimAssociationService.new(user) @@ -241,13 +325,22 @@ end it 'returns appointment with error metadata if claims call fails' do - allow_any_instance_of(TravelPay::ClaimsService) - .to receive(:get_claims_by_date_range) - .with( - { 'start_date' => '2024-01-01T16:45:34Z', - 'end_date' => '2024-01-01T16:45:34Z' } - ) - .and_return(nil) + allow_any_instance_of(TravelPay::ClaimsClient) + .to receive(:get_claims_by_date) + .with(tokens[:veis_token], tokens[:btsss_token], + { 'start_date' => '2024-01-01T16:45:34Z', + 'end_date' => '2024-01-01T16:45:34Z' }) + .and_raise(Common::Exceptions::BackendServiceException.new( + 'VA900', + { source: 'test' }, + 401, + { + 'statusCode' => 401, + 'message' => 'A contact with the specified ICN was not found.', + 'success' => false, + 'data' => nil + } + )) association_service = TravelPay::ClaimAssociationService.new(user) appt_with_claim = association_service.associate_single_appointment_to_claim( @@ -255,10 +348,35 @@ ) expect(appt_with_claim['travelPayClaim']['claim']).to be_nil - expect(appt_with_claim['travelPayClaim']['metadata']['status']).to equal(503) + expect(appt_with_claim['travelPayClaim']['metadata']['status']).to equal(401) expect(appt_with_claim['travelPayClaim']['metadata']['message']) - .to eq('Travel Pay service unavailable.') + .to eq('A contact with the specified ICN was not found.') expect(appt_with_claim['travelPayClaim']['metadata']['success']).to eq(false) end + + it 'handles random, unknown errors' do + allow_any_instance_of(TravelPay::ClaimsClient) + .to receive(:get_claims_by_date) + .and_raise(NameError.new('Uninitialized constant.', 'new_constant')) + + association_service = TravelPay::ClaimAssociationService.new(user) + appt_with_claim = association_service.associate_single_appointment_to_claim({ + 'appointment' => single_appointment + }) + expect(appt_with_claim['travelPayClaim']['metadata']['status']).to equal(520) + expect(appt_with_claim['travelPayClaim']['metadata']['success']).to eq(false) + expect(appt_with_claim['travelPayClaim']['metadata']['message']).to include(/Uninitialized constant/i) + end + + it 'returns 400 with error message if dates are invalid' do + association_service = TravelPay::ClaimAssociationService.new(user) + appt = association_service.associate_single_appointment_to_claim({ + 'appointment' => single_appt_invalid + }) + + expect(appt['travelPayClaim']['metadata']['status']).to equal(400) + expect(appt['travelPayClaim']['metadata']['success']).to eq(false) + expect(appt['travelPayClaim']['metadata']['message']).to include(/invalid date/i) + end end end From 929ee51b935feede7b503dbd3aee70ea8e627a0a Mon Sep 17 00:00:00 2001 From: Tyler Date: Mon, 2 Dec 2024 08:16:44 -0800 Subject: [PATCH 2/2] update state and country v1 docs (#19614) --- .../app/swagger/claims_api/v1/swagger.json | 32 +++++++++---------- .../claims_api/config/schemas/v1/2122.json | 12 +++---- modules/claims_api/config/schemas/v1/526.json | 4 +-- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/modules/claims_api/app/swagger/claims_api/v1/swagger.json b/modules/claims_api/app/swagger/claims_api/v1/swagger.json index 6b24b8608f2..e237e86d835 100644 --- a/modules/claims_api/app/swagger/claims_api/v1/swagger.json +++ b/modules/claims_api/app/swagger/claims_api/v1/swagger.json @@ -3045,7 +3045,7 @@ "example": "DOMESTIC" }, "state": { - "description": "State Veteran resides in.", + "description": "State or province Veteran resides in.", "type": "string", "pattern": "^[a-z,A-Z]{2}$", "example": "OR" @@ -3197,7 +3197,7 @@ "type": "string" }, "state": { - "description": "New state Veteran resides in.", + "description": "New state or province Veteran resides in.", "type": "string", "pattern": "^[a-z,A-Z]{2}$", "example": "OR" @@ -5303,7 +5303,7 @@ "example": "DOMESTIC" }, "state": { - "description": "State Veteran resides in.", + "description": "State or province Veteran resides in.", "type": "string", "pattern": "^[a-z,A-Z]{2}$", "example": "OR" @@ -5455,7 +5455,7 @@ "type": "string" }, "state": { - "description": "New state Veteran resides in.", + "description": "New state or province Veteran resides in.", "type": "string", "pattern": "^[a-z,A-Z]{2}$", "example": "OR" @@ -8953,14 +8953,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", @@ -9093,14 +9093,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", @@ -9229,14 +9229,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", @@ -10821,14 +10821,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", @@ -10961,14 +10961,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", @@ -11097,14 +11097,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", diff --git a/modules/claims_api/config/schemas/v1/2122.json b/modules/claims_api/config/schemas/v1/2122.json index 2786b6d5724..6156837c7b7 100644 --- a/modules/claims_api/config/schemas/v1/2122.json +++ b/modules/claims_api/config/schemas/v1/2122.json @@ -42,14 +42,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", @@ -170,14 +170,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", @@ -292,14 +292,14 @@ "example": "Portland" }, "state": { - "description": "State for the address.", + "description": "State or province for the address.", "type": "string", "example": "OR" }, "country": { "description": "Country of the address.", "type": "string", - "example": "USA" + "example": "US" }, "zipFirstFive": { "description": "Zipcode (First 5 digits) of the address.", diff --git a/modules/claims_api/config/schemas/v1/526.json b/modules/claims_api/config/schemas/v1/526.json index 1e67e13d79a..d6da421b9d6 100644 --- a/modules/claims_api/config/schemas/v1/526.json +++ b/modules/claims_api/config/schemas/v1/526.json @@ -108,7 +108,7 @@ "example": "DOMESTIC" }, "state": { - "description": "State Veteran resides in.", + "description": "State or province Veteran resides in.", "type": "string", "pattern": "^[a-z,A-Z]{2}$", "example": "OR" @@ -250,7 +250,7 @@ "type": "string" }, "state": { - "description": "New state Veteran resides in.", + "description": "New state or province Veteran resides in.", "type": "string", "pattern": "^[a-z,A-Z]{2}$", "example": "OR"