Small bugfix + refactor email reports (#3642)

* use more convenient testing functions

* do not display + sign with 0% change in emails

* Rename module/file/function names

before, `weekly_report` was also used for monthly reports and that was a
bit confusing to read in code.

* Refactor send_email_report.ex

This commit improves readability by refactoring the code into smaller
functions and reducing the number of arguments given to functions.

But more importantly, it stops making duplicate stats queries for every
email recipient by moving the queries out of the for loop.

* Refactor: move querying logic out of the worker module

and merge all stats information under a single `stats` assign.

lib/plausible/stats/compare.ex

@@ -1,15 +1,9 @@
 defmodule Plausible.Stats.Compare do
-  def calculate_change(:bounce_rate, old_stats, new_stats) do
-    old_count = old_stats[:bounce_rate][:value]
-    new_count = new_stats[:bounce_rate][:value]
-
+  def calculate_change(:bounce_rate, old_count, new_count) do
     if old_count > 0, do: new_count - old_count
   end
 
-  def calculate_change(metric, old_stats, new_stats) do
-    old_count = old_stats[metric][:value]
-    new_count = new_stats[metric][:value]
-
+  def calculate_change(_metric, old_count, new_count) do
     percent_change(old_count, new_count)
   end
 

lib/plausible/stats/email_report.ex

@@ -0,0 +1,53 @@
+defmodule Plausible.Stats.EmailReport do
+  @moduledoc """
+  This module exposes a `get/2` function that returns a map
+  of stats needed for email reports. These stats include:
+
+  * Total pageviews
+  * Unique visitors
+  * Bounce rate
+  * A list of Top 5 sources (excluding "Direct / None")
+  * A list of Top 5 pages
+
+  where total pageviews, unique visitors, and bounce rate
+  also include the change compared to previous period.
+  """
+
+  alias Plausible.Stats
+  alias Plausible.Stats.{Query, Compare, Comparisons}
+
+  def get(site, query) do
+    metrics = [:pageviews, :visitors, :bounce_rate]
+
+    Stats.aggregate(site, query, metrics)
+    |> with_comparisons(site, query, metrics)
+    |> put_top_5_pages(site, query)
+    |> put_top_5_sources(site, query)
+  end
+
+  defp with_comparisons(stats, site, query, metrics) do
+    {:ok, prev_query} = Comparisons.compare(site, query, "previous_period")
+    prev_period_stats = Stats.aggregate(site, prev_query, metrics)
+
+    stats
+    |> Enum.map(fn {metric, %{value: value}} ->
+      %{value: prev_value} = Map.fetch!(prev_period_stats, metric)
+      change = Compare.calculate_change(metric, prev_value, value)
+
+      {metric, %{value: value, change: change}}
+    end)
+    |> Enum.into(%{})
+  end
+
+  defp put_top_5_pages(stats, site, query) do
+    pages = Stats.breakdown(site, query, "event:page", [:visitors], {5, 1})
+    Map.put(stats, :pages, pages)
+  end
+
+  defp put_top_5_sources(stats, site, query) do
+    query = Query.put_filter(query, "visit:source", {:is_not, "Direct / None"})
+    sources = Stats.breakdown(site, query, "visit:source", [:visitors], {5, 1})
+
+    Map.put(stats, :sources, sources)
+  end
+end

lib/plausible_web/controllers/api/stats_controller.ex

@@ -447,7 +447,7 @@ defmodule PlausibleWeb.Api.StatsController do
 
       if prev_results do
         prev_value = get_in(prev_results, [key, :value])
-        change = calculate_change(key, prev_value, value)
+        change = Stats.Compare.calculate_change(key, prev_value, value)
 
         %{
           name: name,
@@ -461,14 +461,6 @@ defmodule PlausibleWeb.Api.StatsController do
     end
   end
 
-  def calculate_change(:bounce_rate, old_count, new_count) do
-    if old_count > 0, do: new_count - old_count
-  end
-
-  def calculate_change(_metric, old_count, new_count) do
-    Stats.Compare.percent_change(old_count, new_count)
-  end
-
   def sources(conn, params) do
     site = conn.assigns[:site]
 

lib/plausible_web/email.ex

@@ -116,14 +116,12 @@ defmodule PlausibleWeb.Email do
     |> render("trial_over_email.html", user: user)
   end
 
-  def weekly_report(email, site, assigns) do
-    assigns = Keyword.put(assigns, :site, site)
-
+  def stats_report(email, assigns) do
     base_email(%{layout: nil})
     |> to(email)
-    |> tag("weekly-report")
-    |> subject("#{assigns[:name]} report for #{site.domain}")
-    |> html_body(PlausibleWeb.MJML.WeeklyReport.render(assigns))
+    |> tag("#{assigns.type}-report")
+    |> subject("#{assigns.name} report for #{assigns.site.domain}")
+    |> html_body(PlausibleWeb.MJML.StatsReport.render(assigns))
   end
 
   def spike_notification(email, site, current_visitors, sources, dashboard_link) do

lib/plausible_web/mjml/stats_report.ex

@@ -0,0 +1,7 @@
+defmodule PlausibleWeb.MJML.StatsReport do
+  @moduledoc """
+  MJML rendered for the weekly or monthly report e-mail
+  """
+
+  use MjmlEEx, mjml_template: "templates/stats_report.mjml.eex"
+end

lib/plausible_web/mjml/templates/stats_report.mjml.eex

@@ -37,18 +37,18 @@
       <mj-column>
         <mj-text mj-class="text-label">UNIQUE VISITORS</mj-text>
         <mj-text mj-class="text-lg" css-class="visitors">
-          <%= PlausibleWeb.StatsView.large_number_format(@unique_visitors) %>
+          <%= PlausibleWeb.StatsView.large_number_format(@stats.visitors.value) %>
         </mj-text>
         <%= cond do %>
-          <% @change_visitors == nil -> %>
-            <mj-text mj-class="text-sm">N/A</mj-text>
-          <% @change_visitors >= 0 -> %>
-            <mj-text mj-class="text-sm trend-good">
-            +<%= @change_visitors %>%
+          <% @stats.visitors.change == nil -> %>
+            <mj-text css-class="change-visitors" mj-class="text-sm">N/A</mj-text>
+          <% @stats.visitors.change >= 0 -> %>
+            <mj-text css-class="change-visitors" mj-class="text-sm trend-good">
+            <%= "#{if @stats.visitors.change != 0, do: "+"}#{@stats.visitors.change}%" %>
             </mj-text>
-          <% @change_visitors < 0 -> %>
-            <mj-text mj-class="text-sm trend-bad">
-            <%= @change_visitors %>%
+          <% @stats.visitors.change < 0 -> %>
+            <mj-text css-class="change-visitors" mj-class="text-sm trend-bad">
+            <%= @stats.visitors.change %>%
             </mj-text>
         <% end %>
         <mj-spacer height="30px" />
@@ -56,18 +56,18 @@
       <mj-column>
         <mj-text mj-class="text-label">PAGEVIEWS</mj-text>
         <mj-text mj-class="text-lg" css-class="pageviews">
-          <%= PlausibleWeb.StatsView.large_number_format(@pageviews) %>
+          <%= PlausibleWeb.StatsView.large_number_format(@stats.pageviews.value) %>
         </mj-text>
         <%= cond do %>
-          <% is_nil(@change_pageviews) -> %>
-            <mj-text mj-class="text-sm">N/A</mj-text>
-          <% @change_pageviews >= 0 -> %>
-            <mj-text mj-class="text-sm trend-good">
-            +<%= @change_pageviews %>%
+          <% is_nil(@stats.pageviews.change) -> %>
+            <mj-text css-class="change-pageviews" mj-class="text-sm">N/A</mj-text>
+          <% @stats.pageviews.change >= 0 -> %>
+            <mj-text css-class="change-pageviews" mj-class="text-sm trend-good">
+            <%= "#{if @stats.pageviews.change != 0, do: "+"}#{@stats.pageviews.change}%" %>
             </mj-text>
-          <% @change_pageviews < 0 -> %>
-            <mj-text mj-class="text-sm trend-bad">
-            <%= @change_pageviews %>%
+          <% @stats.pageviews.change < 0 -> %>
+            <mj-text css-class="change-pageviews" mj-class="text-sm trend-bad">
+            <%= @stats.pageviews.change %>%
             </mj-text>
         <% end %>
         <mj-spacer height="30px" />
@@ -76,19 +76,19 @@
       <mj-column>
         <mj-text mj-class="text-label">BOUNCE RATE</mj-text>
         <mj-text mj-class="text-lg">
-          <%= @bounce_rate %>
+          <%= @stats.bounce_rate.value %>
         </mj-text>
 
         <%= cond do %>
-          <% @change_bounce_rate == nil -> %>
-            <mj-text mj-class="text-sm">N/A</mj-text>
-          <% @change_bounce_rate <= 0 -> %>
-            <mj-text mj-class="text-sm trend-good">
-            <%= @change_bounce_rate %>%
+          <% @stats.bounce_rate.change == nil -> %>
+            <mj-text css-class="change-bounce-rate" mj-class="text-sm">N/A</mj-text>
+          <% @stats.bounce_rate.change <= 0 -> %>
+            <mj-text css-class="change-bounce-rate" mj-class="text-sm trend-good">
+            <%= @stats.bounce_rate.change %>%
             </mj-text>
-          <% @change_bounce_rate > 0 -> %>
-            <mj-text mj-class="text-sm trend-bad">
-            +<%= @change_bounce_rate %>%
+          <% @stats.bounce_rate.change > 0 -> %>
+            <mj-text css-class="change-bounce-rate" mj-class="text-sm trend-bad">
+            +<%= @stats.bounce_rate.change %>%
             </mj-text>
         <% end %>
         <mj-spacer height="30px" />
@@ -106,7 +106,7 @@
         <mj-column>
           <mj-text font-weight="bold">Referrer</mj-text>
 
-          <%= for source <- @sources do %>
+          <%= for source <- @stats.sources do %>
           <mj-text css-class="referrer-name">
           <%= source[:source] %>
           </mj-text>
@@ -115,7 +115,7 @@
         <mj-column>
           <mj-text font-weight="bold" align="right">Visitors</mj-text>
 
-          <%= for source <- @sources do %>
+          <%= for source <- @stats.sources do %>
           <mj-text align="right" css-class="referrer-count">
           <%= PlausibleWeb.StatsView.large_number_format(source[:visitors]) %>
           </mj-text>
@@ -137,7 +137,7 @@
             Page
           </mj-text>
 
-          <%= for page <- @pages do %>
+          <%= for page <- @stats.pages do %>
           <mj-text css-class="page-name">
             <%= page[:page] %>
           </mj-text>
@@ -148,7 +148,7 @@
             Visitors
           </mj-text>
 
-          <%= for page <- @pages do %>
+          <%= for page <- @stats.pages do %>
             <mj-text align="right" css-class="page-count">
             <%= PlausibleWeb.StatsView.large_number_format(page[:visitors]) %>
             </mj-text>

lib/plausible_web/mjml/weekly_report.ex

@@ -1,7 +0,0 @@
-defmodule PlausibleWeb.MJML.WeeklyReport do
-  @moduledoc """
-  MJML rendered for the weekly report e-mail
-  """
-
-  use MjmlEEx, mjml_template: "templates/weekly_report.mjml.eex"
-end

lib/workers/send_email_report.ex

@@ -2,26 +2,18 @@ defmodule Plausible.Workers.SendEmailReport do
   use Plausible.Repo
   use Oban.Worker, queue: :send_email_reports, max_attempts: 1
   alias Plausible.Stats.Query
-  alias Plausible.Stats
 
   @impl Oban.Worker
   def perform(%Oban.Job{args: %{"interval" => "weekly", "site_id" => site_id}}) do
     site = Repo.get(Plausible.Site, site_id) |> Repo.preload(:weekly_report)
 
     if site do
-      today = Timex.now(site.timezone) |> DateTime.to_date()
-      date = Timex.shift(today, weeks: -1) |> Timex.end_of_week() |> Date.to_iso8601()
-      query = Query.from(site, %{"period" => "7d", "date" => date})
-
-      for email <- site.weekly_report.recipients do
-        unsubscribe_link =
-          PlausibleWeb.Endpoint.url() <>
-            "/sites/#{URI.encode_www_form(site.domain)}/weekly-report/unsubscribe?email=#{email}"
-
-        send_report(email, site, "Weekly", unsubscribe_link, query)
-      end
-
-      :ok
+      %{site: site}
+      |> Map.put(:type, :weekly)
+      |> Map.put(:name, "Weekly")
+      |> put_last_week_query()
+      |> put_stats()
+      |> send_report_for_all(site.weekly_report.recipients)
     else
       :discard
     end
@@ -32,62 +24,63 @@ defmodule Plausible.Workers.SendEmailReport do
     site = Repo.get(Plausible.Site, site_id) |> Repo.preload(:monthly_report)
 
     if site do
-      last_month =
-        Timex.now(site.timezone)
-        |> Timex.shift(months: -1)
-        |> Timex.beginning_of_month()
-
-      query =
-        Query.from(site, %{
-          "period" => "month",
-          "date" => Timex.format!(last_month, "{ISOdate}")
-        })
-
-      for email <- site.monthly_report.recipients do
-        unsubscribe_link =
-          PlausibleWeb.Endpoint.url() <>
-            "/sites/#{URI.encode_www_form(site.domain)}/monthly-report/unsubscribe?email=#{email}"
-
-        send_report(email, site, Timex.format!(last_month, "{Mfull}"), unsubscribe_link, query)
-      end
-
-      :ok
+      %{site: site}
+      |> Map.put(:type, :monthly)
+      |> put_last_month_query()
+      |> put_monthly_report_name()
+      |> put_stats()
+      |> send_report_for_all(site.monthly_report.recipients)
     else
       :discard
     end
   end
 
-  defp send_report(email, site, name, unsubscribe_link, query) do
-    {:ok, prev_query} = Stats.Comparisons.compare(site, query, "previous_period")
-    curr_period = Stats.aggregate(site, query, [:pageviews, :visitors, :bounce_rate])
-    prev_period = Stats.aggregate(site, prev_query, [:pageviews, :visitors, :bounce_rate])
+  defp send_report_for_all(_assigns, [] = _recipients), do: :ok
 
-    change_pageviews = Stats.Compare.calculate_change(:pageviews, prev_period, curr_period)
-    change_visitors = Stats.Compare.calculate_change(:visitors, prev_period, curr_period)
-    change_bounce_rate = Stats.Compare.calculate_change(:bounce_rate, prev_period, curr_period)
+  defp send_report_for_all(assigns, [email | rest]) do
+    unsubscribe_link =
+      PlausibleWeb.Endpoint.url() <>
+        "/sites/#{URI.encode_www_form(assigns.site.domain)}/#{assigns.type}-report/unsubscribe?email=#{email}"
 
-    source_query = Query.put_filter(query, "visit:source", {:is_not, "Direct / None"})
-    sources = Stats.breakdown(site, source_query, "visit:source", [:visitors], {5, 1})
-    pages = Stats.breakdown(site, query, "event:page", [:visitors], {5, 1})
     user = Plausible.Auth.find_user_by(email: email)
-    login_link = user && Plausible.Sites.is_member?(user.id, site)
+    login_link = user && Plausible.Sites.is_member?(user.id, assigns.site)
 
-    template =
-      PlausibleWeb.Email.weekly_report(email, site,
-        unique_visitors: curr_period[:visitors][:value],
-        change_visitors: change_visitors,
-        pageviews: curr_period[:pageviews][:value],
-        change_pageviews: change_pageviews,
-        bounce_rate: curr_period[:bounce_rate][:value],
-        change_bounce_rate: change_bounce_rate,
-        sources: sources,
-        unsubscribe_link: unsubscribe_link,
-        login_link: login_link,
-        pages: pages,
-        query: query,
-        name: name
-      )
+    template_assigns =
+      assigns
+      |> Map.put(:unsubscribe_link, unsubscribe_link)
+      |> Map.put(:login_link, login_link)
 
-    Plausible.Mailer.send(template)
+    PlausibleWeb.Email.stats_report(email, template_assigns)
+    |> Plausible.Mailer.send()
+
+    send_report_for_all(assigns, rest)
+  end
+
+  defp put_last_month_query(%{site: site} = assigns) do
+    last_month =
+      Timex.now(site.timezone)
+      |> Timex.shift(months: -1)
+      |> Timex.beginning_of_month()
+      |> Timex.format!("{ISOdate}")
+
+    query = Query.from(site, %{"period" => "month", "date" => last_month})
+
+    Map.put(assigns, :query, query)
+  end
+
+  defp put_last_week_query(%{site: site} = assigns) do
+    today = Timex.now(site.timezone) |> DateTime.to_date()
+    date = Timex.shift(today, weeks: -1) |> Timex.end_of_week() |> Date.to_iso8601()
+    query = Query.from(site, %{"period" => "7d", "date" => date})
+
+    Map.put(assigns, :query, query)
+  end
+
+  defp put_monthly_report_name(%{query: query} = assigns) do
+    Map.put(assigns, :name, Timex.format!(query.date_range.first, "{Mfull}"))
+  end
+
+  defp put_stats(%{site: site, query: query} = assigns) do
+    Map.put(assigns, :stats, Plausible.Stats.EmailReport.get(site, query))
   end
 end

test/workers/send_email_report_test.exs

@@ -2,9 +2,13 @@ defmodule Plausible.Workers.SendEmailReportTest do
   use Plausible.DataCase
   use Bamboo.Test
   use Oban.Testing, repo: Plausible.Repo
+  import Plausible.Test.Support.HTML
   alias Plausible.Workers.SendEmailReport
   alias Timex.Timezone
 
+  @green "#15803d"
+  @red "#b91c1c"
+
   describe "weekly reports" do
     test "sends weekly report to all recipients" do
       site = insert(:site, domain: "test-site.com", timezone: "US/Eastern")
@@ -65,9 +69,7 @@ defmodule Plausible.Workers.SendEmailReportTest do
       })
 
       # Should find 2 visiors
