Refactor notification to have subclasses, just a start

app/helpers/notifications_helper.rb

@@ -1,26 +1,26 @@
 module NotificationsHelper
   def object_link(note)
-    target_type = note.action
+    target_type = note.class
     case target_type
-    when 'mentioned'
+    when Notifications::Mentioned
       post = Mention.find(note.target_id).post
       if post
         "#{translation(target_type)} #{link_to t('notifications.post'), object_path(post)}".html_safe
       else
         "#{translation(target_type)} #{t('notifications.deleted')} #{t('notifications.post')}"
       end
-    when 'request_accepted'
+    when Notifications::RequestAccepted
       translation(target_type)
-    when 'new_request'
+    when Notifications::NewRequest
       translation(target_type)
-    when 'comment_on_post'
+    when Notifications::CommentOnPost
       post = Post.where(:id => note.target_id).first
       if post
         "#{translation(target_type)} #{link_to t('notifications.post'), object_path(post)}".html_safe
       else
         "#{translation(target_type)} #{t('notifications.deleted')} #{t('notifications.post')}"
       end
-    when 'also_commented'
+    when Notifications::AlsoCommented
       post = Post.where(:id => note.target_id).first
       if post
         "#{translation(target_type, post.person.name)} #{link_to t('notifications.post'), object_path(post)}".html_safe

app/models/comment.rb

@@ -44,9 +44,9 @@ class Comment < ActiveRecord::Base
 
   def notification_type(user, person)
     if self.post.person == user.person
-      return "comment_on_post"
+      return Notifications::CommentOnPost
     elsif self.post.comments.where(:person_id => user.person.id) != [] && self.person_id != user.person.id
-      return "also_commented"
+      return Notifications::AlsoCommented
     else
       return false
     end

app/models/mention.rb

@@ -19,7 +19,7 @@ class Mention < ActiveRecord::Base
 
 
   def notification_type(*args)
-    'mentioned'
+    Notifications::Mentioned
   end
 
   def delete_notification

app/models/notification.rb

@@ -17,11 +17,11 @@ class Notification < ActiveRecord::Base
 
   def self.notify(recipient, target, actor)
     if target.respond_to? :notification_type
-      if action = target.notification_type(recipient, actor)
+      if note_type = target.notification_type(recipient, actor)
         if target.is_a? Comment
-          n = concatenate_or_create(recipient, target.post, actor, action)
+          n = concatenate_or_create(recipient, target.post, actor, note_type)
         else
-          n = make_notification(recipient, target, actor, action)
+          n = make_notification(recipient, target, actor, note_type)
         end
         n.email_the_user(target, actor) if n
         n.socket_to_user(recipient, :actor => actor) if n
@@ -33,12 +33,14 @@ class Notification < ActiveRecord::Base
   def email_the_user(target, actor)
     self.recipient.mail(self.mail_job, self.recipient_id, actor.id, target.id)
   end
+  def mail_job
+    raise NotImplementedError.new('Subclass this.')
+  end
 
 private
