LG-12091 Send please call email by svalexander · Pull Request #9977 · 18F/identity-idp

svalexander · 2024-01-25T21:04:56Z

🎫 Ticket

Link to the relevant ticket:
LG-12091

🛠 Summary of changes

This pr sends a "Please call" email when a user who has fraud review pending gets passed by usps.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

En:

Es:

Fr:

app/assets/images/email/phone-icon.png

app/views/user_mailer/in_person_please_call.html.erb

WheresHJ · 2024-01-26T18:59:59Z

There is some bolding missing in the second paragraph, as follows:

Call (844) 875-6446 and provide them with the error code ABCD.

Also not sure why the line break in the second paragraph is so different from the Figma, but it's OK.

WheresHJ · 2024-01-26T19:08:04Z

Also, the footer styling is substantially different from the Figma, but this seems to actually be the case in all our emails. Don't think we should fix this here, but might be worth a separate task or investigation @n1zyy

n1zyy · 2024-01-26T19:28:23Z

Also, the footer styling is substantially different from the Figma, but this seems to actually be the case in all our emails. Don't think we should fix this here, but might be worth a separate task or investigation @n1zyy

Interesting! My first thought was that this was maybe an issue with the mail preview itself, but it is not. Live mail I've received looks the same.

The core email template is https://github.com/18F/identity-idp/blob/main/app/views/layouts/user_mailer.html.erb for all these.

I agree about splitting that out into a separate ticket from this one. It probably makes sense for our team to pick it up, but FWIW it's the template used by all emails. fe08811 is where the "Sent at" came in.

n1zyy

I really need to get my PR in since I think this is the logical successor to it.

I think instead of your handle_call_in_needed method, we just want to drop your email send into the code I'm adding for handling fraud_pending_reason users who proof. In hindsight the ticket wasn't super clear about this.

But otherwise, this is looking good.

lib/identity_config.rb

config/locales/user_mailer/en.yml

app/jobs/get_usps_proofing_results_job.rb

WheresHJ · 2024-01-26T19:45:22Z

@n1zyy Yeah, just wanted to flag it while there was an active example. Maybe let's try to track it down and connect with @eryn-sobing in planning.

svalexander · 2024-01-26T20:59:33Z

I think instead of your handle_call_in_needed method, we just want to drop your email send into the code I'm adding for handling fraud_pending_reason users who proof. In hindsight the ticket wasn't super clear about this.

@n1zyy I'll wait to merge your changes into my branch before finalizing this pr

app/jobs/get_usps_proofing_results_job.rb

aduth · 2024-01-30T19:27:59Z

app/services/analytics_events.rb


