LG-7275 Expand metrics for USPS proofing attempts by eileen-nava · Pull Request #6967 · 18F/identity-idp

eileen-nava · 2022-09-15T15:25:41Z

Resolves jira ticket #7275: Collect consistent USPS proofing attempt metrics

Summary

This PR increases the metrics that are collected during USPS proofing attempts. It includes a new method, response_analytics_attributes, which logs the below attributes during both successful and unsuccessful proofing attempts:

fraudSuspected
primaryIdType
secondaryIdType
failureReason
fraudSuspected
proofingState
transactionEndDateTime
transactionStartDateTime
status
assuranceLevel
proofingPostOffice
proofingCity
proofingState
scanCount
responseMessage

The above attributes will not be logged for all USPS proofing attempts that raise exceptions. For example, if there is an exception where the response is not in hash format, then these attributes won't be logged.

changelog: Internal, In-person proofing, expand metrics for proofing attempts

svalexander · 2022-09-16T18:51:54Z

can you link the ticket in the summary?

eileen-nava · 2022-09-16T18:56:52Z

can you link the ticket in the summary?

Thanks for noting that. I linked the ticket in the summary.

app/jobs/get_usps_proofing_results_job.rb

sheldon-b · 2022-09-19T15:16:45Z

spec/jobs/get_usps_proofing_results_job_spec.rb

-        before(:each) do
-          stub_request_passed_proofing_results
-        end
+        let!(:response) { stub_request_passed_proofing_results }

        it_behaves_like('enrollment with a status update', passed: true, status: 'passed')

        it 'logs details about the success' do
          job.perform(Time.zone.now)

