From 1598e6b489125bade2a7bfc144ae346815dedf00 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 6 Apr 2022 11:43:57 +1000 Subject: [PATCH] FIX: users watching tags in open tag groups not notified (#16384) All users are members of the EVERYONE group, but this group is special and is omitted from the group_users table. When checking permission we need to make sure we also add a bypass. This also fixes a very buggy test in post_alerter, it was confirming the broken behavior due to fabricator flow. When it defined the tag group the everyone group automatically had full access then the additional permission fabricated just added one more group. After fix was made to code the test started failing. Fabricators can be risky. --- app/services/post_alerter.rb | 10 ++++++++-- spec/models/tag_user_spec.rb | 5 +++++ spec/services/post_alerter_spec.rb | 6 ++++-- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index da681499fb0..8568d5fc5a5 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -802,7 +802,12 @@ class PostAlerter LEFT JOIN tag_group_memberships tgm ON tag_users.tag_id = tgm.tag_id LEFT JOIN tag_group_permissions tgp ON tgm.tag_group_id = tgp.tag_group_id LEFT JOIN group_users gu ON gu.user_id = tag_users.user_id - WHERE (tgp.group_id IS NULL OR tgp.group_id = gu.group_id OR gu.group_id = :staff_group_id) + WHERE ( + tgp.group_id IS NULL OR + tgp.group_id = gu.group_id OR + tgp.group_id = :everyone_group_id OR + gu.group_id = :staff_group_id + ) AND (tag_users.notification_level = :watching AND tag_users.tag_id IN (:tag_ids) AND (tu.user_id IS NULL OR tu.notification_level = :watching)) @@ -814,7 +819,8 @@ class PostAlerter topic_id: post.topic_id, category_id: post.topic.category_id, tag_ids: tag_ids, - staff_group_id: Group::AUTO_GROUPS[:staff] + staff_group_id: Group::AUTO_GROUPS[:staff], + everyone_group_id: Group::AUTO_GROUPS[:everyone] ) if group_ids.present? diff --git a/spec/models/tag_user_spec.rb b/spec/models/tag_user_spec.rb index 36dc462053e..96af3d950ff 100644 --- a/spec/models/tag_user_spec.rb +++ b/spec/models/tag_user_spec.rb @@ -183,6 +183,11 @@ describe TagUser do it "sets notification levels correctly" do + # define a wide open tag group to ensure it also works + group = TagGroup.new(name: 'Visible & usable by everyone', tag_names: [watched_tag.name]) + group.permissions = [[Group::AUTO_GROUPS[:everyone], TagGroupPermission.permission_types[:full]]] + group.save! + expect(Notification.where(user_id: user.id, topic_id: watched_post.topic_id).count).to eq 1 expect(Notification.where(user_id: user.id, topic_id: tracked_post.topic_id).count).to eq 0 diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index ddc26a448be..b842424438e 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1372,8 +1372,10 @@ describe PostAlerter do tag = Fabricate(:tag) topic = Fabricate(:topic, tags: [tag]) post = Fabricate(:post, topic: topic) - tag_group = Fabricate(:tag_group, tags: [tag]) - Fabricate(:tag_group_permission, tag_group: tag_group, group: group) + + tag_group = TagGroup.new(name: 'Only visible to group', tag_names: [tag.name]) + tag_group.permissions = [[group.id, TagGroupPermission.permission_types[:full]]] + tag_group.save! TagUser.change(user.id, tag.id, TagUser.notification_levels[:watching])