FEAT: add cc addresses and post_id to sent email logs (#25014)

* add cc addresses and post_id to sent email logs
* sort cc addresses by email address filter value and collapse additional addreses into tooltip
* add slice helper for use in ember tempaltes

app/assets/javascripts/admin/addon/controllers/admin-email-sent.js

@@ -4,6 +4,28 @@ import discourseDebounce from "discourse-common/lib/debounce";
 import AdminEmailLogsController from "admin/controllers/admin-email-logs";
 
 export default class AdminEmailSentController extends AdminEmailLogsController {
+  ccAddressDisplayThreshold = 2;
+  sortWithAddressFilter = (addresses) => {
+    if (!Array.isArray(addresses) || addresses.length === 0) {
+      return [];
+    }
+    const targetEmail = this.filter.address;
+
+    if (!targetEmail) {
+      return addresses;
+    }
+
+    return addresses.sort((a, b) => {
+      if (a.includes(targetEmail) && !b.includes(targetEmail)) {
+        return -1;
+      }
+      if (!a.includes(targetEmail) && b.includes(targetEmail)) {
+        return 1;
+      }
+      return 0;
+    });
+  };
+
   @observes("filter.{status,user,address,type,reply_key}")
   filterEmailLogs() {
     discourseDebounce(this, this.loadLogs, INPUT_DELAY);

app/assets/javascripts/admin/addon/templates/email-sent.hbs

@@ -54,7 +54,53 @@
                 "redo"
                 title="admin.email.bounced"
               }}{{/if}}
-            <a href="mailto:{{l.to_address}}">{{l.to_address}}</a>
+            <p><a
+                href="mailto:{{l.to_address}}"
+                title="TO"
+              >{{l.to_address}}</a></p>
+            {{#if l.cc_addresses}}
+              {{#if (gt l.cc_addresses.length this.ccAddressDisplayThreshold)}}
+                {{#each
+                  (slice
+                    0
+                    this.ccAddressDisplayThreshold
+                    (fn this.sortWithAddressFilter l.cc_addresses)
+                  )
+                  as |cc|
+                }}
+                  <p><a href="mailto:{{cc}}" title="CC">{{cc}}</a></p>
+                {{/each}}
+                <DTooltip
+                  @placement="right-start"
+                  @arrow={{true}}
+                  @identifier="email-log-cc-addresses"
+                  @interactive={{true}}
+                >
+                  <:trigger>
+                    {{i18n "admin.email.logs.email_addresses.see_more"}}
+                  </:trigger>
+                  <:content>
+                    <ul>
+                      {{#each
+                        (slice this.ccAddressDisplayThreshold l.cc_addresses)
+                        as |cc|
+                      }}
+                        <li>
+                          <span>
+                            <a href="mailto:{{cc}}" title="CC">{{cc}}</a>
+                          </span>
+                        </li>
+                      {{/each}}
+                    </ul>
+                  </:content>
+                </DTooltip>
+
+              {{else}}
+                {{#each l.cc_addresses as |cc|}}
+                  <p><a href="mailto:{{cc}}" title="CC">{{cc}}</a></p>
+                {{/each}}
+              {{/if}}
+            {{/if}}
           </td>
           <td class="sent-email-type">{{l.email_type}}</td>
           <td class="sent-email-reply-key">
@@ -62,8 +108,12 @@
           </td>
           <td class="sent-email-post-link-with-smtp-response">
             {{#if l.post_url}}
-              <a href={{l.post_url}}>{{l.post_description}}</a><br />
+              <a href={{l.post_url}}>
-            {{/if}}
+                {{l.post_description}}
+              </a>
+              {{i18n "admin.email.logs.post_id" post_id=l.post_id}}
+              <br />
+              /{{/if}}
             <code
               title={{l.smtp_transaction_response}}
             >{{l.smtp_transaction_response}}</code>

app/assets/javascripts/discourse/app/helpers/slice.js

@@ -0,0 +1,10 @@
+export default function slice(...args) {
+  let array = args.pop();
+  if (array instanceof Function) {
+    array = array.call();
+  }
+  if (!Array.isArray(array) || array.length === 0) {
+    return [];
+  }
+  return array.slice(...args);
+}

app/assets/javascripts/discourse/tests/integration/helpers/slice-test.js

@@ -0,0 +1,70 @@
+import { run } from "@ember/runloop";
+import { render } from "@ember/test-helpers";
+import { hbs } from "ember-cli-htmlbars";
+import { setupRenderingTest } from "ember-qunit";
+import { module, test } from "qunit";
+
+module("Integration | Helper | {{slice}}", function (hooks) {
+  setupRenderingTest(hooks);
+
+  test("it slices an array with positional params", async function (assert) {
+    this.set("array", [2, 4, 6]);
+
+    await render(hbs`
+      {{slice 1 3 this.array}}
+    `);
+
+    assert.dom().hasText("4,6", "sliced values");
+  });
+
+  test("it slices when only 2 params are passed", async function (assert) {
+    this.set("array", [2, 4, 6]);
+
+    await render(hbs`
+      {{slice 1 this.array}}
+    `);
+
+    assert.dom().hasText("4,6", "sliced values");
+  });
+
+  test("it recomputes the slice if an item in the array changes", async function (assert) {
+    let array = [2, 4, 6];
+    this.set("array", array);
+
+    await render(hbs`
+      {{slice 1 3 this.array}}
+    `);
+
+    assert.dom().hasText("4,6", "sliced values");
+
+    run(() => array.replace(2, 1, [5]));
+
+    assert.dom().hasText("4,5", "sliced values");
+  });
+
+  test("it allows null array", async function (assert) {
+    this.set("array", null);
+
+    await render(hbs`
+      this is all that will render
+      {{#each (slice 1 2 this.array) as |value|}}
+        {{value}}
+      {{/each}}
+    `);
+
+    assert.dom().hasText("this is all that will render", "no error is thrown");
+  });
+
+  test("it allows undefined array", async function (assert) {
+    this.set("array", undefined);
+
+    await render(hbs`
+      this is all that will render
+      {{#each (slice 1 2 this.array) as |value|}}
+        {{value}}
+      {{/each}}
+    `);
+
+    assert.dom().hasText("this is all that will render", "no error is thrown");
+  });
+});

app/controllers/admin/email_controller.rb

@@ -25,7 +25,7 @@ class Admin::EmailController < Admin::AdminController
       AND post_reply_keys.user_id = email_logs.user_id
     SQL
 
-    email_logs = filter_logs(email_logs, params)
+    email_logs = filter_logs(email_logs, params, include_ccs: params[:type] == "group_smtp")
 
     if (reply_key = params[:reply_key]).present?
       email_logs =
@@ -223,7 +223,7 @@ class Admin::EmailController < Admin::AdminController
 
   private
 
-  def filter_logs(logs, params)
+  def filter_logs(logs, params, include_ccs: false)
     table_name = logs.table_name
 
     logs =
@@ -235,9 +235,14 @@ class Admin::EmailController < Admin::AdminController
         .limit(50)
 
     logs = logs.where("users.username ILIKE ?", "%#{params[:user]}%") if params[:user].present?
-    logs = logs.where("#{table_name}.to_address ILIKE ?", "%#{params[:address]}%") if params[
+
-      :address
+    if params[:address].present?
-    ].present?
+      query = "#{table_name}.to_address ILIKE :address"
+      query += " OR #{table_name}.cc_addresses ILIKE :address" if include_ccs
+
+      logs = logs.where(query, { address: "%#{params[:address]}%" })
+    end
+
     logs = logs.where("#{table_name}.email_type ILIKE ?", "%#{params[:type]}%") if params[
       :type
     ].present?

app/serializers/email_log_serializer.rb

@@ -3,10 +3,19 @@
 class EmailLogSerializer < ApplicationSerializer
   include EmailLogsMixin
 
-  attributes :reply_key, :bounced, :has_bounce_key, :smtp_transaction_response
+  attributes :cc_addresses,
+             :post_id,
+             :reply_key,
+             :bounced,
+             :has_bounce_key,
+             :smtp_transaction_response
 
   has_one :user, serializer: BasicUserSerializer, embed: :objects
 
+  def cc_addresses
+    return if object.cc_addresses.blank?
+    object.cc_addresses_split
+  end
   def include_reply_key?
     reply_keys = @options[:reply_keys]
     reply_keys.present? && reply_keys[[object.post_id, object.user_id]]

config/locales/client.en.yml

@@ -5605,6 +5605,9 @@ en:
             type_placeholder: "digest, signup…"
             reply_key_placeholder: "reply key"
             smtp_transaction_response_placeholder: "SMTP ID"
+          email_addresses:
+            see_more: "[See more...]"
+          post_id: "(Post ID: %{post_id})"
 
       moderation_history:
         performed_by: "Performed By"

spec/requests/admin/email_controller_spec.rb

@@ -57,7 +57,6 @@ RSpec.describe Admin::EmailController do
       before { sign_in(admin) }
 
       it "should return the right response" do
-        email_log
         get "/admin/email/sent.json"
 
         expect(response.status).to eq(200)
@@ -73,6 +72,8 @@ RSpec.describe Admin::EmailController do
         log = response.parsed_body.first
         expect(log["id"]).to eq(email_log.id)
         expect(log["reply_key"]).to eq(post_reply_key.reply_key)
+        expect(log["post_id"]).to eq(post.id)
+        expect(log["post_url"]).to eq(post.url)
       end
 
       it "should be able to filter by reply key" do
@@ -100,7 +101,7 @@ RSpec.describe Admin::EmailController do
 
       it "should be able to filter by smtp_transaction_response" do
         email_log_2 = Fabricate(:email_log, smtp_transaction_response: <<~RESPONSE)
-        250 Ok: queued as pYoKuQ1aUG5vdpgh-k2K11qcpF4C1ZQ5qmvmmNW25SM=@mailhog.example
+          250 Ok: queued as pYoKuQ1aUG5vdpgh-k2K11qcpF4C1ZQ5qmvmmNW25SM=@mailhog.example
         RESPONSE
 
         get "/admin/email/sent.json", params: { smtp_transaction_response: "pYoKu" }
@@ -112,6 +113,70 @@ RSpec.describe Admin::EmailController do
         expect(logs.size).to eq(1)
         expect(logs.first["smtp_transaction_response"]).to eq(email_log_2.smtp_transaction_response)
       end
+
+      context "when type is group_smtp and filter param is address" do
+        let(:email_type) { "group_smtp" }
+        let(:target_email) { user.email }
+
+        it "should be able to filter across both to address and cc addresses" do
+          other_email = "foo@bar.com"
+          another_email = "forty@two.com"
+          email_log_matching_to_address =
+            Fabricate(:email_log, to_address: target_email, email_type: email_type)
+          email_log_matching_cc_address =
+            Fabricate(
+              :email_log,
+              to_address: admin.email,
+              cc_addresses: "#{other_email};#{target_email};#{another_email}",
+              email_type: email_type,
+            )
+
+          get "/admin/email/sent.json", params: { address: target_email, type: email_type }
+
+          expect(response.status).to eq(200)
+          logs = response.parsed_body
+          expect(logs.size).to eq(2)
+          email_log_found_with_to_address =
+            logs.find { |log| log["id"] == email_log_matching_to_address.id }
+          expect(email_log_found_with_to_address["cc_addresses"]).to be_nil
+          expect(email_log_found_with_to_address["to_address"]).to eq target_email
+          email_log_found_with_cc_address =
+            logs.find { |log| log["id"] == email_log_matching_cc_address.id }
+          expect(email_log_found_with_cc_address["to_address"]).not_to eq target_email
+          expect(email_log_found_with_cc_address["cc_addresses"]).to contain_exactly(
+            target_email,
+            other_email,
+            another_email,
+          )
+        end
+      end
+
+      context "when type is not group_smtp and filter param is address" do
+        let(:target_email) { user.email }
+
+        it "should only filter within to address" do
+          other_email = "foo@bar.com"
+          another_email = "forty@two.com"
+          email_log_matching_to_address = Fabricate(:email_log, to_address: target_email)
+          email_log_matching_cc_address =
+            Fabricate(
+              :email_log,
+              to_address: admin.email,
+              cc_addresses: "#{other_email};#{target_email};#{another_email}",
+            )
+
+          get "/admin/email/sent.json", params: { address: target_email }
+
+          expect(response.status).to eq(200)
+          logs = response.parsed_body
+          expect(logs.size).to eq(1)
+          email_log_found_with_to_address =
+            logs.find { |log| log["id"] == email_log_matching_to_address.id }
+          expect(email_log_found_with_to_address["cc_addresses"]).to be_nil
+          expect(email_log_found_with_to_address["to_address"]).to eq target_email
+          expect(logs.find { |log| log["id"] == email_log_matching_cc_address.id }).to be_nil
+        end
+      end
     end
 
     shared_examples "sent emails inaccessible" do
@@ -619,18 +684,18 @@ RSpec.describe Admin::EmailController do
 
   describe "#advanced_test" do
     let(:email) { <<~EMAIL }
-          From: "somebody" <somebody@example.com>
+      From: "somebody" <somebody@example.com>
-          To: someone@example.com
+      To: someone@example.com
-          Date: Mon, 3 Dec 2018 00:00:00 -0000
+      Date: Mon, 3 Dec 2018 00:00:00 -0000
-          Subject: This is some subject
+      Subject: This is some subject
-          Content-Type: text/plain; charset="UTF-8"
+      Content-Type: text/plain; charset="UTF-8"
 
-          Hello, this is a test!
+      Hello, this is a test!
 
-          ---
+      ---
 
-          This part should be elided.
+      This part should be elided.
-        EMAIL
+    EMAIL
 
     context "when logged in as an admin" do
       before { sign_in(admin) }