From 1a12e4cfc8b97e36a47224eb849a756d998c5a85 Mon Sep 17 00:00:00 2001
From: Isaac Janzen <50783505+janzenisaac@users.noreply.github.com>
Date: Tue, 10 May 2022 10:02:28 -0500
Subject: [PATCH] FEATURE: Introduce site setting to allow for non staff pm
 tagging (#16671)

Currently the only way to allow tagging on pms is to use the `allow_staff_to_tag_pms` site setting.  We are removing that site setting and replacing it with `pm_tags_allowed_for_groups` which will allow for non staff tagging. It will be group based permissions instead of requiring the user to be staff.

If the existing value of `allow_staff_to_tag_pms` is `true` then we include the `staff` groups as a default for `pm_tags_allowed_for_groups`.
---
 config/locales/server.en.yml                  |  2 +-
 config/site_settings.yml                      |  9 +++++--
 ..._set_pm_tags_allowed_for_groups_default.rb | 22 ++++++++++++++++
 ...ove_allow_staff_to_tag_pms_site_setting.rb | 10 +++++++
 lib/guardian/tag_guardian.rb                  |  3 ++-
 lib/imap/sync.rb                              |  2 +-
 spec/fabricators/user_fabricator.rb           |  8 ++++++
 spec/lib/imap/sync_spec.rb                    |  3 +--
 spec/models/post_mover_spec.rb                |  4 +--
 spec/models/tag_spec.rb                       |  6 ++---
 spec/requests/categories_controller_spec.rb   |  2 +-
 spec/requests/groups_controller_spec.rb       | 26 +++++++++----------
 spec/requests/list_controller_spec.rb         |  6 ++---
 spec/requests/tags_controller_spec.rb         | 10 +++----
 spec/requests/topics_controller_spec.rb       |  2 +-
 .../serializers/topic_view_serializer_spec.rb | 24 +++++++++++++++--
 spec/services/post_alerter_spec.rb            |  2 +-
 17 files changed, 103 insertions(+), 38 deletions(-)
 create mode 100644 db/migrate/20220506221447_set_pm_tags_allowed_for_groups_default.rb
 create mode 100644 db/migrate/20220510131525_remove_allow_staff_to_tag_pms_site_setting.rb

diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index c5a312f3348..a51a9d53555 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2318,7 +2318,7 @@ en:
     tags_sort_alphabetically: "Show tags in alphabetical order. Default is to show in order of popularity."
     tags_listed_by_group: "List tags by tag group on the <a href='%{base_path}/tags' target='_blank'>Tags page</a>."
     tag_style: "Visual style for tag badges."
-    allow_staff_to_tag_pms: "Allow staff members to tag any personal message"
+    pm_tags_allowed_for_groups: "Allow members of included group(s) to tag any personal message"
     min_trust_level_to_tag_topics: "Minimum trust level required to tag topics"
     suppress_overlapping_tags_in_list: "If tags match exact words in topic titles, don't show the tag"
     remove_muted_tags_from_latest: "Don't show topics tagged only with muted tags in the latest topic list."
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 45f16b69b21..6d287f87e6a 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -2581,8 +2581,13 @@ tags:
   tags_listed_by_group:
     client: true
     default: false
-  allow_staff_to_tag_pms:
-    default: false
+  pm_tags_allowed_for_groups:
+    client: true
+    type: group_list
+    list_type: compact
+    default: ""
+    allow_any: false
+    refresh: true
   suppress_overlapping_tags_in_list:
     default: false
     client: true
diff --git a/db/migrate/20220506221447_set_pm_tags_allowed_for_groups_default.rb b/db/migrate/20220506221447_set_pm_tags_allowed_for_groups_default.rb
new file mode 100644
index 00000000000..e7a36eec4d5
--- /dev/null
+++ b/db/migrate/20220506221447_set_pm_tags_allowed_for_groups_default.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class SetPmTagsAllowedForGroupsDefault < ActiveRecord::Migration[7.0]
+  def up
+
+    # if the old SiteSetting of `allow_staff_to_tag_pms` was set to true, update the new SiteSetting of
+    # `pm_tags_allowed_for_groups` default to include the staff group
+    allow_staff_to_tag_pms = DB.query_single("SELECT value FROM site_settings WHERE name = 'allow_staff_to_tag_pms'").first
+
+    # Dynamically sets the default value
+    if allow_staff_to_tag_pms == "t"
+      # Include all staff groups - admins/moderators/staff
+      default = "1|2|3"
+      execute "INSERT INTO site_settings (name, data_type, value, created_at, updated_at)
+                VALUES ('pm_tags_allowed_for_groups', 20, '#{default}', now(), now())"
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/db/migrate/20220510131525_remove_allow_staff_to_tag_pms_site_setting.rb b/db/migrate/20220510131525_remove_allow_staff_to_tag_pms_site_setting.rb
new file mode 100644
index 00000000000..c8fc5989492
--- /dev/null
+++ b/db/migrate/20220510131525_remove_allow_staff_to_tag_pms_site_setting.rb
@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+class RemoveAllowStaffToTagPmsSiteSetting < ActiveRecord::Migration[7.0]
+  def up
+    execute "DELETE FROM site_settings WHERE name = 'allow_staff_to_tag_pms'"
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/lib/guardian/tag_guardian.rb b/lib/guardian/tag_guardian.rb
index 5d95faf8257..6de9672b87e 100644
--- a/lib/guardian/tag_guardian.rb
+++ b/lib/guardian/tag_guardian.rb
@@ -11,7 +11,8 @@ module TagGuardian
   end
 
   def can_tag_pms?
-    is_staff? && SiteSetting.tagging_enabled && SiteSetting.allow_staff_to_tag_pms
+    return false if @user.blank?
+    SiteSetting.tagging_enabled && @user == Discourse.system_user || @user.group_users.exists?(group_id: SiteSetting.pm_tags_allowed_for_groups.to_s.split("|").map(&:to_i))
   end
 
   def can_admin_tags?
diff --git a/lib/imap/sync.rb b/lib/imap/sync.rb
index f60338064c9..be5b32fb0d4 100644
--- a/lib/imap/sync.rb
+++ b/lib/imap/sync.rb
@@ -407,7 +407,7 @@ module Imap
     end
 
     def tagging_enabled?
-      SiteSetting.tagging_enabled && SiteSetting.allow_staff_to_tag_pms
+      Guardian.new(Discourse.system_user).can_tag_pms?
     end
   end
 end
diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb
index 9c60bc95f7d..385340ea08f 100644
--- a/spec/fabricators/user_fabricator.rb
+++ b/spec/fabricators/user_fabricator.rb
@@ -51,6 +51,10 @@ Fabricator(:moderator, from: :user) do
   username { sequence(:username) { |i| "moderator#{i}" } }
   email { sequence(:email) { |i| "moderator#{i}@discourse.org" } }
   moderator true
+
+  after_create do |user|
+    user.group_users << Fabricate(:group_user, user: user, group: Group[:moderators])
+  end
 end
 
 Fabricator(:admin, from: :user) do
@@ -58,6 +62,10 @@ Fabricator(:admin, from: :user) do
   username { sequence(:username) { |i| "anne#{i}" } }
   email { sequence(:email) { |i| "anne#{i}@discourse.org" } }
   admin true
+
+  after_create do |user|
+    user.group_users << Fabricate(:group_user, user: user, group: Group[:admins])
+  end
 end
 
 Fabricator(:newuser, from: :user) do
diff --git a/spec/lib/imap/sync_spec.rb b/spec/lib/imap/sync_spec.rb
index 37081e03507..7873aedd79b 100644
--- a/spec/lib/imap/sync_spec.rb
+++ b/spec/lib/imap/sync_spec.rb
@@ -6,7 +6,7 @@ describe Imap::Sync do
 
   before do
     SiteSetting.tagging_enabled = true
-    SiteSetting.allow_staff_to_tag_pms = true
+    SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
 
     SiteSetting.enable_imap = true
 
@@ -103,7 +103,6 @@ describe Imap::Sync do
     context "when tagging not enabled" do
       before do
         SiteSetting.tagging_enabled = false
-        SiteSetting.allow_staff_to_tag_pms = false
       end
 
       it "creates a topic from an incoming email but with no tags added" do
diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb
index b3eaee26aae..2e5378e7bf6 100644
--- a/spec/models/post_mover_spec.rb
+++ b/spec/models/post_mover_spec.rb
@@ -1264,8 +1264,8 @@ describe PostMover do
           )).to eq(true)
         end
 
