diff --git a/app/coffeescripts/notifications/NotificationPreferences.js b/app/coffeescripts/notifications/NotificationPreferences.js index 742055e0f78..2f2ddb761b2 100644 --- a/app/coffeescripts/notifications/NotificationPreferences.js +++ b/app/coffeescripts/notifications/NotificationPreferences.js @@ -104,7 +104,6 @@ export default class NotificationPreferences { getTooltipText = (category, channel) => { if ( channel.type === 'sms' && - ENV?.NOTIFICATION_PREFERENCES_OPTIONS?.deprecate_sms_enabled && !ENV?.NOTIFICATION_PREFERENCES_OPTIONS?.allowed_sms_categories.includes(category.category) ) { return 'This notification type is not supported in SMS' diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index 9a797af7a68..8e4637ee209 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -2131,7 +2131,6 @@ class CoursesController < ApplicationController js_env( course_name: @context.name, NOTIFICATION_PREFERENCES_OPTIONS: { - deprecate_sms_enabled: !@domain_root_account.settings[:sms_allowed] && Account.site_admin.feature_enabled?(:deprecate_sms), reduce_push_enabled: Account.site_admin.feature_enabled?(:reduce_push_notifications), allowed_sms_categories: Notification.categories_to_send_in_sms(@domain_root_account), allowed_push_categories: Notification.categories_to_send_in_push, diff --git a/app/controllers/profile_controller.rb b/app/controllers/profile_controller.rb index dfd0410bd0e..3d13671583c 100644 --- a/app/controllers/profile_controller.rb +++ b/app/controllers/profile_controller.rb @@ -235,7 +235,6 @@ class ProfileController < ApplicationController add_crumb(@current_user.short_name, profile_path) add_crumb(t("Account Notification Settings")) js_env NOTIFICATION_PREFERENCES_OPTIONS: { - deprecate_sms_enabled: !@domain_root_account.settings[:sms_allowed] && Account.site_admin.feature_enabled?(:deprecate_sms), reduce_push_enabled: Account.site_admin.feature_enabled?(:reduce_push_notifications), allowed_sms_categories: Notification.categories_to_send_in_sms(@domain_root_account), allowed_push_categories: Notification.categories_to_send_in_push, @@ -264,7 +263,6 @@ class ProfileController < ApplicationController :channels => @user.communication_channels.all_ordered_for_display(@user).map { |c| communication_channel_json(c, @user, session) }, :policies => NotificationPolicy.setup_with_default_policies(@user, full_category_list).map { |p| notification_policy_json(p, @user, session).tap { |json| json[:communication_channel_id] = p.communication_channel_id } }, :categories => categories, - :deprecate_sms_enabled => !@domain_root_account.settings[:sms_allowed] && Account.site_admin.feature_enabled?(:deprecate_sms), :allowed_sms_categories => Notification.categories_to_send_in_sms(@domain_root_account), :update_url => communication_update_profile_path, :show_observed_names => @user.observer_enrollments.any? || @user.as_observer_observation_links.any? ? @user.send_observed_names_in_notifications? : nil diff --git a/app/jsx/notification_preferences/NotificationPreferencesTable.js b/app/jsx/notification_preferences/NotificationPreferencesTable.js index 592f24a32b4..24ad08ad2cb 100644 --- a/app/jsx/notification_preferences/NotificationPreferencesTable.js +++ b/app/jsx/notification_preferences/NotificationPreferencesTable.js @@ -90,10 +90,7 @@ const formatCategoryKey = category => { } const smsNotificationCategoryDeprecated = category => { - return ( - ENV?.NOTIFICATION_PREFERENCES_OPTIONS?.deprecate_sms_enabled && - !ENV?.NOTIFICATION_PREFERENCES_OPTIONS?.allowed_sms_categories.includes(category) - ) + return !ENV?.NOTIFICATION_PREFERENCES_OPTIONS?.allowed_sms_categories.includes(category) } const pushNotificationCategoryRestricted = category => { diff --git a/app/jsx/notification_preferences/__tests__/NotificationPreferencesTable.test.js b/app/jsx/notification_preferences/__tests__/NotificationPreferencesTable.test.js index 58a57fb3682..abf3c75e2b1 100644 --- a/app/jsx/notification_preferences/__tests__/NotificationPreferencesTable.test.js +++ b/app/jsx/notification_preferences/__tests__/NotificationPreferencesTable.test.js @@ -32,7 +32,6 @@ describe('Notification Preferences Table', () => { send_scores_in_emails_text: { label: 'Some Label Text' }, - deprecate_sms_enabled: true, allowed_sms_categories: ['announcement', 'grading'], reduce_push_enabled: true, allowed_push_categories: ['announcement'] @@ -44,7 +43,6 @@ describe('Notification Preferences Table', () => { window.ENV = { NOTIFICATION_PREFERENCES_OPTIONS: { send_scores_in_emails_text: null, - deprecate_sms_enabled: true, allowed_sms_categories: ['announcement', 'grading'] } } diff --git a/app/models/message.rb b/app/models/message.rb index beb7fb3ee43..aeb0929719d 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -664,7 +664,7 @@ class Message < ActiveRecord::Base end check_acct = root_account || user&.account || Account.site_admin - if path_type == 'sms' && !check_acct.settings[:sms_allowed] && Account.site_admin.feature_enabled?(:deprecate_sms) + if path_type == 'sms' if Notification.types_to_send_in_sms(check_acct).exclude?(notification_name) InstStatsd::Statsd.increment("message.skip.#{path_type}.#{notification_name}", short_stat: 'message.skip', diff --git a/config/feature_flags/vice_release_flags.yml b/config/feature_flags/vice_release_flags.yml index a14f766b317..bd544a72b02 100644 --- a/config/feature_flags/vice_release_flags.yml +++ b/config/feature_flags/vice_release_flags.yml @@ -15,14 +15,6 @@ confetti_for_valid_links: a course and no issues are found. applies_to: RootAccount -deprecate_sms: - state: hidden - display_name: Restrict SMS to Select Types - description: |- - Disables SMS notifications for all types except announcements and - grade changes. - applies_to: SiteAdmin - notification_update_account_ui: state: hidden display_name: Account level notification preferences UI update diff --git a/spec/coffeescripts/notifications/NotificationPreferencesSpec.js b/spec/coffeescripts/notifications/NotificationPreferencesSpec.js index 04393d94424..078e5b0ca43 100644 --- a/spec/coffeescripts/notifications/NotificationPreferencesSpec.js +++ b/spec/coffeescripts/notifications/NotificationPreferencesSpec.js @@ -68,7 +68,6 @@ test('policyCellProps with twitter', () => { test('policyCellProps with sms_deprecation', () => { fakeENV.setup() ENV.NOTIFICATION_PREFERENCES_OPTIONS = { - deprecate_sms_enabled: true, allowed_sms_categories: ['announcement', 'grading'] } diff --git a/spec/lib/services/notification_service_spec.rb b/spec/lib/services/notification_service_spec.rb index b9de70852f2..4a31a14b5b2 100644 --- a/spec/lib/services/notification_service_spec.rb +++ b/spec/lib/services/notification_service_spec.rb @@ -56,6 +56,7 @@ module Services end it "processes twilio message type" do + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(@queue).to receive(:send_message).once @message.path_type = "sms" expect{@message.deliver}.not_to raise_error @@ -73,6 +74,7 @@ module Services end it "processes sms message type" do + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(@queue).to receive(:send_message).once @message.path_type = "sms" @message.to = "+18015550100" @@ -80,6 +82,7 @@ module Services end it "expects email sms message type to go through mailer" do + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(@queue).to receive(:send_message).once expect(Mailer).to receive(:create_message).once @message.path_type = "sms" @@ -88,6 +91,7 @@ module Services end it "expects twilio to not call mailer create_message" do + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(@queue).to receive(:send_message).once expect(Mailer).to receive(:create_message).never @message.path_type = "sms" diff --git a/spec/models/message_spec.rb b/spec/models/message_spec.rb index caf4faaefba..e6b2ec991c5 100644 --- a/spec/models/message_spec.rb +++ b/spec/models/message_spec.rb @@ -397,59 +397,32 @@ describe Message do allow(Canvas::Twilio).to receive(:enabled?).and_return(true) end - context 'with the deprecate_sms feature enabled' do - before :each do - Account.site_admin.enable_feature!(:deprecate_sms) - end - - after :each do - Account.site_admin.disable_feature!(:deprecate_sms) - end - - it "allows whitelisted notification types" do - message_model( - dispatch_at: Time.now, - workflow_state: 'staged', - to: '+18015550100', - updated_at: Time.now.utc - 11.minutes, - path_type: 'sms', - notification_name: 'Assignment Graded', - user: @user - ) - expect(@message).to receive(:deliver_via_sms) - @message.deliver - end - - it "does not deliver notification types not on the whitelist" do - message_model( - dispatch_at: Time.now, - workflow_state: 'staged', - to: '+18015550100', - updated_at: Time.now.utc - 11.minutes, - path_type: 'sms', - notification_name: 'Conversation Message', - user: @user - ) - expect(@message).to receive(:deliver_via_sms).never - @message.deliver - end - - it "delivers all sms when the sms_allowed override is set" do - @user.account.settings[:sms_allowed] = true - message_model( - dispatch_at: Time.now, - workflow_state: 'staged', - to: '+18015550100', - updated_at: Time.now.utc - 11.minutes, - path_type: 'sms', - notification_name: 'Conversation Message', - user: @user - ) - expect(@message).to receive(:deliver_via_sms) - @message.deliver - @user.account.settings[:sms_allowed] = nil - end + it "allows whitelisted notification types" do + message_model( + dispatch_at: Time.now, + workflow_state: 'staged', + to: '+18015550100', + updated_at: Time.now.utc - 11.minutes, + path_type: 'sms', + notification_name: 'Assignment Graded', + user: @user + ) + expect(@message).to receive(:deliver_via_sms) + @message.deliver + end + it "does not deliver notification types not on the whitelist" do + message_model( + dispatch_at: Time.now, + workflow_state: 'staged', + to: '+18015550100', + updated_at: Time.now.utc - 11.minutes, + path_type: 'sms', + notification_name: 'Conversation Message', + user: @user + ) + expect(@message).to receive(:deliver_via_sms).never + @message.deliver end it "uses Twilio for E.164 paths" do @@ -461,6 +434,7 @@ describe Message do path_type: 'sms', user: @user ) + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(Canvas::Twilio).to receive(:deliver).with('+18015550100', @message.body, from_recipient_country: true) expect(@message).to receive(:deliver_via_email).never @message.deliver @@ -475,6 +449,7 @@ describe Message do path_type: 'sms', user: @user ) + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(@message).to receive(:deliver_via_email) expect(Canvas::Twilio).to receive(:deliver).never @message.deliver @@ -489,6 +464,7 @@ describe Message do path_type: 'sms', user: @user ) + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(@message).to receive(:deliver_via_email) expect(Canvas::Twilio).to receive(:deliver).never @message.deliver @@ -503,6 +479,7 @@ describe Message do path_type: 'sms', user: @user ) + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(Canvas::Twilio).to receive(:deliver) @message.deliver @message.reload @@ -518,6 +495,7 @@ describe Message do path_type: 'sms', user: @user ) + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(Canvas::Twilio).to receive(:deliver).and_raise('some error') @message.deliver @message.reload @@ -533,6 +511,7 @@ describe Message do path_type: 'sms', user: @user ) + allow(Notification).to receive(:types_to_send_in_sms).and_return([@message.notification_name]) expect(Canvas::Twilio).to receive(:deliver).with('+18015550100', anything, from_recipient_country: true) @message.deliver @message.reload