Skip to content

LG-9065 Make the DDP result class and proofer responsible for building the result hash #8160

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.

Sign up for GitHub

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

Merged
jmhooper merged 1 commit into from
Apr 10, 2023
+27 −43
Merged

LG-9065 Make the DDP result class and proofer responsible for building the result hash #8160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions app/jobs/resolution_proofing_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,18 +85,7 @@ def logger_info_hash(hash)
def add_threatmetrix_result_to_callback_result(callback_log_data:, threatmetrix_result:)
exception = threatmetrix_result.exception.inspect if threatmetrix_result.exception

response_h = Proofing::LexisNexis::Ddp::ResponseRedacter.
redact(threatmetrix_result.response_body)
callback_log_data.result[:context][:stages][:threatmetrix] = {
client: lexisnexis_ddp_proofer.class.vendor_name,
errors: threatmetrix_result.errors,
exception: exception,
success: threatmetrix_result.success?,
timed_out: threatmetrix_result.timed_out?,
transaction_id: threatmetrix_result.transaction_id,
review_status: threatmetrix_result.review_status,
response_body: response_h,
}
callback_log_data.result[:context][:stages][:threatmetrix] = threatmetrix_result.to_h

if exception.present?
callback_log_data.result.merge!(
Expand Down
24 changes: 17 additions & 7 deletions app/services/proofing/ddp_result.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,30 @@ module Proofing
class DdpResult
attr_reader :exception
attr_accessor :context,
:success,
:transaction_id,
:reference,
:review_status,
:response_body
:response_body,
:client

def initialize(
success: true,
errors: {},
context: {},
exception: nil,
transaction_id: nil,
reference: nil,
response_body: nil
review_status: nil,
response_body: nil,
client: nil
)
@success = success
@errors = errors
@context = context
@exception = exception
@transaction_id = transaction_id
@reference = reference
@response_body = response_body
@review_status = review_status
@client = client
end

# rubocop:disable Style/OptionalArguments
Expand All @@ -47,7 +52,7 @@ def failed?
end

def success?
!exception? && !errors?
@success
end

def timed_out?
Expand All @@ -56,9 +61,14 @@ def timed_out?

def to_h
{
client: client,
success: success?,
errors: errors,
exception: exception,
success: success?,
timed_out: timed_out?,
transaction_id: transaction_id,
review_status: review_status,
response_body: Proofing::LexisNexis::Ddp::ResponseRedacter.redact(response_body),
}
end
end
Expand Down
5 changes: 4 additions & 1 deletion app/services/proofing/lexis_nexis/ddp/proofer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def proof(applicant)
build_result_from_response(response)
rescue => exception
NewRelic::Agent.notice_error(exception)
Proofing::DdpResult.new(exception: exception)
Proofing::DdpResult.new(success: false, exception: exception)
end

private
Expand All @@ -63,6 +63,9 @@ def build_result_from_response(verification_response)
result.add_error(:request_result, request_result) unless request_result == 'success'
result.add_error(:review_status, review_status) unless review_status == 'pass'

result.success = !result.errors?
result.client = 'lexisnexis'
Comment on lines +66 to +67
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be clearer to pass in client to the constructor? DdpResult.new(client: 'lexisnexis')

Copy link
Copy Markdown
Contributor Author

@jmhooper jmhooper Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't because of the add_error construction this thing has. That makes it tough to pass in success to the constructor so you'll end up doing something like this:

body = verification_response.response_body

request_result = body['request_result']
review_status = body['review_status']

validate_review_status!(review_status)

result = Proofing::DdpResult.new(
  client: 'lexisnexis',
  response_body: body,
  transaction_id: body['request_id'],
  review_status: review_status,
)

result.add_error(:request_result, request_result) unless request_result == 'success'
result.add_error(:review_status, review_status) unless review_status == 'pass'

result.success = !result.errors?

result

That just looks kinda funny and discontinuous, but I am happy to change it if you think it is better this way.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm yeah that does look funny, maybe leave it as-is for now, but this extra processing of the result seems like maybe one class or the other is doing too much?

Copy link
Copy Markdown
Contributor Author

@jmhooper jmhooper Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the #add_error construction here is the primary divergence from the pattern we use elsewhere. I left it in to try and keep this as small as I could. I can go back around and get it. My primary goal here was to get the #to_h call to work correctly in ResolutionProofingJob.


result
end

Expand Down
28 changes: 5 additions & 23 deletions spec/services/proofing/ddp_result_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,22 +66,15 @@
describe '#success?' do
subject { result.success? }

context 'when there is an error AND an exception' do
let(:result) do
Proofing::DdpResult.new(exception: StandardError.new).add_error('foobar')
end
it { is_expected.to eq(false) }
context 'when it is successful' do
let(:result) { Proofing::DdpResult.new(success: true) }
it { is_expected.to eq(true) }
end

context 'when there is an error and no exception' do
let(:result) { Proofing::DdpResult.new.add_error('foobar') }
context 'when it is unsuccessful' do
let(:result) { Proofing::DdpResult.new(success: false) }
it { is_expected.to eq(false) }
end

context 'when there is no error and no exception' do
let(:result) { Proofing::DdpResult.new }
it { is_expected.to eq(true) }
end
end

describe '#timed_out?' do
Expand Down Expand Up @@ -124,15 +117,4 @@
end
end
end

describe 'reference' do
context 'when provided' do
it 'is present' do
reference = SecureRandom.uuid
result = Proofing::DdpResult.new
result.reference = reference
expect(result.reference).to eq(reference)
end
end
end
end