-        it "can add tags to new message when allow_staff_to_tag_pms is enabled" do
-          SiteSetting.allow_staff_to_tag_pms = true
+        it "can add tags to new message when staff group is included in pm_tags_allowed_for_groups" do
+          SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
           personal_message.move_posts(admin, [p2.id, p5.id], title: "new testing message name", tags: ["tag1", "tag2"], archetype: "private_message")
 
           p2.reload
diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb
index 64195929ab2..8f68f476aa0 100644
--- a/spec/models/tag_spec.rb
+++ b/spec/models/tag_spec.rb
@@ -160,13 +160,13 @@ describe Tag do
       expect(Tag.pm_tags(guardian: Guardian.new(regular_user))).to be_empty
     end
 
-    it "returns nothing if allow_staff_to_tag_pms setting is disabled" do
-      SiteSetting.allow_staff_to_tag_pms = false
+    it "returns nothing if pm_tags_allowed_for_groups setting is empty" do
+      SiteSetting.pm_tags_allowed_for_groups = ""
       expect(Tag.pm_tags(guardian: Guardian.new(admin)).sort).to be_empty
     end
 
     it "returns all pm tags if user is a staff and pm tagging is enabled" do
-      SiteSetting.allow_staff_to_tag_pms = true
+      SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
       tags = Tag.pm_tags(guardian: Guardian.new(admin), allowed_user: regular_user)
       expect(tags.length).to eq(2)
       expect(tags.map { |t| t[:id] }).to contain_exactly("tag-0", "tag-1")
diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb
index 71e6e0f94e9..24eb5d25969 100644
--- a/spec/requests/categories_controller_spec.rb
+++ b/spec/requests/categories_controller_spec.rb
@@ -333,7 +333,7 @@ describe CategoriesController do
 
     describe "logged in" do
       it "raises an exception if they don't have permission to see it" do
-        admin.update!(admin: false)
+        admin.update!(admin: false, group_users: [])
         sign_in(admin)
         get "/c/#{category.id}/show.json"
         expect(response.status).to eq(403)
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
index de23c58a79e..9c45c296b26 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@@ -91,7 +91,7 @@ describe GroupsController do
         sign_in(user)
       end
 
-      fab!(:other_group) { Fabricate(:group, name: "other_group", users: [user, other_user]) }
+      fab!(:group_with_2_users) { Fabricate(:group, name: "other_group", users: [user, other_user]) }
 
       context "with default (descending) order" do
         it "sorts by name" do
@@ -102,7 +102,7 @@ describe GroupsController do
           body = response.parsed_body
 
           expect(body["groups"].map { |g| g["id"] }).to eq([
-            other_group.id, group.id, moderator_group_id
+            group_with_2_users.id, group.id, moderator_group_id
           ])
 
           expect(body["load_more_groups"]).to eq("/groups?order=name&page=1")
@@ -116,7 +116,7 @@ describe GroupsController do
           body = response.parsed_body
 
           expect(body["groups"].map { |g| g["id"] }).to eq([
-            other_group.id, group.id, moderator_group_id
+            group_with_2_users.id, moderator_group_id, group.id
           ])
 
           expect(body["load_more_groups"]).to eq("/groups?order=user_count&page=1")
