diff --git a/lib/report_generator.rb b/lib/report_generator.rb
index 99e85ff..a5f708b 100644
--- a/lib/report_generator.rb
+++ b/lib/report_generator.rb
@@ -2,19 +2,11 @@
module ::DiscourseDataExplorer
class ReportGenerator
- def initialize(creator_user_id)
- @creator_user_id = creator_user_id
- end
-
- def generate(query_id, query_params, recipients)
+ def self.generate(query_id, query_params, recipients)
query = DiscourseDataExplorer::Query.find(query_id)
- return [] unless query
- return [] if recipients.empty?
+ return [] if !query || recipients.empty?
- creator = User.find_by(id: @creator_user_id)
- return [] unless Guardian.new(creator).can_send_private_messages?
-
- usernames = filter_recipients_by_query_access(recipients, query)
+ recipients = filter_recipients_by_query_access(recipients, query)
params = params_to_hash(query_params)
result = DataExplorer.run_query(query, params)
@@ -22,10 +14,12 @@ module ::DiscourseDataExplorer
table = ResultToMarkdown.convert(result[:pg_result])
- build_report_pms(query, table, usernames)
+ build_report_pms(query, table, recipients)
end
- def params_to_hash(query_params)
+ private
+
+ def self.params_to_hash(query_params)
params = JSON.parse(query_params)
params_hash = {}
@@ -45,13 +39,16 @@ module ::DiscourseDataExplorer
params_hash
end
- def build_report_pms(query, table = "", usernames = [])
+ def self.build_report_pms(query, table = "", targets = [])
pms = []
- usernames.flatten.compact.uniq.each do |username|
+ targets.each do |target|
+ name = target[0]
+ pm_type = "target_#{target[1]}s"
+
pm = {}
pm["title"] = "Scheduled Report for #{query.name}"
- pm["target_usernames"] = Array(username)
- pm["raw"] = "Hi #{username}, your data explorer report is ready.\n\n" +
+ pm[pm_type] = Array(name)
+ pm["raw"] = "Hi #{name}, your data explorer report is ready.\n\n" +
"Query Name:\n#{query.name}\n\nHere are the results:\n#{table}\n\n" +
"View query in Data Explorer\n\n" +
"Report created at #{Time.zone.now.strftime("%Y-%m-%d at %H:%M:%S")} (#{Time.zone.name})"
@@ -60,23 +57,25 @@ module ::DiscourseDataExplorer
pms
end
- private
+ def self.filter_recipients_by_query_access(recipients, query)
+ users = User.where(username: recipients)
+ groups = Group.where(name: recipients)
+ emails = recipients - users.pluck(:username) - groups.pluck(:name)
+ result = []
- def filter_recipients_by_query_access(recipients, query)
- recipients.reduce([]) do |names, recipient|
- if (group = Group.find_by(name: recipient)) &&
- (
- group.id == Group::AUTO_GROUPS[:admins] ||
- query.query_groups.exists?(group_id: group.id)
- )
- names.concat group.users.pluck(:username)
- elsif (user = User.find_by(username: recipient)) &&
- Guardian.new(user).user_can_access_query?(query)
- names << recipient
- end
-
- names
+ users.each do |user|
+ result << [user.username, "username"] if Guardian.new(user).user_can_access_query?(query)
end
+
+ groups.each do |group|
+ if group.id == Group::AUTO_GROUPS[:admins] || query.query_groups.exists?(group_id: group.id)
+ result << [group.name, "group_name"]
+ end
+ end
+
+ emails.each { |email| result << [email, "email"] if Email.is_valid?(email) }
+
+ result
end
end
end
diff --git a/plugin.rb b/plugin.rb
index d63fec4..082f2b7 100644
--- a/plugin.rb
+++ b/plugin.rb
@@ -94,7 +94,7 @@ after_initialize do
triggerables [:recurring]
script do |_, fields, automation|
- recipients = Array(fields.dig("recipients", "value"))
+ recipients = Array(fields.dig("recipients", "value")).uniq
query_id = fields.dig("query_id", "value")
query_params = fields.dig("query_params", "value") || {}
@@ -108,17 +108,15 @@ after_initialize do
next
end
- data_explorer_report =
- DiscourseDataExplorer::ReportGenerator.new(automation.last_updated_by_id)
- report_pms = data_explorer_report.generate(query_id, query_params, recipients)
-
- report_pms.each do |pm|
- begin
- utils.send_pm(pm, automation_id: automation.id)
- rescue ActiveRecord::RecordNotSaved => e
- Rails.logger.warn "#{DiscourseDataExplorer::PLUGIN_NAME} - couldn't send PM for automation #{automation.id}: #{e.message}"
+ DiscourseDataExplorer::ReportGenerator
+ .generate(query_id, query_params, recipients)
+ .each do |pm|
+ begin
+ utils.send_pm(pm, automation_id: automation.id, prefers_encrypt: false)
+ rescue ActiveRecord::RecordNotSaved => e
+ Rails.logger.warn "#{DiscourseDataExplorer::PLUGIN_NAME} - couldn't send PM for automation #{automation.id}: #{e.message}"
+ end
end
- end
end
end
end
diff --git a/spec/automation/recurring_data_explorer_result_pm_spec.rb b/spec/automation/recurring_data_explorer_result_pm_spec.rb
index 59528d5..8a687be 100644
--- a/spec/automation/recurring_data_explorer_result_pm_spec.rb
+++ b/spec/automation/recurring_data_explorer_result_pm_spec.rb
@@ -60,17 +60,20 @@ describe "RecurringDataExplorerResultPm" do
automation.trigger!
end.to change { Topic.count }.by(3)
- created_topics = Topic.last(3)
- expect(created_topics.pluck(:archetype)).to eq(
+ user_topics = Topic.first(2)
+ group_topics = Topic.last(1)
+ expect(Topic.last(3).pluck(:archetype)).to eq(
[Archetype.private_message, Archetype.private_message, Archetype.private_message],
)
- expect(created_topics.map { |t| t.allowed_users.pluck(:username).sort }).to match_array(
+ expect(user_topics.map { |t| t.allowed_users.pluck(:username).sort }).to match_array(
[
[user.username, Discourse.system_user.username],
[another_user.username, Discourse.system_user.username],
- [group_user.username, Discourse.system_user.username],
],
)
+ expect(group_topics.map { |t| t.allowed_groups.pluck(:name).sort }).to match_array(
+ [[another_group.name]],
+ )
end
it "has appropriate content from the report generator" do
@@ -78,7 +81,7 @@ describe "RecurringDataExplorerResultPm" do
automation.trigger!
expect(Post.last.raw).to include(
- "Hi #{group_user.username}, your data explorer report is ready.\n\nQuery Name:\n#{query.name}",
+ "Hi #{another_group.name}, your data explorer report is ready.\n\nQuery Name:\n#{query.name}",
)
end
end
diff --git a/spec/report_generator_spec.rb b/spec/report_generator_spec.rb
index fa5d304..c0e5005 100644
--- a/spec/report_generator_spec.rb
+++ b/spec/report_generator_spec.rb
@@ -15,16 +15,10 @@ describe DiscourseDataExplorer::ReportGenerator do
before { SiteSetting.data_explorer_enabled = true }
describe ".generate" do
- it "returns [] if the creator cannot send PMs" do
- result = described_class.new(user.id).generate(query.id, query_params, [user.username])
-
- expect(result).to eq []
- end
-
it "returns [] if the recipient is not in query group" do
Fabricate(:query_group, query: query, group: group)
result =
- described_class.new(user.id).generate(
+ described_class.generate(
query.id,
query_params,
[unauthorised_user.username, unauthorised_group.name],
@@ -38,7 +32,7 @@ describe DiscourseDataExplorer::ReportGenerator do
DiscourseDataExplorer::ResultToMarkdown.expects(:convert).returns("le table")
freeze_time
- result = described_class.new(user.id).generate(query.id, query_params, [user.username])
+ result = described_class.generate(query.id, query_params, [user.username])
expect(result).to eq(
[
@@ -56,21 +50,21 @@ describe DiscourseDataExplorer::ReportGenerator do
end
it "still returns a list of pms if a group or user does not exist" do
- admin = Fabricate(:admin)
+ Fabricate(:query_group, query: query, group: group)
+
SiteSetting.personal_message_enabled_groups = group.id
DiscourseDataExplorer::ResultToMarkdown.expects(:convert).returns("le table")
freeze_time
- result =
- described_class.new(user.id).generate(query.id, query_params, %w[admins non-existent-group])
+ result = described_class.generate(query.id, query_params, [group.name, "non-existent-group"])
expect(result).to eq(
[
{
"title" => "Scheduled Report for #{query.name}",
- "target_usernames" => [admin.username],
+ "target_group_names" => [group.name],
"raw" =>
- "Hi #{admin.username}, your data explorer report is ready.\n\n" +
+ "Hi #{group.name}, your data explorer report is ready.\n\n" +
"Query Name:\n#{query.name}\n\nHere are the results:\nle table\n\n" +
"View query in Data Explorer\n\n" +
"Report created at #{Time.zone.now.strftime("%Y-%m-%d at %H:%M:%S")} (#{Time.zone.name})",
@@ -78,5 +72,63 @@ describe DiscourseDataExplorer::ReportGenerator do
],
)
end
+
+ it "works with email recipients" do
+ DiscourseDataExplorer::ResultToMarkdown.expects(:convert).returns("le table")
+
+ email = "john@doe.com"
+ result = described_class.generate(query.id, query_params, [email])
+
+ expect(result).to eq(
+ [
+ {
+ "title" => "Scheduled Report for #{query.name}",
+ "target_emails" => [email],
+ "raw" =>
+ "Hi #{email}, your data explorer report is ready.\n\n" +
+ "Query Name:\n#{query.name}\n\nHere are the results:\nle table\n\n" +
+ "View query in Data Explorer\n\n" +
+ "Report created at #{Time.zone.now.strftime("%Y-%m-%d at %H:%M:%S")} (#{Time.zone.name})",
+ },
+ ],
+ )
+ end
+
+ it "works with duplicate recipients" do
+ DiscourseDataExplorer::ResultToMarkdown.expects(:convert).returns("table data")
+
+ result = described_class.generate(query.id, query_params, [user.username, user.username])
+
+ expect(result).to eq(
+ [
+ {
+ "title" => "Scheduled Report for #{query.name}",
+ "target_usernames" => [user.username],
+ "raw" =>
+ "Hi #{user.username}, your data explorer report is ready.\n\n" +
+ "Query Name:\n#{query.name}\n\nHere are the results:\ntable data\n\n" +
+ "View query in Data Explorer\n\n" +
+ "Report created at #{Time.zone.now.strftime("%Y-%m-%d at %H:%M:%S")} (#{Time.zone.name})",
+ },
+ ],
+ )
+ end
+
+ it "works with multiple recipient types" do
+ Fabricate(:query_group, query: query, group: group)
+ DiscourseDataExplorer::ResultToMarkdown.expects(:convert).returns("table data")
+
+ result =
+ described_class.generate(
+ query.id,
+ query_params,
+ [group.name, user.username, "john@doe.com"],
+ )
+
+ 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[2]["target_emails"]).to eq(["john@doe.com"])
+ end
end
end