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

Email selected users when reimbursement requests are entered #5208

Merged
merged 8 commits into from
Oct 24, 2023
10 changes: 10 additions & 0 deletions app/controllers/case_contacts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ def update
if @case_contact.valid?
created_at = @case_contact.created_at.strftime("%-I:%-M %p on %m-%e-%Y")
flash[:notice] = "Case contact created at #{created_at}, was successfully updated."
send_reimbursement_email(@case_contact)
redirect_to casa_case_path(@case_contact.casa_case)
else
render :edit
Expand Down Expand Up @@ -169,6 +170,9 @@ def create_case_contact_for_every_selected_casa_case(selected_cases)
end

case_contact = @case_contact.dup

send_reimbursement_email(case_contact)

case_contact.casa_case = casa_case
if @selected_cases.count == 1 && case_contact.valid?
if current_role == "Volunteer"
Expand All @@ -181,6 +185,12 @@ def create_case_contact_for_every_selected_casa_case(selected_cases)
end
end

def send_reimbursement_email(case_contact)
if case_contact.want_driving_reimbursement_changed? && case_contact.want_driving_reimbursement? && !current_user.supervisor.blank?
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you switched this !current_user.supervisor.blank? to reference the case contact's supervisor, you could move the whole if statement into a method on case contact, unit test it, and ensure there are no problems in the future if we allow admins/someone else to submit these on behalf of volunteers. In that case, the current user's supervisor and case contact's supervisor might not be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the great feedback and changes done in this PR. I have gone through the commit and understood the fixes done in the code.

SupervisorMailer.reimbursement_request_email(current_user, current_user.supervisor).deliver_later
end
end

def update_volunteer_address(volunteer = current_user)
content = create_case_contact_params.dig(:casa_case_attributes, :volunteers_attributes, "0", :address_attributes, :content)
return if content.blank?
Expand Down
9 changes: 9 additions & 0 deletions app/mailers/supervisor_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,13 @@ def weekly_digest(supervisor)
subject: "Weekly summary of volunteers' activities for the week of #{Date.today - 7.days}"
)
end

