force content migration emails to send immediately; fixes #8318

content migration notifications used to be in a category that forced them to be
sent immediately, but they inadvertently got moved to a category without that
enforcement. now they have their own category that forces sending again.

test plan:
- set your 'For Administrative Alerts' to something other than 'immediately'
- do a content import or export (not course copy)
- you should get an email as soon as the migration succeeds or fails (not in
  a summary at the end of the day/week).

Change-Id: Ifb0eddf586f3e0defc11f6159f48625e7493ccf8
Reviewed-on: https://gerrit.instructure.com/10385
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>

app/models/notification.rb

@@ -122,7 +122,7 @@ class Notification < ActiveRecord::Base
       policies << fallback_policy
     end
 
-    return false if (!opts[:fallback_channel] && cc && !cc.active?) || policies.empty? || self.registration?
+    return false if (!opts[:fallback_channel] && cc && !cc.active?) || policies.empty? || !self.summarizable?
 
     policies.inject([]) do |list, policy|
       message = Message.new(
@@ -198,11 +198,11 @@ class Notification < ActiveRecord::Base
       
       # For non-essential messages, check if too many have gone out, and if so
       # send this message as a daily summary message instead of immediate.
-      too_many_and_summarizable = user && self.summarizable? && too_many_messages?(user)
+      should_summarize = user && self.summarizable? && too_many_messages?(user)
       channels = CommunicationChannel.find_all_for(user, self, cc)
       fallback_channel = channels.sort_by{|c| c.path_type }.first
-      record_delayed_messages((options || {}).merge(:user => user, :communication_channel => cc, :asset => asset, :fallback_channel => too_many_and_summarizable ? channels.first : nil))
-      if too_many_and_summarizable
+      record_delayed_messages((options || {}).merge(:user => user, :communication_channel => cc, :asset => asset, :fallback_channel => should_summarize ? channels.first : nil))
+      if should_summarize
         channels = channels.select{|cc| cc.path_type != 'email' && cc.path_type != 'sms' }
       end
       channels << "dashboard" if self.dashboard? && self.show_in_feed?
@@ -340,13 +340,17 @@ class Notification < ActiveRecord::Base
   def registration?
     return self.category == "Registration"
   end
+
+  def migration?
+    return self.category == "Migration"
+  end
   
   def summarizable?
-    return !self.registration?
+    return !self.registration? && !self.migration?
   end
   
   def dashboard?
-    return self.category != "Registration" && self.category != "Summaries"
+    return ["Migration", "Registration", "Summaries"].include?(self.category) == false
   end
   
   def category_slug
@@ -361,7 +365,7 @@ class Notification < ActiveRecord::Base
     Notification.find(:all).each do |n|
       if !seen_types[n.category] && (user.nil? || n.relevant_to_user?(user))
         seen_types[n.category] = true
-        res << n if n.category && n.category != "Registration" && n.category != 'Summaries'
+        res << n if n.category && n.dashboard?
       end
     end
     res.sort_by{|n| n.category == "Other" ? "zzzz" : n.category }
@@ -405,6 +409,8 @@ class Notification < ActiveRecord::Base
       'daily'
     when 'Registration'
       'immediately'
+    when 'Migration'
+      'immediately'
     when 'Submission Comment'
       'daily'
     when 'Reminder'
@@ -518,6 +524,7 @@ class Notification < ActiveRecord::Base
     t 'categories.membership_update', 'Membership Update'
     t 'categories.other', 'Other'
     t 'categories.registration', 'Registration'
+    t 'categories.migration', 'Migration'
     t 'categories.reminder', 'Reminder'
     t 'categories.submission_comment', 'Submission Comment'
   end

db/migrate/20120427162634_move_migration_notifications_to_separate_category.rb

@@ -0,0 +1,13 @@
+class MoveMigrationNotificationsToSeparateCategory < ActiveRecord::Migration
+  tag :postdeploy
+
+  def self.up
+    Notification.update_all({ :category => 'Migration' }, 
+                            { :name => ['Migration Export Ready', 'Migration Import Finished', 'Migration Import Failed'] })
+  end
+
+  def self.down
+    Notification.update_all({ :category => 'Other' }, 
+                            { :name => ['Migration Export Ready', 'Migration Import Finished', 'Migration Import Failed'] })
+  end
+end

lib/tasks/db_load_data.rake

@@ -162,7 +162,7 @@ namespace :db do
       Report generation failed
     }
     
