FIX: Optionally linkify URL columns server-side (#330)

Followup da1c99e

We already had the functionality to convert results like
this:

```
|blah_url|
|--------|
|3,https://test.com|
```

To a link clientside, so in the UI it looks like this:

```
<a href="https://test.com">3</a>
```

With the addition of the recurring report to post automation,
and the existing report to PM automation, we also need to be
able to do this server-side, so reports don't come out malformed
if they have these type of count + URL columns.

Author: martin-brennan

assets/javascripts/discourse/components/query-row-content.js

@@ -8,7 +8,7 @@ import TextViewComponent from "./result-types/text";
 export default class QueryRowContent extends Component {
   @cached
   get results() {
-    return this.args.columnComponents.map((t, idx) => {
+    return this.args.columnComponents.map((componentDefinition, idx) => {
       const value = this.args.row[idx],
         id = parseInt(value, 10);
 
@@ -23,27 +23,28 @@ export default class QueryRowContent extends Component {
           component: TextViewComponent,
           textValue: "NULL",
         };
-      } else if (t.name === "text") {
+      } else if (componentDefinition.name === "text") {
         return {
           component: TextViewComponent,
           textValue: escapeExpression(this.args.row[idx].toString()),
         };
       }
 
-      const lookupFunc = this.args[`lookup${capitalize(t.name)}`];
+      const lookupFunc =
+        this.args[`lookup${capitalize(componentDefinition.name)}`];
       if (lookupFunc) {
-        ctx[t.name] = lookupFunc.call(this.args, id);
+        ctx[componentDefinition.name] = lookupFunc.call(this.args, id);
       }
 
-      if (t.name === "url") {
+      if (componentDefinition.name === "url") {
         let [url, name] = guessUrl(value);
         ctx["href"] = url;
         ctx["target"] = name;
       }
 
       try {
         return {
-          component: t.component || TextViewComponent,
+          component: componentDefinition.component || TextViewComponent,
           ctx,
         };
       } catch (e) {
@@ -53,10 +54,10 @@ export default class QueryRowContent extends Component {
   }
 }
 
-function guessUrl(t) {
-  let [dest, name] = [t, t];
+function guessUrl(columnValue) {
+  let [dest, name] = [columnValue, columnValue];
 
-  const split = t.split(/,(.+)/);
+  const split = columnValue.split(/,(.+)/);
 
   if (split.length > 1) {
     name = split[0];

lib/discourse_data_explorer/report_generator.rb

@@ -13,7 +13,8 @@ def self.generate(query_id, query_params, recipients, opts = {})
       query.update!(last_run_at: Time.now)
 
       return [] if opts[:skip_empty] && result[:pg_result].values.empty?
-      table = ResultToMarkdown.convert(result[:pg_result])
+      table =
+        ResultToMarkdown.convert(result[:pg_result], render_url_columns: opts[:render_url_columns])
 
       build_report_pms(query, table, recipients, attach_csv: opts[:attach_csv], result:)
     end
@@ -28,7 +29,8 @@ def self.generate_post(query_id, query_params, opts = {})
       query.update!(last_run_at: Time.now)
 
       return {} if opts[:skip_empty] && result[:pg_result].values.empty?
-      table = ResultToMarkdown.convert(result[:pg_result])
+      table =
+        ResultToMarkdown.convert(result[:pg_result], render_url_columns: opts[:render_url_columns])
 
       build_report_post(query, table, attach_csv: opts[:attach_csv], result:)
     end

lib/discourse_data_explorer/result_to_markdown.rb

@@ -4,7 +4,7 @@
 
 module ::DiscourseDataExplorer
   class ResultToMarkdown
-    def self.convert(pg_result)
+    def self.convert(pg_result, render_url_columns = false)
       relations, colrender = DataExplorer.add_extra_data(pg_result)
       result_data = []
 
@@ -17,7 +17,8 @@ def self.convert(pg_result)
 
         row.each_with_index do |col, col_index|
           col_name = pg_result.fields[col_index]
-          related = relations.dig(colrender[col_index].to_sym) unless colrender[col_index].nil?
+          col_render = colrender[col_index]
+          related = relations.dig(col_render.to_sym) if col_render.present?
 
           if related.is_a?(ActiveModel::ArraySerializer)
             related_row = related.object.find_by(id: col)
@@ -32,6 +33,9 @@ def self.convert(pg_result)
             else
               row_data[col_index] = "#{related_row[column]} (#{col})"
             end
+          elsif col_render == "url" && render_url_columns
+            url, text = guess_url(col)
+            row_data[col_index] = "[#{text}](#{url})"
           else
             row_data[col_index] = col
           end
@@ -45,5 +49,11 @@ def self.convert(pg_result)
 
       "|#{table_headers}\n|#{table_body}\n#{result_data.join}"
     end
+
+    def self.guess_url(column_value)
+      text, url = column_value.split(/,(.+)/)
+
+      [url || column_value, text || column_value]
+    end
   end
 end

plugin.rb

@@ -105,7 +105,12 @@ module ::DiscourseDataExplorer
           end
 
           DiscourseDataExplorer::ReportGenerator
-            .generate(query_id, query_params, recipients, { skip_empty:, attach_csv: })
+            .generate(
+              query_id,
+              query_params,
+              recipients,
+              { skip_empty:, attach_csv:, render_url_columns: true },
+            )
             .each do |pm|
               begin
                 utils.send_pm(pm, automation_id: automation.id, prefers_encrypt: false)
@@ -153,7 +158,7 @@ module ::DiscourseDataExplorer
               DiscourseDataExplorer::ReportGenerator.generate_post(
                 query_id,
                 query_params,
-                { skip_empty:, attach_csv: },
+                { skip_empty:, attach_csv:, render_url_columns: true },
               )
 
             next if post.empty?

spec/automation/recurring_data_explorer_result_pm_spec.rb

@@ -18,7 +18,7 @@
   end
   fab!(:query) { Fabricate(:query, sql: "SELECT 'testabcd' AS data") }
   fab!(:query_group) { Fabricate(:query_group, query: query, group: group) }
-  fab!(:query_group) { Fabricate(:query_group, query: query, group: another_group) }
+  fab!(:query_group_2) { Fabricate(:query_group, query: query, group: another_group) }
 
   let!(:recipients) do
     [user.username, not_allowed_user.username, another_user.username, another_group.name]
@@ -105,5 +105,25 @@
 
       expect { automation.trigger! }.to_not change { Post.count }
     end
+
+    it "works with a query that returns URLs in a number,url format" do
+      freeze_time
+      query.update!(sql: "SELECT 3 || ',https://test.com' AS some_url")
+      automation.update(last_updated_by_id: admin.id)
+      automation.trigger!
+
+      expect(Post.last.raw).to eq(
+        I18n.t(
+          "data_explorer.report_generator.private_message.body",
+          recipient_name: another_group.name,
+          query_name: query.name,
+          table: "| some_url |\n| :----- |\n| [3](https://test.com) |\n",
+          base_url: Discourse.base_url,
+          query_id: query.id,
+          created_at: Time.zone.now.strftime("%Y-%m-%d at %H:%M:%S"),
+          timezone: Time.zone.name,
+        ).chomp,
+      )
+    end
   end
 end

spec/automation/recurring_data_explorer_result_topic_spec.rb

@@ -12,14 +12,12 @@
   fab!(:topic)
 
   fab!(:group) { Fabricate(:group, users: [user, another_user]) }
-  fab!(:another_group) { Fabricate(:group, users: [group_user]) }
 
   fab!(:automation) do
     Fabricate(:automation, script: "recurring_data_explorer_result_topic", trigger: "recurring")
   end
   fab!(:query) { Fabricate(:query, sql: "SELECT 'testabcd' AS data") }
   fab!(:query_group) { Fabricate(:query_group, query: query, group: group) }
-  fab!(:query_group) { Fabricate(:query_group, query: query, group: another_group) }
 
   before do
     SiteSetting.data_explorer_enabled = true
@@ -88,5 +86,24 @@
 
       expect { automation.trigger! }.to_not change { Post.count }
     end
+
+    it "works with a query that returns URLs in a number,url format" do
+      freeze_time
+      query.update!(sql: "SELECT 3 || ',https://test.com' AS some_url")
+      automation.update(last_updated_by_id: admin.id)
+      automation.trigger!
+
+      expect(topic.posts.last.raw).to eq(
+        I18n.t(
+          "data_explorer.report_generator.post.body",
+          query_name: query.name,
+          table: "| some_url |\n| :----- |\n| [3](https://test.com) |\n",
+          base_url: Discourse.base_url,
+          query_id: query.id,
+          created_at: Time.zone.now.strftime("%Y-%m-%d at %H:%M:%S"),
+          timezone: Time.zone.name,
+        ).chomp,
+      )
+    end
   end
 end