@@ -132,7 +132,7 @@ describe GroupsController do
           body = response.parsed_body
 
           expect(body["groups"].map { |g| g["id"] }).to eq([
-            moderator_group_id, group.id, other_group.id
+            moderator_group_id, group.id, group_with_2_users.id
           ])
 
           expect(body["load_more_groups"]).to eq("/groups?asc=true&order=name&page=1")
@@ -146,7 +146,7 @@ describe GroupsController do
           body = response.parsed_body
 
           expect(body["groups"].map { |g| g["id"] }).to eq([
-            moderator_group_id, group.id, other_group.id
+            moderator_group_id, group.id, group_with_2_users.id
           ])
 
           expect(body["load_more_groups"]).to eq("/groups?asc=true&order=user_count&page=1")
@@ -297,18 +297,18 @@ describe GroupsController do
         end
 
         describe 'my groups' do
-          it 'should return the right response' do
-            expect_type_to_return_right_groups('my', [group.id])
+          it 'should return the groups admin is a member of' do
+            expect_type_to_return_right_groups('my', admin.group_users.map(&:group_id))
           end
         end
 
         describe 'owner groups' do
-          it 'should return the right response' do
+          it 'should return the groups admin is a owner of' do
             group2 = Fabricate(:group)
             _group3 = Fabricate(:group)
             group2.add_owner(admin)
 
-            expect_type_to_return_right_groups('owner', [group.id, group2.id])
+            expect_type_to_return_right_groups('owner', admin.group_users.where(owner: true).map(&:group_id))
           end
         end
 
@@ -794,8 +794,7 @@ describe GroupsController do
 
     context "when user is group admin" do
       before do
-        user.update!(admin: true)
-        sign_in(user)
+        sign_in(admin)
       end
 
       it 'should be able to update the group' do
@@ -839,7 +838,7 @@ describe GroupsController do
           .to eq(group.id)
       end
 
-      it "should be able to update an automatic group" do
+      it "they should be able to update an automatic group" do
         group = Group.find(Group::AUTO_GROUPS[:admins])
 
         group.update!(
@@ -861,7 +860,8 @@ describe GroupsController do
             default_notification_level: 1,
             tracking_category_ids: [category.id],
             tracking_tags: [tag.name]
-          }
+          },
+          update_existing_users: false
         }
 
         expect(response.status).to eq(200)
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index c0557655c05..c73ec252f21 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@@ -148,7 +148,7 @@ RSpec.describe ListController do
 
     before do
       SiteSetting.tagging_enabled = true
-      SiteSetting.allow_staff_to_tag_pms = true
+      SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
       Fabricate(:topic_tag, tag: tag, topic: private_message)
     end
 
@@ -158,8 +158,8 @@ RSpec.describe ListController do
       expect(response.status).to eq(404)
     end
 
-    it 'should fail for staff users if disabled' do
-      SiteSetting.allow_staff_to_tag_pms = false
+    it 'should fail for staff users if empty' do
+      SiteSetting.pm_tags_allowed_for_groups = ""
 
       [moderator, admin].each do |user|
         sign_in(user)
diff --git a/spec/requests/tags_controller_spec.rb b/spec/requests/tags_controller_spec.rb
index 0ee9efe7825..260f2d20026 100644
--- a/spec/requests/tags_controller_spec.rb
+++ b/spec/requests/tags_controller_spec.rb
@@ -30,7 +30,7 @@ describe TagsController do
       end
     end
 
-    context "with allow_staff_to_tag_pms" do
+    context "with pm_tags_allowed_for_groups" do
       fab!(:admin) { Fabricate(:admin) }
       fab!(:topic) { Fabricate(:topic, tags: [topic_tag]) }
       fab!(:pm) do
@@ -45,7 +45,7 @@ describe TagsController do
 
       context "enabled" do
         before do
-          SiteSetting.allow_staff_to_tag_pms = true
+          SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
           sign_in(admin)
         end
 
