diff --git a/lib/plausible/imported.ex b/lib/plausible/imported.ex index 7a67aed61656..3497bfc43996 100644 --- a/lib/plausible/imported.ex +++ b/lib/plausible/imported.ex @@ -66,22 +66,24 @@ defmodule Plausible.Imported do @goals_with_path end - @spec any_completed_imports?(Site.t()) :: boolean() - def any_completed_imports?(site) do - get_completed_imports(site) != [] + @spec completed_imports(Site.t()) :: [SiteImport.t()] + def completed_imports(site) do + site + |> Repo.preload(:completed_imports) + |> Map.fetch!(:completed_imports) end @spec earliest_import_start_date(Site.t()) :: Date.t() | nil def earliest_import_start_date(site) do site - |> get_completed_imports() + |> completed_imports() |> Enum.map(& &1.start_date) |> Enum.min(Date, fn -> nil end) end @spec complete_import_ids(Site.t()) :: [non_neg_integer()] def complete_import_ids(site) do - imports = get_completed_imports(site) + imports = completed_imports(site) has_legacy? = Enum.any?(imports, fn %{legacy: legacy?} -> legacy? end) ids = Enum.map(imports, fn %{id: id} -> id end) @@ -93,12 +95,11 @@ defmodule Plausible.Imported do end end - @spec completed_imports_in_query_range(Site.t(), Query.t()) :: [SiteImport.t()] - def completed_imports_in_query_range(%Site{} = site, %Query{} = query) do + @spec completed_imports_in_query_range(Query.t()) :: [SiteImport.t()] + def completed_imports_in_query_range(%Query{} = query) do date_range = Query.date_range(query) - site - |> get_completed_imports() + query.completed_imports |> Enum.reject(fn site_import -> Date.after?(site_import.start_date, date_range.last) or Date.before?(site_import.end_date, date_range.first) @@ -218,10 +219,4 @@ defmodule Plausible.Imported do defp in_range?(date, range) do Date.before?(range.first, date) && Date.after?(range.last, date) end - - defp get_completed_imports(site) do - site - |> Repo.preload(:completed_imports) - |> Map.fetch!(:completed_imports) - end end diff --git a/lib/plausible/stats/aggregate.ex b/lib/plausible/stats/aggregate.ex index de4a9ba3d355..0d64f4592f93 100644 --- a/lib/plausible/stats/aggregate.ex +++ b/lib/plausible/stats/aggregate.ex @@ -14,6 +14,7 @@ defmodule Plausible.Stats.Aggregate do query = query |> Query.set(metrics: metrics, remove_unavailable_revenue_metrics: true) + |> Query.set_include(:dashboard_imports_meta, true) |> QueryOptimizer.optimize() %QueryResult{results: [entry], meta: meta} = QueryRunner.run(site, query) diff --git a/lib/plausible/stats/filters/query_parser.ex b/lib/plausible/stats/filters/query_parser.ex index 38fd78fbe570..db67e4a3aff3 100644 --- a/lib/plausible/stats/filters/query_parser.ex +++ b/lib/plausible/stats/filters/query_parser.ex @@ -11,7 +11,7 @@ defmodule Plausible.Stats.Filters.QueryParser do # is false. Even if we don't want to include imported data, we # might still want to know whether imported data can be toggled # on/off on the dashboard. - imports_meta: false, + dashboard_imports_meta: false, time_labels: false, total_rows: false, comparisons: nil diff --git a/lib/plausible/stats/query.ex b/lib/plausible/stats/query.ex index f396cf6388a0..36062e07b8c3 100644 --- a/lib/plausible/stats/query.ex +++ b/lib/plausible/stats/query.ex @@ -2,13 +2,12 @@ defmodule Plausible.Stats.Query do use Plausible defstruct utc_time_range: nil, - comparison_utc_time_range: nil, interval: nil, period: nil, dimensions: [], filters: [], sample_threshold: 20_000_000, - imports_exist: false, + completed_imports: [], imports_in_range: [], include_imported: false, skip_imported_reason: nil, @@ -26,7 +25,9 @@ defmodule Plausible.Stats.Query do revenue_warning: nil, remove_unavailable_revenue_metrics: false, site_id: nil, - site_native_stats_start_at: nil + site_native_stats_start_at: nil, + comparison_utc_time_range: nil, + comparison_query: nil require OpenTelemetry.Tracer, as: Tracer alias Plausible.Stats.{DateTimeRange, Filters, Imported, Legacy, Comparisons} @@ -134,50 +135,42 @@ defmodule Plausible.Stats.Query do end def put_imported_opts(query, site) do - requested? = query.include.imports - - query = - if site do - struct!(query, - imports_exist: Plausible.Imported.any_completed_imports?(site), - imports_in_range: get_imports_in_range(site, query) - ) - else - query - end - - skip_imported_reason = get_skip_imported_reason(query) - - struct!(query, - include_imported: requested? and is_nil(skip_imported_reason), - skip_imported_reason: skip_imported_reason + query + |> struct!( + completed_imports: + if(site, do: Plausible.Imported.completed_imports(site), else: query.completed_imports) ) + |> set_imports_in_range() + |> set_imports_included() end - defp get_imports_in_range(_site, %__MODULE__{period: period}) + defp set_imports_in_range(%__MODULE__{period: period} = query) when period in ["realtime", "30m"] do - [] + query end - defp get_imports_in_range(site, query) do - in_range = Plausible.Imported.completed_imports_in_query_range(site, query) + defp set_imports_in_range(query) do + struct!(query, + imports_in_range: Plausible.Imported.completed_imports_in_query_range(query) + ) + end - in_comparison_range = - if is_map(query.include.comparisons) do - comparison_query = Comparisons.get_comparison_query(query) - Plausible.Imported.completed_imports_in_query_range(site, comparison_query) - else - [] - end + defp set_imports_included(query) do + requested? = query.include.imports - in_comparison_range ++ in_range + skip_imported_reason = get_skip_imported_reason(query) + + struct!(query, + include_imported: requested? and is_nil(skip_imported_reason), + skip_imported_reason: skip_imported_reason + ) end @spec get_skip_imported_reason(t()) :: nil | :no_imported_data | :out_of_range | :unsupported_query def get_skip_imported_reason(query) do cond do - not query.imports_exist -> + query.completed_imports == [] -> :no_imported_data query.imports_in_range == [] -> diff --git a/lib/plausible/stats/query_optimizer.ex b/lib/plausible/stats/query_optimizer.ex index e3d8798d3026..d799d8406508 100644 --- a/lib/plausible/stats/query_optimizer.ex +++ b/lib/plausible/stats/query_optimizer.ex @@ -4,7 +4,7 @@ defmodule Plausible.Stats.QueryOptimizer do """ use Plausible - alias Plausible.Stats.{DateTimeRange, Filters, Query, TableDecider, Util, Time} + alias Plausible.Stats.{Comparisons, DateTimeRange, Filters, Query, TableDecider, Util, Time} @doc """ This module manipulates an existing query, updating it according to business logic. @@ -49,7 +49,8 @@ defmodule Plausible.Stats.QueryOptimizer do &add_missing_order_by/1, &update_time_in_order_by/1, &extend_hostname_filters_to_visit/1, - &remove_revenue_metrics_if_unavailable/1 + &remove_revenue_metrics_if_unavailable/1, + &add_comparison_query/1 ] end @@ -185,4 +186,13 @@ defmodule Plausible.Stats.QueryOptimizer do else defp remove_revenue_metrics_if_unavailable(query), do: query end + + defp add_comparison_query(query) do + if query.include.comparisons do + comparison_query = Comparisons.get_comparison_query(query) + Query.set(query, comparison_query: comparison_query) + else + query + end + end end diff --git a/lib/plausible/stats/query_result.ex b/lib/plausible/stats/query_result.ex index 1f9603142c74..0b1aa33b69d3 100644 --- a/lib/plausible/stats/query_result.ex +++ b/lib/plausible/stats/query_result.ex @@ -58,11 +58,24 @@ defmodule Plausible.Stats.QueryResult do } defp add_imports_meta(meta, %Query{include: include} = query) do - if include.imports or include.imports_meta do + if include.imports or include.dashboard_imports_meta do + comparison_query = + query.comparison_query || %{include_imported: false, skip_imported_reason: nil} + + imports_included = query.include_imported or comparison_query.include_imported + + imports_skip_reason = + if(imports_included, + do: nil, + else: query.skip_imported_reason || comparison_query.skip_imported_reason + ) + %{ - imports_included: query.include_imported, - imports_skip_reason: query.skip_imported_reason, - imports_warning: @imports_warnings[query.skip_imported_reason] + imports_included: imports_included, + imports_skip_reason: imports_skip_reason, + imports_warning: @imports_warnings[imports_skip_reason], + imports_included_for_main_query: + if(include.dashboard_imports_meta, do: query.include_imported, else: nil) } |> Map.reject(fn {_key, value} -> is_nil(value) end) |> Map.merge(meta) @@ -72,7 +85,7 @@ defmodule Plausible.Stats.QueryResult do end defp add_metric_warnings_meta(meta, query) do - warnings = metric_warnings(query) + warnings = metric_warnings(meta, query) if map_size(warnings) > 0 do Map.put(meta, :metric_warnings, warnings) @@ -111,9 +124,9 @@ defmodule Plausible.Stats.QueryResult do end end - defp metric_warnings(%Query{} = query) do + defp metric_warnings(meta, %Query{} = query) do Enum.reduce(query.metrics, %{}, fn metric, acc -> - case metric_warning(metric, query) do + case metric_warning(metric, meta, query) do nil -> acc %{} = warning -> Map.put(acc, metric, warning) end @@ -132,7 +145,7 @@ defmodule Plausible.Stats.QueryResult do "Revenue metrics are null as there are no matching revenue goals." } - defp metric_warning(metric, %Query{} = query) + defp metric_warning(metric, _meta, %Query{} = query) when metric in @revenue_metrics do if query.revenue_warning do %{ @@ -150,13 +163,17 @@ defmodule Plausible.Stats.QueryResult do warning: "No imports with scroll depth data were found" } - defp metric_warning(:scroll_depth, %Query{} = query) do - if query.include_imported and not Enum.any?(query.imports_in_range, & &1.has_scroll_depth) do - @no_imported_scroll_depth_metric_warning - end + defp metric_warning(:scroll_depth, %{imports_included: true}, %Query{} = query) do + [query, query.comparison_query] + |> Enum.reject(&is_nil/1) + |> Enum.find_value(fn query -> + if query.include_imported and not Enum.any?(query.imports_in_range, & &1.has_scroll_depth) do + @no_imported_scroll_depth_metric_warning + end + end) end - defp metric_warning(_metric, _query), do: nil + defp metric_warning(_metric, _meta, _query), do: nil defp to_iso8601(datetime, timezone) do datetime diff --git a/lib/plausible/stats/query_runner.ex b/lib/plausible/stats/query_runner.ex index ed51a96dd165..1f675dd6468e 100644 --- a/lib/plausible/stats/query_runner.ex +++ b/lib/plausible/stats/query_runner.ex @@ -61,8 +61,7 @@ defmodule Plausible.Stats.QueryRunner do defp add_comparison_query(%__MODULE__{main_query: query, main_results: main_results} = runner) when is_map(query.include.comparisons) do comparison_query = - query - |> Comparisons.get_comparison_query() + query.comparison_query |> Comparisons.add_comparison_filters(main_results) struct!(runner, comparison_query: comparison_query) diff --git a/lib/plausible/stats/timeseries.ex b/lib/plausible/stats/timeseries.ex index 5a01be033421..e217afac7c12 100644 --- a/lib/plausible/stats/timeseries.ex +++ b/lib/plausible/stats/timeseries.ex @@ -6,7 +6,7 @@ defmodule Plausible.Stats.Timeseries do """ use Plausible.ClickhouseRepo - alias Plausible.Stats.{Comparisons, Query, QueryRunner, QueryOptimizer, Time} + alias Plausible.Stats.{Query, QueryRunner, QueryOptimizer, Time} @time_dimension %{ "month" => "time:month", @@ -27,17 +27,11 @@ defmodule Plausible.Stats.Timeseries do ) |> QueryOptimizer.optimize() - comparison_query = - if(query.include.comparisons, - do: Comparisons.get_comparison_query(query), - else: nil - ) - query_result = QueryRunner.run(site, query) { build_result(query_result, query, fn entry -> entry end), - build_result(query_result, comparison_query, fn entry -> entry.comparison end), + build_result(query_result, query.comparison_query, fn entry -> entry.comparison end), query_result.meta } end diff --git a/lib/plausible_web/controllers/api/stats_controller.ex b/lib/plausible_web/controllers/api/stats_controller.ex index 1399d07f0e6c..9cef59a6dae8 100644 --- a/lib/plausible_web/controllers/api/stats_controller.ex +++ b/lib/plausible_web/controllers/api/stats_controller.ex @@ -192,10 +192,7 @@ defmodule PlausibleWeb.Api.StatsController do params = realtime_period_to_30m(params) - query = - site - |> Query.from(params, debug_metadata(conn)) - |> Query.set_include(:imports_meta, true) + query = Query.from(site, params, debug_metadata(conn)) %{ top_stats: top_stats, @@ -220,18 +217,19 @@ defmodule PlausibleWeb.Api.StatsController do end defp with_imported_switch_info(%Jason.OrderedObject{} = meta) do - case {meta[:imports_included], meta[:imports_skip_reason]} do - {true, nil} -> - %{visible: true, togglable: true, tooltip_msg: "Click to exclude imported data"} - - {false, nil} -> - %{visible: true, togglable: true, tooltip_msg: "Click to include imported data"} - - {false, :unsupported_query} -> + case Map.new(meta) do + %{imports_included: false, imports_skip_reason: :unsupported_query} -> %{visible: true, togglable: false, tooltip_msg: "Imported data cannot be included"} - {false, reason} when reason in [:no_imported_data, :out_of_range] -> + %{imports_included: false, imports_skip_reason: reason} + when reason in [:no_imported_data, :out_of_range] -> %{visible: false, togglable: false, tooltip_msg: nil} + + %{imports_included_for_main_query: true} -> + %{visible: true, togglable: true, tooltip_msg: "Click to exclude imported data"} + + %{imports_included_for_main_query: false} -> + %{visible: true, togglable: true, tooltip_msg: "Click to include imported data"} end end diff --git a/test/plausible/imported_test.exs b/test/plausible/imported_test.exs index 1c734b81815b..310f81d279c4 100644 --- a/test/plausible/imported_test.exs +++ b/test/plausible/imported_test.exs @@ -75,7 +75,7 @@ defmodule Plausible.ImportedTest do end end - describe "completed_imports_in_query_range/2" do + describe "completed_imports_in_query_range/1" do setup do site = insert(:site) @@ -85,11 +85,19 @@ defmodule Plausible.ImportedTest do site_import_apr = insert(:site_import, site: site, start_date: ~D[2021-04-10], end_date: ~D[2021-04-20]) - {:ok, %{site: site, site_import_feb: site_import_feb, site_import_apr: site_import_apr}} + site_import_old = + insert(:site_import, site: site, start_date: ~D[2020-01-01], end_date: ~D[2020-02-01]) + + {:ok, + %{ + site: site, + site_import_feb: site_import_feb, + site_import_apr: site_import_apr, + site_import_old: site_import_old + }} end test "returns empty list if no imports exist" do - site = insert(:site) tz = "Etc/UTC" query = %Query{ @@ -97,29 +105,28 @@ defmodule Plausible.ImportedTest do timezone: tz } - assert Imported.completed_imports_in_query_range(site, query) == [] + assert Imported.completed_imports_in_query_range(query) == [] end test "returns imports in query range", %{ - site: site, site_import_feb: site_import_feb, - site_import_apr: site_import_apr + site_import_apr: site_import_apr, + site_import_old: site_import_old } do tz = "Etc/UTC" query = %Query{ utc_time_range: DateTimeRange.new!(~D[2021-01-01], ~D[2021-12-31], tz), - timezone: tz + timezone: tz, + completed_imports: [site_import_feb, site_import_apr, site_import_old] } - imports_in_range = Imported.completed_imports_in_query_range(site, query) + imports_in_range = Imported.completed_imports_in_query_range(query) - assert Enum.find(imports_in_range, &(&1.id == site_import_feb.id)) - assert Enum.find(imports_in_range, &(&1.id == site_import_apr.id)) + assert Enum.sort(imports_in_range) == Enum.sort([site_import_feb, site_import_apr]) end test "returns imports in a non-utc timezone query range", %{ - site: site, site_import_feb: site_import_feb } do datetime_from = DateTime.new!(~D[2021-03-01], ~T[03:00:00], "Etc/UTC") @@ -127,10 +134,11 @@ defmodule Plausible.ImportedTest do query = %Query{ utc_time_range: DateTimeRange.new!(datetime_from, datetime_to), - timezone: "America/Chicago" + timezone: "America/Chicago", + completed_imports: [site_import_feb] } - [site_import] = Imported.completed_imports_in_query_range(site, query) + [site_import] = Imported.completed_imports_in_query_range(query) assert site_import.id == site_import_feb.id end diff --git a/test/plausible/stats/query_parser_test.exs b/test/plausible/stats/query_parser_test.exs index a523dc7e8708..80234d8815c5 100644 --- a/test/plausible/stats/query_parser_test.exs +++ b/test/plausible/stats/query_parser_test.exs @@ -50,7 +50,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do @default_include %{ imports: false, - imports_meta: false, + dashboard_imports_meta: false, time_labels: false, total_rows: false, comparisons: nil @@ -872,7 +872,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do timezone: site.timezone, include: %{ imports: true, - imports_meta: false, + dashboard_imports_meta: false, time_labels: true, total_rows: true, comparisons: nil @@ -937,7 +937,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do mode: "previous_period" }, imports: false, - imports_meta: false, + dashboard_imports_meta: false, time_labels: false, total_rows: false }, @@ -968,7 +968,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do mode: "year_over_year" }, imports: false, - imports_meta: false, + dashboard_imports_meta: false, time_labels: false, total_rows: false }, @@ -1001,7 +1001,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do mode: "custom", date_range: @date_range_30d }, - imports_meta: false, + dashboard_imports_meta: false, imports: false, time_labels: false, total_rows: false diff --git a/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs b/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs index 7075108e2c96..1edcfca55b6b 100644 --- a/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs +++ b/test/plausible_web/controllers/api/stats_controller/top_stats_test.exs @@ -791,7 +791,7 @@ defmodule PlausibleWeb.Api.StatsController.TopStatsTest do assert res["with_imported_switch"] == %{ "visible" => true, "togglable" => true, - "tooltip_msg" => "Click to exclude imported data" + "tooltip_msg" => "Click to include imported data" } end @@ -803,9 +803,9 @@ defmodule PlausibleWeb.Api.StatsController.TopStatsTest do res = json_response(conn, 200) assert res["with_imported_switch"] == %{ - "visible" => true, - "togglable" => true, - "tooltip_msg" => "Click to include imported data" + "visible" => false, + "togglable" => false, + "tooltip_msg" => nil } end end