From c1caa034ce8f38447a3d9916810eb04bf2b20229 Mon Sep 17 00:00:00 2001 From: Jennica Stiehl <25069483+stiehlrod@users.noreply.github.com> Date: Thu, 2 Jan 2025 11:58:03 -0700 Subject: [PATCH] Removes local_bgs_refactored & local_bgs_proxy and their tests. --- .../lib/bgs_service/local_bgs_proxy.rb | 61 ---------- .../lib/bgs_service/local_bgs_refactored.rb | 106 ------------------ .../lib/claims_api/local_bgs_proxy_spec.rb | 83 -------------- .../claims_api/local_bgs_refactored_spec.rb | 90 --------------- 4 files changed, 340 deletions(-) delete mode 100644 modules/claims_api/lib/bgs_service/local_bgs_proxy.rb delete mode 100644 modules/claims_api/spec/lib/claims_api/local_bgs_proxy_spec.rb delete mode 100644 modules/claims_api/spec/lib/claims_api/local_bgs_refactored_spec.rb diff --git a/modules/claims_api/lib/bgs_service/local_bgs_proxy.rb b/modules/claims_api/lib/bgs_service/local_bgs_proxy.rb deleted file mode 100644 index ae75b328e93..00000000000 --- a/modules/claims_api/lib/bgs_service/local_bgs_proxy.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require 'bgs_service/local_bgs_refactored' -require 'bgs_service/local_bgs' - -module ClaimsApi - # Proxy class that switches at runtime between using `LocalBGS` and - # `LocalBGSRefactored` depending on the value of our feature toggle. - class LocalBGSProxy - legacy_ancestors = - LocalBGS.ancestors - - LocalBGSRefactored.ancestors - - legacy_api = - legacy_ancestors.flat_map do |ancestor| - ancestor.instance_methods(false) - [:initialize] - end - - refactored_ancestors = - LocalBGSRefactored.ancestors - - LocalBGS.ancestors - - refactored_api = - refactored_ancestors.flat_map do |ancestor| - ancestor.instance_methods(false) - [:initialize] - end - - # This makes the assumption that we'll maintain compatibility for callers of - # `LocalBGS` by considering only its public instance methods, and in - # particular those not installed by framework-level ancestors. A "one-time" - # check was performed to ensure that instance methods that callers invoke - # directly are contained in `common_api` and not contained in `missing_api`. - missing_api = legacy_api - refactored_api - common_api = legacy_api & refactored_api - - Rails.logger.trace( - "Comparison between LocalBGS and LocalBGSRefactored API's", - missing_api:, - common_api: - ) - - class << self - delegate :breakers_service, to: :get_proxied_klass - - def get_proxied_klass - if Flipper.enabled?(:claims_api_local_bgs_refactor) - LocalBGSRefactored - else - LocalBGS - end - end - end - - delegate(*common_api, to: :proxied) - attr_reader :proxied - - def initialize(...) - @proxied = self.class.get_proxied_klass.new(...) - end - end -end diff --git a/modules/claims_api/lib/bgs_service/local_bgs_refactored.rb b/modules/claims_api/lib/bgs_service/local_bgs_refactored.rb index 17c8363b5e4..397fbdb8032 100644 --- a/modules/claims_api/lib/bgs_service/local_bgs_refactored.rb +++ b/modules/claims_api/lib/bgs_service/local_bgs_refactored.rb @@ -2,53 +2,14 @@ require 'claims_api/bgs_client' require 'claims_api/bgs_client/definitions' -require 'claims_api/bgs_client/error' -require 'bgs_service/local_bgs_refactored/error_handler' require 'bgs_service/local_bgs_refactored/find_definition' -require 'bgs_service/local_bgs_refactored/miscellaneous' module ClaimsApi - ## - # @deprecated Use {BGSClient.perform_request} instead. There ought to be a - # clear separation between the single method that performs the transport to - # BGS and any business logic that invokes said transport. By housing that - # single method as an instance method of this class, we encouraged - # business logic modules to inherit this class and then inevitably start to - # conflate business logic back into the transport layer here. There was a - # particularly easy temptation to put business object state validation as - # well as the dumping and loading of business object state into this layer, - # but that should live in the business logic layer and not here. Commentary - # about deprecation reasons is enumerated inline with the implementation - # below. - ## - # @deprecated Doesn't conform to Rails autoloading conventions, so we don't - # get it autoloaded across the project. - # class LocalBGSRefactored - ## - # @deprecated This bag of miscellaneous behavior is meant for callers of - # this, rather than being centralized here. - include Miscellaneous - - BAD_GATEWAY_EXCEPTIONS = [ - BGSClient::Error::ConnectionFailed, - BGSClient::Error::SSLError, - ## - # @deprecated According to VA API Standards, a timeout should correspond - # to the HTTP error code for a gateway timeout, 504: - # https://department-of-veterans-affairs.github.io/va-api-standards/errors/#choosing-an-error-code - # - BGSClient::Error::TimeoutError - ].freeze - class << self delegate :breakers_service, to: BGSClient end - ## - # @deprecated Can use default named arguments. Not really a deprecation - # reason. - # def initialize(external_uid:, external_key:) external_uid ||= Settings.bgs.external_uid external_key ||= Settings.bgs.external_key @@ -59,72 +20,5 @@ def initialize(external_uid:, external_key:) external_key: ) end - - ## - # @deprecated Prefer doing just transport against bundled BGS service action - # definitions rather than wrapping them at higher abstraction layers. - # - def make_request( # rubocop:disable Metrics/MethodLength - endpoint:, action:, body:, key: nil - ) - action = - FindDefinition.for_action( - endpoint, - action - ) - - request = - BGSClient.const_get(:Request).new( - action, external_id: @external_id - ) - - ## - # @deprecated Every service action result always lives within a particular - # key, so we can always extract the result rather than expose another - # option to the caller. - # - # @deprecated Prefer composing XML documents with an XML builder. Other - # approaches like templating into strings or immediately parsing and - # injecting with open-ended xpath are inconvenient or error-prone. - # - result = request.perform { |xml| xml << body.to_s } - result = { action.key => result } - result = result[key].to_h if key.present? - - request.send(:log_duration, 'transformed_response') do - ## - # @deprecated Callers should be hydrating domain entities anyway, so - # this transformation pass is wasteful. - # - result.deep_transform_keys! do |k| - k.underscore.to_sym - end - end - - result - rescue *BAD_GATEWAY_EXCEPTIONS - ## - # @deprecated We were determining our external interface extremely low in - # the stack by raising one of our externally facing application errors - # here. - # - raise ::Common::Exceptions::BadGateway - rescue BGSClient::Error::BGSFault => e - ## - # @deprecated This error handler handles the logic for multiple different - # callers rather than those callers handling their own logic. - # - ErrorHandler.handle_errors!(e) - {} - end - - ## - # @deprecated Prefer doing just transport against bundled BGS service action - # definitions rather than wrapping them at higher abstraction layers. - # - def healthcheck(endpoint) - service = FindDefinition.for_service(endpoint) - BGSClient.healthcheck(service) - end end end diff --git a/modules/claims_api/spec/lib/claims_api/local_bgs_proxy_spec.rb b/modules/claims_api/spec/lib/claims_api/local_bgs_proxy_spec.rb deleted file mode 100644 index bafda836afc..00000000000 --- a/modules/claims_api/spec/lib/claims_api/local_bgs_proxy_spec.rb +++ /dev/null @@ -1,83 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' -require 'bgs_service/local_bgs_proxy' - -describe ClaimsApi::LocalBGSProxy do - subject do - described_class.new( - external_uid: nil, - external_key: nil - ) - end - - expected_instance_methods = { - convert_nil_values: %i[options], - find_poa_by_participant_id: %i[id], - find_poa_history_by_ptcpnt_id: %i[id], - find_tracked_items: %i[id], - healthcheck: %i[endpoint], - jrn: %i[], - make_request: [endpoint: nil, action: nil, body: nil], - to_camelcase: [claim: nil], - transform_bgs_claim_to_evss: %i[claim], - transform_bgs_claims_to_evss: %i[claims], - validate_opts!: %i[opts required_keys] - } - - expected_instance_methods.each_value(&:freeze) - expected_instance_methods.freeze - - it 'defines the correct set of instance methods' do - actual = described_class.instance_methods(false) - [:proxied] - expect(actual).to match_array(expected_instance_methods.keys) - end - - describe 'claims_api_local_bgs_refactor feature toggling' do - before do - expect(Flipper).to( - receive(:enabled?) - .with(:claims_api_local_bgs_refactor) - .and_return(toggle) - ) - end - - define_singleton_method(:it_delegates_every_instance_method) do |to:| - it "has a proxied of type #{to}" do - expect(subject.proxied).to be_a(to) - end - - expected_instance_methods.each do |meth, args| - describe "when instance method is `#{meth}`" do - it "delegates to `#{to}`" do - if args.empty? - expect(subject.proxied).to receive(meth).with(no_args).once - subject.send(meth) - else - args = args.deep_dup - kwargs = args.extract_options! - expect(subject.proxied).to receive(meth).with(*args, **kwargs).once - subject.send(meth, *args, **kwargs) - end - end - end - end - end - - describe 'with refactor toggled off' do - let(:toggle) { false } - - it_delegates_every_instance_method( - to: ClaimsApi::LocalBGS - ) - end - - describe 'with refactor toggled on' do - let(:toggle) { true } - - it_delegates_every_instance_method( - to: ClaimsApi::LocalBGSRefactored - ) - end - end -end diff --git a/modules/claims_api/spec/lib/claims_api/local_bgs_refactored_spec.rb b/modules/claims_api/spec/lib/claims_api/local_bgs_refactored_spec.rb deleted file mode 100644 index 70f29262bf4..00000000000 --- a/modules/claims_api/spec/lib/claims_api/local_bgs_refactored_spec.rb +++ /dev/null @@ -1,90 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' -require 'bgs_service/local_bgs_proxy' -require 'bgs_service/e_benefits_bnft_claim_status_web_service' - -describe ClaimsApi::EbenefitsBnftClaimStatusWebService do - subject { described_class.new external_uid: 'xUid', external_key: 'xKey' } - - before do - allow(Flipper).to( - receive(:enabled?) - .with(:claims_api_local_bgs_refactor) - .and_return(true) - ) - end - - let(:soap_error_handler) { ClaimsApi::LocalBGSRefactored::ErrorHandler } - - # Testing potential ways the current check could be tricked - describe '#all' do - let(:subject_instance) { subject } - let(:id) { 12_343 } - let(:error_message) { { error: 'Did not work', code: 'XXX' } } - let(:bgs_unknown_error_message) { { error: 'Unexpected error' } } - let(:empty_array) { [] } - - context 'when an error message gets returned it still does not pass the count check' do - it 'returns an empty array' do - expect(error_message.count).to eq(2) # trick the claims count check - # error message should trigger return - allow(subject_instance).to( - receive(:find_benefit_claims_status_by_ptcpnt_id).with(id).and_return(error_message) - ) - expect(subject.all(id)).to eq([]) # verify correct return - end - end - - context 'when claims come back as a hash instead of an array' do - it 'casts the hash as an array' do - VCR.use_cassette('claims_api/bgs/claims/claims_trimmed_down') do - claims = subject_instance.find_benefit_claims_status_by_ptcpnt_id('600061742') - claims[:benefit_claims_dto][:benefit_claim] = claims[:benefit_claims_dto][:benefit_claim][0] - allow(subject_instance).to( - receive(:find_benefit_claims_status_by_ptcpnt_id).with(id).and_return(claims) - ) - - begin - ret = subject_instance.send(:transform_bgs_claims_to_evss, claims) - expect(ret.class).to_be Array - expect(ret.size).to eq 1 - rescue => e - expect(e.message).not_to include 'no implicit conversion of Array into Hash' - end - end - end - end - - # Already being checked but based on an error seen just want to lock this in to ensure nothing gets missed - context 'when an empty array gets returned it still does not pass the count check' do - it 'returns an empty array' do - # error message should trigger return - allow(subject_instance).to( - receive(:find_benefit_claims_status_by_ptcpnt_id).with(id).and_return(empty_array) - ) - expect(subject.all(id)).to eq([]) # verify correct return - end - end - - context 'when an error message gets returns unknown' do - it 'the soap error handler returns unprocessable' do - allow(subject_instance).to receive(:make_request).with(endpoint: 'PersonWebServiceBean/PersonWebService', - action: 'findPersonBySSN', - body: Nokogiri::XML::DocumentFragment.new( - Nokogiri::XML::Document.new - ), - key: 'PersonDTO').and_return(:bgs_unknown_error_message) - begin - allow(soap_error_handler).to receive(:handle_errors!) - .with(:bgs_unknown_error_message).and_raise(Common::Exceptions::UnprocessableEntity) - ret = soap_error_handler.send(:handle_errors!, :bgs_unknown_error_message) - expect(ret.class).to_be Array - expect(ret.size).to eq 1 - rescue => e - expect(e.message).to include 'Unprocessable Entity' - end - end - end - end -end