Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions lib/workers/send_email_report.ex
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
defmodule Plausible.Workers.SendEmailReport do
use Plausible
use Plausible.Repo
use Oban.Worker, queue: :send_email_reports, max_attempts: 1

Expand All @@ -22,9 +23,9 @@ defmodule Plausible.Workers.SendEmailReport do
)
|> Repo.one()

report = site && Map.get(site, report_type)

if report do
with %Plausible.Site{} <- site,
%{} = report <- Map.get(site, report_type),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, makes sense. 2b9223f

true <- ok_to_send?(site) do
date_range = date_range(site, interval)
report_name = report_name(interval, date_range.first)
date_label = Calendar.strftime(date_range.last, "%-d %b %Y")
Expand All @@ -48,7 +49,7 @@ defmodule Plausible.Workers.SendEmailReport do
|> Plausible.Mailer.send()
end)
else
:discard
_ -> :discard
end
end

Expand Down Expand Up @@ -226,4 +227,13 @@ defmodule Plausible.Workers.SendEmailReport do

Date.range(first, last)
end

on_ee do
defp ok_to_send?(site) do
not Plausible.Sites.consolidated?(site) or
Plausible.ConsolidatedView.ok_to_display?(site.team)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something verbose but expressive like:

Suggested change
not Plausible.Sites.consolidated?(site) or
Plausible.ConsolidatedView.ok_to_display?(site.team)
Plausible.Sites.regular?(site) or
(Plausible.Sites.consolidated?(site) and Plausible.ConsolidatedView.ok_to_display?(site.team))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end
else
defp ok_to_send?(_site), do: always(true)
end
end
48 changes: 30 additions & 18 deletions lib/workers/traffic_change_notifier.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ defmodule Plausible.Workers.TrafficChangeNotifier do
@moduledoc """
Oban service sending out traffic drop/spike notifications
"""
use Plausible
use Plausible.Repo
alias Plausible.Stats.{Query, Clickhouse}
alias Plausible.Site.TrafficChangeNotification
Expand All @@ -28,30 +29,32 @@ defmodule Plausible.Workers.TrafficChangeNotifier do
preload: [site: {s, team: t}]
)

for notification <- notifications do
case notification.type do
:spike ->
current_visitors = Clickhouse.current_visitors(notification.site)
for notification <- notifications, ok_to_send?(notification.site) do
handle_notification(notification, now)
end

if current_visitors >= notification.threshold do
stats =
notification.site
|> get_traffic_spike_stats()
|> Map.put(:current_visitors, current_visitors)
:ok
end

notify_spike(notification, stats, now)
end
defp handle_notification(%TrafficChangeNotification{type: :spike} = notification, now) do
current_visitors = Clickhouse.current_visitors(notification.site)

:drop ->
current_visitors = Clickhouse.current_visitors_12h(notification.site)
if current_visitors >= notification.threshold do
stats =
notification.site
|> get_traffic_spike_stats()
|> Map.put(:current_visitors, current_visitors)

if current_visitors < notification.threshold do
notify_drop(notification, current_visitors, now)
end
end
notify_spike(notification, stats, now)
end
end

:ok
defp handle_notification(%TrafficChangeNotification{type: :drop} = notification, now) do
current_visitors = Clickhouse.current_visitors_12h(notification.site)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh, does this already work with multiple site ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. #5876 made it work.


if current_visitors < notification.threshold do
notify_drop(notification, current_visitors, now)
end
end

defp notify_spike(notification, stats, now) do
Expand Down Expand Up @@ -175,4 +178,13 @@ defmodule Plausible.Workers.TrafficChangeNotifier do
)
|> Repo.exists?()
end

