From 35851e90846b6ad8411c6288ae272fa1228638d5 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Thu, 24 Oct 2024 11:27:55 +0300 Subject: [PATCH 01/34] Comparisons: Only render comparisons in top stats when both query and data supports it (#4732) * Comparisons: Only render comparisons in top stats when both query and data support it This fixes a bug from navigation: 1. Comparison disabled, choose a non-realtime period (e.g. press T for "Last 30 days") 2. Enable comparison mode (press X) 3. Choose the realtime period (press R) 4. Choose a non-realtime period again (press T) Kudos to robert for discovering the repro case. * Fix conditional Broken when fixing another bug https://github.com/plausible/analytics/commit/d727ba5ed522b5572d92e725852657007766ae3b#diff-f2361637bc87773faced883d7560491e4612b7581f4748f03241821f4ff8f6feL166 --- assets/js/dashboard/stats/graph/top-stats.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/assets/js/dashboard/stats/graph/top-stats.js b/assets/js/dashboard/stats/graph/top-stats.js index 3cc6c52e6180..24b9d02e0476 100644 --- a/assets/js/dashboard/stats/graph/top-stats.js +++ b/assets/js/dashboard/stats/graph/top-stats.js @@ -31,13 +31,15 @@ export default function TopStats({ data, onMetricUpdate, tooltipBoundary }) { const lastLoadTimestamp = useLastLoadContext() const site = useSiteContext() + const isComparison = query.comparison && data && data.comparing_from + function tooltip(stat) { let statName = stat.name.toLowerCase() statName = stat.value === 1 ? statName.slice(0, -1) : statName return (
- {query.comparison && ( + {isComparison && (
{topStatNumberLong(stat.graph_metric, stat.value)} vs.{' '} {topStatNumberLong(stat.graph_metric, stat.comparison_value)}{' '} @@ -50,7 +52,7 @@ export default function TopStats({ data, onMetricUpdate, tooltipBoundary }) {
)} - {!query.comparison && ( + {!isComparison && (
{topStatNumberLong(stat.graph_metric, stat.value)} {statName}
@@ -147,7 +149,7 @@ export default function TopStats({ data, onMetricUpdate, tooltipBoundary }) { > {topStatNumberShort(stat.graph_metric, stat.value)}

- {query.comparison && stat.change != null ? ( + {!isComparison && stat.change != null ? ( ) : null} - {query.comparison ? ( + {isComparison ? (

{formatDateRange(site, data.from, data.to)}

) : null}
- {query.comparison ? ( + {isComparison ? (

{topStatNumberShort(stat.graph_metric, stat.comparison_value)} From 5dbe1870e0407a466ec92a3c55d87d94300ace8e Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 24 Oct 2024 12:03:22 +0200 Subject: [PATCH 02/34] Implement teams consistency check and improve teams backfill (#4733) * Implement team consistency check script * Improve backfill teams sync check * Fix team grace period sync --- .../data_migration/backfill_teams.ex | 23 +- .../data_migration/teams_consistency_check.ex | 236 ++++++++++++++++++ lib/plausible/teams/team.ex | 8 +- 3 files changed, 261 insertions(+), 6 deletions(-) create mode 100644 lib/plausible/data_migration/teams_consistency_check.ex diff --git a/lib/plausible/data_migration/backfill_teams.ex b/lib/plausible/data_migration/backfill_teams.ex index 89314e57a6d3..70cf8392348d 100644 --- a/lib/plausible/data_migration/backfill_teams.ex +++ b/lib/plausible/data_migration/backfill_teams.ex @@ -88,10 +88,17 @@ defmodule Plausible.DataMigration.BackfillTeams do is_distinct(o.trial_expiry_date, t.trial_expiry_date) or is_distinct(o.accept_traffic_until, t.accept_traffic_until) or is_distinct(o.allow_next_upgrade_override, t.allow_next_upgrade_override) or - is_distinct(o.grace_period["id"], t.grace_period["id"]) or - is_distinct(o.grace_period["is_over"], t.grace_period["is_over"]) or - is_distinct(o.grace_period["end_date"], t.grace_period["end_date"]) or - is_distinct(o.grace_period["manual_lock"], t.grace_period["manual_lock"]), + (is_distinct(o.grace_period, t.grace_period) and + (is_distinct(o.grace_period["id"], t.grace_period["id"]) or + (is_nil(o.grace_period["is_over"]) and t.grace_period["is_over"] == true) or + (o.grace_period["is_over"] == true and t.grace_period["is_over"] == false) or + (o.grace_period["is_over"] == false and t.grace_period["is_over"] == true) or + is_distinct(o.grace_period["end_date"], t.grace_period["end_date"]) or + (is_nil(o.grace_period["manual_lock"]) and t.grace_period["manual_lock"] == true) or + (o.grace_period["manual_lock"] == true and + t.grace_period["manual_lock"] == false) or + (o.grace_period["manual_lock"] == false and + t.grace_period["manual_lock"] == true))), preload: [team_memberships: {tm, user: o}] ) |> @repo.all(timeout: :infinity) @@ -482,11 +489,17 @@ defmodule Plausible.DataMigration.BackfillTeams do :allow_next_upgrade_override, owner.allow_next_upgrade_override ) - |> Ecto.Changeset.put_embed(:grace_period, owner.grace_period) + |> Ecto.Changeset.put_embed(:grace_period, embed_params(owner.grace_period)) |> @repo.update!() end) end + defp embed_params(nil), do: nil + + defp embed_params(grace_period) do + Map.from_struct(grace_period) + end + defp backfill_subscriptions(subscriptions) do subscriptions |> Enum.with_index() diff --git a/lib/plausible/data_migration/teams_consistency_check.ex b/lib/plausible/data_migration/teams_consistency_check.ex new file mode 100644 index 000000000000..118a4a238db2 --- /dev/null +++ b/lib/plausible/data_migration/teams_consistency_check.ex @@ -0,0 +1,236 @@ +defmodule Plausible.DataMigration.TeamsConsitencyCheck do + @moduledoc """ + Verify consistency of teams. + """ + + import Ecto.Query + + alias Plausible.Teams + + @repo Plausible.DataMigration.PostgresRepo + + defmacrop is_distinct(f1, f2) do + quote do + fragment("? IS DISTINCT FROM ?", unquote(f1), unquote(f2)) + end + end + + def run() do + # Teams consistency check + db_url = + System.get_env( + "TEAMS_MIGRATION_DB_URL", + Application.get_env(:plausible, Plausible.Repo)[:url] + ) + + @repo.start(db_url, pool_size: 1) + + check() + end + + defp check() do + # Sites without teams + + sites_without_teams_count = + from( + s in Plausible.Site, + where: is_nil(s.team_id) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{sites_without_teams_count} sites without teams") + + # Teams without owner + + owner_membership_query = + from( + tm in Teams.Membership, + where: tm.team_id == parent_as(:team).id, + where: tm.role == :owner, + select: 1 + ) + + teams_without_owner_count = + from( + t in Plausible.Teams.Team, + as: :team, + where: not exists(owner_membership_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{teams_without_owner_count} teams without owner") + + # Subscriptions without teams + + subscriptions_without_teams_count = + from( + s in Plausible.Billing.Subscription, + where: is_nil(s.team_id) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{subscriptions_without_teams_count} subscriptions without teams") + + # Subscriptions out of sync + + subscriptions_out_of_sync_count = + from( + s in Plausible.Billing.Subscription, + inner_join: u in assoc(s, :user), + left_join: tm in assoc(u, :team_memberships), + on: tm.role == :owner, + where: s.team_id != tm.team_id + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{subscriptions_out_of_sync_count} subscriptions out of sync") + + # Enterprise plans without teams + + enterprise_plans_without_teams_count = + from( + ep in Plausible.Billing.EnterprisePlan, + where: is_nil(ep.team_id) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{enterprise_plans_without_teams_count} enterprise_plans without teams") + + # Enterprise plans out of sync + + enterprise_plans_out_of_sync_count = + from( + ep in Plausible.Billing.EnterprisePlan, + inner_join: u in assoc(ep, :user), + left_join: tm in assoc(u, :team_memberships), + on: tm.role == :owner, + where: ep.team_id != tm.team_id + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{enterprise_plans_out_of_sync_count} enterprise_plans out of sync") + + # Teams out of sync + + teams_out_of_sync_count = + from( + t in Teams.Team, + inner_join: tm in assoc(t, :team_memberships), + inner_join: o in assoc(tm, :user), + where: tm.role == :owner, + where: + is_distinct(o.trial_expiry_date, t.trial_expiry_date) or + is_distinct(o.accept_traffic_until, t.accept_traffic_until) or + is_distinct(o.allow_next_upgrade_override, t.allow_next_upgrade_override) or + (is_distinct(o.grace_period, t.grace_period) and + (is_distinct(o.grace_period["id"], t.grace_period["id"]) or + (is_nil(o.grace_period["is_over"]) and t.grace_period["is_over"] == true) or + (o.grace_period["is_over"] == true and t.grace_period["is_over"] == false) or + (o.grace_period["is_over"] == false and t.grace_period["is_over"] == true) or + is_distinct(o.grace_period["end_date"], t.grace_period["end_date"]) or + (is_nil(o.grace_period["manual_lock"]) and t.grace_period["manual_lock"] == true) or + (o.grace_period["manual_lock"] == true and + t.grace_period["manual_lock"] == false) or + (o.grace_period["manual_lock"] == false and + t.grace_period["manual_lock"] == true))), + preload: [team_memberships: {tm, user: o}] + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{teams_out_of_sync_count} teams out of sync") + + # Non-owner site memberships out of sync + + respective_guest_memberships_query = + from( + tm in Teams.Membership, + inner_join: gm in assoc(tm, :guest_memberships), + on: + gm.site_id == parent_as(:site_membership).site_id and + ((gm.role == :viewer and parent_as(:site_membership).role == :viewer) or + (gm.role == :editor and parent_as(:site_membership).role == :admin)), + where: tm.user_id == parent_as(:site_membership).user_id, + select: 1 + ) + + out_of_sync_nonowner_memberships_count = + from( + m in Plausible.Site.Membership, + as: :site_membership, + where: m.role != :owner, + where: not exists(respective_guest_memberships_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{out_of_sync_nonowner_memberships_count} out of sync non-owner site memberships") + + # Owner site memberships out of sync + + respective_owner_memberships_query = + from( + tm in Teams.Membership, + where: tm.team_id == parent_as(:site).team_id and tm.role == :owner, + select: 1 + ) + + out_of_sync_owner_memberships_count = + from( + m in Plausible.Site.Membership, + as: :site_membership, + inner_join: s in assoc(m, :site), + as: :site, + where: m.role == :owner, + where: not exists(respective_owner_memberships_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{out_of_sync_owner_memberships_count} out of sync owner site memberships") + + # Site invitations out of sync + + respective_guest_invitations_query = + from( + gi in Teams.GuestInvitation, + inner_join: ti in assoc(gi, :team_invitation), + on: ti.email == parent_as(:site_invitation).email, + where: gi.site_id == parent_as(:site_invitation).site_id, + select: 1 + ) + + out_of_sync_site_invitations_count = + from( + i in Plausible.Auth.Invitation, + as: :site_invitation, + where: i.role != :owner, + where: not exists(respective_guest_invitations_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{out_of_sync_site_invitations_count} out of sync site invitations") + + # Site invitations out of sync + + respective_site_transfers_query = + from( + st in Teams.SiteTransfer, + where: st.email == parent_as(:site_invitation).email, + where: st.site_id == parent_as(:site_invitation).site_id, + select: 1 + ) + + out_of_sync_site_transfers_count = + from( + i in Plausible.Auth.Invitation, + as: :site_invitation, + where: i.role == :owner, + where: not exists(respective_site_transfers_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{out_of_sync_site_transfers_count} out of sync site transfers") + end + + defp log(msg) do + IO.puts("[#{NaiveDateTime.utc_now(:second)}] #{msg}") + end +end diff --git a/lib/plausible/teams/team.ex b/lib/plausible/teams/team.ex index e1fe3a056d05..357e268eeb3c 100644 --- a/lib/plausible/teams/team.ex +++ b/lib/plausible/teams/team.ex @@ -35,7 +35,7 @@ defmodule Plausible.Teams.Team do |> put_change(:trial_expiry_date, user.trial_expiry_date) |> put_change(:accept_traffic_until, user.accept_traffic_until) |> put_change(:allow_next_upgrade_override, user.allow_next_upgrade_override) - |> put_embed(:grace_period, user.grace_period) + |> put_embed(:grace_period, embed_params(user.grace_period)) |> put_change(:inserted_at, user.inserted_at) |> put_change(:updated_at, user.updated_at) end @@ -63,6 +63,12 @@ defmodule Plausible.Teams.Team do ) end + defp embed_params(nil), do: nil + + defp embed_params(grace_period) do + Map.from_struct(grace_period) + end + defp trial_expiry() do on_ee do Date.utc_today() |> Date.shift(day: 30) From c1a6501ec1d27e6bb8e841af8604851ba70c9ce4 Mon Sep 17 00:00:00 2001 From: hq1 Date: Thu, 24 Oct 2024 12:24:12 +0200 Subject: [PATCH 03/34] Delete teams along with users on DELETE /me (#4734) * Delete teams along with users on DELETE /me * Delete orphaned teams during backfill * Format --- lib/plausible/auth/auth.ex | 2 ++ .../data_migration/backfill_teams.ex | 22 +++++++++++++++++++ .../controllers/auth_controller_test.exs | 3 +++ 3 files changed, 27 insertions(+) diff --git a/lib/plausible/auth/auth.ex b/lib/plausible/auth/auth.ex index ba1ecae1c2c7..276dec2136ef 100644 --- a/lib/plausible/auth/auth.ex +++ b/lib/plausible/auth/auth.ex @@ -107,6 +107,8 @@ defmodule Plausible.Auth do end end + {:ok, team} = Plausible.Teams.get_or_create(user) + Repo.delete!(team) Repo.delete!(user) end) end diff --git a/lib/plausible/data_migration/backfill_teams.ex b/lib/plausible/data_migration/backfill_teams.ex index 70cf8392348d..68647b3ad6fc 100644 --- a/lib/plausible/data_migration/backfill_teams.ex +++ b/lib/plausible/data_migration/backfill_teams.ex @@ -31,6 +31,24 @@ defmodule Plausible.DataMigration.BackfillTeams do end defp backfill() do + orphaned_teams = + from( + t in Plausible.Teams.Team, + left_join: tm in assoc(t, :team_memberships), + where: is_nil(tm.id), + left_join: sub in assoc(t, :subscription), + where: is_nil(sub.id), + left_join: s in assoc(t, :sites), + where: is_nil(s.id) + ) + |> @repo.all(timeout: :infinity) + + log("Found #{length(orphaned_teams)} orphaned teams...") + + delete_orphaned_teams(orphaned_teams) + + log("Deleted orphaned teams") + sites_without_teams = from( s in Plausible.Site, @@ -387,6 +405,10 @@ defmodule Plausible.DataMigration.BackfillTeams do log("All data are up to date now!") end + def delete_orphaned_teams(teams) do + Enum.each(teams, &@repo.delete!(&1)) + end + defp backfill_teams(sites) do sites |> Enum.map(fn %{id: site_id, memberships: [%{user: owner, role: :owner}]} -> diff --git a/test/plausible_web/controllers/auth_controller_test.exs b/test/plausible_web/controllers/auth_controller_test.exs index b67843715790..8bfb495180be 100644 --- a/test/plausible_web/controllers/auth_controller_test.exs +++ b/test/plausible_web/controllers/auth_controller_test.exs @@ -591,11 +591,14 @@ defmodule PlausibleWeb.AuthControllerTest do site_limit: 1 ) + {:ok, _team} = Plausible.Teams.get_or_create(user) + conn = delete(conn, "/me") assert redirected_to(conn) == "/" assert Repo.reload(site) == nil assert Repo.reload(user) == nil assert Repo.all(Plausible.Billing.Subscription) == [] + assert Repo.all(Plausible.Teams.Team) == [] end test "deletes sites that the user owns", %{conn: conn, user: user, site: owner_site} do From 2ec9e325576962878655f6eb523cf8ef383a4641 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 24 Oct 2024 15:38:58 +0200 Subject: [PATCH 04/34] Extend team consistency checks and improve syncing team and memberships (#4735) * Extend team consistency checks * Sync team against user right after creating the site * Prune orphaned team guest memberships and invitations on site removal --- .../api/external_sites_controller.ex | 2 +- lib/plausible/auth/auth.ex | 15 +-- .../data_migration/teams_consistency_check.ex | 98 +++++++++++++++++++ lib/plausible/site/removal.ex | 16 ++- lib/plausible/sites.ex | 8 ++ .../controllers/site_controller.ex | 2 +- test/plausible/goals_test.exs | 2 +- test/plausible/site/site_removal_test.exs | 29 +++++- 8 files changed, 155 insertions(+), 17 deletions(-) diff --git a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex index 6d5d90054438..447fa0e5469e 100644 --- a/extra/lib/plausible_web/controllers/api/external_sites_controller.ex +++ b/extra/lib/plausible_web/controllers/api/external_sites_controller.ex @@ -97,7 +97,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do def delete_site(conn, %{"site_id" => site_id}) do case get_site(conn.assigns.current_user, site_id, [:owner]) do {:ok, site} -> - {:ok, _} = Plausible.Site.Removal.run(site.domain) + {:ok, _} = Plausible.Site.Removal.run(site) json(conn, %{"deleted" => true}) {:error, :site_not_found} -> diff --git a/lib/plausible/auth/auth.ex b/lib/plausible/auth/auth.ex index 276dec2136ef..b5a31faf9e11 100644 --- a/lib/plausible/auth/auth.ex +++ b/lib/plausible/auth/auth.ex @@ -95,16 +95,19 @@ defmodule Plausible.Auth do def delete_user(user) do Repo.transaction(fn -> - user = - user - |> Repo.preload(site_memberships: :site) + user = Repo.preload(user, site_memberships: :site) for membership <- user.site_memberships do - Repo.delete!(membership) - if membership.role == :owner do - Plausible.Site.Removal.run(membership.site.domain) + Plausible.Site.Removal.run(membership.site) end + + Repo.delete_all( + from( + sm in Plausible.Site.Membership, + where: sm.id == ^membership.id + ) + ) end {:ok, team} = Plausible.Teams.get_or_create(user) diff --git a/lib/plausible/data_migration/teams_consistency_check.ex b/lib/plausible/data_migration/teams_consistency_check.ex index 118a4a238db2..63e59f74242d 100644 --- a/lib/plausible/data_migration/teams_consistency_check.ex +++ b/lib/plausible/data_migration/teams_consistency_check.ex @@ -228,6 +228,104 @@ defmodule Plausible.DataMigration.TeamsConsitencyCheck do |> @repo.aggregate(:count, timeout: :infinity) log("#{out_of_sync_site_transfers_count} out of sync site transfers") + + # Guest memberships out of sync + + respective_site_memberships_query = + from( + sm in Plausible.Site.Membership, + where: sm.site_id == parent_as(:guest_membership).site_id, + where: sm.user_id == parent_as(:team_membership).user_id, + where: + (sm.role == :viewer and parent_as(:guest_membership).role == :viewer) or + (sm.role == :admin and parent_as(:guest_membership).role == :editor), + select: 1 + ) + + out_of_sync_guest_memberships_count = + from( + gm in Plausible.Teams.GuestMembership, + as: :guest_membership, + inner_join: tm in assoc(gm, :team_membership), + as: :team_membership, + where: tm.role != :owner, + where: not exists(respective_site_memberships_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{out_of_sync_guest_memberships_count} out of sync guest memberships") + + # Owner memberships out of sync + + respective_site_memberships_query = + from( + sm in Plausible.Site.Membership, + where: sm.site_id == parent_as(:site).id, + where: sm.user_id == parent_as(:team_membership).user_id, + where: sm.role == :owner, + select: 1 + ) + + out_of_sync_owner_memberships_count = + from( + tm in Plausible.Teams.Membership, + as: :team_membership, + inner_join: t in assoc(tm, :team), + inner_join: s in assoc(t, :sites), + as: :site, + where: tm.role == :owner, + where: not exists(respective_site_memberships_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{out_of_sync_owner_memberships_count} out of sync owner team memberships") + + # Guest invitations out of sync + + respective_site_invitations_query = + from( + i in Plausible.Auth.Invitation, + where: i.site_id == parent_as(:guest_invitation).site_id, + where: i.email == parent_as(:team_invitation).email, + where: + (i.role == :viewer and parent_as(:guest_invitation).role == :viewer) or + (i.role == :admin and parent_as(:guest_invitation).role == :editor), + select: 1 + ) + + out_of_sync_guest_invitations_count = + from( + gi in Plausible.Teams.GuestInvitation, + as: :guest_invitation, + inner_join: ti in assoc(gi, :team_invitation), + as: :team_invitation, + where: ti.role != :owner, + where: not exists(respective_site_invitations_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{out_of_sync_guest_invitations_count} out of sync guest invitations") + + # Team site transfers out of sync + + respective_site_transfers_query = + from( + i in Plausible.Auth.Invitation, + where: i.site_id == parent_as(:site_transfer).site_id, + where: i.email == parent_as(:site_transfer).email, + where: i.role == :owner, + select: 1 + ) + + out_of_sync_site_transfers_count = + from( + st in Plausible.Teams.SiteTransfer, + as: :site_transfer, + where: not exists(respective_site_transfers_query) + ) + |> @repo.aggregate(:count, timeout: :infinity) + + log("#{out_of_sync_site_transfers_count} out of sync team site transfers") end defp log(msg) do diff --git a/lib/plausible/site/removal.ex b/lib/plausible/site/removal.ex index af1fd6387f66..8fe878ab13de 100644 --- a/lib/plausible/site/removal.ex +++ b/lib/plausible/site/removal.ex @@ -6,9 +6,17 @@ defmodule Plausible.Site.Removal do import Ecto.Query - @spec run(String.t()) :: {:ok, map()} - def run(domain) do - result = Repo.delete_all(from(s in Plausible.Site, where: s.domain == ^domain)) - {:ok, %{delete_all: result}} + @spec run(Plausible.Site.t()) :: {:ok, map()} + def run(site) do + Repo.transaction(fn -> + site = Plausible.Teams.load_for_site(site) + + result = Repo.delete_all(from(s in Plausible.Site, where: s.domain == ^site.domain)) + + Plausible.Teams.Memberships.prune_guests(site.team) + Plausible.Teams.Invitations.prune_guest_invitations(site.team) + + %{delete_all: result} + end) end end diff --git a/lib/plausible/sites.ex b/lib/plausible/sites.ex index e61b70e688c1..5e8b97b8a7d1 100644 --- a/lib/plausible/sites.ex +++ b/lib/plausible/sites.ex @@ -206,6 +206,14 @@ defmodule Plausible.Sites do Site.Membership.new(site, user) end) |> maybe_start_trial(user) + |> Ecto.Multi.run(:sync_team, fn + _repo, %{user: user} -> + Plausible.Teams.sync_team(user) + {:ok, nil} + + _repo, _context -> + {:ok, nil} + end) |> Repo.transaction() end end diff --git a/lib/plausible_web/controllers/site_controller.ex b/lib/plausible_web/controllers/site_controller.ex index cdfdcf612c8c..970d8983a6dc 100644 --- a/lib/plausible_web/controllers/site_controller.ex +++ b/lib/plausible_web/controllers/site_controller.ex @@ -325,7 +325,7 @@ defmodule PlausibleWeb.SiteController do def delete_site(conn, _params) do site = conn.assigns[:site] - Plausible.Site.Removal.run(site.domain) + Plausible.Site.Removal.run(site) conn |> put_flash(:success, "Your site and page views deletion process has started.") diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index 672858f8e668..552cef622e14 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -167,7 +167,7 @@ defmodule Plausible.GoalsTest do insert(:goal, %{site: site, event_name: " Signup "}) insert(:goal, %{site: site, page_path: " /Signup "}) - Plausible.Site.Removal.run(site.domain) + Plausible.Site.Removal.run(site) assert [] = Goals.for_site(site) end diff --git a/test/plausible/site/site_removal_test.exs b/test/plausible/site/site_removal_test.exs index 931cb29dfff5..4a8680fe72bf 100644 --- a/test/plausible/site/site_removal_test.exs +++ b/test/plausible/site/site_removal_test.exs @@ -7,13 +7,34 @@ defmodule Plausible.Site.SiteRemovalTest do test "site from postgres is immediately deleted" do site = insert(:site) - assert {:ok, context} = Removal.run(site.domain) + assert {:ok, context} = Removal.run(site) assert context.delete_all == {1, nil} refute Sites.get_by_domain(site.domain) end - test "deletion is idempotent" do - assert {:ok, context} = Removal.run("some.example.com") - assert context.delete_all == {0, nil} + @tag :teams + test "site deletion prunes team guest memberships" do + site = insert(:site) |> Plausible.Teams.load_for_site() |> Repo.preload(:owner) + + team_membership = + insert(:team_membership, user: build(:user), team: site.team, role: :guest) + + insert(:guest_membership, team_membership: team_membership, site: site, role: :viewer) + + team_invitation = + insert(:team_invitation, + email: "sitedeletion@example.test", + team: site.team, + inviter: site.owner, + role: :guest + ) + + insert(:guest_invitation, team_invitation: team_invitation, site: site, role: :viewer) + + assert {:ok, context} = Removal.run(site) + assert context.delete_all == {1, nil} + + refute Repo.reload(team_membership) + refute Repo.reload(team_invitation) end end From 0404522b5a85c51e81a653e5a8315cfefe22161c Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 24 Oct 2024 20:44:28 +0200 Subject: [PATCH 05/34] Sync team consistently on site creation (#4736) --- lib/plausible/sites.ex | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/plausible/sites.ex b/lib/plausible/sites.ex index 5e8b97b8a7d1..9252f50be8f1 100644 --- a/lib/plausible/sites.ex +++ b/lib/plausible/sites.ex @@ -206,13 +206,9 @@ defmodule Plausible.Sites do Site.Membership.new(site, user) end) |> maybe_start_trial(user) - |> Ecto.Multi.run(:sync_team, fn - _repo, %{user: user} -> - Plausible.Teams.sync_team(user) - {:ok, nil} - - _repo, _context -> - {:ok, nil} + |> Ecto.Multi.run(:sync_team, fn _repo, %{user: user} -> + Plausible.Teams.sync_team(user) + {:ok, nil} end) |> Repo.transaction() end @@ -226,7 +222,7 @@ defmodule Plausible.Sites do end) _ -> - multi + Ecto.Multi.put(multi, :user, user) end end From 8bd32b68c9e44d54467fe7cd67ed191ea3491bab Mon Sep 17 00:00:00 2001 From: hq1 Date: Mon, 28 Oct 2024 10:07:18 +0100 Subject: [PATCH 06/34] Bring back premium feature notices in tiles (#4742) * Lost tapes: bring back premium feature notices in tiles * Ensure that e-mail CTA is only presented to business/enterprise customers * Format * Fix typespecs * Add t() type to EnterprisePlan * Reduce Plan.t() definition Found no reason for it to be there. --- lib/plausible/billing/enterprise_plan.ex | 2 ++ lib/plausible/billing/plan.ex | 2 +- lib/plausible/billing/plans.ex | 2 ++ .../components/billing/notice.ex | 25 ++++++++++--------- .../templates/site/settings_funnels.html.heex | 11 ++++---- .../templates/site/settings_props.html.heex | 13 +++++----- .../live/funnel_settings_test.exs | 14 +++++++++++ .../live/props_settings_test.exs | 13 ++++++++++ 8 files changed, 58 insertions(+), 24 deletions(-) diff --git a/lib/plausible/billing/enterprise_plan.ex b/lib/plausible/billing/enterprise_plan.ex index 2a224cce2dc8..9589c1e0d82d 100644 --- a/lib/plausible/billing/enterprise_plan.ex +++ b/lib/plausible/billing/enterprise_plan.ex @@ -2,6 +2,8 @@ defmodule Plausible.Billing.EnterprisePlan do use Ecto.Schema import Ecto.Changeset + @type t() :: %__MODULE__{} + @required_fields [ :user_id, :paddle_plan_id, diff --git a/lib/plausible/billing/plan.ex b/lib/plausible/billing/plan.ex index 78a6b8551c5a..132a7c585d33 100644 --- a/lib/plausible/billing/plan.ex +++ b/lib/plausible/billing/plan.ex @@ -4,7 +4,7 @@ defmodule Plausible.Billing.Plan do use Ecto.Schema import Ecto.Changeset - @type t() :: %__MODULE__{} | :enterprise + @type t() :: %__MODULE__{} embedded_schema do # Due to grandfathering, we sometimes need to check the "generation" (e.g. diff --git a/lib/plausible/billing/plans.ex b/lib/plausible/billing/plans.ex index 901ed2d968b8..15f56e4964ae 100644 --- a/lib/plausible/billing/plans.ex +++ b/lib/plausible/billing/plans.ex @@ -105,6 +105,8 @@ defmodule Plausible.Billing.Plans do end) end + @spec get_subscription_plan(nil | Subscription.t()) :: + nil | :free_10k | Plan.t() | EnterprisePlan.t() def get_subscription_plan(nil), do: nil def get_subscription_plan(subscription) do diff --git a/lib/plausible_web/components/billing/notice.ex b/lib/plausible_web/components/billing/notice.ex index 409d4b4bcf21..eafd5b28e4a3 100644 --- a/lib/plausible_web/components/billing/notice.ex +++ b/lib/plausible_web/components/billing/notice.ex @@ -302,17 +302,24 @@ defmodule PlausibleWeb.Components.Billing.Notice do defp upgrade_call_to_action(assigns) do billable_user = Plausible.Users.with_subscription(assigns.billable_user) - plan = - Plans.get_regular_plan(billable_user.subscription, only_non_expired: true) - - trial? = Plausible.Users.on_trial?(assigns.billable_user) - growth? = plan && plan.kind == :growth + upgrade_assistance_required? = + case Plans.get_subscription_plan(billable_user.subscription) do + %Plausible.Billing.Plan{kind: :business} -> true + %Plausible.Billing.EnterprisePlan{} -> true + _ -> false + end cond do assigns.billable_user.id !== assigns.current_user.id -> ~H"please reach out to the site owner to upgrade their subscription" - growth? || trial? -> + upgrade_assistance_required? -> + ~H""" + please contact hello@plausible.io + to upgrade your subscription + """ + + true -> ~H""" please <.link @@ -322,12 +329,6 @@ defmodule PlausibleWeb.Components.Billing.Notice do upgrade your subscription """ - - true -> - ~H""" - please contact hello@plausible.io - to upgrade your subscription - """ end end diff --git a/lib/plausible_web/templates/site/settings_funnels.html.heex b/lib/plausible_web/templates/site/settings_funnels.html.heex index d583c341a4fc..e668b3e21c9d 100644 --- a/lib/plausible_web/templates/site/settings_funnels.html.heex +++ b/lib/plausible_web/templates/site/settings_funnels.html.heex @@ -12,12 +12,13 @@ Compose Goals into Funnels + +

- <%= live_render(@conn, PlausibleWeb.Live.FunnelSettings, session: %{"site_id" => @site.id, "domain" => @site.domain} ) %> diff --git a/lib/plausible_web/templates/site/settings_props.html.heex b/lib/plausible_web/templates/site/settings_props.html.heex index 35ffc1a21107..fe39e4cc62df 100644 --- a/lib/plausible_web/templates/site/settings_props.html.heex +++ b/lib/plausible_web/templates/site/settings_props.html.heex @@ -13,13 +13,14 @@ create custom metrics. + +
- <%= live_render(@conn, PlausibleWeb.Live.PropsSettings, id: "props-form", session: %{"site_id" => @site.id, "domain" => @site.domain} diff --git a/test/plausible_web/live/funnel_settings_test.exs b/test/plausible_web/live/funnel_settings_test.exs index f46c714de9a2..245cae47525e 100644 --- a/test/plausible_web/live/funnel_settings_test.exs +++ b/test/plausible_web/live/funnel_settings_test.exs @@ -10,6 +10,18 @@ defmodule PlausibleWeb.Live.FunnelSettingsTest do describe "GET /:domain/settings/funnels" do setup [:create_user, :log_in, :create_site] + @tag :ee_only + test "premium feature notice renders", %{conn: conn, site: site, user: user} do + user + |> Plausible.Auth.User.end_trial() + |> Plausible.Repo.update!() + + conn = get(conn, "/#{site.domain}/settings/funnels") + resp = conn |> html_response(200) |> text() + + assert resp =~ "please upgrade your subscription" + end + test "lists funnels for the site and renders help link", %{conn: conn, site: site} do {:ok, _} = setup_funnels(site) conn = get(conn, "/#{site.domain}/settings/funnels") @@ -18,6 +30,8 @@ defmodule PlausibleWeb.Live.FunnelSettingsTest do assert resp =~ "Compose Goals into Funnels" assert resp =~ "From blog to signup" assert resp =~ "From signup to blog" + refute resp =~ "Your account does not have access" + refute resp =~ "please upgrade your subscription" assert element_exists?(resp, "a[href=\"https://plausible.io/docs/funnel-analysis\"]") end diff --git a/test/plausible_web/live/props_settings_test.exs b/test/plausible_web/live/props_settings_test.exs index 79b578d875c3..8154e38a7b6b 100644 --- a/test/plausible_web/live/props_settings_test.exs +++ b/test/plausible_web/live/props_settings_test.exs @@ -6,6 +6,18 @@ defmodule PlausibleWeb.Live.PropsSettingsTest do describe "GET /:domain/settings/properties" do setup [:create_user, :log_in, :create_site] + @tag :ee_only + test "premium feature notice renders", %{conn: conn, site: site, user: user} do + user + |> Plausible.Auth.User.end_trial() + |> Plausible.Repo.update!() + + conn = get(conn, "/#{site.domain}/settings/properties") + resp = conn |> html_response(200) |> text() + + assert resp =~ "please upgrade your subscription" + end + test "lists props for the site and renders links", %{conn: conn, site: site} do {:ok, site} = Plausible.Props.allow(site, ["amount", "logged_in", "is_customer"]) conn = get(conn, "/#{site.domain}/settings/properties") @@ -21,6 +33,7 @@ defmodule PlausibleWeb.Live.PropsSettingsTest do assert resp =~ "amount" assert resp =~ "logged_in" assert resp =~ "is_customer" + refute resp =~ "please upgrade your subscription" end test "lists props with disallow actions", %{conn: conn, site: site} do From b49fc82c2bd0e47b40a7baa8ba439d90be31c814 Mon Sep 17 00:00:00 2001 From: RobertJoonas <56999674+RobertJoonas@users.noreply.github.com> Date: Mon, 28 Oct 2024 10:59:16 +0100 Subject: [PATCH 07/34] add size reporter GH action (#4745) --- .github/workflows/node.yml | 1 + tracker/package.json | 1 + tracker/report-sizes.js | 28 ++++++++++++++++++++++++++++ 3 files changed, 30 insertions(+) create mode 100644 tracker/report-sizes.js diff --git a/.github/workflows/node.yml b/.github/workflows/node.yml index b5aec1de0f0e..d2ddefe800ba 100644 --- a/.github/workflows/node.yml +++ b/.github/workflows/node.yml @@ -32,3 +32,4 @@ jobs: - run: npm run check-format --prefix ./assets - run: npm run test --prefix ./assets - run: npm run deploy --prefix ./tracker + - run: npm run report-sizes --prefix ./tracker diff --git a/tracker/package.json b/tracker/package.json index 35c3f40bff82..d1b222583f15 100644 --- a/tracker/package.json +++ b/tracker/package.json @@ -2,6 +2,7 @@ "scripts": { "deploy": "node compile.js", "test": "npm run deploy && npx playwright test --config=./test/support/playwright.config.js", + "report-sizes": "node report-sizes.js", "start": "node test/support/server.js" }, "license": "MIT", diff --git a/tracker/report-sizes.js b/tracker/report-sizes.js new file mode 100644 index 000000000000..01f63ab43452 --- /dev/null +++ b/tracker/report-sizes.js @@ -0,0 +1,28 @@ +const fs = require('fs'); +const path = require('path'); +const { execSync } = require('child_process'); + +const PrivTrackerDir = '../priv/tracker/js/'; + +const toReport = [ + 'plausible.js', + 'plausible.pageleave.js', + 'plausible.manual.pageleave.js', + 'plausible.hash.pageleave.js' +]; + +const results = []; + +toReport.forEach((filename) => { + const filePath = path.join(PrivTrackerDir, filename); + if (fs.statSync(filePath).isFile()) { + results.push({ + 'Filename': filename, + 'Real Size (Bytes)': fs.statSync(filePath).size, + 'Gzipped Size (Bytes)': execSync(`gzip -c -9 "${filePath}"`).length, + 'Brotli Size (Bytes)': execSync(`brotli -c -q 11 "${filePath}"`).length + }); + } +}); + +console.table(results) From 27ac3b6b5bab958945309463411dc6c283ab78ad Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 28 Oct 2024 11:11:47 +0100 Subject: [PATCH 08/34] Address team syncing discrepancies (#4739) * Clean site transfers after 48 hours * Sync accepting site transfers and invitations within transaction * Add dry run mode to teams backfill and make it a default * Extend invitation clean worker tests --- .../data_migration/backfill_teams.ex | 120 +++++++++++------- .../site/memberships/accept_invitation.ex | 12 +- lib/workers/clean_invitations.ex | 5 + test/support/factory.ex | 7 + test/workers/clean_invitations_test.exs | 53 +++++++- 5 files changed, 146 insertions(+), 51 deletions(-) diff --git a/lib/plausible/data_migration/backfill_teams.ex b/lib/plausible/data_migration/backfill_teams.ex index 68647b3ad6fc..b6c04119b77f 100644 --- a/lib/plausible/data_migration/backfill_teams.ex +++ b/lib/plausible/data_migration/backfill_teams.ex @@ -17,7 +17,9 @@ defmodule Plausible.DataMigration.BackfillTeams do end end - def run() do + def run(opts \\ []) do + dry_run? = Keyword.get(opts, :dry_run?, true) + # Teams backfill db_url = System.get_env( @@ -27,10 +29,10 @@ defmodule Plausible.DataMigration.BackfillTeams do @repo.start(db_url, pool_size: 2 * @max_concurrency) - backfill() + backfill(dry_run?) end - defp backfill() do + defp backfill(dry_run?) do orphaned_teams = from( t in Plausible.Teams.Team, @@ -45,9 +47,11 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(orphaned_teams)} orphaned teams...") - delete_orphaned_teams(orphaned_teams) + if not dry_run? do + delete_orphaned_teams(orphaned_teams) - log("Deleted orphaned teams") + log("Deleted orphaned teams") + end sites_without_teams = from( @@ -62,9 +66,11 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(sites_without_teams)} sites without teams...") - teams_count = backfill_teams(sites_without_teams) + if not dry_run? do + teams_count = backfill_teams(sites_without_teams) - log("Backfilled #{teams_count} teams.") + log("Backfilled #{teams_count} teams.") + end owner_site_memberships_query = from( @@ -90,9 +96,11 @@ defmodule Plausible.DataMigration.BackfillTeams do "Found #{length(users_with_subscriptions_without_sites)} users with subscriptions without sites..." ) - teams_count = backfill_teams_for_users(users_with_subscriptions_without_sites) + if not dry_run? do + teams_count = backfill_teams_for_users(users_with_subscriptions_without_sites) - log("Backfilled #{teams_count} teams from users with subscriptions without sites.") + log("Backfilled #{teams_count} teams from users with subscriptions without sites.") + end # Stale teams sync @@ -123,11 +131,13 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(stale_teams)} teams which have fields out of sync...") - sync_teams(stale_teams) + if not dry_run? do + sync_teams(stale_teams) - # Subsciprtions backfill + log("Brought out of sync teams up to date.") + end - log("Brought out of sync teams up to date.") + # Subsciprtions backfill subscriptions_without_teams = from( @@ -143,9 +153,11 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(subscriptions_without_teams)} subscriptions without team...") - backfill_subscriptions(subscriptions_without_teams) + if not dry_run? do + backfill_subscriptions(subscriptions_without_teams) - log("All subscriptions are linked to a team now.") + log("All subscriptions are linked to a team now.") + end # Enterprise plans backfill @@ -163,9 +175,11 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(enterprise_plans_without_teams)} enterprise plans without team...") - backfill_enterprise_plans(enterprise_plans_without_teams) + if not dry_run? do + backfill_enterprise_plans(enterprise_plans_without_teams) - log("All enterprise plans are linked to a team now.") + log("All enterprise plans are linked to a team now.") + end # Guest Memberships cleanup @@ -190,17 +204,19 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(guest_memberships_to_remove)} guest memberships to remove...") - team_ids_to_prune = remove_guest_memberships(guest_memberships_to_remove) + if not dry_run? do + team_ids_to_prune = remove_guest_memberships(guest_memberships_to_remove) - log("Pruning guest team memberships for #{length(team_ids_to_prune)} teams...") + log("Pruning guest team memberships for #{length(team_ids_to_prune)} teams...") - from(t in Teams.Team, where: t.id in ^team_ids_to_prune) - |> @repo.all(timeout: :infinity) - |> Enum.each(fn team -> - Plausible.Teams.Memberships.prune_guests(team) - end) + from(t in Teams.Team, where: t.id in ^team_ids_to_prune) + |> @repo.all(timeout: :infinity) + |> Enum.each(fn team -> + Plausible.Teams.Memberships.prune_guests(team) + end) - log("Guest memberships cleared.") + log("Guest memberships cleared.") + end # Guest Memberships backfill @@ -230,9 +246,11 @@ defmodule Plausible.DataMigration.BackfillTeams do "Found #{length(site_memberships_to_backfill)} site memberships without guest membership..." ) - backfill_guest_memberships(site_memberships_to_backfill) + if not dry_run? do + backfill_guest_memberships(site_memberships_to_backfill) - log("Backfilled missing guest memberships.") + log("Backfilled missing guest memberships.") + end # Stale guest memberships sync @@ -253,9 +271,11 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(stale_guest_memberships)} guest memberships with role out of sync...") - sync_guest_memberships(stale_guest_memberships) + if not dry_run? do + sync_guest_memberships(stale_guest_memberships) - log("All guest memberships are up to date now.") + log("All guest memberships are up to date now.") + end # Guest invitations cleanup @@ -281,17 +301,19 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(guest_invitations_to_remove)} guest invitations to remove...") - team_ids_to_prune = remove_guest_invitations(guest_invitations_to_remove) + if not dry_run? do + team_ids_to_prune = remove_guest_invitations(guest_invitations_to_remove) - log("Pruning guest team invitations for #{length(team_ids_to_prune)} teams...") + log("Pruning guest team invitations for #{length(team_ids_to_prune)} teams...") - from(t in Teams.Team, where: t.id in ^team_ids_to_prune) - |> @repo.all(timeout: :infinity) - |> Enum.each(fn team -> - Plausible.Teams.Invitations.prune_guest_invitations(team) - end) + from(t in Teams.Team, where: t.id in ^team_ids_to_prune) + |> @repo.all(timeout: :infinity) + |> Enum.each(fn team -> + Plausible.Teams.Invitations.prune_guest_invitations(team) + end) - log("Guest invitations cleared.") + log("Guest invitations cleared.") + end # Guest invitations backfill @@ -321,9 +343,11 @@ defmodule Plausible.DataMigration.BackfillTeams do "Found #{length(site_invitations_to_backfill)} site invitations without guest invitation..." ) - backfill_guest_invitations(site_invitations_to_backfill) + if not dry_run? do + backfill_guest_invitations(site_invitations_to_backfill) - log("Backfilled missing guest invitations.") + log("Backfilled missing guest invitations.") + end # Stale guest invitations sync @@ -344,9 +368,11 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(stale_guest_invitations)} guest invitations with role out of sync...") - sync_guest_invitations(stale_guest_invitations) + if not dry_run? do + sync_guest_invitations(stale_guest_invitations) - log("All guest invitations are up to date now.") + log("All guest invitations are up to date now.") + end # Site transfers cleanup @@ -368,9 +394,11 @@ defmodule Plausible.DataMigration.BackfillTeams do log("Found #{length(site_transfers_to_remove)} site transfers to remove...") - remove_site_transfers(site_transfers_to_remove) + if not dry_run? do + remove_site_transfers(site_transfers_to_remove) - log("Site transfers cleared.") + log("Site transfers cleared.") + end # Site transfers backfill @@ -398,11 +426,13 @@ defmodule Plausible.DataMigration.BackfillTeams do "Found #{length(site_invitations_to_backfill)} ownership transfers without site transfer..." ) - backfill_site_transfers(site_invitations_to_backfill) + if not dry_run? do + backfill_site_transfers(site_invitations_to_backfill) - log("Backfilled missing site transfers.") + log("Backfilled missing site transfers.") - log("All data are up to date now!") + log("All data are up to date now!") + end end def delete_orphaned_teams(teams) do diff --git a/lib/plausible/site/memberships/accept_invitation.ex b/lib/plausible/site/memberships/accept_invitation.ex index bde5bb9dc833..f5c9d6b39bd6 100644 --- a/lib/plausible/site/memberships/accept_invitation.ex +++ b/lib/plausible/site/memberships/accept_invitation.ex @@ -75,22 +75,26 @@ defmodule Plausible.Site.Memberships.AcceptInvitation do site = Repo.preload(invitation.site, :owner) with :ok <- Invitations.ensure_can_take_ownership(site, user) do - Plausible.Teams.Invitations.accept_transfer_sync(invitation, user) - site |> add_and_transfer_ownership(membership, user) |> Multi.delete(:invitation, invitation) + |> Multi.run(:sync_transfer, fn _repo, _context -> + Plausible.Teams.Invitations.accept_transfer_sync(invitation, user) + {:ok, nil} + end) |> finalize_invitation(invitation) end end defp do_accept_invitation(invitation, user) do - Plausible.Teams.Invitations.accept_invitation_sync(invitation, user) - membership = get_or_create_membership(invitation, user) invitation |> add(membership, user) + |> Multi.run(:sync_invitation, fn _repo, _context -> + Plausible.Teams.Invitations.accept_invitation_sync(invitation, user) + {:ok, nil} + end) |> finalize_invitation(invitation) end diff --git a/lib/workers/clean_invitations.ex b/lib/workers/clean_invitations.ex index c822e93a591c..a494a06e19a3 100644 --- a/lib/workers/clean_invitations.ex +++ b/lib/workers/clean_invitations.ex @@ -20,6 +20,11 @@ defmodule Plausible.Workers.CleanInvitations do where: ti.inserted_at < ^cutoff_time ) + Repo.delete_all( + from ti in Plausible.Teams.SiteTransfer, + where: ti.inserted_at < ^cutoff_time + ) + :ok end end diff --git a/test/support/factory.ex b/test/support/factory.ex index 53bf4f357c14..aeb164b4f6e2 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -38,6 +38,13 @@ defmodule Plausible.Factory do } end + def site_transfer_factory do + %Plausible.Teams.SiteTransfer{ + transfer_id: Nanoid.generate(), + email: sequence(:email, &"email-#{&1}@example.com") + } + end + def user_factory(attrs) do pw = Map.get(attrs, :password, "password") diff --git a/test/workers/clean_invitations_test.exs b/test/workers/clean_invitations_test.exs index d354b98af3ff..ff0ee03cac87 100644 --- a/test/workers/clean_invitations_test.exs +++ b/test/workers/clean_invitations_test.exs @@ -2,7 +2,7 @@ defmodule Plausible.Workers.CleanInvitationsTest do use Plausible.DataCase alias Plausible.Workers.CleanInvitations - test "cleans invitation that is more than 48h old" do + test "cleans invitations and transfers that are more than 48h old" do now = NaiveDateTime.utc_now(:second) insert(:invitation, @@ -11,12 +11,38 @@ defmodule Plausible.Workers.CleanInvitationsTest do inviter: build(:user) ) + site = insert(:site, team: build(:team)) + + team_invitation = + insert(:team_invitation, + inserted_at: NaiveDateTime.shift(now, hour: -49), + team: site.team, + inviter: build(:user), + role: :guest + ) + + insert(:guest_invitation, + inserted_at: NaiveDateTime.shift(now, hour: -49), + team_invitation: team_invitation, + site: site, + role: :viewer + ) + + insert(:site_transfer, + inserted_at: NaiveDateTime.shift(now, hour: -49), + site: site, + initiator: build(:user) + ) + CleanInvitations.perform(nil) refute Repo.exists?(Plausible.Auth.Invitation) + refute Repo.exists?(Plausible.Teams.Invitation) + refute Repo.exists?(Plausible.Teams.GuestInvitation) + refute Repo.exists?(Plausible.Teams.SiteTransfer) end - test "does not clean invitation that is less than 48h old" do + test "does not clean invitations and transfers that are less than 48h old" do now = NaiveDateTime.utc_now(:second) insert(:invitation, @@ -25,6 +51,29 @@ defmodule Plausible.Workers.CleanInvitationsTest do inviter: build(:user) ) + site = insert(:site, team: build(:team)) + + team_invitation = + insert(:team_invitation, + inserted_at: NaiveDateTime.shift(now, hour: -47), + team: site.team, + inviter: build(:user), + role: :guest + ) + + insert(:guest_invitation, + inserted_at: NaiveDateTime.shift(now, hour: -47), + team_invitation: team_invitation, + site: site, + role: :viewer + ) + + insert(:site_transfer, + inserted_at: NaiveDateTime.shift(now, hour: -47), + site: site, + initiator: build(:user) + ) + CleanInvitations.perform(nil) assert Repo.exists?(Plausible.Auth.Invitation) From a3ab3c90614e3f6230bfc778e991bc18023bd865 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 28 Oct 2024 14:54:57 +0100 Subject: [PATCH 09/34] Ensure upserted invitations and memberships are up to date on conflict (#4749) --- lib/plausible/teams/invitations.ex | 21 ++++++++++++----- .../memberships/accept_invitation_test.exs | 23 +++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/lib/plausible/teams/invitations.ex b/lib/plausible/teams/invitations.ex index 8e5eaf7c3851..f31016e5084b 100644 --- a/lib/plausible/teams/invitations.ex +++ b/lib/plausible/teams/invitations.ex @@ -195,7 +195,8 @@ defmodule Plausible.Teams.Invitations do |> Teams.SiteTransfer.changeset(initiator: initiator, email: invitee_email) |> Repo.insert( on_conflict: [set: [updated_at: now]], - conflict_target: [:email, :site_id] + conflict_target: [:email, :site_id], + returning: true ) end @@ -301,7 +302,8 @@ defmodule Plausible.Teams.Invitations do |> Teams.GuestMembership.changeset(site, old_guest_membership.role) |> Repo.insert( on_conflict: [set: [updated_at: now, role: old_guest_membership.role]], - conflict_target: [:team_membership_id, :site_id] + conflict_target: [:team_membership_id, :site_id], + returning: true ) end @@ -322,7 +324,8 @@ defmodule Plausible.Teams.Invitations do |> Teams.GuestMembership.changeset(site, :editor) |> Repo.insert( on_conflict: [set: [updated_at: now, role: :editor]], - conflict_target: [:team_membership_id, :site_id] + conflict_target: [:team_membership_id, :site_id], + returning: true ) end @@ -460,7 +463,11 @@ defmodule Plausible.Teams.Invitations do team |> Teams.Invitation.changeset(email: invitee_email, role: :guest, inviter: inviter) - |> Repo.insert(on_conflict: [set: [updated_at: now]], conflict_target: [:team_id, :email]) + |> Repo.insert( + on_conflict: [set: [updated_at: now]], + conflict_target: [:team_id, :email], + returning: true + ) end defp create_guest_invitation(team_invitation, site, role) do @@ -470,7 +477,8 @@ defmodule Plausible.Teams.Invitations do |> Teams.GuestInvitation.changeset(site, role) |> Repo.insert( on_conflict: [set: [updated_at: now]], - conflict_target: [:team_invitation_id, :site_id] + conflict_target: [:team_invitation_id, :site_id], + returning: true ) end @@ -501,7 +509,8 @@ defmodule Plausible.Teams.Invitations do |> Teams.Membership.changeset(user, role) |> Repo.insert( on_conflict: [set: [updated_at: now]], - conflict_target: [:team_id, :user_id] + conflict_target: [:team_id, :user_id], + returning: true ) end diff --git a/test/plausible/site/memberships/accept_invitation_test.exs b/test/plausible/site/memberships/accept_invitation_test.exs index 485f2d82240e..43c2f23bb73e 100644 --- a/test/plausible/site/memberships/accept_invitation_test.exs +++ b/test/plausible/site/memberships/accept_invitation_test.exs @@ -318,6 +318,29 @@ defmodule Plausible.Site.Memberships.AcceptInvitationTest do ) end + @tag :teams + test "does not create redundant guest membership when owner team membership exists" do + user = insert(:user) + {:ok, team} = Plausible.Teams.get_or_create(user) + site = insert(:site, team: team, members: [user]) + + invitation = + insert(:invitation, + site_id: site.id, + inviter: insert(:user), + email: user.email, + role: :admin + ) + + {:ok, team_membership} = + Plausible.Teams.Invitations.accept_invitation_sync(invitation, user) + + team_membership = team_membership |> Repo.reload!() |> Repo.preload(:guest_memberships) + + assert team_membership.role == :owner + assert team_membership.guest_memberships == [] + end + @tag :teams test "sync newly converted membership with team" do inviter = insert(:user) From 1a6fd1dc155233ff036425961d6227e7137694d8 Mon Sep 17 00:00:00 2001 From: RobertJoonas <56999674+RobertJoonas@users.noreply.github.com> Date: Mon, 28 Oct 2024 15:33:53 +0100 Subject: [PATCH 10/34] Remove unused controller action (#4748) * remove unused controller action * remove unused alias --- lib/plausible/stats/filter_suggestions.ex | 37 ----------------------- 1 file changed, 37 deletions(-) diff --git a/lib/plausible/stats/filter_suggestions.ex b/lib/plausible/stats/filter_suggestions.ex index c097e513a0df..353e517c8a13 100644 --- a/lib/plausible/stats/filter_suggestions.ex +++ b/lib/plausible/stats/filter_suggestions.ex @@ -8,7 +8,6 @@ defmodule Plausible.Stats.FilterSuggestions do alias Plausible.Stats.Query alias Plausible.Stats.Imported - alias Plausible.Stats.Filters def filter_suggestions(site, query, "country", filter_search) do matches = Location.search_country(filter_search) @@ -180,42 +179,6 @@ defmodule Plausible.Stats.FilterSuggestions do |> wrap_suggestions() end - # Deprecated (buggy) and will be replaced with a new endpoint (see - # custom_prop_value_filter_suggestion/4). Will only be kept around - # for some time until the dashboards get updated to query the new - # endpoint. Tests have already been adjusted to the new endpoint. - def filter_suggestions(site, query, "prop_value", filter_search) do - filter_query = if filter_search == nil, do: "%", else: "%#{filter_search}%" - - [_op, "event:props:" <> key | _rest] = Filters.get_toplevel_filter(query, "event:props") - - none_q = - from(e in base_event_query(site, Query.remove_top_level_filters(query, ["event:props"])), - select: "(none)", - where: not has_key(e, :meta, ^key), - limit: 1 - ) - - search_q = - from(e in base_event_query(site, query), - select: get_by_key(e, :meta, ^key), - where: - has_key(e, :meta, ^key) and - fragment( - "? ilike ?", - get_by_key(e, :meta, ^key), - ^filter_query - ), - group_by: get_by_key(e, :meta, ^key), - order_by: [desc: fragment("count(*)")], - limit: 25 - ) - - ClickhouseRepo.all(none_q) - |> Kernel.++(ClickhouseRepo.all(search_q)) - |> wrap_suggestions() - end - def filter_suggestions(site, query, filter_name, filter_search) do filter_search = if filter_search == nil, do: "", else: filter_search From 37116a2b1295987a634a992bb62350c973c48a40 Mon Sep 17 00:00:00 2001 From: RobertJoonas <56999674+RobertJoonas@users.noreply.github.com> Date: Mon, 28 Oct 2024 16:30:03 +0100 Subject: [PATCH 11/34] Add tests for the pageleave script (#4744) * move util function to util module * move playwright config file to tracker dir root * update Playwright + add gitignores * Playwright: enable reuseExistingServer (non-CI env) * store tracker src copies to avoid re-compilation in dev env * test pageleave on simple navigation * fix test util fn * rename local_test npm script * make test util able to expect multiple requests * test pageleaves in SPAs * test pageleave with manual URL * test pageleave not sent in manual when pageview not triggered * extend util fn to refute event requests * test pageleaves not sent in excluded hash pages * store hashes instead of file copies to detect /tracker/src changes * drop async * speed up test suite --- .gitignore | 10 ++ tracker/compile.js | 8 +- tracker/dev-compile/can-skip-compile.js | 52 ++++++++++ tracker/package-lock.json | 50 +++++++--- tracker/package.json | 6 +- .../{test/support => }/playwright.config.js | 11 ++- .../fixtures/pageleave-hash-exclusions.html | 18 ++++ tracker/test/fixtures/pageleave-hash.html | 19 ++++ tracker/test/fixtures/pageleave-manual.html | 29 ++++++ tracker/test/fixtures/pageleave.html | 24 +++++ tracker/test/manual.spec.js | 43 ++++----- tracker/test/pageleave.spec.js | 95 +++++++++++++++++++ tracker/test/support/test-utils.js | 49 ++++++++-- 13 files changed, 363 insertions(+), 51 deletions(-) create mode 100644 tracker/dev-compile/can-skip-compile.js rename tracker/{test/support => }/playwright.config.js (82%) create mode 100644 tracker/test/fixtures/pageleave-hash-exclusions.html create mode 100644 tracker/test/fixtures/pageleave-hash.html create mode 100644 tracker/test/fixtures/pageleave-manual.html create mode 100644 tracker/test/fixtures/pageleave.html create mode 100644 tracker/test/pageleave.spec.js diff --git a/.gitignore b/.gitignore index 684a1a938f9a..2e11afb9084b 100644 --- a/.gitignore +++ b/.gitignore @@ -35,6 +35,16 @@ npm-debug.log /assets/node_modules/ /tracker/node_modules/ +# Files generated by Playwright when running tracker tests +/tracker/test-results/ +/tracker/playwright-report/ +/tracker/blob-report/ +/tracker/playwright/.cache/ + +# Stored hash of source tracker files used in development environment +# to detect changes in /tracker/src and avoid unnecessary compilation. +/tracker/dev-compile/last-hash.txt + # test coverage directory /assets/coverage diff --git a/tracker/compile.js b/tracker/compile.js index c4c5add65757..b707b3a976b7 100644 --- a/tracker/compile.js +++ b/tracker/compile.js @@ -3,6 +3,12 @@ const fs = require('fs') const path = require('path') const Handlebars = require("handlebars"); const g = require("generatorics"); +const { canSkipCompile } = require("./dev-compile/can-skip-compile"); + +if (process.env.NODE_ENV === 'dev' && canSkipCompile()) { + console.info('COMPILATION SKIPPED: No changes detected in tracker dependencies') + process.exit(0) +} Handlebars.registerHelper('any', function (...args) { return args.slice(0, -1).some(Boolean) @@ -35,4 +41,4 @@ compilefile(relPath('src/p.js'), relPath('../priv/tracker/js/p.js')) variants.map(variant => { const options = variant.map(variant => variant.replace('-', '_')).reduce((acc, curr) => (acc[curr] = true, acc), {}) compilefile(relPath('src/plausible.js'), relPath(`../priv/tracker/js/plausible.${variant.join('.')}.js`), options) -}) +}) \ No newline at end of file diff --git a/tracker/dev-compile/can-skip-compile.js b/tracker/dev-compile/can-skip-compile.js new file mode 100644 index 000000000000..9a955087d649 --- /dev/null +++ b/tracker/dev-compile/can-skip-compile.js @@ -0,0 +1,52 @@ +const fs = require('fs'); +const path = require('path'); +const crypto = require('crypto'); + +const LAST_HASH_FILEPATH = path.join(__dirname, './last-hash.txt') + +// Re-compilation is only required if any of these files have been changed. +const COMPILE_DEPENDENCIES = [ + path.join(__dirname, '../compile.js'), + path.join(__dirname, '../src/plausible.js'), + path.join(__dirname, '../src/customEvents.js') +] + +function currentHash() { + const combinedHash = crypto.createHash('sha256'); + + for (const filePath of COMPILE_DEPENDENCIES) { + try { + const fileContent = fs.readFileSync(filePath); + const fileHash = crypto.createHash('sha256').update(fileContent).digest(); + combinedHash.update(fileHash); + } catch (error) { + throw new Error(`Failed to read or hash ${filePath}: ${error.message}`); + } + } + + return combinedHash.digest('hex'); +} + +function lastHash() { + if (fs.existsSync(LAST_HASH_FILEPATH)) { + return fs.readFileSync(LAST_HASH_FILEPATH).toString() + } +} + +/** + * Returns a boolean indicating whether the tracker compilation can be skipped. + * Every time this function gets executed, the hash of the tracker dependencies + * will be updated. Compilation can be skipped if the hash hasn't changed since + * the last execution. + */ +exports.canSkipCompile = function() { + const current = currentHash() + const last = lastHash() + + if (current === last) { + return true + } else { + fs.writeFileSync(LAST_HASH_FILEPATH, current) + return false + } +} \ No newline at end of file diff --git a/tracker/package-lock.json b/tracker/package-lock.json index 94e432413b5d..975fe9bb734c 100644 --- a/tracker/package-lock.json +++ b/tracker/package-lock.json @@ -6,13 +6,14 @@ "": { "license": "MIT", "dependencies": { - "@playwright/test": "^1.41.1", "express": "^4.18.1", "generatorics": "^1.1.0", "handlebars": "^4.7.8", "uglify-js": "^3.9.4" }, "devDependencies": { + "@playwright/test": "^1.48.1", + "@types/node": "^22.7.7", "eslint": "^8.56.0", "eslint-plugin-playwright": "^0.20.0" } @@ -151,17 +152,27 @@ } }, "node_modules/@playwright/test": { - "version": "1.41.1", - "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.41.1.tgz", - "integrity": "sha512-9g8EWTjiQ9yFBXc6HjCWe41msLpxEX0KhmfmPl9RPLJdfzL4F0lg2BdJ91O9azFdl11y1pmpwdjBiSxvqc+btw==", + "version": "1.48.1", + "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.48.1.tgz", + "integrity": "sha512-s9RtWoxkOLmRJdw3oFvhFbs9OJS0BzrLUc8Hf6l2UdCNd1rqeEyD4BhCJkvzeEoD1FsK4mirsWwGerhVmYKtZg==", + "dev": true, "dependencies": { - "playwright": "1.41.1" + "playwright": "1.48.1" }, "bin": { "playwright": "cli.js" }, "engines": { - "node": ">=16" + "node": ">=18" + } + }, + "node_modules/@types/node": { + "version": "22.7.7", + "resolved": "https://registry.npmjs.org/@types/node/-/node-22.7.7.tgz", + "integrity": "sha512-SRxCrrg9CL/y54aiMCG3edPKdprgMVGDXjA3gB8UmmBW5TcXzRUYAh8EWzTnSJFAd1rgImPELza+A3bJ+qxz8Q==", + "dev": true, + "dependencies": { + "undici-types": "~6.19.2" } }, "node_modules/@ungap/structured-clone": { @@ -855,6 +866,7 @@ "version": "2.3.2", "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "dev": true, "hasInstallScript": true, "optional": true, "os": [ @@ -1395,31 +1407,33 @@ "integrity": "sha512-5DFkuoqlv1uYQKxy8omFBeJPQcdoE07Kv2sferDCrAq1ohOU+MSDswDIbnx3YAM60qIOnYa53wBhXW0EbMonrQ==" }, "node_modules/playwright": { - "version": "1.41.1", - "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.41.1.tgz", - "integrity": "sha512-gdZAWG97oUnbBdRL3GuBvX3nDDmUOuqzV/D24dytqlKt+eI5KbwusluZRGljx1YoJKZ2NRPaeWiFTeGZO7SosQ==", + "version": "1.48.1", + "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.48.1.tgz", + "integrity": "sha512-j8CiHW/V6HxmbntOfyB4+T/uk08tBy6ph0MpBXwuoofkSnLmlfdYNNkFTYD6ofzzlSqLA1fwH4vwvVFvJgLN0w==", + "dev": true, "dependencies": { - "playwright-core": "1.41.1" + "playwright-core": "1.48.1" }, "bin": { "playwright": "cli.js" }, "engines": { - "node": ">=16" + "node": ">=18" }, "optionalDependencies": { "fsevents": "2.3.2" } }, "node_modules/playwright-core": { - "version": "1.41.1", - "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.41.1.tgz", - "integrity": "sha512-/KPO5DzXSMlxSX77wy+HihKGOunh3hqndhqeo/nMxfigiKzogn8kfL0ZBDu0L1RKgan5XHCPmn6zXd2NUJgjhg==", + "version": "1.48.1", + "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.48.1.tgz", + "integrity": "sha512-Yw/t4VAFX/bBr1OzwCuOMZkY1Cnb4z/doAFSwf4huqAGWmf9eMNjmK7NiOljCdLmxeRYcGPPmcDgU0zOlzP0YA==", + "dev": true, "bin": { "playwright-core": "cli.js" }, "engines": { - "node": ">=16" + "node": ">=18" } }, "node_modules/prelude-ls": { @@ -1796,6 +1810,12 @@ "node": ">=0.8.0" } }, + "node_modules/undici-types": { + "version": "6.19.8", + "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-6.19.8.tgz", + "integrity": "sha512-ve2KP6f/JnbPBFyobGHuerC9g1FYGn/F8n1LWTwNxCEzd6IfqTwUQcNXgEtmmQ6DlRrC1hrSrBnCZPokRrDHjw==", + "dev": true + }, "node_modules/unpipe": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/unpipe/-/unpipe-1.0.0.tgz", diff --git a/tracker/package.json b/tracker/package.json index d1b222583f15..ef202664c872 100644 --- a/tracker/package.json +++ b/tracker/package.json @@ -1,19 +1,21 @@ { "scripts": { "deploy": "node compile.js", - "test": "npm run deploy && npx playwright test --config=./test/support/playwright.config.js", + "test": "npm run deploy && npx playwright test", + "test:local": "NODE_ENV=dev npm run deploy && npx playwright test", "report-sizes": "node report-sizes.js", "start": "node test/support/server.js" }, "license": "MIT", "dependencies": { - "@playwright/test": "^1.41.1", "express": "^4.18.1", "generatorics": "^1.1.0", "handlebars": "^4.7.8", "uglify-js": "^3.9.4" }, "devDependencies": { + "@playwright/test": "^1.48.1", + "@types/node": "^22.7.7", "eslint": "^8.56.0", "eslint-plugin-playwright": "^0.20.0" } diff --git a/tracker/test/support/playwright.config.js b/tracker/playwright.config.js similarity index 82% rename from tracker/test/support/playwright.config.js rename to tracker/playwright.config.js index 67c91de05868..21dc56f49ebe 100644 --- a/tracker/test/support/playwright.config.js +++ b/tracker/playwright.config.js @@ -1,10 +1,11 @@ -const { devices } = require('@playwright/test'); +// @ts-check +const { defineConfig, devices } = require('@playwright/test'); /** * @see https://playwright.dev/docs/test-configuration */ -module.exports = { - testDir: '../', +module.exports = defineConfig({ + testDir: './test', timeout: 60 * 1000, fullyParallel: true, /* Fail the build on CI if you accidentally left test.only in the source code. */ @@ -33,5 +34,7 @@ module.exports = { webServer: { command: 'npm run start', port: 3000, + reuseExistingServer: !process.env.CI }, -} +}); + diff --git a/tracker/test/fixtures/pageleave-hash-exclusions.html b/tracker/test/fixtures/pageleave-hash-exclusions.html new file mode 100644 index 000000000000..aecca154e936 --- /dev/null +++ b/tracker/test/fixtures/pageleave-hash-exclusions.html @@ -0,0 +1,18 @@ + + + + + + + + Plausible Playwright tests + + + + + Ignored Hash Link + Hash Link 1 + Hash Link 2 + + + diff --git a/tracker/test/fixtures/pageleave-hash.html b/tracker/test/fixtures/pageleave-hash.html new file mode 100644 index 000000000000..e164362ccf29 --- /dev/null +++ b/tracker/test/fixtures/pageleave-hash.html @@ -0,0 +1,19 @@ + + + + + + + + Plausible Playwright tests + + + + + + Hash link + + + diff --git a/tracker/test/fixtures/pageleave-manual.html b/tracker/test/fixtures/pageleave-manual.html new file mode 100644 index 000000000000..6fa3fd5423ce --- /dev/null +++ b/tracker/test/fixtures/pageleave-manual.html @@ -0,0 +1,29 @@ + + + + + + + + Plausible Playwright tests + + + + + + Navigate away + + + + + + + diff --git a/tracker/test/fixtures/pageleave.html b/tracker/test/fixtures/pageleave.html new file mode 100644 index 000000000000..bb94b92ee2b6 --- /dev/null +++ b/tracker/test/fixtures/pageleave.html @@ -0,0 +1,24 @@ + + + + + + + + Plausible Playwright tests + + + + + Navigate away + + + + + + + diff --git a/tracker/test/manual.spec.js b/tracker/test/manual.spec.js index 0815d7276682..f67a7fccb153 100644 --- a/tracker/test/manual.spec.js +++ b/tracker/test/manual.spec.js @@ -1,35 +1,34 @@ -const { mockRequest } = require('./support/test-utils') -const { expect, test } = require('@playwright/test'); +/* eslint-disable playwright/expect-expect */ +const { clickPageElementAndExpectEventRequests } = require('./support/test-utils') +const { test } = require('@playwright/test'); const { LOCAL_SERVER_ADDR } = require('./support/server'); -async function clickPageElementAndExpectEventRequest(page, buttonId, expectedBodyParams) { - const plausibleRequestMock = mockRequest(page, '/api/event') - await page.click(buttonId) - const plausibleRequest = await plausibleRequestMock; - - expect(plausibleRequest.url()).toContain('/api/event') - - const body = plausibleRequest.postDataJSON() - - Object.keys(expectedBodyParams).forEach((key) => { - expect(body[key]).toEqual(expectedBodyParams[key]) - }) -} - test.describe('manual extension', () => { test('can trigger custom events with and without a custom URL if pageview was sent with the default URL', async ({ page }) => { await page.goto('/manual.html'); - await clickPageElementAndExpectEventRequest(page, '#pageview-trigger', {n: 'pageview', u: `${LOCAL_SERVER_ADDR}/manual.html`}) - await clickPageElementAndExpectEventRequest(page, '#custom-event-trigger', {n: 'CustomEvent', u: `${LOCAL_SERVER_ADDR}/manual.html`}) - await clickPageElementAndExpectEventRequest(page, '#custom-event-trigger-custom-url', {n: 'CustomEvent', u: `https://example.com/custom/location`}) + await clickPageElementAndExpectEventRequests(page, '#pageview-trigger', [ + {n: 'pageview', u: `${LOCAL_SERVER_ADDR}/manual.html`} + ]) + await clickPageElementAndExpectEventRequests(page, '#custom-event-trigger', [ + {n: 'CustomEvent', u: `${LOCAL_SERVER_ADDR}/manual.html`} + ]) + await clickPageElementAndExpectEventRequests(page, '#custom-event-trigger-custom-url', [ + {n: 'CustomEvent', u: `https://example.com/custom/location`} + ]) }); test('can trigger custom events with and without a custom URL if pageview was sent with a custom URL', async ({ page }) => { await page.goto('/manual.html'); - await clickPageElementAndExpectEventRequest(page, '#pageview-trigger-custom-url', {n: 'pageview', u: `https://example.com/custom/location`}) - await clickPageElementAndExpectEventRequest(page, '#custom-event-trigger', {n: 'CustomEvent', u: `${LOCAL_SERVER_ADDR}/manual.html`}) - await clickPageElementAndExpectEventRequest(page, '#custom-event-trigger-custom-url', {n: 'CustomEvent', u: `https://example.com/custom/location`}) + await clickPageElementAndExpectEventRequests(page, '#pageview-trigger-custom-url', [ + {n: 'pageview', u: `https://example.com/custom/location`} + ]) + await clickPageElementAndExpectEventRequests(page, '#custom-event-trigger', [ + {n: 'CustomEvent', u: `${LOCAL_SERVER_ADDR}/manual.html`} + ]) + await clickPageElementAndExpectEventRequests(page, '#custom-event-trigger-custom-url', [ + {n: 'CustomEvent', u: `https://example.com/custom/location`} + ]) }); }); diff --git a/tracker/test/pageleave.spec.js b/tracker/test/pageleave.spec.js new file mode 100644 index 000000000000..cfc2acad4464 --- /dev/null +++ b/tracker/test/pageleave.spec.js @@ -0,0 +1,95 @@ +/* eslint-disable playwright/expect-expect */ +/* eslint-disable playwright/no-skipped-test */ +const { clickPageElementAndExpectEventRequests, mockRequest } = require('./support/test-utils') +const { test, expect } = require('@playwright/test'); +const { LOCAL_SERVER_ADDR } = require('./support/server'); + +test.describe('pageleave extension', () => { + test.skip(({browserName}) => browserName === 'webkit', 'Not testable on Webkit'); + + test('sends a pageleave when navigating to the next page', async ({ page }) => { + const pageviewRequestMock = mockRequest(page, '/api/event') + await page.goto('/pageleave.html'); + await pageviewRequestMock; + + await clickPageElementAndExpectEventRequests(page, '#navigate-away', [ + {n: 'pageleave', u: `${LOCAL_SERVER_ADDR}/pageleave.html`} + ]) + }); + + test('sends pageleave and pageview on hash-based SPA navigation', async ({ page }) => { + const pageviewRequestMock = mockRequest(page, '/api/event') + await page.goto('/pageleave-hash.html'); + await pageviewRequestMock; + + await clickPageElementAndExpectEventRequests(page, '#hash-nav', [ + {n: 'pageleave', u: `${LOCAL_SERVER_ADDR}/pageleave-hash.html`}, + {n: 'pageview', u: `${LOCAL_SERVER_ADDR}/pageleave-hash.html#some-hash`} + ]) + }); + + test('sends pageleave and pageview on history-based SPA navigation', async ({ page }) => { + const pageviewRequestMock = mockRequest(page, '/api/event') + await page.goto('/pageleave.html'); + await pageviewRequestMock; + + await clickPageElementAndExpectEventRequests(page, '#history-nav', [ + {n: 'pageleave', u: `${LOCAL_SERVER_ADDR}/pageleave.html`}, + {n: 'pageview', u: `${LOCAL_SERVER_ADDR}/another-page`} + ]) + }); + + test('sends pageleave with the manually overridden URL', async ({ page }) => { + await page.goto('/pageleave-manual.html'); + + await clickPageElementAndExpectEventRequests(page, '#pageview-trigger-custom-url', [ + {n: 'pageview', u: 'https://example.com/custom/location'} + ]) + + await clickPageElementAndExpectEventRequests(page, '#navigate-away', [ + {n: 'pageleave', u: 'https://example.com/custom/location'} + ]) + }); + + test('does not send pageleave when pageview was not sent in manual mode', async ({ page }) => { + await page.goto('/pageleave-manual.html'); + + await clickPageElementAndExpectEventRequests(page, '#navigate-away', [], [ + {n: 'pageleave'} + ]) + }); + + test('script.exclusions.hash.pageleave.js sends pageleave only from URLs where a pageview was sent', async ({ page }) => { + const pageBaseURL = `${LOCAL_SERVER_ADDR}/pageleave-hash-exclusions.html` + + const pageviewRequestMock = mockRequest(page, '/api/event') + await page.goto('/pageleave-hash-exclusions.html'); + await pageviewRequestMock; + + // After the initial pageview is sent, navigate to ignored page -> + // pageleave event is sent from the initial page URL + await clickPageElementAndExpectEventRequests(page, '#ignored-hash-link', [ + {n: 'pageleave', u: pageBaseURL, h: 1} + ]) + + // Navigate from ignored page to a tracked page -> + // no pageleave from the current page, pageview on the next page + await clickPageElementAndExpectEventRequests( + page, + '#hash-link-1', + [{n: 'pageview', u: `${pageBaseURL}#hash1`, h: 1}], + [{n: 'pageleave'}] + ) + + // Navigate from a tracked page to another tracked page -> + // pageleave with the last page URL, pageview with the new URL + await clickPageElementAndExpectEventRequests( + page, + '#hash-link-2', + [ + {n: 'pageleave', u: `${pageBaseURL}#hash1`, h: 1}, + {n: 'pageview', u: `${pageBaseURL}#hash2`, h: 1} + ], + ) + }); +}); \ No newline at end of file diff --git a/tracker/test/support/test-utils.js b/tracker/test/support/test-utils.js index 6c01e4f3ba31..10758ecb1bea 100644 --- a/tracker/test/support/test-utils.js +++ b/tracker/test/support/test-utils.js @@ -1,10 +1,10 @@ const { expect } = require("@playwright/test"); // Mocks an HTTP request call with the given path. Returns a Promise that resolves to the request -// data. If the request is not made, resolves to null after 10 seconds. -exports.mockRequest = function (page, path) { +// data. If the request is not made, resolves to null after 3 seconds. +const mockRequest = function (page, path) { return new Promise((resolve, _reject) => { - const requestTimeoutTimer = setTimeout(() => resolve(null), 10000) + const requestTimeoutTimer = setTimeout(() => resolve(null), 3000) page.route(path, (route, request) => { clearTimeout(requestTimeoutTimer) @@ -14,6 +14,8 @@ exports.mockRequest = function (page, path) { }) } +exports.mockRequest = mockRequest + exports.metaKey = function() { if (process.platform === 'darwin') { return 'Meta' @@ -23,13 +25,13 @@ exports.metaKey = function() { } // Mocks a specified number of HTTP requests with given path. Returns a promise that resolves to a -// list of requests as soon as the specified number of requests is made, or 10 seconds has passed. -exports.mockManyRequests = function(page, path, numberOfRequests) { +// list of requests as soon as the specified number of requests is made, or 3 seconds has passed. +const mockManyRequests = function(page, path, numberOfRequests) { return new Promise((resolve, _reject) => { let requestList = [] - const requestTimeoutTimer = setTimeout(() => resolve(requestList), 10000) + const requestTimeoutTimer = setTimeout(() => resolve(requestList), 3000) - page.route('/api/event', (route, request) => { + page.route(path, (route, request) => { requestList.push(request) if (requestList.length === numberOfRequests) { clearTimeout(requestTimeoutTimer) @@ -40,6 +42,8 @@ exports.mockManyRequests = function(page, path, numberOfRequests) { }) } +exports.mockManyRequests = mockManyRequests + exports.expectCustomEvent = function (request, eventName, eventProps) { const payload = request.postDataJSON() @@ -49,3 +53,34 @@ exports.expectCustomEvent = function (request, eventName, eventProps) { expect(payload.p[key]).toEqual(value) } } + +exports.clickPageElementAndExpectEventRequests = async function (page, locatorToClick, expectedBodySubsets, refutedBodySubsets = []) { + const requestsToExpect = expectedBodySubsets.length + const requestsToAwait = requestsToExpect + refutedBodySubsets.length + + const plausibleRequestMockList = mockManyRequests(page, '/api/event', requestsToAwait) + await page.click(locatorToClick) + const requests = await plausibleRequestMockList + + expect(requests.length).toBe(requestsToExpect) + + expectedBodySubsets.forEach((bodySubset) => { + expect(requests.some((request) => { + return hasExpectedBodyParams(request, bodySubset) + })).toBe(true) + }) + + refutedBodySubsets.forEach((bodySubset) => { + expect(requests.every((request) => { + return !hasExpectedBodyParams(request, bodySubset) + })).toBe(true) + }) +} + +function hasExpectedBodyParams(request, expectedBodyParams) { + const body = request.postDataJSON() + + return Object.keys(expectedBodyParams).every((key) => { + return body[key] === expectedBodyParams[key] + }) +} From ad8f49b38dd0fba6183d26fee80b05f438f29031 Mon Sep 17 00:00:00 2001 From: RobertJoonas <56999674+RobertJoonas@users.noreply.github.com> Date: Tue, 29 Oct 2024 11:59:58 +0100 Subject: [PATCH 12/34] fix dashboard crash (#4754) --- assets/js/dashboard/stats/graph/visitor-graph.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/js/dashboard/stats/graph/visitor-graph.js b/assets/js/dashboard/stats/graph/visitor-graph.js index 8d2172bf3afb..4f74868f5c1c 100644 --- a/assets/js/dashboard/stats/graph/visitor-graph.js +++ b/assets/js/dashboard/stats/graph/visitor-graph.js @@ -19,7 +19,7 @@ import { ExclamationCircleIcon } from '@heroicons/react/24/outline' function fetchTopStats(site, query) { const q = { ...query } - if (!isComparisonEnabled(q.comparison)) { + if (!isComparisonEnabled(q.comparison) && query.period !== 'realtime') { q.comparison = 'previous_period' } From dc7e019d58a228f4feb124949553914284a0dd76 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Tue, 29 Oct 2024 12:24:01 +0100 Subject: [PATCH 13/34] Adjust GA4 import timeout and page size to reduce risk of 502s and timeouts (#4753) --- lib/plausible/google/ga4/api.ex | 2 +- lib/plausible/google/ga4/http.ex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/plausible/google/ga4/api.ex b/lib/plausible/google/ga4/api.ex index 729914981447..b5ebc98bdedf 100644 --- a/lib/plausible/google/ga4/api.ex +++ b/lib/plausible/google/ga4/api.ex @@ -14,7 +14,7 @@ defmodule Plausible.Google.GA4.API do expires_at :: String.t() } - @per_page 250_000 + @per_page 200_000 @backoff_factor :timer.seconds(10) @max_attempts 5 diff --git a/lib/plausible/google/ga4/http.ex b/lib/plausible/google/ga4/http.ex index 437fd1ed106f..aedbf5f92922 100644 --- a/lib/plausible/google/ga4/http.ex +++ b/lib/plausible/google/ga4/http.ex @@ -48,7 +48,7 @@ defmodule Plausible.Google.GA4.HTTP do url, [{"Authorization", "Bearer #{report_request.access_token}"}], params, - receive_timeout: 60_000 + receive_timeout: 80_000 ) with {:ok, %{body: body}} <- response, From b5d48a7347fdc6d3e18883eb0231e68a485d0f27 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Wed, 30 Oct 2024 12:19:47 +0200 Subject: [PATCH 14/34] Breakdown comparisons: Tooltip UX, arrows (#4719) * Revert "Revert "Remove no change icon, better alignment"" This reverts commit a69d0a14a7fe038fe110907571240ad99a2ba6a4. * different arrows * Render graph tooltips in react * Use ChangeArrow in graph tooltips * Only display a single date if viewing a single date * Tests for breakdown responses with labels * Simplify code a bit * strokeClass * refactor ChangeArrow * Play with stroke widths * jest tests * Silence logging in jest tests * not variable stroke width * prettier * Conversion rate widths * Update a test * Trim trailing dates for label when comparing running month * Reformat file * Update expect.toHaveTextContent for multiline * Update date range labelling to be more consistent with the frontend * reformat --- assets/js/dashboard/query.ts | 5 ++ .../js/dashboard/stats/graph/graph-tooltip.js | 64 ++++++++------- .../stats/modals/breakdown-modal.tsx | 14 ++-- .../stats/reports/change-arrow.test.tsx | 18 +++++ .../dashboard/stats/reports/change-arrow.tsx | 37 ++++----- assets/js/dashboard/stats/reports/list.js | 4 +- .../stats/reports/metric-value.test.tsx | 78 ++++++++++++++++--- .../dashboard/stats/reports/metric-value.tsx | 48 ++++++++---- assets/js/dashboard/stats/reports/metrics.js | 7 +- .../dashboard/util/url-search-params.test.ts | 5 ++ lib/plausible/stats/breakdown.ex | 47 ++++++++++- lib/plausible/stats/comparisons.ex | 12 +-- lib/plausible/stats/query.ex | 17 +++- .../controllers/api/stats_controller.ex | 27 ++++++- lib/plausible_web/live/sites.ex | 32 +++++++- .../api/stats_controller/browsers_test.exs | 10 +++ .../api/stats_controller/sources_test.exs | 9 +++ 17 files changed, 341 insertions(+), 93 deletions(-) diff --git a/assets/js/dashboard/query.ts b/assets/js/dashboard/query.ts index c93e2b03ddfd..c9d345220147 100644 --- a/assets/js/dashboard/query.ts +++ b/assets/js/dashboard/query.ts @@ -55,6 +55,11 @@ export const queryDefaultValue = { export type DashboardQuery = typeof queryDefaultValue +export type BreakdownResultMeta = { + date_range_label: string + comparison_date_range_label?: string +} + export function addFilter( query: DashboardQuery, filter: Filter diff --git a/assets/js/dashboard/stats/graph/graph-tooltip.js b/assets/js/dashboard/stats/graph/graph-tooltip.js index fffeec828c8f..046e6b651dcd 100644 --- a/assets/js/dashboard/stats/graph/graph-tooltip.js +++ b/assets/js/dashboard/stats/graph/graph-tooltip.js @@ -1,6 +1,9 @@ +import React from 'react' +import { createRoot } from 'react-dom/client' import dateFormatter from './date-formatter' import { METRIC_LABELS } from './graph-util' import { MetricFormatterShort } from '../reports/metric-formatter' +import { ChangeArrow } from '../reports/change-arrow' const renderBucketLabel = function(query, graphData, label, comparison = false) { let isPeriodFull = graphData.full_intervals?.[label] @@ -52,6 +55,9 @@ const buildTooltipData = function(query, graphData, metric, tooltipModel) { return { label, formattedValue, comparisonLabel, formattedComparisonValue, comparisonDifference } } + +let tooltipRoot + export default function GraphTooltip(graphData, metric, query) { return (context) => { const tooltipModel = context.tooltip @@ -64,6 +70,7 @@ export default function GraphTooltip(graphData, metric, query) { tooltipEl.style.display = 'none' tooltipEl.style.opacity = 0 document.body.appendChild(tooltipEl) + tooltipRoot = createRoot(tooltipEl) } if (tooltipEl && offset && window.innerWidth < 768) { @@ -81,42 +88,41 @@ export default function GraphTooltip(graphData, metric, query) { if (tooltipModel.body) { const tooltipData = buildTooltipData(query, graphData, metric, tooltipModel) - tooltipEl.innerHTML = ` -