From 1c1fba63e7391b377a464a156680690bff0b39f2 Mon Sep 17 00:00:00 2001
From: Raphael Sofaer <raphael@joindiaspora.com>
Date: Tue, 1 Mar 2011 10:20:23 -0800
Subject: [PATCH] Refactor notification to have subclasses, just a start
---
app/helpers/notifications_helper.rb | 12 +++++------
app/models/comment.rb | 4 ++--
app/models/mention.rb | 2 +-
app/models/notification.rb | 21 ++++++++++---------
app/models/request.rb | 4 ++--
.../20110228180709_notification_subclasses.rb | 9 ++++----
db/schema.rb | 2 +-
spec/factories.rb | 2 +-
spec/models/comment_spec.rb | 4 ++--
spec/models/mention_spec.rb | 6 +++---
spec/models/notification_spec.rb | 10 ++++-----
spec/models/request_spec.rb | 4 ++--
spec/models/user/connecting_spec.rb | 4 ++--
spec/support/no_id_on_object.rb | 5 +++++
14 files changed, 47 insertions(+), 42 deletions(-)
create mode 100644 spec/support/no_id_on_object.rb
diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb
index c48b32af48..b98db63beb 100644
--- a/app/helpers/notifications_helper.rb
+++ b/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
diff --git a/app/models/comment.rb b/app/models/comment.rb
index 275acffba2..3348906323 100644
--- a/app/models/comment.rb
+++ b/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
diff --git a/app/models/mention.rb b/app/models/mention.rb
index 22dae4a026..86759d08b0 100644
--- a/app/models/mention.rb
+++ b/app/models/mention.rb
@@ -19,7 +19,7 @@ class Mention < ActiveRecord::Base
def notification_type(*args)
- 'mentioned'
+ Notifications::Mentioned
end
def delete_notification
diff --git a/app/models/notification.rb b/app/models/notification.rb
index 14bc6bb7ab..6af9d9b4bc 100644
--- a/app/models/notification.rb
+++ b/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)
- if n = Notification.where(:target_id => target.id,
+ def self.concatenate_or_create(recipient, target, actor, notification_type)
+ 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)
- n = Notification.new(:target => target,
- :action => action,
+ def self.make_notification(recipient, target, actor, notification_type)
+ n = notification_type.new(:target => target,
:recipient_id => recipient.id)
n.actors << actor
n.unread = false if target.is_a? Request
diff --git a/app/models/request.rb b/app/models/request.rb
index 6f8c58a855..90c17b8864 100644
--- a/app/models/request.rb
+++ b/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
diff --git a/db/migrate/20110228180709_notification_subclasses.rb b/db/migrate/20110228180709_notification_subclasses.rb
index 96f2d04fcf..18d210ac1c 100644
--- a/db/migrate/20110228180709_notification_subclasses.rb
+++ b/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}
- where action = #{key.to_s}")
+ set type = '#{value}'
+ 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}
- where type = #{value}")
+ set action = '#{key.to_s}'
+ where type = '#{value}'")
+ end
remove_column :notifications, :type
end
end
diff --git a/db/schema.rb b/db/schema.rb
index f3c65a25f9..2dfe5390c1 100644
--- a/db/schema.rb
+++ b/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"
diff --git a/spec/factories.rb b/spec/factories.rb
index 798077740e..1e784e7618 100644
--- a/spec/factories.rb
+++ b/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
diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb
index 708574ae94..e80cfcf3d4 100644
--- a/spec/models/comment_spec.rb
+++ b/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
diff --git a/spec/models/mention_spec.rb b/spec/models/mention_spec.rb
index e16f68eff7..8f08abcc47 100644
--- a/spec/models/mention_spec.rb
+++ b/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
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 56ed2269a2..21b3ceaf42 100644
--- a/spec/models/notification_spec.rb
+++ b/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
diff --git a/spec/models/request_spec.rb b/spec/models/request_spec.rb
index ffa1ce6f35..fd6fb20746 100644
--- a/spec/models/request_spec.rb
+++ b/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
diff --git a/spec/models/user/connecting_spec.rb b/spec/models/user/connecting_spec.rb
index 0d650766bb..8cc5d3c908 100644
--- a/spec/models/user/connecting_spec.rb
+++ b/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
diff --git a/spec/support/no_id_on_object.rb b/spec/support/no_id_on_object.rb
new file mode 100644
index 0000000000..16f8621a79
--- /dev/null
+++ b/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
--
GitLab