Skip to content

LG-8645 – Rename GetUspsProofingResultsJob method and improve tests#8365

Merged
tomas-nava merged 5 commits intomainfrom
tomas/lg-8645-rename-method-and-refactor-tests
May 9, 2023
Merged

LG-8645 – Rename GetUspsProofingResultsJob method and improve tests#8365
tomas-nava merged 5 commits intomainfrom
tomas/lg-8645-rename-method-and-refactor-tests

Conversation

@tomas-nava
Copy link
Contributor

🎫 Ticket

related to LG-8645

🛠 Summary of changes

Small refactor to rename a method in GetUspsProofingResultsJob and improve tests for the class.

@tomas-nava tomas-nava requested a review from svalexander May 9, 2023 14:49
Comment on lines +4 to +5
email_type:,
enrollment_status:,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added email_type as a new parameter and renamed status to enrollment_status

Comment on lines -47 to -49
before(:each) do
stub_request_passed_proofing_results
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this stub was causing only 'passed' emails to be sent, no matter what the test calling this shared example had configured

Comment on lines -56 to +65
email_type: anything,
email_type: email_type,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that we're no longer always sending 'passed' emails, we can expect a specific email_type

job.perform(Time.zone.now)
if email_type == 'deadline passed'
expect(job_analytics).to have_logged_event(
'GetUspsProofingResultsJob: deadline passed email initiated',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

expired enrollments trigger sending of a completely different type of email; now that we're no longer always sending 'passed' emails, we can test specifically for them

Comment on lines -1068 to -1075
expect(job_analytics).to have_logged_event(
'GetUspsProofingResultsJob: Success or failure email initiated',
email_type: 'Failed',
enrollment_code: pending_enrollment.enrollment_code,
service_provider: anything,
timestamp: anything,
wait_until: nil,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this expectation was redundant with the 'enrollment_with_a_status_update' shared example above

analytics(user: enrollment.user).idv_in_person_usps_proofing_results_job_email_initiated(
**email_analytics_attributes(enrollment),
email_type: 'Failed',
email_type: 'Failed unsupported secondary ID',
Copy link
Contributor

Choose a reason for hiding this comment

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

my one thought with this is that there isn't really a separate version of the failed email for unsupported ids. We have a standard failure email (which is the one we're sending in this case) and a failure b/c of suspected fraud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I was hesitant at first, but noticed that we log 'Failed unsupported ID type' in handle_unsupported_id_type above, and think this message would be useful for filtering when searching logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok that makes sense to me then

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this event is established, it'd be good to first confirm there has been a discussion w/ product to check if they currently have dashboards or reports that would need to be adjusted for the updated value.

Copy link
Contributor

@svalexander svalexander left a comment

Choose a reason for hiding this comment

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

LGTM

@tomas-nava tomas-nava merged commit ac59e06 into main May 9, 2023
@tomas-nava tomas-nava deleted the tomas/lg-8645-rename-method-and-refactor-tests branch May 9, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants