From 44816c69dd558f634a011d24eeb0519c97e80dd7 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Tue, 2 Jul 2024 11:57:19 +0200 Subject: [PATCH] Fixup --- .../controllers/site_controller.ex | 20 +- lib/plausible_web/router.ex | 14 +- .../site/settings_email_reports.html.heex | 39 ++-- .../templates/site/traffic_change_form.heex | 2 +- ...40702055817_traffic_drop_notifications.exs | 1 - .../controllers/site_controller_test.exs | 180 +++++++++++------- 6 files changed, 155 insertions(+), 101 deletions(-) diff --git a/lib/plausible_web/controllers/site_controller.ex b/lib/plausible_web/controllers/site_controller.ex index 351bbf67295f..f895a237a54b 100644 --- a/lib/plausible_web/controllers/site_controller.ex +++ b/lib/plausible_web/controllers/site_controller.ex @@ -484,7 +484,7 @@ defmodule PlausibleWeb.SiteController do %{ site_id: site.id, type: type, - threshold: (if type == "spike", do: 10, else: 1), + threshold: if(type == "spike", do: 10, else: 1), recipients: [conn.assigns[:current_user].email] } ) @@ -507,7 +507,9 @@ defmodule PlausibleWeb.SiteController do site = conn.assigns[:site] Repo.delete_all( - from(mr in Plausible.Site.TrafficChangeNotification, where: mr.site_id == ^site.id and mr.type == ^type) + from(mr in Plausible.Site.TrafficChangeNotification, + where: mr.site_id == ^site.id and mr.type == ^type + ) ) conn @@ -515,9 +517,14 @@ defmodule PlausibleWeb.SiteController do |> redirect(external: Routes.site_path(conn, :settings_email_reports, site.domain)) end - def update_traffic_change_notification(conn, %{"traffic_change_notification" => params, "type" => type}) do + def update_traffic_change_notification(conn, %{ + "traffic_change_notification" => params, + "type" => type + }) do site = conn.assigns[:site] - notification = Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id, type: type) + + notification = + Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id, type: type) Plausible.Site.TrafficChangeNotification.changeset(notification, params) |> Repo.update!() @@ -539,7 +546,10 @@ defmodule PlausibleWeb.SiteController do |> redirect(external: Routes.site_path(conn, :settings_email_reports, site.domain)) end - def remove_traffic_change_notification_recipient(conn, %{"recipient" => recipient, "type" => type}) do + def remove_traffic_change_notification_recipient(conn, %{ + "recipient" => recipient, + "type" => type + }) do site = conn.assigns[:site] Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id, type: type) diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 5bdbf6794037..a9e52c06d4f5 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -322,9 +322,17 @@ defmodule PlausibleWeb.Router do SiteController, :remove_monthly_report_recipient - post "/sites/:website/traffic-change-notification/:type/enable", SiteController, :enable_traffic_change_notification - post "/sites/:website/traffic-change-notification/:type/disable", SiteController, :disable_traffic_change_notification - put "/sites/:website/traffic-change-notification/:type", SiteController, :update_traffic_change_notification + post "/sites/:website/traffic-change-notification/:type/enable", + SiteController, + :enable_traffic_change_notification + + post "/sites/:website/traffic-change-notification/:type/disable", + SiteController, + :disable_traffic_change_notification + + put "/sites/:website/traffic-change-notification/:type", + SiteController, + :update_traffic_change_notification post "/sites/:website/traffic-change-notification/:type/recipients", SiteController, diff --git a/lib/plausible_web/templates/site/settings_email_reports.html.heex b/lib/plausible_web/templates/site/settings_email_reports.html.heex index c7cefe257cd6..8398e981ccaf 100644 --- a/lib/plausible_web/templates/site/settings_email_reports.html.heex +++ b/lib/plausible_web/templates/site/settings_email_reports.html.heex @@ -195,25 +195,24 @@ <% end %> -<%= render("traffic_change_form", -conn: @conn, -notification: @spike_notification, -site: @site, -heading: "Traffic Spike Notifications", -subtitle: "Get notified when your site has unusually high number of current visitors", -toggle_text: "Send notifications of traffic spikes", -threshold_label: "Current visitor threshold", -type: :spike +<%= render("traffic_change_form", + conn: @conn, + notification: @spike_notification, + site: @site, + heading: "Traffic Spike Notifications", + subtitle: "Get notified when your site has unusually high number of current visitors", + toggle_text: "Send notifications of traffic spikes", + threshold_label: "Current visitor threshold", + type: :spike ) %> - -<%= render("traffic_change_form", - conn: @conn, - notification: @drop_notification, - site: @site, - heading: "Traffic Drop Notifications", - subtitle: "Get notified when your site has unusually low number of visitors within 12 hours", - toggle_text: "Send notifications of traffic drops", - threshold_label: "12 hours visitor threshold", - type: :drop - ) %> +<%= render("traffic_change_form", + conn: @conn, + notification: @drop_notification, + site: @site, + heading: "Traffic Drop Notifications", + subtitle: "Get notified when your site has unusually low number of visitors within 12 hours", + toggle_text: "Send notifications of traffic drops", + threshold_label: "12 hours visitor threshold", + type: :drop +) %> diff --git a/lib/plausible_web/templates/site/traffic_change_form.heex b/lib/plausible_web/templates/site/traffic_change_form.heex index 2778b722027c..f5df69b7e509 100644 --- a/lib/plausible_web/templates/site/traffic_change_form.heex +++ b/lib/plausible_web/templates/site/traffic_change_form.heex @@ -44,7 +44,7 @@ <%= number_input(f, :threshold, - min: 0, + min: 0, class: "focus:ring-indigo-500 dark:bg-gray-900 focus:border-indigo-500 block w-full rounded-none rounded-l-md pl-10 sm:text-sm border-gray-300 dark:border-gray-500 dark:text-gray-100" ) %> diff --git a/priv/repo/migrations/20240702055817_traffic_drop_notifications.exs b/priv/repo/migrations/20240702055817_traffic_drop_notifications.exs index 0709e813a902..d0ed53effb96 100644 --- a/priv/repo/migrations/20240702055817_traffic_drop_notifications.exs +++ b/priv/repo/migrations/20240702055817_traffic_drop_notifications.exs @@ -9,6 +9,5 @@ defmodule Plausible.Repo.Migrations.TrafficDropNotifications do end create unique_index(:spike_notifications, [:site_id, :type]) - end end diff --git a/test/plausible_web/controllers/site_controller_test.exs b/test/plausible_web/controllers/site_controller_test.exs index 20e69c98ad2b..2d4ac6d7382f 100644 --- a/test/plausible_web/controllers/site_controller_test.exs +++ b/test/plausible_web/controllers/site_controller_test.exs @@ -1229,109 +1229,147 @@ defmodule PlausibleWeb.SiteControllerTest do end for type <- [:spike, :drop] do - describe "POST /sites/:website/traffic-change-notification/#{type}/enable" do - setup [:create_user, :log_in, :create_site] + describe "POST /sites/:website/traffic-change-notification/#{type}/enable" do + setup [:create_user, :log_in, :create_site] - test "creates a #{type} notification record with the user email", %{ - conn: conn, - site: site, - user: user - } do - post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/enable") + test "creates a #{type} notification record with the user email", %{ + conn: conn, + site: site, + user: user + } do + post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/enable") - notification = Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id, type: unquote(type)) - assert notification.recipients == [user.email] - end + notification = + Repo.get_by(Plausible.Site.TrafficChangeNotification, + site_id: site.id, + type: unquote(type) + ) - test "does not allow duplicate #{type} notification to be created", %{ - conn: conn, - site: site - } do - post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/enable") - post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/enable") + assert notification.recipients == [user.email] + end - assert Repo.aggregate( - from(s in Plausible.Site.TrafficChangeNotification, where: s.site_id == ^site.id and s.type == ^unquote(type)), - :count - ) == 1 + test "does not allow duplicate #{type} notification to be created", %{ + conn: conn, + site: site + } do + post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/enable") + post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/enable") + + assert Repo.aggregate( + from(s in Plausible.Site.TrafficChangeNotification, + where: s.site_id == ^site.id and s.type == ^unquote(type) + ), + :count + ) == 1 + end end - end - describe "POST /sites/:website/traffic-change-notification/#{type}/disable" do - setup [:create_user, :log_in, :create_site] + describe "POST /sites/:website/traffic-change-notification/#{type}/disable" do + setup [:create_user, :log_in, :create_site] - test "deletes the #{type} notification record", %{conn: conn, site: site} do - insert(:"#{unquote(type)}_notification", site: site) + test "deletes the #{type} notification record", %{conn: conn, site: site} do + insert(:"#{unquote(type)}_notification", site: site) - post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/disable") + post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/disable") - refute Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id) + refute Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id) + end end - end - describe "PUT /sites/:website/traffic-change-notification/#{type}" do - setup [:create_user, :log_in, :create_site] + describe "PUT /sites/:website/traffic-change-notification/#{type}" do + setup [:create_user, :log_in, :create_site] - test "updates #{type} notification threshold", %{conn: conn, site: site} do - insert(:"#{unquote(type)}_notification", site: site, threshold: 10) + test "updates #{type} notification threshold", %{conn: conn, site: site} do + insert(:"#{unquote(type)}_notification", site: site, threshold: 10) - put(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}", %{ - "traffic_change_notification" => %{"threshold" => "15"} - }) + put(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}", %{ + "traffic_change_notification" => %{"threshold" => "15"} + }) + + notification = + Repo.get_by(Plausible.Site.TrafficChangeNotification, + site_id: site.id, + type: unquote(type) + ) - notification = Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id, type: unquote(type)) - assert notification.threshold == 15 + assert notification.threshold == 15 + end end - end - describe "POST /sites/:website/traffic-change-notification/#{type}/recipients" do - setup [:create_user, :log_in, :create_site] + describe "POST /sites/:website/traffic-change-notification/#{type}/recipients" do + setup [:create_user, :log_in, :create_site] - test "adds a recipient to the #{type} notification", %{conn: conn, site: site} do - insert(:"#{unquote(type)}_notification", site: site) + test "adds a recipient to the #{type} notification", %{conn: conn, site: site} do + insert(:"#{unquote(type)}_notification", site: site) - post(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/recipients", - recipient: "user@email.com" - ) + post( + conn, + "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/recipients", + recipient: "user@email.com" + ) - report = Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id, type: unquote(type)) - assert report.recipients == ["user@email.com"] + report = + Repo.get_by(Plausible.Site.TrafficChangeNotification, + site_id: site.id, + type: unquote(type) + ) + + assert report.recipients == ["user@email.com"] + end end - end - describe "DELETE /sites/:website/traffic-change-notification/#{type}/recipients/:recipient" do - setup [:create_user, :log_in, :create_site] + describe "DELETE /sites/:website/traffic-change-notification/#{type}/recipients/:recipient" do + setup [:create_user, :log_in, :create_site] - test "removes a recipient from the #{type} notification", %{conn: conn, site: site} do - insert(:"#{unquote(type)}_notification", site: site, recipients: ["recipient@email.com"]) + test "removes a recipient from the #{type} notification", %{conn: conn, site: site} do + insert(:"#{unquote(type)}_notification", site: site, recipients: ["recipient@email.com"]) - delete(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/recipients/recipient@email.com") + delete( + conn, + "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/recipients/recipient@email.com" + ) - report = Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id, type: unquote(type)) - assert report.recipients == [] - end + report = + Repo.get_by(Plausible.Site.TrafficChangeNotification, + site_id: site.id, + type: unquote(type) + ) - test "fails to remove a recipient from the #{type} notification in a foreign website", %{ - conn: conn - } do - site = insert(:site) - insert(:"#{unquote(type)}_notification", site: site, recipients: ["recipient@email.com"]) + assert report.recipients == [] + end - conn = - delete(conn, "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/recipients/recipient@email.com") + test "fails to remove a recipient from the #{type} notification in a foreign website", %{ + conn: conn + } do + site = insert(:site) + insert(:"#{unquote(type)}_notification", site: site, recipients: ["recipient@email.com"]) - assert conn.status == 404 + conn = + delete( + conn, + "/sites/#{site.domain}/traffic-change-notification/#{unquote(type)}/recipients/recipient@email.com" + ) - conn = - delete(conn, "/sites/#{site.domain}/traffic-change-notification/recipients/#{unquote(type)}/recipient%40email.com") + assert conn.status == 404 - assert conn.status == 404 + conn = + delete( + conn, + "/sites/#{site.domain}/traffic-change-notification/recipients/#{unquote(type)}/recipient%40email.com" + ) - report = Repo.get_by(Plausible.Site.TrafficChangeNotification, site_id: site.id, type: unquote(type)) - assert [_] = report.recipients + assert conn.status == 404 + + report = + Repo.get_by(Plausible.Site.TrafficChangeNotification, + site_id: site.id, + type: unquote(type) + ) + + assert [_] = report.recipients + end end end - end describe "GET /sites/:website/shared-links/new" do setup [:create_user, :log_in, :create_site]