-
-      page_count = html_body |> Floki.find(".page-count") |> Floki.text() |> String.trim()
-      assert page_count == "2"
+      assert text_of_element(html_body, ".page-count") == "2"
     end
 
     test "includes the correct stats" do
@@ -92,33 +94,115 @@ defmodule Plausible.Workers.SendEmailReportTest do
         html_body: html_body
       })
 
-      {:ok, document} = Floki.parse_document(html_body)
+      assert text_of_element(html_body, ".visitors") == "2"
+      assert text_of_element(html_body, ".pageviews") == "3"
+      assert text_of_element(html_body, ".referrer-name") == "Google"
+      assert text_of_element(html_body, ".referrer-count") == "1"
+      assert text_of_element(html_body, ".page-name") == "/"
+      assert text_of_element(html_body, ".page-count") == "2"
+    end
 
-      visitors =
-        Floki.find(document, ".visitors")
-        |> List.first()
-        |> Floki.text()
-        |> String.trim()
+    test "renders correct signs (+/-) and trend colors for positive percentage changes" do
+      now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second)
+      week_ago = now |> Timex.shift(days: -7)
+      two_weeks_ago = now |> Timex.shift(days: -14)
 
-      assert visitors == "2"
+      site = insert(:site, inserted_at: Timex.shift(now, days: -15))
+      insert(:weekly_report, site: site, recipients: ["user@email.com"])
 
