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

Add job to send virtual hearing reminder emails #15581

Merged
merged 35 commits into from Nov 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
2a5ede9
add new columns for when the last reminder emails were sent to the ap…
ferristseng Nov 5, 2020
57896f0
run migrate and update schema
ferristseng Nov 5, 2020
d0a265c
handle reminder email type
ferristseng Nov 5, 2020
35d33dc
add method to get emails that might need a reminder sent
ferristseng Nov 5, 2020
9864721
add job to send reminder emails
ferristseng Nov 5, 2020
8a527bc
add some documentation to clarify what emails are included in the all…
ferristseng Nov 5, 2020
eca4762
add tests for the new virtual hearing repository method
ferristseng Nov 5, 2020
2e4e157
send appellant or representative reminders separately
ferristseng Nov 5, 2020
e1681ab
refactor logic about whether or not to send reminder emails to a sepa…
ferristseng Nov 5, 2020
fd046f5
refactoring for clarity and update the job to avoid sending duplicate…
ferristseng Nov 5, 2020
e8c2e13
add some tests for the send reminder emails job
ferristseng Nov 5, 2020
fd310c2
add a space
ferristseng Nov 5, 2020
61be568
add tests for VirtualHearings::ReminderService
ferristseng Nov 5, 2020
32e86c2
round the days_until_hearing valuee
ferristseng Nov 6, 2020
fef3bff
add more tests
ferristseng Nov 6, 2020
beb7d28
update migration to inherit from Caseflow::Migration
ferristseng Nov 6, 2020
ab8d0f9
run make docs
ferristseng Nov 6, 2020
2994dca
work around complexity warning
ferristseng Nov 6, 2020
bb3c395
use 1.day
ferristseng Nov 6, 2020
28f5df4
add a comment about an assumption
ferristseng Nov 6, 2020
b875978
adding new tests for job
ferristseng Nov 6, 2020
d5ebc0a
reminders are sent by the CASEFLOW user
ferristseng Nov 6, 2020
2bdaa14
renaming schema columns based on feedback
ferristseng Nov 9, 2020
98f7e17
run make docs
ferristseng Nov 9, 2020
be1ec0d
add job to whitelist
ferristseng Nov 12, 2020
bc70cf8
early return if days_until_hearing is <= 0
ferristseng Nov 12, 2020
be2664e
fix typo
ferristseng Nov 12, 2020
ef06351
use Float::INFINITY instead of large number
ferristseng Nov 12, 2020
a2e2242
add small comment about 3-day reminders
ferristseng Nov 12, 2020
55753ab
add more tests around disposition and reminder emails
ferristseng Nov 12, 2020
e73bfc8
remove whitespace
ferristseng Nov 12, 2020
e9b611f
add logic to ensure reminder emails aren't sent if the hearing was co…
ferristseng Nov 16, 2020
8e47544
codeclimate warnings
ferristseng Nov 16, 2020
5663dae
include email type when writing datadog metrics
ferristseng Nov 16, 2020
31ffa4c
Merge branch 'master' into ftseng/CASEFLOW-194-reminder-emails-job
Nov 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/api/v1/jobs_controller.rb
Expand Up @@ -19,6 +19,7 @@ class Api::V1::JobsController < Api::ApplicationController
"prepare_establish_claim" => PrepareEstablishClaimTasksJob,
"push_priority_appeals_to_judges" => PushPriorityAppealsToJudgesJob,
"reassign_old_tasks" => ReassignOldTasksJob,
"send_reminder_emails_job" => VirtualHearings::SendReminderEmailsJob,
"retrieve_documents_for_reader" => RetrieveDocumentsForReaderJob,
"set_appeal_age_aod" => SetAppealAgeAodJob,
"stats_collector" => StatsCollectorJob,
Expand Down
45 changes: 38 additions & 7 deletions app/jobs/virtual_hearings/send_email.rb
Expand Up @@ -7,10 +7,16 @@ class RecipientIsDeceasedVeteran < StandardError; end

