From 20e7fb1c958790fa2dafc80bf6fe76b0556a2b92 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Wed, 29 Jan 2020 11:03:47 +1100 Subject: [PATCH] FIX: correct notification when tag or category is added (#8801) Regression was created here: https://github.com/discourse/discourse/pull/8750 When tag or category is added and the user is watching that category/tag we changed notification type to `edited` instead of `new post`. However, the logic here should be a little bit more sophisticated. If the user has already seen the post, notification should be `edited`. However, when user hasn't yet seen post, notification should be "new reply". The case for that is when for example topic is under private category and set for publishing later. In that case, we modify an existing topic, however, for a user, it is like a new post. Discussion on meta: https://meta.discourse.org/t/publication-of-timed-topics-dont-trigger-new-topic-notifications/139335/13 --- app/jobs/regular/notify_category_change.rb | 2 +- app/jobs/regular/notify_tag_change.rb | 2 +- app/services/post_alerter.rb | 5 +++-- spec/jobs/notify_tag_change_spec.rb | 2 +- spec/models/topic_spec.rb | 21 +++++++++++++++++++++ 5 files changed, 27 insertions(+), 5 deletions(-) diff --git a/app/jobs/regular/notify_category_change.rb b/app/jobs/regular/notify_category_change.rb index d6bf0dc0c2c..fbad7582e4a 100644 --- a/app/jobs/regular/notify_category_change.rb +++ b/app/jobs/regular/notify_category_change.rb @@ -7,7 +7,7 @@ module Jobs if post&.topic&.visible? post_alerter = PostAlerter.new - post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]), include_tag_watchers: false, new_record: false) + post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]), include_tag_watchers: false) post_alerter.notify_first_post_watchers(post, post_alerter.category_watchers(post.topic)) end end diff --git a/app/jobs/regular/notify_tag_change.rb b/app/jobs/regular/notify_tag_change.rb index d88deaf965c..dff6769de97 100644 --- a/app/jobs/regular/notify_tag_change.rb +++ b/app/jobs/regular/notify_tag_change.rb @@ -7,7 +7,7 @@ module Jobs if post&.topic&.visible? post_alerter = PostAlerter.new - post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]), include_category_watchers: false, new_record: false) + post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]), include_category_watchers: false) post_alerter.notify_first_post_watchers(post, post_alerter.tag_watchers(post.topic)) end end diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 55e27c36cd5..c66619dcc26 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -551,7 +551,7 @@ class PostAlerter end end - def notify_post_users(post, notified, include_category_watchers: true, include_tag_watchers: true, new_record: true) + def notify_post_users(post, notified, include_category_watchers: true, include_tag_watchers: true) return unless post.topic warn_if_not_sidekiq @@ -609,9 +609,10 @@ class PostAlerter DiscourseEvent.trigger(:before_create_notifications_for_users, notify, post) - notification_type = new_record ? Notification.types[:posted] : Notification.types[:edited] + already_seen_users = TopicUser.where(topic_id: post.topic.id).where("highest_seen_post_number >= ?", post.post_number).pluck(:user_id) notify.pluck(:id).each do |user_id| + notification_type = already_seen_users.include?(user_id) ? Notification.types[:edited] : Notification.types[:posted] user = User.find_by(id: user_id) create_notification(user, notification_type, post) end diff --git a/spec/jobs/notify_tag_change_spec.rb b/spec/jobs/notify_tag_change_spec.rb index ee96235e2e7..5215f0fa342 100644 --- a/spec/jobs/notify_tag_change_spec.rb +++ b/spec/jobs/notify_tag_change_spec.rb @@ -24,7 +24,7 @@ describe ::Jobs::NotifyTagChange do notification = Notification.last expect(notification.user_id).to eq(user.id) expect(notification.topic_id).to eq(post.topic_id) - expect(notification.notification_type).to eq(Notification.types[:edited]) + expect(notification.notification_type).to eq(Notification.types[:posted]) end it 'doesnt create notification for user watching category' do diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 8d320412817..b6494b209ec 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1400,6 +1400,27 @@ describe Topic do topic.change_category_to_id(new_category.id) end.to change { Notification.count }.by(2) + expect(Notification.where( + user_id: user.id, + topic_id: topic.id, + post_number: 1, + notification_type: Notification.types[:posted] + ).exists?).to eq(true) + + expect(Notification.where( + user_id: another_user.id, + topic_id: topic.id, + post_number: 1, + notification_type: Notification.types[:watching_first_post] + ).exists?).to eq(true) + end + + it 'should generate the modified notification for the topic if already seen' do + TopicUser.create!(topic_id: topic.id, highest_seen_post_number: topic.posts.first.post_number, user_id: user.id) + expect do + topic.change_category_to_id(new_category.id) + end.to change { Notification.count }.by(2) + expect(Notification.where( user_id: user.id, topic_id: topic.id,