FIX: send single report PM to groups (#284)

The main change here is that we now send a single PM to a group rather than individual PMs to each group member.

This change also adds email recipients correctly as target_emails since support was added within Discourse Automation.

lib/report_generator.rb

@@ -2,19 +2,11 @@
 
 module ::DiscourseDataExplorer
   class ReportGenerator
-    def initialize(creator_user_id)
+    def self.generate(query_id, query_params, recipients)
-      @creator_user_id = creator_user_id
-    end
-
-    def generate(query_id, query_params, recipients)
       query = DiscourseDataExplorer::Query.find(query_id)
-      return [] unless query
+      return [] if !query || recipients.empty?
-      return [] if recipients.empty?
 
-      creator = User.find_by(id: @creator_user_id)
+      recipients = filter_recipients_by_query_access(recipients, query)
-      return [] unless Guardian.new(creator).can_send_private_messages?
-
-      usernames = 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[pm_type] = Array(name)
-        pm["raw"] = "Hi #{username}, your data explorer report is ready.\n\n" +
+        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" +
           "<a href='#{Discourse.base_url}/admin/plugins/explorer?id=#{query.id}'>View query in Data Explorer</a>\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)
+      users.each do |user|
-      recipients.reduce([]) do |names, recipient|
+        result << [user.username, "username"] if Guardian.new(user).user_can_access_query?(query)
-        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
       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

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
-            DiscourseDataExplorer::ReportGenerator.new(automation.last_updated_by_id)
+            .generate(query_id, query_params, recipients)
-          report_pms = data_explorer_report.generate(query_id, query_params, recipients)
+            .each do |pm|
-
+              begin
-          report_pms.each do |pm|
+                utils.send_pm(pm, automation_id: automation.id, prefers_encrypt: false)
-            begin
+              rescue ActiveRecord::RecordNotSaved => e
-              utils.send_pm(pm, automation_id: automation.id)
+                Rails.logger.warn "#{DiscourseDataExplorer::PLUGIN_NAME} - couldn't send PM for automation #{automation.id}: #{e.message}"
-            rescue ActiveRecord::RecordNotSaved => e
+              end
-              Rails.logger.warn "#{DiscourseDataExplorer::PLUGIN_NAME} - couldn't send PM for automation #{automation.id}: #{e.message}"
             end
-          end
         end
       end
     end

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)
+      user_topics = Topic.first(2)
-      expect(created_topics.pluck(:archetype)).to eq(
+      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

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 =
+      result = described_class.generate(query.id, query_params, [group.name, "non-existent-group"])
-        described_class.new(user.id).generate(query.id, query_params, %w[admins 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" +
                 "<a href='#{Discourse.base_url}/admin/plugins/explorer?id=#{query.id}'>View query in Data Explorer</a>\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" +
+                "<a href='#{Discourse.base_url}/admin/plugins/explorer?id=#{query.id}'>View query in Data Explorer</a>\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" +
+                "<a href='#{Discourse.base_url}/admin/plugins/explorer?id=#{query.id}'>View query in Data Explorer</a>\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