-
-
Notifications
You must be signed in to change notification settings - Fork 486
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
Feature/1886 Volunteer receives SMS to notify of follow up generated by admin or supervisor #3738
Conversation
…ave a phone number if sms preference is picked)
@@ -60,6 +60,7 @@ notifications/base_notification.rb | |||
notifications/followup_notification.rb | |||
notifications/followup_resolved_notification.rb | |||
notifications/youth_birthday_notification.rb | |||
notifications/delivery_methods/sms.rb |
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.
ideally let's not add to the skipped tests file
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.
For sure. I'm beginning to figure out how this could be tested, which will hopefully lay the groundwork for testing the other notifications in .allow_skipping_tests
@@ -7,7 +7,8 @@ class FollowupNotification < BaseNotification | |||
# Add your delivery methods | |||
# | |||
deliver_by :database | |||
# deliver_by :email, mailer: "UserMailer" | |||
# deliver_by :email, mailer: "UserMailer", if: :email_notifications? | |||
# deliver_by :sms, class: "DeliveryMethods::Sms", if: :sms_notifications? |
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.
is this supposed to be commented out?
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.
Yes, the email one was commented out when I started this. https://github.com/rubyforgood/casa/pull/1612/files is the email and database pr
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.
Also the "noticed gem" was first added here #1611
What github issue is this PR for, if any?
Resolves #1886
What changed, and why?
Admins and Supervisors can send SMS messages to notify volunteers about case contact follow-ups
delivery_methods/sms.rb
tofollowup_notification.rb
sms_notifications?
andemail_notifications?
before corresponding deliveriesvalid_phone_number_if_receive_sms_notifications
to ensure users add aphone number
that will receive SMSsSmsBodyHelper
to store message bodiesHow will this affect user permissions?
How is this tested? (please write tests!) 💖💪
Previous tests:
Follow-up tests
→ spec/system/users/edit_spec.rbTwilio delivery tests
→ spec/services/twilio_service_spec.rbShort.io tests
→ spec/services/short_url_service_spec.rbNew test:
Ensure a valid phone number is required before users select the receive SMS preference →
edit_spec.rb
Removed tests in:
→ spec/lib/tasks/case_contact_types_reminder_spec.rb
→ spec/lib/tasks/no_contact_made_reminder_spec.rb
They were removed because Volunteers can no longer have the SMS preference without
first
adding a phone numberScreenshots please :)
send-followup-sms.mov