def initialize(virtual_hearing:, type:)
@virtual_hearing = virtual_hearing
@type = type
@type = type.to_s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small refactor here so we don't have to call to_s when we're using type

end

def call
# Assumption: Reminders and confirmation/cancellation/change emails are sent
# separately, so this will return early if any reminder emails are sent. If
# reminder emails are being sent, we are assuming the other emails have all
# already been sent too.
return if send_reminder

if !virtual_hearing.appellant_email_sent
virtual_hearing.update!(appellant_email_sent: send_email(appellant_recipient))
end
Expand All @@ -30,19 +36,39 @@ def call
delegate :appeal, to: :hearing
delegate :veteran, to: :appeal

def send_reminder
if type == "appellant_reminder" && send_email(appellant_recipient)
virtual_hearing.update!(appellant_reminder_sent_at: Time.zone.now)

return true
end

if type == "representative_reminder" &&
!virtual_hearing.representative_email.nil? &&
send_email(representative_recipient)
virtual_hearing.update!(representative_reminder_sent_at: Time.zone.now)

return true
end

false
end

def email_for_recipient(recipient)
args = {
mail_recipient: recipient,
virtual_hearing: virtual_hearing
}

case type.to_s
case type
when "confirmation"
VirtualHearingMailer.confirmation(**args)
when "cancellation"
VirtualHearingMailer.cancellation(**args)
when "updated_time_confirmation"
VirtualHearingMailer.updated_time_confirmation(**args)
when "appellant_reminder", "representative_reminder"
VirtualHearingMailer.reminder(**args)
else
fail ArgumentError, "Invalid type of email to send: `#{type}`"
end
Expand All @@ -57,7 +83,10 @@ def external_message_id(msg)
DataDogService.increment_counter(
app_name: Constants.DATADOG_METRICS.HEARINGS.APP_NAME,
metric_group: Constants.DATADOG_METRICS.HEARINGS.VIRTUAL_HEARINGS_GROUP_NAME,
metric_name: "emails.submitted"
metric_name: "emails.submitted",
attrs: {
email_type: type
}
)

Rails.logger.info(
Expand Down Expand Up @@ -101,7 +130,7 @@ def send_email(recipient)

msg = email.deliver_now!
rescue StandardError, Savon::Error, BGS::ShareError => error
# Savon::Error and BGS::ShareError are sometimes thrown when making requests to BGS enpoints
# Savon::Error and BGS::ShareError are sometimes thrown when making requests to BGS endpoints
Raven.capture_exception(error)

Rails.logger.warn("Failed to send #{type} email to #{recipient.title}: #{error}")
Expand All @@ -126,11 +155,11 @@ def create_sent_hearing_email_event(recipient, external_id)
)
SentHearingEmailEvent.create!(
hearing: hearing,
email_type: type,
email_type: type.ends_with?("reminder") ? "reminder" : type,
email_address: recipient.email,
external_message_id: external_id,
recipient_role: recipient_is_veteran ? "veteran" : recipient.title.downcase,
sent_by: virtual_hearing.updated_by
sent_by: type.ends_with?("reminder") ? User.system_user : virtual_hearing.updated_by
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tagging the system user as the person who sent the email, since technically no one is triggering the email to be sent

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense

)
rescue StandardError => error
Raven.capture_exception(error)
Expand Down Expand Up @@ -185,6 +214,8 @@ def appellant_recipient
end

def should_judge_receive_email?
!virtual_hearing.judge_email.nil? && !virtual_hearing.judge_email_sent && type.to_s != "cancellation"
!virtual_hearing.judge_email.nil? &&
!virtual_hearing.judge_email_sent &&
%w[confirmation updated_time_confirmation].include?(type)
end
end
37 changes: 37 additions & 0 deletions app/jobs/virtual_hearings/send_reminder_emails_job.rb
@@ -0,0 +1,37 @@
# frozen_string_literal: true