-      pageviews = Floki.find(document, ".pageviews") |> Floki.text() |> String.trim()
-      assert pageviews == "3"
+      populate_stats(site, [
+        build(:pageview, timestamp: two_weeks_ago),
+        build(:pageview, user_id: 1, timestamp: week_ago),
+        build(:pageview, user_id: 2, timestamp: week_ago),
+        build(:pageview, user_id: 2, timestamp: week_ago)
+      ])
 
-      referrer_name =
-        document |> Floki.find(".referrer-name") |> List.first() |> Floki.text() |> String.trim()
+      perform_job(SendEmailReport, %{"site_id" => site.id, "interval" => "weekly"})
 
-      referrer_count =
-        document |> Floki.find(".referrer-count") |> List.first() |> Floki.text() |> String.trim()
+      assert_delivered_email_matches(%{
+        to: [nil: "user@email.com"],
+        html_body: html_body
+      })
 
-      assert referrer_name == "Google"
-      assert referrer_count == "1"
+      visitors_change_container = find(html_body, ".change-visitors div")
+      assert text(visitors_change_container) == "+100%"
+      assert text_of_attr(visitors_change_container, "style") =~ @green
 
-      page_name = document |> Floki.find(".page-name") |> Floki.text() |> String.trim()
-      page_count = document |> Floki.find(".page-count") |> Floki.text() |> String.trim()
+      pageviews_change_container = find(html_body, ".change-pageviews div")
+      assert text(pageviews_change_container) == "+200%"
+      assert text_of_attr(pageviews_change_container, "style") =~ @green
 
