LG-7185 email reminders

[svalexander]

🎫 Ticket

Lg-7185

🛠 Summary of changes

This ticket is to create a reminder email, a job to check days left till enrollment due date and to add a flag to the enrollment object.
This first pr is to add a template for the reminder email. Currently this email should be sent when the user has 10 days left to enroll, and then again at 3 days left. The only difference between the 3 day and 10 day emails is the number of days. For this reason a singular template is used.
In the InPersonEnrollment a method is added that counts the number of days until the due_date. This method is used by the presenter, and will eventually be used by the job that is later written.
This reminder email displays the same information as the ready to verify email does - the only differences being the subject, header and intro paragraph. For this reason this pr uses the same presenter with some minor additions to the presenter.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Change days_to_due_date method so today is equal to 3 days before enrollment due_date
• Navigate to http://localhost:3000/rails/mailers/user_mailer/in_person_ready_to_verify_reminder.html?locale=en
• Ensure that email subject and headings have the correct number days
• Repeat for 10 days

👀 Screenshots

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

en - 3 days:

en- 10 days:

es- 3 days:

es- 10 days:

fr- 3 days:

fr- 10 days:

[sheldon-b]

Looks good! I left a mix of non-blocking suggestions and small requests. And then the one kind of major thing is making sure that the partner agency name value we show to users is a human-friendly one

[sheldon-b]

This is non-blocking, it's kind of beyond the scope of this ticket. But I feel like expiration_date is a slightly better name, it better describes what the consequence of this date is and relates to the expired status of enrollments. Would require updating a few other places in the code to make this change cohesive though 🤷

Suggested change

	def due_date
	def expiration_date

[sheldon-b]

Same as above, non-blocking suggestion

Suggested change

	def days_to_due_date
	def days_to_expiration

[sheldon-b]

I believe this value is a machine-readable value, not a human-readable one. eg urn:gov:gsa:openidconnect:sp:sinatra. I think we'd need to get the value from the ServiceProvider.friendly_name field, similar to how we do it here. I prefer that naming too, sp_name, to follow the convention we already have in the code

[svalexander]

i'm thinking through this, it definitely makes sense to ensure that the name is easy to read.Looking at what's happening w/ the example you linked to sp_name gets passed to the presenter (and the name is retrieved from the session). What i'm thinking through right now is this presenter is used for 2 emails, one of which doesn't need the sp_name - i guess it can be an optional param or we could add a method that gets the friendly name using the issuer.

[sheldon-b]

Oh, we do something similar in the "You successfully verified with IPP" email. We pull the human-friendly service provider name off of the enrollment. Here is where it's used in the email and then here's where it's defined in the presenter.

This seems closer to how you're doing it already and wouldn't require us to pass in the sp_name to the constructor 🤷

[svalexander]

yup i think that matches what we're trying to do here. i think what's tripping me up when it comes to the enrollment model is that i don't see service_provider as part of the schema and the model says the enrollment belongs to the service_provider. Does the service_provider conversely belong to the enrollment?

[sheldon-b]

ServiceProvider has_many enrollments. But yeah Rails adds that link implicitly for us based on how the enrollment belongs_to :service_provider

[sheldon-b]

That particular relationship is kind of unusual because we use the issuer as the foreign key rather than having something like service_provider_id

[svalexander]

ahh ok interesting. rails magic definitely throws me off at times.

[sheldon-b]

Just dropping some comments for now, I'll come back and finish reviewing when I can

[sheldon-b]

I think this would be misleading for enrollments that have status establishing. Some preference for just returning nil in that case

Suggested change

	def due_date
	start_date = enrollment_established_at.presence || created_at
	start_date + IdentityConfig.store.in_person_enrollment_validity_in_days.days
	end
	def due_date
	nil unless established_at
	established_at + IdentityConfig.store.in_person_enrollment_validity_in_days.days
	end

[svalexander]

we were using created_at here to account for some enrollments that only have created_at defined when the enrollment status changes to pending and the user becomes ready to verify. We want to default to created_at if there's no enrollment_established_at value. (ticket)

[sheldon-b]

An enrollment doesn't have a due date until it's pending though. If we use the created_at timestamp we're just making something up that isn't real. That date has no relevance to a user or USPS. Seems like the only reason to use created_at as a fallback is to return some sort of date if we ever execute that codepath, so that we're showing the user some sort of date. But if we ever execute that codepath with an enrollment that doesn't have enrollment_established_at then wouldn't it be a bug?

It's not a hill I want to die on but I am curious why we defined it that way in the ticket, I'm going to ask the ticket creator

[tomas-nava]

I'm fine with removing the fallback to created_at. Note though, that

1. the variable should be enrollment_established_at in sheldon-b's suggested change, and
2. the days_to_due_date method will have to be updated to return nil if due_date returns nil (otherwise, it'll return (DateTime.now...nil).count which is Inifinity).
3. the days_remaining method on the ReadyToVerifyPresenter will have to be updated to anticipate a nil value from days_to_due_date (otherwise it'll raise a NoMethodError).

I don't think we have to be too rigorous about protecting against this case though, because when we write the job, we shouldn't be generating emails for non-pending enrollments, which means we should never be in the circumstance where we're getting a nil value for due date.

[sheldon-b]

I have a question about what should happen if there is no service provider. We'll see that a lot in lower environments today. And after we finish the pilot I think there will be ways for production users to get into the IPP flow without an associated partner, such as by going through the password reset flow

I left a question on the Figma file about it

[svalexander]

that's a good question. i left it as an empty string and so in examples i looked at w/o one it looks like an unfinished sentence -it definitely is not ideal. would be good to adjust for sure.

[sheldon-b]

Lizzie suggested we discuss it at design critique on Monday so I'm going to plan to attend for that. Do you want to join too?

[sheldon-b]

We discussed during design critique and tentatively agreed to fall back to Login.gov when there's no service provider, similar to how we do in other places. Design is going to look into other places where this might be an issue in IPP and make suggestions for a fallback

Suggested change

	service_provider ? service_provider.friendly_name : ''
	service_provider&.friendly_name || APP_NAME

[tomas-nava]

this is being addressed in #7256

[sheldon-b]

One last minor comment. Looks good! 🎉

[sheldon-b]

Since this is only used in the user_mailer I suggest we move it to config/locales/user_mailer/en.yml and similar for es/fr