From 9306bc2d044d72478c8969dedffe9af49b4d4c9f Mon Sep 17 00:00:00 2001 From: David Battersby Date: Mon, 28 Apr 2025 12:12:20 +0400 Subject: [PATCH] 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. --- .../report_generator.rb | 35 +++++++--- .../recurring_data_explorer_result_pm_spec.rb | 8 +-- spec/report_generator_spec.rb | 66 ++++++++++++++++--- 3 files changed, 85 insertions(+), 24 deletions(-) diff --git a/lib/discourse_data_explorer/report_generator.rb b/lib/discourse_data_explorer/report_generator.rb index 9fc9ac7..a518b14 100644 --- a/lib/discourse_data_explorer/report_generator.rb +++ b/lib/discourse_data_explorer/report_generator.rb @@ -125,18 +125,33 @@ module ::DiscourseDataExplorer 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) } diff --git a/spec/automation/recurring_data_explorer_result_pm_spec.rb b/spec/automation/recurring_data_explorer_result_pm_spec.rb index 775d03e..377231f 100644 --- a/spec/automation/recurring_data_explorer_result_pm_spec.rb +++ b/spec/automation/recurring_data_explorer_result_pm_spec.rb @@ -61,8 +61,8 @@ describe "RecurringDataExplorerResultPM" do 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 @@ describe "RecurringDataExplorerResultPM" do 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 @@ describe "RecurringDataExplorerResultPM" do 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, diff --git a/spec/report_generator_spec.rb b/spec/report_generator_spec.rb index a1d5c94..694be1f 100644 --- a/spec/report_generator_spec.rb +++ b/spec/report_generator_spec.rb @@ -170,20 +170,66 @@ describe DiscourseDataExplorer::ReportGenerator do ) 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