From 8b1772ac0fbb32dafa8a18e4600161d7095df411 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Fri, 4 Mar 2016 22:55:49 +1100 Subject: [PATCH] FIX: if user undos like, and relikes notify FEATURE: roll up likes for notify on all likes --- .../components/notification-item.js.es6 | 12 +++- app/models/post_action.rb | 7 ++- app/models/post_alert_observer.rb | 61 ++++++++++++++++++- app/services/post_alerter.rb | 17 +++++- config/locales/client.en.yml | 4 ++ spec/models/post_alert_observer_spec.rb | 5 +- spec/services/post_alerter_spec.rb | 43 +++++++++++-- 7 files changed, 132 insertions(+), 17 deletions(-) diff --git a/app/assets/javascripts/discourse/components/notification-item.js.es6 b/app/assets/javascripts/discourse/components/notification-item.js.es6 index 844bf83edf9..64411038dfc 100644 --- a/app/assets/javascripts/discourse/components/notification-item.js.es6 +++ b/app/assets/javascripts/discourse/components/notification-item.js.es6 @@ -1,3 +1,4 @@ +const LIKED_TYPE = 5; const INVITED_TYPE = 8; const GROUP_SUMMARY_TYPE = 16; @@ -80,7 +81,16 @@ export default Ember.Component.extend({ const count = notification.get('data.inbox_count'); const group_name = notification.get('data.group_name'); text = I18n.t(this.get('scope'), {count, group_name}); - } else { + } else if (notification.get('notification_type') === LIKED_TYPE && notification.get("data.count") > 1) { + const count = notification.get('data.count') - 2; + const username2 = notification.get('data.username2'); + if (count===0) { + text = I18n.t('notifications.liked_2', {description, username, username2}); + } else { + text = I18n.t('notifications.liked_many', {description, username, username2, count}); + } + } + else { text = I18n.t(this.get('scope'), {description, username}); } text = Discourse.Emoji.unescape(text); diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 943dbc8ebd7..d84c9a808f2 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -147,10 +147,10 @@ SQL # so callback is called action.save action.add_moderator_post_if_needed(moderator, :agreed, delete_post) - @trigger_spam = true if action.post_action_type_id == PostActionType.types[:spam] + trigger_spam = true if action.post_action_type_id == PostActionType.types[:spam] end - DiscourseEvent.trigger(:confirmed_spam_post, post) if @trigger_spam + DiscourseEvent.trigger(:confirmed_spam_post, post) if trigger_spam update_flagged_posts_count end @@ -272,6 +272,7 @@ SQL post_action.recover! action_attrs.each { |attr, val| post_action.send("#{attr}=", val) } post_action.save + PostAlertObserver.after_create_post_action(post_action) else post_action = create(where_attrs.merge(action_attrs)) if post_action && post_action.errors.count == 0 @@ -480,7 +481,7 @@ SQL elsif PostActionType.auto_action_flag_types.include?(post_action_type) && SiteSetting.flags_required_to_hide_post > 0 - old_flags, new_flags = PostAction.flag_counts_for(post.id) + _old_flags, new_flags = PostAction.flag_counts_for(post.id) if new_flags >= SiteSetting.flags_required_to_hide_post hide_post!(post, post_action_type, guess_hide_reason(post)) diff --git a/app/models/post_alert_observer.rb b/app/models/post_alert_observer.rb index ce2b366526a..f1913ab7767 100644 --- a/app/models/post_alert_observer.rb +++ b/app/models/post_alert_observer.rb @@ -1,10 +1,14 @@ class PostAlertObserver < ActiveRecord::Observer observe :post_action, :post_revision - def alerter + def self.alerter @alerter ||= PostAlerter.new end + def alerter + self.class.alerter + end + # Dispatch to an after_save_#{class_name} method def after_save(model) method_name = callback_for('after_save', model) @@ -17,13 +21,60 @@ class PostAlertObserver < ActiveRecord::Observer send(method_name, model) if respond_to?(method_name) end + def refresh_like_notification(post, read) + return unless post && post.user_id + + usernames = post.post_actions.where(post_action_type_id: PostActionType.types[:like]) + .joins(:user) + .order('post_actions.created_at desc') + .where('post_actions.created_at > ?', 1.day.ago) + .pluck(:username) + + if usernames.length > 0 + data = { + topic_title: post.topic.title, + username: usernames[0], + display_username: usernames[0], + username2: usernames[1], + count: usernames.length + } + Notification.create( + notification_type: Notification.types[:liked], + topic_id: post.topic_id, + post_number: post.post_number, + user_id: post.user_id, + read: read, + data: data.to_json + ) + end + end + def after_save_post_action(post_action) # We only care about deleting post actions for now return if post_action.deleted_at.blank? - Notification.where(post_action_id: post_action.id).each(&:destroy) + + if post_action.post_action_type_id == PostActionType.types[:like] && post_action.post + + read = true + + Notification.where( + topic_id: post_action.post.topic_id, + user_id: post_action.post.user_id, + post_number: post_action.post.post_number, + notification_type: Notification.types[:liked] + ).each do |notification| + read = false unless notification.read + notification.destroy + end + + refresh_like_notification(post_action.post, read) + else + # not using destroy_all cause we want stuff to trigger + Notification.where(post_action_id: post_action.id).each(&:destroy) + end end - def after_create_post_action(post_action) + def self.after_create_post_action(post_action) # We only notify on likes for now return unless post_action.is_like? @@ -40,6 +91,10 @@ class PostAlertObserver < ActiveRecord::Observer ) end + def after_create_post_action(post_action) + self.class.after_create_post_action(post_action) + end + def after_create_post_revision(post_revision) post = post_revision.post diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 8fff00b59bc..97cbcfc482b 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -251,6 +251,19 @@ class PostAlerter return if existing_notification && !should_notify_previous?(user, existing_notification, opts) + notification_data = {} + + if existing_notification && + existing_notification.created_at > 1.day.ago && + user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:always] + + data = existing_notification.data_hash + notification_data["username2"] = data["display_username"] + notification_data["count"] = (data["count"] || 1).to_i + 1 + # don't use destroy so we don't trigger a notification count refresh + Notification.where(id: existing_notification.id).destroy_all + end + collapsed = false if type == Notification.types[:replied] || type == Notification.types[:posted] @@ -287,13 +300,13 @@ class PostAlerter end end - notification_data = { + notification_data.merge!({ topic_title: topic_title, original_post_id: original_post.id, original_post_type: original_post.post_type, original_username: original_username, display_username: opts[:display_username] || post.user.username - } + }) if group = opts[:group] notification_data[:group_id] = group.id diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9ecbee2a7cf..78ce98f7f69 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1014,6 +1014,10 @@ en: posted: "

{{username}} {{description}}

" edited: "

{{username}} {{description}}

" liked: "

{{username}} {{description}}

" + liked_2: "

{{username}}, {{username2}} {{description}}

" + liked_many: + one: "

{{username}}, {{username2}} and 1 other {{description}}

" + other: "

{{username}}, {{username2}} and {{count}} others {{description}}

" private_message: "

{{username}} {{description}}

" invited_to_private_message: "

{{username}} {{description}}

" invited_to_topic: "

{{username}} {{description}}

" diff --git a/spec/models/post_alert_observer_spec.rb b/spec/models/post_alert_observer_spec.rb index c4de6041739..be6f8c7876c 100644 --- a/spec/models/post_alert_observer_spec.rb +++ b/spec/models/post_alert_observer_spec.rb @@ -21,11 +21,8 @@ describe PostAlertObserver do end context 'when removing a liked post' do - before do - PostAction.act(evil_trout, post, PostActionType.types[:like]) - end - it 'removes a notification' do + PostAction.act(evil_trout, post, PostActionType.types[:like]) expect { PostAction.remove_act(evil_trout, post, PostActionType.types[:like]) }.to change(Notification, :count).by(-1) diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index b481c889866..82f3a0a274d 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -50,12 +50,27 @@ describe PostAlerter do end context 'likes' do + + it 'notifies on likes after an undo' do + ActiveRecord::Base.observers.enable :all + + post = Fabricate(:post, raw: 'I love waffles') + + PostAction.act(evil_trout, post, PostActionType.types[:like]) + PostAction.remove_act(evil_trout, post, PostActionType.types[:like]) + PostAction.act(evil_trout, post, PostActionType.types[:like]) + + expect(Notification.count(post_number: 1, topic_id: post.topic_id)).to eq(1) + end + it 'notifies on likes correctly' do ActiveRecord::Base.observers.enable :all post = Fabricate(:post, raw: 'I love waffles') + PostAction.act(evil_trout, post, PostActionType.types[:like]) - PostAction.act(Fabricate(:admin), post, PostActionType.types[:like]) + admin = Fabricate(:admin) + PostAction.act(admin, post, PostActionType.types[:like]) # one like expect(Notification.count(post_number: 1, topic_id: post.topic_id)).to eq(1) @@ -66,8 +81,28 @@ describe PostAlerter do admin2 = Fabricate(:admin) PostAction.act(admin2, post, PostActionType.types[:like]) - # two likes one edit - expect(Notification.count(post_number: 1, topic_id: post.topic_id)).to eq(2) + expect(Notification.count(post_number: 1, topic_id: post.topic_id)).to eq(1) + + # adds info to the notification + notification = Notification.find_by(post_number: 1, + topic_id: post.topic_id) + + + expect(notification.data_hash["count"].to_i).to eq(2) + expect(notification.data_hash["username2"]).to eq(evil_trout.username) + + # this is a tricky thing ... removing a like should fix up the notifications + PostAction.remove_act(evil_trout, post, PostActionType.types[:like]) + + # rebuilds the missing notification + expect(Notification.count(post_number: 1, topic_id: post.topic_id)).to eq(1) + notification = Notification.find_by(post_number: 1, + topic_id: post.topic_id) + + expect(notification.data_hash["count"]).to eq(2) + expect(notification.data_hash["username"]).to eq(admin2.username) + expect(notification.data_hash["username2"]).to eq(admin.username) + post.user.user_option.update_columns(like_notification_frequency: UserOption.like_notification_frequency_type[:first_time_and_daily]) @@ -82,7 +117,7 @@ describe PostAlerter do end # first happend within the same day, no need to notify - expect(Notification.count(post_number: 1, topic_id: post.topic_id)).to eq(3) + expect(Notification.count(post_number: 1, topic_id: post.topic_id)).to eq(2) end end