+  # Tracks please call emails that are initiated during GetUspsProofingResultsJob
+  def idv_in_person_usps_proofing_results_job_please_call_email_initiated(
+    **extra


This may fail when merged to main since #9946 was merged and requires explicit documentation for every received analytics argument. I'd suggest merging main or rebasing the branch to make sure that check passes.

since it's it's called from within an existing job, it might get a free pass due to allowed_extra_analytics: [:*] on many existing files

I am wondering how I can use a string rather than the method name in track_event() like other analytics events do.

Method name is the expected event name, this is a recent-ish change in process which is why other events log a string.

Related: https://github.com/18F/identity-idp/blob/main/docs/backend.md#analytics

…en user in review passes usps check

JackRyan1989

This looks great so far! I just have a few small comments and questions that should be fairly easy to resolve. Good job Shannon!

JackRyan1989 · 2024-02-01T15:45:42Z

app/jobs/get_usps_proofing_results_job.rb

    end
  end

+  def handle_call_in_needed(enrollment, response)


Nit: This method name is slightly confusing to me. Could we include 'fraud' somewhere in the name so it's clear it relates directly to fraud?

Good idea, updated to handle_passed_with_fraud_review_pending

app/jobs/get_usps_proofing_results_job.rb

JackRyan1989 · 2024-02-01T15:57:06Z

app/jobs/get_usps_proofing_results_job.rb


+  def send_please_call_email(user, enrollment)
+    user.confirmed_email_addresses.each do |email_address|
+      # rubocop:disable IdentityIdp/MailLaterLinter


Nit: Is there an easy-ish formatting thing we can do so we're not disabling the linter? I feel like that's a bit of a flag.

This is a custom rule that enforces the use of deliver_now_or_later vs deliver_later. I'd wonder if we should be using deliver_now_and_later to avoid the lint? There are a few cases where we want to ensure it's delivered exclusively now or later, but I'm not sure that applies here.

https://github.com/18F/identity-idp/blob/main/lib/linters/mail_later_linter.rb

Ok interesting. Thanks for that info @aduth. Yeah unfortunately I don't know enough about the motivation for enforcing that we do these calls async. This might be a good thing to bring back to our team and see what folks think.

The idea with deliver_now_or_later is that we'd typically want to deliver messages asynchronously (to avoid blocking the response), but this is controlled through configuration to allow us the option to deliver synchronously if needed in an environment or if there's issues with asynchronous delivery.

See also:

identity-idp/config/initializers/ext_action_mailer.rb

Lines 14 to 20 in 21d5b41

def deliver_now_or_later(opts = {})

if IdentityConfig.store.deliver_mail_async

deliver_later(opts)

else

deliver_now

end

end

digging back through some old prs and it looks like we previously made the decision to use deliver_later for the emails sent in this job, pr where we made the switch

I generally don't like the pattern of rubocop:disable for stuff without a really compelling reason. But, as you pointed out, and as is found in some old Slack threads, there's precedent here: every other mailer call in this file is deliver_later.

So, in the end, unless others object, I would support leaving this as-is. I'd rather the file stay consistent, even if it's not strictly necessary in this case.

n1zyy · 2024-02-02T19:55:22Z

Also, the footer styling is substantially different from the Figma, but this seems to actually be the case in all our emails. Don't think we should fix this here, but might be worth a separate task or investigation @n1zyy

Filed LG-12314 for this so it doesn't get lost.

n1zyy

Sorry for being slow on the update here, but I'm now on Team here. Thanks for your patience in getting this story through!

add email ui and preview

f0c395e

svalexander marked this pull request as draft January 25, 2024 21:05

aduth reviewed Jan 25, 2024

View reviewed changes

app/assets/images/email/phone-icon.png Outdated Show resolved Hide resolved

svalexander requested review from a team and carmenrosalop and removed request for a team January 26, 2024 16:38

zachmargolis reviewed Jan 26, 2024

View reviewed changes

app/views/user_mailer/in_person_please_call.html.erb Outdated Show resolved Hide resolved

send email in usps job

005ba2a

n1zyy reviewed Jan 26, 2024

View reviewed changes

lib/identity_config.rb Show resolved Hide resolved

config/locales/user_mailer/en.yml Outdated Show resolved Hide resolved

app/jobs/get_usps_proofing_results_job.rb Outdated Show resolved Hide resolved

bold number and code

952de5b

svalexander commented Jan 26, 2024

View reviewed changes

app/jobs/get_usps_proofing_results_job.rb Outdated Show resolved Hide resolved

svalexander added 8 commits January 26, 2024 17:48

add strings and remove activate profile

6f6b7b3

add transations

aed1583

Merge branch 'main' into shannon/lg-12091-send-please-call-email

2027a98

add compressed icon

cfbccec

delete larger file

fa98dc9

rename image and use app_name

1ca20a3

fix email not sending

9a82efc

update analytics event

d32c38b

aduth reviewed Jan 30, 2024

View reviewed changes

svalexander added 4 commits January 31, 2024 11:20

add specs

76a08d4

Merge branch 'main' into shannon/lg-12091-send-please-call-email

ad3256e

update analytics event

5715bba

changelog: User-Facing Improvements, Please call email, send email wh…

89dc098

…en user in review passes usps check

svalexander marked this pull request as ready for review January 31, 2024 19:18

svalexander changed the title ~~add email ui and preview~~ LG-12091 Send please call email Jan 31, 2024

svalexander requested review from a team and JackRyan1989 January 31, 2024 19:20

lint fix for please call view

9d8aead

JackRyan1989 reviewed Feb 1, 2024

View reviewed changes

svalexander added 2 commits February 1, 2024 12:00

updates to analytics event and spec

d85df46

change method name

adb7c10

n1zyy approved these changes Feb 2, 2024

View reviewed changes

svalexander merged commit 759241d into main Feb 5, 2024

svalexander deleted the shannon/lg-12091-send-please-call-email branch February 5, 2024 15:09

mitchellhenke mentioned this pull request Feb 6, 2024

Deploy RC 353 to Production #10046

Merged

svalexander mentioned this pull request Mar 7, 2024

LG-12317 add es and fr translations for please call email #10202

Merged

4 tasks

	def deliver_now_or_later(opts = {})
	if IdentityConfig.store.deliver_mail_async
	deliver_later(opts)
	else
	deliver_now
	end
	end

Conversation

svalexander commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

Uh oh!

Uh oh!

Uh oh!

WheresHJ commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WheresHJ commented Jan 26, 2024

Uh oh!

n1zyy commented Jan 26, 2024

Uh oh!

n1zyy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WheresHJ commented Jan 26, 2024

Uh oh!

svalexander commented Jan 26, 2024

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!

Choose a reason for hiding this comment

Uh oh!

JackRyan1989 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1zyy Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1zyy commented Feb 2, 2024

Uh oh!

n1zyy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

svalexander commented Jan 25, 2024 •

edited

Loading

WheresHJ commented Jan 26, 2024 •

edited

Loading

n1zyy Feb 2, 2024 •

edited

Loading