-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Prevent email reports when consolidated view ineligible #5882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
f11bb40
7c29431
f4cf56c
2b9223f
a5f454b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||||||||||
|
|
||||||||||
|
|
@@ -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), | ||||||||||
| 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") | ||||||||||
|
|
@@ -48,7 +49,7 @@ defmodule Plausible.Workers.SendEmailReport do | |||||||||
| |> Plausible.Mailer.send() | ||||||||||
| end) | ||||||||||
| else | ||||||||||
| :discard | ||||||||||
| _ -> :discard | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
|
|
@@ -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) | ||||||||||
|
||||||||||
| 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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 | ||
RobertJoonas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooh, does this already work with multiple site ids?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
@@ -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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch!?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, makes sense. 2b9223f