From cd4f95e209c01d94903605df984ff0f3e9268cf3 Mon Sep 17 00:00:00 2001 From: Brian Palmer Date: Wed, 12 Jun 2013 16:42:36 -0600 Subject: [PATCH] survey notification support Special survey account notifications (announcements) can be set up on the site_admin account. These survey notifications will only appear for accounts that have the "Account Surveys" setting enabled in their account settings, and they'll only show up for 1/N users in those accounts each month. N is configurable, defaults to 9. closes CNVS-6036 test plan: * On a regular account, create an announcement in account settings. There shouldn't be any options related to surveys available. * On the site admin account, create an announcement. Select to make it a survey. You can leave N at 9 or change it. * Verify that the survey doesn't show up for any users on accounts that don't have "Account Surveys" enabled. * Enable the "Account Surveys" setting on an account. Verify that the survey shows up for (roughly) 1 out of every N users in the account, on their dashboard. Change the time on the computer running canvas to another month. Verify that the survey shows up for a different set of 1/N users in the account. Change-Id: If11467d2153acee24a010ba45d516b0b320a4634 Reviewed-on: https://gerrit.instructure.com/21432 Tested-by: Jenkins Reviewed-by: Cody Cutrer QA-Review: Jeremy Putnam Product-Review: Brian Palmer --- app/controllers/accounts_controller.rb | 2 +- app/models/account.rb | 13 +++- app/models/account_notification.rb | 65 +++++++++++++++---- app/stylesheets/account_settings.sass | 7 ++ app/views/accounts/settings.html.erb | 22 ++++++- ..._functionality_to_account_notifications.rb | 18 +++++ public/javascripts/account_settings.js | 18 +++-- spec/controllers/accounts_controller_spec.rb | 5 +- spec/models/account_notification_spec.rb | 35 ++++++++++ spec/models/account_spec.rb | 8 +-- spec/selenium/admin/admin_settings_spec.rb | 15 ++++- 11 files changed, 181 insertions(+), 27 deletions(-) create mode 100644 db/migrate/20130611194212_add_survey_functionality_to_account_notifications.rb diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 99cb23427ef..c5144180f64 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -258,7 +258,7 @@ class AccountsController < ApplicationController :default_group_storage_quota, :default_group_storage_quota_mb].each { |key| params[:account].delete key } end if params[:account][:services] - params[:account][:services].slice(*Account.services_exposed_to_ui_hash(nil, @current_user).keys).each do |key, value| + params[:account][:services].slice(*Account.services_exposed_to_ui_hash(nil, @current_user, @account).keys).each do |key, value| @account.set_service_availability(key, value == '1') end params[:account].delete :services diff --git a/app/models/account.rb b/app/models/account.rb index 08c08d8de41..446bfa27999 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1123,7 +1123,14 @@ class Account < ActiveRecord::Base :description => "", :default => false, :expose_to_ui => :setting - } + }, + :account_survey_notifications => { + :name => "Account Surveys", + :description => "", + :default => false, + :expose_to_ui => :setting, + :expose_to_ui_proc => proc { |user, account| user && account && account.grants_right?(user, :manage_site_settings) }, + }, }.merge(@plugin_services || {}).freeze end @@ -1198,12 +1205,12 @@ class Account < ActiveRecord::Base # if expose_as is nil, all services exposed in the ui are returned # if it's :service or :setting, then only services set to be exposed as that type are returned - def self.services_exposed_to_ui_hash(expose_as = nil, current_user = nil) + def self.services_exposed_to_ui_hash(expose_as = nil, current_user = nil, account = nil) if expose_as self.allowable_services.reject { |key, setting| setting[:expose_to_ui] != expose_as } else self.allowable_services.reject { |key, setting| !setting[:expose_to_ui] } - end.reject { |key, setting| setting[:expose_to_ui_proc] && !setting[:expose_to_ui_proc].call(current_user) } + end.reject { |key, setting| setting[:expose_to_ui_proc] && !setting[:expose_to_ui_proc].call(current_user, account) } end def service_enabled?(service) diff --git a/app/models/account_notification.rb b/app/models/account_notification.rb index 0b41ce65619..561e208c58a 100644 --- a/app/models/account_notification.rb +++ b/app/models/account_notification.rb @@ -1,6 +1,7 @@ class AccountNotification < ActiveRecord::Base attr_accessible :subject, :icon, :message, - :account, :user, :start_at, :end_at + :account, :user, :start_at, :end_at, + :required_account_service, :months_in_display_cycle validates_presence_of :start_at, :end_at, :account_id before_validation :infer_defaults @@ -8,7 +9,12 @@ class AccountNotification < ActiveRecord::Base belongs_to :user validates_length_of :message, :maximum => maximum_text_length, :allow_nil => false, :allow_blank => false sanitize_field :message, Instructure::SanitizeField::SANITIZE - + + ACCOUNT_SERVICE_NOTIFICATION_FLAGS = %w[account_survey_notifications] + validates_inclusion_of :required_account_service, in: ACCOUNT_SERVICE_NOTIFICATION_FLAGS, allow_nil: true + + validates_inclusion_of :months_in_display_cycle, in: 1..48, allow_nil: true + def infer_defaults self.start_at ||= Time.now.utc self.end_at ||= self.start_at + 2.weeks @@ -16,16 +22,10 @@ class AccountNotification < ActiveRecord::Base end def self.for_user_and_account(user, account) - closed_ids = user.preferences[:closed_notifications] || [] - now = Time.now.utc - # Refreshes every 10 minutes at the longest - current = Rails.cache.fetch(['account_notifications2', account].cache_key, :expires_in => 10.minutes) do - Shard.partition_by_shard([Account.site_admin, account]) do |accounts| - AccountNotification.where("account_id IN (?) AND start_at ?", accounts, now, now).order('start_at DESC').all - end - end + current = self.for_account(account) user.shard.activate do + closed_ids = user.preferences[:closed_notifications] || [] # If there are ids marked as 'closed' that are no longer # applicable, they probably need to be cleared out. current_ids = current.map(&:id) @@ -33,7 +33,50 @@ class AccountNotification < ActiveRecord::Base closed_ids = user.preferences[:closed_notifications] &= current_ids user.save! end - current.reject { |announcement| closed_ids.include?(announcement.id) } + current.reject! { |announcement| closed_ids.include?(announcement.id) } + + # filter out announcements that have a periodic cycle of display, + # and the user isn't in the set of users to display it to this month (based + # on user id) + current.reject! do |announcement| + if months_in_period = announcement.months_in_display_cycle + !self.display_for_user?(user.id, months_in_period) + end + end + end + + current + end + + def self.for_account(account) + # Refreshes every 10 minutes at the longest + Rails.cache.fetch(['account_notifications2', account].cache_key, :expires_in => 10.minutes) do + now = Time.now.utc + # we always check the given account for the flag, even if the announcement is from the site_admin account + # this allows us to make a global announcement that is filtered to only accounts with this flag + enabled_flags = ACCOUNT_SERVICE_NOTIFICATION_FLAGS & account.allowed_services_hash.keys.map(&:to_s) + + Shard.partition_by_shard([Account.site_admin, account]) do |accounts| + AccountNotification.where("account_id IN (?) AND start_at ?", accounts, now, now). + where("required_account_service IS NULL OR required_account_service IN (?)", enabled_flags). + order('start_at DESC').all + end end end + + def self.default_months_in_display_cycle + Setting.get_cached("account_notification_default_months_in_display_cycle", "9").to_i + end + + # private + def self.display_for_user?(user_id, months_in_period, current_time = Time.now.utc) + # we just need a stable reference point, doesn't matter what it is, so + # let's use unix epoch + start_time = Time.at(0).utc + months_since_start_time = (current_time.year - start_time.year) * 12 + (current_time.month - start_time.month) + periods_since_start_time = months_since_start_time / months_in_period + months_into_current_period = months_since_start_time % months_in_period + mod_value = (Random.new(periods_since_start_time).rand(months_in_period) + months_into_current_period) % months_in_period + user_id % months_in_period == mod_value + end end diff --git a/app/stylesheets/account_settings.sass b/app/stylesheets/account_settings.sass index b2cae8e7299..3560a0beb0f 100644 --- a/app/stylesheets/account_settings.sass +++ b/app/stylesheets/account_settings.sass @@ -93,3 +93,10 @@ legend font-size: 15px font-weight: bold + +#survey_announcement_field + line-height: 30px + input#account_notification_months_in_display_cycle + width: 25px + padding: 0 + margin-top: 6px diff --git a/app/views/accounts/settings.html.erb b/app/views/accounts/settings.html.erb index e6f3bed98fe..4a1f1716516 100644 --- a/app/views/accounts/settings.html.erb +++ b/app/views/accounts/settings.html.erb @@ -282,7 +282,7 @@ TEXT <% end %> <% end %> <% f.fields_for :services do |services| %> - <% Account.services_exposed_to_ui_hash(:setting, @current_user).sort_by { |k,h| h[:name] }.each do |key, service| %> + <% Account.services_exposed_to_ui_hash(:setting, @current_user, @account).sort_by { |k,h| h[:name] }.each do |key, service| %>
<%= services.check_box key, :checked => @account.service_enabled?(key) %> <%= services.label key, service[:name] + " " %> @@ -652,6 +652,16 @@ TEXT    <%= link_to(context_user_name(@account, announcement.user_id), user_path(announcement.user_id)) %>
+ <% if announcement.required_account_service %> +
+ <%= Account.allowable_services[announcement.required_account_service.to_sym].try(:[], :name) %> +
+ <% end %> + <% if announcement.months_in_display_cycle %> +
+ <%= t :announcement_sent_to_subset, "Sent to 1 / %{denominator} users each month", denominator: announcement.months_in_display_cycle %> +
+ <% end %>
<%= user_content(announcement.message) %>
@@ -684,8 +694,16 @@ TEXT <% if @account.site_admin? %> + + <%= label_tag :confirm_global_announcement, t(:confirm_global_announcement, "This announcement will be shown to *all* Canvas users. Confirm that you want to create it.", :wrapper => '\1') %> + - <%= label_tag :confirm_global_announcement, t(:confirm_global_announcement, "This announcement will be shown to *all* Canvas users. Confirm that you want to create it.", :wrapper => '\1') %> + +
+ <%# don't use the rails check_box helper here, because we don't want the extra hidden element when not checked %> + + + <% end %> diff --git a/db/migrate/20130611194212_add_survey_functionality_to_account_notifications.rb b/db/migrate/20130611194212_add_survey_functionality_to_account_notifications.rb new file mode 100644 index 00000000000..c18a2c93725 --- /dev/null +++ b/db/migrate/20130611194212_add_survey_functionality_to_account_notifications.rb @@ -0,0 +1,18 @@ +class AddSurveyFunctionalityToAccountNotifications < ActiveRecord::Migration + tag :predeploy + + def self.up + add_column :account_notifications, :required_account_service, :string + add_column :account_notifications, :months_in_display_cycle, :int + # this table is small enough for transactional index creation + add_index :account_notifications, [:account_id, :end_at, :start_at], name: "index_account_notifications_by_account_and_timespan" + remove_index :account_notifications, [:account_id, :start_at] + end + + def self.down + remove_column :account_notifications, :required_account_setting + remove_column :account_notifications, :months_in_display_cycle + add_index :account_notifications, [:account_id, :start_at] + remove_index :account_notifications, name: "index_account_notifications_by_account_and_timespan" + end +end diff --git a/public/javascripts/account_settings.js b/public/javascripts/account_settings.js index d128e16e0fa..62f79599d22 100644 --- a/public/javascripts/account_settings.js +++ b/public/javascripts/account_settings.js @@ -37,20 +37,30 @@ define([ }); $("#add_notification_form").submit(function(event) { var $this = $(this); - var $confirmation = $this.find('#confirm_global_announcement:not(:checked)'); + var $confirmation = $this.find('#confirm_global_announcement:visible:not(:checked)'); if ($confirmation.length > 0) { $confirmation.errorBox(I18n.t('confirms.global_announcement', "You must confirm the global announcement")); return false; } - var result = $this.validateForm({ + var validations = { object_name: 'account_notification', required: ['start_at', 'end_at', 'subject', 'message'], - date_fields: ['start_at', 'end_at'] - }); + date_fields: ['start_at', 'end_at'], + numbers: [] + }; + if ($('#account_notification_months_in_display_cycle').length > 0) { + validations.numbers.push('months_in_display_cycle'); + } + var result = $this.validateForm(validations); if(!result) { return false; } }); + $("#account_notification_required_account_service").click(function(event) { + $this = $(this); + $("#confirm_global_announcement_field").showIf(!$this.is(":checked")); + $("#account_notification_months_in_display_cycle").prop("disabled", !$this.is(":checked")); + }); $(".delete_notification_link").click(function(event) { event.preventDefault(); var $link = $(this); diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index 551dd64b6ca..362684a7f5b 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/spec/controllers/accounts_controller_spec.rb @@ -293,19 +293,22 @@ describe AccountsController do it "should allow updating services that appear in the ui for the current user" do Account.register_service(:test1, { name: 'test1', description: '', expose_to_ui: :setting, default: false }) - Account.register_service(:test2, { name: 'test2', description: '', expose_to_ui: :setting, default: false, expose_to_ui_proc: proc { |user| false } }) + Account.register_service(:test2, { name: 'test2', description: '', expose_to_ui: :setting, default: false, expose_to_ui_proc: proc { |user, account| false } }) user_session(user) @account = Account.create! + Account.register_service(:test3, { name: 'test3', description: '', expose_to_ui: :setting, default: false, expose_to_ui_proc: proc { |user, account| account == @account } }) Account.site_admin.add_user(@user) post 'update', id: @account.id, account: { services: { 'test1' => '1', 'test2' => '1', + 'test3' => '1', } } @account.reload @account.allowed_services.should match(%r{\+test1}) @account.allowed_services.should_not match(%r{\+test2}) + @account.allowed_services.should match(%r{\+test3}) end describe "quotas" do diff --git a/spec/models/account_notification_spec.rb b/spec/models/account_notification_spec.rb index 9490923a437..b8454c3c3c8 100644 --- a/spec/models/account_notification_spec.rb +++ b/spec/models/account_notification_spec.rb @@ -49,6 +49,41 @@ describe AccountNotification do @user.preferences[:closed_notifications].should == [] end + describe "survey notifications" do + it "should only display for flagged accounts" do + flag = AccountNotification::ACCOUNT_SERVICE_NOTIFICATION_FLAGS.first + @announcement = Account.site_admin.announcements.create!(message: "hello", required_account_service: flag) + @a1 = account_model + @a2 = account_model + @a2.enable_service(flag) + @a2.save! + AccountNotification.for_account(@a1).should == [] + AccountNotification.for_account(@a2).should == [@announcement] + end + + describe "display_for_user?" do + it "should select each mod value once throughout the cycle" do + AccountNotification.display_for_user?(5, 3, Time.zone.parse('2012-04-02')).should == false + AccountNotification.display_for_user?(6, 3, Time.zone.parse('2012-04-02')).should == false + AccountNotification.display_for_user?(7, 3, Time.zone.parse('2012-04-02')).should == true + + AccountNotification.display_for_user?(5, 3, Time.zone.parse('2012-05-05')).should == true + AccountNotification.display_for_user?(6, 3, Time.zone.parse('2012-05-05')).should == false + AccountNotification.display_for_user?(7, 3, Time.zone.parse('2012-05-05')).should == false + + AccountNotification.display_for_user?(5, 3, Time.zone.parse('2012-06-04')).should == false + AccountNotification.display_for_user?(6, 3, Time.zone.parse('2012-06-04')).should == true + AccountNotification.display_for_user?(7, 3, Time.zone.parse('2012-06-04')).should == false + end + + it "should shift the mod values each new cycle" do + AccountNotification.display_for_user?(7, 3, Time.zone.parse('2012-04-02')).should == true + AccountNotification.display_for_user?(7, 3, Time.zone.parse('2012-07-02')).should == false + AccountNotification.display_for_user?(7, 3, Time.zone.parse('2012-09-02')).should == true + end + end + end + context "sharding" do specs_require_sharding diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 0f8c892982d..fe5b421baf9 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -266,7 +266,7 @@ describe Account do Account.services_exposed_to_ui_hash(:setting).keys.should == Account.allowable_services.reject { |h,k| k[:expose_to_ui] != :setting || (k[:expose_to_ui_proc] && !k[:expose_to_ui_proc].call(nil)) }.keys end - it "should filter based on user if a proc is specified" do + it "should filter based on user and account if a proc is specified" do user1 = User.create! user2 = User.create! Account.register_service(:myservice, { @@ -274,11 +274,11 @@ describe Account do description: "Nope", expose_to_ui: :setting, default: false, - expose_to_ui_proc: proc { |user| user == user2 }, + expose_to_ui_proc: proc { |user, account| user == user2 && account == Account.default }, }) Account.services_exposed_to_ui_hash(:setting).keys.should_not be_include(:myservice) - Account.services_exposed_to_ui_hash(:setting, user1).keys.should_not be_include(:myservice) - Account.services_exposed_to_ui_hash(:setting, user2).keys.should be_include(:myservice) + Account.services_exposed_to_ui_hash(:setting, user1, Account.default).keys.should_not be_include(:myservice) + Account.services_exposed_to_ui_hash(:setting, user2, Account.default).keys.should be_include(:myservice) end end diff --git a/spec/selenium/admin/admin_settings_spec.rb b/spec/selenium/admin/admin_settings_spec.rb index 4773ed4a71d..08326edef48 100644 --- a/spec/selenium/admin/admin_settings_spec.rb +++ b/spec/selenium/admin/admin_settings_spec.rb @@ -42,6 +42,7 @@ describe "settings tabs" do notification.start_at.to_s.should include_text today notification.end_at.to_s.should include_text tomorrow f("#tab-announcements .user_content").text.should == "this is a message" + notification end describe "site admin" do @@ -70,6 +71,16 @@ describe "settings tabs" do f("#confirm_global_announcement").click end end + + it "should create survey announcements" do + notification = add_announcement do + f("#account_notification_required_account_service").click + get_value("#account_notification_months_in_display_cycle").should == AccountNotification.default_months_in_display_cycle.to_s + set_value(f("#account_notification_months_in_display_cycle"), "12") + end + notification.required_account_service.should == "account_survey_notifications" + notification.months_in_display_cycle.should == 12 + end end end @@ -162,7 +173,9 @@ describe "settings tabs" do context "announcements tab" do it "should add an announcement" do - add_announcement + notification = add_announcement + notification.required_account_service.should be_nil + notification.months_in_display_cycle.should be_nil end it "should delete an announcement" do