From 4135951a5b9aebde4c9a445755abcc99dbc5c103 Mon Sep 17 00:00:00 2001 From: Andrew Herzberg Date: Wed, 24 Apr 2024 12:48:27 -0700 Subject: [PATCH] add upstream service failures to mutli status response (#16465) --- .../mobile/v0/appointments_controller.rb | 13 +++---- .../mobile/v2/appointments/provider_names.rb | 13 +++++-- .../services/mobile/v2/appointments/proxy.rb | 38 ++++++++++++++++--- .../appointments_vaos_v2_list_request_spec.rb | 29 ++++++++++++++ .../v2/appointments/provider_names_spec.rb | 22 +++++++---- .../support/schemas/VAOS_v2_appointments.json | 2 +- 6 files changed, 93 insertions(+), 24 deletions(-) diff --git a/modules/mobile/app/controllers/mobile/v0/appointments_controller.rb b/modules/mobile/app/controllers/mobile/v0/appointments_controller.rb index 6525148a734..adff7bb4ea5 100644 --- a/modules/mobile/app/controllers/mobile/v0/appointments_controller.rb +++ b/modules/mobile/app/controllers/mobile/v0/appointments_controller.rb @@ -87,7 +87,7 @@ def fetch_appointments # The mobile app does not distinguish between VA and CC errors so we are only indicating that there are errors # If we ever want to distinguish be VA and CC errors, it will require coordination with the front-end team def partial_errors(failures) - if failures.present? + if appointment_errors?(failures) { errors: [{ source: 'VA Service' }] } @@ -95,12 +95,11 @@ def partial_errors(failures) end def get_response_status(failures) - case failures&.size - when 0, nil - :ok - else - :multi_status - end + appointment_errors?(failures) ? :multi_status : :ok + end + + def appointment_errors?(failures) + failures.any? { |failure| failure[:appointment_errors].present? } end def filter_by_date_range(appointments) diff --git a/modules/mobile/app/services/mobile/v2/appointments/provider_names.rb b/modules/mobile/app/services/mobile/v2/appointments/provider_names.rb index 1f7473ecc86..4ec4528201d 100644 --- a/modules/mobile/app/services/mobile/v2/appointments/provider_names.rb +++ b/modules/mobile/app/services/mobile/v2/appointments/provider_names.rb @@ -20,10 +20,17 @@ def form_names_from_appointment_practitioners_list(practitioners_list) return nil if practitioners_list.blank? provider_names = [] + missing_providers = [] practitioners_list.each do |practitioner| - provider_names << find_provider_name(practitioner) + name, id = find_provider_name(practitioner) + if name + provider_names << name + else + missing_providers << id + end end - provider_names.compact.join(', ').presence + provider_names = provider_names.compact.join(', ').presence + [provider_names, missing_providers] end private @@ -41,7 +48,7 @@ def find_provider_name(practitioner) name = provider_data&.name&.strip&.presence # cache even if it's nil to avoid duplicate requests @providers_cache[id] = name - name + [name, id] end def find_practitioner_name_in_list(practitioner) diff --git a/modules/mobile/app/services/mobile/v2/appointments/proxy.rb b/modules/mobile/app/services/mobile/v2/appointments/proxy.rb index 40348c892be..4fa4ed8ee02 100644 --- a/modules/mobile/app/services/mobile/v2/appointments/proxy.rb +++ b/modules/mobile/app/services/mobile/v2/appointments/proxy.rb @@ -24,13 +24,21 @@ def get_appointments(start_date:, end_date:, include_pending:, pagination_params filterer = PresentationFilter.new(include_pending:) appointments = appointments.keep_if { |appt| filterer.user_facing?(appt) } - appointments = merge_clinic_facility_address(appointments) - appointments = merge_auxiliary_clinic_info(appointments) - appointments = merge_provider_names(appointments) + appointments, missing_facilities = merge_clinic_facility_address(appointments) + appointments, missing_clinics = merge_auxiliary_clinic_info(appointments) + appointments, missing_providers = merge_provider_names(appointments) appointments = vaos_v2_to_v0_appointment_adapter.parse(appointments) - - [appointments.sort_by(&:start_date_utc), response[:meta][:failures]] + failures = [ + { appointment_errors: Array.wrap(response[:meta][:failures]) }, + { missing_facilities: }, + { missing_clinics: }, + { missing_providers: } + ] + failures.reject! { |failure| failure.values.first&.empty? } + Rails.logger.info('Mobile Appointment Partial Error', errors: failures) if failures.any? + + [appointments.sort_by(&:start_date_utc), failures] end private @@ -43,12 +51,18 @@ def merge_clinic_facility_address(appointments) cached_facilities[facility_id] = appointments_helper.get_facility(facility_id) end + missing_facilities = [] + appointments.each do |appt| facility_id = appt[:location_id] next unless facility_id appt[:location] = cached_facilities[facility_id] + + missing_facilities << facility_id unless cached_facilities[facility_id] end + + [appointments, missing_facilities] end def merge_auxiliary_clinic_info(appointments) @@ -59,6 +73,8 @@ def merge_auxiliary_clinic_info(appointments) cached_clinics[clinic_id] = appointments_helper.get_clinic(location_id, clinic_id) end + missing_clinics = [] + appointments.each do |appt| clinic_id = appt[:clinic] next unless clinic_id @@ -68,16 +84,26 @@ def merge_auxiliary_clinic_info(appointments) physical_location = cached_clinics.dig(clinic_id, :physical_location) appt[:physical_location] = physical_location + + missing_clinics << clinic_id unless cached_clinics[clinic_id] end + [appointments, missing_clinics] end def merge_provider_names(appointments) provider_names_proxy = ProviderNames.new(@user) + missing_providers = [] appointments.each do |appt| practitioners_list = appt[:practitioners] - names = provider_names_proxy.form_names_from_appointment_practitioners_list(practitioners_list) + next unless practitioners_list + + names, appointment_missing_providers = + provider_names_proxy.form_names_from_appointment_practitioners_list(practitioners_list) appt[:healthcare_provider] = names + missing_providers.concat(appointment_missing_providers) unless names end + + [appointments, missing_providers] end def appointments_helper diff --git a/modules/mobile/spec/request/appointments_vaos_v2_list_request_spec.rb b/modules/mobile/spec/request/appointments_vaos_v2_list_request_spec.rb index 60f14b0c007..42149d33ca9 100644 --- a/modules/mobile/spec/request/appointments_vaos_v2_list_request_spec.rb +++ b/modules/mobile/spec/request/appointments_vaos_v2_list_request_spec.rb @@ -13,6 +13,7 @@ before do Flipper.enable('va_online_scheduling') allow_any_instance_of(VAOS::UserService).to receive(:session).and_return('stubbed_token') + allow(Rails.logger).to receive(:info) Timecop.freeze(Time.zone.parse('2022-01-01T19:25:00Z')) end @@ -66,6 +67,7 @@ end end end + expect(response).to have_http_status(:ok) location = response.parsed_body.dig('data', 0, 'attributes', 'location') physical_location = response.parsed_body.dig('data', 0, 'attributes', 'physicalLocation') expect(response.body).to match_json_schema('VAOS_v2_appointments') @@ -84,6 +86,14 @@ 'url' => nil, 'code' => nil }) expect(physical_location).to eq('MTZ OPC, LAB') + expect(response.parsed_body['meta']).to eq({ + 'pagination' => { 'currentPage' => 1, + 'perPage' => 10, + 'totalPages' => 1, + 'totalEntries' => 1 }, + 'upcomingAppointmentsCount' => 0, + 'upcomingDaysLimit' => 7 + }) end end @@ -97,6 +107,9 @@ end end end + expect(response).to have_http_status(:ok) + expect(Rails.logger).to have_received(:info).with('Mobile Appointment Partial Error', + errors: [{ missing_facilities: ['983'] }]) expect(response.body).to match_json_schema('VAOS_v2_appointments') location = response.parsed_body.dig('data', 0, 'attributes', 'location') expect(location).to eq({ 'id' => nil, @@ -155,6 +168,10 @@ end end end + expect(response).to have_http_status(:ok) + expect(Rails.logger).to have_received(:info).with('Mobile Appointment Partial Error', + errors: [{ missing_facilities: ['999AA'] }, + { missing_clinics: ['999'] }]) expect(response.body).to match_json_schema('VAOS_v2_appointments') expect(response.parsed_body.dig('data', 0, 'attributes', 'healthcareService')).to be_nil end @@ -187,6 +204,14 @@ end expect(response).to have_http_status(:multi_status) + appointment_error = { errors: [{ appointment_errors: [{ system: 'the system', + id: 'id-string', + status: 'status-string', + code: 0, + trace_id: 'traceId-string', + message: 'msg-string', + detail: 'detail-string' }] }] } + expect(Rails.logger).to have_received(:info).with('Mobile Appointment Partial Error', appointment_error) expect(response.parsed_body['data'].count).to eq(1) expect(response.parsed_body['meta']).to include( { @@ -287,6 +312,8 @@ def fetch_appointments end expect(response).to have_http_status(:ok) expect(appointment['attributes']['healthcareProvider']).to be_nil + expect(Rails.logger).to have_received(:info).with('Mobile Appointment Partial Error', + errors: [{ missing_providers: ['1407938061'] }]) end it 'falls back to nil when provider service returns 500' do @@ -304,6 +331,8 @@ def fetch_appointments end expect(response).to have_http_status(:ok) expect(appointment['attributes']['healthcareProvider']).to be_nil + expect(Rails.logger).to have_received(:info).with('Mobile Appointment Partial Error', + errors: [{ missing_providers: ['1407938061'] }]) end end diff --git a/modules/mobile/spec/services/v2/appointments/provider_names_spec.rb b/modules/mobile/spec/services/v2/appointments/provider_names_spec.rb index 526a040733f..6f4b080541b 100644 --- a/modules/mobile/spec/services/v2/appointments/provider_names_spec.rb +++ b/modules/mobile/spec/services/v2/appointments/provider_names_spec.rb @@ -69,27 +69,31 @@ end it 'returns names as first_name last_name' do - name = subject.form_names_from_appointment_practitioners_list(single_practitioner_with_name) + name, missing_providers = subject.form_names_from_appointment_practitioners_list(single_practitioner_with_name) expect(name).to eq('CAROLYN KNIEFEL') + expect(missing_providers).to eq([]) end it 'handles partial names predictably' do partial_name_data = single_practitioner_with_name.first partial_name_data[:name].delete(:given) - name = subject.form_names_from_appointment_practitioners_list([partial_name_data]) + name, missing_providers = subject.form_names_from_appointment_practitioners_list([partial_name_data]) expect(name).to eq('KNIEFEL') + expect(missing_providers).to eq([]) end it 'aggregates multiple names as a comma separated list' do - name = subject.form_names_from_appointment_practitioners_list(multiple_practioners_with_names) + name, missing_providers = subject.form_names_from_appointment_practitioners_list(multiple_practioners_with_names) expect(name).to eq('CAROLYN KNIEFEL, MARCY NADEAU') + expect(missing_providers).to eq([]) end it 'forms names from upstream when an identifier is found without a name' do allow_any_instance_of(VAOS::V2::MobilePPMSService).to\ receive(:get_provider).with('1407938061').and_return(provider_response) - name = subject.form_names_from_appointment_practitioners_list(practitioner_without_name) + name, missing_providers = subject.form_names_from_appointment_practitioners_list(practitioner_without_name) expect(name).to eq('DEHGHAN, AMIR') + expect(missing_providers).to eq([]) end it 'can request multiple upstream providers' do @@ -107,8 +111,10 @@ receive(:get_provider).with('1407938061').and_return(provider_response) allow_any_instance_of(VAOS::V2::MobilePPMSService).to\ receive(:get_provider).with('1407938062').and_return(second_provider_response) - name = subject.form_names_from_appointment_practitioners_list(multiple_practitioners_without_names) + name, missing_providers = + subject.form_names_from_appointment_practitioners_list(multiple_practitioners_without_names) expect(name).to eq('DEHGHAN, AMIR, J. Jones') + expect(missing_providers).to eq([]) end it 'only requests an upstream provider once' do @@ -121,8 +127,9 @@ it 'returns nil when the ppms service raises an error' do allow_any_instance_of(VAOS::V2::MobilePPMSService).to\ receive(:get_provider).and_raise(Common::Exceptions::BackendServiceException) - name = subject.form_names_from_appointment_practitioners_list(practitioner_without_name) + name, missing_providers = subject.form_names_from_appointment_practitioners_list(practitioner_without_name) expect(name).to be_nil + expect(missing_providers).to eq(['1407938061']) end it 'returns nil if the returned provider does not match the expected structure' do @@ -130,8 +137,9 @@ allow_any_instance_of(VAOS::V2::MobilePPMSService).to\ receive(:get_provider).with('1407938061').and_return(nameless_provider) - name = subject.form_names_from_appointment_practitioners_list(practitioner_without_name) + name, missing_providers = subject.form_names_from_appointment_practitioners_list(practitioner_without_name) expect(name).to be_nil + expect(missing_providers).to eq(['1407938061']) end end end diff --git a/modules/mobile/spec/support/schemas/VAOS_v2_appointments.json b/modules/mobile/spec/support/schemas/VAOS_v2_appointments.json index 1a3f2ce3bc3..3d419e31c69 100644 --- a/modules/mobile/spec/support/schemas/VAOS_v2_appointments.json +++ b/modules/mobile/spec/support/schemas/VAOS_v2_appointments.json @@ -235,7 +235,7 @@ ], "properties": { "errors": { - "type": "null" + "type": ["array","null"] }, "pagination": { "type": "object",