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

Conversation

ferristseng
Copy link
Contributor

@ferristseng ferristseng commented Nov 5, 2020

Resolves #15400
Resolves https://vajira.max.gov/browse/CASEFLOW-194

Description

  • Add VirtualHearings::SendReminderEmailsJob to send reminder emails on a schedule (TBD)
  • Update VirtualHearingRepository with new method to find virtual hearings that might need reminder emails
  • Add VirtualHearings::ReminderService to determine when reminder emails need to be sent
  • Update VirtualHearings::SendEmail to send reminder emails
  • Add new DB columns: appellant_reminder_sent and representative_reminder_sent
  • Add a bunch of new tests for job and supporting classes

Acceptance Criteria

  • New template Completed in Create templates for virtual hearing reminder emails #15557
  • New job that looks for upcoming hearings, sends emails, and updates DB to say it was sent
    • DB update requires two new flags
  • Recording metrics on sends in Datadog, also alerting with failed sends as we do now for confirmation Should be handled already by logic in VirtualHearings::SendEmail

Database Changes

Only for Schema Changes

  • Timestamps (created_at, updated_at) for new tables
  • Column comments updated
  • Have your migration classes inherit from Caseflow::Migration, especially when adding indexes (use add_safe_index)
  • Verify that migrate:rollback works as desired (change supported functions)
  • Query profiling performed (eyeball Rails log, check bullet and fasterer output)
  • Appropriate indexes added (especially for foreign keys, polymorphic columns, unique constraints, and Rails scopes)
  • DB schema docs updated with make docs (after running make migrate)
  • #appeals-schema notified with summary and link to this PR
  • Any non-obvious semantics or logic useful for interpreting database data is documented at Caseflow Data Model and Dictionary

@va-bot
Copy link
Collaborator

va-bot commented Nov 5, 2020

2 Warnings
⚠️ This is a Big PR. Try to break this down if possible. Stacked pull requests encourage more detailed and thorough code reviews
⚠️ This PR changes the schema. Please use the PR template checklist.

Generated by 🚫 Danger

@codeclimate
Copy link

codeclimate bot commented Nov 5, 2020

Code Climate has analyzed commit 31ffa4c and detected 0 issues on this pull request.

View more on Code Climate.

@ferristseng ferristseng requested review from a team and rubaiyat22 and removed request for a team November 6, 2020 18:42
@@ -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

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

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

app/models/hearings/virtual_hearing.rb Show resolved Hide resolved
Copy link
Contributor

@rubaiyat22 rubaiyat22 left a comment

Choose a reason for hiding this comment

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

approved with some comments and suggestions!

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

Choose a reason for hiding this comment

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

That makes sense


def days_from_hearing_day_to_last_sent_reminder
# Pick arbitrarily big value if the reminder has never been sent.
return 9999 if last_sent_reminder.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about using Float::INFINITY here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would work! Let me try that and see if the tests still pass

app/services/virtual_hearings/reminder_service.rb Outdated Show resolved Hide resolved
spec/repositories/virtual_hearing_repository_spec.rb Outdated Show resolved Hide resolved
spec/services/virtual_hearings/reminder_service_spec.rb Outdated Show resolved Hide resolved
@ferristseng ferristseng added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Nov 16, 2020
@va-bot va-bot merged commit c719db3 into master Nov 16, 2020
@va-bot va-bot deleted the ftseng/CASEFLOW-194-reminder-emails-job branch November 16, 2020 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master Team: Tango 💃
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Virtual hearing reminder emails
4 participants