class VirtualHearings::SendReminderEmailsJob < ApplicationJob
def perform
VirtualHearingRepository.maybe_ready_for_reminder_email.each do |virtual_hearing|
send_reminder_emails(virtual_hearing)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VirtualHearing::SendEmail seems pretty good at handling errors, so I don't know if we need to handle errors here? I could add a very broad begin/rescue

end
end

private

def send_reminder_emails(virtual_hearing)
if should_send_appellant_reminder?(virtual_hearing)
VirtualHearings::SendEmail
.new(virtual_hearing: virtual_hearing, type: :appellant_reminder)
.call
end

if virtual_hearing.representative_email.present? && should_sent_representative_reminder?(virtual_hearing)
VirtualHearings::SendEmail
.new(virtual_hearing: virtual_hearing, type: :representative_reminder)
.call
end
end

def should_send_appellant_reminder?(virtual_hearing)
VirtualHearings::ReminderService
.new(virtual_hearing, virtual_hearing.appellant_reminder_sent_at)
.should_send_reminder_email?
end

def should_sent_representative_reminder?(virtual_hearing)
VirtualHearings::ReminderService
.new(virtual_hearing, virtual_hearing.representative_reminder_sent_at)
.should_send_reminder_email?
end
end
3 changes: 2 additions & 1 deletion app/models/hearings/sent_hearing_email_event.rb
Expand Up @@ -28,7 +28,8 @@ class << self; undef_method :sent_to_appellant; end
{
confirmation: "confirmation",
cancellation: "cancellation",
updated_time_confirmation: "updated_time_confirmation"
updated_time_confirmation: "updated_time_confirmation",
reminder: "reminder"
}
), _prefix: :is

Expand Down
1 change: 1 addition & 0 deletions app/models/hearings/virtual_hearing.rb
Expand Up @@ -86,6 +86,7 @@ def base_url
HearingDay::REQUEST_TYPES[:central]
].freeze

# Whether or not all non-reminder emails were sent.
rubaiyat22 marked this conversation as resolved.
Show resolved Hide resolved
def all_emails_sent?
appellant_email_sent &&
(judge_email.nil? || judge_email_sent) &&
Expand Down
84 changes: 68 additions & 16 deletions app/repositories/virtual_hearing_repository.rb
Expand Up @@ -3,20 +3,16 @@
class VirtualHearingRepository
class << self
def ready_for_deletion
virtual_hearings_for_ama_hearings = VirtualHearing.eligible_for_deletion
.where(hearing_type: Hearing.name)
.joins("INNER JOIN hearings ON hearings.id = virtual_hearings.hearing_id")
.joins("INNER JOIN hearing_days ON hearing_days.id = hearings.hearing_day_id")
virtual_hearings_for_ama_hearings = virtual_hearings_joined_with_hearings_and_hearing_day(Hearing)
.eligible_for_deletion
.where(
"hearing_days.scheduled_for < :today OR
hearings.disposition='postponed' OR hearings.disposition='cancelled' OR
virtual_hearings.request_cancelled = true", today: Time.zone.today
)