-    create_notification 'ContentMigration', 'Other', 0,
+    create_notification 'ContentMigration', 'Migration', 0,
     'http://<%= HostUrl.default_host %>', %{
       Migration Export Ready
 
@@ -173,7 +173,7 @@ namespace :db do
       The extraction process for the course, <%= asset.migration_settings[:course_name] %>, has completed.  To finish importing content into <%= asset.context.name %> you'll need to click the following link:
     }
     
-    create_notification 'ContentMigration', 'Other', 0,
+    create_notification 'ContentMigration', 'Migration', 0,
     'http://<%= HostUrl.default_host %>', %{
       Migration Import Finished
 
@@ -184,7 +184,7 @@ namespace :db do
       Importing <%= asset.migration_settings[:course_name] %> into <%= asset.context.name %> has finished.
     }
       
-    create_notification 'ContentMigration', 'Other', 0,
+    create_notification 'ContentMigration', 'Migration', 0,
     'http://<%= HostUrl.default_host %>', %{
       Migration Import Failed
 

spec/models/content_migration_spec.rb

@@ -632,18 +632,35 @@ describe ContentMigration do
       aq.question_data[:answers][1][:left_html].should == data2[:answers][1][:left_html]
     end
 
-    it "should send the correct emails" do
-      Notification.create!(:name => 'Migration Export Ready')
-      Notification.create!(:name => 'Migration Import Failed')
-      Notification.create!(:name => 'Migration Import Finished')
+    context "notifications" do
+      before(:each) do
+        Notification.create!(:name => 'Migration Export Ready', :category => 'Migration')
+        Notification.create!(:name => 'Migration Import Failed', :category => 'Migration')
+        Notification.create!(:name => 'Migration Import Finished', :category => 'Migration')
+      end
 
-      run_course_copy
+      it "should send the correct emails" do
+        run_course_copy
 
-      @cm.messages_sent['Migration Export Ready'].should be_blank
-      @cm.messages_sent['Migration Import Finished'].should be_blank
-      @cm.messages_sent['Migration Import Failed'].should be_blank
+        @cm.messages_sent['Migration Export Ready'].should be_blank
+        @cm.messages_sent['Migration Import Finished'].should be_blank
+        @cm.messages_sent['Migration Import Failed'].should be_blank
+      end
+
+      it "should send notifications immediately" do
+        communication_channel_model(:user_id => @user).confirm!
+        @cm.source_course = nil # so that it's not a course copy
+        @cm.save!
+
+        @cm.workflow_state = 'exported'
+        expect { @cm.save! }.to change(DelayedMessage, :count).by 0
+        @cm.messages_sent['Migration Export Ready'].should_not be_blank
+
+        @cm.workflow_state = 'imported'
+        expect { @cm.save! }.to change(DelayedMessage, :count).by 0
+        @cm.messages_sent['Migration Import Finished'].should_not be_blank
+      end
     end
-
   end
 
   context "import_object?" do

spec/models/notification_spec.rb

@@ -246,6 +246,18 @@ describe Notification do
       messages.map(&:to).sort.should == ['dashboard', 'value for path']
     end
 
+    it "should force certain categories to send immediately" do
+      notification_set(:notification_opts => { :name => "Thing 1", :category => 'Not Migration' })
+      @notification_policy.frequency = 'daily'
+      @notification_policy.save!
+      expect { @notification.create_message(@assignment, @user) }.to change(DelayedMessage, :count).by 1
+
+      notification_set(:notification_opts => { :name => "Thing 2", :category => 'Migration' })
+      @notification_policy.frequency = 'daily'
+      @notification_policy.save!
+      expect { @notification.create_message(@assignment, @user) }.to change(DelayedMessage, :count).by 0
+    end
+
     context "sharding" do
       it_should_behave_like "sharding"