Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle missing scroll depth #2 #5017

Merged
merged 23 commits into from
Jan 29, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
c3c1039
add migration
RobertJoonas Jan 9, 2025
7974895
move scroll_depth_enabled? fn
RobertJoonas Jan 15, 2025
4d02ae4
maybe set engagement_metrics_enabled_at when requesting dashboard
RobertJoonas Jan 15, 2025
8ab805b
maybe set engagement_metrics_enabled_at in shared_link action
RobertJoonas Jan 15, 2025
9850450
maybe set engagement_metrics_enabled_at on full export
RobertJoonas Jan 15, 2025
fb279b3
fix tests
RobertJoonas Jan 16, 2025
cdf2c83
feature gate scroll depth on the dashboard with site.engagement_metri…
RobertJoonas Jan 16, 2025
19fdda7
feature gate scroll depth in full export too
RobertJoonas Jan 16, 2025
8c3c922
fix npm ci
RobertJoonas Jan 16, 2025
9631f60
Rename things into FE, remove unneccessary flag checks
macobo Jan 23, 2025
cc26755
Continue with renaming
macobo Jan 23, 2025
8ea7088
Merge remote-tracking branch 'origin/master' into handle-missing-scro…
macobo Jan 23, 2025
8ea9eca
Rename site flag to be more descriptive
macobo Jan 23, 2025
80fc6c6
Move business logic, calculate based on scroll depth, make more preca…
macobo Jan 23, 2025
cf2bb17
Some docs
macobo Jan 23, 2025
7c1bf77
Rename to site.scrollDepthVisible in frontend
macobo Jan 23, 2025
422ed9a
Update migration
macobo Jan 27, 2025
957f029
Fix template
macobo Jan 27, 2025
3a94d2c
Remove boilerplate from tests
macobo Jan 27, 2025
a6516cd
Update tests
macobo Jan 27, 2025
8586103
More straight-forward test
macobo Jan 27, 2025
9132205
Update condition
macobo Jan 28, 2025
09fb06b
Merge remote-tracking branch 'origin/master' into missing-scroll-depth
macobo Jan 28, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions assets/js/dashboard/site-context.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ describe('parseSiteFromDataset', () => {
data-revenue-goals='[{"currency":"USD","display_name":"Purchase"}]'
data-funnels='[{"id":1,"name":"From homepage to login","steps_count":3}]'
data-has-props="true"
data-scroll-depth-visible="true"
data-logged-in="true"
data-stats-begin="2021-09-07"
data-native-stats-begin="2022-09-02"
Expand All @@ -43,6 +44,7 @@ describe('parseSiteFromDataset', () => {
revenueGoals: [{ currency: 'USD', display_name: 'Purchase' }],
funnels: [{ id: 1, name: 'From homepage to login', steps_count: 3 }],
hasProps: true,
scrollDepthVisible: true,
statsBegin: '2021-09-07',
nativeStatsBegin: '2022-09-02',
embedded: false,
Expand Down
2 changes: 2 additions & 0 deletions assets/js/dashboard/site-context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export function parseSiteFromDataset(dataset: DOMStringMap): PlausibleSite {
offset: parseInt(dataset.offset!, 10),
hasGoals: dataset.hasGoals === 'true',
hasProps: dataset.hasProps === 'true',
scrollDepthVisible: dataset.scrollDepthVisible === 'true',
funnelsAvailable: dataset.funnelsAvailable === 'true',
propsAvailable: dataset.propsAvailable === 'true',
conversionsOptedOut: dataset.conversionsOptedOut === 'true',
Expand Down Expand Up @@ -36,6 +37,7 @@ const siteContextDefaultValue = {
offset: 0,
hasGoals: false,
hasProps: false,
scrollDepthVisible: false,
funnelsAvailable: false,
propsAvailable: false,
conversionsOptedOut: false,
Expand Down
2 changes: 1 addition & 1 deletion assets/js/dashboard/stats/graph/graph-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export function getGraphableMetrics(query, site) {
return ["visitors", "events", "conversion_rate"]
} else if (isPageFilter) {
const pageFilterMetrics = ["visitors", "visits", "pageviews", "bounce_rate"]
return site.flags.scroll_depth ? [...pageFilterMetrics, "scroll_depth"] : pageFilterMetrics
return site.scrollDepthVisible ? [...pageFilterMetrics, "scroll_depth"] : pageFilterMetrics
} else {
return ["visitors", "visits", "pageviews", "views_per_visit", "bounce_rate", "visit_duration"]
}
Expand Down
2 changes: 1 addition & 1 deletion assets/js/dashboard/stats/modals/pages.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function PagesModal() {
metrics.createTimeOnPage()
]

return site.flags.scroll_depth ? [...defaultMetrics, metrics.createScrollDepth()] : defaultMetrics
return site.scrollDepthVisible ? [...defaultMetrics, metrics.createScrollDepth()] : defaultMetrics
}

return (
Expand Down
1 change: 1 addition & 0 deletions assets/test-utils/app-context-providers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export const TestContextProviders = ({
offset: 0,
hasGoals: false,
hasProps: false,
scrollDepthVisible: false,
funnelsAvailable: false,
propsAvailable: false,
conversionsOptedOut: false,
Expand Down
5 changes: 2 additions & 3 deletions lib/plausible/exports.ex
Original file line number Diff line number Diff line change
Expand Up @@ -418,8 +418,7 @@ defmodule Plausible.Exports do
site = Plausible.Repo.get(Plausible.Site, site_id)
current_user = current_user_id && Plausible.Repo.get(Plausible.Auth.User, current_user_id)

scroll_depth_enabled? =
PlausibleWeb.Api.StatsController.scroll_depth_enabled?(site, current_user)
include_scroll_depth? = Plausible.Stats.ScrollDepth.check_feature_visible!(site, current_user)

base_q =
from(e in sampled("events_v2"),
Expand All @@ -429,7 +428,7 @@ defmodule Plausible.Exports do
order_by: selected_as(:date)
)

if scroll_depth_enabled? do
if include_scroll_depth? do
max_scroll_depth_per_visitor_q =
from(e in "events_v2",
where: ^export_filter(site_id, date_range),
Expand Down
1 change: 1 addition & 0 deletions lib/plausible/site.ex
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ defmodule Plausible.Site do
field :conversions_enabled, :boolean, default: true
field :props_enabled, :boolean, default: true
field :funnels_enabled, :boolean, default: true
field :scroll_depth_visible_at, :naive_datetime

field :ingest_rate_limit_scale_seconds, :integer, default: 60
# default is set via changeset/2
Expand Down
8 changes: 8 additions & 0 deletions lib/plausible/sites.ex
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,14 @@ defmodule Plausible.Sites do
|> Repo.update!()
end

def set_scroll_depth_visible_at(site) do
utc_now = NaiveDateTime.utc_now(:second)

site
|> Ecto.Changeset.change(%{scroll_depth_visible_at: utc_now})
|> Repo.update()
end

def has_goals?(site) do
Repo.exists?(
from(g in Plausible.Goal,
Expand Down
63 changes: 63 additions & 0 deletions lib/plausible/stats/scroll_depth.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
defmodule Plausible.Stats.ScrollDepth do
@moduledoc """
Module to check whether the scroll depth metric is available and visible for a site.
"""

import Ecto.Query
require Logger

alias Plausible.ClickhouseRepo

def feature_available?(site, user) do
FunWithFlags.enabled?(:scroll_depth, for: user) ||
FunWithFlags.enabled?(:scroll_depth, for: site)
end

def feature_visible?(site, user) do
feature_available?(site, user) && not is_nil(site.scroll_depth_visible_at)
end

@doc """
Checks whether the scroll depth feature is visible for a site and updates the site record if it is.

Note this function queries ClickHouse and may take a while to complete.
"""
def check_feature_visible!(site, user) do
cond do
not feature_available?(site, user) ->
false

not is_nil(site.scroll_depth_visible_at) ->
true

is_nil(site.scroll_depth_visible_at) ->
visible? = has_scroll_data_last_30d?(site)

if visible? do
Plausible.Sites.set_scroll_depth_visible_at(site)
end

visible?
end
end

defp has_scroll_data_last_30d?(site) do
try do
ClickhouseRepo.exists?(
from(e in "events_v2",
where:
e.site_id == ^site.id and
e.name == "pageleave" and
e.timestamp >= fragment("toStartOfDay(now() - toIntervalDay(30))") and
e.scroll_depth > 0 and e.scroll_depth <= 100
)
)
rescue
# Avoid propagating error to the user, bringing down the site.
error ->
Logger.error("Error checking scroll data for site #{site.id}: #{inspect(error)}")

false
end
end
end
13 changes: 4 additions & 9 deletions lib/plausible_web/controllers/api/stats_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -389,16 +389,16 @@ defmodule PlausibleWeb.Api.StatsController do
page_filter? =
Filters.filtering_on_dimension?(query, "event:page", behavioral_filters: :ignore)

scroll_depth_enabled? = scroll_depth_enabled?(site, current_user)
include_scroll_depth? = Plausible.Stats.ScrollDepth.feature_visible?(site, current_user)

metrics = [:visitors, :visits, :pageviews, :sample_percent]

metrics =
cond do
page_filter? && scroll_depth_enabled? && query.include_imported ->
page_filter? && include_scroll_depth? && query.include_imported ->
metrics ++ [:scroll_depth]

page_filter? && scroll_depth_enabled? ->
page_filter? && include_scroll_depth? ->
metrics ++ [:bounce_rate, :scroll_depth, :time_on_page]

page_filter? && query.include_imported ->
Expand Down Expand Up @@ -851,7 +851,7 @@ defmodule PlausibleWeb.Api.StatsController do
params = Map.put(params, "property", "event:page")
query = Query.from(site, params, debug_metadata(conn))

include_scroll_depth? = scroll_depth_enabled?(site, current_user)
include_scroll_depth? = Plausible.Stats.ScrollDepth.feature_visible?(site, current_user)

extra_metrics =
cond do
Expand Down Expand Up @@ -1663,9 +1663,4 @@ defmodule PlausibleWeb.Api.StatsController do
defp toplevel_goal_filter?(query) do
Filters.filtering_on_dimension?(query, "event:goal", max_depth: 0)
end

def scroll_depth_enabled?(site, user) do
FunWithFlags.enabled?(:scroll_depth, for: user) ||
FunWithFlags.enabled?(:scroll_depth, for: site)
end
end
18 changes: 14 additions & 4 deletions lib/plausible_web/controllers/stats_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,16 @@ defmodule PlausibleWeb.StatsController do

def stats(%{assigns: %{site: site}} = conn, _params) do
site = Plausible.Repo.preload(site, :owner)
current_user = conn.assigns[:current_user]
stats_start_date = Plausible.Sites.stats_start_date(site)
can_see_stats? = not Sites.locked?(site) or conn.assigns[:site_role] == :super_admin
demo = site.domain == PlausibleWeb.Endpoint.host()
dogfood_page_path = if demo, do: "/#{site.domain}", else: "/:dashboard"
skip_to_dashboard? = conn.params["skip_to_dashboard"] == "true"

scroll_depth_visible? =
Plausible.Stats.ScrollDepth.check_feature_visible!(site, current_user)

cond do
(stats_start_date && can_see_stats?) || (can_see_stats? && skip_to_dashboard?) ->
conn
Expand All @@ -68,11 +72,12 @@ defmodule PlausibleWeb.StatsController do
revenue_goals: list_revenue_goals(site),
funnels: list_funnels(site),
has_props: Plausible.Props.configured?(site),
scroll_depth_visible: scroll_depth_visible?,
stats_start_date: stats_start_date,
native_stats_start_date: NaiveDateTime.to_date(site.native_stats_start_at),
title: title(conn, site),
demo: demo,
flags: get_flags(conn.assigns[:current_user], site),
flags: get_flags(current_user, site),
is_dbip: is_dbip(),
dogfood_page_path: dogfood_page_path,
load_dashboard_js: true
Expand Down Expand Up @@ -193,8 +198,8 @@ defmodule PlausibleWeb.StatsController do
defp csv_graph_metrics(query, site, current_user) do
include_scroll_depth? =
!query.include_imported &&
PlausibleWeb.Api.StatsController.scroll_depth_enabled?(site, current_user) &&
Filters.filtering_on_dimension?(query, "event:page", behavioral_filters: :ignore)
Filters.filtering_on_dimension?(query, "event:page", behavioral_filters: :ignore) &&
Plausible.Stats.ScrollDepth.feature_visible?(site, current_user)

{metrics, column_headers} =
if Filters.filtering_on_dimension?(query, "event:goal", max_depth: 0) do
Expand Down Expand Up @@ -341,9 +346,13 @@ defmodule PlausibleWeb.StatsController do
defp render_shared_link(conn, shared_link) do
cond do
!shared_link.site.locked ->
current_user = conn.assigns[:current_user]
shared_link = Plausible.Repo.preload(shared_link, site: :owner)
stats_start_date = Plausible.Sites.stats_start_date(shared_link.site)

scroll_depth_visible? =
Plausible.Stats.ScrollDepth.check_feature_visible!(shared_link.site, current_user)

conn
|> put_resp_header("x-robots-tag", "noindex, nofollow")
|> delete_resp_header("x-frame-options")
Expand All @@ -353,6 +362,7 @@ defmodule PlausibleWeb.StatsController do
revenue_goals: list_revenue_goals(shared_link.site),
funnels: list_funnels(shared_link.site),
has_props: Plausible.Props.configured?(shared_link.site),
scroll_depth_visible: scroll_depth_visible?,
stats_start_date: stats_start_date,
native_stats_start_date: NaiveDateTime.to_date(shared_link.site.native_stats_start_at),
title: title(conn, shared_link.site),
Expand All @@ -362,7 +372,7 @@ defmodule PlausibleWeb.StatsController do
embedded: conn.params["embed"] == "true",
background: conn.params["background"],
theme: conn.params["theme"],
flags: get_flags(conn.assigns[:current_user], shared_link.site),
flags: get_flags(current_user, shared_link.site),
is_dbip: is_dbip(),
load_dashboard_js: true
)
Expand Down
1 change: 1 addition & 0 deletions lib/plausible_web/templates/stats/stats.html.heex
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
data-revenue-goals={Jason.encode!(@revenue_goals)}
data-funnels={Jason.encode!(@funnels)}
data-has-props={to_string(@has_props)}
data-scroll-depth-visible={to_string(@scroll_depth_visible)}
data-logged-in={to_string([email protected][:current_user])}
data-stats-begin={@stats_start_date}
data-native-stats-begin={@native_stats_start_date}
Expand Down
14 changes: 8 additions & 6 deletions test/plausible/exports_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ defmodule Plausible.ExportsTest do
# for e2e export->import tests please see Plausible.Imported.CSVImporterTest

describe "export_queries/2" do
test "returns named ecto queries" do
queries = Plausible.Exports.export_queries(_site_id = 1, nil)
setup [:create_user, :create_site]

test "returns named ecto queries", %{site: site} do
queries = Plausible.Exports.export_queries(site.id, nil)
assert queries |> Map.values() |> Enum.all?(&match?(%Ecto.Query{}, &1))

assert Map.keys(queries) == [
Expand All @@ -24,9 +26,9 @@ defmodule Plausible.ExportsTest do
]
end

test "with date range" do
test "with date range", %{site: site} do
queries =
Plausible.Exports.export_queries(_site_id = 1, nil,
Plausible.Exports.export_queries(site.id, nil,
date_range: Date.range(~D[2023-01-01], ~D[2024-03-12])
)

Expand All @@ -44,8 +46,8 @@ defmodule Plausible.ExportsTest do
]
end

test "with custom extension" do
queries = Plausible.Exports.export_queries(_site_id = 1, nil, extname: ".ch")
test "with custom extension", %{site: site} do
queries = Plausible.Exports.export_queries(site.id, nil, extname: ".ch")

assert Map.keys(queries) == [
"imported_browsers.ch",
Expand Down
Loading
Loading