virtual_hearings_for_legacy_hearings = VirtualHearing.eligible_for_deletion
.where(hearing_type: LegacyHearing.name)
.joins("INNER JOIN legacy_hearings ON legacy_hearings.id = virtual_hearings.hearing_id")
.joins("INNER JOIN hearing_days ON hearing_days.id = legacy_hearings.hearing_day_id")
virtual_hearings_for_legacy_hearings = virtual_hearings_joined_with_hearings_and_hearing_day(LegacyHearing)
.eligible_for_deletion
.where(
"hearing_days.scheduled_for < :today OR
legacy_hearings.vacols_id in (:postponed_or_cancelled_vacols_ids) OR
Expand All @@ -27,14 +23,36 @@ def ready_for_deletion
virtual_hearings_for_ama_hearings + virtual_hearings_for_legacy_hearings
end

def pending_appellant_or_rep_emails_sql
<<-SQL
NOT virtual_hearings.appellant_email_sent
OR (
virtual_hearings.representative_email IS NOT null
AND NOT virtual_hearings.representative_email_sent
# Get all virtual hearings that *might* need to have a reminder email sent.
#
# @note The logic to determine whether or not a reminder email needs to be sent is
# complex enough that it's not worth putting in an SQL query for maintainability reasons.
# This method will find all active virtual hearings that are occurring within the next 7
# days.
def maybe_ready_for_reminder_email
rubaiyat22 marked this conversation as resolved.
Show resolved Hide resolved
ama_virtual_hearings_ready_for_email = virtual_hearings_joined_with_hearings_and_hearing_day(Hearing)
.not_cancelled
.where(
"hearings.disposition NOT IN (:non_active_hearing_dispositions) OR hearings.disposition IS NULL",
non_active_hearing_dispositions: [:postponed, :cancelled]
)
SQL
.where(where_hearing_occurs_within_the_timeframe)

legacy_virtual_hearings_ready_for_email = virtual_hearings_joined_with_hearings_and_hearing_day(LegacyHearing)
.not_cancelled
.where(where_hearing_occurs_within_the_timeframe)

postponed_or_cancelled_legacy = postponed_or_cancelled_vacols_ids

unless postponed_or_cancelled_legacy.empty?
# Active Hearings
legacy_virtual_hearings_ready_for_email = legacy_virtual_hearings_ready_for_email.where(
"legacy_hearings.vacols_id NOT IN (:postponed_or_cancelled_vacols_ids)",
postponed_or_cancelled_vacols_ids: postponed_or_cancelled_vacols_ids
)
end

ama_virtual_hearings_ready_for_email + legacy_virtual_hearings_ready_for_email
end

def cancelled_hearings_with_pending_emails
Expand All @@ -61,13 +79,47 @@ def hearings_with_pending_conference_or_pending_emails

private

# Returns virtual hearings joined with either the legacy hearing or hearings table,
# and joined with the hearing day table.
def virtual_hearings_joined_with_hearings_and_hearing_day(appeal_type)
table = appeal_type.table_name

VirtualHearing
.where(hearing_type: appeal_type.name)
.joins("INNER JOIN #{table} ON #{table}.id = virtual_hearings.hearing_id")
.joins("INNER JOIN hearing_days ON hearing_days.id = #{table}.hearing_day_id")
end

def postponed_or_cancelled_vacols_ids
VACOLS::CaseHearing.by_dispositions(
[
VACOLS::CaseHearing::HEARING_DISPOSITIONS.key("postponed"),
VACOLS::CaseHearing::HEARING_DISPOSITIONS.key("cancelled")
]
).pluck(:hearing_pkseq).map(&:to_s)
).pluck(:hearing_pkseq).map(&:to_s) || []
end

def pending_appellant_or_rep_emails_sql
rubaiyat22 marked this conversation as resolved.
Show resolved Hide resolved
<<-SQL
NOT virtual_hearings.appellant_email_sent
OR (
virtual_hearings.representative_email IS NOT null
AND NOT virtual_hearings.representative_email_sent
)
SQL
end

# Returns a where clause that can be used to find all hearings that occur within
# a given timeframe (in days).
#
# @note Requires a join with the `hearing_days` table.
def where_hearing_occurs_within_the_timeframe
<<-SQL
DATE_PART(
'day',
hearing_days.scheduled_for::timestamp - '#{Time.zone.today}'::timestamp
) BETWEEN 1 AND 7
SQL
end
end
end
60 changes: 60 additions & 0 deletions app/services/virtual_hearings/reminder_service.rb
@@ -0,0 +1,60 @@
# frozen_string_literal: true

##
# Service that determines whether or not a send a reminder to the appellant or representative
# about their virtual hearing.

class VirtualHearings::ReminderService
def initialize(virtual_hearing, last_sent_reminder)
@virtual_hearing = virtual_hearing
@last_sent_reminder = last_sent_reminder
end

def should_send_reminder_email?
return false if days_until_hearing <= 0

should_send_2_day_reminder? ||
should_send_3_day_friday_reminder? ||
should_send_7_day_reminder?
end

private

attr_reader :virtual_hearing
attr_reader :last_sent_reminder

def should_send_2_day_reminder?
days_between_hearing_and_created_at > 2 &&
days_until_hearing <= 2 && days_from_hearing_day_to_last_sent_reminder > 2
end

# The 3 day reminder is a special reminder that is sent on Friday, *only* if the hearing
# itself is on Monday.
def should_send_3_day_friday_reminder?
ferristseng marked this conversation as resolved.
Show resolved Hide resolved
days_between_hearing_and_created_at > 3 &&
days_until_hearing <= 3 && virtual_hearing.hearing.scheduled_for.monday?
end

def should_send_7_day_reminder?
days_between_hearing_and_created_at > 7 &&
days_until_hearing <= 7 && days_from_hearing_day_to_last_sent_reminder > 7
end

# Determines the date between when the hearing is scheduled, and when the virtual hearing was scheduled.
# If the virtual hearing was scheduled within a reminder period, we skip sending the reminder for that period
# because the confirmation will have redundant information.
def days_between_hearing_and_created_at
(virtual_hearing.hearing.scheduled_for - virtual_hearing.created_at) / 1.day
end

def days_until_hearing
((virtual_hearing.hearing.scheduled_for - Time.zone.now.utc) / 1.day).round
end

def days_from_hearing_day_to_last_sent_reminder
# Pick arbitrarily big value if the reminder has never been sent.
return Float::INFINITY if last_sent_reminder.nil?

(virtual_hearing.hearing.scheduled_for - last_sent_reminder) / 1.day
end
end
@@ -0,0 +1,12 @@
class AddReminderEmailFlagsToVirtualHearings < Caseflow::Migration
def change
add_column :virtual_hearings,
:appellant_reminder_sent_at,
:datetime,
comment: "The datetime the last reminder email was sent to the appellant."
add_column :virtual_hearings,
:representative_reminder_sent_at,
:datetime,
comment: "The datetime the last reminder email was sent to the representative."
end
end
4 changes: 3 additions & 1 deletion db/schema.rb
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2020_10_20_135542) do
ActiveRecord::Schema.define(version: 2020_11_04_175215) do

# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
Expand Down Expand Up @@ -1491,6 +1491,7 @@
t.string "alias_with_host", comment: "Alias for conference in pexip with client_host"
t.string "appellant_email", comment: "Appellant's email address"
t.boolean "appellant_email_sent", default: false, null: false, comment: "Determines whether or not a notification email was sent to the appellant"
t.datetime "appellant_reminder_sent_at", comment: "The datetime the last reminder email was sent to the appellant."
t.string "appellant_tz", limit: 50, comment: "Stores appellant timezone"
t.boolean "conference_deleted", default: false, null: false, comment: "Whether or not the conference was deleted from Pexip"
t.integer "conference_id", comment: "ID of conference from Pexip"
Expand All @@ -1506,6 +1507,7 @@
t.boolean "judge_email_sent", default: false, null: false, comment: "Whether or not a notification email was sent to the judge"
t.string "representative_email", comment: "Veteran's representative's email address"
t.boolean "representative_email_sent", default: false, null: false, comment: "Whether or not a notification email was sent to the veteran's representative"
t.datetime "representative_reminder_sent_at", comment: "The datetime the last reminder email was sent to the representative."
t.string "representative_tz", limit: 50, comment: "Stores representative timezone"
t.boolean "request_cancelled", default: false, comment: "Determines whether the user has cancelled the virtual hearing request"
t.datetime "updated_at", null: false, comment: "Automatic timestamp of when virtual hearing was updated"
Expand Down