-      assert page_name == "/"
-      assert page_count == "2"
+      bounce_rate_change_container = find(html_body, ".change-bounce-rate div")
+      assert text(bounce_rate_change_container) == "-50%"
+      assert text_of_attr(bounce_rate_change_container, "style") =~ @green
+    end
+
+    test "renders correct signs (+/-) and trend colors for negative percentage changes" do
+      now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second)
+      week_ago = now |> Timex.shift(days: -7)
+      two_weeks_ago = now |> Timex.shift(days: -14)
+
+      site = insert(:site, inserted_at: Timex.shift(now, days: -15))
+      insert(:weekly_report, site: site, recipients: ["user@email.com"])
+
+      populate_stats(site, [
+        build(:pageview, user_id: 1, timestamp: two_weeks_ago),
+        build(:pageview, user_id: 2, timestamp: two_weeks_ago),
+        build(:pageview, user_id: 2, timestamp: two_weeks_ago),
+        build(:pageview, timestamp: week_ago)
+      ])
+
+      perform_job(SendEmailReport, %{"site_id" => site.id, "interval" => "weekly"})
+
+      assert_delivered_email_matches(%{
+        to: [nil: "user@email.com"],
+        html_body: html_body
+      })
+
+      visitors_change_container = find(html_body, ".change-visitors div")
+      assert text(visitors_change_container) == "-50%"
+      assert text_of_attr(visitors_change_container, "style") =~ @red
+
+      pageviews_change_container = find(html_body, ".change-pageviews div")
+      assert text(pageviews_change_container) == "-67%"
+      assert text_of_attr(pageviews_change_container, "style") =~ @red
+
+      bounce_rate_change_container = find(html_body, ".change-bounce-rate div")
+      assert text(bounce_rate_change_container) == "+50%"
+      assert text_of_attr(bounce_rate_change_container, "style") =~ @red
+    end
+
+    test "renders 0% changes with a green color and without a sign" do
+      now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second)
+      week_ago = now |> Timex.shift(days: -7)
+      two_weeks_ago = now |> Timex.shift(days: -14)
+
+      site = insert(:site, inserted_at: Timex.shift(now, days: -15))
+      insert(:weekly_report, site: site, recipients: ["user@email.com"])
+
+      populate_stats(site, [
+        build(:pageview, timestamp: two_weeks_ago),
+        build(:pageview, timestamp: week_ago)
+      ])
+
+      perform_job(SendEmailReport, %{"site_id" => site.id, "interval" => "weekly"})
+
+      assert_delivered_email_matches(%{
+        to: [nil: "user@email.com"],
+        html_body: html_body
+      })
+
+      visitors_change_container = find(html_body, ".change-visitors div")
+      assert text(visitors_change_container) == "0%"
+      assert text_of_attr(visitors_change_container, "style") =~ @green
+
+      pageviews_change_container = find(html_body, ".change-pageviews div")
+      assert text(pageviews_change_container) == "0%"
+      assert text_of_attr(pageviews_change_container, "style") =~ @green
+
+      bounce_rate_change_container = find(html_body, ".change-bounce-rate div")
+      assert text(bounce_rate_change_container) == "0%"
+      assert text_of_attr(bounce_rate_change_container, "style") =~ @green
     end
   end