@@ -66,7 +66,7 @@ describe TagsController do
 
       context "disabled" do
         before do
-          SiteSetting.allow_staff_to_tag_pms = false
+          SiteSetting.pm_tags_allowed_for_groups = ""
           sign_in(admin)
         end
 
@@ -239,7 +239,7 @@ describe TagsController do
     end
 
     it "handles special tag 'none'" do
-      SiteSetting.allow_staff_to_tag_pms = true
+      SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
 
       sign_in(admin)
 
@@ -477,7 +477,7 @@ describe TagsController do
     fab!(:tag) { Fabricate(:tag, topics: [personal_message], name: 'test') }
 
     before do
-      SiteSetting.allow_staff_to_tag_pms = true
+      SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
     end
 
     context "as a regular user" do
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index 80da19936d1..3f14d1c8c96 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -417,7 +417,7 @@ RSpec.describe TopicsController do
         before { sign_in(admin) }
 
         it "returns success" do
-          SiteSetting.allow_staff_to_tag_pms = true
+          SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
 
           expect do
             post "/t/#{message.id}/move-posts.json", params: {
diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb
index e2577b77e9f..753a054e50a 100644
--- a/spec/serializers/topic_view_serializer_spec.rb
+++ b/spec/serializers/topic_view_serializer_spec.rb
@@ -14,6 +14,7 @@ describe TopicViewSerializer do
 
   fab!(:topic) { Fabricate(:topic) }
   fab!(:user) { Fabricate(:user) }
+  fab!(:user_2) { Fabricate(:user) }
   fab!(:admin) { Fabricate(:admin) }
 
   describe '#featured_link and #featured_link_root_domain' do
@@ -198,9 +199,17 @@ describe TopicViewSerializer do
       ])
     end
 
+    fab!(:group) { Fabricate(:group) }
+    fab!(:pm_between_reg_users) do
+      Fabricate(:private_message_topic, tags: [tag], topic_allowed_users: [
+        Fabricate.build(:topic_allowed_user, user: user),
+        Fabricate.build(:topic_allowed_user, user: user_2)
+      ])
+    end
+
     before do
       SiteSetting.tagging_enabled = true
-      SiteSetting.allow_staff_to_tag_pms = true
+      SiteSetting.pm_tags_allowed_for_groups = "1|2|3|4"
     end
 
     it "should not include the tag for normal users" do
@@ -215,8 +224,19 @@ describe TopicViewSerializer do
       end
     end
 
+    it "should include the tag for users in allowed groups" do
+      SiteSetting.pm_tags_allowed_for_groups = "1|2|3|#{group.id}"
+
+      user.group_users << Fabricate(:group_user, group: group, user: user)
+      json = serialize_topic(pm_between_reg_users, user)
+      expect(json[:tags]).to eq([tag.name])
+
+      json = serialize_topic(pm_between_reg_users, user_2)
+      expect(json[:tags]).to eq(nil)
+    end
+
     it "should not include the tag if pm tags disabled" do
-      SiteSetting.allow_staff_to_tag_pms = false
+      SiteSetting.pm_tags_allowed_for_groups = ""
 
       [moderator, admin].each do |user|
         json = serialize_topic(pm, user)
diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb
index b842424438e..fa70bf29a98 100644
--- a/spec/services/post_alerter_spec.rb
+++ b/spec/services/post_alerter_spec.rb
@@ -1438,7 +1438,7 @@ describe PostAlerter do
 
       before do
         SiteSetting.tagging_enabled = true
-        SiteSetting.allow_staff_to_tag_pms = true
+        SiteSetting.pm_tags_allowed_for_groups = "1|2|3"
         Jobs.run_immediately!
         TopicUser.change(user.id, post.topic.id, notification_level: TopicUser.notification_levels[:watching])
         TopicUser.change(staged.id, post.topic.id, notification_level: TopicUser.notification_levels[:watching])