-
Notifications
You must be signed in to change notification settings - Fork 66
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
[Document Upload Failure] Email veteran on Form 0781/Form 0781a upload retry exhaustion #18206
Changes from 3 commits
01af4c7
6828019
0cf0817
f45cd70
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,104 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'va_notify/service' | ||
|
||
module EVSS | ||
module DisabilityCompensationForm | ||
class Form0781DocumentUploadFailureEmail < Job | ||
STATSD_METRIC_PREFIX = 'api.form_526.veteran_notifications.form0781_upload_failure_email' | ||
|
||
# retry for one day | ||
sidekiq_options retry: 14 | ||
|
||
sidekiq_retries_exhausted do |msg, _ex| | ||
job_id = msg['jid'] | ||
error_class = msg['error_class'] | ||
error_message = msg['error_message'] | ||
timestamp = Time.now.utc | ||
form526_submission_id = msg['args'].first | ||
|
||
# Job status records are upserted in the JobTracker module | ||
# when the retryable_error_handler is called | ||
form_job_status = Form526JobStatus.find_by(job_id:) | ||
bgjob_errors = form_job_status.bgjob_errors || {} | ||
new_error = { | ||
"#{timestamp.to_i}": { | ||
caller_method: __method__.to_s, | ||
timestamp:, | ||
form526_submission_id: | ||
} | ||
} | ||
form_job_status.update( | ||
status: Form526JobStatus::STATUS[:exhausted], | ||
bgjob_errors: bgjob_errors.merge(new_error) | ||
) | ||
|
||
Rails.logger.warn( | ||
'Form0781DocumentUploadFailureEmail retries exhausted', | ||
{ | ||
job_id:, | ||
timestamp:, | ||
form526_submission_id:, | ||
error_class:, | ||
error_message: | ||
} | ||
) | ||
|
||
StatsD.increment("#{STATSD_METRIC_PREFIX}.exhausted") | ||
rescue => e | ||
Rails.logger.error( | ||
'Failure in Form0781DocumentUploadFailureEmail#sidekiq_retries_exhausted', | ||
{ | ||
job_id:, | ||
messaged_content: e.message, | ||
submission_id: form526_submission_id, | ||
pre_exhaustion_failure: { | ||
error_class:, | ||
error_message: | ||
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. are 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. Yes, on lines 15 and 16: the rescue is within the block so should have access to those local variables, unless did you catch me missing a Ruby quirk here you're hinting at? 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. ah i see. |
||
} | ||
} | ||
) | ||
raise e | ||
end | ||
|
||
def perform(form526_submission_id) | ||
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. Has approx 10 statements - TooManyStatements |
||
form526_submission = Form526Submission.find(form526_submission_id) | ||
|
||
with_tracking('Form0781DocumentUploadFailureEmail', form526_submission.saved_claim_id, form526_submission_id) do | ||
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. Refers to 'form526_submission' more than self (maybe move it to another class?) - FeatureEnvy |
||
notify_client = VaNotify::Service.new(Settings.vanotify.services.benefits_disability.api_key) | ||
|
||
email_address = form526_submission.veteran_email_address | ||
first_name = form526_submission.get_first_name | ||
date_submitted = form526_submission.format_creation_time_for_mailers | ||
|
||
notify_client.send_email( | ||
email_address:, | ||
template_id: mailer_template_id, | ||
personalisation: { | ||
first_name:, | ||
date_submitted: | ||
} | ||
) | ||
|
||
StatsD.increment("#{STATSD_METRIC_PREFIX}.success") | ||
end | ||
rescue => e | ||
retryable_error_handler(e) | ||
end | ||
|
||
private | ||
|
||
def retryable_error_handler(error) | ||
# Needed to log the error properly in the Sidekiq::Form526JobStatusTracker::JobTracker, | ||
# which is included near the top of this job's inheritance tree in EVSS::DisabilityCompensationForm::JobStatus | ||
super(error) | ||
raise error | ||
end | ||
|
||
def mailer_template_id | ||
Settings.vanotify.services | ||
.benefits_disability.template_id.form0781_upload_failure_notification_template_id | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'rails_helper' | ||
|
||
RSpec.describe EVSS::DisabilityCompensationForm::Form0781DocumentUploadFailureEmail, type: :job do | ||
subject { described_class } | ||
|
||
freeheeling marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let!(:form526_submission) { create(:form526_submission) } | ||
let(:notification_client) { double('Notifications::Client') } | ||
let(:formatted_submit_date) do | ||
# We display dates in mailers in the format "May 1, 2024 3:01 p.m. EDT" | ||
form526_submission.created_at.strftime('%B %-d, %Y %-l:%M %P %Z').sub(/([ap])m/, '\1.m.') | ||
end | ||
|
||
before do | ||
Sidekiq::Job.clear_all | ||
allow(Notifications::Client).to receive(:new).and_return(notification_client) | ||
end | ||
|
||
describe '#perform' do | ||
it 'dispatches a failure notification email' do | ||
expect(notification_client).to receive(:send_email).with( | ||
# Email address and first_name are from our User fixtures | ||
# form0781_upload_failure_notification_template_id is a placeholder in settings.yml | ||
{ | ||
email_address: '[email protected]', | ||
template_id: 'form0781_upload_failure_notification_template_id', | ||
personalisation: { | ||
first_name: 'BEYONCE', | ||
date_submitted: formatted_submit_date | ||
} | ||
} | ||
) | ||
|
||
subject.perform_async(form526_submission.id) | ||
subject.drain | ||
end | ||
end | ||
|
||
describe 'logging' do | ||
it 'increments a Statsd metric' do | ||
allow(notification_client).to receive(:send_email) | ||
|
||
expect do | ||
subject.perform_async(form526_submission.id) | ||
subject.drain | ||
end.to trigger_statsd_increment( | ||
'api.form_526.veteran_notifications.form0781_upload_failure_email.success' | ||
) | ||
end | ||
|
||
it 'creates a Form526JobStatus' do | ||
allow(notification_client).to receive(:send_email) | ||
|
||
expect do | ||
subject.perform_async(form526_submission.id) | ||
subject.drain | ||
end.to change(Form526JobStatus, :count).by(1) | ||
end | ||
|
||
context 'when an error throws when sending an email' do | ||
before do | ||
allow_any_instance_of(VaNotify::Service).to receive(:send_email).and_raise(Common::Client::Errors::ClientError) | ||
end | ||
|
||
it 'passes the error to the included JobTracker retryable_error_handler and re-raises the error' do | ||
# Sidekiq::Form526JobStatusTracker::JobTracker is included in this job's inheritance hierarchy | ||
expect_any_instance_of( | ||
Sidekiq::Form526JobStatusTracker::JobTracker | ||
).to receive(:retryable_error_handler).with(an_instance_of(Common::Client::Errors::ClientError)) | ||
|
||
expect do | ||
subject.perform_async(form526_submission.id) | ||
subject.drain | ||
end.to raise_error(Common::Client::Errors::ClientError) | ||
end | ||
end | ||
end | ||
|
||
context 'when retries are exhausted' do | ||
let!(:form526_job_status) { create(:form526_job_status, :retryable_error, form526_submission:, job_id: 123) } | ||
let(:retry_params) do | ||
{ | ||
'jid' => 123, | ||
'error_class' => 'JennyNotFound', | ||
'error_message' => 'I tried to call you before but I lost my nerve', | ||
'args' => [form526_submission.id] | ||
} | ||
end | ||
|
||
let(:exhaustion_time) { DateTime.new(1985, 10, 26).utc } | ||
|
||
before do | ||
allow(notification_client).to receive(:send_email) | ||
end | ||
|
||
it 'increments a StatsD exhaustion metric, logs to the Rails logger and updates the job status' do | ||
Timecop.freeze(exhaustion_time) do | ||
described_class.within_sidekiq_retries_exhausted_block(retry_params) do | ||
expect(Rails.logger).to receive(:warn).with( | ||
'Form0781DocumentUploadFailureEmail retries exhausted', | ||
{ | ||
job_id: 123, | ||
error_class: 'JennyNotFound', | ||
error_message: 'I tried to call you before but I lost my nerve', | ||
timestamp: exhaustion_time, | ||
form526_submission_id: form526_submission.id | ||
} | ||
).and_call_original | ||
expect(StatsD).to receive(:increment).with( | ||
'api.form_526.veteran_notifications.form0781_upload_failure_email.exhausted' | ||
) | ||
end | ||
|
||
expect(form526_job_status.reload.status).to eq(Form526JobStatus::STATUS[:exhausted]) | ||
end | ||
end | ||
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.
Assumes too much for instance variable '@notify_client' - InstanceVariableAssumption