ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

remove deprecate_sms feature flag 

fixes VICE-1001
flag=deprecate_sms

The sms_allowed account setting was used to allow institutions to
opt out of the deprecation. As we've cleaned up all such institutions
and don't intend to support this behavior any more, the setting was
also removed.

test plan:
  - make sure you have a mobile device set up as a communication channel
    - this can be done on /profile/settings under 'ways to contact'
    - enter whatever number you'd like
    - in a rails console, you can find your confirmation code with
      > CommunicationChannel.last.confirmation_code
  - navigate to /profile/communication
    - the SMS options should be limited to announcements and grading
  - message delivery/prevention is adequately tested in unit tests and
    difficult to manually test without AWS resources

qa risk: low

Change-Id: I59ac6f020ad57da39f05f37dd6cf754f86daed8d
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/256900
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Caleb Guanzon <cguanzon@instructure.com>
QA-Review: Caleb Guanzon <cguanzon@instructure.com>
Product-Review: Caleb Guanzon <cguanzon@instructure.com>
This commit is contained in:
Davis Hyer 2021-01-15 13:42:56 -07:00
parent df7bd93b75
commit bfa0faf959
10 changed files with 37 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/coffeescripts/notifications/NotificationPreferences.js
View File

@ -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'

1
app/controllers/courses_controller.rb
View File

@ -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,

2
app/controllers/profile_controller.rb
View File

@ -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

5
app/jsx/notification_preferences/NotificationPreferencesTable.js
View File

@ -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 => {

2
app/jsx/notification_preferences/__tests__/NotificationPreferencesTable.test.js
View File

@ -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']
}
}

2
app/models/message.rb
View File

@ -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',

8
config/feature_flags/vice_release_flags.yml
View File

@ -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

1
spec/coffeescripts/notifications/NotificationPreferencesSpec.js
View File

@ -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']
}

4
spec/lib/services/notification_service_spec.rb
View File

@ -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"

83
spec/models/message_spec.rb
View File

@ -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