diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb
index 7856f6edfd69696396bc3d37b7e4ecbab84e49e3..0f9b02989749034ff01607540b0b14b323057b37 100644
--- a/app/controllers/notifications_controller.rb
+++ b/app/controllers/notifications_controller.rb
@@ -19,7 +19,7 @@ class NotificationsController < ApplicationController
def index
@notifications = Notification.find(:all, :conditions => {:recipient_id => current_user.id},
- :order => 'created_at desc', :include => [:target]).paginate :page => params[:page], :per_page => 25
+ :order => 'created_at desc', :include => [:target, {:actors => :profile}]).paginate :page => params[:page], :per_page => 25
@group_days = @notifications.group_by{|note| note.created_at.strftime("%B %d") }
respond_with @notifications
end
diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb
index e5c7396672f860e29165cca50dc6d13d600d5d24..5c97f7dd1949bb40a74e8e1d2d029d7ae11797a3 100644
--- a/app/helpers/notifications_helper.rb
+++ b/app/helpers/notifications_helper.rb
@@ -8,16 +8,16 @@ module NotificationsHelper
when 'new_request'
translation
when 'comment_on_post'
- comment = Comment.where(:id => note.target_id).first
- if comment
- "#{translation} #{link_to t('notifications.post'), object_path(comment.post)}".html_safe
+ post = Post.where(:id => note.target_id).first
+ if post
+ "#{translation} #{link_to t('notifications.post'), object_path(post)}".html_safe
else
"#{translation} #{t('notifications.deleted')} #{t('notifications.post')}"
end
when 'also_commented'
- comment = Comment.where(:id => note.target_id).first
- if comment
- "#{translation} #{link_to t('notifications.post'), object_path(comment.post)}".html_safe
+ post = Post.where(:id => note.target_id).first
+ if post
+ "#{translation} #{link_to t('notifications.post'), object_path(post)}".html_safe
else
"#{translation} #{t('notifications.deleted')} #{t('notifications.post')}"
end
@@ -38,4 +38,12 @@ module NotificationsHelper
link_to new_notification_text(count), notifications_path
end
end
+
+ def notification_people_link(note)
+ note.actors.collect{ |person| link_to("#{person.name.titlecase}", person_path(person))}.join(" , ").html_safe
+ end
+
+ def peoples_names(note)
+ note.actors.map{|p| p.name}.join(",")
+ end
end
diff --git a/app/helpers/sockets_helper.rb b/app/helpers/sockets_helper.rb
index ac8ef29e055bd579653564d77774380b2478894f..1c6ee13ac87f15d7a3f926c15e98d491a0807c2b 100644
--- a/app/helpers/sockets_helper.rb
+++ b/app/helpers/sockets_helper.rb
@@ -51,7 +51,7 @@ module SocketsHelper
v = render_to_string(:partial => 'comments/comment', :locals => {:comment => object, :person => object.person})
elsif object.is_a? Notification
- v = render_to_string(:partial => 'notifications/popup', :locals => {:note => object, :person => object.actor})
+ v = render_to_string(:partial => 'notifications/popup', :locals => {:note => object, :person => opts[:actor]})
else
raise "#{object.inspect} with class #{object.class} is not actionhashable." unless object.is_a? Retraction
diff --git a/app/models/notification.rb b/app/models/notification.rb
index 9f5bc4eff8ec5f5344e1850e1c36c8bf6927f02e..514d3afeabb44c1d327c56a47746e40e40b4ef41 100644
--- a/app/models/notification.rb
+++ b/app/models/notification.rb
@@ -7,7 +7,8 @@ class Notification < ActiveRecord::Base
include Diaspora::Socketable
belongs_to :recipient, :class_name => 'User'
- belongs_to :actor, :class_name => 'Person'
+ has_many :notification_actors, :dependent => :destroy
+ has_many :actors, :class_name => 'Person', :through => :notification_actors, :source => :person
belongs_to :target, :polymorphic => true
def self.for(recipient, opts={})
@@ -17,27 +18,52 @@ class Notification < ActiveRecord::Base
def self.notify(recipient, target, actor)
if target.respond_to? :notification_type
if action = target.notification_type(recipient, actor)
- n = create(:target => target,
- :action => action,
- :actor => actor,
- :recipient => recipient)
- n.email_the_user if n
- n.socket_to_user(recipient) if n
+ if target.is_a? Comment
+ n = concatenate_or_create(recipient, target.post, actor, action)
+ else
+ n = make_notification(recipient, target, actor, action)
+ end
+ n.email_the_user(target, actor) if n
+ n.socket_to_user(recipient, :actor => actor) if n
n
end
end
end
- def email_the_user
+ def email_the_user(comment, actor)
case self.action
when "new_request"
- self.recipient.mail(Job::MailRequestReceived, self.recipient_id, self.actor_id)
+ self.recipient.mail(Job::MailRequestReceived, self.recipient_id, actor.id)
when "request_accepted"
- self.recipient.mail(Job::MailRequestAcceptance, self.recipient_id, self.actor_id)
+ self.recipient.mail(Job::MailRequestAcceptance, self.recipient_id, actor.id)
when "comment_on_post"
- self.recipient.mail(Job::MailCommentOnPost, self.recipient_id, self.actor_id, target.id)
+ self.recipient.mail(Job::MailCommentOnPost, self.recipient_id, actor.id, comment.id)
when "also_commented"
- self.recipient.mail(Job::MailAlsoCommented, self.recipient_id, self.actor_id, target.id)
+ self.recipient.mail(Job::MailAlsoCommented, self.recipient_id, actor.id, comment.id)
end
end
+
+private
+ def self.concatenate_or_create(recipient, target, actor, action)
+ if n = Notification.where(:target_id => target.id,
+ :action => action,
+ :recipient_id => recipient.id).first
+ unless n.actors.include?(actor)
+ n.actors << actor
+ n.save!
+ end
+ n
+ else
+ make_notification(recipient, target, actor, action)
+ end
+ end
+
+ def self.make_notification(recipient, target, actor, action)
+ n = Notification.new(:target_id => target.id,
+ :action => action,
+ :recipient_id => recipient.id)
+ n.actors << actor
+ n.save!
+ n
+ end
end
diff --git a/app/models/notification_actor.rb b/app/models/notification_actor.rb
new file mode 100644
index 0000000000000000000000000000000000000000..dc788cac36f5bd09850a6b74d85d68a103f9de34
--- /dev/null
+++ b/app/models/notification_actor.rb
@@ -0,0 +1,10 @@
+# Copyright (c) 2010, Diaspora Inc. This file is
+# licensed under the Affero General Public License version 3 or later. See
+# the COPYRIGHT file.
+
+class NotificationActor < ActiveRecord::Base
+
+ belongs_to :notification
+ belongs_to :person
+
+end
diff --git a/app/models/person.rb b/app/models/person.rb
index f502e478c4c21ac0267a55c1d216837943161979..66b5ca45986dc04bb035e80e2c64e92ab80c472e 100644
--- a/app/models/person.rb
+++ b/app/models/person.rb
@@ -29,6 +29,9 @@ class Person < ActiveRecord::Base
belongs_to :owner, :class_name => 'User'
+ has_many :notification_actors
+ has_many :notifications, :through => :notification_actors
+
before_destroy :remove_all_traces
before_validation :clean_url
@@ -183,6 +186,6 @@ class Person < ActiveRecord::Base
Post.where(:person_id => id).delete_all
Comment.where(:person_id => id).delete_all
Contact.where(:person_id => id).delete_all
- Notification.where(:actor_id => id).delete_all
+ Notification.joins(:notification_actors).where(:notification_actors => {:person_id => self.id}).all.each{ |n| n.destroy}
end
end
diff --git a/app/views/notifications/_popup.haml b/app/views/notifications/_popup.haml
index 8293a4af904bd17519bd423bc40b083d1d82c5dd..5f4a17a6df6db1b28bd53c140c179d570c4c5b52 100644
--- a/app/views/notifications/_popup.haml
+++ b/app/views/notifications/_popup.haml
@@ -2,5 +2,5 @@
-# licensed under the Affero General Public License version 3 or later. See
-# the COPYRIGHT file.
-= link_to "#{person.name.titleize}", person_path(person)
+= link_to "#{person.name.titleize}", person_path(person.id)
= object_link(note)
diff --git a/app/views/notifications/index.html.haml b/app/views/notifications/index.html.haml
index 3b5d52bada4c5976697f845e26e63ab76b717ded..f709cb457e6abef3e4eefcd54c592a61b8c8f0e0 100644
--- a/app/views/notifications/index.html.haml
+++ b/app/views/notifications/index.html.haml
@@ -37,8 +37,7 @@
%h2
= t('.notifications')
.span-13.last.left
- .button.mark_all_read
- = link_to t('.mark_all_as_read'), "#"
+ = link_to t('.mark_all_as_read'), "#", :class => "button mark_all_read"
.span-24.last
%ul.stream.notifications
@@ -49,7 +48,7 @@
- notes.each do |note|
.stream_element{:data=>{:guid => note.id}, :class => "#{note.unread ? 'unread' : ''}"}
%span.from
- = link_to "#{note.actor.name.titleize}", person_path(note.actor)
+ = notification_people_link(note)
= object_link(note)
%span.time= timeago(note.created_at)
diff --git a/app/views/shared/_notification.haml b/app/views/shared/_notification.haml
index cb68338c559dd687e035c51ebe5e8a3c022ea1cb..5a6d7699edff8321780d08fdb04e71392327c451 100644
--- a/app/views/shared/_notification.haml
+++ b/app/views/shared/_notification.haml
@@ -2,7 +2,7 @@
-# licensed under the Affero General Public License version 3 or later. See
-# the COPYRIGHT file.
-= link_to t('.new', :type => object.class.to_s, :from => object.person.name), object_path(object.post)
+= link_to t('.new', :type => object.class.to_s, :from => peoples_names(note)), object_path(object.post)
= link_to "#{note.actor.name.titelize}", person_path(note.actor)
diff --git a/db/migrate/20110130072907_notification_multiple_people.rb b/db/migrate/20110130072907_notification_multiple_people.rb
new file mode 100644
index 0000000000000000000000000000000000000000..f0322a96dfa4eeae789dff08859edc9d2fffd380
--- /dev/null
+++ b/db/migrate/20110130072907_notification_multiple_people.rb
@@ -0,0 +1,71 @@
+class NotificationMultiplePeople < ActiveRecord::Migration
+ def self.up
+ create_table :notification_actors do |t|
+ t.integer :notification_id
+ t.integer :person_id
+ t.timestamps
+ end
+
+ add_index :notification_actors, :notification_id
+ add_index :notification_actors, [:notification_id, :person_id] , :unique => true
+ add_index :notification_actors, :person_id ## if i am not mistaken we don't need this one because we won't query person.notifications
+
+ #make the notification actors table
+ execute "INSERT INTO notification_actors (notification_id, person_id) " +
+ " SELECT id , actor_id " +
+ " FROM notifications"
+
+ #update the notifications to reference the post
+ execute "UPDATE notifications, comments " +
+ "SET notifications.target_id = comments.post_id, " +
+ "target_type = 'Post' " +
+ "WHERE (notifications.target_id = comments.id " +
+ "AND (notifications.action = 'comment_on_post' " +
+ "OR notifications.action = 'also_commented'))"
+
+ #select all the notifications to keep
+ execute "CREATE TEMPORARY TABLE keep_table " +
+ "(SELECT id as keep_id, actor_id , target_type , target_id , recipient_id , action " +
+ "FROM notifications WHERE action = 'comment_on_post' OR action = 'also_commented' " +
+ "GROUP BY target_type , target_id , recipient_id , action) "
+
+ #get a table of with ids of the notifications that need to be deleted and with the ones that need
+ #to replace them
+ execute "CREATE TEMPORARY TABLE keep_delete " +
+ "( SELECT n1.keep_id, n2.id as delete_id, " +
+ "n2.actor_id, n1.target_type, n1.target_id, n1.recipient_id, n1.action " +
+ "FROM keep_table n1, notifications n2 " +
+ "WHERE n1.keep_id != n2.id " +
+ "AND n1.actor_id != n2.actor_id "+
+ "AND n1.target_type = n2.target_type AND n1.target_id = n2.target_id " +
+ "AND n1.recipient_id = n2.recipient_id AND n1.action = n2.action " +
+ "AND (n1.action = 'comment_on_post' OR n1.action = 'also_commented') "+
+ "GROUP BY n2.actor_id , n2.target_type , n2.target_id , n2.recipient_id , n2.action)"
+
+ #have the notifications actors reference the notifications that need to be kept
+ execute "UPDATE notification_actors, keep_delete "+
+ "SET notification_actors.notification_id = keep_delete.keep_id "+
+ "WHERE notification_actors.notification_id = keep_delete.delete_id"
+
+ #delete all the notifications that need to be deleted
+ execute "DELETE notifications.* " +
+ "FROM notifications, keep_delete " +
+ "WHERE notifications.id != keep_delete.keep_id AND "+
+ "notifications.target_type = keep_delete.target_type AND "+
+ "notifications.target_id = keep_delete.target_id AND "+
+ "notifications.recipient_id = keep_delete.recipient_id AND "+
+ "notifications.action = keep_delete.action"
+
+
+ remove_column :notifications, :actor_id
+ remove_column :notifications, :mongo_id
+ end
+
+ def self.down
+ remove_index :notification_actors, :notification_id
+ remove_index :notification_actors, [:notification_id, :person_id]
+ remove_index :notification_actors, :person_id
+
+ drop_table :notification_actors
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index b48fb9b95eb082eaa9ff4f9b16930687607fc4d2..78e1b15fe9106be472efe77065662fec3bd997d0 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
#
# It's strongly recommended to check this file into your version control system.
-ActiveRecord::Schema.define(:version => 20110127000953) do
+ActiveRecord::Schema.define(:version => 20110130072907) do
create_table "aspect_memberships", :force => true do |t|
t.integer "aspect_id", :null => false
@@ -293,11 +293,21 @@ ActiveRecord::Schema.define(:version => 20110127000953) do
add_index "mongo_users", ["mongo_id"], :name => "index_mongo_users_on_mongo_id", :unique => true
+ create_table "notification_actors", :force => true do |t|
+ t.integer "notification_id"
+ t.integer "person_id"
+ t.datetime "created_at"
+ t.datetime "updated_at"
+ end
+
+ add_index "notification_actors", ["notification_id", "person_id"], :name => "index_notification_actors_on_notification_id_and_person_id", :unique => true
+ add_index "notification_actors", ["notification_id"], :name => "index_notification_actors_on_notification_id"
+ add_index "notification_actors", ["person_id"], :name => "index_notification_actors_on_person_id"
+
create_table "notifications", :force => true do |t|
t.string "target_type"
t.integer "target_id"
t.integer "recipient_id", :null => false
- t.integer "actor_id", :null => false
t.string "action", :null => false
t.boolean "unread", :default => true, :null => false
t.datetime "created_at"
diff --git a/spec/factories.rb b/spec/factories.rb
index 86a449acc44a967d60694e76c613c865ce58f152..798077740e6b1c492356ef2afb64d3a6a2d26df7 100644
--- a/spec/factories.rb
+++ b/spec/factories.rb
@@ -91,10 +91,11 @@ end
Factory.define(:notification) do |n|
n.association :recipient, :factory => :user
- n.association :actor, :factory => :person
n.association :target, :factory => :comment
+
n.after_build do |note|
- note.action = note.target.notification_type(note.recipient, note.actor)
+ note.actors << Factory.build( :person )
+ note.action = note.target.notification_type(note.recipient, note.actors.first)
end
end
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 1e15ce06ebded1102988aa755f13a1c2585f3f48..aaf1837c01df92c4e6b4f9e6de015e451fd820d9 100644
--- a/spec/models/notification_spec.rb
+++ b/spec/models/notification_spec.rb
@@ -14,21 +14,15 @@ describe Notification do
@opts = {:target_id => @sm.id,
:target_type => @sm.class.name,
:action => "comment_on_post",
- :actor_id => @person.id,
+ :actors => [@person],
:recipient_id => @user.id}
@note = Notification.new(@opts)
+ @note.actors =[ @person]
end
- it 'contains a type' do
- @note.target_type.should == StatusMessage.name
- end
-
- it 'contains a target_id' do
- @note.target_id.should == @sm.id
- end
-
- it 'contains a person_id' do
- @note.actor_id == @person.id
+ it 'destoys the associated notification_actor' do
+ @note.save
+ lambda{@note.destroy}.should change(NotificationActor, :count).by(-1)
end
describe '.for' do
@@ -47,7 +41,7 @@ describe Notification do
describe '.notify' do
it 'does not call Notification.create if the object does not have a notification_type' do
- Notification.should_not_receive(:create)
+ Notification.should_not_receive(:make_notificatin)
Notification.notify(@user, @sm, @person)
end
context 'with a request' do
@@ -55,7 +49,7 @@ describe Notification do
@request = Request.diaspora_initialize(:from => @user.person, :to => @user2.person, :into => @aspect)
end
it 'calls Notification.create if the object has a notification_type' do
- Notification.should_receive(:create).once
+ Notification.should_receive(:make_notification).once
Notification.notify(@user, @request, @person)
end
@@ -63,11 +57,11 @@ describe Notification do
opts = {:target_id => @request.id,
:target_type => "Request",
:action => @request.notification_type(@user, @person),
- :actor_id => @person.id,
+ :actors => [@person],
:recipient_id => @user.id}
n = Notification.create(opts)
- Notification.stub!(:create).and_return n
+ Notification.stub!(:make_notification).and_return n
n.should_receive(:socket_to_user).once
Notification.notify(@user, @request, @person)
@@ -77,14 +71,33 @@ describe Notification do
it 'calls mail' do
opts = {
:action => "new_request",
- :actor_id => @person.id,
+ :actors => [@person],
:recipient_id => @user.id}
n = Notification.new(opts)
n.stub!(:recipient).and_return @user
@user.should_receive(:mail)
- n.email_the_user
+ n.email_the_user("mock", @person)
+ end
+ end
+
+ context 'multiple people' do
+
+ before do
+ @user3 = bob
+ @sm = @user3.post(:status_message, :message => "comment!", :to => :all)
+ Postzord::Receiver.new(@user3, :person => @user2.person, :object => @user2.comment("hey", :on => @sm)).receive_object
+ Postzord::Receiver.new(@user3, :person => @user.person, :object => @user.comment("hey", :on => @sm)).receive_object
+ end
+
+ it "updates the notification with a more people if one already exists" do
+ Notification.where(:recipient_id => @user3.id,:target_id => @sm.id).first.actors.count.should == 2
+ end
+
+ it 'handles double comments from the same person without raising' do
+ Postzord::Receiver.new(@user3, :person => @user2.person, :object => @user2.comment("hey", :on => @sm)).receive_object
+ Notification.where(:recipient_id => @user3.id,:target_id => @sm.id).first.actors.count.should == 2
end
end
end
diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb
index 2756cd61811a000d0f6fa3352158ede672b9baa1..0a80469f415efda0205edc117a87c6ba4c6964cb 100644
--- a/spec/models/person_spec.rb
+++ b/spec/models/person_spec.rb
@@ -106,7 +106,7 @@ describe Person do
end
it "deletes all notifications from a person's actions" do
- note = Factory(:notification, :actor => @deleter, :recipient => @user)
+ note = Factory(:notification, :actors => [@deleter], :recipient => @user)
@deleter.destroy
Notification.where(:id => note.id).first.should be_nil
end