DEV: improve group query access for report PM (#372)

This change updates the report generator script to handle groups first, that way when the option users_from_group: true is used we can determine query access based on the user rather than the whole group.

Author: dbattersby

lib/discourse_data_explorer/report_generator.rb

@@ -125,18 +125,33 @@ def self.filter_recipients_by_query_access(recipients, query, users_from_group:
       emails = recipients - users.pluck(:username) - groups.pluck(:name)
       result = []
 
-      users.each do |user|
-        result << [user.username, "username"] if Guardian.new(user).user_can_access_query?(query)
+      query_group_ids = [Group::AUTO_GROUPS[:admins]].concat(query.groups.pluck(:group_id)).uniq
+
+      if users_from_group
+        result.concat(
+          User
+            .joins(:group_users)
+            .where(group_users: { group_id: groups.ids })
+            .where(
+              "users.admin OR EXISTS (
+                SELECT 1 FROM group_users gu
+                WHERE gu.user_id = users.id
+                AND gu.group_id IN (?)
+              )",
+              query_group_ids,
+            )
+            .distinct
+            .pluck(:username)
+            .map { |username| [username, "username"] },
+        )
+      else
+        groups.each do |group|
+          result << [group.name, "group_name"] if query_group_ids.include?(group.id)
+        end
       end
 
-      groups.each do |group|
-        if group.id == Group::AUTO_GROUPS[:admins] || query.query_groups.exists?(group_id: group.id)
-          if users_from_group
-            result.concat(group.users.pluck(:username).map { |username| [username, "username"] })
-          else
-            result << [group.name, "group_name"]
-          end
-        end
+      users.each do |user|
+        result << [user.username, "username"] if Guardian.new(user).user_can_access_query?(query)
       end
 
       emails.each { |email| result << [email, "email"] if Email.is_valid?(email) }

spec/automation/recurring_data_explorer_result_pm_spec.rb

@@ -61,8 +61,8 @@
         automation.trigger!
       end.to change { Topic.count }.by(3)
 
-      user_topics = Topic.first(2)
-      group_topics = Topic.last(1)
+      user_topics = Topic.last(2)
+      group_topics = Topic.first(1)
       expect(Topic.last(3).pluck(:archetype)).to eq(
         [Archetype.private_message, Archetype.private_message, Archetype.private_message],
       )
@@ -83,7 +83,7 @@
       automation.update(last_updated_by_id: admin.id)
       automation.trigger!
 
-      expect(Post.last.raw).to eq(
+      expect(Post.first.raw).to eq(
         I18n.t(
           "data_explorer.report_generator.private_message.body",
           recipient_name: another_group.name,
@@ -112,7 +112,7 @@
       automation.update(last_updated_by_id: admin.id)
       automation.trigger!
 
-      expect(Post.last.raw).to eq(
+      expect(Post.first.raw).to eq(
         I18n.t(
           "data_explorer.report_generator.private_message.body",
           recipient_name: another_group.name,

spec/report_generator_spec.rb

@@ -170,20 +170,66 @@
         )
 
       expect(result.length).to eq(3)
-      expect(result[0]["target_usernames"]).to eq([user.username])
-      expect(result[1]["target_group_names"]).to eq([group.name])
+      expect(result[0]["target_group_names"]).to eq([group.name])
+      expect(result[1]["target_usernames"]).to eq([user.username])
       expect(result[2]["target_emails"]).to eq(["john@doe.com"])
     end
 
-    it "extracts users from group when option is selected" do
-      Fabricate(:query_group, query: query, group: group)
-      DiscourseDataExplorer::ResultToMarkdown.expects(:convert).returns("le table")
-      freeze_time
+    describe "with users_from_group" do
+      fab!(:valid_group) { Fabricate(:group, users: [user]) }
+      fab!(:invalid_group) { Fabricate(:group, users: []) }
 
-      result =
-        described_class.generate(query.id, query_params, [group.name], { users_from_group: true })
-      expect(result.length).to eq(1)
-      expect(result[0]["target_usernames"]).to eq([user.username])
+      describe "when true" do
+        let(:opts) { { users_from_group: true } }
+
+        it "does not work when no query groups are set" do
+          result = described_class.generate(query.id, query_params, [group.name], opts)
+          expect(result).to eq []
+        end
+
+        it "works when user is a member of automation group and query group" do
+          Fabricate(:query_group, query: query, group: valid_group)
+          result = described_class.generate(query.id, query_params, [group.name], opts)
+
+          expect(result.length).to eq(1)
+          expect(result[0]["target_usernames"]).to eq([user.username])
+        end
+
+        it "does not work when user is a member of automation group but not query group" do
+          Fabricate(:query_group, query: query, group: invalid_group)
+          result = described_class.generate(query.id, query_params, [group.name], opts)
+
+          expect(result).to eq []
+        end
+
+        it "works when user has access to one group in query groups" do
+          Fabricate(:query_group, query: query, group: valid_group)
+          Fabricate(:query_group, query: query, group: invalid_group)
+
+          result = described_class.generate(query.id, query_params, [group.name], opts)
+
+          expect(result.length).to eq(1)
+          expect(result[0]["target_usernames"]).to eq([user.username])
+        end
+      end
+
+      describe "when false" do
+        let(:opts) { { users_from_group: false } }
+
+        it "works when group has query access" do
+          Fabricate(:query_group, query: query, group: group)
+          result = described_class.generate(query.id, query_params, [group.name], opts)
+
+          expect(result.length).to eq(1)
+          expect(result[0]["target_group_names"]).to eq([group.name])
+        end
+
+        it "doesn't work when group doesn't have query access" do
+          result = described_class.generate(query.id, query_params, [group.name], opts)
+
+          expect(result).to eq []
+        end
+      end
     end
 
     it "works with attached csv file" do