Email selected users when reimbursement requests are entered #5208
Conversation
sarvaiyanidhi
requested review from
compwron,
FireLemons and
littleforest
as code owners
github-actions
bot
added
ruby
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -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? | |||
| SupervisorMailer.reimbursement_request_email(current_user, current_user.supervisor).deliver | |||
There was a problem hiding this comment.
| SupervisorMailer.reimbursement_request_email(current_user, current_user.supervisor).deliver | |
| SupervisorMailer.reimbursement_request_email(current_user, current_user.supervisor).deliver_later |
There was a problem hiding this comment.
Could you also add/modify spec/requests/case_contacts_spec.rb and add an expectation that SupervisorMailer.reimbursement_request_email is called when those conditions are met, and another expection that it is not called when the conditions are not met?
There was a problem hiding this comment.
Thank you for the feedback. Have done the required changes and pushed the updated code.
| @@ -3,6 +3,7 @@ | |||
|
|
|||
| RSpec.describe SupervisorMailerPreview do | |||
| let!(:supervisor) { create(:supervisor) } | |||
| let!(:volunteer) { create(:volunteer) } | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you @schoork for the feedback. I have addressed the reported point and pushed the fix. Docker check is failing due to other issue, can you trigger it again and review the changes again to merge it.
| if supervisor.receive_reimbursement_email | ||
| mail(to: @supervisor.email, subject: "New reimbursement request from #{@volunteer.display_name}") | ||
| end |
There was a problem hiding this comment.
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
Moved logic to model, fixed tests
|
|
||
| describe ".reimbursement_request_email" do | ||
| let(:supervisor) { create(:supervisor, receive_reimbursement_email: true) } | ||
| let(:volunteer) { create(:volunteer) } |
There was a problem hiding this comment.
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.
| @@ -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? | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
Resolves #4476
What changed, and why?
This PR implements feature of sending email on creation of new reimbursement request to Supervisor if reimbursement notifications enabled
How will this affect user permissions?
How is this tested? (please write tests!) 💖💪
Have added required tests for email feature.
Screenshots please :)