From f48d7a0826811a2ec4f43c3ae5bd0e2c448b0bdb Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sun, 9 Jan 2011 22:07:32 -0800 Subject: [PATCH 01/18] wip --- app/models/notification.rb | 2 +- app/models/notification_actor.rb | 10 ++++++++ app/models/person.rb | 2 ++ ...0110023610_notification_multiple_people.rb | 25 +++++++++++++++++++ db/schema.rb | 11 ++++++++ 5 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 app/models/notification_actor.rb create mode 100644 db/migrate/20110110023610_notification_multiple_people.rb diff --git a/app/models/notification.rb b/app/models/notification.rb index 9f5bc4eff..dfe54a962 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -7,7 +7,7 @@ class Notification < ActiveRecord::Base include Diaspora::Socketable belongs_to :recipient, :class_name => 'User' - belongs_to :actor, :class_name => 'Person' + has_many :actors, :class_name => 'Person', :through => :notification_actors belongs_to :target, :polymorphic => true def self.for(recipient, opts={}) diff --git a/app/models/notification_actor.rb b/app/models/notification_actor.rb new file mode 100644 index 000000000..dc788cac3 --- /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 f502e478c..4f99c3c5d 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -29,6 +29,8 @@ class Person < ActiveRecord::Base belongs_to :owner, :class_name => 'User' + has_many :notifications, :through => :notification_actors + before_destroy :remove_all_traces before_validation :clean_url diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb new file mode 100644 index 000000000..42f6c0382 --- /dev/null +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -0,0 +1,25 @@ +class NotificationMultiplePeople < ActiveRecord::Migration + def self.up + create_table :notification_actors do |t| + t.integer :notifications_id + t.integer :person_id + t.timestamps + end + + add_index :notification_actors, :notifications_id + add_index :notification_actors, [:notifications_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 + + execute "INSERT INTO notification_actors (id, person_id) " + + " SELECT id , actor_id " + + " FROM notifications" + end + + def self.down + remove_index :notification_actors, :notifications_id + remove_index :notification_actors, [:notifications_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 b48fb9b95..59135130a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -293,6 +293,17 @@ 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 "notifications_id" + t.integer "person_id" + t.datetime "created_at" + t.datetime "updated_at" + end + + add_index "notification_actors", ["notifications_id", "person_id"], :name => "index_notification_actors_on_notifications_id_and_person_id", :unique => true + add_index "notification_actors", ["notifications_id"], :name => "index_notification_actors_on_notifications_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" From 9d4d0cecdbe91d7aa82a1baf010c864f2d552ebe Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Mon, 10 Jan 2011 13:22:34 -0800 Subject: [PATCH 02/18] wip --- app/models/notification.rb | 3 +- app/models/person.rb | 1 + ...0110023610_notification_multiple_people.rb | 34 +++++++++++++++---- db/schema.rb | 16 ++------- 4 files changed, 34 insertions(+), 20 deletions(-) diff --git a/app/models/notification.rb b/app/models/notification.rb index dfe54a962..e07ec0bf5 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' - has_many :actors, :class_name => 'Person', :through => :notification_actors + has_many :notification_actors + has_many :actors, :class_name => 'Person', :through => :notification_actors, :source => :person belongs_to :target, :polymorphic => true def self.for(recipient, opts={}) diff --git a/app/models/person.rb b/app/models/person.rb index 4f99c3c5d..d75191d04 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -29,6 +29,7 @@ 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 diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb index 42f6c0382..ed52bbba6 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -1,23 +1,45 @@ class NotificationMultiplePeople < ActiveRecord::Migration def self.up create_table :notification_actors do |t| - t.integer :notifications_id + t.integer :notification_id t.integer :person_id t.timestamps end - add_index :notification_actors, :notifications_id - add_index :notification_actors, [:notifications_id, :person_id] , :unique => true + 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 - execute "INSERT INTO notification_actors (id, person_id) " + + execute "INSERT INTO notification_actors (notification_id, person_id) " + " SELECT id , actor_id " + " FROM notifications" + + #TODO in sql + #bump up target to status message id if comment_on_post, also_commented + ['comment_on_post', 'also_commented'].each do |type| + + Notification.where(:type => 'comment_on_post').all.each{|n| + n.target_id => Comment.find(n.target_id).post} + + #for each user + all = Notification.where(:type => 'comment_on_post', :user => user).all + first = all.first + all[1..all.length-1].each{ |a| + first << a.notification_actors + a.delete + } + + end + + # all notification of same type with the same + + + remove_column :notification, :actor_id end def self.down - remove_index :notification_actors, :notifications_id - remove_index :notification_actors, [:notifications_id, :person_id] + remove_index :notification_actors, :notification_id + remove_index :notification_actors, [:notification_id, :person_id] remove_index :notification_actors, :person_id drop_table :notification_actors diff --git a/db/schema.rb b/db/schema.rb index 59135130a..2fa0551f1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -47,7 +47,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.text "youtube_titles" t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" end add_index "comments", ["guid"], :name => "index_comments_on_guid", :unique => true @@ -61,7 +60,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.boolean "pending", :default => true, :null => false t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" end add_index "contacts", ["mongo_id"], :name => "index_contacts_on_mongo_id" @@ -86,7 +84,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.integer "aspect_id" t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" end add_index "invitations", ["aspect_id"], :name => "index_invitations_on_aspect_id" @@ -294,14 +291,14 @@ 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 "notifications_id" + t.integer "notification_id" t.integer "person_id" t.datetime "created_at" t.datetime "updated_at" end - add_index "notification_actors", ["notifications_id", "person_id"], :name => "index_notification_actors_on_notifications_id_and_person_id", :unique => true - add_index "notification_actors", ["notifications_id"], :name => "index_notification_actors_on_notifications_id" + 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| @@ -313,7 +310,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.boolean "unread", :default => true, :null => false t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" end add_index "notifications", ["mongo_id"], :name => "index_notifications_on_mongo_id" @@ -329,7 +325,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.integer "owner_id" t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" end add_index "people", ["diaspora_handle"], :name => "index_people_on_diaspora_handle", :unique => true @@ -365,7 +360,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.text "youtube_titles" t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" end add_index "posts", ["guid"], :name => "index_posts_on_guid" @@ -390,7 +384,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.integer "person_id", :null => false t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" end add_index "profiles", ["first_name", "last_name", "searchable"], :name => "index_profiles_on_first_name_and_last_name_and_searchable" @@ -405,7 +398,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.integer "aspect_id" t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" end add_index "requests", ["mongo_id"], :name => "index_requests_on_mongo_id" @@ -422,8 +414,6 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.string "nickname" t.datetime "created_at" t.datetime "updated_at" - t.string "mongo_id" - t.string "user_mongo_id" end add_index "services", ["mongo_id"], :name => "index_services_on_mongo_id" From 74e0d03b8fae8da9c9a69ae09364d41a2aba3580 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Mon, 10 Jan 2011 16:46:23 -0800 Subject: [PATCH 03/18] wip' --- .../20110110023610_notification_multiple_people.rb | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb index ed52bbba6..0dcfcd03b 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -15,10 +15,22 @@ class NotificationMultiplePeople < ActiveRecord::Migration " FROM notifications" #TODO in sql + # 1) set target type + # 2) update the target_id from the comment to comment.post_id + execute "UPDATE notifications "+ + " SET target_id = comment.post_id, target_type = 'post' " + + " FROM notifications " + + " INNER JOIN comments " + + " WHERE notifications.target_id = comments.id " + + #bump up target to status message id if comment_on_post, also_commented ['comment_on_post', 'also_commented'].each do |type| - Notification.where(:type => 'comment_on_post').all.each{|n| + Notification.joins(:target).where(:action => "comment_on_post").update_all(:target => target) + + + Notification.where(:action => 'comment_on_post').all.each{|n| n.target_id => Comment.find(n.target_id).post} #for each user From d7f06b9e6e6e4d65c909c7c204841e5ab7ce3a79 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Fri, 28 Jan 2011 22:33:19 -0800 Subject: [PATCH 04/18] removed commented out non working migration code for now --- ...0110023610_notification_multiple_people.rb | 51 ++++++++++--------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb index 0dcfcd03b..68e277088 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -1,7 +1,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration def self.up create_table :notification_actors do |t| - t.integer :notification_id + t.integer :notification_id t.integer :person_id t.timestamps end @@ -10,36 +10,39 @@ class NotificationMultiplePeople < ActiveRecord::Migration 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 - execute "INSERT INTO notification_actors (notification_id, person_id) " + - " SELECT id , actor_id " + - " FROM notifications" + #execute "INSERT INTO notification_actors (notification_id, person_id) " + + #" SELECT id , actor_id " + + #" FROM notifications" + + + ##TODO in sql + ## 1) set target type + ## 2) update the target_id from the comment to comment.post_id + #execute "UPDATE notifications "+ + #" SET target_id = comment.post_id, target_type = 'post' " + + #" FROM notifications " + + #" INNER JOIN comments " + + #" WHERE notifications.target_id = comments.id AND notifications.action = 'comment_on_post' " + - #TODO in sql - # 1) set target type - # 2) update the target_id from the comment to comment.post_id - execute "UPDATE notifications "+ - " SET target_id = comment.post_id, target_type = 'post' " + - " FROM notifications " + - " INNER JOIN comments " + - " WHERE notifications.target_id = comments.id " - #bump up target to status message id if comment_on_post, also_commented - ['comment_on_post', 'also_commented'].each do |type| + ##bump up target to status message id if comment_on_post, also_commented + #['comment_on_post', 'also_commented'].each do |type| - Notification.joins(:target).where(:action => "comment_on_post").update_all(:target => target) + #Notification.joins(:target).where(:action => "comment_on_post").update_all(:target => target) - Notification.where(:action => 'comment_on_post').all.each{|n| - n.target_id => Comment.find(n.target_id).post} + #Notification.where(:action => 'comment_on_post').all.each{|n| + #n.target_id => Comment.find(n.target_id).post} - #for each user - all = Notification.where(:type => 'comment_on_post', :user => user).all - first = all.first - all[1..all.length-1].each{ |a| - first << a.notification_actors - a.delete - } + ##for each user + #all = Notification.where(:type => 'comment_on_post', :user => user).all + #first = all.first + #all[1..all.length-1].each{ |a| + #first << a.notification_actors + #a.delete + #} end From 8c77f65cdb4a2bf795365642ee562b742e3438e0 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sat, 29 Jan 2011 02:01:40 -0800 Subject: [PATCH 05/18] notes for later --- ...110110023610_notification_multiple_people.rb | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb index 68e277088..490b54de7 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -10,21 +10,20 @@ class NotificationMultiplePeople < ActiveRecord::Migration 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 + + #create table notification_actors (id MEDIUMINT NOT NULL AUTO_INCREMENT, notification_id integer, person_id integer, created_at datetime, updated_at datetime); + + #execute "INSERT INTO notification_actors (notification_id, person_id) " + #" SELECT id , actor_id " + #" FROM notifications" + #UPDATE notifications, comments SET notifications.target_id = comments.post_id, target_type = 'post' WHERE notifications.target_id = comments.id AND (notifications.type = 'comment_on_post' OR notifications.type = 'also_commented_on_post'); + - ##TODO in sql - ## 1) set target type - ## 2) update the target_id from the comment to comment.post_id - #execute "UPDATE notifications "+ - #" SET target_id = comment.post_id, target_type = 'post' " + - #" FROM notifications " + - #" INNER JOIN comments " + - #" WHERE notifications.target_id = comments.id AND notifications.action = 'comment_on_post' " - + + ##TODO in sql ##bump up target to status message id if comment_on_post, also_commented From 05cab33dad20924a51985b69dc4367629542c010 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sat, 29 Jan 2011 18:36:04 -0800 Subject: [PATCH 06/18] notification migration need to try it out --- ...0110023610_notification_multiple_people.rb | 65 ++++++++++--------- 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb index 490b54de7..258a86c56 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -10,42 +10,45 @@ class NotificationMultiplePeople < ActiveRecord::Migration 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 - - #create table notification_actors (id MEDIUMINT NOT NULL AUTO_INCREMENT, notification_id integer, person_id integer, created_at datetime, updated_at datetime); - - - #execute "INSERT INTO notification_actors (notification_id, person_id) " + - #" SELECT id , actor_id " + - #" FROM notifications" + #make the notification actors table + execute "INSERT INTO notification_actors (notification_id, person_id) " + + " SELECT id , actor_id " + + " FROM notifications" - #UPDATE notifications, comments SET notifications.target_id = comments.post_id, target_type = 'post' WHERE notifications.target_id = comments.id AND (notifications.type = 'comment_on_post' OR notifications.type = 'also_commented_on_post'); - + #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, target_type , target_id , recipient_id , action as keep_id " + + "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, " + + "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.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'))" - ##TODO in sql + #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" - - ##bump up target to status message id if comment_on_post, also_commented - #['comment_on_post', 'also_commented'].each do |type| - - #Notification.joins(:target).where(:action => "comment_on_post").update_all(:target => target) - - - #Notification.where(:action => 'comment_on_post').all.each{|n| - #n.target_id => Comment.find(n.target_id).post} - - ##for each user - #all = Notification.where(:type => 'comment_on_post', :user => user).all - #first = all.first - #all[1..all.length-1].each{ |a| - #first << a.notification_actors - #a.delete - #} - - end - - # all notification of same type with the same + #delete all the notifications that need to be deleted + execute "DELETE notifications.*" + + "FROM notifications, keep_delete" + + "WHERE notifications.id = keep_delete.delete_id" remove_column :notification, :actor_id From 5d3e08227e2cf800e2a3279ee908cfb8ba48bab7 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sat, 29 Jan 2011 18:46:05 -0800 Subject: [PATCH 07/18] whitespace --- ...0110023610_notification_multiple_people.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb index 258a86c56..7a86259f1 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -16,7 +16,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration " FROM notifications" #update the notifications to reference the post - execute "UPDATE notifications, comments" + + execute "UPDATE notifications, comments " + "SET notifications.target_id = comments.post_id, " + "target_type = 'Post' " + "WHERE (notifications.target_id = comments.id " + @@ -24,20 +24,20 @@ class NotificationMultiplePeople < ActiveRecord::Migration "OR notifications.action = 'also_commented'))" #select all the notifications to keep - execute "CREATE TEMPORARY TABLE keep_table" + + execute "CREATE TEMPORARY TABLE keep_table " + "(SELECT id as keep_id, target_type , target_id , recipient_id , action as keep_id " + - "FROM notifications WHERE action = 'comment_on_post' OR action = 'also_commented'" + - "GROUP BY 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, " + - "n1.target_type, n1.target_id, n1.recipient_id, n1.action" + - "FROM keep_table n1, notifications n2" + + "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.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.recipient_id = n2.recipient_id AND n1.action = n2.action " "AND (n1.action = 'comment_on_post' OR n1.action = 'also_commented'))" #have the notifications actors reference the notifications that need to be kept @@ -46,9 +46,9 @@ class NotificationMultiplePeople < ActiveRecord::Migration "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.delete_id" + execute "DELETE notifications.* " + + "FROM notifications, keep_delete " + + "WHERE notifications.id = keep_delete.delete_id " remove_column :notification, :actor_id From 2b4d27d2cffaf267b5d7e57f971aae92d853da58 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sat, 29 Jan 2011 18:53:13 -0800 Subject: [PATCH 08/18] another typo --- db/migrate/20110110023610_notification_multiple_people.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb index 7a86259f1..4b3f1c944 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -25,7 +25,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration #select all the notifications to keep execute "CREATE TEMPORARY TABLE keep_table " + - "(SELECT id as keep_id, target_type , target_id , recipient_id , action as keep_id " + + "(SELECT id as keep_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) " From 3119fc660f7a5a810d3e64447b5432ad30a9c5f1 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sat, 29 Jan 2011 20:56:21 -0800 Subject: [PATCH 09/18] another space --- db/migrate/20110110023610_notification_multiple_people.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20110110023610_notification_multiple_people.rb b/db/migrate/20110110023610_notification_multiple_people.rb index 4b3f1c944..f1f670c2a 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110110023610_notification_multiple_people.rb @@ -25,7 +25,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration #select all the notifications to keep execute "CREATE TEMPORARY TABLE keep_table " + - "(SELECT id as keep_id, target_type , target_id , recipient_id , action" + + "(SELECT id as keep_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) " From 17a2c9e311b1dfa53a527a3a3c3a4d37b185f062 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sat, 29 Jan 2011 23:34:11 -0800 Subject: [PATCH 10/18] moving over some code migration successful --- app/helpers/notifications_helper.rb | 20 ++++++--- app/helpers/sockets_helper.rb | 2 +- app/models/notification.rb | 45 ++++++++++++++----- app/models/person.rb | 2 +- app/views/notifications/_popup.haml | 2 +- app/views/notifications/index.html.haml | 2 +- app/views/shared/_notification.haml | 2 +- ...130072907_notification_multiple_people.rb} | 6 +-- db/schema.rb | 13 +++++- spec/models/notification_spec.rb | 22 ++++++--- spec/models/person_spec.rb | 2 +- 11 files changed, 84 insertions(+), 34 deletions(-) rename db/migrate/{20110110023610_notification_multiple_people.rb => 20110130072907_notification_multiple_people.rb} (97%) diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index e5c739667..5c97f7dd1 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 ac8ef29e0..1c6ee13ac 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 e07ec0bf5..c34a1b3e9 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -18,27 +18,50 @@ 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(actor) if n + n.socket_to_uid(user, :actor => person) 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 + n.actors << NotificationActor.new(:actor => actor) + n.save! + n + else + n = 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 << NotificationActor.new(:actor => actor) + n.save! + n + end end diff --git a/app/models/person.rb b/app/models/person.rb index d75191d04..01d0474fb 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -186,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 => {:actor_id => self}).delete_all end end diff --git a/app/views/notifications/_popup.haml b/app/views/notifications/_popup.haml index 8293a4af9..5f4a17a6d 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 3b5d52bad..c7163b635 100644 --- a/app/views/notifications/index.html.haml +++ b/app/views/notifications/index.html.haml @@ -49,7 +49,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 cb68338c5..5a6d7699e 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/20110110023610_notification_multiple_people.rb b/db/migrate/20110130072907_notification_multiple_people.rb similarity index 97% rename from db/migrate/20110110023610_notification_multiple_people.rb rename to db/migrate/20110130072907_notification_multiple_people.rb index f1f670c2a..11e42c434 100644 --- a/db/migrate/20110110023610_notification_multiple_people.rb +++ b/db/migrate/20110130072907_notification_multiple_people.rb @@ -36,8 +36,8 @@ class NotificationMultiplePeople < ActiveRecord::Migration "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.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.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'))" #have the notifications actors reference the notifications that need to be kept @@ -51,7 +51,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration "WHERE notifications.id = keep_delete.delete_id " - remove_column :notification, :actor_id + remove_column :notifications, :actor_id end def self.down diff --git a/db/schema.rb b/db/schema.rb index 2fa0551f1..78e1b15fe 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 @@ -47,6 +47,7 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.text "youtube_titles" t.datetime "created_at" t.datetime "updated_at" + t.string "mongo_id" end add_index "comments", ["guid"], :name => "index_comments_on_guid", :unique => true @@ -60,6 +61,7 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.boolean "pending", :default => true, :null => false t.datetime "created_at" t.datetime "updated_at" + t.string "mongo_id" end add_index "contacts", ["mongo_id"], :name => "index_contacts_on_mongo_id" @@ -84,6 +86,7 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.integer "aspect_id" t.datetime "created_at" t.datetime "updated_at" + t.string "mongo_id" end add_index "invitations", ["aspect_id"], :name => "index_invitations_on_aspect_id" @@ -305,11 +308,11 @@ ActiveRecord::Schema.define(:version => 20110127000953) do 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" t.datetime "updated_at" + t.string "mongo_id" end add_index "notifications", ["mongo_id"], :name => "index_notifications_on_mongo_id" @@ -325,6 +328,7 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.integer "owner_id" t.datetime "created_at" t.datetime "updated_at" + t.string "mongo_id" end add_index "people", ["diaspora_handle"], :name => "index_people_on_diaspora_handle", :unique => true @@ -360,6 +364,7 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.text "youtube_titles" t.datetime "created_at" t.datetime "updated_at" + t.string "mongo_id" end add_index "posts", ["guid"], :name => "index_posts_on_guid" @@ -384,6 +389,7 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.integer "person_id", :null => false t.datetime "created_at" t.datetime "updated_at" + t.string "mongo_id" end add_index "profiles", ["first_name", "last_name", "searchable"], :name => "index_profiles_on_first_name_and_last_name_and_searchable" @@ -398,6 +404,7 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.integer "aspect_id" t.datetime "created_at" t.datetime "updated_at" + t.string "mongo_id" end add_index "requests", ["mongo_id"], :name => "index_requests_on_mongo_id" @@ -414,6 +421,8 @@ ActiveRecord::Schema.define(:version => 20110127000953) do t.string "nickname" t.datetime "created_at" t.datetime "updated_at" + t.string "mongo_id" + t.string "user_mongo_id" end add_index "services", ["mongo_id"], :name => "index_services_on_mongo_id" diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 1e15ce06e..61af92feb 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -14,9 +14,10 @@ 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 @@ -27,8 +28,9 @@ describe Notification do @note.target_id.should == @sm.id end - it 'contains a person_id' do - @note.actor_id == @person.id + + it 'has many people' do + @note.associations[:people].type.should == :many end describe '.for' do @@ -47,7 +49,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 +57,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 @@ -84,9 +86,17 @@ describe Notification do n.stub!(:recipient).and_return @user @user.should_receive(:mail) - n.email_the_user + n.email_the_user("mock", @person) end end + + it "updates the notification with a more people if one already exists" do + @user3 = bob + sm = @user3.post(:status_message, :message => "comment!", :to => :all) + @user3.receive_object(@user2.reload.comment("hey", :on => sm), @user2.person) + @user3.receive_object(@user.reload.comment("way", :on => sm), @user.person) + Notification.where(:user_id => @user.id,:target_id => sm.id).first.people.count.should == 2 + end end end end diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 2756cd618..0a80469f4 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 From 17dde3a7b93afa7b50f3c5f7c2d2de562755d1d3 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sat, 29 Jan 2011 23:58:48 -0800 Subject: [PATCH 11/18] farther along towards getting the specs to pass --- app/models/notification.rb | 8 ++++---- app/models/person.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/notification.rb b/app/models/notification.rb index c34a1b3e9..1e887eb06 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -23,8 +23,8 @@ class Notification < ActiveRecord::Base else n = make_notification(recipient, target, actor, action) end - n.email_the_user(actor) if n - n.socket_to_uid(user, :actor => person) if n + n.email_the_user(target, actor) if n + n.socket_to_user(recipient, :actor => actor) if n n end end @@ -48,7 +48,7 @@ private if n = Notification.where(:target_id => target.id, :action => action, :recipient_id => recipient.id).first - n.actors << NotificationActor.new(:actor => actor) + n.actors << actor n.save! n else @@ -60,7 +60,7 @@ private n = Notification.new(:target_id => target.id, :action => action, :recipient_id => recipient.id) - n.actors << NotificationActor.new(:actor => actor) + n.actors << actor n.save! n end diff --git a/app/models/person.rb b/app/models/person.rb index 01d0474fb..742f8b4e4 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -186,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.joins(:notification_actors).where(:notification_actors => {:actor_id => self}).delete_all + Notification.where(:actors => self).delete_all end end From a4988a28b4fea9b1e750ab7b85e3c9b53e9e696b Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sun, 30 Jan 2011 10:00:03 -0800 Subject: [PATCH 12/18] WIP --- app/models/person.rb | 2 +- spec/factories.rb | 2 +- spec/models/notification_spec.rb | 5 +++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/person.rb b/app/models/person.rb index 742f8b4e4..66b5ca459 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -186,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(:actors => self).delete_all + Notification.joins(:notification_actors).where(:notification_actors => {:person_id => self.id}).all.each{ |n| n.destroy} end end diff --git a/spec/factories.rb b/spec/factories.rb index 86a449acc..cf3fd93c6 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -91,7 +91,7 @@ end Factory.define(:notification) do |n| n.association :recipient, :factory => :user - n.association :actor, :factory => :person + n.association :actors, :factory => :person n.association :target, :factory => :comment n.after_build do |note| note.action = note.target.notification_type(note.recipient, note.actor) diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 61af92feb..e6b44b516 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -33,6 +33,11 @@ describe Notification do @note.associations[:people].type.should == :many end + it 'destoys the associated notification_actor' do + @note.save + lambda{@note.destroy}.should change(NotificationActors, :count).by(-1) + end + describe '.for' do it 'returns all of a users notifications' do user2 = Factory.create(:user) From cfef9a747e83c4a89a8e37f8d680e63647bd1491 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Sun, 30 Jan 2011 20:10:34 -0800 Subject: [PATCH 13/18] all green --- app/controllers/notifications_controller.rb | 2 +- app/models/notification.rb | 4 +-- app/views/notifications/index.html.haml | 3 +-- spec/factories.rb | 5 ++-- spec/models/notification_spec.rb | 27 ++++++--------------- 5 files changed, 14 insertions(+), 27 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 7856f6edf..0f9b02989 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/models/notification.rb b/app/models/notification.rb index 1e887eb06..efca7def6 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -7,7 +7,7 @@ class Notification < ActiveRecord::Base include Diaspora::Socketable belongs_to :recipient, :class_name => 'User' - has_many :notification_actors + has_many :notification_actors, :dependent => :destroy has_many :actors, :class_name => 'Person', :through => :notification_actors, :source => :person belongs_to :target, :polymorphic => true @@ -48,7 +48,7 @@ private if n = Notification.where(:target_id => target.id, :action => action, :recipient_id => recipient.id).first - n.actors << actor + n.actors << actor unless n.actors.include?(actor) n.save! n else diff --git a/app/views/notifications/index.html.haml b/app/views/notifications/index.html.haml index c7163b635..f709cb457 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 diff --git a/spec/factories.rb b/spec/factories.rb index cf3fd93c6..798077740 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 :actors, :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 e6b44b516..5ede818b4 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -20,22 +20,9 @@ describe Notification do @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 'has many people' do - @note.associations[:people].type.should == :many - end - it 'destoys the associated notification_actor' do @note.save - lambda{@note.destroy}.should change(NotificationActors, :count).by(-1) + lambda{@note.destroy}.should change(NotificationActor, :count).by(-1) end describe '.for' do @@ -70,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) @@ -84,7 +71,7 @@ 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) @@ -98,9 +85,9 @@ describe Notification do it "updates the notification with a more people if one already exists" do @user3 = bob sm = @user3.post(:status_message, :message => "comment!", :to => :all) - @user3.receive_object(@user2.reload.comment("hey", :on => sm), @user2.person) - @user3.receive_object(@user.reload.comment("way", :on => sm), @user.person) - Notification.where(:user_id => @user.id,:target_id => sm.id).first.people.count.should == 2 + 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 + Notification.where(:recipient_id => @user3.id,:target_id => sm.id).first.actors.count.should == 2 end end end From ac50a0119abd58de17ecdadf8c0a7817b16bc380 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Mon, 31 Jan 2011 10:23:26 -0800 Subject: [PATCH 14/18] spec for not raising on already existant actor --- app/models/notification.rb | 8 +++++--- spec/models/notification_spec.rb | 23 +++++++++++++++++------ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/app/models/notification.rb b/app/models/notification.rb index efca7def6..514d3afea 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -48,11 +48,13 @@ private if n = Notification.where(:target_id => target.id, :action => action, :recipient_id => recipient.id).first - n.actors << actor unless n.actors.include?(actor) - n.save! + unless n.actors.include?(actor) + n.actors << actor + n.save! + end n else - n = make_notification(recipient, target, actor, action) + make_notification(recipient, target, actor, action) end end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 5ede818b4..aaf1837c0 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -82,12 +82,23 @@ describe Notification do end end - it "updates the notification with a more people if one already exists" 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 - Notification.where(:recipient_id => @user3.id,:target_id => sm.id).first.actors.count.should == 2 + 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 end From 03e8aa31f4e480312aeb12bd812a5ecb10681b22 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Mon, 31 Jan 2011 11:02:51 -0800 Subject: [PATCH 15/18] fixed notifications migration --- .../20110130072907_notification_multiple_people.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/db/migrate/20110130072907_notification_multiple_people.rb b/db/migrate/20110130072907_notification_multiple_people.rb index 11e42c434..15902c3fb 100644 --- a/db/migrate/20110130072907_notification_multiple_people.rb +++ b/db/migrate/20110130072907_notification_multiple_people.rb @@ -38,7 +38,8 @@ class NotificationMultiplePeople < ActiveRecord::Migration "WHERE n1.keep_id != n2.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'))" + "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 "+ @@ -48,7 +49,11 @@ class NotificationMultiplePeople < ActiveRecord::Migration #delete all the notifications that need to be deleted execute "DELETE notifications.* " + "FROM notifications, keep_delete " + - "WHERE notifications.id = keep_delete.delete_id " + "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 From 7cb23fd803e7d1e7f5f6da87e17650619bc9a7ba Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Mon, 31 Jan 2011 11:27:56 -0800 Subject: [PATCH 16/18] added a select for the actor id to group by it --- db/migrate/20110130072907_notification_multiple_people.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20110130072907_notification_multiple_people.rb b/db/migrate/20110130072907_notification_multiple_people.rb index 15902c3fb..1460d9e81 100644 --- a/db/migrate/20110130072907_notification_multiple_people.rb +++ b/db/migrate/20110130072907_notification_multiple_people.rb @@ -33,7 +33,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration #to replace them execute "CREATE TEMPORARY TABLE keep_delete " + "( SELECT n1.keep_id, n2.id as delete_id, " + - "n1.target_type, n1.target_id, n1.recipient_id, n1.action " + + "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.target_type = n2.target_type AND n1.target_id = n2.target_id " + From 2d078a212a7eb1622bb314d1ed11cc7344677192 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Mon, 31 Jan 2011 11:31:59 -0800 Subject: [PATCH 17/18] remove mongo id from the notifications table --- db/migrate/20110130072907_notification_multiple_people.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/migrate/20110130072907_notification_multiple_people.rb b/db/migrate/20110130072907_notification_multiple_people.rb index 1460d9e81..9e1786f16 100644 --- a/db/migrate/20110130072907_notification_multiple_people.rb +++ b/db/migrate/20110130072907_notification_multiple_people.rb @@ -57,6 +57,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration remove_column :notifications, :actor_id + remove_column :notifications, :mongo_id end def self.down From a789be737a44117f4dd9240af81c3b16f3adcf10 Mon Sep 17 00:00:00 2001 From: zhitomirskiyi Date: Mon, 31 Jan 2011 11:42:49 -0800 Subject: [PATCH 18/18] also making sure that the notification actor id is unique in the keep delete table --- db/migrate/20110130072907_notification_multiple_people.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/db/migrate/20110130072907_notification_multiple_people.rb b/db/migrate/20110130072907_notification_multiple_people.rb index 9e1786f16..f0322a96d 100644 --- a/db/migrate/20110130072907_notification_multiple_people.rb +++ b/db/migrate/20110130072907_notification_multiple_people.rb @@ -25,7 +25,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration #select all the notifications to keep execute "CREATE TEMPORARY TABLE keep_table " + - "(SELECT id as keep_id, target_type , target_id , recipient_id , action " + + "(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) " @@ -36,6 +36,7 @@ class NotificationMultiplePeople < ActiveRecord::Migration "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') "+