+          response_as_json = JSON.parse(response[:body])
          expect(job_analytics).to have_logged_event(
            'GetUspsProofingResultsJob: Enrollment status updated',
-            fraud_suspected: false,
            reason: 'Successful status update',
+            fraud_suspected: response_as_json['fraudSuspected'],
+            primary_id_type: response_as_json['primaryIdType'],
+            secondary_id_type: response_as_json['secondaryIdType'],
+            failure_reason: response_as_json['failureReason'],
+            transaction_end_date_time: response_as_json['transactionEndDateTime'],
+            transaction_start_date_time: response_as_json['transactionStartDateTime'],
+            status: response_as_json['status'],
+            assurance_level: response_as_json['assuranceLevel'],
+            proofing_post_office: response_as_json['proofingPostOffice'],
+            proofing_city: response_as_json['proofingCity'],
+            proofing_state: response_as_json['proofingState'],
+            scan_count: response_as_json['scanCount'],
+            response_message: response_as_json['responseMessage'],


Could we move this all to the 'enrollment with a status update' shared example by doing this?

before do stub_request_passed_proofing_results end it_behaves_like( 'enrollment with a status update', passed: true, response_json: UspsInPersonProofing::Mock::Fixtures.request_passed_proofing_results_response, status: 'passed', )

And then in the shared example we could do this:

RSpec.shared_examples 'enrollment with a status update' do |passed:, status:, response_json:| it 'logs a message with common attributes' do <...omitted code...> response = JSON.parse(response_json) expect(job_analytics).to have_logged_event( 'GetUspsProofingResultsJob: Enrollment status updated', assurance_level: response['assuranceLevel'], enrollment_code: pending_enrollment.enrollment_code, enrollment_id: pending_enrollment.id, failure_reason: response['failureReason'], fraud_suspected: response['fraudSuspected'], minutes_since_last_status_check: 15.0, minutes_since_last_status_update: 2.days.in_minutes, minutes_to_completion: 3.days.in_minutes, passed: passed, primary_id_type: response['primaryIdType'], proofing_city: response['proofingCity'], proofing_post_office: response['proofingPostOffice'], proofing_state: response['proofingState'], response_message: response['responseMessage'], scan_count: response['scanCount'], secondary_id_type: response['secondaryIdType'], status: response['status'], transaction_end_date_time: response['transactionEndDateTime'], transaction_start_date_time: response['transactionStartDateTime'], ) end

And we could then use that shared example for most the test cases where you've added these checks. I like this approach because the shared examples already check for logged attributes and this would reduce a lot of similar looking lines of test code. And then we could revert the changes to usps_ipp_helper.rb

zachmargolis · 2022-09-20T21:34:45Z

app/jobs/get_usps_proofing_results_job.rb

+      fraud_suspected: response['fraudSuspected'],
+      primary_id_type: response['primaryIdType'],
+      secondary_id_type: response['secondaryIdType'],
+      failure_reason: response['failureReason'],
+      transaction_end_date_time: response['transactionEndDateTime'],
+      transaction_start_date_time: response['transactionStartDateTime'],
+      status: response['status'],


IMO I think all this switching of cases might be a good hint to move the processing of the response to the USPS proofer client class, have it return a Struct or equivalent object that has snake_case keys already (similar to https://cm-jira.usa.gov/browse/LG-7025)

Thanks for raising this point! I will make another ticket to address it.

eileen-nava · 2022-09-21T17:05:47Z

Heads-up: This PR is not ready for re-review. I will post when it is. Almost there!

eileen-nava · 2022-09-21T16:27:58Z

app/jobs/get_usps_proofing_results_job.rb

+      fraud_suspected: response['fraudSuspected'],
+      primary_id_type: response['primaryIdType'],
+      secondary_id_type: response['secondaryIdType'],
+      failure_reason: response['failureReason'],
+      transaction_end_date_time: response['transactionEndDateTime'],
+      transaction_start_date_time: response['transactionStartDateTime'],
+      status: response['status'],


Thanks for raising this point! I will make another ticket to address it.

eileen-nava · 2022-09-21T16:31:59Z

app/services/usps_in_person_proofing/mock/fixtures.rb

      end

-      def self.request_passed_proofing_unsupported_status_results_response
+      def self.request_passed_proofing_unsupported_status_response


I shortened this method name because it set off rubocop's "line too long" check. I realize that the method name is no longer consistent with the many self.foo_results_response methods in this file. I thought about renaming those methods to self.foo_response, but decided to prioritize making the smallest change possible.

If other folks think it's worth it to refactor the self.foo_results_response methods to self.foo_response, please give a shout.

There's a lot of ways we can work around the Rubocop restriction, and I'd personally prefer to pursue those before choosing renaming the method, or at least when we'd be introducing inconsistency by doing so (like you mentioned).

Example:

it_behaves_like( 'enrollment encountering an exception', reason: 'Bad response structure', response_json: UspsInPersonProofing::Mock::Fixtures. request_passed_proofing_unsupported_status_result_response, )

Agreed. I'll take the approach you suggest.

eileen-nava · 2022-09-21T16:34:22Z

...rvices/usps_in_person_proofing/mock/responses/request_expired_proofing_results_response.json

+  "transactionEndDateTime": "12/17/2020 034055",
+  "secondaryIdType": "Deed of Trust",
+  "responseMessage": "More than 30 days have passed since opt-in to IPP",
+  "fraudSuspected": null,


I changed this value because I don't think we'd suspect fraud for an expired id. Also, the related method, handle_expired_status_update, has the parameter fraud_suspected: nil for the method idv_in_person_usps_proofing_results_job_enrollment_updated.

The USPS API documentation specifies that this field would only ever be true or false, so I think the fixtures should reflect the values we'd actually expect.

(Aside: Curious if these fixtures were provided to us by USPS directly. Their documentation claims the type of this field is a String of true or false, so I'm curious if that means true and false, or if it means "true" and "false". The previous fixture code would imply the former.)

I just checked in dev where we have some expired enrollments -- the USPS response body is this:

{"responseMessage"=>"More than 30 days have passed since opt-in to IPP"}

While I'm there I checked on the value of fraud_suspected for a passed enrollment. Looks like it returns the value as a boolean:

"fraudSuspected"=>false,

Can we update the expired mock response json value to match that actual response I got back @eileen-nava?

Absolutely! I'll update the expired mock response json value to match that actual response. Thanks for pointing this out, @aduth and @sheldon-b.

Thanks for checking those @sheldon-b !

eileen-nava · 2022-09-21T18:28:22Z

This PR is ready for re-review. Thanks for everyone who provided feedback so far.

eileen-nava · 2022-09-22T17:13:39Z

Update: I responded to the most recent round of feedback.

sheldon-b

Nice! Looking good. I have a few more comments, mostly minor styling things that would be nice to enforce with a linter (separate PR/ticket, not asking you to do that)

sheldon-b · 2022-09-22T17:24:15Z

...rvices/usps_in_person_proofing/mock/responses/request_expired_proofing_results_response.json

+  "status": "In-person expired",
+  "proofingPostOffice": "WILKES BARRE",
+  "proofingCity": "WILKES BARRE",
+  "proofingState": "PA",
+  "enrollmentCode": "2090002197604352",
+  "primaryIdType": "Uniformed Services identification card",
+  "transactionStartDateTime": "12/17/2020 033855",
+  "transactionEndDateTime": "12/17/2020 034055",
+  "secondaryIdType": "Deed of Trust",
+  "responseMessage": "More than 30 days have passed since opt-in to IPP",
+  "fraudSuspected": false,
+  "proofingConfirmationNumber": "350040248346707",
+  "ippAssuranceLevel": "1.5"
+}


Per this comment I suggest deleting most of these fields because USPS doesn't actually return them for expired enrollments

Suggested change

"status": "In-person expired",

"proofingPostOffice": "WILKES BARRE",

"proofingCity": "WILKES BARRE",

"proofingState": "PA",

"enrollmentCode": "2090002197604352",

"primaryIdType": "Uniformed Services identification card",

"transactionStartDateTime": "12/17/2020 033855",

"transactionEndDateTime": "12/17/2020 034055",

"secondaryIdType": "Deed of Trust",

"responseMessage": "More than 30 days have passed since opt-in to IPP",

"fraudSuspected": false,

"proofingConfirmationNumber": "350040248346707",

"ippAssuranceLevel": "1.5"

}

"responseMessage": "More than 30 days have passed since opt-in to IPP"

}