on_ee do
defp ok_to_send?(site) do
not Plausible.Sites.consolidated?(site) or
Plausible.ConsolidatedView.ok_to_display?(site.team)
end
else
defp ok_to_send?(_site), do: always(true)
end
end
38 changes: 38 additions & 0 deletions test/workers/send_email_report_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,25 @@ defmodule Plausible.Workers.SendEmailReportTest do
assert text_of_element(html_body, ".page-name") == "/"
assert text_of_element(html_body, ".page-count") == "2"
end

test "does not send weekly report for consolidated view with ok_to_display? false" do
{:ok, team} = new_user() |> Plausible.Teams.get_or_create()

site = new_site(team: team)
new_site(team: team)

consolidated_view = new_consolidated_view(team)

insert(:weekly_report, site: consolidated_view, recipients: ["[email protected]"])

Plausible.Repo.delete!(site)

refute Plausible.ConsolidatedView.ok_to_display?(team)

perform_job(SendEmailReport, %{"site_id" => consolidated_view.id, "interval" => "weekly"})

assert_no_emails_delivered()
end
end

test "renders correct signs (+/-) and trend colors for positive percentage changes" do
Expand Down Expand Up @@ -540,6 +559,25 @@ defmodule Plausible.Workers.SendEmailReportTest do
assert goal_names == ["Signup", "Purchase", "Visit /thank-you"]
assert goal_conversions == ["2", "1", "1"]
end

test "does not send monthly report for consolidated view with ok_to_display? false" do
{:ok, team} = new_user() |> Plausible.Teams.get_or_create()

site = new_site(team: team)
new_site(team: team)

consolidated_view = new_consolidated_view(team)

insert(:monthly_report, site: consolidated_view, recipients: ["[email protected]"])

Plausible.Repo.delete!(site)

refute Plausible.ConsolidatedView.ok_to_display?(site.team)

perform_job(SendEmailReport, %{"site_id" => consolidated_view.id, "interval" => "monthly"})

assert_no_emails_delivered()
end
end
end
end
58 changes: 58 additions & 0 deletions test/workers/traffic_change_notifier_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,29 @@ defmodule Plausible.Workers.TrafficChangeNotifierTest do
assert html_body =~ @view_dashboard_text
refute html_body =~ @review_installation_text
end

test "does not notify traffic drop on consolidated view when ok_to_display? is false" do
user = new_user()
{:ok, team} = Plausible.Teams.get_or_create(user)
site = new_site(team: team)
new_site(team: team)

consolidated_view = new_consolidated_view(team)

Plausible.Repo.delete(site)

refute Plausible.ConsolidatedView.ok_to_display?(team)

insert(:drop_notification,
site: consolidated_view,
threshold: 10,
recipients: [user.email]
)

TrafficChangeNotifier.perform(nil)

assert_no_emails_delivered()
end
end

test "does not send notifications more than once every 12 hours" do
Expand Down Expand Up @@ -283,6 +306,41 @@ defmodule Plausible.Workers.TrafficChangeNotifierTest do
assert html_body =~ "<b>2</b> visitors on <b>/a</b>"
assert html_body =~ "<b>1</b> visitor on <b>/b</b>"
end

test "does not notify traffic spike on consolidated view when ok_to_display? is false" do
user = new_user()
{:ok, team} = Plausible.Teams.get_or_create(user)
site1 = new_site(team: team)
site2 = new_site(team: team)

consolidated_view = new_consolidated_view(team)

insert(:spike_notification,
site: consolidated_view,
threshold: 2,
recipients: ["[email protected]"]
)

populate_stats(site1, [
build(:pageview, timestamp: minutes_ago(1)),
build(:pageview, timestamp: minutes_ago(2)),
build(:pageview, timestamp: minutes_ago(3))
])

Plausible.Repo.delete(site2)

refute Plausible.ConsolidatedView.ok_to_display?(team)

insert(:drop_notification,
site: consolidated_view,
threshold: 10,
recipients: [user.email]
)

TrafficChangeNotifier.perform(nil)

assert_no_emails_delivered()
end
end

test "ignores 'Direct / None' source" do
Expand Down
Loading