-  def self.concatenate_or_create(recipient, target, actor, action)
+  def self.concatenate_or_create(recipient, target, actor, notification_type)
-    if n = Notification.where(:target_id => target.id,
+    if n = notification_type.where(:target_id => target.id,
                               :target_type => target.class.base_class,
-                               :action => action,
                                :recipient_id => recipient.id).first
       unless n.actors.include?(actor)
         n.actors << actor
@@ -48,13 +50,12 @@ private
       n.save!
       n
     else
-      make_notification(recipient, target, actor, action)
+      make_notification(recipient, target, actor, notification_type)
     end
   end
 
-  def self.make_notification(recipient, target, actor, action)
+  def self.make_notification(recipient, target, actor, notification_type)
-    n = Notification.new(:target => target,
+    n = notification_type.new(:target => target,
-                               :action => action,
                                :recipient_id => recipient.id)
     n.actors << actor
     n.unread = false if target.is_a? Request

app/models/request.rb

@@ -56,9 +56,9 @@ class Request < ActiveRecord::Base
 
   def notification_type(user, person)
     if Contact.where(:user_id => user.id, :person_id => person.id).first
-      "request_accepted"
+      Notifications::RequestAccepted
     else
-      "new_request"
+      Notifications::NewRequest
     end
   end
 

db/migrate/20110228180709_notification_subclasses.rb

@@ -8,8 +8,8 @@ class NotificationSubclasses < ActiveRecord::Migration
       :mentioned => 'Notifications::Mentioned'
     }.each_pair do |key, value|
       execute("UPDATE notifications
-              set type = #{value}
+              set type = '#{value}'
-              where action = #{key.to_s}")
+              where action = '#{key.to_s}'")
     end
     remove_column :notifications, :action
   end
@@ -23,8 +23,9 @@ class NotificationSubclasses < ActiveRecord::Migration
       :mentioned => 'Notifications::Mentioned'
     }.each_pair do |key, value|
       execute("UPDATE notifications
-              set action = #{key.to_s}
+              set action = '#{key.to_s}'
-              where type = #{value}")
+              where type = '#{value}'")
+    end
     remove_column :notifications, :type
   end
 end

db/schema.rb

@@ -318,10 +318,10 @@ ActiveRecord::Schema.define(:version => 20110228201109) do
     t.string   "target_type"
     t.integer  "target_id"
     t.integer  "recipient_id",                   :null => false
-    t.string   "action",                         :null => false
     t.boolean  "unread",       :default => true, :null => false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string   "type"
   end
 
   add_index "notifications", ["recipient_id"], :name => "index_notifications_on_recipient_id"

spec/factories.rb

@@ -92,10 +92,10 @@ end
 Factory.define(:notification) do |n|
   n.association :recipient, :factory => :user
   n.association :target, :factory => :comment
+  n.type 'Notifications::AlsoCommented'
 
   n.after_build do |note|
     note.actors << Factory.build( :person )
-    note.action = note.target.notification_type(note.recipient, note.actors.first)
   end
 end
 

spec/models/comment_spec.rb

@@ -20,7 +20,7 @@ describe Comment do
 
     it "returns 'comment_on_post' if the comment is on a post you own" do
       comment = bob.comment("why so formal?", :on => @alices_post)
-      comment.notification_type(alice, bob.person).should == 'comment_on_post'
+      comment.notification_type(alice, bob.person).should == Notifications::CommentOnPost
     end
 
     it 'returns false if the comment is not on a post you own and no one "also_commented"' do
@@ -39,7 +39,7 @@ describe Comment do
       end
 
       it "returns 'also_commented' if another person commented on a post you commented on" do
-        @comment.notification_type(bob, alice.person).should == 'also_commented'
+        @comment.notification_type(bob, alice.person).should == Notifications::AlsoCommented
       end
     end
   end

spec/models/mention_spec.rb

@@ -21,7 +21,7 @@ describe Mention do
       @m.save
     end
 
-    it 'should only notify if the person is local' do 
+    it 'should only notify if the person is local' do
       m = Mention.new(:person => Factory(:person), :post => @sm)
       Notification.should_not_receive(:notify)
       m.save
@@ -44,10 +44,10 @@ describe Mention do
 
   describe '#notification_type' do
     it "returns 'mentioned'" do
-     Mention.new.notification_type.should == 'mentioned' 
+     Mention.new.notification_type.should == Notifications::Mentioned
     end
   end
-  
+
   describe 'after destroy' do
     it 'destroys a notification' do
       @user = alice

spec/models/notification_spec.rb

@@ -13,7 +13,7 @@ describe Notification do
     @aspect  = @user.aspects.create(:name => "dudes")
     @opts = {:target_id => @sm.id,
       :target_type => @sm.class.name,
-      :action => "comment_on_post",
+      :type => 'Notifications::CommentOnPost',
       :actors => [@person],
       :recipient_id => @user.id}
     @note = Notification.new(@opts)
@@ -61,11 +61,10 @@ describe Notification do
       it 'sockets to the recipient' do
         opts = {:target_id => @request.id,
           :target_type => "Request",
-          :action => @request.notification_type(@user, @person),
           :actors => [@person],
           :recipient_id => @user.id}
 
-        n = Notification.create(opts)
+        n = @request.notification_type(@user, @person).create(opts)
         Notification.stub!(:make_notification).and_return n
 
         n.should_receive(:socket_to_user).once
@@ -75,15 +74,14 @@ describe Notification do
       describe '#emails_the_user' do
         it 'calls mail' do
           opts = {
-            :action => "new_request",
             :actors => [@person],
             :recipient_id => @user.id}
 
-            n = Notification.new(opts)
+            n = Notifications::NewRequest.new(opts)
             n.stub!(:recipient).and_return @user
 
             @user.should_receive(:mail)
-            n.email_the_user("mock", @person)
+            n.email_the_user(@request, @person)
         end
       end
 

spec/models/request_spec.rb

@@ -56,11 +56,11 @@ describe Request do
     end
     it "returns 'request_accepted' if there is a pending contact" do
       Contact.create(:user_id => @user.id, :person_id => @person.id)
-      @request.notification_type(@user, @person).should  == "request_accepted"
+      @request.notification_type(@user, @person).should  == Notifications::RequestAccepted
     end
 
     it 'returns new_request if there is not a pending contact' do
-      @request.notification_type(@user, @person).should  == "new_request"
+      @request.notification_type(@user, @person).should  == Notifications::NewRequest
     end
   end
 

spec/models/user/connecting_spec.rb

@@ -67,7 +67,7 @@ describe Diaspora::UserModules::Connecting do
       end
 
       it 'enqueues a mail job' do
-        Resque.should_receive(:enqueue).with(Job::MailRequestReceived, user.id, person.id)
+        Resque.should_receive(:enqueue).with(Job::MailRequestReceived, user.id, person.id, anything)
         zord = Postzord::Receiver.new(user, :object => @r, :person => person)
         zord.receive_object
       end
@@ -87,7 +87,7 @@ describe Diaspora::UserModules::Connecting do
         Request.where(:sender_id => user2.person.id, :recipient_id => user.person.id).should be_empty
       end
       it 'enqueues a mail job' do
-        Resque.should_receive(:enqueue).with(Job::MailRequestAcceptance, user.id, user2.person.id).once
+        Resque.should_receive(:enqueue).with(Job::MailRequestAcceptance, user.id, user2.person.id, nil).once
         zord = Postzord::Receiver.new(user, :object => @acceptance, :person => user2.person)
         zord.receive_object
       end

spec/support/no_id_on_object.rb

@@ -0,0 +1,5 @@
+class Object
+  def id
+    raise "You have called id on a non-ActiveRecord object."
+  end
+end