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
This commit is contained in:
Krzysztof Kotlarek 2020-01-29 11:03:47 +11:00 committed by GitHub
parent 9621af2214
commit 20e7fb1c95
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 27 additions and 5 deletions

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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,