sheldon-b · 2022-09-22T17:25:50Z

spec/jobs/get_usps_proofing_results_job_spec.rb

-                                                                reason: 'Request exception'|
+                                                                reason: 'Request exception',
+                                                                response_json: {}|


Doesn't look like response_json is used in these shared examples. Should we revert this change?

sheldon-b · 2022-09-22T17:28:18Z

spec/jobs/get_usps_proofing_results_job_spec.rb

+        it_behaves_like(
+          'enrollment with a status update', passed: true, status: 'passed',
+                                             response_json: UspsInPersonProofing::Mock::Fixtures.
+                                             request_passed_proofing_results_response
+        )


I believe this style is more consistent with the rest of the codebase (and a little easier to read imo). I think I'll see if there's a rubocop rule to enforce this better in a separate PR

Similar comment for the other places below with the same styling

Suggested change

it_behaves_like(

'enrollment with a status update', passed: true, status: 'passed',

response_json: UspsInPersonProofing::Mock::Fixtures.

request_passed_proofing_results_response

)

it_behaves_like(

'enrollment with a status update',

passed: true,

status: 'passed',

response_json: UspsInPersonProofing::Mock::Fixtures.

request_passed_proofing_results_response

)

sheldon-b · 2022-09-22T17:29:51Z

spec/jobs/get_usps_proofing_results_job_spec.rb

-            failure_reason: 'Clerk indicates that ID name or address does not match source data.',
-            fraud_suspected: false,
-            primary_id_type: 'Uniformed Services identification card',
-            proofing_state: 'PA',
-            reason: 'Failed status',
-            secondary_id_type: 'Deed of Trust',
-            transaction_end_date_time: '12/17/2020 034055',
-            transaction_start_date_time: '12/17/2020 033855',


spec/jobs/get_usps_proofing_results_job_spec.rb

eileen-nava added 2 commits September 14, 2022 17:13

expand metrics for USPS proofing attempts

fba34bf

Merge branch 'main' into em/7275-proof-attempt-metrics

623a90e

eileen-nava force-pushed the em/7275-proof-attempt-metrics branch from ccf6192 to ea826da Compare September 16, 2022 16:37

LG-7275 Expand metrics for USPS proofing attempts

dc29936

changelog: Internal, In-person proofing, expand metrics for proofing attempts

eileen-nava force-pushed the em/7275-proof-attempt-metrics branch from ea826da to dc29936 Compare September 16, 2022 18:19

eileen-nava marked this pull request as ready for review September 16, 2022 18:39

eileen-nava requested review from a team and NavaTim September 16, 2022 18:42

svalexander reviewed Sep 16, 2022

View reviewed changes

app/jobs/get_usps_proofing_results_job.rb Show resolved Hide resolved

aduth reviewed Sep 16, 2022

View reviewed changes

app/jobs/get_usps_proofing_results_job.rb Show resolved Hide resolved

sheldon-b reviewed Sep 19, 2022

View reviewed changes

enumerate params from analytics methods

63d66bd

zachmargolis reviewed Sep 20, 2022

View reviewed changes

refactor usps proofing tests to be DRYer

689c5bc

clean up usps proofing job and spec

25ca8a9

eileen-nava commented Sep 21, 2022

View reviewed changes

tweak fixtures for expired id & unsupported status

ae8baf4

sheldon-b approved these changes Sep 22, 2022

View reviewed changes

clean up formatting and fixture

a94dbd7

eileen-nava merged commit 6be4a5e into main Sep 22, 2022

eileen-nava deleted the em/7275-proof-attempt-metrics branch September 22, 2022 19:55

aduth mentioned this pull request Sep 22, 2022

LG-6896: Send 'fraud suspected' in-person proofing failed email #7013

Merged

solipet mentioned this pull request Sep 26, 2022

Deploy RC 212 to Prod #7031

Merged

Conversation

eileen-nava commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

svalexander commented Sep 16, 2022

Uh oh!

eileen-nava commented Sep 16, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileen-nava commented Sep 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileen-nava commented Sep 21, 2022

Uh oh!

eileen-nava commented Sep 22, 2022

Uh oh!

sheldon-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eileen-nava commented Sep 15, 2022 •

edited

Loading