def reimbursement_request_email(volunteer, supervisor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be better to pass the case contact here. You still get volunteer and supervisor, but further down the road we could get more info into the email.

Also see comment for send_reimbursement_email method.

@volunteer = volunteer
@casa_organization = volunteer.casa_org
@supervisor = supervisor
if supervisor.receive_reimbursement_email
mail(to: @supervisor.email, subject: "New reimbursement request from #{@volunteer.display_name}")
end
Comment on lines +27 to +29
Copy link

@javiguerra777 javiguerra777 Oct 23, 2023

Choose a reason for hiding this comment

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

Good use of an if conditional to make sure that receive_reimbursement_email is part of the supervisor parameter before sending mail to the supervisor email

end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<meta itemprop="name" content="Volunteer Reimbursement Request Reminder" style="font-family: Helvetica,Arial,sans-serif; box-sizing: border-box; font-size: 14px; margin: 0;">
<style>/* Email styles need to be inline */</style>
<table width="100%" cellpadding="0" cellspacing="0" style="font-family: Helvetica,Arial,sans-serif; box-sizing: border-box; font-size: 14px; margin: 0;">
<tr style="font-family: Helvetica,Arial,sans-serif; box-sizing: border-box; font-size: 14px; margin: 0;">
<td class="content-block" style="font-family: Helvetica,Arial,sans-serif; box-sizing: border-box; font-size: 14px; vertical-align: top; margin: 0; padding: 0 0 20px;" valign="top">
Hello <%= @supervisor.display_name %>,
</td>
</tr>
<tr style="font-family: Helvetica,Arial,sans-serif; box-sizing: border-box; font-size: 14px; margin: 0;">
<td class="content-block" style="font-family: Helvetica,Arial,sans-serif; box-sizing: border-box; font-size: 14px; vertical-align: top; margin: 0; padding: 0 0 20px;" valign="top">
<%= @volunteer.display_name %> has submitted a reimbursement request, please follow up on the reimbursements page
<%= link_to "using this link", reimbursements_url %>.
</td>
</tr>
</table>
7 changes: 7 additions & 0 deletions lib/mailers/previews/supervisor_mailer_preview.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ def weekly_digest
SupervisorMailer.weekly_digest(supervisor)
end
end

def reimbursement_request_email
volunteer = params.has_key?(:volunteer_id) ? Volunteer.find_by(id: params[:volunteer_id]) : Volunteer.last
supervisor = params.has_key?(:supervisor_id) ? Supervisor.find_by(id: params[:supervisor_id]) : Supervisor.last
supervisor.receive_reimbursement_email = true
SupervisorMailer.reimbursement_request_email(volunteer, supervisor)
end
end

# :nocov:
17 changes: 17 additions & 0 deletions spec/mailers/previews/supervisor_mailer_preview_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

RSpec.describe SupervisorMailerPreview do
let!(:supervisor) { create(:supervisor) }
let!(:volunteer) { create(:volunteer, casa_org: supervisor.casa_org, supervisor: supervisor) }

describe "#account_setup" do
context "When no ID is passed" do
Expand Down Expand Up @@ -49,4 +50,20 @@
xit { expect(email.to).to eq ["[email protected]"] }
end
end

describe "#reimbursement_request_email" do
context "When no ID is passed" do
let(:preview) { described_class.new }
let(:email) { preview.reimbursement_request_email }

it { expect(email.to).to eq [supervisor.email] }
end

context "When passed ID is valid" do
let(:preview) { described_class.new(volunteer_id: volunteer.id, supervisor_id: supervisor.id) }
let(:email) { preview.reimbursement_request_email }

it { expect(email.to).to eq [supervisor.email] }
end
end
end
14 changes: 14 additions & 0 deletions spec/mailers/supervisor_mailer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,18 @@
expect(email_body).to include("This invitation will expire on #{expiration_date} (two weeks).")
end
end

describe ".reimbursement_request_email" do
let(:supervisor) { create(:supervisor, receive_reimbursement_email: true) }
let(:volunteer) { create(:volunteer) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make it explicit that the volunteer's supervisor is the supervisor created above? Might get flaky in the future if we don't.

let(:casa_organization) { volunteer.casa_org }

let(:mail) { SupervisorMailer.reimbursement_request_email(volunteer, supervisor) }

it "sends email reminder" do
expect(mail.subject).to eq("New reimbursement request from #{volunteer.display_name}")
expect(mail.to).to eq([supervisor.email])
expect(mail.body.encoded).to match("#{volunteer.display_name} has submitted a reimbursement request")
schoork marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
26 changes: 26 additions & 0 deletions spec/requests/case_contacts_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,32 @@
end
end

context "reimbursement mail to supervisor" do
let(:case_contact) { create(:case_contact, :wants_reimbursement, casa_case: casa_case, creator: admin) }
let(:supervisor) { create(:supervisor, receive_email_notifications: true) }

it "sends reimbursement request email when conditions are met" do
allow(case_contact).to receive(:want_driving_reimbursement?).and_return(true)
allow(admin).to receive(:supervisor).and_return(supervisor)

mailer_double = double("SupervisorMailer")
allow(SupervisorMailer).to receive(:reimbursement_request_email).and_return(mailer_double)

expect(mailer_double).to receive(:deliver_later)
request
end

it "does not send reimbursement request email when conditions are not met" do
allow(case_contact).to receive(:want_driving_reimbursement?).and_return(false)

mailer_double = double("SupervisorMailer")
allow(SupervisorMailer).to receive(:reimbursement_request_email).and_return(mailer_double)

expect(mailer_double).not_to receive(:deliver_later)
request
end
end

context "with additional expense" do
let(:params) do
{
Expand Down