-
Notifications
You must be signed in to change notification settings - Fork 167
LG-9564: Notify users when verify by mail codes expire 📣 #9475
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
Changes from 13 commits
cf122a2
876cbd6
eb85115
478d993
4270527
60f2dfd
0c5a449
1c80225
44a6488
fb84072
a8dcfa4
ac51ba2
9d6c19a
6848b16
b992712
13f5bdd
8f1a68b
8fa8a7d
a60f2b2
99af297
53c773b
995f9a6
5d89724
6c61149
54eb141
9fb8790
e556d6b
0e58319
a6a3cb0
e8c25c2
f58f177
072f4f4
ab6c876
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| class SendGpoCodeExpirationNoticesJob < ApplicationJob | ||
| queue_as :low | ||
|
|
||
| def initialize(analytics: nil) | ||
| @analytics = analytics | ||
| end | ||
|
|
||
| def perform | ||
| codes_to_send_notifications_for.find_each do |code| | ||
| user = code.profile.user | ||
|
|
||
| user.send_email_to_all_addresses(:gpo_code_expired, code_sent_at: code.code_sent_at) | ||
| code.update(expiration_notice_sent_at: Time.zone.now) | ||
|
|
||
| analytics.idv_gpo_expiration_email_sent(user_id: user.uuid) | ||
| end | ||
| end | ||
|
|
||
| def codes_to_send_notifications_for | ||
| # We are looking at a 48 hr window in which all codes will _definitely_ be expired. | ||
| from, to = calculate_notification_window_bounds | ||
| expired_codes_needing_notification_sent_between( | ||
| from: from, | ||
| to: to, | ||
| ) | ||
|
matthinz marked this conversation as resolved.
Outdated
|
||
| end | ||
|
|
||
| def calculate_notification_window_bounds(as_of: Time.zone.now) | ||
| to = as_of.beginning_of_day - IdentityConfig.store.usps_confirmation_max_days.days | ||
| from = to - 2.days | ||
| [from, to] | ||
|
matthinz marked this conversation as resolved.
Outdated
|
||
| end | ||
|
|
||
| private | ||
|
|
||
| def analytics | ||
| @analytics ||= Analytics.new(user: AnonymousUser.new, request: nil, session: {}, sp: nil) | ||
| end | ||
|
|
||
| def expired_codes_needing_notification_sent_between( | ||
| from:, | ||
| to: | ||
| ) | ||
| GpoConfirmationCode.joins(:profile). | ||
| # 1. Exclude codes that we've already sent an expiration notice for | ||
| where(expiration_notice_sent_at: nil). | ||
|
|
||
| # 2. Exclude codes not sent in the window we're looking at | ||
|
matthinz marked this conversation as resolved.
Outdated
|
||
| where(code_sent_at: from...to). | ||
|
matthinz marked this conversation as resolved.
Outdated
|
||
|
|
||
| # 3. Exclude codes where the associated profile does not have the GPO pending timestamp set | ||
| # (meaning they either completed GPO or reset their password). | ||
| where.not(profile: { gpo_verification_pending_at: nil }). | ||
|
|
||
| # 4. Exclude codes where the associated profile has been deactivated for some reason | ||
| where(profile: { deactivation_reason: nil }). | ||
|
|
||
| # 5. Exclude codes where the user has since gotten an active profile (no point in notifying) | ||
| where.not( | ||
| profile: { | ||
| user_id: User.joins(:profiles).where( | ||
| profiles: { | ||
| active: true, | ||
| }, | ||
| ), | ||
| }, | ||
| ). | ||
|
|
||
| # 6. Only include codes that are the most recent code sent for the profile | ||
| where( | ||
| code_sent_at: GpoConfirmationCode. | ||
| select('max(code_sent_at)'). | ||
| from('usps_confirmation_codes child'). | ||
| where('child.profile_id = usps_confirmation_codes.profile_id'), | ||
| ) | ||
|
matthinz marked this conversation as resolved.
Outdated
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -460,12 +460,12 @@ def analytics | |
| @analytics ||= Analytics.new(user: self, request: nil, session: {}, sp: nil) | ||
| end | ||
|
|
||
| def send_email_to_all_addresses(user_mailer_template) | ||
| def send_email_to_all_addresses(user_mailer_template, **args) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor change here to support forwarding args to |
||
| confirmed_email_addresses.each do |email_address| | ||
| UserMailer.with( | ||
| user: self, | ||
| email_address: email_address, | ||
| ).send(user_mailer_template). | ||
| ).send(user_mailer_template, **args). | ||
| deliver_now_or_later | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
|
|
||
| <%= t( | ||
| 'user_mailer.gpo_code_expired.body.intro', | ||
| code_sent_at: @code_sent_at, | ||
| app_name: APP_NAME, | ||
| ) %> | ||
|
|
||
| <%= link_to( | ||
| t('user_mailer.gpo_code_expired.body.learn_more_link'), | ||
| help_center_redirect_url( | ||
| category: 'verify-your-identity', | ||
| article: 'verify-your-address-by-mail', | ||
| flow: :idv, | ||
| step: :gpo_send_letter, | ||
| ), | ||
| { style: "text-decoration: 'underline'" }, | ||
| ) %> | ||
|
|
||
| <table class="button expanded large radius"> | ||
| <tbody> | ||
| <tr> | ||
| <td> | ||
| <table style="margin-bottom: 1em; margin-top: 1em"> | ||
| <tbody> | ||
| <tr> | ||
| <td style="text-align: center"> | ||
| <%= link_to t('user_mailer.gpo_code_expired.body.cta'), | ||
| idv_url, | ||
| target: '_blank', | ||
| class: 'float-center', | ||
| align: 'center', | ||
| rel: 'noopener' %> | ||
| </td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| </td> | ||
| <td class="expander"></td> | ||
| </tr> | ||
| </tbody> | ||
| </table> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| class AddExpirationNoticeSentAtToUspsConfirmationCodes < ActiveRecord::Migration[7.0] | ||
| disable_ddl_transaction! | ||
|
|
||
| def change | ||
| add_column :usps_confirmation_codes, :expiration_notice_sent_at, :datetime, precision: nil | ||
| add_index :usps_confirmation_codes, :expiration_notice_sent_at, algorithm: :concurrently | ||
|
Comment on lines
+5
to
+6
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Migration to add an indexed column to |
||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I commented on the ticket, we are assuming here that the config value
max_mail_events_window_in_days, currently set to 30, will always be less than the code expiration, currently set to 31. I still think that we should check that assumption somewhere so we're not accidentally sending users emails inviting them to restart IdV if they're rate limited in the future when someone changes the config value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an additional condition,
user_is_not_rate_limited_for_gpoin 072f4f4 (and fixed a bug in it in ab6c876). The idea is that we just don't email folks who are currently rate limited for GPO.