Scroll depth dashboard warnings (imported data) (#5051)

* add migration

* add schema field

* mark site_imports with has_scroll_depth

* add function to get imports in query range

* add scroll_depth metric warning in QueryResult

* return scroll_depth warning in top stats

* render minimalistic warning in top stats

* minimalistic warning in Top Pages breakdown

* prettier format

* silence credo

* add test

* use a snapshot of SiteImport schema in data migration

* also use a snapshot list of imported_* tables

* moduledoc (credo)

* change tooltip message

* change metric warnings structure in top stats response

* pass meta from queryresult directly

* revert top_stats_entry refactor

* prettier

* stop using SiteImport module in data migration

Author: RobertJoonas

assets/js/dashboard/components/sort-button.tsx

@@ -17,11 +17,11 @@ export const SortButton = ({
   return (
     <button
       onClick={toggleSort}
-      title={next.hint}
       className={classNames('group', 'hover:underline', 'relative')}
     >
       {children}
       <span
+        title={next.hint}
         className={classNames(
           'absolute',
           'rounded inline-block h-4 w-4',

assets/js/dashboard/components/table.tsx

@@ -4,6 +4,7 @@ import classNames from 'classnames'
 import React, { ReactNode } from 'react'
 import { SortDirection } from '../hooks/use-order-by'
 import { SortButton } from './sort-button'
+import { Tooltip } from '../util/tooltip'
 
 export type ColumnConfiguraton<T extends Record<string, unknown>> = {
   /** Unique column ID, used for sorting purposes and to get the value of the cell using rowItem[key] */
@@ -17,6 +18,8 @@ export type ColumnConfiguraton<T extends Record<string, unknown>> = {
   width: string
   /** Aligns column content. */
   align?: 'left' | 'right'
+  /** A warning to be rendered as a tooltip for the column header */
+  metricWarning?: string
   /**
    * Function used to transform the value found at item[key] for the cell. Superseded by renderItem if present. @example 1120 => "1.1k"
    */
@@ -100,6 +103,29 @@ export const Table = <T extends Record<string, string | number | ReactNode>>({
   columns: ColumnConfiguraton<T>[]
   data: T[] | { pages: T[][] }
 }) => {
+  const renderColumnLabel = (column: ColumnConfiguraton<T>) => {
+    if (column.metricWarning) {
+      return (
+        <Tooltip
+          info={warningSpan(column.metricWarning)}
+          className="inline-block"
+        >
+          {column.label + ' *'}
+        </Tooltip>
+      )
+    } else {
+      return column.label
+    }
+  }
+
+  const warningSpan = (warning: string) => {
+    return (
+      <span className="text-xs font-normal whitespace-nowrap">
+        {'*' + warning}
+      </span>
+    )
+  }
+
   return (
     <table className="w-max overflow-x-auto md:w-full table-striped table-fixed">
       <thead>
@@ -115,10 +141,10 @@ export const Table = <T extends Record<string, string | number | ReactNode>>({
                   toggleSort={column.onSort}
                   sortDirection={column.sortDirection ?? null}
                 >
-                  {column.label}
+                  {renderColumnLabel(column)}
                 </SortButton>
               ) : (
-                column.label
+                renderColumnLabel(column)
               )}
             </TableHeaderCell>
           ))}

assets/js/dashboard/query.ts

@@ -52,6 +52,7 @@ export type DashboardQuery = typeof queryDefaultValue
 export type BreakdownResultMeta = {
   date_range_label: string
   comparison_date_range_label?: string
+  metric_warnings: Record<string, Record<string, string>> | undefined
 }
 
 export function addFilter(

assets/js/dashboard/stats/graph/top-stats.js

@@ -64,6 +64,14 @@ export default function TopStats({ data, onMetricUpdate, tooltipBoundary }) {
             <SecondsSinceLastLoad lastLoadTimestamp={lastLoadTimestamp} />s ago
           </p>
         )}
+
+        {stat.name === 'Scroll depth' &&
+          data.meta.metric_warnings?.scroll_depth?.code ===
+            'no_imported_scroll_depth' && (
+            <p className="font-normal text-xs whitespace-nowrap">
+              * Does not include imported data
+            </p>
+          )}
       </div>
     )
   }
@@ -115,6 +123,7 @@ export default function TopStats({ data, onMetricUpdate, tooltipBoundary }) {
         {statExtraName && (
           <span className="hidden sm:inline-block ml-1">{statExtraName}</span>
         )}
+        {stat.warning_code && <span className="inline-block ml-1">*</span>}
       </div>
     )
   }

assets/js/dashboard/stats/modals/breakdown-modal.tsx

@@ -152,6 +152,7 @@ export default function BreakdownModal<TListItem extends { name: string }>({
           key: m.key,
           width: m.width,
           align: 'right',
+          metricWarning: getMetricWarning(m, meta),
           renderValue: (item) => m.renderValue(item, meta),
           onSort: m.sortable ? () => toggleSortByMetric(m) : undefined,
           sortDirection: orderByDictionary[m.key]
@@ -233,3 +234,15 @@ const ExternalLinkIcon = ({ url }: { url?: string }) =>
       </svg>
     </a>
   ) : null
+
+const getMetricWarning = (metric: Metric, meta: BreakdownResultMeta | null) => {
+  const warnings = meta?.metric_warnings
+
+  if (warnings) {
+    const code = warnings[metric.key]?.code
+
+    if (code == 'no_imported_scroll_depth') {
+      return 'Does not include imported data'
+    }
+  }
+}

lib/plausible/data_migration/site_imports.ex

@@ -9,20 +9,51 @@ defmodule Plausible.DataMigration.SiteImports do
 
   import Ecto.Query
 
-  alias Plausible.ClickhouseRepo
-  alias Plausible.Imported
-  alias Plausible.Imported.SiteImport
-  alias Plausible.Repo
-  alias Plausible.Site
+  alias Plausible.{Repo, ClickhouseRepo, Site}
 
-  require Plausible.Imported.SiteImport
+  defmodule SiteImportSnapshot do
+    @moduledoc """
+    A snapshot of the Plausible.Imported.SiteImport schema from April 2024.
+    """
+
+    use Ecto.Schema
+
+    schema "site_imports" do
+      field :start_date, :date
+      field :end_date, :date
+      field :label, :string
+      field :source, Ecto.Enum, values: [:universal_analytics, :google_analytics_4, :csv, :noop]
+      field :status, Ecto.Enum, values: [:pending, :importing, :completed, :failed]
+      field :legacy, :boolean, default: false
+
+      belongs_to :site, Plausible.Site
+      belongs_to :imported_by, Plausible.Auth.User
+
+      timestamps()
+    end
+  end
+
+  @imported_tables_april_2024 [
+    "imported_visitors",
+    "imported_sources",
+    "imported_pages",
+    "imported_entry_pages",
+    "imported_exit_pages",
+    "imported_custom_events",
+    "imported_locations",
+    "imported_devices",
+    "imported_browsers",
+    "imported_operating_systems"
+  ]
+
+  def imported_tables_april_2024(), do: @imported_tables_april_2024
 
   def run(opts \\ []) do
     dry_run? = Keyword.get(opts, :dry_run?, true)
 
     site_import_query =
-      from(i in Imported.SiteImport,
-        where: i.site_id == parent_as(:site).id and i.status == ^SiteImport.completed(),
+      from(i in SiteImportSnapshot,
+        where: i.site_id == parent_as(:site).id and i.status == ^:completed,
         select: 1
       )
 
@@ -37,7 +68,7 @@ defmodule Plausible.DataMigration.SiteImports do
       |> Repo.all(log: false)
 
     site_imports =
-      from(i in Imported.SiteImport, where: i.status == ^SiteImport.completed())
+      from(i in SiteImportSnapshot, where: i.status == ^:completed)
       |> Repo.all(log: false)
 
     legacy_site_imports = backfill_legacy_site_imports(sites_with_only_legacy_import, dry_run?)
@@ -64,7 +95,7 @@ defmodule Plausible.DataMigration.SiteImports do
           |> Map.put(:site_id, site.id)
           |> Map.take([:legacy, :start_date, :end_date, :source, :status, :site_id])
 
-        %Imported.SiteImport{}
+        %SiteImportSnapshot{}
         |> Ecto.Changeset.change(params)
         |> insert!(dry_run?)
       end
@@ -146,11 +177,11 @@ defmodule Plausible.DataMigration.SiteImports do
   # Exposed for testing purposes
   @doc false
   def imported_stats_end_date(site_id, import_ids) do
-    [first_schema | schemas] = Imported.schemas()
+    [first_table | tables] = @imported_tables_april_2024
 
     query =
-      Enum.reduce(schemas, max_date_query(first_schema, site_id, import_ids), fn schema, query ->
-        from(s in subquery(union_all(query, ^max_date_query(schema, site_id, import_ids))))
+      Enum.reduce(tables, max_date_query(first_table, site_id, import_ids), fn table, query ->
+        from(s in subquery(union_all(query, ^max_date_query(table, site_id, import_ids))))
       end)
 
     dates = ClickhouseRepo.all(from(q in query, select: q.max_date), log: false)
@@ -211,8 +242,8 @@ defmodule Plausible.DataMigration.SiteImports do
     entity
   end
 
-  defp max_date_query(schema, site_id, import_ids) do
-    from(q in schema,
+  defp max_date_query(table, site_id, import_ids) do
+    from(q in table,
       where: q.site_id == ^site_id,
       where: q.import_id in ^import_ids,
       select: %{max_date: fragment("max(?)", q.date)}
@@ -222,12 +253,12 @@ defmodule Plausible.DataMigration.SiteImports do
   defp from_legacy(%Site.ImportedData{} = data) do
     status =
       case data.status do
-        "ok" -> SiteImport.completed()
-        "error" -> SiteImport.failed()
-        _ -> SiteImport.importing()
+        "ok" -> :completed
+        "error" -> :failed
+        _ -> :importing
       end
 
-    %SiteImport{
+    %SiteImportSnapshot{
       id: 0,
       legacy: true,
       start_date: data.start_date,

lib/plausible/imported.ex

@@ -9,10 +9,9 @@ defmodule Plausible.Imported do
 
   import Ecto.Query
 
-  alias Plausible.Imported
+  alias Plausible.{Site, Repo, Imported}
   alias Plausible.Imported.SiteImport
-  alias Plausible.Repo
-  alias Plausible.Site
+  alias Plausible.Stats.Query
 
   require Plausible.Imported.SiteImport
 
@@ -97,6 +96,17 @@ 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
+    date_range = Query.date_range(query)
+
+    site
+    |> get_completed_imports()
+    |> Enum.filter(fn site_import ->
+      site_import.start_date <= date_range.last && site_import.end_date >= date_range.first
+    end)
+  end
+
   @spec get_import(Site.t(), non_neg_integer()) :: SiteImport.t() | nil
   def get_import(site, import_id) do
     Repo.get_by(SiteImport, id: import_id, site_id: site.id)

lib/plausible/imported/csv_importer.ex

@@ -5,6 +5,7 @@ defmodule Plausible.Imported.CSVImporter do
   """
 
   use Plausible.Imported.Importer
+  import Ecto.Query, only: [from: 2]
 
   @impl true
   def name(), do: :csv
@@ -54,6 +55,26 @@ defmodule Plausible.Imported.CSVImporter do
       {:error, Exception.message(e)}
   end
 
+  def on_success(site_import, _extra_data) do
+    has_scroll_depth? =
+      Plausible.ClickhouseRepo.exists?(
+        from(i in "imported_pages",
+          where: i.site_id == ^site_import.site_id,
+          where: i.import_id == ^site_import.id,
+          where: not is_nil(i.scroll_depth),
+          select: 1
+        )
+      )
+
+    if has_scroll_depth? do
+      site_import
+      |> Ecto.Changeset.change(%{has_scroll_depth: true})
+      |> Plausible.Repo.update!()
+    end
+
+    :ok
+  end
+
   defp import_s3(ch, site_import, uploads) do
     %{
       id: import_id,

lib/plausible/imported/site_import.ex

@@ -22,6 +22,7 @@ defmodule Plausible.Imported.SiteImport do
     field :source, Ecto.Enum, values: ImportSources.names()
     field :status, Ecto.Enum, values: @statuses
     field :legacy, :boolean, default: false
+    field :has_scroll_depth, :boolean, default: false
 
     belongs_to :site, Site
     belongs_to :imported_by, User

lib/plausible/stats/query_result.ex

@@ -9,6 +9,7 @@ defmodule Plausible.Stats.QueryResult do
 
   use Plausible
   alias Plausible.Stats.{DateTimeRange, Query, QueryRunner}
+  alias Plausible.Imported
 
   defstruct results: [],
             meta: %{},
@@ -89,8 +90,8 @@ defmodule Plausible.Stats.QueryResult do
     end
   end
 
-  defp add_metric_warnings_meta(meta, %QueryRunner{main_query: query}) do
-    warnings = metric_warnings(query)
+  defp add_metric_warnings_meta(meta, runner) do
+    warnings = metric_warnings(meta, runner)
 
     if map_size(warnings) > 0 do
       Map.put(meta, :metric_warnings, warnings)
@@ -129,9 +130,9 @@ defmodule Plausible.Stats.QueryResult do
     end
   end
 
-  defp metric_warnings(query) do
+  defp metric_warnings(meta, %QueryRunner{main_query: query} = runner) do
     Enum.reduce(query.metrics, %{}, fn metric, acc ->
-      case metric_warning(metric, query) do
+      case metric_warning(metric, meta, runner) do
         nil -> acc
         %{} = warning -> Map.put(acc, metric, warning)
       end
@@ -150,7 +151,8 @@ defmodule Plausible.Stats.QueryResult do
         "Revenue metrics are null as there are no matching revenue goals."
     }
 
-    defp metric_warning(metric, query) when metric in @revenue_metrics do
+    defp metric_warning(metric, _meta, %QueryRunner{main_query: query})
+         when metric in @revenue_metrics do
       if query.revenue_warning do
         %{
           code: query.revenue_warning,
@@ -162,7 +164,30 @@ defmodule Plausible.Stats.QueryResult do
     end
   end
 
-  defp metric_warning(_metric, _query), do: nil
+  @no_imported_scroll_depth_metric_warning %{
+    code: :no_imported_scroll_depth,
+    warning: "No imports with scroll depth data were found"
+  }
+
+  defp metric_warning(:scroll_depth, %{imports_included: true} = _meta, %QueryRunner{} = runner) do
+    %QueryRunner{site: site, main_query: main_query, comparison_query: comparison_query} = runner
+
+    imports_in_main_range = Imported.completed_imports_in_query_range(site, main_query)
+
+    imports_in_comparison_range =
+      case comparison_query do
+        nil -> []
+        %Query{} = query -> Imported.completed_imports_in_query_range(site, query)
+      end
+
+    imports_in_range = imports_in_main_range ++ imports_in_comparison_range
+
+    if not Enum.any?(imports_in_range, & &1.has_scroll_depth) do
+      @no_imported_scroll_depth_metric_warning
+    end
+  end
+
+  defp metric_warning(_metric, _meta, _query), do: nil
 
   defp to_iso8601(datetime, timezone) do
     datetime