Skip to content
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.

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

LG-15000: Remove in_person_please_call email #11664

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

matthinz
Copy link
Member

(This PR is 3 of 3. It can't be merged until #11604 and #11662 are in production)

🎫 Ticket

Link to the relevant ticket:
LG-15000

🛠 Summary of changes

#11604 added a new idv_please_call email, which is identical to in_person_please_call.

#11662 started sending that email from other parts of IDV and updated the in-person code to send idv_please_call instead of in_person_please_call

This PR removes references to in_person_please_call.

@matthinz matthinz requested a review from a team December 18, 2024 00:13
@matthinz matthinz force-pushed the matthinz/15000-idv-please-call-part-2 branch from e917b8a to 6360574 Compare December 18, 2024 00:20
@matthinz matthinz force-pushed the matthinz/15000-idv-please-call-part-3 branch from 6975326 to 3cf4651 Compare December 18, 2024 00:22
@matthinz matthinz force-pushed the matthinz/15000-idv-please-call-part-2 branch 2 times, most recently from 10eecd3 to 51cc5a3 Compare December 18, 2024 06:24
- Slightly improve error output
- Capture behavior in a dedicated spec

[skip changelog]
Assert that a certain email was not sent.
These specs weren't actually testing the thing we thought they were, since the text_part of the email did not contain HTML.
This is the exact same email, content-wise, just with a new name to reflect the fact that it is used throughout idv.

[skip changelog]
@matthinz matthinz force-pushed the matthinz/15000-idv-please-call-part-2 branch from 51cc5a3 to c50cb70 Compare December 18, 2024 18:26
Many clients won't display inline images referenced by URL for privacy reasons.
This has been renamed to idv_please_call.

[skip changelog]
@matthinz matthinz force-pushed the matthinz/15000-idv-please-call-part-3 branch from 3cf4651 to 5b4faea Compare December 18, 2024 18:29
Copy link
Member

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

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

Obviously this can't go to prod yet, but the code looks good.

@@ -2,7 +2,7 @@
require_relative './user_mailer_preview'

RSpec.describe UserMailerPreview do
it_behaves_like 'a mailer preview', preview_methods_that_can_be_missing: [:in_person_please_call]
Copy link
Member

Choose a reason for hiding this comment

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

Academic curiosity: Is preview_methods_that_can_be_missing a metaprogramming thing I'm not aware of, or was this just a symbolic note for humans? I don't see this as a method anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just some syntactic sugar. Rewritten:

it_behaves_like(
  'a mailer preview',
   { preview_methods_that_can_be_missing:[:in_person_please_call] }
)

Where the first argument is the name of the shared example and anything after is an argument passed to the parameterized example.

Copy link
Contributor

@lmgeorge lmgeorge left a comment

Choose a reason for hiding this comment

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

issue: This method is also called in GetUspsProofingResultsJob. (I don't understand how the spec for that job is passing if you've eliminated the method though.

In GetUspsProofingResultsJob
https://github.com/18F/identity-idp/blob/main/app/jobs/get_usps_proofing_results_job.rb#L610-L619

In the get_usps_proofing_results_job_spec:

allow(user_mailer).to receive(:in_person_please_call).and_return(mail_deliverer)

it 'sends the please call email' do
expect(user_mailer).to have_received(:in_person_please_call).with(
enrollment: enrollment,
visited_location_name: visited_location_name,
)

@matthinz
Copy link
Member Author

@lmgeorge Yeah, this PR is dependent on #11662 landing first, which removes those references.

@lmgeorge
Copy link
Contributor

Yeah, this PR is dependent on #11662 landing first, which removes those references.

Dang it, I misremembered the initial description as 1 of 3. 🫠

Copy link
Contributor

@lmgeorge lmgeorge left a comment

Choose a reason for hiding this comment

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

Ignore previous comment, user error! This looks great.

Base automatically changed from matthinz/15000-idv-please-call-part-2 to main December 23, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants